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

Add rustls support behind a non-default feature. #125

Merged
merged 2 commits into from
Apr 13, 2020
Merged

Add rustls support behind a non-default feature. #125

merged 2 commits into from
Apr 13, 2020

Conversation

kiljacken
Copy link
Contributor

This commit adds support for using rustls with stripe-rs. This avoids a dependency on native OpenSSL for people who'd rather avoid that. This is useful when building a static binary as you don't need to build a statically linkable OpenSSL first.

@kiljacken kiljacken changed the title Add rustls support behind a non-default feature. WIP: Add rustls support behind a non-default feature. Feb 18, 2020
This commit adds support for using rustls with stripe-rs. This avoids a
dependency on native OpenSSL for people who'd rather avoid that. This is
useful when building a static binary as you don't need to build a
statically linkable OpenSSL first.
@kiljacken kiljacken changed the title WIP: Add rustls support behind a non-default feature. Add rustls support behind a non-default feature. Feb 18, 2020
@kiljacken
Copy link
Contributor Author

This should be ready for review now.

This is a breaking change for users using no-default-features as we need the default tls implementation to be a feature to not pull in OpenSSL no matter what. If rust-lang/cargo#5954 was implemented this would not be necessary, but that's not how the world is currently.

@kestred
Copy link
Collaborator

kestred commented Feb 22, 2020

This is a breaking change for users using no-default-features as we need the default tls implementation to be a feature to not pull in OpenSSL no matter what.

Backward compatibility can be maintained by using a similar pattern that I adopted for blocking vs. async (where previously blocking was the default).

In this pattern, the newly introduced option becomes:

#[cfg(feature = "rustls-tls")]

And the existing feature is handled with:

#[cfg(any(feature = "default-tls", not(feature = "rustls-tls")))]

// Alternatively, just the below, with no separate feature flag:
#[cfg(not(feature = "rustls-tls"))]

@kiljacken
Copy link
Contributor Author

The main issue with this is that we can't make the hyper-tls dependency optional if it's not behind a feature, so you'll have to link a native openssl no matter what with that approach. I'm not sure if there's a great way to make this change without being breaking, without the linked cargo issue being implemented at some point.

The main reason i opened this PR was to eleminate the need to link to OpenSSL to simplify the flow of building a static binary for e.g. container or AWS Lambda use.

@kestred
Copy link
Collaborator

kestred commented Apr 13, 2020

The next release will be a "major" version, so I'll merge this in.

@kestred kestred merged commit 3433cb1 into wyyerd:master Apr 13, 2020
@kiljacken kiljacken deleted the rustls branch April 13, 2020 15:57
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.

2 participants