Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests: pin tests to a numbered TLS1.2 policy #4905

Merged
merged 2 commits into from
Nov 19, 2024
Merged

Conversation

toidiu
Copy link
Contributor

@toidiu toidiu commented Nov 19, 2024

#4765

Description of changes:

This PR pins some tests that were relying on the "default" security policy to a numbered TLS1.2 policy. This paves the path to using TLS1.3 by default.

I have added comments to each file justifying why each change is safe.

Testing:

Existing tests should all pass.

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 Nov 19, 2024
Copy link
Contributor Author

@toidiu toidiu Nov 19, 2024

Choose a reason for hiding this comment

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

Test comment mentions TLS1.2

Comment on lines 595 to +598
/* Self-talk: Test interaction between TLS1.2 session resumption and serialization */
{
DEFER_CLEANUP(struct s2n_config *resumption_config = s2n_config_new(), s2n_config_ptr_free);
EXPECT_SUCCESS(s2n_config_set_cipher_preferences(resumption_config, "20240501"));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test comment mentions TLS1.2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renegotiation is only supported for TLS1.2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renegotiation is only supported for TLS1.2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

test comment mentions TLS1.2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

EMS supported for TLS1.2

@@ -759,7 +759,7 @@ int main(int argc, char **argv)
struct s2n_connection *conn = NULL;
EXPECT_NOT_NULL(conn = s2n_connection_new(S2N_CLIENT));
EXPECT_SUCCESS(s2n_connection_set_config(conn, config));
EXPECT_SUCCESS(s2n_connection_set_cipher_preferences(conn, "default"));
EXPECT_SUCCESS(s2n_connection_set_cipher_preferences(conn, "20240501"));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

test comment specifies TLS1.2

Comment on lines +1449 to +1457
EXPECT_SUCCESS(s2n_config_set_cipher_preferences(client_configuration, "20240501"));

DEFER_CLEANUP(struct s2n_config *server_configuration = s2n_config_new(),
s2n_config_ptr_free);
EXPECT_NOT_NULL(server_configuration);
EXPECT_SUCCESS(s2n_config_set_session_tickets_onoff(server_configuration, 1));
EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(server_configuration,
chain_and_key));
EXPECT_SUCCESS(s2n_config_set_cipher_preferences(server_configuration, "20240501"));
Copy link
Contributor Author

@toidiu toidiu Nov 19, 2024

Choose a reason for hiding this comment

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

Test checks for TLS1.2 below

EXPECT_SUCCESS(s2n_connection_set_cipher_preferences(server_conn, "default"));
EXPECT_SUCCESS(s2n_connection_set_cipher_preferences(server_conn, "20240501"));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test checks for TLS1.2 below

EXPECT_SUCCESS(s2n_config_set_cipher_preferences(config, "default"));
EXPECT_SUCCESS(s2n_config_set_cipher_preferences(config, "20240501"));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test checks for TLS1.2 below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

npn is supported for TLS1.2

@@ -78,6 +78,7 @@ int main(int argc, char **argv)
S2N_DEFAULT_TEST_CERT_CHAIN, S2N_DEFAULT_TEST_PRIVATE_KEY));

DEFER_CLEANUP(struct s2n_config *tls12_config = s2n_config_new(), s2n_config_ptr_free);
EXPECT_SUCCESS(s2n_config_set_cipher_preferences(tls12_config, "20240501"));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name of the config is tls12_config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests are all TLS1.2 specific eg. "Test that we ignore Warning Alerts in S2N_ALERT_IGNORE_WARNINGS mode in TLS1.2"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hello request is only supported in TLS1.2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test checks that TLS1.2 was negotiated

@toidiu toidiu marked this pull request as ready for review November 19, 2024 01:32
@toidiu toidiu requested a review from goatgoose November 19, 2024 01:33
@toidiu toidiu requested review from lrstewart and jmayclin and removed request for jmayclin November 19, 2024 17:30
@toidiu toidiu enabled auto-merge (squash) November 19, 2024 19:24
@toidiu toidiu merged commit e826931 into aws:main Nov 19, 2024
37 checks passed
@toidiu toidiu deleted the ak-pinUnit branch November 19, 2024 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants