Skip to content

tests: policy snapshot test #5309

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

Merged
merged 3 commits into from
May 16, 2025
Merged

tests: policy snapshot test #5309

merged 3 commits into from
May 16, 2025

Conversation

lrstewart
Copy link
Contributor

@lrstewart lrstewart commented May 14, 2025

Release Summary:

Resolved issues:

resolves #5306

Description of changes:

I wrote a quick lightweight snapshot test for policies, using the policy util we added to pretty-print our security policies.

Call-outs:

The PR is huge because of all the snapshots, but they were all generated automatically. And you know they match the actual output of the script bc that's the whole point of the new test ;) Only the new workflow and the script need to be reviewed.

Testing:

Here's a failed run where I made a bunch of changes to security policies: https://github.com/lrstewart/s2n/actions/runs/15033574096/job/42251031147?pr=65

Another failed run, because I needed to pull in the upstream ML-DSA changes: https://github.com/aws/s2n-tls/actions/runs/15055345610/job/42319868415?pr=5309

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

@github-actions github-actions bot added the s2n-core team label May 14, 2025
@lrstewart lrstewart force-pushed the policy_snapshot branch 3 times, most recently from fad45b7 to 8ed3b4b Compare May 14, 2025 23:55
@lrstewart lrstewart marked this pull request as ready for review May 15, 2025 00:19
@lrstewart lrstewart requested review from goatgoose and dougch May 15, 2025 00:19
Copy link
Contributor

@goatgoose goatgoose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!!

Copy link
Contributor

@dougch dougch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor nit/question.

POLICIES=$(grep -o '{ .version = "[^"]*"' $SECURITY_POLICIES_C \
| sed 's/{ .version = "\(.*\)"/\1/' | grep -v "^null$")

COUNT=$(echo "$POLICIES" | wc -l)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a chance that a malformed policy will result in ones getting missed? Do you need to compare the count of policies in the directory with what ends up in $POLICIES ?

Copy link
Contributor Author

@lrstewart lrstewart May 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that'd help. Since we run the command once for every line in $POLICIES, I don't see a scenario where those two numbers wouldn't match. If we had a malformed policy of some kind, we'd end up writing the policy usage message to a file, which would still mean we'd write the file. Luckily in that case the command would fail, failing the script.

$ build/bin/policy
policy <version>
example: policy default_tls13

$ echo $?
1

$ build/bin/policy asdf
policy <version>
example: policy default_tls13

$ echo $?
1

The check would probably help if we had a source for the expected number of policies other than $POLICIES, but I'm not sure what other source we could use.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh but at least for this initial commit, I manually compared the number of policies in the policies array to the number of files written and sanity-checked the files written.

@maddeleine maddeleine added this pull request to the merge queue May 16, 2025
Merged via the queue into aws:main with commit c2653a4 May 16, 2025
47 checks passed
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.

Security policy snapshot test
4 participants