Don't prepend OpenSSL::SSL::SSLSocket #3

girasquid · 2014-10-29T19:21:12Z

@nwjsmith @peterjm @fw42 for review please

This is my test to confirm that this works:

#!/usr/bin/env ruby

require 'bundler/setup'
require 'byebug'
require 'test/unit'
require 'tcr'


TCR.configure do |c|
  c.cassette_library_dir = '.'
  c.hook_tcp_ports = '*'
end
class TCRTCPSocketTest < Test::Unit::TestCase
  def test_google_dot_com
    TCR.use_cassette('google_ssl') do
      a_socket = connect_to('google.com', 443)
      a_socket.write('gibberish')
      a_socket.close
    end
  end

  def connect_to(host, port)
    sock = TCPSocket.new(host, port)
    ssl = OpenSSL::SSL::SSLSocket.new(sock, OpenSSL::SSL::SSLContext.new)
    ssl.sync = true
    ssl.connect
    ssl.setsockopt Socket::IPPROTO_TCP, Socket::TCP_NODELAY, 1
    ssl
  end

end

Running the test without these changes fails like this:

ruby test.rb
Run options: 

# Running tests:

[1/1] TCRTCPSocketTest#test_google_dot_com = 0.08 s
  1) Error:
TCRTCPSocketTest#test_google_dot_com:
ArgumentError: wrong number of arguments (0 for 2+)
    /home/vagrant/src/scratch/tcr/lib/tcr/socket_extension.rb:3:in `initialize'
    /home/vagrant/src/scratch/tcr/lib/tcr/socket_extension.rb:4:in `initialize'
    /home/vagrant/src/scratch/tcr/lib/tcr/socket_extension.rb:4:in `initialize'
    test.rb:24:in `new'
    test.rb:24:in `connect_to'
    test.rb:16:in `block in test_google_dot_com'
    /home/vagrant/src/scratch/tcr/lib/tcr.rb:48:in `use_cassette'
    test.rb:15:in `test_google_dot_com'
    /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/minitest/unit.rb:1265:in `run'
    /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/test/unit/testcase.rb:17:in `run'
    /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/minitest/unit.rb:940:in `block in _run_suite'
    /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/minitest/unit.rb:933:in `map'
    /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/minitest/unit.rb:933:in `_run_suite'
    /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/test/unit.rb:663:in `block in _run_suites'
    /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/test/unit.rb:661:in `each'
    /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/test/unit.rb:661:in `_run_suites'
    /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/minitest/unit.rb:884:in `_run_anything'
    /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/minitest/unit.rb:1092:in `run_tests'
    /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/minitest/unit.rb:1079:in `block in _run'
    /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/minitest/unit.rb:1078:in `each'
    /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/minitest/unit.rb:1078:in `_run'
    /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/minitest/unit.rb:1066:in `run'
    /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/test/unit.rb:27:in `run'
    /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/test/unit.rb:780:in `run'
    /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/test/unit.rb:372:in `block (2 levels) in autorun'
    /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/test/unit.rb:33:in `run_once'
    /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/test/unit.rb:371:in `block in autorun'

Finished tests in 0.096612s, 10.3507 tests/s, 0.0000 assertions/s.
1 tests, 0 assertions, 0 failures, 1 errors, 0 skips

ruby -v: ruby 2.1.3p242-shopify (development) [x86_64-linux]

Appears related to something about C extensions.

peterjm · 2014-10-29T20:23:48Z

lib/tcr/ssl_socket_extension.rb

@@ -0,0 +1,15 @@
+require 'openssl'
+
+module OpenSSL::SSL


Should this be a module that gets included in OpenSSL::SSL::SSLSocket, instead of modifying the file directly? Should be functionally the same, but it's more symmetrical to the way that TCPSocket is being done.

+1 for this

Here's what I tried:

module TCR module SSLSocketExtension def initialize_with_tcr(s, context) initialize_without_tcr(s, context) if TCR.record_port?(s.remote_address.ip_port) && TCR.cassette extend(TCR::Recordable) self.cassette = TCR.cassette end end alias_method :initialize_without_tcr, :initialize alias_method :initialize, :initialize_with_tcr end end

And then in tcr.rb:
OpenSSL::SSL::SSLSocket.send(:include, TCR::SSLSocketExtension)

Unfortunately, that fails the same way as the original:

~/src/scratch: ruby test.rb Run options: # Running tests: [1/1] TCRTCPSocketTest#test_google_dot_com = 0.08 s 1) Error: TCRTCPSocketTest#test_google_dot_com: ArgumentError: wrong number of arguments (0 for 2) /home/vagrant/src/scratch/tcr/lib/tcr/ssl_socket_extension.rb:4:in `initialize_with_tcr' test.rb:24:in `initialize' test.rb:24:in `new' test.rb:24:in `connect_to' test.rb:16:in `block in test_google_dot_com' /home/vagrant/src/scratch/tcr/lib/tcr.rb:48:in `use_cassette' test.rb:15:in `test_google_dot_com' /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/minitest/unit.rb:1265:in `run' /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/test/unit/testcase.rb:17:in `run' /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/minitest/unit.rb:940:in `block in _run_suite' /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/minitest/unit.rb:933:in `map' /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/minitest/unit.rb:933:in `_run_suite' /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/test/unit.rb:663:in `block in _run_suites' /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/test/unit.rb:661:in `each' /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/test/unit.rb:661:in `_run_suites' /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/minitest/unit.rb:884:in `_run_anything' /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/minitest/unit.rb:1092:in `run_tests' /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/minitest/unit.rb:1079:in `block in _run' /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/minitest/unit.rb:1078:in `each' /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/minitest/unit.rb:1078:in `_run' /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/minitest/unit.rb:1066:in `run' /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/test/unit.rb:27:in `run' /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/test/unit.rb:780:in `run' /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/test/unit.rb:372:in `block (2 levels) in autorun' /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/test/unit.rb:33:in `run_once' /usr/lib/shopify-ruby/2.1.3-shopify1/lib/ruby/2.1.0/test/unit.rb:371:in `block in autorun' Finished tests in 0.097923s, 10.2121 tests/s, 0.0000 assertions/s. 1 tests, 0 assertions, 0 failures, 1 errors, 0 skips ruby -v: ruby 2.1.3p242-shopify (development) [x86_64-linux]

