Allow RSA client certs on ECDHE-ECDSA mutual auth#10553
Draft
julek-wolfssl wants to merge 1 commit into
Draft
Conversation
The TLS 1.2 server derived the single advertised ClientCertificateType and the signature_algorithms list in its CertificateRequest from the negotiated cipher suite's own signature algorithm. On an ECDHE-ECDSA suite only ecdsa_sign was offered (and only ECDSA sig algs), so RSA clients could not authenticate even though the server could happily verify an RSA certificate. The same was true in reverse for an RSA server: the CertificateRequest only advertised rsa_sign. Refactor SendCertificateRequest to advertise certificate_types and signature_algorithms covering both sig families when both are compiled in. Three static helpers in internal.c keep the logic in one place without mutating ssl->suites: GetServerCertReqCertTypes - certificate_types to emit GetServerCertReqHashSigAlgo - signature_algorithms to emit InServerCertReqHashSigAlgo - membership check used for verification The advertised lists are written to stack buffers in the caller. To keep DoCertificateVerify in agreement with what we actually sent, the SupportedHashSigAlgo call site there is replaced with InServerCertReqHashSigAlgo, which rebuilds the same list locally and looks up the client's chosen algo. Replace the magic certTypes buffer size with a new MAX_CERT_REQ_CERT_TYPE_CNT constant declared next to ClientCertificateType. Add two end-to-end mutual-auth tests covering both directions: test_tls12_ecdhe_ecdsa_rsa_client_cert - ECDSA server, RSA client test_tls12_ecdhe_rsa_ecdsa_client_cert - RSA server, ECDSA client Update test_certreq_sighash_algos to permit RSA / RSA-PSS sig algs in the ECDHE-ECDSA CertificateRequest; the previous assertion locked in the ECDSA-only behaviour that this change corrects. TLS 1.3 is unaffected: RFC 8446 removed certificate_types from CertificateRequest, and TLS 1.3 cipher suites do not bind a signature algorithm, so the server's hashSigAlgo already covers both sig families when either has been compiled in.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes TLS 1.2 mutual-auth interoperability where the server’s CertificateRequest was overly tied to the negotiated cipher suite’s signature algorithm (e.g., ECDHE-ECDSA only advertising ECDSA), preventing clients with the “other” certificate type (RSA/ECDSA) from authenticating even though verification support exists.
Changes:
- Refactors TLS 1.2
SendCertificateRequestlogic to advertise broadercertificate_typesandsignature_algorithms(RSA + ECDSA when available), and alignsCertificateVerifyvalidation with what was advertised. - Introduces
MAX_CERT_REQ_CERT_TYPE_CNTfor sizing the emittedcertificate_typeslist. - Adds two end-to-end API tests for mixed RSA/ECDSA mutual-auth, and relaxes an existing test that previously enforced ECDSA-only behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
wolfssl/internal.h |
Adds MAX_CERT_REQ_CERT_TYPE_CNT constant for TLS 1.2 CertificateRequest sizing. |
src/internal.c |
Implements helpers to build certificate_types/signature_algorithms, updates CertificateRequest emission and CertificateVerify validation. |
tests/api/test_tls.h |
Registers two new TLS 1.2 mutual-auth test entry points. |
tests/api/test_tls.c |
Adds ECDSA-server/RSA-client and RSA-server/ECDSA-client mutual-auth tests. |
tests/api.c |
Updates test_certreq_sighash_algos expectations to allow RSA/RSA-PSS in ECDHE-ECDSA CertificateRequest. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+25857
to
+25862
| { | ||
| certTypes[n++] = rsa_sign; | ||
| #ifdef HAVE_ECC | ||
| certTypes[n++] = ecdsa_sign; | ||
| #endif | ||
| } |
Comment on lines
+25889
to
+25895
| InitSuitesHashSigAlgo(hashSigAlgo, haveSig, 1, 0, | ||
| ssl->buffers.keySz, &localSz); | ||
| if (localSz > suites->hashSigAlgoSz) { | ||
| *hashSigAlgoSz = localSz; | ||
| return; | ||
| } | ||
| } |
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
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.
The TLS 1.2 server derived the single advertised ClientCertificateType
and the signature_algorithms list in its CertificateRequest from the
negotiated cipher suite's own signature algorithm. On an ECDHE-ECDSA
suite only ecdsa_sign was offered (and only ECDSA sig algs), so RSA
clients could not authenticate even though the server could happily
verify an RSA certificate. The same was true in reverse for an RSA
server: the CertificateRequest only advertised rsa_sign.
Refactor SendCertificateRequest to advertise certificate_types and
signature_algorithms covering both sig families when both are compiled
in. Three static helpers in internal.c keep the logic in one place
without mutating ssl->suites:
GetServerCertReqCertTypes - certificate_types to emit
GetServerCertReqHashSigAlgo - signature_algorithms to emit
InServerCertReqHashSigAlgo - membership check used for verification
The advertised lists are written to stack buffers in the caller. To
keep DoCertificateVerify in agreement with what we actually sent, the
SupportedHashSigAlgo call site there is replaced with
InServerCertReqHashSigAlgo, which rebuilds the same list locally and
looks up the client's chosen algo.
Replace the magic certTypes buffer size with a new
MAX_CERT_REQ_CERT_TYPE_CNT constant declared next to
ClientCertificateType.
Add two end-to-end mutual-auth tests covering both directions:
test_tls12_ecdhe_ecdsa_rsa_client_cert - ECDSA server, RSA client
test_tls12_ecdhe_rsa_ecdsa_client_cert - RSA server, ECDSA client
Update test_certreq_sighash_algos to permit RSA / RSA-PSS sig algs in
the ECDHE-ECDSA CertificateRequest; the previous assertion locked in
the ECDSA-only behaviour that this change corrects.
TLS 1.3 is unaffected: RFC 8446 removed certificate_types from
CertificateRequest, and TLS 1.3 cipher suites do not bind a signature
algorithm, so the server's hashSigAlgo already covers both sig
families when either has been compiled in.