Skip to content

[http client]: add HTTPS support #210

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

Merged
merged 18 commits into from
Mar 12, 2021
Merged

[http client]: add HTTPS support #210

merged 18 commits into from
Mar 12, 2021

Conversation

niklasad1
Copy link
Contributor

@niklasad1 niklasad1 commented Feb 12, 2021

Attempt to add support for HTTPS via feature flags for rustls 0.21 for tokio 0.2 and rustls 0.22 tokio 1.0.
It's so damn ugly so I'm almost willing to maintain two different branches instead...

Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

LGTM modulo nits.

It's surprising how hard the "easy" things are sometimes. I don't know how you could have done any better, but it sure is ugly.


#[tokio::test]
async fn https_works() {
let client = HttpClient::new("https://kusama-rpc.polkadot.io", HttpConfig::default()).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't mind this actually, but some would make a big stink about having tests depend on the network.
@maciejhirsz do you have a strong opinion here (if not, then let's keep it as-is)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, I don't know how to test it locally, I suppose I need to create a self-signed certificate on localhost? This was simpler :)

Any suggestions?

@niklasad1
Copy link
Contributor Author

I don't know how you could have done any better, but it sure is ugly.

We could enable TLS globally and make it non-optional, we have it for the websocket client now but I think optional is better even if it's harder to maintain and makes the code a little bit ugglier.

@dvdplm
Copy link
Contributor

dvdplm commented Mar 12, 2021

@niklasad1 is this ready to be merged?

@niklasad1
Copy link
Contributor Author

niklasad1 commented Mar 12, 2021

@niklasad1 is this ready to be merged?

Yeah but I think we should provide a uniform TLS configuration for the clients i.e, either enable TLS for both or make it configurable. I filed #241 but should be addressed in another PR.

Also, #227 could make this cleaner I suppose ^^

@dvdplm
Copy link
Contributor

dvdplm commented Mar 12, 2021

Up to you to merge or not. FWIW I think TLS enabled for both is ok, at least for now.

@niklasad1
Copy link
Contributor Author

I will enable TLS for both it makes configuration simpler for now.

After we remove the tokio features it would be easier for users to configure it.

@niklasad1 niklasad1 merged commit 2efb6f7 into master Mar 12, 2021
@niklasad1 niklasad1 deleted the http-client-https branch March 12, 2021 12:07
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