From 704982e1f7138030799b874a560082a649176af1 Mon Sep 17 00:00:00 2001 From: Apoorv Kothari Date: Tue, 10 Sep 2024 15:13:45 -0700 Subject: [PATCH] cleanup detection logic --- bindings/rust/s2n-tls/src/fingerprint.rs | 2 -- bindings/rust/s2n-tls/src/testing/s2n_tls.rs | 3 --- codebuild/bin/grep_simple_mistakes.sh | 6 +++--- tests/unit/s2n_client_hello_test.c | 4 ---- tests/unit/s2n_config_test.c | 8 +++----- tests/unit/s2n_connection_preferences_test.c | 4 ---- tests/unit/s2n_security_policies_test.c | 8 -------- tls/s2n_config.c | 8 -------- tls/s2n_config.h | 4 ---- tls/s2n_security_policies.c | 17 ----------------- 10 files changed, 6 insertions(+), 58 deletions(-) diff --git a/bindings/rust/s2n-tls/src/fingerprint.rs b/bindings/rust/s2n-tls/src/fingerprint.rs index f50cb248738..0b83371ab4c 100644 --- a/bindings/rust/s2n-tls/src/fingerprint.rs +++ b/bindings/rust/s2n-tls/src/fingerprint.rs @@ -394,7 +394,6 @@ mod tests { #[test] fn connection_fingerprint() -> Result<(), Box> { - // TODO not protocol dependent let pair = simple_handshake()?; let client_hello = pair.server.client_hello()?; @@ -663,7 +662,6 @@ mod tests { #[test] #[allow(deprecated)] fn legacy_connection_fingerprint() -> Result<(), Box> { - // TODO not protocol dependent let pair = simple_handshake()?; let client_hello = pair.server.client_hello()?; diff --git a/bindings/rust/s2n-tls/src/testing/s2n_tls.rs b/bindings/rust/s2n-tls/src/testing/s2n_tls.rs index 350c6a9a32d..26537363b6b 100644 --- a/bindings/rust/s2n-tls/src/testing/s2n_tls.rs +++ b/bindings/rust/s2n-tls/src/testing/s2n_tls.rs @@ -16,7 +16,6 @@ mod tests { #[test] fn handshake_default() { - // TODO not protocol dependent let config = build_config(&security::DEFAULT).unwrap(); assert!(TestPair::handshake_with_config(&config).is_ok()); } @@ -642,7 +641,6 @@ mod tests { #[test] fn no_application_protocol() -> Result<(), Error> { - // TODO not protocol dependent let config = config_builder(&security::DEFAULT)?.build()?; let mut pair = TestPair::from_config(&config); pair.handshake()?; @@ -652,7 +650,6 @@ mod tests { #[test] fn application_protocol() -> Result<(), Error> { - // TODO not protocol dependent let config = config_builder(&security::DEFAULT)?.build()?; let mut pair = TestPair::from_config(&config); pair.server diff --git a/codebuild/bin/grep_simple_mistakes.sh b/codebuild/bin/grep_simple_mistakes.sh index 1f7868a1082..583f0a7bbbb 100755 --- a/codebuild/bin/grep_simple_mistakes.sh +++ b/codebuild/bin/grep_simple_mistakes.sh @@ -225,16 +225,16 @@ if [[ -n $S2N_ENSURE_WITH_INVALID_ERROR_CODE ]]; then fi ############################################# -# Assert test don't specify the "default" security policy +# Assert tests don't specify the "default" security policy. # # Since the "default" policies are subject to change, tests should instead specify # an immutable numbered policy to avoid unwanted testing behavior. ############################################# S2N_DEFAULT_SECURITY_POLICY_USAGE=$(find "$PWD" -type f -name "s2n*.c" -not -path "*/bindings/*" -not -path "*/bin/*") declare -A KNOWN_DEFAULT_USAGE -KNOWN_DEFAULT_USAGE["$PWD/tls/s2n_config.c"]=2 +KNOWN_DEFAULT_USAGE["$PWD/tls/s2n_config.c"]=1 KNOWN_DEFAULT_USAGE["$PWD/tls/s2n_security_policies.c"]=5 -KNOWN_DEFAULT_USAGE["$PWD/tests/unit/s2n_security_policies_test.c"]=5 +KNOWN_DEFAULT_USAGE["$PWD/tests/unit/s2n_security_policies_test.c"]=1 KNOWN_DEFAULT_USAGE["$PWD/tests/unit/s2n_client_hello_test.c"]=2 KNOWN_DEFAULT_USAGE["$PWD/tests/unit/s2n_connection_preferences_test.c"]=1 KNOWN_DEFAULT_USAGE["$PWD/tests/unit/s2n_config_test.c"]=1 diff --git a/tests/unit/s2n_client_hello_test.c b/tests/unit/s2n_client_hello_test.c index 63a5c94d0ef..9ec93c4dc9c 100644 --- a/tests/unit/s2n_client_hello_test.c +++ b/tests/unit/s2n_client_hello_test.c @@ -583,9 +583,7 @@ int main(int argc, char **argv) for (size_t i = 0; i < s2n_array_len(test_cases); i++) { struct s2n_connection *conn = s2n_connection_new(S2N_CLIENT); EXPECT_NOT_NULL(conn); - dbg_bail = false; EXPECT_SUCCESS(s2n_connection_set_cipher_preferences(conn, test_cases[i].security_policy)); - dbg_bail = true; EXPECT_SUCCESS(s2n_client_hello_send(conn)); EXPECT_SUCCESS(s2n_parse_client_hello(conn)); @@ -1567,9 +1565,7 @@ int main(int argc, char **argv) DEFER_CLEANUP(struct s2n_connection *client = s2n_connection_new(S2N_CLIENT), s2n_connection_ptr_free); EXPECT_SUCCESS(s2n_connection_set_config(client, config)); - dbg_bail = false; EXPECT_SUCCESS(s2n_connection_set_cipher_preferences(client, security_policy)); - dbg_bail = true; EXPECT_SUCCESS(s2n_handshake_write_header(&client->handshake.io, TLS_CLIENT_HELLO)); EXPECT_SUCCESS(s2n_client_hello_send(client)); diff --git a/tests/unit/s2n_config_test.c b/tests/unit/s2n_config_test.c index 5a96073d188..50ecc50476f 100644 --- a/tests/unit/s2n_config_test.c +++ b/tests/unit/s2n_config_test.c @@ -72,9 +72,7 @@ int main(int argc, char **argv) const struct s2n_security_policy *default_security_policy = NULL, *tls13_security_policy = NULL, *fips_security_policy = NULL; EXPECT_SUCCESS(s2n_find_security_policy_from_version("default_tls13", &tls13_security_policy)); EXPECT_SUCCESS(s2n_find_security_policy_from_version("default_fips", &fips_security_policy)); - dbg_bail = false; EXPECT_SUCCESS(s2n_find_security_policy_from_version("default", &default_security_policy)); - dbg_bail = true; char cert[S2N_MAX_TEST_PEM_SIZE] = { 0 }; EXPECT_SUCCESS(s2n_read_test_pem(S2N_DEFAULT_TEST_CERT_CHAIN, cert, S2N_MAX_TEST_PEM_SIZE)); @@ -1094,7 +1092,7 @@ int main(int argc, char **argv) struct s2n_security_policy rfc9151_applied_locally = security_policy_rfc9151; rfc9151_applied_locally.certificate_preferences_apply_locally = true; - /* rfc9151 doesn't allow SHA256 signatures, but does allow SHA384 signatures, + /* rfc9151 doesn't allow SHA256 signatures, but does allow SHA384 signatures, * so ecdsa_p384_sha256 is invalid and ecdsa_p384_sha384 is valid */ /* valid certs are accepted */ @@ -1121,8 +1119,8 @@ int main(int argc, char **argv) /* certs in default_certs_by_type are validated */ { - /* s2n_config_set_cert_chain_and_key_defaults populates default_certs_by_type - * but doesn't populate domain_name_to_cert_map + /* s2n_config_set_cert_chain_and_key_defaults populates default_certs_by_type + * but doesn't populate domain_name_to_cert_map */ DEFER_CLEANUP(struct s2n_config *config = s2n_config_new_minimal(), s2n_config_ptr_free); EXPECT_SUCCESS(s2n_config_set_cert_chain_and_key_defaults(config, &invalid_cert, 1)); diff --git a/tests/unit/s2n_connection_preferences_test.c b/tests/unit/s2n_connection_preferences_test.c index 0bacbcdecb4..1dc72bddbc9 100644 --- a/tests/unit/s2n_connection_preferences_test.c +++ b/tests/unit/s2n_connection_preferences_test.c @@ -31,9 +31,7 @@ int main(int argc, char **argv) const struct s2n_security_policy *default_security_policy = NULL, *tls13_security_policy = NULL, *fips_security_policy = NULL; EXPECT_SUCCESS(s2n_find_security_policy_from_version("default_tls13", &tls13_security_policy)); EXPECT_SUCCESS(s2n_find_security_policy_from_version("default_fips", &fips_security_policy)); - dbg_bail = false; EXPECT_SUCCESS(s2n_find_security_policy_from_version("default", &default_security_policy)); - dbg_bail = true; /* Test default TLS1.2 */ if (!s2n_is_in_fips_mode()) { @@ -261,9 +259,7 @@ int main(int argc, char **argv) /* The static configs were mutated. Reset them to allow following unit tests to use them. */ s2n_wipe_static_configs(); - dbg_bail = false; EXPECT_SUCCESS(s2n_config_defaults_init()); - dbg_bail = true; }; /* s2n_connection_get_curve */ diff --git a/tests/unit/s2n_security_policies_test.c b/tests/unit/s2n_security_policies_test.c index 0b85d146dac..10d1176db94 100644 --- a/tests/unit/s2n_security_policies_test.c +++ b/tests/unit/s2n_security_policies_test.c @@ -507,9 +507,7 @@ int main(int argc, char **argv) for (size_t i = 0; i < s2n_array_len(tls12_only_security_policy_strings); i++) { security_policy = NULL; - dbg_bail = false; EXPECT_SUCCESS(s2n_find_security_policy_from_version(tls12_only_security_policy_strings[i], &security_policy)); - dbg_bail = true; EXPECT_FALSE(s2n_security_policy_supports_tls13(security_policy)); } @@ -970,9 +968,7 @@ int main(int argc, char **argv) { /* 20230317 */ { - dbg_bail = false; EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20230317, "default", rsa_chain_and_key)); - dbg_bail = true; EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20230317, "default_tls13", rsa_chain_and_key)); EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20230317, "default_fips", rsa_chain_and_key)); EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20230317, "20230317", rsa_chain_and_key)); @@ -998,10 +994,8 @@ int main(int argc, char **argv) /* 20240331 */ { - dbg_bail = false; EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20240331, "default", rsa_chain_and_key)); - dbg_bail = true; EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20240331, "default_tls13", rsa_chain_and_key)); EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20240331, @@ -1055,11 +1049,9 @@ int main(int argc, char **argv) { .cert = ecdsa_chain_and_key, .start_index = 1 }, }; - dbg_bail = false; EXPECT_OK(s2n_test_default_backwards_compatible("default", versioned_policies, s2n_array_len(versioned_policies), supported_certs, s2n_array_len(supported_certs))); - dbg_bail = true; }; /* "default_tls13" */ diff --git a/tls/s2n_config.c b/tls/s2n_config.c index 6d4cb3d34c4..e911eb96879 100644 --- a/tls/s2n_config.c +++ b/tls/s2n_config.c @@ -51,10 +51,6 @@ static int monotonic_clock(void *data, uint64_t *nanoseconds) return 0; } -/* Used to add exception when creating a new config */ -bool dbg_config_init = true; -/* Control exception to the "default" policy usage */ -bool dbg_bail = true; static int wall_clock(void *data, uint64_t *nanoseconds) { struct timespec current_time = { 0 }; @@ -104,11 +100,7 @@ static int s2n_config_init(struct s2n_config *config) config->client_hello_cb_mode = S2N_CLIENT_HELLO_CB_BLOCKING; - /* TODO remove */ - /* avoid bailing when creating a new config `s2n_config_new()` */ - dbg_config_init = false; POSIX_GUARD(s2n_config_setup_default(config)); - dbg_config_init = true; if (s2n_use_default_tls13_config()) { POSIX_GUARD(s2n_config_setup_tls13(config)); } else if (s2n_is_in_fips_mode()) { diff --git a/tls/s2n_config.h b/tls/s2n_config.h index 4de4c49a498..801777281e2 100644 --- a/tls/s2n_config.h +++ b/tls/s2n_config.h @@ -31,10 +31,6 @@ #include "utils/s2n_blob.h" #include "utils/s2n_set.h" -/* TODO remove */ -extern bool dbg_config_init; -extern bool dbg_bail; - #define S2N_MAX_TICKET_KEYS 48 #define S2N_MAX_TICKET_KEY_HASHES 500 /* 10KB */ diff --git a/tls/s2n_security_policies.c b/tls/s2n_security_policies.c index 63ab67e9864..551ad22b4a4 100644 --- a/tls/s2n_security_policies.c +++ b/tls/s2n_security_policies.c @@ -18,7 +18,6 @@ #include "api/s2n.h" #include "tls/s2n_certificate_keys.h" #include "tls/s2n_connection.h" -#include "utils/s2n_init.h" #include "utils/s2n_safety.h" /* TLS1.2 default as of 05/24 */ @@ -1271,22 +1270,6 @@ int s2n_find_security_policy_from_version(const char *version, const struct s2n_ POSIX_ENSURE_REF(version); POSIX_ENSURE_REF(security_policy); - bool matches_default = strcmp(version, "default") == 0; - bool should_bail = - /* allow for exception for tests which actually want to test the "default" policy */ - dbg_bail && - /* allow for s2n_config_new object creation */ - dbg_config_init && - /* s2n_init() creates a "default" static config so only bail after initialization is complete; */ - s2n_is_initialized() && - /* attempting to use the "default" policy */ - matches_default; - - if (should_bail) { - printf("\nBail------- s2n_find_from_version: config_init: %d", dbg_config_init); - POSIX_BAIL(S2N_ERR_INVALID_SECURITY_POLICY); - } - for (int i = 0; security_policy_selection[i].version != NULL; i++) { if (!strcasecmp(version, security_policy_selection[i].version)) { *security_policy = security_policy_selection[i].security_policy;