feat: apply cert signature preferences locally #4407
Conversation
tls/s2n_config.c
Outdated
| if (!security_policy->certificate_preferences_apply_locally | ||
| || security_policy->certificate_signature_preferences == NULL) { | ||
| return S2N_RESULT_OK; | ||
| } |
There was a problem hiding this comment.
Not sure if it's worth removing the optimization, but I feel like it would make more sense for this check to be in s2n_security_policy_validate_certificate_chain. It seems a bit strange for validate_certificate_chain to fail even when certificate_preferences_apply_locally is false. And s2n_config_validate_certificate_preferences shouldn't really care about these fields since it calls validate_certificate_chain.
There was a problem hiding this comment.
Let me go ahead and benchmark it. I think the point about moving the null check makes sense, so as long as there isn't a big impact on the performance I'll go ahead and make the change.
There was a problem hiding this comment.
It only saves about 15 nanoseconds, so I'll go ahead and remove it. But ran into some other issues with that, which are addressed in #4416
- spacing/spelling corrections - add specific errno for securityu policy violations - use EXPECT_FAILURE_WITH_ERRNO instead of EXPECT_FAILURE - use EXPECT_NOT_NULL in more places - fix the missing trailing comma - unconditionally validate the domain map
- remove shadowed variables - use else statement to help beleaguered cppcheck see the light
* replace missed EXPECT_ERROR * fix lines breaks for comments * refactor unit tests to focus on function behavior, not semantic cases * refactor s2n_config_validate test to rely on internal methods
* rename config_validate_stored_certificates * remove unused variable
- move line breaks to 120 characters - manually break lines instead of using clang-format - move null check and locality checks interior to validate methods - remove documentation of test assumptions
- clang-format
- address compiler errors. I should really rebase this to mainline so I can use the nice features that I've added around cmake and Werror.
- use specific error message for local validation failure - return existing on-descript error message for peer cert failure - add test case to ensure failure message doesn't change - address CI failure by removing internal method
- remove shadowed variable - push condition checks inward
tls/s2n_config.c
Outdated
| /* Validate that the certificate type is allowed by the security policy. */ | ||
| POSIX_GUARD_RESULT(s2n_security_policy_validate_certificate_chain(config->security_policy, cert_key_pair)); | ||
|
|
There was a problem hiding this comment.
Doesn't this introduce potential ordering issues when building configs?
- If I add my certificates before setting my security policy, those certificates are validated against the default security policy. So default policies can never enforce certificate restrictions.
- If I want to swap out my security policy later, and the old and new policies have different certificate restrictions, I may be stuck and have to just create an entirely new config.
In general we've tried to avoid checks like this because 1) You have to make sure you cover every entry point 2) Ordering of calls starts to matter. In the past, we've solved the problem by not validating the config until you try to set it on a connection. We don't have a config builder, but setting it on a connection essentially finalizes it.
There was a problem hiding this comment.
I removed the checks for load_cert and s2n_config_set_cipher_preferences, and pushed those validations to s2n_connection_set_config.
I left the check for s2n_connection_set_cipher_preference in because otherwise we'd be missing the case where
- a config is set on the connection
- a strict security policy override is set on the connection
But this does introduce an ordering dependency. Let me know if you'd prefer to remove it
- don't validate cert chain at load time - don't validate config when setting the policy - don't validate connection when setting the policy - always evaluate when setting a config on a connection - remove no longer applicable unit tests
|
ASAN failures should be addressed in #4430 |
- different if/else construction to avoid cppcheck
- remove change from rfc9151 policy - modify test cases to account for rfc9151 no longer applying locally - apply validation to local certs as well - simplify the security policy retrieval in s2n_connection
| /* restore root values */ | ||
| root.info = valid_root; |
There was a problem hiding this comment.
Nit: This is probably fine, but edging towards dangerous. Similar to the previous comment: tests should share as little state as possible, and what state they do share should be const if at all possible.
- remove typo/comment - restore security policy table state
- clang format
- add null check for static analyzers
- const correctness in pointer swapping
Part of #4339
Description of changes:
This PR adds a new security-policy feature that allow certificate signature preferences to be applied locally.
This
cert_descriptionfield introduced in the previous PR is now stored ons2n_certobjects when they are created. When ans2n_certis actually added to a config, we validate thecert_descriptionto ensure that it is allowed by the security policy certificate preferencesThis PR also introduced an
s2n_config_validate_loaded_certsmethod. This is needed because a customer might load certificates into their config before they set a cipher preferences. When they set they cipher preference, we need a way to confirm that the currently loaded certificates are accepted by the security policy certificate preferences.Call Outs:
I had to change the order of field initialization for the static configs. Because setting the security policy now required validating the other fields in the config, setting the security policy must be the last step so that all of the fields are valid.
Testing:
All existing tests should pass, and new unit tests were added for new functionality.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.