tests: use sig schemes as source of truth for valid hash+sig algs #5129
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
force-pushed
the
openssl3fips_evp_validalg
branch
6 times, most recently
from
7ee526a to
ddf0c81
Compare
lrstewart
force-pushed
the
openssl3fips_evp_validalg
branch
from
ddf0c81 to
50f9458
Compare
lrstewart
commented
Feb 20, 2025
| #include "tls/s2n_signature_scheme.c" | ||
| #include "tls/s2n_signature_scheme.h" |
There was a problem hiding this comment.
This typo made me think I was going insane.
My pointer comparisons in this test weren't working in the OSX github action, and ONLY in the OSX github action. I couldn't repro locally. Like sig_scheme == all_prefs->signature_schemes[all_i] was false for the same signature scheme. I added print statements to print the pointer addresses. They were different?!
It was because of this typo. s2n_signature_preferences_all was the duplicate copy from this include, using its own local copies of the signature schemes. The signature schemes from security_policy_selection were the originals used by the rest of the library -_-
The best part is that I have only myself to blame 😔
maddeleine
reviewed
Feb 20, 2025
maddeleine
approved these changes
Feb 21, 2025
CarolYeh910
approved these changes
Feb 21, 2025
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.
Release Summary:
Resolved issues:
related to #5105
Description of changes:
Fixing an issue that keeps causing problems in my hash and signing PRs.
It's hard to define "all valid hash algs" in the context of signing. Different libcryptos decide to forbid different combinations, like no MD5 at all or no MD5+SHA1 for ECDSA. We have more than one test trying to define those rules, and the reasoning behind those definitions aren't particularly clear.
Rather than try to separately define all those banned combinations, we should just treat "all valid hash algs" as all the hash algs for which there exists a signature scheme. Signature schemes are the real source of truth for all allowed hash alg + sig scheme combinations. They're the only combinations we'll ever need to care about.
Call-outs:
The one "miss" here is that we no longer check that signing methods flag "invalid" algs as invalid. I'm not particularly concerned about that. Without a signature scheme to trigger the signing process, those methods will never get any "invalid" combination in the first place. Also, the libcryptos will happily reject what they define as invalid combos 🙃
Testing:
This is a testing change
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.