d-e-s-o / apca

A crate for interacting with the Alpaca API at alpaca.markets.
GNU General Public License v3.0
145 stars 37 forks source link

Optionally use rustls instead of hyper-tls #57

Open carlosskii opened 1 year ago

carlosskii commented 1 year ago

I'm cross-compiling a bot to x86_64-unknown-linux-musl. This crate requires hyper-tls, which requires native-tls, which uses OpenSSL. The build script for OpenSSL always fails, as it cannot find the proper cross compiler. While I could install a cross-compiler and proceed as normal, this is generally not recommended. Instead, crates can often use rustls instead of OpenSSL, which does not require a cross-compiler.

The hyper crate says that rustls is possible. I'm not 100% sure what specific logic is needed to do this, but not much will change anyway. The hyper-tls connector just needs to be swapped with a rustls connector.

This would be an opt-in feature, but having a feature flag for this would make cross-compiling and no-dynamic-library builds much easier.

I will be implementing this in a public fork regardless, since I need it for my bot. Would you like it pulled into the main repository when ready?

Thanks!

d-e-s-o commented 1 year ago

I'm cross-compiling a bot to x86_64-unknown-linux-musl. This crate requires hyper-tls, which requires native-tls, which uses OpenSSL. The build script for OpenSSL always fails, as it cannot find the proper cross compiler.

You can use the vendored OpenSSL. E.g., https://github.com/d-e-s-o/apcacli/blob/main/.github/workflows/build.yml

Edit: Though I suppose it's using a cross compiler, so perhaps not what you want. Not sure why using one would not be recommended? (and by whom?)

I will be implementing this in a public fork regardless, since I need it for my bot. Would you like it pulled into the main repository when ready?

I think it may be useful, yeah; feel free to contribute it back.

carlosskii commented 1 year ago

I added a rustls option to my fork. It's not complete yet, but it should be working (basic checks from my bot are passing). I'll test it for a little longer, then re-open the pull request.

One more thing: I needed to change how WebSocket connections are formed. More specifically, I had to manually set TLS configuration settings. native-tls can rarely return an error that cannot be handled outside of apca. Should I create an error variant for it, or just use the standard Str error variant since it cannot be handled externally?

d-e-s-o commented 1 year ago

One more thing: I needed to change how WebSocket connections are formed. More specifically, I had to manually set TLS configuration settings. native-tls can rarely return an error that cannot be handled outside of apca. Should I create an error variant for it, or just use the standard Str error variant since it cannot be handled externally?

Will probably have to look at the code. If it's only on one or two code paths it may be fine to use Str.

jaredmcqueen commented 1 year ago

I'm interested in this feature too, as my container builds result in:

error: WebSocket(Tls(Native(Ssl(Error { code: ErrorCode(1), cause: Some(Ssl(ErrorStack([Error { code: 337047686, library: "SSL routines", function: "tls_process_server_certificate", reason: "certificate verify failed", file: "ssl/statem/statem_clnt.c", line: 1919 }]))) }, X509VerifyResult { code: 20, error: "unable to get local issuer certificate" }))))

Adding this line in my Dockerfile fixes things: RUN apt-get update && apt-get install -y ca-certificates && rm -rf /var/lib/apt/lists/*

d-e-s-o commented 1 year ago

@carlosskii do you intend to open a pull request?