Skip to content

Add versioning to dotnet-dev-certs #10908

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Jun 6, 2019
Merged

Add versioning to dotnet-dev-certs #10908

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
35a3c0b
Initial work
jkotalik Jun 5, 2019
7f87ebd
Fix checks and port some deleted code back
jkotalik Jun 5, 2019
807360e
make it so it builds
jkotalik Jun 5, 2019
3fa40e8
Feedback:
jkotalik Jun 5, 2019
bda214f
Nit
jkotalik Jun 5, 2019
2b4b472
nit
jkotalik Jun 5, 2019
bebfafe
Feedback
jkotalik Jun 5, 2019
3686dda
Update CertificateManager.cs
jkotalik Jun 5, 2019
7d6f608
Enable tests on non-helix workloads
jkotalik Jun 5, 2019
419f937
Update src/Shared/CertificateGeneration/CertificateManager.cs
jkotalik Jun 5, 2019
e315116
Merge branch 'jkotalik/devCert' of https://github.com/aspnet/AspNetCo…
jkotalik Jun 5, 2019
83f9a3f
Fixture
jkotalik Jun 5, 2019
9a7be15
nit
jkotalik Jun 5, 2019
8671d21
Update CertificateManagerTests.cs
jkotalik Jun 6, 2019
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
146 changes: 40 additions & 106 deletions src/Shared/CertificateGeneration/CertificateManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ internal class CertificateManager
private static readonly string MacOSTrustCertificateCommandLineArguments = "security add-trusted-cert -d -r trustRoot -k " + MacOSSystemKeyChain + " ";
private const int UserCancelledErrorCode = 1223;

// Setting to 0 means we don't append the version byte,
// which is what all machines currently have.
public int AspNetHttpsCertificateVersion { get; set; } = 1;

