test: amend implicit use of "default" policy in tests #4778
Conversation
toidiu
force-pushed
the
ak-implicitDefaultUsage
branch
from
60f4ba4 to
b50b152
Compare
toidiu
force-pushed
the
ak-implicitDefaultUsage
branch
from
252650a to
952988c
Compare
toidiu
force-pushed
the
ak-implicitDefaultUsage
branch
from
952988c to
88e358a
Compare
toidiu
changed the title
| /* s2n_config_new() matches s2n_fetch_default_config() */ | ||
| /* s2n_config_new matches s2n_fetch_default_config() */ |
There was a problem hiding this comment.
I'd recommend listing every manual change in the PR description, or putting the manual changes in a separate PR. Otherwise changes like this are difficult to find/review.
There was a problem hiding this comment.
All the manual changes are in a single commit: 42d17d6
I can mention that in the description.
There was a problem hiding this comment.
Can you make those manual changes in a separate PR before making the automated changes?
| EXPECT_SUCCESS(s2n_config_set_cipher_preferences(config, s2n_auto_gen_old_default_security_policy())); | ||
| EXPECT_EQUAL(config->security_policy, default_security_policy); |
There was a problem hiding this comment.
These tests will now break when the default policy is updated. But I guess it makes sense to wait to fix these in another PR to avoid messing with the auto generation?
This file also contains other tests that test the default config itself. Are there similar checks in these kinds of tests that will fail when the policy is swapped? Just want to make sure that they won't start silently testing the old default policy now that the old default policy is being specifically set.
There was a problem hiding this comment.
These tests will now break when the default policy is updated. But I guess it makes sense to wait to fix these in another PR to avoid messing with the auto generation?
I think the test is trying to cover "default behavior of configs" (what policy does it get initialized with by default) (see 3rd point below).
Therefore the auto-gen is actually changing the intent of the test because and should not be applied.
This file also contains other tests that test the default config itself. Are there similar checks in these kinds of tests that will fail when the policy is swapped? Just want to make sure that they won't start silently testing the old default policy now that the old default policy is being specifically set.
This is a good point but this is how I was thinking about this:
-
If you want to test with the "default" policy you should have specified
set_cipher_preference("default")
covered in explicit detection -
if you really want to test with tls_12 and were lazy, you might not have specified anything
implicit detection which this PR covers -
if you want to test the "default behavior of configs" (what policy does it get initialized with by default)
this is what you are calling out. I would like to think that these type of tests only exist in s2n_config_test and possibly s2n_security_policy_test. I would audit those test files manually. If not then we would need to audit all tests to make sure of this.
There was a problem hiding this comment.
Yeah I agree with all of this. Though for 1, this PR (in combination with the s2n_connection PR) will still address the case where the test is using the default policy without calling set_cipher_preferences right?
For 3, I agree - I'm not aware of other tests that are testing default configs themselves, so it seems reasonable to me to restrict the audit to this file. To make this easier to review, it might help to skip s2n_config_test in the code generation script and manually remove implicit usages from just this file in a followup PR.
| EXPECT_SUCCESS(s2n_config_set_cipher_preferences(client_config, s2n_auto_gen_old_default_security_policy())); | ||
| EXPECT_SUCCESS(s2n_config_set_cipher_preferences(client_config, "20170210")); |
There was a problem hiding this comment.
I've seen several of these double set_cipher_preferences calls. I remember it being discussed to have the script to do a search for these to remove the obviously unnecessary auto generated call. It probably doesn't make sense for this PR anyway, but do you still think that's worth doing?
|
During review, we discovered that some tests in-fact do want to test the "default" policy. This means that pinning these policies would result in testing regression. Closing this PR until we have a better path forward. |
TODO: revert auto-gen script commit prior to merging.
#4765
Description of changes:
As part of adding TLS 1.3 support to the "default", "default_fips" policy, we need to detect and amend implicit use of the "default*" policy in tests. This occurs since
s2n_config_new()(also s2n_connection_new() but that will be a separate PR) is initialized with the "default*" policy by default.The solution is to auto-insert code which pins the security policy on the config to a numbered policy, eg:
EXPECT_SUCCESS(s2n_config_set_cipher_preferences(config, "20240501"));Callout
Manual changes to prepare for the auto-generation script are in commit
Auto-insertion script is in commit.
Test fixup via the auto-insertion script is in commit
Testing:
Tests should continue to pass.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.