Add a shared sources library with code to generate certificates for development #263

javiercn · 2017-09-21T20:38:46Z

No description provided.

natemcmaster · 2017-09-21T20:46:47Z

shared/Microsoft.Extensions.Certificates.Generation.Sources/CertificateManager.cs

+                        case CertificatePurpose.All:
+                            matchingCertificates = matchingCertificates
+                                .Where(c => HasOid(c, AspNetHttpsOid) || HasOid(c, AspNetIdentityOid))
+                                .ToList();


You can remove the extra .ToList() calls to avoid multiple iterations. The final ToList at the end of this method should be sufficient.

natemcmaster · 2017-09-21T20:51:23Z

shared/Microsoft.Extensions.Certificates.Generation.Sources/CertificateManager.cs

+            StoreLocation location,
+            bool isValid)
+        {
+            var certificates = new List<X509Certificate2>();


Unless I'm missing something, it seems like you could simplify by calling. Not sure why you need this list yet.

var certificates = store.Certificates.OfType<X509Certificate2>().ToList();

natemcmaster · 2017-09-21T20:53:39Z

shared/Microsoft.Extensions.Certificates.Generation.Sources/CertificateManager.cs

+                var bytes = certificate.Export(X509ContentType.Pkcs12, password);
+                try
+                {
+                    File.WriteAllBytes(path, bytes);


Consider adding Directory.CreateDirectory for the parent folder

natemcmaster · 2017-09-21T20:56:05Z

test/Microsoft.Extensions.Certificates.Generation.Tests/CertificateManagerTests.cs

+            const string CertificateName = nameof(EnsureCreateHttpsCertificate_CreatesACertificate_WhenThereAreNoHttpsCertificates) + ".cer";
+            var manager = new CertificateManager();
+
+            manager.RemoveAllCertificates(CertificatePurpose.HTTPS, StoreName.My, StoreLocation.CurrentUser);


😬 what if I have certs I want to keep? IMO it would be better to track certs you've made in the test run and ensure they are cleaned up at the end of the test.

Oh hell yes. Naughty.

natemcmaster · 2017-09-21T20:57:20Z

Test fail on linux and macos :-/

javiercn · 2017-09-21T21:55:58Z

shared/Microsoft.Extensions.Certificates.Generation.Sources/CertificateManager.cs

+            }
+        }
+
+        public X509Certificate2 CreateAspNetCoreHttpsDevelopmentCertificate(DateTimeOffset notBefore, DateTimeOffset notAfter)


@blowdart Can you review the crypto?

javiercn · 2017-09-21T21:56:11Z

shared/Microsoft.Extensions.Certificates.Generation.Sources/CertificateManager.cs

+        }
+
+        public X509Certificate2 CreateApplicationTokenSigningDevelopmentCertificate(DateTimeOffset notBefore, DateTimeOffset notAfter)
+        {


@blowdart Can you review the crypto?

blowdart · 2017-09-21T21:57:22Z

Adding Levi, because HAHA :D

blowdart · 2017-09-21T22:00:33Z

shared/Microsoft.Extensions.Certificates.Generation.Sources/CertificateManager.cs

+                    }
+                    if (isValid)
+                    {
+                        // Ensure the certificate hasn't expired, has a private key and its exportable


Why check for exportable? What if someone doesn't want autogenerated and creates their own non-exportable key?

Non exportable certificates can't be used by docker

blowdart · 2017-09-21T22:02:47Z

shared/Microsoft.Extensions.Certificates.Generation.Sources/CertificateManager.cs

+            return EnsureCertificate(notBefore, notAfter, CertificatePurpose.Signing, path, trust, includePrivateKey, password);
+        }
+
+        public EnsureCertificateResult EnsureCertificate(


Naming. EnsureCertificateWhat? IsValid?

blowdart · 2017-09-21T22:03:55Z

shared/Microsoft.Extensions.Certificates.Generation.Sources/CertificateManager.cs

+                    return EnsureCertificateResult.ErrorSavingTheCertificateIntoTheCurrentUserPersonalStore;
+                }
+            }
+            if (path != null)


