[TLS 1.3] Server-Side Client Authentication#3081
Merged
Conversation
aa3818b to
8ff80d0
Compare
Codecov ReportBase: 87.97% // Head: 87.99% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #3081 +/- ##
==========================================
+ Coverage 87.97% 87.99% +0.01%
==========================================
Files 600 600
Lines 66902 67054 +152
Branches 6687 6698 +11
==========================================
+ Hits 58857 59003 +146
- Misses 5223 5235 +12
+ Partials 2822 2816 -6
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
8ff80d0 to
cf095ed
Compare
7 tasks
cf095ed to
17d2c97
Compare
randombit
approved these changes
Dec 15, 2022
Owner
randombit
left a comment
There was a problem hiding this comment.
Just reviewed the final 5 commits
| // This will call the modification callback multiple times. Once for | ||
| // each certificate in the `cert_chain`. Users that want to add an | ||
| // extension to a specific Certificate Entry might have a hard time | ||
| // to distinguish them. |
Owner
There was a problem hiding this comment.
We could perhaps add a variant that additionally accepts the certificate in question, which by default just forwards the call to tls_modify_extensions and discards the cert. This could happen later on, even post-3.0, so I don't consider it at all essential.
Collaborator
Author
There was a problem hiding this comment.
That's roughly what I had in mind writing this comment. I'd add it when the need comes up.
Scenario: After receiving a Client Hello the server responds with its first flight. The client fails to decode the Server Hello and responds with an Alert. It cannot protect this Alert, as it didn't understand the server's key negotiation messages and is forced to send the Alert in the clear. The server needs to expect that and allow unprotected Alerts, despite it already having derived key material.
This makes Certificate_13 smarter. The class now takes care of gathering the necessary information in its own constructor.
The message class now takes full care of it's own creation by pulling the required private key from the credentials manager
17d2c97 to
a009a56
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Pull Request Dependencies
Description
Adds support for client authentication to the TLS 1.3 server implementation. Respective RFC 8448 test is passing, BoGo tests are passing.
Messages
Certificate,Certificate_RequestandCertificateVerifyThe refactoring in this patch aims at making construction of those messages more self-contained. Instead of handing in the detailed information those messages need for their own construction, we hand in the higher-level objects that contain the necessary information. E.g. instead of passing the correct private key to
CertificateVerify, it now gets a reference to theCredentials_Managerenabling its constructor to gather the correct private key pointer internally. This moves low-level logic and orchestration out of the Client/Server implementation classes.For
Certificate_Request_13this even moves the decision whether or not to even request certificate authentication from the client into a factory method (seeCertificate_Request_13::maybe_create()). I'm myself not sure yet, whether I actually like that pattern. Though, it takes complexity out oftls_server_impl_13.cppwhich I consider a good thing.Certificate::setup_entries()might seem to be overly simple, but it was created as a pre-text to the OCSP stapling pull request where it will be filled with more logic.FIX: Unprotected Alerts
During the development of this functionality, BoGo tests replied with a plaintext TLS alert when we (the server) already exclusively expected encrypted records. Turns out, that is not okay and needed a fix (
TLS::Cipher_State::must_expect_unprotected_alert_traffic()).Scenario: After receiving a Client Hello we (the server) respond with our first flight. Having just sent the server hello (with enough cryptographic information to encrypt further traffic) we expected the client to respond with protected records only. However, if the client fails to decode the Server Hello and must respond with an Alert, it is forced to send this alert in the clear, as it didn't understand our key negotiation info. We (as the server) need to expect that and allow unprotected Alerts, despite already having derived valid key material.
It appears to me, that the described situation is the only scenario where unprotected records need to be expected by either the client or the server despite cryptographic material already being available. Unfortunately, that was not explicitly pointed out in the RFC it seems.