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 feature #575

Closed
enfipy opened this issue Jul 28, 2020 · 10 comments
Closed

Add rustls feature #575

enfipy opened this issue Jul 28, 2020 · 10 comments
Labels
enhancement New feature or request needs rust feature This issue or PR needs a Rust feature to be stabilized before it can be merged into master

Comments

@enfipy
Copy link

enfipy commented Jul 28, 2020

Currently sqlx-rt uses native-tls. But library native-tls is dynamically linked and it's hard sometimes to set up the environment for it (ex. docker image).

But hopefully, there are rustls and tokio-rustls that can statically link SSL during compilation. I think it will be useful for many developers to have it.

Similar feature exists in actix-web.

@mehcode
Copy link
Member

mehcode commented Jul 28, 2020

It's not possible for Cargo to activate a feature dependent on another feature in a workable fashion. We would want tls-native and tls-rustls as features (perhaps with the first being default). tls-native would need to turn on tokio-tls if runtime-tokio or async-native-tls if runtime-actix-std.

So this is blocked on rust-lang/cargo#3494

I definitely do want this though.

@mehcode mehcode added enhancement New feature or request needs rust feature This issue or PR needs a Rust feature to be stabilized before it can be merged into master labels Jul 28, 2020
@olvyko
Copy link

olvyko commented Jul 28, 2020

@mehcode Maybe there is some workaround for this, because we really need rustls. Or only a fork help in this case?

@JADSN
Copy link

JADSN commented Jul 28, 2020

Besides, with rustls support we can build sqlx to musl target.

@mehcode
Copy link
Member

mehcode commented Jul 28, 2020

I'm not aware of a way to work-around this outside of a fork. If you can figure out a way, I'll merge a PR.


Besides, with rustls support we can build sqlx to musl target.

You can do that now by vendoring OpenSSL. We (LaunchBadge) do this for our production services.

See #473 (comment)

@olvyko
Copy link

olvyko commented Jul 28, 2020

One way is to add two more features 'runtime-actix-rustls' and 'runtime-tokio-rustls' to sqlx-rt.
https://github.com/olvyko/sqlx/commit/0fd74fa0c9c8f16e55d540b89792b9946d776332
But as I see, sqlx-core depends on native-tls code that exported from sqlx-rt so it should be changed too

@repnop
Copy link
Contributor

repnop commented Aug 3, 2020

Sorry if this is a silly question, but is there a reason to not switch to rustls wholesale and get rid of native-tls?

@jplatte
Copy link
Contributor

jplatte commented Sep 29, 2020

One way is to add two more features 'runtime-actix-rustls' and 'runtime-tokio-rustls' to sqlx-rt.
olvyko@0fd74fa
But as I see, sqlx-core depends on native-tls code that exported from sqlx-rt so it should be changed too

I don't think passing through the feature from one sqlx crate to another is a problem. The only problem I can see is that this is a breaking change. Since there is no default runtime, you also couldn't set a default [TLS backend + runtime combination] and it would have to be selected explicitly, like runtimes now.

@mehcode Would you be open to a PR that doubles the amount of "runtimes" by having a native-tls and a rustls variant of each (or triples it to also allow no tls)?

@Absolucy
Copy link
Contributor

Absolucy commented Oct 8, 2020

There's mostly API-identical alternatives to tokio-native-tls and async-native-tls: tokio-rustls and async-tls

tokio-rustls is being stubborn to convert to, though?

error[E0308]: mismatched types
  --> sqlx-core/src/net/tls.rs:49:26
   |
49 |                 .connect(host, stream)
   |                          ^^^^ expected struct `sqlx_rt::tokio_rustls::webpki::DNSNameRef`, found `&str`

error[E0308]: mismatched types
  --> sqlx-core/src/net/tls.rs:49:26
   |
49 |                 .connect(host, stream)
   |                          ^^^^ expected struct `sqlx_rt::tokio_rustls::webpki::DNSNameRef`, found `&str`

