Simple TLSv1 - Githubissues

alieander commented 7 years ago

This implements a TLSv1 flag for servers that are not looking to do full cert/key authentication.

Changes

Add an additional optional option (tls_v1) that can toggle the use of an SSLSocket without the need for a full ssl_context
Ensure TLS works with this new flag alone, with this flag and a passed ssl_context, or with just a ssl_context
Small test using this flag through consumption and production

Fixes https://github.com/wistia/nsq-ruby/issues/24

ghost commented 7 years ago

@alieander, thanks for your PR! By analyzing the annotation information on this pull request, we identified @bschwartz, @mmoutenot and @freerobby to be potential reviewers

standingwave commented 7 years ago

One question but otherwise LGTM. 👍

bschwartz commented 7 years ago

Whoa, you're speedy @alieander! Code looks great to me. Question about naming though. Feels a bit weird to have both tls_v1 and ssl_context -- they're very connected and there's room for some confusion (i.e. tls_v1: false, ssl_context: {...} will still connect via TLS).

Would it be crazy to do this something like this:

# with auth
producer = Nsq::Producer.new(
  nsqlookupd: ['1.2.3.4:4161', '6.7.8.9:4161'],
  topic: 'topic-of-great-esteem',
  ssl: {
    key: '/path/to/ssl/key.pem',
    certificate: '/path/to/ssl/certificate.pem'
  }
)

# simple TLS
producer = Nsq::Producer.new(
  nsqlookupd: ['1.2.3.4:4161', '6.7.8.9:4161'],
  topic: 'topic-of-great-esteem',
  ssl: true
)

We would map ssl_context to ssl to maintain backwards compatibility, of course. The big downside of this to me is that ssl could be a boolean or a hash and that seems a bit odd. The nice part is it feels pretty simple and straightforward.

Thoughts?

cc @freerobby

bschwartz commented 7 years ago

Thanks @alieander! I was thinking that we'd just use ssl. So what was ssl_context just becomes ssl and you can specify a boolean or a hash with key & cert. So if ssl is truthy, we connect via TLS. And if it's a hash with a key and cert, we do full auth.

We'd keep ssl_context around behind the scenes (@ssl = opts[:ssl] || opts[:ssl_context]) to maintain backwards compatibility, but anyone new just does this:

# with auth
producer = Nsq::Producer.new(
  nsqlookupd: ['1.2.3.4:4161', '6.7.8.9:4161'],
  topic: 'topic-of-great-esteem',
  ssl: {
    key: '/path/to/ssl/key.pem',
    certificate: '/path/to/ssl/certificate.pem'
  }
)

Again, I don't have a super strong opinion on this. Curious what y'all think makes sense. This approach definitely has some downsides.

Also, kind of related, I think I'll add a CHANGELOG to this project. Should make discovery of all this stuff easier.

freerobby commented 7 years ago

@alieander Thanks for putting this together. Your implementation looks great to me. My only question, like @bschwartz, is what the best interface is.

From the discussion so far, there are two things I'd like to resolve:

Setting tls_v1: false and still connecting via TLS is confusing.
Changing the default of whether to connect via TLS based on whether some other options are set creates an ambiguous specification. Certainly it makes sense as a practical matter and I expect people to configure that accordingly, but I'd rather leave it to the developer to resolve a weird situation than make a reasonable but hidden assumption in the spec itself.

Here's an alternative that I think achieves both of these goals. What if TLS is enabled by default in all situations. This would invite the following usages:

# TLS enabled
producer = Nsq::Producer.new(
  nsqlookupd: ['1.2.3.4:4161', '6.7.8.9:4161'],
  tls_v1: {
    key: '/path/to/ssl/key.pem',
    certificate: '/path/to/ssl/certificate.pem'
  }
)

# TLS enabled
producer = Nsq::Producer.new(
  nsqlookupd: ['1.2.3.4:4161', '6.7.8.9:4161']
)

# TLS disabled
producer = Nsq::Producer.new(
  nsqlookupd: ['1.2.3.4:4161', '6.7.8.9:4161'],
  tls_v1: {
    enabled: false
  }
)

It would also allow the following usages:

# TLS enabled
producer = Nsq::Producer.new(
  nsqlookupd: ['1.2.3.4:4161', '6.7.8.9:4161'],
  tls_v1: {
    enabled: true
  }
)

# TLS enabled
producer = Nsq::Producer.new(
  nsqlookupd: ['1.2.3.4:4161', '6.7.8.9:4161'],
  tls_v1: {
    key: '/path/to/ssl/key.pem',
    certificate: '/path/to/ssl/certificate.pem',
    enabled: true
  }
)

# TLS disabled
producer = Nsq::Producer.new(
  nsqlookupd: ['1.2.3.4:4161', '6.7.8.9:4161'],
  tls_v1: {
    key: '/path/to/ssl/key.pem',
    certificate: '/path/to/ssl/certificate.pem',
    enabled: false
  }
)

