-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
Kestrel: Support full cert chain #41944
Merged
Merged
Changes from all commits
Commits
Show all changes
30 commits
Select commit
Hold shift + click to select a range
0bb35c9
Kestrel: Support full cert chain
HaoK 21a26a7
Update doc comments (per blowdart)
HaoK db88bda
Update CertificateConfigLoader.cs
HaoK c571af2
Cleanup
HaoK 1ba7ca4
Fixup
HaoK 9436963
Add a test to verify full chain is loaded
HaoK 4271b6e
Fix GetTestCertWithkey
HaoK f9cd65d
Add cert test helpers from runtime
HaoK 8251c0a
Add test
HaoK 9a549c7
Send client cert
HaoK b8ceb60
Remove test
HaoK 83cec9d
Update CertHelper.cs
HaoK 510a195
Fix warnings
HaoK 9e7d947
Cleanup
HaoK e18c53d
Remove offline
HaoK 2311399
Dump chainElement when it fails to build in test
HaoK 54da27a
Remove cert from chain, also print more useful status
HaoK 6db0573
Cleanup
HaoK 4a0fc6f
Undo changes to full chain
HaoK 5091181
See if offline is causing test failures
HaoK 03e2f51
Update SniOptionsSelector.cs
HaoK b8a66c1
Remove offline
HaoK e8369cf
Test
HaoK f949d16
Tweak test cleanup
HaoK 0a25765
Skip on mac for now
HaoK 5760a8d
Add more logging
HaoK d543da6
Try a long chain
HaoK 91c423b
Print issuer and subject
HaoK 70028d6
Simplify test
HaoK ff756dd
Skip on OSX
HaoK File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -168,5 +168,4 @@ private static void RequireAuthorizationCore<TBuilder>(TBuilder builder, IEnumer | |
} | ||
}); | ||
} | ||
|
||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,3 @@ | ||
#nullable enable | ||
Microsoft.AspNetCore.Server.Kestrel.Https.HttpsConnectionAdapterOptions.ServerCertificateChain.get -> System.Security.Cryptography.X509Certificates.X509Certificate2Collection? | ||
Microsoft.AspNetCore.Server.Kestrel.Https.HttpsConnectionAdapterOptions.ServerCertificateChain.set -> void |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this if block, you could just make
certificate
befullChain[0]
(assuming it's non-empty), and then you could removecertificate
from the collection; depending on what public API guarantees you're making.The two halves of that sentence are:
If you don't remove it, because you want the "full" chain to be in the HttpsOptions.ServerCertificateChain collection, then you have to decide if you want to have the same instance in the property and the collection. If "yes" to the full chain and "no" to the same instance... then leave the code as-is 😄.
And the reason I put "full" in quotes is that I don't think Let's Encrypt puts their root cert in that file, so the "full" chain is really "the chain except the root". But it's as "full" as that file is, I suppose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adityamandaleeka @blowdart @davidfowl @halter73 I can't say I have anywhere near enough context for this feature request/area to be able to personally decide how this should work...
Do we want to just vote or something? Personally I like what @bartonjs is suggesting, i.e. we just use fullChain[0] and remove the cert from the collection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea remove it. A chain is everything above the leaf in my mind. Need to document carefully though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a bunch of research here a year ago and I can dig up my notes. When you get the certs from certbot for lets encrypt you can get both the fullchain.pem or the chain.pem + cert.pem (https://community.letsencrypt.org/t/generating-cert-pem-chain-pem-and-fullchain-pem-from-order-certificate/78376/6).
We should also support loading the full chai from PFX files if it is there. NGINX supports the full chain as well (https://serverfault.com/questions/987612/nginx-ssl-config-for-cert-pem-and-chain-pem).
I'd like us to support both providing just the intermediates without the leaf cert and the full chain and we can remove the leaf cert (assuming this is easy). That makes us a bit more friendly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand the nuances here. I updated the PR to Jeremy's suggestion so we remove the cert from the chain if we have a key path.
Is pfx support something different or does that just work?
The PR currently only supports full chain right? So is the intermediates only scenario we don't assume fullChain[0] is the leaf?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some differences in using the cert from the chain vs the current GetCertificate method, as a bunch of tests now fail. So I think I'm just leave this code alone for now, and I will file a new issue to track @davidfowl and @bartonjs suggestions which I'd imagine we can take in rc2 still. I just want to try and get something in for rc1 this week
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bartonjs so the MacOS error is: "Status: One or more certificates required to validate this certificate cannot be found." This is a bit weird since this works fine on ubuntu and windows, but I know we've skipped on MacOS many of our other cert tests today... I'm tempted to just skip this on macOS and file an issue to look at this later, unless this is something that is indicative of something seriously being broken
I filed #43193 to track resolving the full chain/intermediate/removing a cert.
I think many tests were all failing because the new test I copied from runtime was nuking all of our test certs which broke all the other tests as part of its cleanup, I scoped its cleanup to only the certs it was creating and things seem happier now.