feat: 'latest' option for strict policy #5488
Conversation
lrstewart
force-pushed
the
build_latest
branch
from
827eb25 to
a252f16
Compare
| DEFER_CLEANUP(struct s2n_test_cert_chain_list cert_chains = { 0 }, | ||
| s2n_test_cert_chains_free); | ||
| EXPECT_OK(s2n_test_cert_chains_init(&cert_chains)); | ||
| EXPECT_OK(s2n_test_cert_chains_set_supported(&cert_chains, | ||
| S2N_PKEY_TYPE_ECDSA, 1)); | ||
| EXPECT_OK(s2n_test_cert_chains_set_supported(&cert_chains, | ||
| S2N_PKEY_TYPE_RSA_PSS, S2N_TEST_CERT_UNSUPPORTED)); | ||
| EXPECT_OK(s2n_test_cert_chains_set_supported(&cert_chains, | ||
| S2N_PKEY_TYPE_MLDSA, S2N_TEST_CERT_UNSUPPORTED)); |
There was a problem hiding this comment.
Note that we are now loading separate copies of each certificate for each of these tests, rather than all tests sharing one set of certificates. This is wildly inefficient, but I've chosen to accept that inefficiency to keep the usage simple. Different tests set different "supported" values. To only load certificates once, we could:
- Make all tests share one s2n_test_cert_chain_list, and reset + reuse the "supported" values between tests (like add a s2n_test_cert_chains_clear_supported method). Dangerous: if we mess up the reuse, then tests can affect each other unexpectedly.
- Separate the "supported" values from the certificates. Like, define
struct s2n_test_supported_cert_chain_list { struct s2n_test_cert_chain_list *cert_chain_list; uint64_t supported[S2N_MAX_TEST_CERT_CHAINS]; }. We'd then be tracking "supported" values and certificates in two different arrays, only tied together by array index. I try to avoid that construction because it's easy to make mistakes. - Do a variant of 2, but with more complicated initialization. We could have one struct with "entry" defined as
{ struct s2n_cert_chain_and_key *chain, const char *name }and one with "entry" defined as{ struct s2n_cert_chain_and_key *chain, const char *name, uint64_t supported }, and we could have a method that constructed the later from the former. That seems like overkill though.
Since this is just a test, I vote we stick to the inefficient but simple and safe way.
| static S2N_RESULT s2n_test_security_policies_compatible_for_policy(const struct s2n_security_policy *policy, | ||
| const struct s2n_security_policy *other_policy, struct s2n_cert_chain_and_key *cert_chain) |
There was a problem hiding this comment.
Since the diff is a bit messy, the summary of my changes to these helper function is: I introduced a "for_policy" version of each one that takes an s2n_security_policy instead of a version string. The original variants of the helper functions now look up the policy from the version and call the "for_policy" variants.
tests/testlib/s2n_test_certs.c
Outdated
| if (!s2n_is_rsa_pss_certs_supported() && strstr(dir->d_name, "pss")) { | ||
| continue; | ||
| } | ||
| if (s2n_libcrypto_is_openssl_fips() && strstr(dir->d_name, "rsae_pkcs_1024_sha1")) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Alternatively, I could not do any checks at all and just treat failed calls to s2n_test_cert_permutation_load_server_chain_from_name as an indicator that a cert isn't supported. After all, we're not trying to test the cert loading process here. That would be simpler, but we'd lose visibility into which certs are available when and could silently ignore meaningful errors.
Release Summary:
Description of changes:
Add a mechanism to mimic the non-versioned behavior of the current defaults for the strict policy. The use case for the strict policy is owned clients and servers, meaning that the compatibility concerns are well defined and testable.
We could consider adding a LATEST for the compatibility policy later, but I have not encountered a customer who needed that yet.
Testing:
Most of this change is about testing. We need to test that changes to LATEST are safe. I extended the testing we currently do for default policies to cover more certificates.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.