error[E0308]: try expression alternatives have incompatible types
  --> sqlx-core/src/net/tls.rs:48:13
   |
48 | /             connector
49 | |                 .connect(host, stream)
50 | |                 .await
51 | |                 .map_err(|err| Error::Tls(err.into()))?,
   | |_______________________________________________________^ expected enum `sqlx_rt::TlsStream`, found struct `sqlx_rt::tokio_rustls::client::TlsStream`
   |
   = note: expected enum `sqlx_rt::TlsStream<S>`
            found struct `sqlx_rt::tokio_rustls::client::TlsStream<S>`
help: try using a variant of the expected enum
   |
48 |             sqlx_rt::TlsStream::Client(connector
49 |                 .connect(host, stream)
50 |                 .await
51 |                 .map_err(|err| Error::Tls(err.into()))?),
   |

error[E0308]: try expression alternatives have incompatible types
  --> sqlx-core/src/net/tls.rs:48:13
   |
48 | /             connector
49 | |                 .connect(host, stream)
50 | |                 .await
51 | |                 .map_err(|err| Error::Tls(err.into()))?,
   | |_______________________________________________________^ expected enum `sqlx_rt::TlsStream`, found struct `sqlx_rt::tokio_rustls::client::TlsStream`
   |
   = note: expected enum `sqlx_rt::TlsStream<S>`
            found struct `sqlx_rt::tokio_rustls::client::TlsStream<S>`
help: try using a variant of the expected enum
   |
48 |             sqlx_rt::TlsStream::Client(connector
49 |                 .connect(host, stream)
50 |                 .await
51 |                 .map_err(|err| Error::Tls(err.into()))?),
   |

error[E0599]: no method named `get_ref` found for tuple `(&S, &dyn sqlx_rt::tokio_rustls::rustls::Session)` in the current scope
   --> sqlx-core/src/net/tls.rs:170:51
    |
170 |             MaybeTlsStream::Tls(s) => s.get_ref().get_ref().get_ref(),
    |                                                   ^^^^^^^ method not found in `(&S, &dyn sqlx_rt::tokio_rustls::rustls::Session)`

error[E0599]: no method named `get_ref` found for tuple `(&S, &dyn sqlx_rt::tokio_rustls::rustls::Session)` in the current scope
   --> sqlx-core/src/net/tls.rs:170:51
    |
170 |             MaybeTlsStream::Tls(s) => s.get_ref().get_ref().get_ref(),
    |                                                   ^^^^^^^ method not found in `(&S, &dyn sqlx_rt::tokio_rustls::rustls::Session)`

error[E0599]: no method named `get_mut` found for tuple `(&mut S, &mut dyn sqlx_rt::tokio_rustls::rustls::Session)` in the current scope
   --> sqlx-core/src/net/tls.rs:189:51
    |
189 |             MaybeTlsStream::Tls(s) => s.get_mut().get_mut().get_mut(),
    |                                                   ^^^^^^^ method not found in `(&mut S, &mut dyn sqlx_rt::tokio_rustls::rustls::Session)`

error[E0599]: no method named `get_mut` found for tuple `(&mut S, &mut dyn sqlx_rt::tokio_rustls::rustls::Session)` in the current scope
   --> sqlx-core/src/net/tls.rs:189:51
    |
189 |             MaybeTlsStream::Tls(s) => s.get_mut().get_mut().get_mut(),
    |                                                   ^^^^^^^ method not found in `(&mut S, &mut dyn sqlx_rt::tokio_rustls::rustls::Session)`

error: aborting due to 4 previous errors; 4 warnings emitted

@jplatte
Copy link
Contributor

jplatte commented Oct 20, 2020

#735 implements this and is now ready for review 🙂

@jplatte
Copy link
Contributor

jplatte commented Nov 18, 2020

#735 was merged in a way that GitHub didn't pick up (and is part of SQLx 0.4), this can be closed 🙂

@enfipy enfipy closed this as completed Nov 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs rust feature This issue or PR needs a Rust feature to be stabilized before it can be merged into master
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants