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 append protocol preferences #2534

Merged
merged 9 commits into from
Feb 2, 2021

Conversation

camshaft
Copy link
Contributor

@camshaft camshaft commented Jan 25, 2021

Description of changes:

The current s2n_{config,connection}_set_protocol_preferences use null-terminated strings to pass ALPN values. This is a bad idea for a few reasons:

  • right now it's impossible to have an ALPN value with a \0 (which is technically allowed by the spec).
  • Passing a value with a null byte in the middle will silently accept half of the provided string without any indication that the rest was rejected.
  • It forces the caller to copy strings into a temporary buffer when the string isn't null terminated.
  • it's inefficient to iterate through the whole string just to find the length.

This PR adds s2n_{config,connection}_append_protocol_preference methods which take an explicit pointer and length and then copy the value into the protocol_preferences blob. This should fix all of the issues stated above.

I've also added a minimum length check (required by the spec), added tests, and cleaned up a memory leak that happened in the previous version when a protocol value failed to validate.

I used doxygen-style documentation for the new methods in anticipation of that being used in the near future.

Testing:

There is now a dedicated s2n_protocol_preference_test unit test for checking the behavior of the API. Before it was implicitly checked in other tests. I've added a lot of validation checks in this new module as well.

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

@codecov
Copy link

codecov bot commented Jan 25, 2021

Codecov Report

Merging #2534 (9c16020) into main (5962569) will increase coverage by 0.07%.
The diff coverage is 97.29%.

@@            Coverage Diff             @@
##             main    #2534      +/-   ##
==========================================
+ Coverage   82.10%   82.17%   +0.07%     
==========================================
  Files         272      272              
  Lines       19095    19095              
==========================================
+ Hits        15677    15691      +14     
+ Misses       3418     3404      -14     

Impacted file tree graph

@camshaft camshaft marked this pull request as ready for review January 26, 2021 00:03
api/s2n.h Outdated Show resolved Hide resolved
api/s2n.h Outdated Show resolved Hide resolved
tls/s2n_protocol_preferences.c Show resolved Hide resolved
tls/s2n_protocol_preferences.c Outdated Show resolved Hide resolved
tls/s2n_protocol_preferences.c Outdated Show resolved Hide resolved
tls/s2n_protocol_preferences.c Outdated Show resolved Hide resolved
tls/s2n_connection.c Outdated Show resolved Hide resolved
tls/s2n_protocol_preferences.c Outdated Show resolved Hide resolved
tls/s2n_protocol_preferences.c Show resolved Hide resolved
tls/s2n_protocol_preferences.c Show resolved Hide resolved
tls/s2n_connection.c Outdated Show resolved Hide resolved
tests/unit/s2n_protocol_preferences_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_protocol_preferences_test.c Outdated Show resolved Hide resolved
tls/s2n_protocol_preferences.c Outdated Show resolved Hide resolved
tls/s2n_protocol_preferences.c Outdated Show resolved Hide resolved
@camshaft camshaft merged commit d356b58 into aws:main Feb 2, 2021
@camshaft camshaft deleted the protocol-preferences-append branch February 2, 2021 20:30
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