Upstream merge 2024 11 18 #2012
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
skmcgrail
force-pushed
the
upstream-merge-2024-11-18
branch
from
fb3c49c to
600b961
Compare
nebeid
reviewed
Nov 28, 2024
skmcgrail
force-pushed
the
upstream-merge-2024-11-18
branch
from
600b961 to
4fdca57
Compare
skmcgrail
force-pushed
the
upstream-merge-2024-11-18
branch
from
4fdca57 to
0fff81b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2012 +/- ##
==========================================
+ Coverage 78.65% 78.67% +0.02%
==========================================
Files 598 598
Lines 103329 103347 +18
Branches 14688 14692 +4
==========================================
+ Hits 81272 81312 +40
+ Misses 21404 21383 -21
+ Partials 653 652 -1 ☔ View full report in Codecov by Sentry. |
justsmth
previously approved these changes
Dec 4, 2024
nebeid
previously approved these changes
Dec 4, 2024
TLS <= 1.2 servers indicate supported client certificate key types with
a certificate_types field in the CertificateRequest. Historically, we've
just ignored this field, because we've always outsourced certificate
selection to the caller anyway. This meant that, if you configured an
RSA client certificate in response to a server that requested only ECDSA
certificates, we would happily send the certificate and leave it to the
server to decide if it was happy.
Strictly speaking, this was in violation of RFC 5246:
- The end-entity certificate provided by the client MUST contain a
key that is compatible with certificate_types. [...]
Although prior TLS versions didn't say anything useful about this either
way.
Once we move certificate selection into the library, we'll want to start
evaluating supported algorithms ourselves. A natural implementation of
it will, as a side effect, cause us to enforce this match, even when
only a single certificate is configured. Since this is unlikely to have
any real compatibility impact (every TLS server I've seen just hardcodes
this list), let's just try turning it on. On the off chance it does
break someone, I've left a flag, SSL_set_check_client_certificate_type,
for folks to turn this check off. The flag will most likely be
unnecessary, in which case we can retire it after a few months.
If this does cause a problem, we can opt to turn it off for the default
certificate, or only enable it when multiple certificates are
configured, or lean on the sigalgs list (doesn't work for 1.0/1.1), but
these all result in some slightly suboptimal behavior, so I think we
should treat them as contingency plans.
Update-Note: A TLS 1.2 (or below) client, using client certificates,
connecting to a TLS server which doesn't support its certificate type
will now fail the connection slightly earlier, rather than sending the
certificate and waiting for the server to reject it. The connection
should fail either way, but now it will fail earlier with
SSL_R_UNKNOWN_CERTIFICATE_TYPE. If the server was buggy and did not
correctly advertise its own capabilities (very very unlikely), this may
cause a connection to fail despite previously succeeding. We have
included a temporary API, SSL_set_check_client_certificate_type, to
disable this behavior in the unlikely event this has any impact, but
please contact the BoringSSL team if you need it, as it will interfere
with improvements down the line.
This change does not affect servers requesting client certificates, only
clients sending them.
Bug: 249
Change-Id: I159bc444c4ee79fbe5c476d4253b48d58d2538be
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66687
Commit-Queue: David Benjamin <[email protected]>
Reviewed-by: Bob Beck <[email protected]>
(cherry picked from commit 60c2867092af66bbe369f00d8214b6d06fcb376a)
This is a remnant of when we tried to correlate AEAD selection with post-quantum curves. Also remove a redundant call to tls1_get_shared_group. We already saved the result in hs->new_session, so there's no need to compute it again. Change-Id: I2425ad40bf664f4d248e1dcf610f574a6cad68bc Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66689 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]> (cherry picked from commit 860db9e98f23c6e2692afb143a04987cc232e1f5)
skmcgrail
force-pushed
the
upstream-merge-2024-11-18
branch
from
0fff81b to
899bd22
Compare
nebeid
approved these changes
Dec 4, 2024
justsmth
approved these changes
Dec 4, 2024
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.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.