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.

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

merged 6 commits into from
Feb 8, 2021

Conversation

HaoK
Copy link
Member

@HaoK HaoK commented Feb 1, 2021

Fixes #29679

@ghost ghost added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label Feb 1, 2021
@HaoK HaoK added this to the 6.0-preview2 milestone Feb 1, 2021
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

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.

@HaoK HaoK marked this pull request as ready for review February 3, 2021 22:08
@HaoK HaoK requested a review from Tratcher as a code owner February 3, 2021 22:08
@HaoK HaoK requested review from bartonjs, blowdart and a team and removed request for a team February 3, 2021 22:08
@Tratcher
Copy link
Member

Tratcher commented Feb 4, 2021

@aalsamoht FYI

@HaoK HaoK merged commit ae6e9c8 into main Feb 8, 2021
@HaoK HaoK deleted the haok/store branch February 8, 2021 18:25
@HaoK
Copy link
Member Author

HaoK commented Feb 8, 2021

Thanks for your help @bartonjs !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ExtraStore property to CertificateAuthenticationOptions
4 participants