Use NIST P-256 for key generation when client do not specify curve #2265
Conversation
- Also add tests to ensure P-256 is included in ecc preference in all valid usable security policies
Codecov Report
@@ Coverage Diff @@
## main #2265 +/- ##
===========================================
+ Coverage 38.57% 80.72% +42.14%
===========================================
Files 164 265 +101
Lines 12626 17879 +5253
===========================================
+ Hits 4871 14433 +9562
+ Misses 7755 3446 -4309 |
| EXPECT_NOT_NULL(client_conn = s2n_connection_new(S2N_CLIENT)); | ||
| EXPECT_SUCCESS(s2n_connection_set_config(client_conn, tls12_config)); | ||
| EXPECT_SUCCESS(s2n_client_hello_send(client_conn)); | ||
| EXPECT_EQUAL(client_conn->client_protocol_version, S2N_TLS12); | ||
| EXPECT_EQUAL(client_conn->client_hello_version, S2N_TLS12); |
There was a problem hiding this comment.
Why store this as a blob instead of just executing it as part of the test? What's the benefit?
There was a problem hiding this comment.
it is not possible to execute the current test with code unless we can expose not sending supported_groups extension
tls/s2n_security_policies.c
Outdated
| GUARD(s2n_check_ecc_preferences_curves_list(ecc_preference)); | ||
| if (security_policy != &security_policy_null) { |
There was a problem hiding this comment.
Nit: Might want a newline between line 638 and line 639
tls/s2n_security_policies.c
Outdated
| @@ -636,6 +636,11 @@ int s2n_security_policies_init() | |||
| const struct s2n_ecc_preferences *ecc_preference = security_policy->ecc_preferences; | |||
| notnull_check(ecc_preference); | |||
| GUARD(s2n_check_ecc_preferences_curves_list(ecc_preference)); | |||
| if (security_policy != &security_policy_null) { | |||
| /* catch any offending security policy that does not support P-256 */ | |||
| GUARD_AS_POSIX(s2n_ecc_preferences_includes_p256(ecc_preference)); | |||
There was a problem hiding this comment.
Why does this require a separate method? Why not just use s2n_ecc_preferences_includes_curve directly?
There was a problem hiding this comment.
And why do this is production code instead of in a unit test that iterates over all security policies?
There was a problem hiding this comment.
Why does this require a separate method? Why not just use s2n_ecc_preferences_includes_curve directly?
I can use s2n_ecc_preferences_includes_curve() if that's preferred
And why do this is production code instead of in a unit test that iterates over all security policies?
- this is where all security policies is in scope
- this is inline with checks done in this security_policies block. even though it has a one time initialization cost during runtime, but a violation of this rule will also be caught during unit tests in this function
Resolved issues:
resolves #2261 (using solution from option 3)
Description of changes:
Use NIST P-256 as the fallback key generation selection when client do not specify curve
Also add tests to ensure P-256 is included in ecc preferences for all
valid usable security policies
Testing:
Unit tests along with client hello receive test against a client_hello with no supported_groups extension.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.