Change Events#ssl_error signature from (error, peeraddr, peercert) to…

… (error, ssl_socket) (#2375)

* Change Events#ssl_error signature from (error, peeraddr, peercert) to (error, ssl_socket)

The method signature should include the socket, so it determines what socket info is logged

Co-authored-by: ocowchun <ocowchun@gmail.com>

* test_events.rb - add test_ssl_error

Co-authored-by: ocowchun <ocowchun@gmail.com>
Co-authored-by: Nate Berkopec <nate.berkopec@gmail.com>

History.md

@@ -7,6 +7,7 @@
   * Your bugfix goes here (#Github Number)
 
 * Refactor
+  * Change Events#ssl_error signature from (error, peeraddr, peercert) to (error, ssl_socket) (#2375)
   * Remove unneeded attr_accessor statements from Server (#2373)
 
 ## 5.0.0

lib/puma/events.rb

@@ -106,10 +106,12 @@ def parse_error(error, req)
     end
 
     # An SSL error has occurred.
-    # +error+ an exception object, +peeraddr+ peer address,
-    # and +peercert+ any peer certificate (if present).
+    # @param error <Puma::MiniSSL::SSLError>
+    # @param ssl_socket <Puma::MiniSSL::Socket>
     #
-    def ssl_error(error, peeraddr, peercert)
+    def ssl_error(error, ssl_socket)
+      peeraddr = ssl_socket.peeraddr.last rescue "<unknown>"
+      peercert = ssl_socket.peercert
       subject = peercert ? peercert.subject : nil
       @error_logger.info(error: error, text: "SSL error, peer: #{peeraddr}, peer cert: #{subject}")
     end

lib/puma/reactor.rb

@@ -237,23 +237,12 @@ def run_internal
 
               # SSL handshake failure
               rescue MiniSSL::SSLError => e
-                @server.lowlevel_error(e, c.env)
-
-                ssl_socket = c.io
-                begin
-                  addr = ssl_socket.peeraddr.last
-                # EINVAL can happen when browser closes socket w/security exception
-                rescue IOError, Errno::EINVAL
-                  addr = "<unknown>"
-                end
-
-                cert = ssl_socket.peercert
+                @server.lowlevel_error e, c.env
+                @events.ssl_error e, c.io
 
                 c.close
                 clear_monitor mon
 
-                @events.ssl_error e, addr, cert
-
               # The client doesn't know HTTP well
               rescue HttpParserError => e
                 @server.lowlevel_error(e, c.env)

lib/puma/server.rb

@@ -234,13 +234,9 @@ def run(background=true)
             process_now = true
           end
         rescue MiniSSL::SSLError => e
-          ssl_socket = client.io
-          addr = ssl_socket.peeraddr.last
-          cert = ssl_socket.peercert
-
+          @events.ssl_error e, client.io
           client.close
 
-          @events.ssl_error e, addr, cert
         rescue HttpParserError => e
           client.write_error(400)
           client.close
@@ -437,16 +433,10 @@ def process_client(client, buffer)
 
       # SSL handshake error
       rescue MiniSSL::SSLError => e
-        lowlevel_error(e, client.env)
-
-        ssl_socket = client.io
-        addr = ssl_socket.peeraddr.last
-        cert = ssl_socket.peercert
-
+        lowlevel_error e, client.env
+        @events.ssl_error e, client.io
         close_socket = true
 
-        @events.ssl_error e, addr, cert
-
       # The client doesn't know HTTP well
       rescue HttpParserError => e
         lowlevel_error(e, client.env)

test/test_events.rb

@@ -179,6 +179,43 @@ def test_parse_error
     sleep 0.1 # important so that the previous data is sent as a packet
     assert_match %r!HTTP parse error, malformed request!, events.stderr.string
     assert_match %r!\("GET #{path}" - \(-\)\)!, events.stderr.string
-    server.stop(true)
+  ensure
+    sock.close if sock && !sock.closed?
+    server.stop true
   end
+
+  # test_puma_server_ssl.rb checks that ssl errors are raised correctly,
+  # but it mocks the actual error code.  This test the code, but it will
+  # break if the logged message changes
+  def test_ssl_error
+    events = Puma::Events.strings
+
+    ssl_mock = -> (addr, subj) {
+      obj = Object.new
+      obj.define_singleton_method(:peeraddr) { addr }
+      if subj
+        cert = Object.new
+        cert.define_singleton_method(:subject) { subj }
+        obj.define_singleton_method(:peercert) { cert }
+      else
+        obj.define_singleton_method(:peercert) { nil }
+      end
+      obj
+    }
+
+    events.ssl_error OpenSSL::SSL::SSLError, ssl_mock.call(['127.0.0.1'], 'test_cert')
+    error = events.stderr.string
+    assert_includes error, "SSL error"
+    assert_includes error, "peer: 127.0.0.1"
+    assert_includes error, "cert: test_cert"
+
+    events.stderr.string = ''
+
+    events.ssl_error OpenSSL::SSL::SSLError, ssl_mock.call(nil, nil)
+    error = events.stderr.string
+    assert_includes error, "SSL error"
+    assert_includes error, "peer: <unknown>"
+    assert_includes error, "cert: :"
+
+  end if ::Puma::HAS_SSL
 end

test/test_puma_server_ssl.rb

@@ -12,10 +12,10 @@
   class SSLEventsHelper < ::Puma::Events
     attr_accessor :addr, :cert, :error
 
-    def ssl_error(error, peeraddr, peercert)
+    def ssl_error(error, ssl_socket)
       self.error = error
-      self.addr = peeraddr
-      self.cert = peercert
+      self.addr = ssl_socket.peeraddr.last rescue "<unknown>"
+      self.cert = ssl_socket.peercert
     end
   end