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

test: pin static testing policies to numbered versions #4845

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

toidiu
Copy link
Contributor

@toidiu toidiu commented Oct 15, 2024

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.

@toidiu toidiu marked this pull request as ready for review October 21, 2024 22:31
@toidiu toidiu requested a review from goatgoose October 21, 2024 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant