net/tls: improve handshake failure error reporting (CORE-12666) - #304
net/tls: improve handshake failure error reporting (CORE-12666)#304mnajda-redpanda wants to merge 2 commits into
Conversation
On SSL_ERROR_SSL, do_handshake() only consulted verify() when the error queue held one of three hard-coded reason codes. Reason codes vary with the TLS and OpenSSL version, so key on SSL_get_verify_result() instead. The two no-peer-certificate codes stay explicit, since a missing certificate leaves that result at X509_V_OK. No observable change with current OpenSSL, where SSL_R_CERTIFICATE_VERIFY_FAILED accompanies every non-OK result.
OpenSSL words a received alert as "tlsv1 alert unknown ca", which reads as though the local side found the fault. It is the opposite: the peer rejected the certificate we presented. Prefix "Received TLS alert from peer: " when an ERR_LIB_SSL reason is at or above SSL_AD_REASON_OFFSET; OpenSSL maps a received alert N to reason 1000 + N, so no per-code table is needed. The original text is kept verbatim, so log matchers keep working.
There was a problem hiding this comment.
Pull request overview
Improves OpenSSL-backed TLS handshake failure reporting by clarifying when failures are due to TLS alerts received from the peer, and by gating verify() on SSL_get_verify_result() rather than version-dependent OpenSSL reason codes. Adds loopback-based unit tests to ensure peer-caused alerts preserve their detail and that client-certificate DN reporting remains intact.
Changes:
- Prefixes OpenSSL “alert” error strings (reason codes
>= SSL_AD_REASON_OFFSET) with “Received TLS alert from peer” to clarify directionality. - Adjusts handshake error handling to consult
verify()based onSSL_get_verify_result()(plus explicit “no peer cert” reason codes). - Adds OpenSSL-only loopback tests covering (a) unknown client CA reporting including DN, and (b) preserving received-alert detail without masking it with “no certificate presented by peer”.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
tests/unit/tls_test.cc |
Adds loopback-based unit tests and helpers to validate improved handshake error reporting behavior (OpenSSL backend). |
src/net/tls_openssl.cc |
Updates OpenSSL handshake failure logic and error formatting to better attribute peer alerts and gate verify() appropriately. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@mnajda-redpanda - unless there is something where we urgently need to get this into our fork (e.g., up against a release), please try all seastar changes upstream first, then downstream to here. Otherwise, they often (a) don't get upstreamed, or (b) get upstreamed but with additional changes due to sesatar review, which then complicates the rebase and leads to more work overall. Especially for SSL which is seeing upstream changes also. |
|
Upstream change: scylladb#3593 |
1. Received alerts name the peer.
tlsv1 alert unknown careads as thoughwe found the fault; it means the peer rejected the certificate we presented.
Reasons at or above
SSL_AD_REASON_OFFSETnow get a prefix:2.
verify()gated onSSL_get_verify_result()instead of three hard-codedreason codes, which vary by TLS/OpenSSL version.
Why not the ticket's fix
The ticket reads the incident as a client presenting a certificate with an
unknown CA. It was the other way round:
SSL_R_TLSV1_ALERT_UNKNOWN_CAis raisedwhen we receive an
unknown_ca(48)alert, so the client had rejected thebroker's certificate chain and hung up before sending one of its own.
Two consequences for the proposed fix:
verify()cannot surface what the ticket asked for.
client_auth::REQUIREit would make the message worse.verify()throws
no certificate presented by peer, which describes a consequence ofthe peer's abort rather than its cause, and displaces the alert that actually
explains the failure.
The case the ticket describes — a client presenting an untrusted certificate —
already reported the DN before this PR.
Tests
Loopback-based, OpenSSL backend only:
test_x509_server_rejects_client_cert_from_unknown_ca— DN must reach theerror. Passes before and after; fails if the new gate is removed.
test_server_handshake_error_preserves_ssl_error_detail— received alert mustbe named and
no certificate presented by peermust not appear. Fails beforethis PR.