feat: add cert key preferences #4434
Conversation
- remove unused struct field - remove incorrect comment - move validation to openssl_x509 get info - move apply local check up in the check root cert function - refactor x509 test to use explicit expected info
- add more explicit comment for valid chain scope
- move valid cert case into test loop
tls/s2n_x509_validator.c
Outdated
| struct s2n_cert_info info = { 0 }; | ||
| RESULT_GUARD(s2n_openssl_x509_get_cert_info(root, &info)); | ||
|
|
||
| RESULT_ENSURE_OK(s2n_security_policy_validate_cert_key(sp, &info), S2N_ERR_CERT_UNTRUSTED); |
There was a problem hiding this comment.
Is this a protocol error or a usage error? It kind of seems like a usage error, since the invalid cert should have never been loaded into the trust store. Not sure, though. Would S2N_ERR_SECURITY_POLICY_INCOMPATIBLE_CERT potentially be more accurate?
There was a problem hiding this comment.
I think usage error makes more since. And I agree that S2N_ERR_SECURITY_POLICY_INCOMPATIBLE_CERT is more descriptive, so I went ahead and switched it to that.
How stingy are we with adding new Err's? The current description for that err is
"Incompatibility found between loaded certificates and chosen security policy"
which is technically accurate, but I think it might be nice to have a more explicit S2N_ERR_SECURITY_POLICY_INCOMPATIBLE_ROOT error for this situation, since I would imagine that some customers might not be thinking of the certs in their trust store as "loaded".
There was a problem hiding this comment.
Hm I feel like it does makes sense to describe certs as being "loaded" into the trust store, but that could definitely just be me. I don't think it would be bad to add a new error if you think it would be more clear.
If you do add a new error I would maybe go with something like INCOMPATIBLE_TRUSTED_CERT/TRUST_STORE_CERT since a cert in the trust store doesn't have to be a root.
- revert accidental sig scheme changes - redefine buffers in each scope - move validate cert chain test out of loop scope - return different error code for failed root
- remove unnecessary uint16_t max check
- represent bits using int - add approrpiate >0 check
tls/s2n_x509_validator.c
Outdated
| struct s2n_cert_info info = { 0 }; | ||
| RESULT_GUARD(s2n_openssl_x509_get_cert_info(root, &info)); | ||
|
|
||
| RESULT_ENSURE_OK(s2n_security_policy_validate_cert_key(sp, &info), S2N_ERR_CERT_UNTRUSTED); |
There was a problem hiding this comment.
Hm I feel like it does makes sense to describe certs as being "loaded" into the trust store, but that could definitely just be me. I don't think it would be bad to add a new error if you think it would be more clear.
If you do add a new error I would maybe go with something like INCOMPATIBLE_TRUSTED_CERT/TRUST_STORE_CERT since a cert in the trust store doesn't have to be a root.
- add null check for the ec_group before calling get_curve_name - use get0_pubkey rather that doesn't increment reference count
- remove unused cleanup function
- clang format
| /* modulus for RSA key, size for EC key */ | ||
| uint16_t bits; |
There was a problem hiding this comment.
I remember we had this conversation, but remind me: why use this definition of "bits" rather than the standard "bits of security" so that we can define it the same across key types? And is there a reason we need to enforce per-type and not a global "bits of security" minimum?
There was a problem hiding this comment.
is there a reason we need to enforce per-type and not a global "bits of security" minimum?
Government compliance regimes might not agree with OpenSSL's, or even their own, definition of security or they might have some other motivations. For example, RSA3072 and EC256 both provide 128 bits of security, but RFC9151 only allows RSA3072, and does not allow EC256. This prevents us from configuring some global minimum.
why use this definition of "bits" rather than the standard "bits of security"
This "bits" value is only technically necessary for distinguishing between different RSA keys, and it seemed much more readable to check that s2n_rsa_rsae_2048 is using 2048 rather than 112
- revert get0_pubkey to address ci failures - remove naught semicolon causing ci failures - use variable in sizeof invocation rather than type - pass in length to test fixture constructor - make test fixtures const - rename validator test case - directly rely on stdint - correct prefix on header include - correct sloppy variable name - add additionaly ensure ref on output of function
- move sha1 check more explicitly behind the conditions - don't use get0_ec_key since it's not available everywhere - ensure on != NID_undef - pass in error to verify functions - bail with a decode error if the pubkey is null
- assert root cert key checks whenever cert key preferences are defined - don't rely on silly initializer lists/memcmp - change test case for new behavior in root key check
- clang format
- add note about unresolved security policy structure
- add note about unresolved security policy structure - try different compile definition to gate tests
- clang-format. I just set this up to format on save so we'll see how that goes.
Co-authored-by: Sam Clark <3758302+goatgoose@users.noreply.github.com>
- remove unnecessary test + sp. Now that root cert validation is no longer conditional, remove negative test.
Description of changes:
s2n-tls does not currently offer a way to define cert key preferences in cert policies, this PR adds them.
These cert key preferences can apply in 3 difference places
While cert signature preferences were no enforced against self signed certs, the key type of a self signed cert does influence the security of the chain so we do enforce the cert key preferences.
Call-outs:
There was previous discussion about using the openssl
verify_cbfor enforcing the root preferences, but that turns out to have little benefit since the libcrypto's I looked at where not capable of backtracking and trying alternate chains given a failed verify callback. Instead we prefer to use the more boringget_chainAPI.CI won't pass until #4433 is merged into mainline
Testing:
New unit tests were added to cover new functionality.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.