Added certificate signature preferences #2370
Merged
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
Codecov Report
@@ Coverage Diff @@
## main #2370 +/- ##
==========================================
+ Coverage 81.92% 81.96% +0.03%
==========================================
Files 271 271
Lines 18663 18703 +40
==========================================
+ Hits 15290 15330 +40
Misses 3373 3373 |
lrstewart
reviewed
Nov 9, 2020
| @@ -138,20 +151,23 @@ const struct s2n_signature_scheme s2n_rsa_pss_rsae_sha256 = { | |||
| .iana_value = TLS_SIGNATURE_SCHEME_RSA_PSS_RSAE_SHA256, | |||
| .hash_alg = S2N_HASH_SHA256, | |||
| .sig_alg = S2N_SIGNATURE_RSA_PSS_RSAE, | |||
| .libcrypto_nid = NID_rsassaPss, | |||
There was a problem hiding this comment.
Is this the right nid? Why does this id not include the hash algorithm?
There was a problem hiding this comment.
I am still looking into why the id does not include the hash, but this is the correct id.
There was a problem hiding this comment.
Resolved offline; I will make a github issue to track this.
lrstewart
reviewed
Nov 9, 2020
rday
reviewed
Nov 16, 2020
lrstewart
reviewed
Nov 16, 2020
maddeleine
force-pushed
the
sha1_removal
branch
from
9fca4d7
to
508aa96
Compare
lrstewart
requested changes
Nov 25, 2020
| @@ -87,7 +89,7 @@ void s2n_x509_trust_store_wipe(struct s2n_x509_trust_store *store); | |||
| /** Initialize the validator in unsafe mode. No validity checks for OCSP, host checks, or X.509 will be performed. */ | |||
| int s2n_x509_validator_init_no_x509_validation(struct s2n_x509_validator *validator); | |||
|
|
|||
| /** Initialize the validator in safe mode. Will use trust store to validate x.509 cerficiates, ocsp responses, and will call | |||
| /** Initialize the validator in safe mode. Will use trust store to validate x.509 certificates, ocsp responses, and will call | |||
There was a problem hiding this comment.
Loving all these spelling fixes!
tls/s2n_x509_validator.c
Outdated
Comment on lines
567
to
568
| if (conn->config->security_policy->certificate_signature_preferences == NULL) { | ||
| *validation_code = S2N_CERT_OK; | ||
| return S2N_RESULT_OK; | ||
| } |
There was a problem hiding this comment.
What if there's an override policy on the connection? You need to use s2n_connection_get_security_policy to retrieve the policy.
There was a problem hiding this comment.
Also, do you have a test for this case? It's a pretty important case.
lrstewart
reviewed
Nov 25, 2020
ttjsu-aws
reviewed
Nov 30, 2020
maddeleine
force-pushed
the
sha1_removal
branch
from
d55940a
to
c61632a
Compare
lrstewart
reviewed
Dec 1, 2020
lrstewart
approved these changes
Dec 2, 2020
ttjsu-aws
reviewed
Dec 2, 2020
feliperodri
pushed a commit
that referenced
this pull request
Dec 4, 2020
* Added certificate signature preferences
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 #1538 #1442
Description of changes:
This adds a certificate signature preferences struct to our security policies. This struct indicates which signature algorithms the peer will accept on the certificate. The only security policy to take advantage of this is the "default_tls13" policy, which now will not allow SHA-1 legacy signatures in certificates. Also I found a few spelling mistakes.
Call-outs:
Our test certificates aren't named for the signature algorithms they contain, so if you would like to check that I am testing the correct signature algorithms, use the openssl command:
openssl x509 -noout -text -in name_of_certfile.pem | grep "Signature Algorithm"Another call out is that I had to add the Openssl ids for signature schemes as s2n uses Openssl's x509 functions to parse certificates.
Testing:
How is this change tested (unit tests, fuzz tests, etc.)? Are there any testing steps to be verified by the reviewer?
Unit tests, integ tests that use authentication still pass.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.