test: pin static testing policies to numbered versions

[toidiu]

Problem:

Currently, a new s2n_config object is configured with one of three static configs based on runtime settings and testing overrides.

The security policy selection depends on testing override function calls and makes use of the "default" and "default_fips" policies.

enable_tls13_int_test()       // 13 not-fips    "default_tls13"
disable_tls13_int_test()
  in_fips_mode                // 12 fips        "default_fips"
  else                        // 12 not-fips    "default"
else //reset_tls13_in_test()
  in_fips_mode                // 12 fips        "default_fips"
  else                        // 12 not-fips    "default"

As we attempt to evolve the default* policies, maintaining the current behavior becomes cumbersome. Specifically, along with adding TLS1.3 support to the "default" policy, we will also add a new test which temporarily set the "default" to TLS1.2. This "toggling" of the "default" policy will make the current config selection very complicated.

Description of changes:

To solve this issue, this PR creates dedicated static configs for testing override and pins them to numbered policies rather than rely on the "default" policy.

// Current ==========
static struct s2n_config s2n_default_config = { 0 };
static struct s2n_config s2n_default_fips_config = { 0 };
static struct s2n_config s2n_default_tls13_config = { 0 };

// Proposed changes ==========
static struct s2n_config s2n_default_config = { 0 };
static struct s2n_config s2n_default_fips_config = { 0 };
/* Dedicated configs for testing different protocols since test can override the
 * default security policy behavior.
 */
static struct s2n_config s2n_testing_default_tls12_config = { 0 }; // pinned to "20240501"
static struct s2n_config s2n_testing_default_tls12_fips_config = { 0 }; // pinned to "20240502"
static struct s2n_config s2n_testing_default_tls13_config = { 0 }; // pinned to "20240503"

The cost of this strategy is 2 additional static configs, however, since these are only used in testing, we are able to avoid extra costs of the s2n_config_load_system_certs by gating initialization of test configs behind a s2n_in_unit_test check.

Callout:

I have made other refactor changes to make reasoning about testing overrides and policy selection easier to reason about:

• capture policy override and cofig selection behavior as enum: s2n_default_security_policy_selection and s2n_testing_security_policy_override
• remove function s2n_config_testing_defaults_init_tls13_certs: was used to avoid the costs of s2n_config_load_system_certs, however we can fully avoid the costs by gating initialization to unit tests.

Testing:

Added unit testing with and without the testing overrides.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[goatgoose]

Is this additional logic necessary? If we're not in a unit tests, won't s2n_testing_override always be S2N_TESTING_POLICY_NO_OVERRIDE, just like s2n_use_default_tls13_config_flag was always false?

In general I think it's better to limit decisions based on s2n_in_unit_test(), since these decisions can't be tested in unit tests.

[goatgoose]

Same comment - I'm not sure if this branch is needed.

[goatgoose]

This might be clearer:

Suggested change

	bool uses_tls13 = false;
	switch (s2n_testing_override) {
	case S2N_TESTING_POLICY_OVERRIDE_TLS12:
	break;
	case S2N_TESTING_POLICY_OVERRIDE_TLS13:
	uses_tls13 = true;
	break;
	case S2N_TESTING_POLICY_NO_OVERRIDE:
	break;
	}
	return uses_tls13;
	switch (s2n_testing_override) {
	case S2N_TESTING_POLICY_OVERRIDE_TLS13:
	return true;
	default:
	return false;
	}

[goatgoose]

Why is this function still needed? Wasn't it only used to select the default TLS 1.3 config, which has now been replaced with your new selection logic?

[goatgoose]

naming nits - I think we usually say "for test" or "for testing" if something is for testing. So these names might be more consistent and might make their purposes more clear:

Suggested change

	typedef enum {
	S2N_TESTING_POLICY_OVERRIDE_TLS12 = 0,
	S2N_TESTING_POLICY_OVERRIDE_TLS13,
	S2N_TESTING_POLICY_NO_OVERRIDE,
	} s2n_testing_security_policy_override;
	S2N_RESULT s2n_get_testing_security_policy_override(s2n_testing_security_policy_override *override);
	typedef enum {
	S2N_TLS12_POLICY_OVERRIDE = 0,
	S2N_TLS13_POLICY_OVERRIDE,
	S2N_NO_POLICY_OVERRIDE,
	} s2n_security_policy_override_for_testing;
	S2N_RESULT s2n_get_get_security_policy_override_for_testing(s2n_security_policy_override_for_testing *override);

[goatgoose]

It seems safer to remove this default config return and instead add a default to the switch statement.

[goatgoose]

nit - we try to keep the s2n namespace, even in tests.

Suggested change

	S2N_RESULT test_policy_behavior(const struct s2n_security_policy *policy,
	S2N_RESULT s2n_test_policy_behavior(const struct s2n_security_policy *policy,

[goatgoose]

This diff is currently very difficult to review. Is there any way to improve it? If not, it might be easier to review as a separate file with the old one deleted. Then we can always rename it back in another PR.

[goatgoose]

Is this needed? I don't think these tests override anything.

[goatgoose]

nit - defer cleanup is better for new tests.

Suggested change

	struct s2n_connection *conn = NULL;
	DEFER_CLEANUP(struct s2n_connection *conn = s2n_connection_new(S2N_CLIENT), s2n_connection_ptr_free);

[lrstewart]

Do these need to live in the main source? Or can they be moved to the testing library? You're adding more complexity to the source here than I think you need to. See if you can simplify.

("default" and "default_fips" have real customer use cases. "default_tls13" had real customer use cases when it was first added, before we changed how we configured tls1.3, but could probably be removed now)