Show fwlink on HTTPS certificate errors (#83) #94

cesarblum · 2017-05-02T22:22:35Z

muratg · 2017-05-02T22:29:05Z

src/Microsoft.AspNetCore/CertificateConfigurationException.cs

+    internal class CertificateConfigurationException : Exception
+    {
+        public CertificateConfigurationException(string message)
+            : base(message + ". For information on configuring HTTPS see https://go.microsoft.com/fwlink/?linkid=848054.")


Why not $"{message}. For infor...."?

Why is it adding a period to the previous message at all?

O I C. All the previous exception messages should be complete sentences (wherever possible) and end with periods... Though some of them I can see why they don't...

Actually I suggested fixes for all the exception messages below. Then remove the leading period on this one here.

muratg

Looks good (apart from string formatting nit below.)

after @danroth27 confirms the fwlink to be correct.

Eilon · 2017-05-02T23:17:21Z

src/Microsoft.AspNetCore/CertificateConfigurationException.cs

+    internal class CertificateConfigurationException : Exception
+    {
+        public CertificateConfigurationException(string message)
+            : base(message + ". For information on configuring HTTPS see https://go.microsoft.com/fwlink/?linkid=848054.")


Why is it adding a period to the previous message at all?

Eilon · 2017-05-02T23:18:03Z

src/Microsoft.AspNetCore/CertificateConfigurationException.cs

+    internal class CertificateConfigurationException : Exception
+    {
+        public CertificateConfigurationException(string message)
+            : base(message + ". For information on configuring HTTPS see https://go.microsoft.com/fwlink/?linkid=848054.")


O I C. All the previous exception messages should be complete sentences (wherever possible) and end with periods... Though some of them I can see why they don't...

Eilon · 2017-05-02T23:18:16Z

src/Microsoft.AspNetCore/CertificateLoader.cs

@@ -99,7 +99,7 @@ private X509Certificate2 LoadSingle(string certificateName)

            if (!certificateConfiguration.Exists())
            {
-                throw new InvalidOperationException($"No certificate named {certificateName} found in configuration");
+                throw new CertificateConfigurationException($"No certificate named {certificateName} found in configuration for the current environment");


This sentence needs to end with a period.

Eilon · 2017-05-02T23:18:38Z

src/Microsoft.AspNetCore/CertificateLoader.cs

@@ -119,7 +119,7 @@ private X509Certificate2 LoadSingle(IConfigurationSection certificateConfigurati
                    certificateSource = new CertificateStoreSource(_certificateStoreLoader);
                    break;
                default:
-                    throw new InvalidOperationException($"Invalid certificate source kind: {sourceKind}");
+                    throw new CertificateConfigurationException($"Invalid certificate source kind: {sourceKind}");


Change this to be a sentence: Invalid certificate source kind '{sourceKind}'.

Eilon · 2017-05-02T23:19:58Z

src/Microsoft.AspNetCore/CertificateLoader.cs

@@ -163,7 +163,7 @@ public override X509Certificate2 Load()

                if (error != null)
                {
-                    throw error;
+                    throw new CertificateConfigurationException($"Unable to load certificate from file '{Path}': {error.Message}");


Unable to load certificate from file '{Path}'. Error details: '{error.Message}'.

Eilon · 2017-05-02T23:20:21Z

src/Microsoft.AspNetCore/CertificateLoader.cs

@@ -203,7 +203,7 @@ public override X509Certificate2 Load()
            {
                if (!Enum.TryParse(StoreLocation, ignoreCase: true, result: out StoreLocation storeLocation))
                {
-                    throw new InvalidOperationException($"Invalid store location: {StoreLocation}");
+                    throw new CertificateConfigurationException($"Invalid certificate store location: {StoreLocation}");


The certificate store location '{StoreLocation}' is invalid.

Eilon · 2017-05-02T23:20:32Z

src/Microsoft.AspNetCore/KestrelServerOptionsSetup.cs

@@ -63,7 +63,7 @@ private void BindConfiguration(KestrelServerOptions options)

                    if (certificate == null)
                    {
-                        throw new InvalidOperationException($"Unable to load certificate for endpoint '{endPoint.Key}'");
+                        throw new CertificateConfigurationException($"Unable to load certificate for endpoint '{endPoint.Key}'");


End with period.

Eilon · 2017-05-02T23:20:57Z

src/Microsoft.AspNetCore/CertificateConfigurationException.cs

+    internal class CertificateConfigurationException : Exception
+    {
+        public CertificateConfigurationException(string message)
+            : base(message + ". For information on configuring HTTPS see https://go.microsoft.com/fwlink/?linkid=848054.")


Actually I suggested fixes for all the exception messages below. Then remove the leading period on this one here.

Eilon

Nice!

danroth27 · 2017-05-03T16:59:13Z

The CertificateLoader will be used for other scenarios than loading the HTTPS certificate. We're using it to load signing certificates as well. So I don't think the information about how to enable HTTPS is being added in the right context. I.e. you don't need instructions about setting up HTTPS if the configuration for your signing certificate is invalid. I think the HTTPS information needs to be added when setting up HTTPS with Kestrel.

Also, we should log a warning if the CertificateLoader try to find a certificate with a particular subject name in a store but doesn't find any.

danroth27

We should only add the message with information on setting up HTTPS in the context of setting up HTTPS. We need to be able to use the CertificateLoader for things other than the HTTPS certificate.

cesarblum · 2017-05-03T19:46:22Z

src/Microsoft.AspNetCore/CertificateLoader.cs

@@ -99,7 +125,7 @@ private X509Certificate2 LoadSingle(string certificateName)

            if (!certificateConfiguration.Exists())
            {
-                throw new InvalidOperationException($"No certificate named {certificateName} found in configuration");
+                throw new KeyNotFoundException($"No certificate named '{certificateName}' found in configuration for the current environment.");


@Tratcher Is it ok to throw this exception type here?

cesarblum · 2017-05-03T19:46:38Z

src/Microsoft.AspNetCore/CertificateConfigurationException.cs

+
+namespace Microsoft.AspNetCore
+{
+    internal class CertificateConfigurationException : Exception


@Tratcher Is it ok to keep this internal?

cesarblum · 2017-05-03T19:47:17Z

test/Microsoft.AspNetCore.FunctionalTests/CertificateLoaderTests.cs

+            Assert.Equal(0, loadedCertificates.Count());
+            Assert.Single(logger.LogMessages, logMessage =>
+                logMessage.LogLevel == LogLevel.Warning &&
+                logMessage.Message == "Unable to find a matching certificate for subject 'localhost' in store 'My' in 'CurrentUser'.");


@Eilon Is the language alright? Should log messages also end with a period?

cesarblum · 2017-05-03T19:51:09Z

src/Microsoft.AspNetCore/KestrelServerOptionsSetup.cs

@@ -27,7 +31,7 @@ public void Configure(KestrelServerOptions options)

        private void BindConfiguration(KestrelServerOptions options)
        {
-            var certificateLoader = new CertificateLoader(_configurationRoot.GetSection("Certificates"));
+            var certificateLoader = new CertificateLoader(_configurationRoot.GetSection("Certificates"), _loggerFactory);


@danroth27 I think we should put CertificateLoader in DI.

@davidfowl What do you think?

cesarblum requested review from Eilon, muratg and danroth27 May 2, 2017 22:22

dnfclas added the cla-already-signed label May 2, 2017

muratg reviewed May 2, 2017

View reviewed changes

muratg approved these changes May 2, 2017

View reviewed changes

Eilon suggested changes May 2, 2017

View reviewed changes

Eilon approved these changes May 3, 2017

View reviewed changes

danroth27 suggested changes May 3, 2017

View reviewed changes

cesarblum force-pushed the cesarbs/83 branch from 4a96092 to 50bacbc Compare May 3, 2017 19:41

cesarblum commented May 3, 2017

View reviewed changes

cesarblum force-pushed the cesarbs/83 branch from 8bf69c3 to ea2b1e3 Compare May 3, 2017 21:04

Cesar Blum Silveira added 8 commits May 3, 2017 14:56

Show fwlink on certificate errors (#83).

11a7c7d

Address feedback on error messages.

2efe346

Only show fwlink on HTTPS configuration errors.

ad74969

Preserve original exception on error when loading certificate from file.

1ac7cd7

Log warning when certificate is not found in store.

36cc8af

Change CertificateLoader internal ctor signature.

02dddaa

Remove CertificateConfigurationException.

6b1bb2b

Pass environment name to CertificateLoader.

ff8a42f

cesarblum force-pushed the cesarbs/83 branch from ea2b1e3 to ff8a42f Compare May 3, 2017 21:57

cesarblum changed the title ~~Show fwlink on certificate errors (#83)~~ Show fwlink on HTTPS certificate errors (#83) May 3, 2017

Update HTTPS endpoint error message.

e41f2db

danroth27 approved these changes May 3, 2017

View reviewed changes

Cesar Blum Silveira added 2 commits May 3, 2017 15:24

Minor tweak.

bafa211

Doc comment fix.

46fb42d

cesarblum merged commit 012d999 into rel/2.0.0-preview1 May 3, 2017

cesarblum deleted the cesarbs/83 branch May 3, 2017 22:55

Show fwlink on HTTPS certificate errors (#83) #94

Show fwlink on HTTPS certificate errors (#83) #94

Uh oh!

Conversation

cesarblum commented May 2, 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!

muratg 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!

Eilon left a comment

Choose a reason for hiding this comment

Uh oh!

danroth27 commented May 3, 2017

Uh oh!

danroth27 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!

Uh oh!