Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: don't iterate over certs if not validating certs #4797

Merged
merged 3 commits into from
Sep 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions tests/unit/s2n_config_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -1147,6 +1147,43 @@ int main(int argc, char **argv)
EXPECT_ERROR_WITH_ERRNO(s2n_config_validate_loaded_certificates(config, &rfc9151_applied_locally),
S2N_ERR_SECURITY_POLICY_INCOMPATIBLE_CERT);
};

/* when cert preferences don't apply locally, certs in domain map are not iterated over
*
* Some customers load large numbers of certificates, so even iterating
* over every certificate without performing any validation is expensive.
*/
{
struct s2n_security_policy non_local_rfc9151 = security_policy_rfc9151;

/* Assert that the security policy WOULD apply,
* if certificate_preferences_apply_locally was true.
*/
EXPECT_NOT_NULL(non_local_rfc9151.certificate_key_preferences);
EXPECT_NOT_NULL(non_local_rfc9151.certificate_signature_preferences);
EXPECT_TRUE(non_local_rfc9151.certificate_key_preferences->count > 0);
EXPECT_TRUE(non_local_rfc9151.certificate_signature_preferences->count > 0);

DEFER_CLEANUP(struct s2n_config *config = s2n_config_new(), s2n_config_ptr_free);
EXPECT_NOT_NULL(config);
EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(config, valid_cert));

/* Invalidate the domain map so that attempting to use it will trigger
* an error. We want to ensure that we DON'T use it.
* Iterating over a map requires that map to be immutable / complete.
*/
EXPECT_OK(s2n_map_unlock(config->domain_name_to_cert_map));

/* Control case: if local validation needed, attempt to use invalid domain map */
non_local_rfc9151.certificate_preferences_apply_locally = true;
EXPECT_ERROR_WITH_ERRNO(
s2n_config_validate_loaded_certificates(config, &non_local_rfc9151),
S2N_ERR_MAP_MUTABLE);

/* Test case: if no local validation needed, do not use invalid domain map */
non_local_rfc9151.certificate_preferences_apply_locally = false;
EXPECT_OK(s2n_config_validate_loaded_certificates(config, &non_local_rfc9151));
};
};

/* Checks that servers don't use a config before the client hello callback is executed.
Expand Down
9 changes: 9 additions & 0 deletions tls/s2n_config.c
Original file line number Diff line number Diff line change
Expand Up @@ -574,6 +574,15 @@ S2N_RESULT s2n_config_validate_loaded_certificates(const struct s2n_config *conf
return S2N_RESULT_OK;
}

/* Duplicates a check in s2n_security_policy_validate_certificate_chain.
* If a large number of certificates are configured, even iterating
* over the chains to call s2n_security_policy_validate_certificate_chain
* could be prohibitively expensive.
*/
if (!security_policy->certificate_preferences_apply_locally) {
return S2N_RESULT_OK;
}

/* validate the default certs */
for (int i = 0; i < S2N_CERT_TYPE_COUNT; i++) {
struct s2n_cert_chain_and_key *cert = config->default_certs_by_type.certs[i];
Expand Down
Loading