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

create rfc9151 security policy #3431

Merged
merged 7 commits into from
Aug 25, 2022
Merged

create rfc9151 security policy #3431

merged 7 commits into from
Aug 25, 2022

Conversation

toidiu
Copy link
Contributor

@toidiu toidiu commented Aug 6, 2022

Description of changes:

Adds a new security policy with AES256, SHA384 and support for TLS1.3

Testing:

Verified that we can connect via tls1.3/tls1.2 and rsa/ecdsa

# ./build/bin/s2nd --cert tests/pems/ecdsa_p384_pkcs1_cert.pem --key tests/pems/ecdsa_p384_pkcs1_key.pem -c "rfc9151" 127.1.0.1 8888

# ./build/bin/s2nc -i -c "20210816" 127.1.0.1 8888
# CONNECTED:
# Handshake: NEGOTIATED|FULL_HANDSHAKE|TLS12_PERFECT_FORWARD_SECRECY|WITH_SESSION_TICKET
# Client hello version: 33
# Client protocol version: 33
# Server protocol version: 34
# Actual protocol version: 33
# Server name: 127.1.0.1
# Curve: secp384r1
# KEM: NONE
# KEM Group: NONE
# Cipher negotiated: ECDHE-ECDSA-AES256-GCM-SHA384
# Server signature negotiated: ECDSA+SHA384
# Early Data status: NOT REQUESTED
# s2n is ready

# ./build/bin/s2nc -i -c "rfc9151" 127.1.0.1 8888
# CONNECTED:
# Handshake: NEGOTIATED|FULL_HANDSHAKE|MIDDLEBOX_COMPAT
# Client hello version: 33
# Client protocol version: 34
# Server protocol version: 34
# Actual protocol version: 34
# Server name: 127.1.0.1
# Curve: secp384r1
# KEM: NONE
# KEM Group: NONE
# Cipher negotiated: TLS_AES_256_GCM_SHA384
# Server signature negotiated: ECDSA+SHA384
# Early Data status: NOT REQUESTED
# s2n is ready




#############
./build/bin/s2nd --cert tests/pems/rsa_2048_pkcs1_cert.pem --key tests/pems/rsa_2048_pkcs1_key.pem -c "rfc9151" 127.1.0.1 8888

# ./build/bin/s2nc -i -c "rfc9151" 127.1.0.1 8888
# CONNECTED:
# Handshake: NEGOTIATED|FULL_HANDSHAKE|MIDDLEBOX_COMPAT
# Client hello version: 33
# Client protocol version: 34
# Server protocol version: 34
# Actual protocol version: 34
# Server name: 127.1.0.1
# Curve: secp384r1
# KEM: NONE
# KEM Group: NONE
# Cipher negotiated: TLS_AES_256_GCM_SHA384
# Server signature negotiated: RSA-PSS-RSAE+SHA384
# Early Data status: NOT REQUESTED
# s2n is ready


# ./build/bin/s2nc -i -c "default" 127.1.0.1 8888
# CONNECTED:
# Handshake: NEGOTIATED|FULL_HANDSHAKE|TLS12_PERFECT_FORWARD_SECRECY|WITH_SESSION_TICKET
# Client hello version: 33
# Client protocol version: 33
# Server protocol version: 34
# Actual protocol version: 33
# Server name: 127.1.0.1
# Curve: secp384r1
# KEM: NONE
# KEM Group: NONE
# Cipher negotiated: ECDHE-RSA-AES256-GCM-SHA384
# Server signature negotiated: RSA+SHA384
# Early Data status: NOT REQUESTED
# s2n is ready

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 Aug 6, 2022
@zaherd zaherd requested a review from lrstewart August 8, 2022 05:46
@toidiu toidiu changed the title create 20220805 security policy create rfc9151 security policy Aug 9, 2022
Copy link
Contributor

@lrstewart lrstewart left a comment

Choose a reason for hiding this comment

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

Looks good now! I would also suggest adding a note about this policy to the documentation

@scottarc
Copy link

This looks like a good security policy, but SHA512 isn't included in CNSA (and therefore RFC 9151), so the name is misleading for the current implementation. We'll need to either remove SHA512 from this security policy or name it something different.

@toidiu toidiu requested a review from a team as a code owner August 17, 2022 20:45
@toidiu
Copy link
Contributor Author

toidiu commented Aug 17, 2022

This looks like a good security policy, but SHA512 isn't included in CNSA (and therefore RFC 9151), so the name is misleading for the current implementation. We'll need to either remove SHA512 from this security policy or name it something different.

Lets create a security policy for rfc9151 here. We can create a new one that includes sha512 if that is needed later. b7b81f1

@toidiu toidiu enabled auto-merge (squash) August 22, 2022 17:28
@toidiu toidiu requested review from maddeleine and lrstewart August 22, 2022 17:28
&s2n_tls13_aes_256_gcm_sha384,
};

const struct s2n_cipher_preferences cipher_preferences_rfc9151 = {
Copy link
Contributor

Choose a reason for hiding this comment

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

out of curiosity, is there a reason why we are not using a date version string eg something like 20220822?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/aws/s2n-tls/blob/main/docs/USAGE-GUIDE.md

Numbered versions are fixed and will never change.

we try to avoid numbered version where possible since the numbers dont really mean much and since policy cant ever change, it will become outdated at some point.

Copy link
Contributor

@camshaft camshaft Aug 23, 2022

Choose a reason for hiding this comment

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

I wouldn't say we avoid them; they're all over. Outside of default, I think all of them are date-versioned. However, in this case I actually think the RFC number is a pretty reasonable version since RFCs are unable to change after being published. So we still have immutability and it actually means something as opposed to some arbitrary date.

@toidiu toidiu force-pushed the ak-newSecPolicy branch 2 times, most recently from 4fda4d8 to fe60244 Compare August 24, 2022 21:00
@toidiu toidiu merged commit f933ecc into aws:main Aug 25, 2022
@toidiu toidiu deleted the ak-newSecPolicy branch August 30, 2022 21:19
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.

6 participants