diff --git a/History.md b/History.md index e9731d8289..89af1ab2db 100644 --- a/History.md +++ b/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 diff --git a/lib/puma/events.rb b/lib/puma/events.rb index 36af4a6625..92679f428e 100644 --- a/lib/puma/events.rb +++ b/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 + # @param ssl_socket # - def ssl_error(error, peeraddr, peercert) + def ssl_error(error, ssl_socket) + peeraddr = ssl_socket.peeraddr.last rescue "" + peercert = ssl_socket.peercert subject = peercert ? peercert.subject : nil @error_logger.info(error: error, text: "SSL error, peer: #{peeraddr}, peer cert: #{subject}") end diff --git a/lib/puma/reactor.rb b/lib/puma/reactor.rb index 8fcc2fa267..2941bea0ec 100644 --- a/lib/puma/reactor.rb +++ b/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 = "" - 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) diff --git a/lib/puma/server.rb b/lib/puma/server.rb index 37a552f35a..e5d0af63e2 100644 --- a/lib/puma/server.rb +++ b/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) diff --git a/test/test_events.rb b/test/test_events.rb index bdc61eb4c2..d0da584cfb 100644 --- a/test/test_events.rb +++ b/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: " + assert_includes error, "cert: :" + + end if ::Puma::HAS_SSL end diff --git a/test/test_puma_server_ssl.rb b/test/test_puma_server_ssl.rb index c31368c54b..1a5cf2e608 100644 --- a/test/test_puma_server_ssl.rb +++ b/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 "" + self.cert = ssl_socket.peercert end end