Why is exporting the key a requirement for being valid?

We need to export the key into a PFX for container/unix scenarios, we don't want to take into account any certificate that doesn't cover all our scenarios (has not expired, has a private key and the key is exportable)

The idea is that tooling will run a single command/gesture and ensure there is a valid certificate (not expired, with an exportable private key) that can be used to https and token signing. For containers scenarios the certificate will be exported into a pfx file somewhere on disk and that folder will be mapped so that it can get pick up by kestrel

cesarblum · 2017-10-03T23:08:07Z

shared/Microsoft.Extensions.Certificates.Generation.Sources/CertificateManager.cs

+                        matchingCertificates = matchingCertificates
+                            .Where(c => c.NotBefore <= now &&
+                                now <= c.NotAfter &&
+                                ((c.GetRSAPrivateKey() is RSACryptoServiceProvider rsaPrivateKey &&


Can you add a flag to make the exportability check optional (and instead just check that the cert has a private key)?

halter73 · 2017-10-04T00:18:59Z

shared/Microsoft.Extensions.Certificates.Generation.Sources/CertificateManager.cs

+
+namespace Microsoft.Extensions.Certificates.Generation
+{
+    internal class CertificateManager


Why is this shared source instead of a traditional library?

We don't want to expose public API for this. It's a complete implementation detail

Tratcher · 2017-10-04T19:09:11Z

What repos / components will consume this? Or in other words, why is this a shared sources package?

javiercn · 2017-10-04T19:24:01Z

DotNetTools, Identity and Kestrel. Its a shared sources package because this is an implementation detail. We don't want to have public API or an actual package just for sharing a couple of methods but we want the crypto to be done in a single place.

halter73 · 2017-10-04T19:33:06Z

Do you expect Kestrel to populate the X509Store with a dev cert if one isn't there already?

javiercn · 2017-10-04T19:47:05Z

@halter73 Nope. Tooling does that. Kestrel should throw an exception if it can't find a valid certificate to use, with an error message pointing to what command line gesture to perform to create it and a forwarding link pointing to more detailed documentation.

javiercn · 2017-10-06T00:30:36Z

shared/Microsoft.Extensions.Certificates.Generation.Sources/CertificateManager.cs

+                                (!requireExportable || IsExportable(c)));
+                    }
+
+                    matchingCertificates = matchingCertificates.ToList();


We need to force the enumeration here or else disposing the certificates later causes issues.

javiercn · 2017-10-06T01:12:01Z

@natemcmaster Addressed feedback

natemcmaster

A couple more nits about namespaces and project layout, but otherwise the certificate management part looks fine. Almost ready to approve this.

natemcmaster · 2017-10-06T16:20:33Z

shared/Microsoft.Extensions.Certificates.Generation.Sources/CertificateManager.cs

+using System.Security.Cryptography.X509Certificates;
+using System.Text;
+
+namespace Microsoft.Extensions.Certificates.Generation


Nit: seems like this should be Microsoft.AspNetCore.Certificates.Generation given that all the constants below say "aspnetcore ...." in them.

It doesn't depend on any ASP.NET specific library, which I believe is the main driving factor for deciding between AspNetCore and Extensions.

Nate's right, this is an ASP.NET component because it is all about ASP.NET Certificates as shown by the constants below.

natemcmaster · 2017-10-06T16:20:51Z

shared/Microsoft.Extensions.Certificates.Generation.Sources/CertificateManager.cs

+    internal class CertificateManager
+    {
+        public const string AspNetHttpsOid = "1.3.6.1.4.1.311.84.1.1";
+        public const string AspNetHttpsOidFriendlyName = "Asp.Net Core HTTPS development certificate";


Nit: capitalization. "ASP.NET"

natemcmaster · 2017-10-06T16:21:54Z

shared/Microsoft.Extensions.Certificates.Generation.Sources/CertificateManager.cs

+
+        public void ExportCertificate(X509Certificate2 certificate, string path, bool includePrivateKey, string password)
+        {
+            if (Path.GetDirectoryName(path) != "")


Should this be an error if string.IsNullOrEmpty(Path.GetDirectoryName(path)) or !Path.IsRooted(path)? Seems like it would be better to require absolute paths, otherwise you get output relative to Directory.GetCurrentDirectory(), which changes based on how you invoke your process (IIS, IIS Express, command-line, VS, etc).

I'm going to put all those semantics on the actual exe we will build. I want to keep the library flexible

natemcmaster · 2017-10-06T16:26:21Z

Common.sln

@@ -52,6 +52,8 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "build", "build", "{D2D23857
 		build\repo.props = build\repo.props
 	EndProjectSection
 EndProject
+Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Microsoft.Extensions.Certificates.Generation.Tests", "test\Microsoft.Extensions.Certificates.Generation.Tests\Microsoft.Extensions.Certificates.Generation.Tests.csproj", "{02774467-AA73-4283-A46D-635F1FB0B90D}"


Nit: can you condense this into the existing test project, Microsoft.Extensions.Internal.Test? Creating an new test assembly for one test class seems like a overkill...plus it makes tests a lot slower as test assemblies can't run in parallel.

This is netcoreapp2.0 only, so its kind of painful to be moving it to the internal project. Build time is not a problem in common and I rather keep it simple

#if NETCOREAPP2_0

natemcmaster · 2017-10-06T20:22:14Z

Thanks for addressing the feedback. LGTM

…evelopment

See #263

dnfclas added the cla-already-signed label Sep 21, 2017

natemcmaster suggested changes Sep 21, 2017

View reviewed changes

javiercn requested a review from blowdart September 21, 2017 21:55

javiercn commented Sep 21, 2017

View reviewed changes

blowdart requested a review from GrabYourPitchforks September 21, 2017 21:57

blowdart reviewed Sep 21, 2017

View reviewed changes

cesarblum mentioned this pull request Oct 3, 2017

Add "zero config" HTTPS support using local development certificate. aspnet/KestrelHttpServer#2093

Merged

cesarblum reviewed Oct 3, 2017

View reviewed changes

halter73 reviewed Oct 4, 2017

View reviewed changes

javiercn force-pushed the certificates branch from 41db6c6 to 089c5f8 Compare October 6, 2017 00:24

javiercn commented Oct 6, 2017

View reviewed changes

javiercn force-pushed the certificates branch from 089c5f8 to da1d7f0 Compare October 6, 2017 00:37

natemcmaster reviewed Oct 6, 2017

View reviewed changes

natemcmaster approved these changes Oct 6, 2017

View reviewed changes

Add a shared sources library with code to generate certificates for d…

d7e26a0

…evelopment

javiercn force-pushed the certificates branch from d7c2064 to d7e26a0 Compare October 6, 2017 20:26

javiercn merged commit d7e26a0 into dev Oct 6, 2017

natemcmaster deleted the certificates branch October 25, 2018 00:25

natemcmaster pushed a commit that referenced this pull request Nov 5, 2018

Disable test (#265)

4f4c400

See #263

ghost locked as resolved and limited conversation to collaborators May 30, 2023

Add a shared sources library with code to generate certificates for development #263

Add a shared sources library with code to generate certificates for development #263

Uh oh!

Conversation

javiercn commented Sep 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natemcmaster commented Sep 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blowdart commented Sep 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tratcher commented Oct 4, 2017

Uh oh!

javiercn commented Oct 4, 2017

Uh oh!

halter73 commented Oct 4, 2017

Uh oh!

javiercn commented Oct 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javiercn commented Oct 6, 2017

Uh oh!

natemcmaster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natemcmaster commented Oct 6, 2017

Uh oh!

Uh oh!