test: pin tests to TLS 1.2/TLS 1.3 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
| 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.
There was a problem hiding this comment.
The connection policy always takes precedence over the config policy. So if the test is overriding the connection policy, why is setting the config policy working?
There was a problem hiding this comment.
Yea good point. Let me check if I was setting the cipher pref after the testing override behavior.
There was a problem hiding this comment.
Yeah if we have to do config here instead of connection that's fine, but I'm now worried about WHY we have to do config. I think we should confirm that before merging this.
There was a problem hiding this comment.
The test fix requires setting the config because the actual test case is setting client->security_policy_override to NULL. This means that we end up using client->config->ecc_prefs and the ecc_prefs for a FIPS policy doesn't have s2n_ecc_curve_x25519
Here is a detailed explanation of whats happening: https://gist.github.com/toidiu/c3ddeca499ba3bcf83328520b8a18cc4
There was a problem hiding this comment.
That makes much more sense! Thanks for confirming.
That makes this test kind of tricky though. The test isn't just relying on the policy supporting x25519 and TLS1.3, it's also relying on the policy not supporting pq. And I don't think it'd fail if that assumption isn't met. I wonder if it'd be safer to use a versioned policy instead of "default_tls13", and to leave a comment explaining why that policy was chosen. I'm imagining if/when we want to someday add pq to the default policy 🙃
To be clear, you're not making the test worse by using "default_tls13". The test already was missing safeguards around its policy assumptions.
There was a problem hiding this comment.
I was considering "default_tls13" deprecated but you can never be certain of that. updated in 371ceec
toidiu
changed the title
#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.