Problem with verify_peer: false

[hading]

Going from Bunny 2.7.0 to 2.9.1 I found that verify_peer: false was no longer being respected when in my configuration options for a connection. I think I have a handle on the problem.

From an irb session with 2.9.1:

irb(main):028:0> b = Bunny.new(ssl: true, host: 'rabbitmq.host', user: 'user', password: 'password', vhost: 'downloader', verify_peer: false)
W, [2018-01-23T17:32:08.049997 #34207]  WARN -- #<Bunny::Session:0x7f8d4a1fe698 @rabbitmq.host:5671, vhost=, addresses=[rabbitmq.host:5671]>: Using TLS but no client certificate is provided! If RabbitMQ is configured to verify peer
certificate, connection upgrade will fail!

=> #<Bunny::Session:0x7f8d4a1fe698 user@rabbitmq.host:5671, vhost=downloader, addresses=[rabbitmq.host:5671]>
irb(main):029:0> t = b.transport
=> #<Bunny::Transport:0x007f8d4a1fd248 @session=#<Bunny::Session:0x7f8d4a1fe698 user@rabbitmq.host:5671, vhost=downloader, addresses=[rabbitmq.host:5671]>, @session_thread=#<Thread:0x007f8d4a07ef70 run>, @host="rabbitmq.host", @port=5671, @opts={:ssl=>true, :host=>"rabbitmq.host", :user=>"user", :password=>"password", :vhost=>"downloader", :verify_peer=>true, :session_thread=>#<Thread:0x007f8d4a07ef70 run>}, @logger=#<Logger:0x007f8d4a1fe2b0 @level=2, @progname="#<Bunny::Session:0x7f8d4a1fe698 user@rabbitmq.host:5671, vhost=downloader, addresses=[rabbitmq.host:5671]>", @default_formatter=#<Logger::Formatter:0x007f8d4a1fe210 @datetime_format=nil>, @formatter=nil, @logdev=#<Logger::LogDevice:0x007f8d4a1fe1c0 @shift_period_suffix=nil, @shift_size=nil, @shift_age=nil, @filename=nil, @dev=#<IO:<STDOUT>>, @mon_owner=nil, @mon_count=0, @mon_mutex=#<Thread::Mutex:0x007f8d4a1fe170>>>, @tls_enabled=true, @read_timeout=30.0, @write_timeout=30.0, @connect_timeout=30.0, @disconnect_timeout=30.0, @writes_mutex=#<Monitor:0x007f8d4a1fd1a8 @mon_owner=nil, @mon_count=0, @mon_mutex=#<Thread::Mutex:0x007f8d4a1fd158>>, @tls_certificate_path=nil, @tls_key_path=nil, @tls_certificate=nil, @tls_key=nil, @tls_certificate_store=nil, @tls_ca_certificates=["/usr/local/etc/openssl/cert.pem", "/usr/local/etc/openssl/certs/ca-certificates.crt", "/usr/local/etc/openssl/certs/ca-bundle.crt", "/usr/local/etc/openssl/certs/ca-bundle.pem"], @verify_peer=true, @tls_context=#<OpenSSL::SSL::SSLContext:0x007f8d4a1fcdc0 @cert_store=#<OpenSSL::X509::Store:0x007f8d4a1fca78 @verify_callback=nil, @error=nil, @error_string=nil, @chain=nil, @time=nil>, @verify_mode=3>>

Note that in @opts for the transport, :verify_peer is now true.

The problem appears to be in transport.rb in the prepare_tls_context method, in this section:

if (opts[:verify_ssl] || opts[:verify_peer] || opts[:verify]).nil?
        opts[:verify_peer] = true
  end

It's best illustrated by another little irb snippet:

irb(main):030:0> false || nil
=> nil
irb(main):031:0> nil || false
=> false

I confirmed that if I also pass verify: false then things are back to normal. It's unclear to me what the section of code referenced above is really supposed to be doing, as the verify_peer instance variable is set more robustly later in the same method.

[michaelklishin]

Both nil and false evaluate to false in boolean expressions. 3914c06 adds :verify to the list of the keys to be more unified with the options of RabbitMQ server.

This means that if opts[:verify] is nil, the entire expression evaluates to true. Removing opts[:verify] from the expression should restore the original behaviour but it kind of goes against the spirit of #534 (which has had a couple of unforeseen changes to date).

Perhaps opts[:verify_ssl] || opts[:verify_peer] || opts[:verify] should instead be opts[:verify_ssl] && opts[:verify_peer] &&| opts[:verify]. In other words, if neither of the values are set, default to true.

[hading]

They both evaluate to 'falsey', but they don't explicitly evaluate to 'false' (this is in Ruby 2.4.1.p111). Again,

irb(main):030:0> false || nil
=> nil
irb(main):031:0> nil || false
=> false

However, in the method, the check is done with nil?, which evaluates to true in one case and false in the other, i.e.

irb(main):032:0> (false || nil).nil?
=> true
irb(main):033:0> (nil || false).nil?
=> false

So if both verify_ssl and verify_peer are falsey, then the value of opts[:verify] will determine what happens, and the result is different depending on whether it is unset (nil) or explicitly set to false. Which doesn't seem like what one would expect. (I expect to just have to set verify_peer to false.)

I'm not familiar with the code really, but why is this potential resetting of opts[:verify_peer] even needed? It looks like @verify_peer is set correctly later in the method except for the fact that opts[:verify_peer] may have been modified already.

[michaelklishin]

What one would expect depends on how many keys one is aware of. As of 2.9.0 there is one more key. We can lower its priority or we can declare that things work as expected (for 2.9.0). There was a version bump for a reason.

[hading]

That's fine too, but the docs don't really reflect that if you want to skip peer verification that setting verify_peer: false isn't enough to do so.

[michaelklishin]

I'm fine with a PR that gives verify_peer the highest priority for backwards compatibility.

[hading]

What is the intent of that bit of code? Is it to set a default if none of those three keys are specified? If so then that still doesn't do it. I'd think you'd want:

opts[:verify_ssl].nil? && opts[:verify_peer].nil? && opts[:verify].nil?
or the equivalent. Just changing the order of the keys in the existing statement just creates the same problem if they are supposed to be equivalent (and that I don't know).

[michaelklishin]

AFAICT it simply sets the default.

[michaelklishin]

2.9.2 is up on rubygems.org.