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

api: add method to get the iana value for the negotiated cipher suite #2550

Merged
merged 3 commits into from
Feb 12, 2021

Conversation

camshaft
Copy link
Contributor

@camshaft camshaft commented Feb 1, 2021

Description of changes:

Currently there's no way to get the IANA value for a connection's negotiated cipher suite. There is the s2n_connection_get_cipher method which returns a const char *, but this is less than ideal when trying to match on values programmatically.

This change adds a s2n_connection_get_cipher_iana_value method which takes two byte pointers and sets those to the negotiated cipher suite. I feel like this is the interface with the fewest sharp edges.

The other options I considered were:

  • Taking a uint16_t. It's nice to just pass a single pointer, but it leads to some ambiguity in the endianness of the contained value.
  • Taking a uint8_t * and requiring that there be 2 elements. This is impossible to enforce and hard to document.

Testing:

I added some checks alongside the current tests for the s2n_connection_get_cipher method.

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

@camshaft camshaft marked this pull request as ready for review February 1, 2021 21:41
@codecov
Copy link

codecov bot commented Feb 1, 2021

Codecov Report

Merging #2550 (d4292ea) into main (bcebb35) will increase coverage by 0.05%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##             main    #2550      +/-   ##
==========================================
+ Coverage   82.09%   82.14%   +0.05%     
==========================================
  Files         272      272              
  Lines       19188    19186       -2     
==========================================
+ Hits        15752    15760       +8     
+ Misses       3436     3426      -10     

Impacted file tree graph

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.

Do you want to put this API with the "semi-public" QUIC APIs for now, or do you think it's worth adding to the main APIs?

tls/s2n_connection.c Outdated Show resolved Hide resolved
tests/unit/s2n_handshake_test.c Outdated Show resolved Hide resolved
@camshaft
Copy link
Contributor Author

camshaft commented Feb 1, 2021

Do you want to put this API with the "semi-public" QUIC APIs for now, or do you think it's worth adding to the main APIs?

I think this is worth having outside of QUIC.

tls/s2n_x509_validator.c Outdated Show resolved Hide resolved
tests/unit/s2n_cipher_info_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_cipher_info_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_cipher_info_test.c Outdated Show resolved Hide resolved
tls/s2n_connection.c Outdated Show resolved Hide resolved
tls/s2n_connection.c Outdated Show resolved Hide resolved
tests/unit/s2n_cipher_info_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_cipher_info_test.c Outdated Show resolved Hide resolved
tls/s2n_connection.c Outdated Show resolved Hide resolved
tests/unit/s2n_cipher_info_test.c Show resolved Hide resolved
@camshaft camshaft merged commit 016b7d1 into aws:main Feb 12, 2021
@camshaft camshaft deleted the cipher-iana-id branch February 12, 2021 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants