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

feat(bindings/s2n-tls): add client_hello_version #4609

Merged
merged 9 commits into from
Jun 28, 2024

Conversation

jmayclin
Copy link
Contributor

Description of changes:

This adds bindings for the s2n_connection_get_client_hello_version method.

Testing:

Added simple test as a sanity check to the rust bindings. Functionality testing is contained in the C codebase.

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 Jun 15, 2024
@jmayclin jmayclin marked this pull request as ready for review June 17, 2024 08:39
@jmayclin jmayclin requested review from lrstewart and goatgoose June 17, 2024 08:40
@@ -857,6 +857,13 @@ impl Connection {
version.try_into()
}

pub fn client_hello_version(&self) -> Result<Version, Error> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is s2n_connection_get_client_hello_version() needed instead of s2n_connection_get_client_protocol_version() or s2n_client_hello_get_legacy_protocol_version()?

Copy link
Contributor

Choose a reason for hiding this comment

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

The fact that this question was asked probably means we need a comment.

I added a comment to s2n_client_hello_get_raw_message about this use case, but it isn't mentioned on s2n_connection_get_client_protocol_version itself. The usage guide also has a section on the different protocol versions, but s2n_connection_get_client_protocol_version isn't even mentioned. Clearly we've got some documentation gaps :(

And of course, updating the C documentation doesn't fix Rust :( #4615

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh wait, there is a note elsewhere in the usage guide about it:

You can determine whether an SSLv2 ClientHello was received by checking the value
of `s2n_connection_get_client_hello_version()`. If an SSLv2 ClientHello was
received, then `s2n_connection_get_client_protocol_version()` will still report
the real protocol version requested by the client.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ohh got it okay, that makes sense. Would this be easier to understand if we just made the rust version be client_hello_is_sslv2() rather than expose an additional protocol version getter?

- switch to is_sslv2
- add comments
@jmayclin jmayclin requested a review from goatgoose June 21, 2024 19:48
@jmayclin jmayclin requested a review from lrstewart June 21, 2024 23:48
@jmayclin jmayclin enabled auto-merge (squash) June 27, 2024 23:30
@jmayclin jmayclin merged commit ed04a08 into aws:main Jun 28, 2024
34 checks passed
@jmayclin jmayclin deleted the tls-record-version-header-bindings branch July 1, 2024 00:55
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.

3 participants