Add TLS support, gated by a feature flag #305

gferon · 2020-03-31T08:35:43Z

Following up on my earlier attempt in MR #244.

TODO:

Implement TLS for synchronous client with native-tls
Implement TLS with tokio-tls
Implement TLS with async-native-tls
Add testing facilities for redis+tls

Resolves issue #241

src/connection.rs

gferon · 2020-04-01T15:30:11Z

@Marwes I guess I should also add something in the documentation about the new feature flag?

Marwes · 2020-04-02T11:39:50Z

There doesn't seem to be a place where features are documented atm (though it would be nice, problably in the top level docs in src/lib.rs).

Adding some/a test for TLS, even without certificate validation would be useful.

gferon · 2020-04-02T11:48:15Z

There doesn't seem to be a place where features are documented atm (though it would be nice, problably in the top level docs in src/lib.rs).

Adding some/a test for TLS, even without certificate validation would be useful.

Alright, I'll figure something out later then.

…ver when running integration tests

gferon · 2020-04-21T21:49:24Z

@Marwes could you try to help me figure out what's going on with the test suite? It's most probably linked to my change, but I can't figure it out right now.

badboy

I gave this a rough review now.

Good work so far.
I'm surprised how simple the overall logic is.
I had some nits and comments about the feature usage, some clarification there would be nice.
I will also give that another look with a less-sleep brain.

There are some merge conflicts right now, if you can get them fixed that would be helpful.

src/aio.rs

src/aio_async_std.rs

src/connection.rs

Terkwood · 2020-06-05T13:11:21Z

Curious about the build errors here. The summary text says they were stopped, but the logs were a little bit harder to understand -- did someone intervene to stop the builds, or did they just idle for a long time and then fail?

Marwes · 2020-06-05T14:31:28Z

travis stops the build automatically if no output has been received for 10 minutes. So there is a bug in here somewhere.

Terkwood · 2020-06-09T14:35:47Z

src/connection.rs

+        /// You should think very carefully before you use this method. If hostname
+        /// verification is not used, any valid certificate for any site will be
+        /// trusted for use from any other. This introduces a significant
+        /// vulnerability to man-in-the-middle attacks.


Terkwood · 2020-06-09T14:40:17Z

Potentially related, in the travis log: https://travis-ci.org/github/mitsuhiko/redis-rs/jobs/693945209#L253

Warning: The home dir /var/run/stunnel4 you specified can't be accessed: No such file or directory

But probably benign. I worry that something related to travis might be the issue here, not the change set. I'll pull down the repo and try running tests on my own machine.

Terkwood · 2020-06-09T14:50:24Z

On my local machine (a linux laptop built in approximately 1433 AD), I was able to get the tests to pass:

REDISRS_SERVER_TYPE=tcp RUST_TEST_THREADS=1 cargo test --no-default-features --tests -- --nocapture

This is the same spot that failed in the travis build. So I'm not sure there's anything wrong with the change set, and I'm a little bit more suspicious of the travis build's config. Too -- and I haven't dug into this -- some of the other builds attached to the last commit, here, passed.

Just out of sheer masochism, I wonder if re-triggering the builds causes different results than the last run.

Terkwood · 2020-06-09T14:52:08Z

There have been changes since then, but previous iterations of this work got 100% green-lights on Travis:

https://travis-ci.org/github/mitsuhiko/redis-rs/builds/669753751

789b308

Terkwood · 2020-06-09T14:53:13Z

Someone has kindly kicked off the build for the latest commit: https://travis-ci.org/github/mitsuhiko/redis-rs/builds/693945206

Thank you!

Terkwood · 2020-06-09T14:59:14Z

Seems to be hanging at the same spot (test_getset). Run hasn't terminated yet...

Terkwood · 2020-06-09T15:09:52Z

Failures on all of the non-lint builds, but not in the same places. 🔬

gferon · 2020-07-07T20:46:43Z

I was excited when I saw the update to redis 6.0, and I was too fast... the build from the ppa:chris-lea/redis-server isn't built with TLS support 😿

Terkwood · 2020-07-07T20:50:17Z

😔 Thanks for looking into it... Worth a try

This reverts commit da2023b.

gferon · 2020-07-07T21:16:04Z

I finally figured it out.

I introduced the issue in my PR when I changed the match arm in tests/support/mod.rs (see the fix that restores this code to its original state).

Before my change, the net2::TcpListener2 was dropped before the redis-server is spawned. After I changed it, well... it didn't work anymore 💥

I thought I knew enough about sockets... It turns out that SO_REUSEADDR on Linux does not behave exactly as I thought it would. But hey, I learned a lot in the process!

@Terkwood I think it's finally ready to be merged.

Terkwood · 2020-07-07T21:54:15Z

examples/basic.rs

@@ -154,7 +155,12 @@ fn do_redis_code() -> redis::RedisResult<()> {

 fn main() {
    // at this point the errors are fatal, let's just fail hard.
-    match do_redis_code() {
+    let url = if env::args().nth(1) == Some("--tls".into()) {
+        "rediss://127.0.0.1:6380/#insecure"


Terkwood · 2020-07-07T21:57:51Z

Huge progress!! 🏁🏁

badboy · 2020-07-08T08:06:41Z

Awesome!
I'm going to land this.

@gferon Can you add some documentation in lib.rs for this in a follow up PR?

tibbon · 2020-09-21T20:53:54Z

How does one use the TLS feature?

tibbon · 2020-09-21T21:10:14Z

I'm getting this error:

david@MacBook-Pro-2 crane % cargo run -- -tls
   Compiling redis v0.17.0
error[E0432]: unresolved import `async_native_tls`
 --> /Users/david/.cargo/registry/src/github.com-1ecc6299db9ec823/redis-0.17.0/src/aio_async_std.rs:4:5
  |
4 | use async_native_tls::{TlsConnector, TlsStream};
  |     ^^^^^^^^^^^^^^^^ use of undeclared crate or module `async_native_tls`

error[E0433]: failed to resolve: use of undeclared crate or module `tokio_tls`
   --> /Users/david/.cargo/registry/src/github.com-1ecc6299db9ec823/redis-0.17.0/src/aio.rs:100:32
    |
100 |             let tls_connector: tokio_tls::TlsConnector = if insecure {
    |                                ^^^^^^^^^ use of undeclared crate or module `tokio_tls`

error[E0599]: no variant or associated item named `TcpTlsTokio` found for enum `aio::ActualConnection` in the current scope
   --> /Users/david/.cargo/registry/src/github.com-1ecc6299db9ec823/redis-0.17.0/src/aio.rs:113:40
    |
113 |                 .map(ActualConnection::TcpTlsTokio)?)
    |                                        ^^^^^^^^^^^
    |                                        |
    |                                        variant or associated item not found in `aio::ActualConnection`
    |                                        help: there is a variant with a similar name: `TcpTokio`
...
125 | pub(crate) enum ActualConnection {
    | -------------------------------- variant or associated item `TcpTlsTokio` not found here

error[E0599]: no variant or associated item named `TcpTlsAsyncStd` found for enum `aio::ActualConnection` in the current scope
   --> /Users/david/.cargo/registry/src/github.com-1ecc6299db9ec823/redis-0.17.0/src/aio_async_std.rs:161:42
    |
161 |             .map(|con| ActualConnection::TcpTlsAsyncStd(TlsStreamAsyncStdWrapped(con)))?)
    |                                          ^^^^^^^^^^^^^^
    |                                          |
    |                                          variant or associated item not found in `aio::ActualConnection`
    |                                          help: there is a variant with a similar name: `TcpAsyncStd`
    |
   ::: /Users/david/.cargo/registry/src/github.com-1ecc6299db9ec823/redis-0.17.0/src/aio.rs:125:1
    |
125 | pub(crate) enum ActualConnection {
    | -------------------------------- variant or associated item `TcpTlsAsyncStd` not found here

error: aborting due to 4 previous errors

I've included these in my Cargo.toml:

redis = { version = "0.17.0" , features = ["tls"] }
native-tls = { version = "0.2" }
tokio-tls = { version = "0.3" }
async-native-tls = { version = "0.3" }

tibbon · 2020-09-21T21:13:05Z

Seems to be that this is the magic line: redis = { version = "0.17.0" , features = ["tls", "async-std-tls-comp", "tokio-tls-comp"] }

gferon · 2020-09-22T07:31:18Z

Seems to be that this is the magic line: redis = { version = "0.17.0" , features = ["tls", "async-std-tls-comp", "tokio-tls-comp"] }

You're right, I just pushed #382 to make sure those features are more discoverable.

gferon added 3 commits March 31, 2020 10:31

Add TLS support, gated by a feature flag

5f95ee0

Propagate errors when instantiating TlsConnector

0d22d09

Fix unrelated clippy error

3ecbaab

gferon force-pushed the redis-tls branch from 924170d to 3ecbaab Compare April 1, 2020 14:39

Add support to async-native-tls

05b86cb

Marwes reviewed Apr 1, 2020

View reviewed changes

src/connection.rs Outdated Show resolved Hide resolved

Address review comments

789b308

gferon requested a review from Marwes April 2, 2020 10:14

gferon added 2 commits April 7, 2020 23:24

Add a way to generate a self-signed cert, spawn stunnel and redis-ser…

87e48f7

…ver when running integration tests

Make sure stunnel doesn't fork itself

b94207a

gferon closed this Apr 17, 2020

gferon reopened this Apr 17, 2020

weihanglo mentioned this pull request May 1, 2020

Plan for Redis 6 supports #312

Closed

5 tasks

badboy reviewed May 28, 2020

View reviewed changes

gferon added 2 commits June 2, 2020 19:15

Merge remote-tracking branch 'upstream/master' into HEAD

8c922ee

Address review comments

21e478e

gferon requested a review from badboy June 2, 2020 17:29

Terkwood approved these changes Jun 9, 2020

View reviewed changes

gferon force-pushed the redis-tls branch from 1eb2643 to 72c776c Compare June 26, 2020 10:54

gferon added 6 commits June 26, 2020 15:26

Use --test-threads instead of RUST_TEST_THREADS in Makefile

cf389ae

Oops, forgot to kill stunnel

596402d

Retry connecting a little less aggressively

da2023b

Use tempdir for stunnel config

d3baac8

Merge remote-tracking branch 'upstream/master' into redis-tls

d589226

Adding eprinltn everywhere

209c11d

gferon force-pushed the redis-tls branch from 3de1a6a to 209c11d Compare July 7, 2020 18:59

gferon added 3 commits July 7, 2020 21:10

Add pid in stunnel.conf

da94607

Set permissions of tls key to 0644

1261566

Print TCP port we got for redis

2c37e62

Merge remote-tracking branch 'upstream/master' into redis-6.0-tls

7e66621

Restore match to previous state

d331d20

gferon force-pushed the redis-tls branch from c92e0b3 to d331d20 Compare July 7, 2020 20:53

gferon added 4 commits July 7, 2020 23:02

Back to hiding stdout and stderr

0e9f715

Revert "Retry connecting a little less aggressively"

72e03ca

This reverts commit da2023b.

Fix Makefile

66fcafb

Remove chmod, it doesn't work on windows

5a7a108

Terkwood approved these changes Jul 7, 2020

View reviewed changes

badboy merged commit f60933a into redis-rs:master Jul 8, 2020

wraithan mentioned this pull request Aug 27, 2021

Update TLS tests to use redis native TLS instead of stunnel #532

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TLS support, gated by a feature flag #305

Add TLS support, gated by a feature flag #305

gferon commented Mar 31, 2020 •

edited

gferon commented Apr 1, 2020

Marwes commented Apr 2, 2020

gferon commented Apr 2, 2020

gferon commented Apr 21, 2020

badboy left a comment

Terkwood commented Jun 5, 2020 •

edited

Marwes commented Jun 5, 2020

Terkwood Jun 9, 2020

Terkwood commented Jun 9, 2020

Terkwood commented Jun 9, 2020

Terkwood commented Jun 9, 2020

Terkwood commented Jun 9, 2020

Terkwood commented Jun 9, 2020

Terkwood commented Jun 9, 2020

gferon commented Jul 7, 2020

Terkwood commented Jul 7, 2020

gferon commented Jul 7, 2020

Terkwood Jul 7, 2020

Terkwood commented Jul 7, 2020

badboy commented Jul 8, 2020

tibbon commented Sep 21, 2020

tibbon commented Sep 21, 2020

tibbon commented Sep 21, 2020

gferon commented Sep 22, 2020

Add TLS support, gated by a feature flag #305

Add TLS support, gated by a feature flag #305

Conversation

gferon commented Mar 31, 2020 • edited

gferon commented Apr 1, 2020

Marwes commented Apr 2, 2020

gferon commented Apr 2, 2020

gferon commented Apr 21, 2020

badboy left a comment

Choose a reason for hiding this comment

Terkwood commented Jun 5, 2020 • edited

Marwes commented Jun 5, 2020

Terkwood Jun 9, 2020

Choose a reason for hiding this comment

Terkwood commented Jun 9, 2020

Terkwood commented Jun 9, 2020

Terkwood commented Jun 9, 2020

Terkwood commented Jun 9, 2020

Terkwood commented Jun 9, 2020

Terkwood commented Jun 9, 2020

gferon commented Jul 7, 2020

Terkwood commented Jul 7, 2020

gferon commented Jul 7, 2020

Terkwood Jul 7, 2020

Choose a reason for hiding this comment

Terkwood commented Jul 7, 2020

badboy commented Jul 8, 2020

tibbon commented Sep 21, 2020

tibbon commented Sep 21, 2020

tibbon commented Sep 21, 2020

gferon commented Sep 22, 2020

gferon commented Mar 31, 2020 •

edited

Terkwood commented Jun 5, 2020 •

edited