Skip to content

Commit

Permalink
fix: don't iterate over certs if not validating certs (#4797)
Browse files Browse the repository at this point in the history
  • Loading branch information
lrstewart authored Sep 25, 2024
1 parent 0a887d7 commit edc8736
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 0 deletions.
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

0 comments on commit edc8736

Please sign in to comment.