Support rediss:// urls

[freeformz]

rediss:// is a valid scheme based on IANA :
https://www.iana.org/assignments/uri-schemes/prov/rediss

We (Heroku) would like to move secure redis connections forward, but
have this chicken and egg problem with some drivers. Drivers for other
languages already support rediss://

[garyburd]

Why do you need InsecureSkipVerify?

[freeformz]

I suspect it's the sanest option as most setups I've seen in the wild currently make fake certs with the intent to just encrypt the connection (vs. also ensuring you are connecting to what you think you are).

Users who need more security can still DialURL(url, DialTLS(&tls.Config{<better security options>}).

[garyburd]

I prefer that the feature be secure by default and that apps use the option to make it less secure. Does Heroku work without InsecureSkipVerify?

[freeformz]

No. This because we use self signed certs and even if we didn't we would have to restart stunnel (breaking connections) to rotate the cert. Most customers find this objectionable. We've done the same thing with postgres for years.

With that said, if you are set on not having InsecureSkipVerify set then I can make that change. I may follow up later with a pr to add it as a url encoded option as well.

[garyburd]

A couple of more issues:

• The call redis.DialURL("redis://host/db", DialTLS(tlsConfig)) ignores the protocol and uses TLS to dial.
• The application cannot specify the zero TLS config using nil as with tls.Dial.

Because it's possible dial using a TLS connection today using the DialNetDial option, this change can be localized to DialURL. How about adding this to DialURL only with query parameter to specify skip verify?

[freeformz]

What I don't like about pushing the options to the query params (that is where I started) is that:

1. We're making up a URL query scheme that will apply to only this driver. Other drivers for other languages (or go) that may have gone this route will have their own, slightly different set of query params.
2. DialURL gets more complicated.
3. Why make a go developer learn some new made up url encoded syntax (and have to document it) when they likely already know how to configure a &tls.Config{}?
4. It's harder to take an env var and pass it through to DialURL as it will first need to be:

   u, err := url.Parse("rediss://localhost:1234/0")
   if err != nil {
    return err
   }
   v := u.Query()
   v.Set("skip-verify","true")
   u.RawQuery = v.Encode()
   u.String()   // rediss://localhost:1234/0?skip-verify=true

If you are set on query params though I'll do it.

[garyburd]

OK, the query parameter is a bad idea.

Add func DialTLSConfig(*tls.Config) DialOption and use it from DialURL only. This new dial option specifies the config to use when a TLS connection is dialed. The option does not specify that TLS should be used.

[freeformz]

Okay, how about now.

FWIW: I still think InsecureSkipVerify:true is better given the state of probably all redis installations behind an stunnel. But I have no numbers except for our own setups to point to.

[freeformz]

There are some other alternatives here:

(1)

if do.tlsConfig == nil {
  return nil, errors.New("a valid *tls.Config is required")
}

(2)

if do.tlsConfig == nil {
  if do.serverName == "" {
    return nil, errors.New("a valid tlsConfig or serverName is required")
  }
  tlsConfig = &tls.Config{ServerName: do.serverName}
}

The later requires adding a serverName option to dialOptions.

TBH: I don't like either, but nor do I like InsecureSkipVerify:true (but see previous comments on why I think it's sanest atm).

[freeformz]

I guess I could also set a different dialer instead that is a wrapper around tls.Dial like so:

DialNetDial(func(network, addr string) (net.Conn, error) {
  tls.Dial(network, addr, do.tlsConfig)
})

That actually may make more sense.

[freeformz]

And nope, that doesn't work because I won't be able to override things w/o doing this:

c, err := DialURL("rediss://localhost:1234", DialNetDial(func(network, address string) (net.Conn, error) {
  tls.Dial(network, address, &tls.Config{InsecureSkipVerify: true})
})

Which is a lot more verbose and then what's the point in all of this.

[garyburd]

Set the host name unless specified in the TLS config. Close underlying network connection when returning an error.

if do.dialTLS {
    tlsConfig :=  cloneTLSConfig(do.tlsConfig)
    if tlsConfig.SeverName == "" {
                  host, port, err = net.SplitHostPort(addr)
                  if err != nil {
                       netConn.Close()
                       return nil, nil, err
                  }
                  tlsConfig.ServerName = host
    }
    tlsConn := tls.Client(netConn, tlsConfig)
    if err := tlsConn.Handshake(); err != nil {
                    netConn.Close()
        return nil, err
    }
    if !tlsConfig.InsecureSkipVerify {
        if err := tlsConn.VerifyHostname(cfg.ServerName); err != nil {
                            netConn.Close()
            return nil, nil, err
        }
    }
    netConn = tlsConn
}

Copy cloneTLSConfig from the standard libary. The comment for cloneTLSConfig describes why the clone function is needed.

DialURL sets TLS flag only. Do not scribble on caller's slice.

if u.Scheme == "rediss" {
           options := append([]DialOption{func(do *dialOptions) {
        do.dialTLS = true
    }}, options...)
  }

Heroku users can use the function like this:

 c, err := DialURL(os.Getenv("REDIS_URL"), redis.DialTLSConfig(&tls.Config{InsecureSkipVerify:true}))

I think it's fine to add a shorter dial option:

 c, err := DialURL(os.Getenv("REDIS_URL"), redis.DialTLSInsecureSkipVerify(true))

[freeformz]

Making those changes, but....

I'm pretty sure that the manual VerifyHostname call isn't required as Handshake() calls clientHandshake, which makes a clientHandshakeState and when not resuming a connection will call the clientHandshakeState's doFullHandshake(), which if !InsecureSkipVerify then the x509 cert's Verify() method is called, which calls the cert's VerifyHostname method. This is (AFAICT) the same thing that the config's VerifyHostname method does.

Oddly enough the net/http code also calls VerifyHostname manually casting doubt on what I found, so I'm just not sure ATM, so I'll leave in the what appears to be an extra manual VerifyHostname and ask around.

[garyburd]

The net/http code is the inspiration for the snippet in my previous comment. Now that you explain it, it looks like the VerifyHostname is not required. Thank you for investigating this.

[freeformz]

How's this?

[garyburd]

It looks good. I'd like to add a test, but the test requires some work (setup localhost tunnel using crypto.tls package) . I'll look at doing that later. Did you test the PR with your service?

[freeformz]

I did some manual (not super comprehensive) testing https://github.com/freeformz/testrediss/blob/master/redigo/main.go

[freeformz]

PS: If you choose to test something on Heroku, then note that the hobby plan does not support SSL and that the urls returned for premium-* plans are not rediss://, hence this function for now: https://github.com/freeformz/testrediss/blob/master/test.go#L9

[garyburd]

Thank you for implementing this feature!

[freeformz]

Thanks for being patient. ;-)