refactor: combine TLS1.2 and TLS1.3 sig scheme representations #4498
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lrstewart
commented
Apr 11, 2024
Comment on lines
+379
to
-436
| /* ECDSA */ | ||
| &s2n_ecdsa_sha384, | ||
|
|
||
| /* RSA PSS - TLS 1.3 */ | ||
| &s2n_rsa_pss_pss_sha384, | ||
|
|
||
| /* ECDSA - TLS 1.2 */ | ||
| &s2n_ecdsa_sha384, /* same iana value as TLS 1.3 s2n_ecdsa_secp384r1_sha384 */ |
There was a problem hiding this comment.
It doesn't matter that the TLS1.3 and the TLS1.2 version were separated by RSA-PSS-PSS, because TLS1.2 can't use RSA-PSS-PSS. So s2n_ecdsa_sha384 was always the first TLS1.2 option, and moving it up doesn't change that.
Comment on lines
-445
to
+393
| /* ECDSA - TLS 1.3 */ | ||
| &s2n_ecdsa_secp384r1_sha384, | ||
| /* ECDSA */ | ||
| &s2n_ecdsa_sha384, |
There was a problem hiding this comment.
If you expand the diff, there were no other options between s2n_ecdsa_secp384r1_sha384 and s2n_ecdsa_sha384. Just a big comment.
goatgoose
reviewed
Apr 11, 2024
goatgoose
approved these changes
Apr 12, 2024
toidiu
reviewed
Apr 12, 2024
jmayclin
reviewed
Apr 12, 2024
| @@ -54,60 +54,6 @@ static S2N_RESULT s2n_test_security_policies_compatible(const struct s2n_securit | |||
| return S2N_RESULT_OK; | |||
| } | |||
|
|
|||
| static S2N_RESULT s2n_test_get_missing_duplicate_signature_scheme( | |||
There was a problem hiding this comment.
It served its purpose 😔
jmayclin
approved these changes
Apr 12, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolved issues:
Resolves #3916
Description of changes:
Combine the separate TLS1.2 and TLS1.3 ECDSA signature scheme structs into one single struct that can represent both.
This does not change any behavior because:
Testing:
Deleted tests related to using duplicate IANAs safely, since we don't have duplicate IANAs anymore :) Added a test to enforce that we don't have any duplicate IANAs.
I also added a self-talk test that uses a different curve/hash for every possible use of a curve/hash, to prove they're all properly independent.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.