I think that including might have the same problem as prepending.

lol wtf ruby

With some help from @peterjm, got this updated to be a module that can be included on OpenSSL::SSL::SSLSocket (you need to use self.included). This has been updated and works now.

peterjm · 2014-10-29T20:24:15Z

The approach looks good to me. Thanks for all your investigation on this!

nwjsmith · 2014-10-29T20:25:30Z

Other than Peter's suggestion, this looks good!

fw42 · 2014-10-29T23:41:41Z

lib/tcr/ssl_socket_extension.rb

+
+module OpenSSL::SSL
+  class SSLSocket
+    alias_method :original_initialize, :initialize


why not do this in a more alias_method_chain kinda way? seems easier to follow for most people, i.e.

alias_method :initialize_without_tcr, :initialize alias_method :initialize, :initialize_with_tcr def initalize_with_tcr # things ... initialize_without_tcr end

Thanks, changed

fw42 · 2014-10-30T13:19:20Z

🚢 🇮🇹

…cket

peterjm · 2014-10-30T13:49:03Z

🚢

Don't prepend OpenSSL::SSL::SSLSocket

peterjm reviewed Oct 29, 2014
View reviewed changes

fw42 reviewed Oct 29, 2014
View reviewed changes

girasquid force-pushed the dont_prepend_ssl branch from a9c9d2b to c8a5fdc Compare October 30, 2014 13:15

Extract separate extension for SSL; don't prepend to OpenSSL::SSL::So…

c7c5e8d

…cket

girasquid force-pushed the dont_prepend_ssl branch from c8a5fdc to c7c5e8d Compare October 30, 2014 13:46

girasquid added a commit that referenced this pull request Oct 30, 2014

Merge pull request #3 from Shopify/dont_prepend_ssl

7b3314e

Don't prepend OpenSSL::SSL::SSLSocket

girasquid merged commit 7b3314e into master Oct 30, 2014

girasquid deleted the dont_prepend_ssl branch October 30, 2014 13:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't prepend OpenSSL::SSL::SSLSocket #3

Don't prepend OpenSSL::SSL::SSLSocket #3

girasquid commented Oct 29, 2014

peterjm Oct 29, 2014

nwjsmith Oct 29, 2014

girasquid Oct 30, 2014

fw42 Oct 30, 2014

girasquid Oct 30, 2014

peterjm commented Oct 29, 2014

nwjsmith commented Oct 29, 2014

fw42 Oct 29, 2014

girasquid Oct 30, 2014

fw42 commented Oct 30, 2014

peterjm commented Oct 30, 2014

Don't prepend OpenSSL::SSL::SSLSocket #3

Don't prepend OpenSSL::SSL::SSLSocket #3

Conversation

girasquid commented Oct 29, 2014

peterjm Oct 29, 2014

Choose a reason for hiding this comment

nwjsmith Oct 29, 2014

Choose a reason for hiding this comment

girasquid Oct 30, 2014

Choose a reason for hiding this comment

fw42 Oct 30, 2014

Choose a reason for hiding this comment

girasquid Oct 30, 2014

Choose a reason for hiding this comment

peterjm commented Oct 29, 2014

nwjsmith commented Oct 29, 2014

fw42 Oct 29, 2014

Choose a reason for hiding this comment

girasquid Oct 30, 2014

Choose a reason for hiding this comment

fw42 commented Oct 30, 2014

peterjm commented Oct 30, 2014