public IList<X509Certificate2> ListCertificates(
CertificatePurpose purpose,
StoreName storeName,
Expand Down Expand Up @@ -83,7 +87,8 @@ public IList<X509Certificate2> ListCertificates(
var validCertificates = matchingCertificates
.Where(c => c.NotBefore <= now &&
now <= c.NotAfter &&
(!requireExportable || !RuntimeInformation.IsOSPlatform(OSPlatform.Windows) || IsExportable(c)))
(!requireExportable || !RuntimeInformation.IsOSPlatform(OSPlatform.Windows) || IsExportable(c))
&& MatchesVersion(c))
.ToArray();

var invalidCertificates = matchingCertificates.Except(validCertificates);
Expand Down Expand Up @@ -117,6 +122,25 @@ public IList<X509Certificate2> ListCertificates(
bool HasOid(X509Certificate2 certificate, string oid) =>
certificate.Extensions.OfType<X509Extension>()
.Any(e => string.Equals(oid, e.Oid.Value, StringComparison.Ordinal));

bool MatchesVersion(X509Certificate2 c)
{
var byteArray = c.Extensions.OfType<X509Extension>()
.Where(e => string.Equals(AspNetHttpsOid, e.Oid.Value, StringComparison.Ordinal))
.Single()
.RawData;

if ((byteArray.Length == AspNetHttpsOidFriendlyName.Length && byteArray[0] == (byte)'A') || byteArray.Length == 0)
{
// No Version set, default to 0
return 0 >= AspNetHttpsCertificateVersion;
}
else
{
// Version is in the only byte of the byte array.
return byteArray[0] >= AspNetHttpsCertificateVersion;
}
}
#if !XPLAT
bool IsExportable(X509Certificate2 c) =>
((c.GetRSAPrivateKey() is RSACryptoServiceProvider rsaPrivateKey &&
Expand Down Expand Up @@ -171,10 +195,22 @@ public X509Certificate2 CreateAspNetCoreHttpsDevelopmentCertificate(DateTimeOffs
pathLengthConstraint: 0,
critical: true);

byte[] bytePayload;

if (AspNetHttpsCertificateVersion != 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

This raises the question of if we need to support creating old cert versions. I don't think we do, at least not yet. The new cert should be compatible with old code (it just adds some EKUs that were technically required anyway). If we make a breaking change to the cert we could add this kind of logic back.

That would also mean AspNetHttpCertificateVersion can be static readonly.

Copy link
Contributor Author

@jkotalik jkotalik Jun 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I doubt we need to, and even if we need to, we can work around it. I'd rather not over-engineer this. The only reason AspNetHttpCertificateVersion is public is for testing. I can make it internal.

Copy link
Member

Choose a reason for hiding this comment

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

We don't need to support going back and I don't think we would ever need. The cert needs to be backwards compatible or we would have to use a different OID if we have to make breaking changes

{
bytePayload = new byte[1];
bytePayload[0] = (byte)AspNetHttpsCertificateVersion;
}
else
{
bytePayload = Encoding.ASCII.GetBytes(AspNetHttpsOidFriendlyName);
}

var aspNetHttpsExtension = new X509Extension(
new AsnEncodedData(
new Oid(AspNetHttpsOid, AspNetHttpsOidFriendlyName),
Encoding.ASCII.GetBytes(AspNetHttpsOidFriendlyName)),
bytePayload),
critical: false);

extensions.Add(basicConstraints);
Expand Down Expand Up @@ -633,7 +669,7 @@ private static void RemoveCertificateFromKeyChain(string keyChain, X509Certifica
}
}

public EnsureCertificateResult EnsureAspNetCoreHttpsDevelopmentCertificate(
public DetailedEnsureCertificateResult EnsureAspNetCoreHttpsDevelopmentCertificate(
DateTimeOffset notBefore,
DateTimeOffset notAfter,
string path = null,
Expand All @@ -645,109 +681,7 @@ public EnsureCertificateResult EnsureAspNetCoreHttpsDevelopmentCertificate(
return EnsureValidCertificateExists(notBefore, notAfter, CertificatePurpose.HTTPS, path, trust, includePrivateKey, password, subject);
}

public EnsureCertificateResult EnsureValidCertificateExists(
DateTimeOffset notBefore,
DateTimeOffset notAfter,
CertificatePurpose purpose,
string path = null,
bool trust = false,
bool includePrivateKey = false,
string password = null,
string subjectOverride = null)
{
if (purpose == CertificatePurpose.All)
{
throw new ArgumentException("The certificate must have a specific purpose.");
}

var certificates = ListCertificates(purpose, StoreName.My, StoreLocation.CurrentUser, isValid: true).Concat(
ListCertificates(purpose, StoreName.My, StoreLocation.LocalMachine, isValid: true));

certificates = subjectOverride == null ? certificates : certificates.Where(c => c.Subject == subjectOverride);

var result = EnsureCertificateResult.Succeeded;

X509Certificate2 certificate = null;
if (certificates.Count() > 0)
{
certificate = certificates.FirstOrDefault();
result = EnsureCertificateResult.ValidCertificatePresent;
}
else
{
try
{
switch (purpose)
{
case CertificatePurpose.All:
throw new InvalidOperationException("The certificate must have a specific purpose.");
case CertificatePurpose.HTTPS:
certificate = CreateAspNetCoreHttpsDevelopmentCertificate(notBefore, notAfter, subjectOverride);
break;
default:
throw new InvalidOperationException("The certificate must have a purpose.");
}
}
catch
{
return EnsureCertificateResult.ErrorCreatingTheCertificate;
}

try
{
certificate = SaveCertificateInStore(certificate, StoreName.My, StoreLocation.CurrentUser);
}
catch
{
return EnsureCertificateResult.ErrorSavingTheCertificateIntoTheCurrentUserPersonalStore;
}
}
if (path != null)
{
try
{
ExportCertificate(certificate, path, includePrivateKey, password);
}
catch
{
return EnsureCertificateResult.ErrorExportingTheCertificate;
}
}

if ((RuntimeInformation.IsOSPlatform(OSPlatform.Windows) || RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) && trust)
{
try
{
TrustCertificate(certificate);
}
catch (UserCancelledTrustException)
{
return EnsureCertificateResult.UserCancelledTrustStep;
}
catch
{
return EnsureCertificateResult.FailedToTrustTheCertificate;
}
}

return result;
}

// This is just to avoid breaking changes across repos.
// Will be renamed back to EnsureAspNetCoreHttpsDevelopmentCertificate once updates are made elsewhere.
public DetailedEnsureCertificateResult EnsureAspNetCoreHttpsDevelopmentCertificate2(
DateTimeOffset notBefore,
DateTimeOffset notAfter,
string path = null,
bool trust = false,
bool includePrivateKey = false,
string password = null,
string subject = LocalhostHttpsDistinguishedName)
{
return EnsureValidCertificateExists2(notBefore, notAfter, CertificatePurpose.HTTPS, path, trust, includePrivateKey, password, subject);
}

public DetailedEnsureCertificateResult EnsureValidCertificateExists2(
public DetailedEnsureCertificateResult EnsureValidCertificateExists(
DateTimeOffset notBefore,
DateTimeOffset notAfter,
CertificatePurpose purpose,
Expand Down
Loading