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

hyper-rustls isn't actually supported (v0.20.0) #346

Closed
jsurany opened this issue Apr 18, 2023 · 4 comments
Closed

hyper-rustls isn't actually supported (v0.20.0) #346

jsurany opened this issue Apr 18, 2023 · 4 comments

Comments

@jsurany
Copy link

jsurany commented Apr 18, 2023

error

When building without tls (native-tls) and enabling hyper-rustls in my configuration

# Cargo.toml / dependencies
octocrab = { version = "0.20", default-features = false, features = ["hyper-rustls", "retry", "stream", "timeout", "tracing"] }

attempting to connect gives me the following error

Error: Service Error: error trying to connect: invalid URL, scheme is not http
trace logs

[2023-04-18T17:27:03Z TRACE tower::buffer::service] sending request to buffer worker
[2023-04-18T17:27:03Z TRACE tower::buffer::worker] worker polling for next message
[2023-04-18T17:27:03Z TRACE tower::buffer::worker] processing new request
[2023-04-18T17:27:03Z TRACE tower::buffer::worker] resumed=false message=worker received request; waiting for service readiness
[2023-04-18T17:27:03Z DEBUG tower::buffer::worker] service.ready=true message=processing request
[2023-04-18T17:27:03Z DEBUG octocrab] HTTP; http.method=GET http.url=https://api.github.com/repos/REDACTED/files otel.name="HTTP" otel.kind="client"
[2023-04-18T17:27:03Z TRACE tracing::span::active] -> HTTP;
[2023-04-18T17:27:03Z DEBUG octocrab] requesting
[2023-04-18T17:27:03Z TRACE tracing::span::active] <- HTTP;
[2023-04-18T17:27:03Z TRACE tower::buffer::worker] returning response future
[2023-04-18T17:27:03Z TRACE tower::buffer::worker] worker polling for next message
[2023-04-18T17:27:03Z TRACE tracing::span::active] -> HTTP;
[2023-04-18T17:27:03Z TRACE hyper::client::pool] checkout waiting for idle connection: ("https", api.github.com)
[2023-04-18T17:27:03Z TRACE hyper::client::connect::http] Http::connect; scheme=Some("https"), host=Some("api.github.com"), port=None
[2023-04-18T17:27:03Z TRACE hyper::client::pool] checkout dropped for ("https", api.github.com)
[2023-04-18T17:27:03Z TRACE hyper::client::pool] checkout waiting for idle connection: ("https", api.github.com)
[2023-04-18T17:27:03Z TRACE hyper::client::connect::http] Http::connect; scheme=Some("https"), host=Some("api.github.com"), port=None
[2023-04-18T17:27:03Z TRACE hyper::client::pool] checkout dropped for ("https", api.github.com)
[2023-04-18T17:27:03Z TRACE hyper::client::pool] checkout waiting for idle connection: ("https", api.github.com)
[2023-04-18T17:27:03Z TRACE hyper::client::connect::http] Http::connect; scheme=Some("https"), host=Some("api.github.com"), port=None
[2023-04-18T17:27:03Z TRACE hyper::client::pool] checkout dropped for ("https", api.github.com)
[2023-04-18T17:27:03Z TRACE hyper::client::pool] checkout waiting for idle connection: ("https", api.github.com)
[2023-04-18T17:27:03Z TRACE hyper::client::connect::http] Http::connect; scheme=Some("https"), host=Some("api.github.com"), port=None
[2023-04-18T17:27:03Z TRACE hyper::client::pool] checkout dropped for ("https", api.github.com)
[2023-04-18T17:27:03Z DEBUG octocrab] HTTP; otel.status_code="ERROR"
[2023-04-18T17:27:03Z ERROR octocrab] failed with error error trying to connect: invalid URL, scheme is not http
[2023-04-18T17:27:03Z TRACE tracing::span::active] <- HTTP;
[2023-04-18T17:27:03Z TRACE tracing::span] -- HTTP;
[2023-04-18T17:27:04Z TRACE tower::buffer::worker] buffer already closed

snippet

here's the bit of code

    let crab = OctocrabBuilder::new()
        .personal_token(TOKEN)
        .build()?;
    let stream = crab
        .pulls(ORG, REPO)
        .list_files(PR_NUM)
        .await?; // error here

what I think the issue is

looking at the library, there is nothing actually using the hyper_rustls crate when it is enabled, and since tls is not enabled in my Cargo.toml file, the hyper::client::HttpConnector is what is used for the connector when OctocrabBuilder::build is called. When the resulting request is sent to an https address, this understandably fails because the request is not http.

fix ?

I cloned octocrab locally and made some small changes, notably in OctocrabBuilder::build

     let client: hyper::Client<_, String> = {
            #[cfg(all(not(feature = "tls"), not(feature = "hyper-rustls")))]
            let mut connector = HttpConnector::new();
            #[cfg(all(feature = "tls", not(feature="hyper-rustls")))]
            let connector = HttpsConnector::new();
            #[cfg(all(not(feature = "tls"), feature="hyper-rustls"))]
            let connector = HttpsConnectorBuilder::new()
                .with_native_roots()  // enabled the `rustls-native-certs` feature in hyper-rustls
                .https_only()
                .enable_http1()
                .build();

            #[cfg(feature = "timeout")]
            let connector = self.set_connect_timeout_service(connector);

            hyper::Client::builder().build(connector)
        };

    ...

and for my use case that worked. I'm hesitant to try to open a PR for this because I know that this connector is built differently than with the tls connector and I'm not sure what parameters are best outside of my use case.

@L1ghtman2k
Copy link
Contributor

There was a significant change from 0.19.0 version of octocrab to 0.20.0, where the underlying reqwest library was replaced by hyper/http/tower libraries. As part of that move, the underlying tls libraries were also changed. I believe you are right, in that hyper-rustls isn't being utilized as of 0.20.0 (I believe the entry inside cargo.toml file was forgotten to be cleaned up)

Could you try to instead use the tls feature?

@jsurany
Copy link
Author

jsurany commented Apr 19, 2023

tls isn't ideal because due to constraints in my environment I'd have to statically link the vendored openssl. My preference is rustls.

I can make a PR if this project is open to it.

@XAMPPRocky
Copy link
Owner

XAMPPRocky commented Apr 19, 2023

Thank you for your issue! PRs are always welcome and encouraged. It should be easy and preferred to use rustls in Octocrab.

XAMPPRocky pushed a commit that referenced this issue Apr 24, 2023
* enable rustls, and use rustls as a default HTTPSClientBuilder as per #346

* use https_or_http as mock server currently doesn't support https endpoints, and workarounds are rather ugly
@L1ghtman2k
Copy link
Contributor

this should now be resolved

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

No branches or pull requests

3 participants