Fix handshake failures being reported as authentication failures - #126
Merged
Conversation
`SSLAuthFail` now means a failed handshake on a session created with verification on, where the peer's chain did not verify or the peer sent no certificate when one was required. Everything else at the SSL layer is `SSLError`. `SSL_get_verify_result` alone would not do. A server built with `set_server_verify(true)` whose client arrives with no certificate is the mutual-TLS rejection you turn that flag on to get, and on the verify result alone that server would report `SSLError`. The rule is off for a session created with `set_client_verify(false)`. Turning verification off does not stop OpenSSL verifying a client's peer chain and recording the result, only from aborting over it. A client handed a chain it cannot verify carries that failed result for life, and without the guard every later handshake failure on that session would report `SSLAuthFail`. The error queue gets scanned rather than read at the head, because libcrypto puts an entry there before libssl does — measured at 3 of 3 and 7 of 7. Reason 199 gets pushed alone, so the head was right by accident. The library is checked per entry because 199 is `SSL_R_PEER_DID_NOT_RETURN_A_CERTIFICATE` in libssl and an ASN.1 error in libcrypto. Two failures you would call authentication failures report `SSLError` here: a peer presenting a certificate it cannot prove it holds, and a peer whose certificate will not parse. Both verify clean, and no reason code for them means the same thing on every backend, so covering them needs its own design. The connection closes either way and no plaintext gets delivered, but an application alerting on `auth_failed` loses that alert for the case that most deserves it. Closes #118
SeanTAllen
force-pushed
the
issue-118-ssl-auth-fail
branch
from
August 1, 2026 20:54
92bd01a to
c8aff68
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
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.
SSLAuthFail's docstring was one line: "The peer's certificate could not be verified." A session set that state for every SSL-layer handshake failure, bytes that were not a TLS record and two sessions with no protocol version in common included, with the certificate check reporting no problem. SinceSSLConnectioncalls the wrapped protocol'sauth_failedfor a session in that state, bytes from a peer that spoke no TLS came back to an application as an authentication failure.A failed handshake now reports
SSLAuthFailonly when the session was created with verification on and either the peer's chain did not verify or the peer presented no certificate when one was required. The other placeSSLAuthFailgets set, for a certificate that is not valid for the hostname, runs on a handshake that succeeded and is untouched.SSL.readalready reportedSSLErrorfor the same class of failure and is untouched too.Two authentication failures report
SSLErroron purpose, and that is the part worth arguing about. A peer presenting a certificate it cannot prove it holds, and a peer whose certificate will not parse, both reportedSSLAuthFailonmainand reportSSLErrorhere: the chain verifies, so the verify result isX509_V_OK, and the SSL reasons are 123 and 524301 rather than 199. Certificates are public, so presenting a copy of the real one is what an impersonator without the key does, which means the attacker picks the quieter failure. I left it narrow anyway. A reason set is not portable. On LibreSSL the client-auth direction of that same failure raises an EVP error rather than an SSL one, so the same failure would land in different states on different backends. "Did the peer present a certificate" does not work either: it would fire for an ALPN failure that happens after the certificate arrives, and OpenSSL does not retain the peer certificate after a failed handshake. Covering it properly needs its own design, most likely a verify callback. Nothing is bypassed: the connection closes either way and no plaintext is delivered. But an application that alerts onauth_failedloses that alert for the case that most deserves it. A test pins the boundary, so moving it has to be deliberate.For a wrapped protocol this leaves three outcomes distinguishable where there were two. An authentication failure gives
auth_failedthenclosed. Any other handshake failure givesclosedwith noconnectedoracceptedbefore it, becauseSSLConnectionswallows the TCP-level events and re-issues them only from_poll'sSSLReadyarm. A peer disconnecting normally givesclosedafter one of them. Before this,auth_failedfired for both kinds of failure and said nothing about which. One case stays unreadable and is unchanged here: a peer that rejects the session after it has reachedSSLReadyarrives asconnectedthenclosed.set_server_verifydefaults tofalse, so a defaultSSLConnectionserver never reportsSSLAuthFailnow and never callsauth_failed. It could before. It sends no certificate request, so it has no peer identity to reject.607 of the 759 inserted lines are tests. Eleven tests are new: nine drive
SSLin memory, one runs a realSSLConnectionagainst a plain TCP server answering a TLS client with HTTP, and one is a unit test on the error-code field extraction. Each part of the rule has a test that fails when it is deleted: the untrusted-chain tests on both sides for the verify result, the no-peer-certificate test for the reason scan, the verification-off test for the verify guard, and the non-TLS-bytes, no-shared-version, peer-rejected-our-certificate andSSLConnectiontests for the classification itself. Three things below that are not covered. Four assert a statemainalready produces, and those are what stops someone narrowing the rule too far.The verification-off test does not pin the reason the guard exists.
_peer_auth_failedreturns onnot _verifybefore it reads the verify result, so it passes whether or not OpenSSL recorded one. OpenSSL does record one for a client created withset_client_verify(false). Deleting the guard is what makes that test fail.Three mutations in
_peer_auth_failedno test fails on, and I could not construct one that does: widening the OpenSSL 3.x reason mask to the 1.1.x one, dropping the per-entry library check, and reading only the head instead of scanning the queue. None is reachable through the public API — both masks give the same value for every reason a backend raises, and nothing in the package puts a foreign entry on the queue ahead of a real one on demand. What the new unit test does pin is the extraction the guard rests on: the per-backend shifts, and that_ERRLibrary.oftells libraries apart rather than answeringssl()for everything. The handshake tests catch neither.The suite passes on the five Linux backend configurations CI builds: OpenSSL 1.1.1w, 3.6.2 and 4.0.0, and LibreSSL 3.9.2 and 4.2.1. The two Windows jobs build against LibreSSL 3.9.1, which I did not run locally.
Closes #118