This still allows for a weird case of specifying a key and certificate while disabling SSL, but it requires the developer to set that weird case explicitly, if they want it. It also makes SSL opt-out rather than opt-in, which I think is good (provided we add a major version bump).

bschwartz commented 7 years ago

@freerobby I'd prefer to keep it TLS off by default unless we really know that the vast majority of folks use it (since it's a breaking change, as you pointed out). I don't have a great sense of the numbers here, but if I had to guess I'd think there's a lot of non-TLS usage (especially on the producer side). Also, feels weird to bump a major version just to make TLS the default with no other big features or benefits.

freerobby commented 7 years ago

@bschwartz I like enabling TLS by default because when people don't think about something, we should do the most responsible thing for them, and I think encrypting traffic exemplifies that.

On release versioning, I know it's common to combine sets of breaking changes into a single major release, but I think that's a practice that developed from calendar-based lifecycles, i.e. if you are planning to update something every two years, it makes sense for your vendor to fold breaking changes in on that schedule. That's not how we operate though, and with semver, major releases don't have to be a big event; it's just a qualitative distinction about what kind of change people should expect when they upgrade. I'd rather get better interfaces into people's hands quickly, even if it means releasing five major versions in a year instead of two. It might look like a big deal on a Rubygems version list, but it's not actually any more work for users to vet over that timespan.

I defer to you on the best course of action here -- just wanted to put those thoughts in writing.

freerobby commented 7 years ago

@alieander I just talked with @bschwartz out of band. We looked at what other libraries[1][2] do here and it's what you originally had, except that if someone passes in a key/cert and disables tls, we disable tls instead of enabling it. Since I took us on a runaway train here I will send you a PR for that. :) If you like it, you can fold it into yours and then we'll be good to go. Thanks for bearing with!

[1]https://github.com/nsqio/go-nsq/blob/master/config.go#L155 [2]http://pynsq.readthedocs.io/en/latest/async_conn.html?highlight=tls

alieander commented 7 years ago

@freerobby @bschwartz No worries at all! This sounds great and I am always glad to help though hopefully I didn't derail to many too much! 😄

It is always good to have good discussion on the software we make and use.

To comment on the TLS first idea: I really like the idea of having stronger defaults. It might be a service to the idea to set it aside for further discussion in a PR of its own.

freerobby commented 7 years ago

@alieander I have this working to spec on a branch, but I'm having trouble testing TLS in isolation, and I want to be sure I'm goofing before I send it your way. Specifically, I see:

OpenSSL::SSL::SSLError: SSL_connect returned=1 errno=0 state=SSLv2/v3 read server hello A: unknown protocol
./lib/nsq/connection.rb:360:in `connect'
./lib/nsq/connection.rb:360:in `upgrade_to_ssl_socket'
./lib/nsq/connection.rb:324:in `open_connection'
./lib/nsq/connection.rb:53:in `initialize'
./lib/nsq/client_base.rb:90:in `new'
./lib/nsq/client_base.rb:90:in `add_connection'
./lib/nsq/consumer.rb:80:in `add_connection'
./lib/nsq/consumer.rb:40:in `initialize'
./spec/spec_helper.rb:57:in `new'
./spec/spec_helper.rb:57:in `new_consumer'
./spec/lib/nsq/consumer_spec.rb:18:in `block (3 levels) in <top (required)>'
-e:1:in `load'
-e:1:in `<main>'

As I mention above, it seems to work when tested via a consumer/producer as you have done, but in isolation here it fails reliably. Is there anything I might be missing here, maybe some indirect setup work?

If it's easier, a quick way to reproduce this on your branch is to go into spec_helper.rb, and in new_consumer, add tls_v1: true to the end of the hash, and then run consumer_spec.rb.

I'm running nsq v0.3.7

alieander commented 7 years ago

@freerobby I think you are attempting to connect to a NON TLS enabled nsqd https://github.com/wistia/nsq-ruby/blob/rg-tls-v1-simple/spec/lib/nsq/connection_spec.rb#L5-L7

There are tls specific spec support helpers here: https://github.com/wistia/nsq-ruby/tree/rg-tls-v1-simple/spec/support

... I am using them in other tls tests here as an example: https://github.com/wistia/nsq-ruby/blob/rg-tls-v1-simple/spec/lib/nsq/tls_connection_spec.rb#L15-L26

freerobby commented 7 years ago

@alieander Gotcha, thanks. I did not realize when I wrote that that we never added native TLS support to nsq-cluster, so I was confused why it worked in some places and not others. Should have seen you were passing in those certs! Will go back and use those and send your way shortly. Thanks!

freerobby commented 7 years ago

Discussion moved to https://github.com/wistia/nsq-ruby/pull/26

wistia / nsq-ruby

Simple TLSv1 #25

Changes