Clean up selecting a signature algorithm #4285
Conversation
lrstewart
force-pushed
the
sig_schemes_4c
branch
from
270adef to
e9f2366
Compare
lrstewart
force-pushed
the
sig_schemes_4c
branch
from
e9f2366 to
6c072f7
Compare
| ERR_ENTRY(S2N_ERR_INVALID_SIGNATURE_SCHEME, "Invalid signature scheme") \ | ||
| ERR_ENTRY(S2N_ERR_NO_VALID_SIGNATURE_SCHEME, "Unable to negotiate a supported signature scheme") \ |
There was a problem hiding this comment.
I want to differentiate "no valid options to choose from" from "the option chosen by the peer is invalid". I'm not sure about this wording though :/
| /* If only unknown algorithms are offered, expect choosing a scheme to fail for TLS1.3 */ | ||
| conn->actual_protocol_version = S2N_TLS13; | ||
| EXPECT_SUCCESS(s2n_client_signature_algorithms_extension.recv(conn, &signature_algorithms_extension)); | ||
| EXPECT_EQUAL(conn->handshake_params.client_sig_hash_algs.len, sig_hash_algs.len); | ||
| EXPECT_FAILURE(s2n_choose_sig_scheme_from_peer_preference_list(conn, &conn->handshake_params.client_sig_hash_algs, | ||
| &conn->handshake_params.server_cert_sig_scheme)); |
There was a problem hiding this comment.
This isn't actually what is happening in this test. If only unknown algorithms are offered, TLS1.3 should choose a fallback option instead of failing-- see the tests in s2n_signature_algorithms_test.
What's happening here is actually that the default config is being used, so there are no certificates configured. With no certificates, there are no valid signature schemes to choose from. The peer's preferences aren't even considered.
I just removed this test rather than fixing it. We already test this correctly in s2n_signature_algorithms_test, and it isn't really relevant to the extension's parsing.
Co-authored-by: Sam Clark <[email protected]>
Resolved issues:
Related to #4257
Description of changes:
s2n-tls's logic for choosing a signature algorithm is difficult to follow, particularly the handling of defaults. We made it worse with the rather hacky fix for the SHA1 issue.
This cleanup should make the difference between pre-TLS1.2 and TLS1.2 default behavior clearer, and makes our non-RFC fallback logic more explicit. It also makes the signature algorithm selection logic follow the same pattern as our cipher suite selection logic, providing some nice consistency.
Call-outs
I'm only refactoring signature algorithm selection. See #4257 for other outstanding issues with s2n_signature_algorithms.c.
Testing:
I wrote completely new tests. Because I didn't use the old tests, I'd appreciate if reviewers paid particular attention to my new tests.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.