Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
b852683
macOS dev-certs changes
adityamandaleeka Jun 14, 2022
d7f6b51
Remove unnecessary trust checks for deletion.
adityamandaleeka Jun 16, 2022
2f6d1f2
Add specific return message for existing cert trusted.
adityamandaleeka Jun 16, 2022
765e1a7
Clean up unnecessary code.
adityamandaleeka Jun 17, 2022
22dd412
Throw on keychain add failure.
adityamandaleeka Jun 17, 2022
a9a1808
Fix handling of create+trust workflow.
adityamandaleeka Jun 17, 2022
df248a9
Feedback.
adityamandaleeka Jun 17, 2022
dc5e8cf
Add warning about changing exit codes.
adityamandaleeka Jun 18, 2022
0ff62c4
Handle torn state cases.
adityamandaleeka Jun 21, 2022
77b079c
Make --check also check for consistent state.
adityamandaleeka Jun 21, 2022
7a835ad
Rev certificate version.
adityamandaleeka Jun 22, 2022
6929323
Revert the changes around handling the old cert model.
adityamandaleeka Jun 22, 2022
b5f447e
Update OID for dev cert.
adityamandaleeka Jun 22, 2022
d511850
Make OID change on macOS only.
adityamandaleeka Jun 23, 2022
42e9f11
Revert version change that's no longer needed.
adityamandaleeka Jun 23, 2022
a96c046
Show warning on https, trust, and check commands.
adityamandaleeka Jun 23, 2022
5c54334
Add aka.ms link to explainer doc for cert change.
adityamandaleeka Jun 24, 2022
58cdbae
Oops.
adityamandaleeka Jun 24, 2022
14a0f26
Feedback.
adityamandaleeka Jun 24, 2022
5048398
Fix KestrelConfigurationLoaderTests to use new dev cert on macOS.
adityamandaleeka Jul 15, 2022
0346d22
Bring back 6.0 compat, prompt on https when updating state.
adityamandaleeka Jul 26, 2022
983aee6
Remove OID change warning.
adityamandaleeka Jul 26, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 3 additions & 11 deletions src/Servers/Kestrel/Core/src/KestrelServerOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -285,18 +285,10 @@ private void EnsureDefaultCert()
Debug.Assert(status.FailureMessage != null, "Status with a failure result must have a message.");
logger.DeveloperCertificateFirstRun(status.FailureMessage);

// Now that we've displayed a warning in the logs so that the user gets a notification that a prompt might appear, try
// and access the certificate key, which might trigger a prompt.
status = CertificateManager.Instance.CheckCertificateState(DefaultCertificate, interactive: true);
if (!status.Success)
{
logger.BadDeveloperCertificateState();
}
// Prevent binding to HTTPS if the certificate is not valid (avoid the prompt)
DefaultCertificate = null;
}

logger.LocatedDevelopmentCertificate(DefaultCertificate);

