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): Add hyper compatibility crate #4617

Merged
merged 13 commits into from
Aug 1, 2024

Conversation

goatgoose
Copy link
Contributor

@goatgoose goatgoose commented Jun 19, 2024

Resolved issues:

Part of #4618

Description of changes:

Adds a new s2n-tls-hyper crate to provide compatibility with hyper clients.

This crate makes it easier to use s2n-tls as the TLS provider in hyper by exposing an HttpsConnector struct that implements tower_service::Service, allowing for it to be provided to the hyper client builder.

This crate is based on similar crates that provide hyper compatibility to TLS implementations, such as to rustls, via hyper-rustls, and native-tls, via hyper-tls.

Call-outs:

  • The MaybeHttpsStream is currently definitely an https stream, but exists as an enum so it can later be configured to use TCP without TLS.
  • There currently isn't any purpose for the HttpsConnector Builder, but I intend to add at least one option to it later.

Testing:

I added a simple get request test just to check that the HttpsConnector seems to be working, but I plan to add more sophisticated tests in another PR.

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

Co-authored-by: James Mayclin <maycj@amazon.com>
@goatgoose goatgoose marked this pull request as ready for review June 19, 2024 19:43
@goatgoose goatgoose mentioned this pull request Jun 19, 2024
6 tasks
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.

My default inclination, especially for public crates, is to be as generous as possible with the documentation. I think that's especially useful since we (s2n-tls maintainers) don't spent that much time in the HTTP domain.

Along those lines I think that we should generally have doc comments for essentially every public struct. "What is it, what does it do, and when is it used?" Are useful points to start with.

bindings/rust/s2n-tls-hyper/src/connector.rs Outdated Show resolved Hide resolved
bindings/rust/s2n-tls-hyper/src/connector.rs Outdated Show resolved Hide resolved
bindings/rust/s2n-tls-hyper/src/stream.rs Show resolved Hide resolved
bindings/rust/s2n-tls-hyper/README.md Outdated Show resolved Hide resolved
bindings/rust/s2n-tls-hyper/src/connector.rs Show resolved Hide resolved
bindings/rust/s2n-tls-hyper/src/connector.rs Outdated Show resolved Hide resolved
bindings/rust/s2n-tls-hyper/src/connector.rs Outdated Show resolved Hide resolved
bindings/rust/s2n-tls-hyper/src/connector.rs Outdated Show resolved Hide resolved
@goatgoose goatgoose force-pushed the hyper-crate-init branch 4 times, most recently from 71135fc to c5f1bc4 Compare June 24, 2024 16:56
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was getting link errors when building the new doc tests with AWS-LC-FIPS:
https://github.com/aws/s2n-tls/actions/runs/9646640129/job/26603549106?pr=4617

It seems like this is related to this issue: rust-lang/cargo#8531. AWS-LC works around this issue by disabling doc tests when testing FIPS, so I did this as well.

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.

I'm happy with the way that this is looking. We'll probably end up slightly shifting things as development goes along, but this looks like a good foundation.

Edit: Actually forgot about my struct/bound comment. Would like that addressed before approving, but I don't see a way to revoke my approval, lol

.github/workflows/ci_rust.yml Outdated Show resolved Hide resolved
bindings/rust/s2n-tls-hyper/src/stream.rs Show resolved Hide resolved
bindings/rust/s2n-tls-hyper/src/stream.rs Outdated Show resolved Hide resolved
bindings/rust/s2n-tls-hyper/src/stream.rs Outdated Show resolved Hide resolved
@goatgoose goatgoose requested a review from jmayclin June 25, 2024 22:58
@goatgoose goatgoose enabled auto-merge (squash) July 1, 2024 16:28
@dougch dougch disabled auto-merge July 17, 2024 15:24
@lrstewart lrstewart enabled auto-merge (squash) July 31, 2024 20:46
goatgoose and others added 2 commits July 31, 2024 16:56
Co-authored-by: Lindsay Stewart <stewart.r.lindsay@gmail.com>
@lrstewart lrstewart merged commit 5c9d554 into aws:main Aug 1, 2024
35 checks passed
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.

4 participants