Skip to content

Make it easier to add certs to the extra store #29828

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 6 commits into from
Feb 8, 2021
Merged

Make it easier to add certs to the extra store #29828

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
eba1965
Update CertificateAuthenticationOptions.cs
HaoK Feb 1, 2021
97828b2
Update CertificateAuthenticationHandler.cs
HaoK Feb 1, 2021
f3e805d
Update CertificateTests.cs
HaoK Feb 1, 2021
dbc6004
Update baseline
HaoK Feb 1, 2021
39dad31
Actualy copy the additional chain certs
HaoK Feb 2, 2021
42f0e70
Update tests
HaoK Feb 3, 2021
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
2 changes: 2 additions & 0 deletions src/Security/Authentication/Certificate/src/CertificateAuthenticationHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,8 @@ private X509ChainPolicy BuildChainPolicy(X509Certificate2 certificate)
chainPolicy.TrustMode = Options.ChainTrustValidationMode;
}

chainPolicy.ExtraStore.AddRange(Options.AdditionalChainCertificates);

if (!Options.ValidateValidityPeriod)
{
chainPolicy.VerificationFlags |= X509VerificationFlags.IgnoreNotTimeValid;
Expand Down
5 changes: 5 additions & 0 deletions src/Security/Authentication/Certificate/src/CertificateAuthenticationOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@ public class CertificateAuthenticationOptions : AuthenticationSchemeOptions
/// Collection of X509 certificates which are trusted components of the certificate chain.
/// </summary>
public X509Certificate2Collection CustomTrustStore { get; set; } = new X509Certificate2Collection();

/// <summary>
/// Collection of X509 certificates which are added to the X509Chain.ChainPolicy.ExtraStore of the certificate chain.
/// </summary>
public X509Certificate2Collection AdditionalChainCertificates { get; set; } = new X509Certificate2Collection();

/// <summary>
/// Method used to validate certificate chains against <see cref="CustomTrustStore"/>.
Expand Down
2 changes: 2 additions & 0 deletions src/Security/Authentication/Certificate/src/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
#nullable enable
Microsoft.AspNetCore.Authentication.Certificate.CertificateAuthenticationOptions.AdditionalChainCertificates.get -> System.Security.Cryptography.X509Certificates.X509Certificate2Collection!
Microsoft.AspNetCore.Authentication.Certificate.CertificateAuthenticationOptions.AdditionalChainCertificates.set -> void
38 changes: 37 additions & 1 deletion src/Security/Authentication/test/CertificateTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,41 @@ public async Task VerifyValidClientCertWithTrustedChainAuthenticates()
Assert.Equal(HttpStatusCode.OK, response.StatusCode);
}

[Fact]
public async Task VerifyValidClientCertWithAdditionalCertificatesAuthenticates()
{
using var host = await CreateHost(
new CertificateAuthenticationOptions
{
Events = successfulValidationEvents,
ChainTrustValidationMode = X509ChainTrustMode.CustomRootTrust,
CustomTrustStore = new X509Certificate2Collection() { Certificates.SelfSignedPrimaryRoot, },
AdditionalChainCertificates = new X509Certificate2Collection() { Certificates.SignedSecondaryRoot },
RevocationMode = X509RevocationMode.NoCheck
}, Certificates.SignedClient);

using var server = host.GetTestServer();
Copy link
Member Author

Choose a reason for hiding this comment

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

@bartonjs I added the new ExtraStore property but its not clear to me how exactly do I test that they are working as intended? They are being added to the chain's property now but I'm not sure how to measure the effect externally, is this just additional certs that developers would use themselves?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know where all the chain can appear, so I'll stick to a high level test concept:

  • Have a 3+ certificate chain which does not come from a public trust system and does not use Authority Information Access (AIA) chaining (either statically generated or on the fly using the CertificateRequest API).
  • Add the root to the custom trust store (and set the mode to CustomRootTrust), add all of the intermediates to AdditionalChainCertificates.
  • When the chain builds, it builds successfully.
  • Bonus test, first do it without adding the certs to AdditionalChainCertificates and see that the chain can't be built.
  • Other bonus test, add the root to AdditionalChainCertificates and don't add it to custom trust. The chain should have all of the certificates but be untrusted.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @bartonjs that's exactly what I was hoping for, a high level overview. The bonus test is actually what this current test is doing, (and it fails). So one thing is we don't actually expose the ChainPolicy that we use to validate externally. So there's no easy way to do anything with the ExtraStore since we don't pass the X509Chain we build to any of our events.

See

aspnetcore/src/Security/Authentication/Certificate/src/CertificateAuthenticationHandler.cs

Line 125 in 39dad31

var chainPolicy = BuildChainPolicy(clientCertificate);

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool updated and added a test for a positive case and a rejected/forbidden one.

var response = await server.CreateClient().GetAsync("https://example.com/");
Assert.Equal(HttpStatusCode.OK, response.StatusCode);
}

[Fact]
public async Task VerifyValidClientCertFailsWithoutAdditionalCertificatesAuthenticates()
{
using var host = await CreateHost(
new CertificateAuthenticationOptions
{
Events = successfulValidationEvents,
ChainTrustValidationMode = X509ChainTrustMode.CustomRootTrust,
CustomTrustStore = new X509Certificate2Collection() { Certificates.SelfSignedPrimaryRoot, },
RevocationMode = X509RevocationMode.NoCheck
}, Certificates.SignedClient);

using var server = host.GetTestServer();
var response = await server.CreateClient().GetAsync("https://example.com/");
Assert.Equal(HttpStatusCode.Forbidden, response.StatusCode);
}

[Fact]
public async Task VerifyHeaderIsUsedIfCertIsNotPresent()
{
Expand Down Expand Up @@ -570,7 +605,7 @@ public async Task VerifyValidationResultCanBeCached(bool cache)
Assert.Equal(Expected, name.First().Value);
count = responseAsXml.Elements("claim").Where(claim => claim.Attribute("Type").Value == "ValidationCount");
Assert.Single(count);
var expected = cache ? "1" : "2";
var expected = cache ? "1" : "2";
Assert.Equal(expected, count.First().Value);
}

Expand Down Expand Up @@ -693,6 +728,7 @@ private static async Task<IHost> CreateHost(
options.RevocationFlag = configureOptions.RevocationFlag;
options.RevocationMode = configureOptions.RevocationMode;
options.ValidateValidityPeriod = configureOptions.ValidateValidityPeriod;
options.AdditionalChainCertificates = configureOptions.AdditionalChainCertificates;
});
}
else
Expand Down