test: pin tests to TLS 1.2 policy #4926
Conversation
| @@ -63,13 +64,15 @@ mod tests { | |||
| } | |||
|
|
|||
| #[test] | |||
| fn resume_session() -> Result<(), Box<dyn Error>> { | |||
| fn resume_tls12_session() -> Result<(), Box<dyn Error>> { | |||
There was a problem hiding this comment.
There is a test named resume_tls13_session below and this one is tls1.2 specific.
tests/unit/s2n_self_talk_ktls_test.c
Outdated
There was a problem hiding this comment.
TLS1.3 test is done below and explicitly sets the default_tls13 policy.
There was a problem hiding this comment.
These were mean to only support TLS 1.2. We toggle on fips support to maintain current behavior.
| let tls12 = handshake_with_domain(domain, "default").await?; | ||
| let tls12 = handshake_with_domain(domain, "20240501").await?; | ||
| assert_eq!(tls12.as_ref().actual_protocol_version()?, Version::TLS12); |
There was a problem hiding this comment.
We want to test TLS1.2 here
There was a problem hiding this comment.
This test were not written to work with a fips security policy so this maintains the current behavior by pinning test to "default_tls13".
There was a problem hiding this comment.
This tests the following ecc preferences and requires a security policy which supports all of them (aka non-FIPS). Once we switch to TLS1.3 by default, it will be possible to use a FIPS TLS1.3 policy by default, which breaks this test.
- p256
- x25519 (not supported with a FIPS security policy)
- p384
| pub(crate) const TESTING_TLS12: Policy = policy!("20240501"); | ||
|
|
There was a problem hiding this comment.
Adding this didn't help as much as I thought it would :/ You've still got the integration tests and the tokio tests using the magic number. Maybe a pub policy is worth it.
There was a problem hiding this comment.
I think we should think hard before exposing more "default" policies, especially from the Rust bindings. For the moment, I have created a TESTING_TLS12 policy gated to the unstable-testing feature. I consider this a 2-way decision so we can always change it if we need.
| DEFER_CLEANUP(struct s2n_config *config = s2n_config_new(), | ||
| s2n_config_ptr_free); | ||
| EXPECT_NOT_NULL(config); | ||
| EXPECT_SUCCESS(s2n_config_set_cipher_preferences(config, "default_tls13")); |
There was a problem hiding this comment.
Nit: Why bother creating a config? Couldn't you just use s2n_connection_set_cipher_preferences?
There was a problem hiding this comment.
s2n_connection_set_cipher_preferences doesnt work.
s2n_connection_set_cipher_preferences sets the conn->security_policy_override field but it get overridden by this test which is also directly modifying the field.
| DEFAULT_TLS13, | ||
| TESTING_TLS12, |
There was a problem hiding this comment.
Rust doesn't get mad at you for putting a non-pub struct in a pub list?
There was a problem hiding this comment.
Nope. I couldn't find docs but experimenting showed that its essentially ok to pub export a pub(crate). It does complain if its "private" i.e. not-pub
#4765
Description of changes:
Pinning some tests to a numbered TLS1.2 policy in preparation for default TLS1.3 support.
Testing:
Existing tests should pass.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.