diff --git a/CHANGELOG.md b/CHANGELOG.md index 954aa1ae..51fa2d13 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- A TLS peer that closes the connection without sending a `close_notify` alert + no longer raises a bare `OpenSSL::SSL::SSLError` ("SSL_read: unexpected eof + while reading") out of a read. OpenSSL 3 reports that close as an error + rather than as the end of the stream, so TLS connections bypassed the + premature-EOF check that plain connections get. This sort of close is now reported + as EOF as it does for a plain socket: a body delimited by connection close finishes + normally, while a `Content-Length` or chunked body that ends early raises + `HTTP::ConnectionError`. - Building a default `Host` header now raises `HTTP::RequestError` when the request URI has a nil host (previously `NoMethodError`) or an empty host (e.g. `https:///path` or `https://:123/path`, which previously produced diff --git a/lib/http/timeout/global.rb b/lib/http/timeout/global.rb index d38b8790..59b05c29 100644 --- a/lib/http/timeout/global.rb +++ b/lib/http/timeout/global.rb @@ -111,14 +111,6 @@ def write(data) private - # Reads from socket in non-blocking mode - # - # @api private - # @return [String, Symbol] - def read_nonblock(size, buffer = nil) - @socket.read_nonblock(size, buffer, exception: false) - end - # Writes to socket in non-blocking mode # # @api private diff --git a/lib/http/timeout/null.rb b/lib/http/timeout/null.rb index 524a04a5..618ade1b 100644 --- a/lib/http/timeout/null.rb +++ b/lib/http/timeout/null.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require "io/wait" +require "openssl" require "timeout" module HTTP @@ -14,6 +15,11 @@ class Null # @api private NATIVE_CONNECT_TIMEOUT = RUBY_VERSION >= "3.4" + # OpenSSL's message for a peer that closed without a close_notify alert + # + # @api private + UNEXPECTED_EOF = "unexpected eof while reading" + # Timeout configuration options # # @example @@ -132,6 +138,10 @@ def start_tls(host, ssl_socket_class, ssl_context) def readpartial(size, buffer = nil) @socket.readpartial(size, buffer) rescue EOFError + :eof + rescue OpenSSL::SSL::SSLError => e + raise unless unexpected_eof?(e) + :eof end @@ -150,6 +160,29 @@ def write(data) private + # Reads from socket in non-blocking mode + # + # @param [Integer] size + # @param [String, nil] buffer + # @api private + # @return [String, Symbol, nil] + def read_nonblock(size, buffer = nil) + @socket.read_nonblock(size, buffer, exception: false) + rescue OpenSSL::SSL::SSLError => e + raise unless unexpected_eof?(e) + + nil + end + + # Whether an SSL error is a peer closing without a close_notify alert + # + # @param [OpenSSL::SSL::SSLError] error + # @api private + # @return [Boolean] + def unexpected_eof?(error) + error.message.include?(UNEXPECTED_EOF) + end + # Retries reading on wait readable # # @api private diff --git a/lib/http/timeout/per_operation.rb b/lib/http/timeout/per_operation.rb index 5a5d3855..6d7737c6 100644 --- a/lib/http/timeout/per_operation.rb +++ b/lib/http/timeout/per_operation.rb @@ -136,7 +136,7 @@ def connect_ssl def readpartial(size, buffer = nil) timeout = false loop do - result = @socket.read_nonblock(size, buffer, exception: false) + result = read_nonblock(size, buffer) return :eof if result.nil? return result unless WAIT_RESULTS.include?(result) diff --git a/sig/http.rbs b/sig/http.rbs index 9b1c4a69..0d11af9b 100644 --- a/sig/http.rbs +++ b/sig/http.rbs @@ -1498,6 +1498,7 @@ module HTTP module Timeout class Null NATIVE_CONNECT_TIMEOUT: bool + UNEXPECTED_EOF: String attr_reader options: Hash[Symbol, Numeric] attr_reader socket: untyped @@ -1516,6 +1517,8 @@ module HTTP def read_timeout: () -> Numeric? def write_timeout: () -> Numeric? + def read_nonblock: (Integer size, ?String? buffer) -> untyped + def unexpected_eof?: (OpenSSL::SSL::SSLError error) -> bool def rescue_readable: (?Numeric? timeout) { () -> untyped } -> untyped def rescue_writable: (?Numeric? timeout) { () -> untyped } -> untyped def open_socket: (untyped socket_class, String host, Integer port, ?connect_timeout: Numeric?) -> untyped @@ -1566,7 +1569,6 @@ module HTTP private - def read_nonblock: (Integer size, ?String? buffer) -> untyped def write_nonblock: (String data) -> untyped def perform_io: (?Numeric? per_op_timeout) { () -> untyped } -> untyped def handle_io_result: (untyped result) -> untyped diff --git a/test/http/connection_test.rb b/test/http/connection_test.rb index 8aae7f61..1c886a5f 100644 --- a/test/http/connection_test.rb +++ b/test/http/connection_test.rb @@ -403,6 +403,46 @@ def test_readpartial_finishes_cleanly_when_not_framed assert_equal "", chunk2 end + # Delivers `response`, then closes the way OpenSSL reports a peer + # disconnecting without a close_notify alert. Going through a real timeout + # handler is the point: it is what turns that SSLError into the :eof + # #check_premature_eof judges, so TLS reaches the same verdict a plain socket + # would for the same response framing. + def build_tls_eof_connection(response) + reads = [response] + raw_socket = fake( + close: nil, + closed?: false, + readpartial: proc { |*| + raise OpenSSL::SSL::SSLError, "SSL_read: unexpected eof while reading" if reads.empty? + + reads.shift + } + ) + socket = HTTP::Timeout::Null.new + socket.instance_variable_set(:@socket, raw_socket) + socket.define_singleton_method(:connect) { |*, **| nil } + + conn = HTTP::Connection.new(build_req, HTTP::Options.new(timeout_class: fake(new: socket))) + conn.instance_variable_set(:@pending_response, true) + conn.tap(&:read_headers!) + end + + def test_readpartial_treats_tls_unexpected_eof_as_the_end_of_an_unframed_body + conn = build_tls_eof_connection("HTTP/1.1 200 OK\r\n\r\nhello") + + assert_equal "hello", conn.readpartial + assert_equal "", conn.readpartial + end + + def test_readpartial_detects_premature_eof_when_tls_closes_mid_framed_body + conn = build_tls_eof_connection("HTTP/1.1 200 OK\r\nContent-Length: 100\r\n\r\nhello") + + assert_equal "hello", conn.readpartial + err = assert_raises(HTTP::ConnectionError) { conn.readpartial } + assert_includes err.message, "response body ended prematurely" + end + def test_readpartial_finishes_response_when_parser_says_finished req = build_req call_count = 0 diff --git a/test/http/timeout/null_test.rb b/test/http/timeout/null_test.rb index 8126949a..3b513c60 100644 --- a/test/http/timeout/null_test.rb +++ b/test/http/timeout/null_test.rb @@ -86,6 +86,47 @@ def test_start_tls_skips_post_connection_check_when_verify_hostname_false refute post_connection_check_called end + # -- unexpected eof -- + # + # OpenSSL 3 raises instead of signalling EOF when a peer closes the connection + # without a close_notify alert. Both read paths report it the way the socket + # itself reports an ordinary close, leaving the connection layer to decide + # whether that EOF was premature. Subclasses inherit #read_nonblock, so this + # covers their reads too. + + def stub_socket(method, message) + socket = fake( + to_io: @io, + closed?: false, + method => proc { |*| raise OpenSSL::SSL::SSLError, message } + ) + @timeout.instance_variable_set(:@socket, socket) + end + + def test_readpartial_returns_eof_when_tls_peer_closes_without_close_notify + stub_socket(:readpartial, "SSL_read: unexpected eof while reading") + + assert_equal :eof, @timeout.readpartial(10) + end + + def test_readpartial_reraises_ssl_errors_that_are_not_an_unexpected_eof + stub_socket(:readpartial, "SSL_read: decryption failed or bad record mac") + err = assert_raises(OpenSSL::SSL::SSLError) { @timeout.readpartial(10) } + assert_match(/decryption failed/, err.message) + end + + def test_read_nonblock_returns_nil_when_tls_peer_closes_without_close_notify + stub_socket(:read_nonblock, "SSL_read: unexpected eof while reading") + + assert_nil @timeout.send(:read_nonblock, 10) + end + + def test_read_nonblock_reraises_ssl_errors_that_are_not_an_unexpected_eof + stub_socket(:read_nonblock, "SSL_read: decryption failed or bad record mac") + err = assert_raises(OpenSSL::SSL::SSLError) { @timeout.send(:read_nonblock, 10) } + assert_match(/decryption failed/, err.message) + end + # -- #rescue_readable (private) -- def test_rescue_readable_yields_the_block