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

bin: tool to print security policies #4524

Merged
merged 4 commits into from
May 1, 2024
Merged

bin: tool to print security policies #4524

merged 4 commits into from
May 1, 2024

Conversation

lrstewart
Copy link
Contributor

Description of changes:

While working on the default security policy updates, I wrote a quick tool to generate readable policy diffs. Ideally we just delete it again once we make security policies easier to deal with 🥲 Until then, I figured I'd commit it in case anyone else needs it.

I had to add names / better names to a bunch of existing structs to get good results. I'd appreciate if someone spot-checked them.

Testing:

Here's the diffs I generated for the default policy updates: https://gist.github.com/lrstewart/ca096f37b33d06fdb6cd28a1337d4edc

Other examples:

$ ./bin/policy rfc9151
name: rfc9151
min version: TLS1.2
rules:
- forward secret: no
- supports fips: no
cipher suites:
- TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384
- TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
- TLS_RSA_WITH_AES_256_GCM_SHA384
- TLS_DHE_RSA_WITH_AES_256_GCM_SHA384
- TLS_AES_256_GCM_SHA384
signature schemes:
- ecdsa_sha384
- rsa_pss_pss_sha384
- rsa_pss_rsae_sha384
- rsa_pkcs1_sha384
curves:
- secp384r1
certificate signature schemes:
- ecdsa_sha384
- rsa_pkcs1_sha384

With PQ / kems:

$ ./bin/policy KMS-PQ-TLS-1-0-2020-07
name: KMS-PQ-TLS-1-0-2020-07
min version: TLS1.0
rules:
- forward secret: yes
- supports fips: no
cipher suites:
- TLS_ECDHE_KYBER_RSA_WITH_AES_256_GCM_SHA384
- TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
- TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
- TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384
- TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA
- TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256
- TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA
- TLS_DHE_RSA_WITH_AES_256_CBC_SHA256
- TLS_DHE_RSA_WITH_AES_128_CBC_SHA256
- TLS_DHE_RSA_WITH_AES_256_CBC_SHA
- TLS_DHE_RSA_WITH_AES_128_CBC_SHA
signature schemes:
- rsa_pkcs1_sha256
- rsa_pkcs1_sha384
- rsa_pkcs1_sha512
- legacy_rsa_pkcs1_sha224
- ecdsa_sha256
- ecdsa_sha384
- ecdsa_sha512
- legacy_ecdsa_sha224
- rsa_pkcs1_sha1
- ecdsa_sha1
curves:
- secp256r1
- secp384r1
pq:
- revision: 0
- kems:
-- kyber512r3
- kem groups:
-- x25519_kyber-512-r3
-- secp256r1_kyber-512-r3

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 Apr 25, 2024
@lrstewart lrstewart marked this pull request as ready for review April 25, 2024 04:52
@lrstewart lrstewart requested review from goatgoose, camshaft and maddeleine and removed request for camshaft April 25, 2024 04:52
CMakeLists.txt Show resolved Hide resolved
bin/policy.c Outdated Show resolved Hide resolved
@lrstewart lrstewart requested a review from goatgoose April 26, 2024 21:59
@@ -210,6 +210,7 @@ const struct s2n_record_algorithm s2n_tls13_record_alg_chacha20_poly1305 = {
struct s2n_cipher_suite s2n_null_cipher_suite = {
.available = 1,
.name = "TLS_NULL_WITH_NULL_NULL",
.iana_name = "TLS_NULL_WITH_NULL_NULL",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need both .iana_name and .name? Like, what are we printing in the script, the iana name or our name? Cause it seems like the script uses both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The script only uses the iana name. "name" for cipher suites is the Openssl-style name.

Is the confusion maybe that I also called the new signatures scheme field "iana_name" instead of just "name"? Would just "name" make more sense there, since there isn't any existing name field?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it would be easier to understand if we were only printing "name" or only printing "iana_name".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not everything I'm printing has an iana name. Rules and certificate types don't, and I'm not sure about kems / kem groups either.

I suppose I could change the cipher suite "name" to "openssl_name", but I'm not sure I really see the benefit.

@lrstewart lrstewart requested a review from maddeleine April 29, 2024 22:36
bin/policy.c Outdated Show resolved Hide resolved
lrstewart and others added 2 commits May 1, 2024 11:52
Co-authored-by: maddeleine <59030281+maddeleine@users.noreply.github.com>
@lrstewart lrstewart enabled auto-merge (squash) May 1, 2024 18:53
@raycoll
Copy link
Contributor

raycoll commented May 1, 2024

is there a specific serialization output format a calling application should expect(i.e. json/yaml)? or is that a future iteration for this tool? it would be awesome to have outputs we can reliably pass around to other tools(either to display in UIs or programmatically diff, like the overview mentioned).

@lrstewart
Copy link
Contributor Author

is there a specific serialization output format a calling application should expect(i.e. json/yaml)? or is that a future iteration for this tool? it would be awesome to have outputs we can reliably pass around to other tools(either to display in UIs or programmatically diff, like the overview mentioned).

At the moment this is just text intended for humans. A more official serialization story would likely be part of the planned upcoming improvements to security policies. I suspect it wouldn't even be part of this tool, I suspect we'll eventually just delete this tool.

@lrstewart lrstewart merged commit 1ed5624 into aws:main May 1, 2024
33 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.

4 participants