Skip to content

Replace successful_posthandshake_op with successful_read_op

Michael Catanzaro requested to merge mcatanzaro/successful-posthandshake-op into master

Turns out the current code to guess whether to return G_TLS_ERROR_CERTIFICATE_REQUIRED is racy. With TLS 1.2, the client would see a handshake failure if a client certificate was required but not provided (or not accepted). That's no longer the case with TLS 1.3; instead, the client will see a successful handshake before it sees the server close the connection. GnuTLS is no longer able to indicate why the connection was closed unless the server sends GNUTLS_A_CERTIFICATE_REQUIRED, but we don't support alerts yet, #65. In the meantime, we're stuck using a heuristic to decide whether to return G_TLS_ERROR_CERTIFICATE_REQUIRED: if the server requested a certificate, and we did not provide one, and an operation fails, and we have never successfully done any operation after the handshake, then assume the server rejected our certificate and return G_TLS_ERROR_CERTIFICATE_REQUIRED. This behavior was added in 14be4745.

However, there in a window of time during which the client may see write operations succeed even though it failed to provide an acceptable certificate and the connection is about to be closed by the server. If a write succeeds, then our heuristic to decide whether to return G_TLS_ERROR_CERTIFICATE_REQUIRED fails. So let's change the heuristic to check only for reads instead. A read can never succeed because a properly-implemented server would not write any data before it verifies that the client certificate is acceptable.

This commit fixes 14be4745. It's sort of covered by existing tests, in that we have tests that check for G_TLS_ERROR_CERTIFICATE_REQUIRED, although they don't seem to be tripping this race. We have a libsoup test /ssl/tls-interaction that is really hitting this race, although this change won't be enough to fix that test, because that test needs to be changed to no longer expect the TLS 1.2 behavior.

Merge request reports