Include intermediate certs in TLS handshake #5453
Merged
Conversation
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
achamayou
reviewed
Jul 13, 2023
achamayou
approved these changes
Jul 13, 2023
achamayou
added
auto-backport
|
present_intermediate_certs@73128 aka 20230714.12 vs main ewma over 20 builds from 72744 to 73119 Click to see tablemain
present_intermediate_certs
|
achamayou
approved these changes
Jul 13, 2023
Co-authored-by: Amaury Chamayou <amaury@xargs.fr>
💔 All backports failed
Manual backportTo create the backport manually run: Questions ?Please refer to the Backport tool documentation and see the Github Action logs for details |
eddyashton
added
the
backported
eddyashton
added a commit
to eddyashton/CCF
that referenced
this pull request
Jul 14, 2023
(cherry picked from commit 467c57d)
eddyashton
added a commit
that referenced
this pull request
Jul 14, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This doesn't make a difference for most of our end-to-end tests, but does for an ACME-endorsed interface. We end up with
Service Cert <- Intermediate <- Root(potentially multiple intermediates, but one is sufficient for demo). We store in the KV, and attempt to set as the server cert, the chain (/bundle)[Service Cert] \n [Intermediate], but when passing that to OpenSSL we actually end up callingPEM_read_bio_X509which only extracts[Service Cert], and drop theIntermediateon the ground.That means that a connecting browser, which likely only has
Rootin their trust store, can't build a complete chain and won't trust CCF.The solution is retain the chain (an
X509_STACKin OpenSSLese), though we have to split the bundle and remember it separately, and then pass it through touse_cert_and_key.The end-to-end test we have using Pebble to obtain ACME endorsements had missed this, because it fetched both the Root and Intermediate from Pebble, and added them to the trust store for the test connection. I've changed that to only fetch the Root, which should match what we get in a real service.
A pure digression, but something I've not been able to find documentation on - I assume that for a real ACME endorsement we end up with a longer chain, with some overlap between the ACME service's response and the CAs in a system's trust store. ie if we end up with
Service Cert <- This Let's Encrypt Instance <- Let's Encrypt Master <- ISRG Root, there's a chance thatLet's Encryptreturns a chain includingLet's Encrypt Master, and that's also in a system trust store. So maybe there's some overlap and redundancy, if we blindly retain the entire chain. But I don't think we can make any assessment of where the cut-off point is, so we assume the endorser/ACME protocol is handling it sensibly, and retain whatever they give us.