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

s2n-tls rust binding: expose selected application protocol #4599

Merged
merged 6 commits into from
Jun 18, 2024

Conversation

zh-jq-b
Copy link
Contributor

@zh-jq-b zh-jq-b commented Jun 12, 2024

Resolved issues:

none

Description of changes:

Add a Connection::selected_application_protocol() method to get the selected ALPN value.

Call-outs:

none

Testing:

Not yet. I didn't found any ALPN related testcases in s2n-tls/src/testing/s2n_tls.rs. Maybe I should add one?

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

Copy link
Contributor

@jmayclin jmayclin left a comment

Choose a reason for hiding this comment

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

Hi Zhang! Thanks for the PR!

I left some comments for you to address, and I'd also ask that you add a test. It would probably be sufficient to just have a single negative and positive case.

bindings/rust/s2n-tls/src/connection.rs Outdated Show resolved Hide resolved
bindings/rust/s2n-tls/src/connection.rs Outdated Show resolved Hide resolved
@zh-jq-b zh-jq-b force-pushed the get_alpn branch 3 times, most recently from cbe0841 to 8e36ebc Compare June 13, 2024 04:05
Copy link
Contributor

@jmayclin jmayclin left a comment

Choose a reason for hiding this comment

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

Sorry for all the nits, but looking good!

bindings/rust/s2n-tls/src/testing/s2n_tls.rs Outdated Show resolved Hide resolved
bindings/rust/s2n-tls/src/testing/s2n_tls.rs Outdated Show resolved Hide resolved
bindings/rust/s2n-tls/src/connection.rs Outdated Show resolved Hide resolved
@jmayclin jmayclin requested a review from lrstewart June 15, 2024 00:53
@lrstewart lrstewart merged commit 933f379 into aws:main Jun 18, 2024
34 checks passed
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