if (!CertificateManager.Instance.IsTrusted(DefaultCertificate))
else if (!CertificateManager.Instance.IsTrusted(DefaultCertificate))
{
logger.DeveloperCertificateNotTrusted();
}
Expand Down
77 changes: 55 additions & 22 deletions src/Shared/CertificateGeneration/CertificateManager.cs
Original file line number Diff line number Diff line change
@@ -1,12 +1,9 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Diagnostics.Tracing;
using System.IO;
using System.Linq;
using System.Security.Cryptography;
using System.Security.Cryptography.X509Certificates;
Expand All @@ -19,6 +16,8 @@ namespace Microsoft.AspNetCore.Certificates.Generation;
internal abstract class CertificateManager
{
internal const int CurrentAspNetCoreCertificateVersion = 2;

// OID used for HTTPS certs
internal const string AspNetHttpsOid = "1.3.6.1.4.1.311.84.1.1";
internal const string AspNetHttpsOidFriendlyName = "ASP.NET Core HTTPS development certificate";

Expand Down Expand Up @@ -78,7 +77,7 @@ public IList<X509Certificate2> ListCertificates(
{
using var store = new X509Store(storeName, location);
store.Open(OpenFlags.ReadOnly);
certificates.AddRange(store.Certificates.OfType<X509Certificate2>());
PopulateCertificatesFromStore(store, certificates);
IEnumerable<X509Certificate2> matchingCertificates = certificates;
matchingCertificates = matchingCertificates
.Where(c => HasOid(c, AspNetHttpsOid));
Expand Down Expand Up @@ -161,6 +160,11 @@ bool IsValidCertificate(X509Certificate2 certificate, DateTimeOffset currentDate
GetCertificateVersion(certificate) >= AspNetHttpsCertificateVersion;
}

protected virtual void PopulateCertificatesFromStore(X509Store store, List<X509Certificate2> certificates)
{
certificates.AddRange(store.Certificates.OfType<X509Certificate2>());
}

public IList<X509Certificate2> GetHttpsCertificates() =>
ListCertificates(StoreName.My, StoreLocation.CurrentUser, isValid: true, requireExportable: true);

Expand Down Expand Up @@ -340,6 +344,15 @@ public EnsureCertificateResult EnsureAspNetCoreHttpsDevelopmentCertificate(
result = EnsureCertificateResult.FailedToTrustTheCertificate;
return result;
}

if (result == EnsureCertificateResult.ValidCertificatePresent)
{
result = EnsureCertificateResult.ExistingHttpsCertificateTrusted;
}
else
{
result = EnsureCertificateResult.NewHttpsCertificateTrusted;
}
Comment on lines +348 to +355
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we avoid adding new results here? This is a contract between us and the tooling teams, so adding new results changes that contract and might break them.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@javiercn Ah, I see, good to know. I'll check out how the tooling stuff consumes this today.

I'll also add a note mentioning this contract next to the enum declaration so someone doesn't accidentally break the contract in the future.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... can you point me to where this is used by tooling? I can't seem to find it.

cc @vijayrkn

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @vijayrkn! Looks like the tooling just calls the dev-certs tool and relies on some of the status codes here which are the exit codes of the tool:

private const int Success = 0;
private const int ErrorCreatingTheCertificate = 1;
private const int ErrorSavingTheCertificate = 2;
private const int ErrorExportingTheCertificate = 3;
private const int ErrorTrustingTheCertificate = 4;
private const int ErrorUserCancelledTrustPrompt = 5;
private const int ErrorNoValidCertificateFound = 6;
private const int ErrorCertificateNotTrusted = 7;

The EnsureCertificateResult enum changes I made won't change the actual exit codes from the tool, since these turn into "Success" here:

https://github.com/dotnet/aspnetcore/pull/42251/files#diff-1454ec4889b7f660676a246d1b0f74f29b980e4370e1932e012e2b968d33702aR433

So I think we're safe.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding @BillHiebert in case there are other places where we call into the tool.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a note of caution about modifying the exit codes.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also impacts VS 4 Mac and Docker container tools. FYI.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see now that this only affects the internal reporting, but that the tool still returns 0 (success) so I think we are good here.

}

DisposeCertificates(!isNewCertificate ? certificates : certificates.Append(certificate));
Expand Down Expand Up @@ -411,13 +424,6 @@ internal ImportCertificateResult ImportCertificate(string certificatePath, strin

public void CleanupHttpsCertificates()
{
// On OS X we don't have a good way to manage trusted certificates in the system keychain
// so we do everything by invoking the native toolchain.
// This has some limitations, like for example not being able to identify our custom OID extension. For that
// matter, when we are cleaning up certificates on the machine, we start by removing the trusted certificates.
// To do this, we list the certificates that we can identify on the current user personal store and we invoke
// the native toolchain to remove them from the sytem keychain. Once we have removed the trusted certificates,
// we remove the certificates from the local user store to finish up the cleanup.
var certificates = ListCertificates(StoreName.My, StoreLocation.CurrentUser, isValid: false);
var filteredCertificates = certificates.Where(c => c.Subject == Subject);

Expand All @@ -430,6 +436,8 @@ public void CleanupHttpsCertificates()

foreach (var certificate in filteredCertificates)
{
// RemoveLocations.All will first remove from the trusted roots (e.g. keychain on
// macOS) and then from the local user store.
RemoveCertificate(certificate, RemoveLocations.All);
}
}
Expand Down Expand Up @@ -745,22 +753,15 @@ internal static void DisposeCertificates(IEnumerable<X509Certificate2> disposabl
}
}

private static void RemoveCertificateFromUserStore(X509Certificate2 certificate)
protected virtual void RemoveCertificateFromUserStore(X509Certificate2 certificate)
{
try
{
if (Log.IsEnabled())
{
Log.RemoveCertificateFromUserStoreStart(GetDescription(certificate));
}
using var store = new X509Store(StoreName.My, StoreLocation.CurrentUser);
store.Open(OpenFlags.ReadWrite);
var matching = store.Certificates
.OfType<X509Certificate2>()
.Single(c => c.SerialNumber == certificate.SerialNumber);

store.Remove(matching);
store.Close();
RemoveCertificateFromUserStoreCore(certificate);
Log.RemoveCertificateFromUserStoreEnd();
}
catch (Exception ex) when (Log.IsEnabled())
Expand All @@ -770,6 +771,17 @@ private static void RemoveCertificateFromUserStore(X509Certificate2 certificate)
}
}

protected virtual void RemoveCertificateFromUserStoreCore(X509Certificate2 certificate)
{
using var store = new X509Store(StoreName.My, StoreLocation.CurrentUser);
store.Open(OpenFlags.ReadWrite);
var matching = store.Certificates
.OfType<X509Certificate2>()
.Single(c => c.SerialNumber == certificate.SerialNumber);

store.Remove(matching);
}

internal static string ToCertificateDescription(IEnumerable<X509Certificate2> certificates)
{
var list = certificates.ToList();
Expand Down Expand Up @@ -944,8 +956,8 @@ public sealed class CertificateManagerEventSource : EventSource
[Event(55, Level = EventLevel.Verbose, Message = "Finished importing the certificate to the keychain.")]
internal void MacOSAddCertificateToKeyChainEnd() => WriteEvent(55);

[Event(56, Level = EventLevel.Error, Message = "An error has occurred while importing the certificate to the keychain: {0}.")]
internal void MacOSAddCertificateToKeyChainError(int exitCode) => WriteEvent(56, exitCode);
[Event(56, Level = EventLevel.Error, Message = "An error has occurred while importing the certificate to the keychain: {0}, {1}")]
internal void MacOSAddCertificateToKeyChainError(int exitCode, string output) => WriteEvent(56, exitCode, output);

[Event(57, Level = EventLevel.Verbose, Message = "Writing the certificate to: {0}.")]
public void WritePemKeyToDisk(string path) => WriteEvent(57, path);
Expand All @@ -970,6 +982,27 @@ public sealed class CertificateManagerEventSource : EventSource

[Event(64, Level = EventLevel.Error, Message = "The provided certificate '{0}' is not a valid ASP.NET Core HTTPS development certificate.")]
internal void NoHttpsDevelopmentCertificate(string description) => WriteEvent(64, description);

[Event(65, Level = EventLevel.Verbose, Message = "The certificate is already trusted.")]
public void MacOSCertificateAlreadyTrusted() => WriteEvent(65);

[Event(66, Level = EventLevel.Verbose, Message = "Saving the certificate {1} to the user profile folder '{0}'.")]
internal void MacOSAddCertificateToUserProfileDirStart(string directory, string certificate) => WriteEvent(66, directory, certificate);

[Event(67, Level = EventLevel.Verbose, Message = "Finished saving the certificate to the user profile folder.")]
internal void MacOSAddCertificateToUserProfileDirEnd() => WriteEvent(67);

[Event(68, Level = EventLevel.Error, Message = "An error has occurred while saving certificate '{0}' in the user profile folder: {1}.")]
internal void MacOSAddCertificateToUserProfileDirError(string certificateThumbprint, string errorMessage) => WriteEvent(68, certificateThumbprint, errorMessage);

[Event(69, Level = EventLevel.Error, Message = "An error has occurred while removing certificate '{0}' from the user profile folder: {1}.")]
internal void MacOSRemoveCertificateFromUserProfileDirError(string certificateThumbprint, string errorMessage) => WriteEvent(69, certificateThumbprint, errorMessage);

[Event(70, Level = EventLevel.Error, Message = "The file '{0}' is not a valid certificate.")]
internal void MacOSFileIsNotAValidCertificate(string path) => WriteEvent(70, path);

[Event(71, Level = EventLevel.Warning, Message = "The on-disk store directory was not found.")]
internal void MacOSDiskStoreDoesNotExist() => WriteEvent(71);
}

internal sealed class UserCancelledTrustException : Exception
Expand Down
2 changes: 2 additions & 0 deletions src/Shared/CertificateGeneration/EnsureCertificateResult.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,7 @@ internal enum EnsureCertificateResult
FailedToTrustTheCertificate,
UserCancelledTrustStep,
FailedToMakeKeyAccessible,
ExistingHttpsCertificateTrusted,
NewHttpsCertificateTrusted
}

Loading