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

Support rustls #97

Closed
wants to merge 2 commits into from
Closed

Support rustls #97

wants to merge 2 commits into from

Conversation

tyrchen
Copy link
Contributor

@tyrchen tyrchen commented Dec 2, 2023

For certain environments, users have to avoid native-tls. Thus using rustls would be a good alternative.

@loyd
Copy link
Collaborator

loyd commented Dec 5, 2023

Looks great; thanks for your contribution.

I will release the next minor release with breaking changes and disable by default TLS or use rustls as a default backend for TLS.

Sadly, hyper-rustls doesn't support hyper v1 yet: rustls/hyper-rustls#234
Thus, the next release is postponed until it will be migrated.

.with_native_roots()
.https_or_http()
.enable_http2()
.build();
Copy link
Collaborator

@loyd loyd Dec 5, 2023

Choose a reason for hiding this comment

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

It seems that we should use wrap_connector here to provide the configured connector above (with set_keepalive and possibly other options in the future).

Copy link
Contributor

Choose a reason for hiding this comment

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

We'll also need to disable the enforcement of http on the connector here (that only happens in the native-tls connector as of now). I've opened a PR with all these changes here: #102.

@@ -71,6 +71,13 @@ impl Default for Client {
connector
});

#[cfg(feature = "rustls")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should explicitly specify the priority of features (both in the code and readme) if both are enabled (because they can be enabled in different parts of the resulting program):

#[cfg(all(feature = "rustls", not(feature = "tls")))]

@cole-h
Copy link
Contributor

cole-h commented Jan 29, 2024

Now that hyper-rustls has been updated to support Hyper v1, is there anything I can do to help get this merged?

@cole-h
Copy link
Contributor

cole-h commented Jan 31, 2024

(I think) I made all the requested changes in a branch in my fork (https://github.com/cole-h/clickhouse.rs/tree/rustls); here's the commit I made on top of this PR's changes: cole-h@612bd42

Feel free to apply that to this PR; I'd also be happy to open a PR from my branch that includes and replaces this PR, just let me know!

@cole-h cole-h mentioned this pull request Feb 6, 2024
@blind-oracle
Copy link
Contributor

Hey, any chance to merge this? Thanks.

@loyd loyd changed the title support rustls Support rustls Jul 15, 2024
@loyd
Copy link
Collaborator

loyd commented Jul 15, 2024

Now all clickhouse.rs (the master branch), hyper-tls and hyper-rustls support hyper v1, so I'm closing this PR in favor of #102 and after rebasing that branch I'm going to release clickhouse v0.12 with it

@loyd loyd closed this Jul 15, 2024
@loyd loyd reopened this Jul 16, 2024
@loyd loyd closed this Jul 16, 2024
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.

4 participants