Skip to content

fix: no server signature scheme expected with rsa kex#5481

Merged
lrstewart merged 2 commits intoaws:mainfrom
lrstewart:nosig
Aug 26, 2025
Merged

fix: no server signature scheme expected with rsa kex#5481
lrstewart merged 2 commits intoaws:mainfrom
lrstewart:nosig

Conversation

@lrstewart
Copy link
Copy Markdown
Contributor

@lrstewart lrstewart commented Aug 25, 2025 •
edited
Loading

Release Summary:

Resolved issues:

resolves #5477

Description of changes:

s2n-tls was choosing a server signature scheme even when no server signature is necessary due to RSA kex. This change fixes that.

Call-outs:

We could instead just fix all the getters that report server signature scheme, but I don't think that's the right choice because:

  1. We would have to apply the same fix to all getters. There are currently 2, I'm adding a 3rd in another PR, there may be more someday.
  2. Future internal logic might assume a server signature scheme and not account for none
  3. Client and server will not agree. The client correctly marks all these situations as "none" because the server never sends it a server signature.

Testing:

In addition to the new unit tests, I repeated the manual s2nc/s2nd test from the issue. New server output:

s2nd localhost 8000 --ciphers test_all_rsa_kex
libcrypto: AWS-LC 1.49.1
Listening on localhost:8000
CONNECTED:
Handshake: NEGOTIATED|FULL_HANDSHAKE|WITH_SESSION_TICKET
Client hello version: 33
Client protocol version: 33
Server protocol version: 33
Actual protocol version: 33
Server name: localhost
Curve: NONE
Cipher negotiated: AES128-SHA
Server signature negotiated: None+None <-- CORRECT NOW
Early Data status: NOT REQUESTED
JA3: a24955ad4bc89eb0c960d34b2a6f1486
Wire bytes in: 498
Wire bytes out: 2704
s2n is ready

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added the s2n-core team label Aug 25, 2025
@lrstewart lrstewart marked this pull request as ready for review August 25, 2025 21:36
lrstewart
lrstewart commented Aug 25, 2025
View reviewed changes
tests/testlib/s2n_testlib.h
Comment on lines 223 to 229
int s2n_test_cert_permutation_load_server_chain(struct s2n_cert_chain_and_key **chain_and_key,
const char *type, const char *siganture, const char *size, const char *digest);
const char *type, const char *signature, const char *size, const char *digest);

int s2n_test_cert_permutation_get_ca_path(char *output, const char *type, const char *siganture,
int s2n_test_cert_permutation_get_ca_path(char *output, const char *type, const char *signature,
const char *size, const char *digest);
S2N_RESULT s2n_test_cert_permutation_get_server_chain_path(char *output, const char *type,
const char *siganture, const char *size, const char *digest);
const char *signature, const char *size, const char *digest);

Copy link
Copy Markdown
Contributor Author

@lrstewart lrstewart Aug 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Irrelevant to this PR, it just bugged me. If it confuses the diff I can remove it.

tests/unit/s2n_client_signature_algorithms_extension_test.c
Comment on lines +84 to 85
conn->secure->cipher_suite = &s2n_ecdhe_rsa_with_aes_256_gcm_sha384;

Copy link
Copy Markdown
Contributor Author

@lrstewart lrstewart Aug 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "null" cipher suite "uses" RSA kex:

s2n-tls/tls/s2n_cipher_suites.c

Lines 210 to 218 in 795f4dc

struct s2n_cipher_suite s2n_null_cipher_suite = {
.available = 1,
.name = "TLS_NULL_WITH_NULL_NULL",
.iana_name = "TLS_NULL_WITH_NULL_NULL",
.iana_value = { TLS_NULL_WITH_NULL_NULL },
.key_exchange_alg = &s2n_rsa,
.auth_method = S2N_AUTHENTICATION_RSA,
.record_alg = &s2n_record_alg_null,
};
So I explicitly set a cipher suite.

@lrstewart lrstewart added this pull request to the merge queue Aug 26, 2025
Merged via the queue into aws:main with commit d83ebbb Aug 26, 2025
49 checks passed
@lrstewart lrstewart deleted the nosig branch August 26, 2025 23:08
@BrewTestBot BrewTestBot mentioned this pull request Sep 15, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jmayclin jmayclin jmayclin approved these changes

+1 more reviewer

@goatgoose goatgoose goatgoose approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

s2n-core team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

s2n-tls server reports signature scheme with RSA kex

3 participants

@lrstewart @goatgoose @jmayclin