Pure Ruby EM SSL (working start_tls) #712

portertech · 2016-04-16T08:22:16Z

This pull request adds SSL/TLS support to the pure Ruby EM implementation.

@amdprophet & I went into the belly of the beast that is em/pure_ruby. The pure Ruby implementation leaves much to be desired, but it's a start!

$ rake test_em_pure_ruby                                         
EM Library Type: pure_ruby
Loaded suite /home/portertech/.gem/ruby/2.3.0/gems/rake-11.1.2/lib/rake/rake_test_loader
Started
.........
======================================================================================================================================
omitted. [test_no_dhparam(TestSslDhParam)]
/home/portertech/projects/sensu/eventmachine/tests/test_ssl_dhparam.rb:49:in `test_no_dhparam'
======================================================================================================================================
.............
======================================================================================================================================
omitted. [test_accept_server(TestSslVerify)]
/home/portertech/projects/sensu/eventmachine/tests/test_ssl_verify.rb:101:in `test_accept_server'
======================================================================================================================================
======================================================================================================================================
omitted. [test_deny_server(TestSslVerify)]
/home/portertech/projects/sensu/eventmachine/tests/test_ssl_verify.rb:116:in `test_deny_server'
======================================================================================================================================
.

Finished in 1.25792021 seconds.
--------------------------------------------------------------------------------------------------------------------------------------
26 tests, 57 assertions, 0 failures, 0 errors, 0 pendings, 3 omissions, 0 notifications
100% passed
--------------------------------------------------------------------------------------------------------------------------------------
20.67 tests/s, 45.31 assertions/s

sodabrew · 2016-04-16T15:04:26Z

lib/em/pure_ruby.rb

+      ctx.cert = OpenSSL::X509::Certificate.new(tls_parms[:cert_chain]) if tls_parms[:cert_chain]
+      ctx.key = OpenSSL::PKey::RSA.new(tls_parms[:priv_key]) if tls_parms[:priv_key]
+      verify_mode = OpenSSL::SSL::VERIFY_NONE
+      if tls_parms[:verify_peer] || tls_parms[:verify_peer] == 1


Is this redundant because 1 is truthy?

I'll happily remove the extra conditional, wasn't sure if all consuming libraries were following the rules and using a boolean.

portertech · 2016-04-16T19:56:29Z

Looks like we do not want to set context ssl_version, but instead use context options or ciphers to limit (like the C++ reactor).

sodabrew · 2016-04-18T16:40:24Z

lib/em/pure_ruby.rb

+        EM_PROTO_TLSv1_2 => OpenSSL::SSL::OP_NO_TLSv1_2
+      }.inject(0) do |options, (bits, option)|
+        options |= option if (bits & protocols_bitmask) == 0
+        options


Yes, this better matches the C++ by looping over the entire array. Note that ssl_version might also be a non-array (but I think I coerce it to an array in lib/em/connection.rb, so by the time it gets to the reactor it should always be an array? gotta double check).

This is something I implemented differently in EM 1.2 vs. how Ruby does it, is allowing an array of possible ssl_versions. I want to propose this upstream; the way Ruby does it now makes it fairly hard to write "Allow the following SSL/TLS protocol versions: TLS 1.0, 1.1, or 1.2"

portertech · 2016-04-18T21:17:10Z

@sodabrew I think I have taken this as far as I can for the time being. These changes allow Sensu to run on platforms that cannot successfully compile the c++ reactor 👍

portertech · 2016-04-18T21:17:36Z

@amdprophet it's happening!

portertech · 2016-04-18T21:20:06Z

I made sure to comment on the few SSL test omission lines.

amdprophet · 2016-04-18T21:33:09Z

sodabrew · 2016-04-18T23:49:58Z

I'll give this another look over and run the test suite myself in a few hours. Assuming I don't find anything else at issue, are you still working on this, or ready for merge?

portertech · 2016-04-19T01:20:56Z

@sodabrew ready for merge 👍

sodabrew · 2016-04-19T05:12:26Z

The stock OS X Ruby 2.0 may have an older OpenSSL under the hood?

  NameError: uninitialized constant OpenSSL::SSL::SSLErrorWaitReadable

and I found myself replacing this nice Hash inject with a less elegant pile of ifs, because I don't have the TLSv1_1, TLSv1_2 macros available:

-      ssl_options = {
-        EM_PROTO_SSLv2 => OpenSSL::SSL::OP_NO_SSLv2,
-        EM_PROTO_SSLv3 => OpenSSL::SSL::OP_NO_SSLv3,
-        EM_PROTO_TLSv1 => OpenSSL::SSL::OP_NO_TLSv1,
-        EM_PROTO_TLSv1_1 => OpenSSL::SSL::OP_NO_TLSv1_1,
-        EM_PROTO_TLSv1_2 => OpenSSL::SSL::OP_NO_TLSv1_2
-      }.inject(OpenSSL::SSL::OP_ALL) do |options, (bits, option)|
-        options |= option if (bits & protocols_bitmask) == 0
-        options
-      end
+      options = OpenSSL::SSL::OP_ALL
+      # The basic SSL / TLS versions are widely defined across OpenSSL versions
+      options |= OpenSSL::SSL::OP_NO_SSLv2 if defined?(OpenSSL::SSL::OP_NO_SSLv2) && EM_PROTO_SSLv2 & protocols_bitmask == 0
+      options |= OpenSSL::SSL::OP_NO_SSLv3 if defined?(OpenSSL::SSL::OP_NO_SSLv3) && EM_PROTO_SSLv3 & protocols_bitmask == 0
+      options |= OpenSSL::SSL::OP_NO_TLSv1 if defined?(OpenSSL::SSL::OP_NO_TLSv1) && EM_PROTO_TLSv1 & protocols_bitmask == 0
+      # But these TLS 1.x versions are not defined until later OpenSSL versions
+      options |= OpenSSL::SSL::OP_NO_TLSv1_1 if defined?(OpenSSL::SSL::OP_NO_TLSv1_1) && EM_PROTO_TLSv1_1 & protocols_bitmask == 0
+      options |= OpenSSL::SSL::OP_NO_TLSv1_2 if defined?(OpenSSL::SSL::OP_NO_TLSv1_2) && EM_PROTO_TLSv1_2 & protocols_bitmask == 0

portertech · 2016-04-19T14:56:11Z

@sodabrew would you like me to apply your patch to this pr?

sodabrew · 2016-04-19T15:43:54Z

Yes please do. I hadn't looked at the context for SSLErrorWaitReadable to see if that can be wrapped in an if defined? block, but I think that is important to avoid breaking older OpenSSL versions.

portertech · 2016-04-19T16:15:07Z

@sodabrew I'll make the necessary changes 👍 Thanks for catching these.

portertech · 2016-04-19T16:50:54Z

lib/em/pure_ruby.rb

+# SSLErrorWaitReadable and SSLErrorWaitWritable OpenSSL classes.
+# Set them if not defined.
+unless defined?(OpenSSL::SSL::SSLErrorWaitReadable)
+  OpenSSL::SSL::SSLErrorWaitReadable = OpenSSL::SSL::SSLError


@sodabrew WDYT?

I think this is OK? I wonder if it will work-alike.

The problem is that this will rescue instances of OpenSSL::SSL::SSLError in the blocks for the Read/Write errors. I think the way to roll is to flip the situation around, so below you'd rescue a new class EventMachine::Error::SSLErrorWaitWritable, and then define it as:

if defined?(OpenSSL::SSL::SSLErrorWaitWritable) EventMachine::Error::SSLErrorWaitWritable = OpenSSL::SSL::SSLErrorWaitWritable else class EventMachine::Error::SSLErrorWaitWritable < EventMachine::Error; end end

This is a better approach, I'll apply a similar patch 👍

Figuring out what openssl threw before wait writable etc.

portertech · 2016-04-20T19:59:31Z

@sodabrew ready for your review & testing once again 👍

portertech · 2016-04-20T20:01:06Z

portertech · 2016-04-22T18:46:15Z

@sodabrew please let me know if there are any other changes that I need to make. We would love to see this make it into upstream 👍

sodabrew · 2016-04-24T22:16:32Z

Would you mind squashing to fewer commits? I think just a few commits should suffice for the final form of this PR.

[pure_ruby_tls] initial implementation [pure_ruby_tls] closer to working start_tls [pure_ruby_tls] working start_tls implementation [pure_ruby_tls] fixed connection completed [pure_ruby_tls] debugging handshake errors when certs are used [pure_ruby_tls] fully operational battle station [pure_ruby_tls] implemented fail_if_no_peer_cert, cipher_list, and protocols_bitmask [pure_ruby_tls] support testing pure ruby reactor [pure_ruby_tls] ssl/tls tcp support support [pure_ruby_tls] working tls server [pure_ruby_tls] delete dup tests [pure_ruby_tls] fixed tcp client ready?() for aix and solaris [pure_ruby_tls] only use Socket::TCP_INFO on linux [pure_ruby_tls] working ssl_handshake_completed [pure_ruby_tls] default cert/key, error classes, working unbind, fix blocking eventable_write, and tests! [pure_ruby_tls] changed default cert subject to include EM, removed verify_peer == 1 [pure_ruby_tls] working ssl verify with peer cert [pure_ruby_tls] fixed ssl options, OP_ALL [pure_ruby_tls] working get peer cert and cipher [pure_ruby_tls] fixed ssl_verify_peer connection close, working sni_hostname [pure_ruby_tls] working dhparam [pure_ruby_tls] working ecdh curve on ruby 2.3 [pure_ruby_tls] omit verify peer tests that do not pass due to default server chain [pure_ruby_tls] support older versions of ruby and openssl [pure_ruby_tls] fixed ssl wait readable/writable error handling on older rubies [pure_ruby_tls] older rubies missing default dh keys [pure_ruby_tls] removed duplicate default cert creator

portertech · 2016-04-25T19:02:10Z

@sodabrew commits clean up 👍

sodabrew · 2016-04-25T19:54:18Z

Thank you!

sodabrew reviewed Apr 16, 2016
View reviewed changes

sodabrew reviewed Apr 18, 2016
View reviewed changes

portertech reviewed Apr 19, 2016
View reviewed changes

portertech force-pushed the feature/pure_ruby_tls branch from 891fbe7 to adafc7b Compare April 25, 2016 19:01

sodabrew merged commit c17c518 into eventmachine:master Apr 25, 2016

sodabrew added this to the v1.2.1 milestone Apr 25, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pure Ruby EM SSL (working start_tls) #712

Pure Ruby EM SSL (working start_tls) #712

portertech commented Apr 16, 2016 •

edited

Loading

sodabrew Apr 16, 2016

portertech Apr 16, 2016

portertech commented Apr 16, 2016

sodabrew Apr 18, 2016

portertech commented Apr 18, 2016

portertech commented Apr 18, 2016

portertech commented Apr 18, 2016

amdprophet commented Apr 18, 2016

sodabrew commented Apr 18, 2016

portertech commented Apr 19, 2016

sodabrew commented Apr 19, 2016

portertech commented Apr 19, 2016

sodabrew commented Apr 19, 2016

portertech commented Apr 19, 2016

portertech Apr 19, 2016

sodabrew Apr 20, 2016

sodabrew Apr 20, 2016

portertech Apr 20, 2016

portertech Apr 20, 2016

portertech commented Apr 20, 2016

portertech commented Apr 20, 2016

portertech commented Apr 22, 2016

sodabrew commented Apr 24, 2016

portertech commented Apr 25, 2016

sodabrew commented Apr 25, 2016

Pure Ruby EM SSL (working start_tls) #712

Pure Ruby EM SSL (working start_tls) #712

Conversation

portertech commented Apr 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

portertech commented Apr 16, 2016

Choose a reason for hiding this comment

portertech commented Apr 18, 2016

portertech commented Apr 18, 2016

portertech commented Apr 18, 2016

amdprophet commented Apr 18, 2016

sodabrew commented Apr 18, 2016

portertech commented Apr 19, 2016

sodabrew commented Apr 19, 2016

portertech commented Apr 19, 2016

sodabrew commented Apr 19, 2016

portertech commented Apr 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

portertech commented Apr 20, 2016

portertech commented Apr 20, 2016

portertech commented Apr 22, 2016

sodabrew commented Apr 24, 2016

portertech commented Apr 25, 2016

sodabrew commented Apr 25, 2016

portertech commented Apr 16, 2016 •

edited

Loading