From 43e68ce5cc5b0d27b5a81d332dd848a637025c4f Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Tue, 14 May 2024 10:44:59 -0300 Subject: [PATCH 1/2] Remove requires_connection test helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The aim of this helper is to cause some tests to be skipped if there is no Internet connection. For some reason (which I haven’t investigated) when run on GitHub Actions these tests are always being skipped, causing me to not notice the test failures fixed in 139a59b. I am happy to remove this check; presumably if there’s no Internet connection then the affected tests will just fail, and that’s fine by me. I understand the intent of the author in adding this helper, and understand that being able to run some of the test suite offline may be a desirable thing, but I think not at the cost of it hiding failing tests by default. (There’s also a requires_port helper which is only used in the proxy-related tests; in order to remove that one we’d need to start running proxies locally and in CI, and I don’t want to spend time on that now. We don’t make use of this library’s proxy functionality in ably-ruby.) --- spec/external_spec.rb | 225 ++++++++++++++++++------------------ spec/helper.rb | 4 - spec/pipelining_spec.rb | 90 +++++++-------- spec/socksify_proxy_spec.rb | 82 +++++++------ spec/ssl_spec.rb | 96 ++++++++------- 5 files changed, 239 insertions(+), 258 deletions(-) diff --git a/spec/external_spec.rb b/spec/external_spec.rb index b40b74e7..cbadfd40 100644 --- a/spec/external_spec.rb +++ b/spec/external_spec.rb @@ -1,149 +1,146 @@ require 'helper' -requires_connection do +describe EventMachine::AblyHttpRequest::HttpRequest do + + it "should follow redirects on HEAD method (external)" do + EventMachine.run { + http = EventMachine::AblyHttpRequest::HttpRequest.new('http://www.google.com/').head :redirects => 1 + http.errback { failed(http) } + http.callback { + http.response_header.status.should == 200 + EM.stop + } + } + end - describe EventMachine::AblyHttpRequest::HttpRequest do + it "should follow redirect to https and initiate the handshake" do + EventMachine.run { + http = EventMachine::AblyHttpRequest::HttpRequest.new('http://github.com/').get :redirects => 5 - it "should follow redirects on HEAD method (external)" do - EventMachine.run { - http = EventMachine::AblyHttpRequest::HttpRequest.new('http://www.google.com/').head :redirects => 1 - http.errback { failed(http) } - http.callback { - http.response_header.status.should == 200 - EM.stop - } + http.errback { failed(http) } + http.callback { + http.response_header.status.should == 200 + EventMachine.stop } - end + } + end - it "should follow redirect to https and initiate the handshake" do - EventMachine.run { - http = EventMachine::AblyHttpRequest::HttpRequest.new('http://github.com/').get :redirects => 5 + it "should perform a streaming GET" do + EventMachine.run { - http.errback { failed(http) } - http.callback { - http.response_header.status.should == 200 - EventMachine.stop - } + # digg.com uses chunked encoding + http = EventMachine::AblyHttpRequest::HttpRequest.new('http://www.httpwatch.com/httpgallery/chunked/').get + + http.errback { failed(http) } + http.callback { + http.response_header.status.should == 200 + EventMachine.stop } - end + } + end - it "should perform a streaming GET" do - EventMachine.run { + it "should handle a 100 continue" do + EventMachine.run { + # 8.2.3 Use of the 100 (Continue) Status - http://www.ietf.org/rfc/rfc2616.txt + # + # An origin server SHOULD NOT send a 100 (Continue) response if + # the request message does not include an Expect request-header + # field with the "100-continue" expectation, and MUST NOT send a + # 100 (Continue) response if such a request comes from an HTTP/1.0 + # (or earlier) client. There is an exception to this rule: for + # compatibility with RFC 2068, a server MAY send a 100 (Continue) + # status in response to an HTTP/1.1 PUT or POST request that does + # not include an Expect request-header field with the "100- + # continue" expectation. This exception, the purpose of which is + # to minimize any client processing delays associated with an + # undeclared wait for 100 (Continue) status, applies only to + # HTTP/1.1 requests, and not to requests with any other HTTP- + # version value. + # + # 10.1.1: 100 Continue - http://www.ietf.org/rfc/rfc2068.txt + # The client may continue with its request. This interim response is + # used to inform the client that the initial part of the request has + # been received and has not yet been rejected by the server. The client + # SHOULD continue by sending the remainder of the request or, if the + # request has already been completed, ignore this response. The server + # MUST send a final response after the request has been completed. + + url = 'http://ws.serviceobjects.com/lv/LeadValidation.asmx/ValidateLead_V2' + http = EventMachine::AblyHttpRequest::HttpRequest.new(url).post :body => {:name => :test} + + http.errback { failed(http) } + http.callback { + http.response_header.status.should == 500 + http.response.should match('Missing') + EventMachine.stop + } + } + end - # digg.com uses chunked encoding - http = EventMachine::AblyHttpRequest::HttpRequest.new('http://www.httpwatch.com/httpgallery/chunked/').get + it "should detect deflate encoding" do + EventMachine.run { - http.errback { failed(http) } - http.callback { - http.response_header.status.should == 200 - EventMachine.stop - } + options = {:head => {"accept-encoding" => "deflate"}, :redirects => 5} + http = EventMachine::AblyHttpRequest::HttpRequest.new('https://www.bing.com/').get options + + http.errback { failed(http) } + http.callback { + http.response_header.status.should == 200 + http.response_header["CONTENT_ENCODING"].should == "deflate" + + EventMachine.stop } - end + } + end - it "should handle a 100 continue" do - EventMachine.run { - # 8.2.3 Use of the 100 (Continue) Status - http://www.ietf.org/rfc/rfc2616.txt - # - # An origin server SHOULD NOT send a 100 (Continue) response if - # the request message does not include an Expect request-header - # field with the "100-continue" expectation, and MUST NOT send a - # 100 (Continue) response if such a request comes from an HTTP/1.0 - # (or earlier) client. There is an exception to this rule: for - # compatibility with RFC 2068, a server MAY send a 100 (Continue) - # status in response to an HTTP/1.1 PUT or POST request that does - # not include an Expect request-header field with the "100- - # continue" expectation. This exception, the purpose of which is - # to minimize any client processing delays associated with an - # undeclared wait for 100 (Continue) status, applies only to - # HTTP/1.1 requests, and not to requests with any other HTTP- - # version value. - # - # 10.1.1: 100 Continue - http://www.ietf.org/rfc/rfc2068.txt - # The client may continue with its request. This interim response is - # used to inform the client that the initial part of the request has - # been received and has not yet been rejected by the server. The client - # SHOULD continue by sending the remainder of the request or, if the - # request has already been completed, ignore this response. The server - # MUST send a final response after the request has been completed. - - url = 'http://ws.serviceobjects.com/lv/LeadValidation.asmx/ValidateLead_V2' - http = EventMachine::AblyHttpRequest::HttpRequest.new(url).post :body => {:name => :test} + it "should stream chunked gzipped data" do + EventMachine.run { + options = {:head => {"accept-encoding" => "gzip"}} + # GitHub sends chunked gzip, time for a little Inception ;) + http = EventMachine::AblyHttpRequest::HttpRequest.new('https://github.com/igrigorik/em-http-request/commits/master').get options - http.errback { failed(http) } - http.callback { - http.response_header.status.should == 500 - http.response.should match('Missing') - EventMachine.stop - } + http.errback { failed(http) } + http.callback { + http.response_header.status.should == 200 + http.response_header["CONTENT_ENCODING"].should == "gzip" + http.response.should == '' + + EventMachine.stop } - end - it "should detect deflate encoding" do - EventMachine.run { + body = '' + http.stream do |chunk| + body << chunk + end + } + end - options = {:head => {"accept-encoding" => "deflate"}, :redirects => 5} - http = EventMachine::AblyHttpRequest::HttpRequest.new('https://www.bing.com/').get options + context "keepalive" do + it "should default to non-keepalive" do + EventMachine.run { + headers = {'If-Modified-Since' => 'Thu, 05 Aug 2010 22:54:44 GMT'} + http = EventMachine::AblyHttpRequest::HttpRequest.new('http://www.google.com/images/logos/ps_logo2.png').get :head => headers - http.errback { failed(http) } + http.errback { fail } + start = Time.now.to_i http.callback { - http.response_header.status.should == 200 - http.response_header["CONTENT_ENCODING"].should == "deflate" - + (Time.now.to_i - start).should be_within(2).of(0) EventMachine.stop } } end - it "should stream chunked gzipped data" do + it "should work with keep-alive servers" do EventMachine.run { - options = {:head => {"accept-encoding" => "gzip"}} - # GitHub sends chunked gzip, time for a little Inception ;) - http = EventMachine::AblyHttpRequest::HttpRequest.new('https://github.com/igrigorik/em-http-request/commits/master').get options + http = EventMachine::AblyHttpRequest::HttpRequest.new('https://github.com/igrigorik/em-http-request').get :keepalive => true http.errback { failed(http) } http.callback { http.response_header.status.should == 200 - http.response_header["CONTENT_ENCODING"].should == "gzip" - http.response.should == '' - EventMachine.stop } - - body = '' - http.stream do |chunk| - body << chunk - end } end - - context "keepalive" do - it "should default to non-keepalive" do - EventMachine.run { - headers = {'If-Modified-Since' => 'Thu, 05 Aug 2010 22:54:44 GMT'} - http = EventMachine::AblyHttpRequest::HttpRequest.new('http://www.google.com/images/logos/ps_logo2.png').get :head => headers - - http.errback { fail } - start = Time.now.to_i - http.callback { - (Time.now.to_i - start).should be_within(2).of(0) - EventMachine.stop - } - } - end - - it "should work with keep-alive servers" do - EventMachine.run { - http = EventMachine::AblyHttpRequest::HttpRequest.new('https://github.com/igrigorik/em-http-request').get :keepalive => true - - http.errback { failed(http) } - http.callback { - http.response_header.status.should == 200 - EventMachine.stop - } - } - end - end - end + end diff --git a/spec/helper.rb b/spec/helper.rb index 9b819dbc..8bd861cc 100644 --- a/spec/helper.rb +++ b/spec/helper.rb @@ -14,10 +14,6 @@ def failed(http = nil) http ? fail(http.error) : fail end -def requires_connection(&blk) - blk.call if system('ping -t1 -c1 google.com 2>&1 > /dev/null') -end - def requires_port(port, &blk) port_open = true begin diff --git a/spec/pipelining_spec.rb b/spec/pipelining_spec.rb index d83f008b..1ebb073c 100644 --- a/spec/pipelining_spec.rb +++ b/spec/pipelining_spec.rb @@ -1,66 +1,62 @@ require 'helper' -requires_connection do +describe EventMachine::AblyHttpRequest::HttpRequest do - describe EventMachine::AblyHttpRequest::HttpRequest do + it "should perform successful pipelined GETs" do + EventMachine.run do - it "should perform successful pipelined GETs" do - EventMachine.run do + # Mongrel doesn't support pipelined requests - bah! + conn = EventMachine::AblyHttpRequest::HttpRequest.new('http://www.bing.com/') - # Mongrel doesn't support pipelined requests - bah! - conn = EventMachine::AblyHttpRequest::HttpRequest.new('http://www.bing.com/') + pipe1 = conn.get :keepalive => true + pipe2 = conn.get :path => '/news', :keepalive => true - pipe1 = conn.get :keepalive => true - pipe2 = conn.get :path => '/news', :keepalive => true + processed = 0 + stop = proc { EM.stop if processed == 2} - processed = 0 - stop = proc { EM.stop if processed == 2} + pipe1.errback { failed(conn) } + pipe1.callback { + processed += 1 + pipe1.response_header.status.should == 200 + stop.call + } - pipe1.errback { failed(conn) } - pipe1.callback { - processed += 1 - pipe1.response_header.status.should == 200 - stop.call - } + pipe2.errback { failed(conn) } + pipe2.callback { + processed += 1 + pipe2.response_header.status.should == 200 + pipe2.response.should match(/html/i) + stop.call + } - pipe2.errback { failed(conn) } - pipe2.callback { - processed += 1 - pipe2.response_header.status.should == 200 - pipe2.response.should match(/html/i) - stop.call - } - - end end + end - it "should perform successful pipelined HEAD requests" do - EventMachine.run do - conn = EventMachine::AblyHttpRequest::HttpRequest.new('http://www.bing.com/') - - pipe1 = conn.head :keepalive => true - pipe2 = conn.head :path => '/news', :keepalive => true + it "should perform successful pipelined HEAD requests" do + EventMachine.run do + conn = EventMachine::AblyHttpRequest::HttpRequest.new('http://www.bing.com/') - processed = 0 - stop = proc { EM.stop if processed == 2} + pipe1 = conn.head :keepalive => true + pipe2 = conn.head :path => '/news', :keepalive => true - pipe1.errback { failed(conn) } - pipe1.callback { - processed += 1 - pipe1.response_header.status.should == 200 - stop.call - } + processed = 0 + stop = proc { EM.stop if processed == 2} - pipe2.errback { failed(conn) } - pipe2.callback { - processed += 1 - pipe2.response_header.status.should == 200 - stop.call - } + pipe1.errback { failed(conn) } + pipe1.callback { + processed += 1 + pipe1.response_header.status.should == 200 + stop.call + } - end + pipe2.errback { failed(conn) } + pipe2.callback { + processed += 1 + pipe2.response_header.status.should == 200 + stop.call + } end - end + end end diff --git a/spec/socksify_proxy_spec.rb b/spec/socksify_proxy_spec.rb index ff93b8b4..35ad3112 100644 --- a/spec/socksify_proxy_spec.rb +++ b/spec/socksify_proxy_spec.rb @@ -1,60 +1,56 @@ require 'helper' -requires_connection do +requires_port(8080) do + describe EventMachine::AblyHttpRequest::HttpRequest do - requires_port(8080) do - describe EventMachine::AblyHttpRequest::HttpRequest do + # ssh -D 8080 igvita + let(:proxy) { {:proxy => { :host => '127.0.0.1', :port => 8080, :type => :socks5 }} } - # ssh -D 8080 igvita - let(:proxy) { {:proxy => { :host => '127.0.0.1', :port => 8080, :type => :socks5 }} } + it "should use SOCKS5 proxy" do + EventMachine.run { + http = EventMachine::AblyHttpRequest::HttpRequest.new('http://jsonip.com/', proxy).get - it "should use SOCKS5 proxy" do - EventMachine.run { - http = EventMachine::AblyHttpRequest::HttpRequest.new('http://jsonip.com/', proxy).get - - http.errback { failed(http) } - http.callback { - http.response_header.status.should == 200 - http.response.should match('173.230.151.99') - EventMachine.stop - } + http.errback { failed(http) } + http.callback { + http.response_header.status.should == 200 + http.response.should match('173.230.151.99') + EventMachine.stop } - end + } end end +end - requires_port(8081) do - describe EventMachine::AblyHttpRequest::HttpRequest do +requires_port(8081) do + describe EventMachine::AblyHttpRequest::HttpRequest do - # brew install tinyproxy - let(:http_proxy) { {:proxy => { :host => '127.0.0.1', :port => 8081 }} } + # brew install tinyproxy + let(:http_proxy) { {:proxy => { :host => '127.0.0.1', :port => 8081 }} } - it "should use HTTP proxy by default" do - EventMachine.run { - http = EventMachine::AblyHttpRequest::HttpRequest.new('http://jsonip.com/', http_proxy).get + it "should use HTTP proxy by default" do + EventMachine.run { + http = EventMachine::AblyHttpRequest::HttpRequest.new('http://jsonip.com/', http_proxy).get - http.errback { failed(http) } - http.callback { - http.response_header.status.should == 200 - http.response.should match(/\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}/) - EventMachine.stop - } + http.errback { failed(http) } + http.callback { + http.response_header.status.should == 200 + http.response.should match(/\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}/) + EventMachine.stop } - end - - it "should auto CONNECT via HTTP proxy for HTTPS requests" do - EventMachine.run { - http = EventMachine::AblyHttpRequest::HttpRequest.new('https://ipjson.herokuapp.com/', http_proxy).get - - http.errback { failed(http) } - http.callback { - http.response_header.status.should == 200 - http.response.should match(/\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}/) - EventMachine.stop - } + } + end + + it "should auto CONNECT via HTTP proxy for HTTPS requests" do + EventMachine.run { + http = EventMachine::AblyHttpRequest::HttpRequest.new('https://ipjson.herokuapp.com/', http_proxy).get + + http.errback { failed(http) } + http.callback { + http.response_header.status.should == 200 + http.response.should match(/\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}/) + EventMachine.stop } - end + } end end - end diff --git a/spec/ssl_spec.rb b/spec/ssl_spec.rb index 82244d98..72ce9973 100644 --- a/spec/ssl_spec.rb +++ b/spec/ssl_spec.rb @@ -1,71 +1,67 @@ require 'helper' -requires_connection do +describe EventMachine::AblyHttpRequest::HttpRequest do + it "should initiate SSL/TLS on HTTPS connections" do + EventMachine.run { + http = EventMachine::AblyHttpRequest::HttpRequest.new('https://mail.google.com:443/mail/').get - describe EventMachine::AblyHttpRequest::HttpRequest do - it "should initiate SSL/TLS on HTTPS connections" do - EventMachine.run { - http = EventMachine::AblyHttpRequest::HttpRequest.new('https://mail.google.com:443/mail/').get - - http.errback { failed(http) } - http.callback { - http.response_header.status.should == 301 - EventMachine.stop - } + http.errback { failed(http) } + http.callback { + http.response_header.status.should == 301 + EventMachine.stop } - end + } + end - describe "TLS hostname verification" do - before do - @cve_warning = "[WARNING; ably-em-http-request] TLS hostname validation is disabled (use 'tls: {verify_peer: true}'), see" + - " CVE-2020-13482 and https://github.com/igrigorik/em-http-request/issues/339 for details" - @orig_stderr = $stderr - $stderr = StringIO.new - end + describe "TLS hostname verification" do + before do + @cve_warning = "[WARNING; ably-em-http-request] TLS hostname validation is disabled (use 'tls: {verify_peer: true}'), see" + + " CVE-2020-13482 and https://github.com/igrigorik/em-http-request/issues/339 for details" + @orig_stderr = $stderr + $stderr = StringIO.new + end - after do - $stderr = @orig_stderr - end + after do + $stderr = @orig_stderr + end - it "should not warn if verify_peer is specified" do - EventMachine.run { - http = EventMachine::AblyHttpRequest::HttpRequest.new('https://mail.google.com:443/mail', {tls: {verify_peer: false}}).get + it "should not warn if verify_peer is specified" do + EventMachine.run { + http = EventMachine::AblyHttpRequest::HttpRequest.new('https://mail.google.com:443/mail', {tls: {verify_peer: false}}).get - http.callback { - $stderr.rewind - $stderr.string.chomp.should_not eq(@cve_warning) + http.callback { + $stderr.rewind + $stderr.string.chomp.should_not eq(@cve_warning) - EventMachine.stop - } + EventMachine.stop } - end + } + end - it "should not warn if verify_peer is true" do - EventMachine.run { - http = EventMachine::AblyHttpRequest::HttpRequest.new('https://mail.google.com:443/mail', {tls: {verify_peer: true}}).get + it "should not warn if verify_peer is true" do + EventMachine.run { + http = EventMachine::AblyHttpRequest::HttpRequest.new('https://mail.google.com:443/mail', {tls: {verify_peer: true}}).get - http.callback { - $stderr.rewind - $stderr.string.chomp.should_not eq(@cve_warning) + http.callback { + $stderr.rewind + $stderr.string.chomp.should_not eq(@cve_warning) - EventMachine.stop - } + EventMachine.stop } - end + } + end - it "should warn if verify_peer is unspecified" do - EventMachine.run { - http = EventMachine::AblyHttpRequest::HttpRequest.new('https://mail.google.com:443/mail').get + it "should warn if verify_peer is unspecified" do + EventMachine.run { + http = EventMachine::AblyHttpRequest::HttpRequest.new('https://mail.google.com:443/mail').get - http.callback { - $stderr.rewind - $stderr.string.chomp.should eq(@cve_warning) + http.callback { + $stderr.rewind + $stderr.string.chomp.should eq(@cve_warning) - EventMachine.stop - } + EventMachine.stop } - end + } end end - end From 1edfedc84a6c0c0494f26efa5d9781dc947ffb13 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Tue, 14 May 2024 15:27:32 -0300 Subject: [PATCH 2/2] Revert "Fix server certificate verification" This reverts commit 9996b7b8db3a8c8f57c56854730a085955a652f0. --- lib/em-http/http_connection.rb | 122 ++++++--------------------------- 1 file changed, 21 insertions(+), 101 deletions(-) diff --git a/lib/em-http/http_connection.rb b/lib/em-http/http_connection.rb index db003e9c..3627a28f 100644 --- a/lib/em-http/http_connection.rb +++ b/lib/em-http/http_connection.rb @@ -40,118 +40,36 @@ def unbind(reason=nil) # TLS verification support, original implementation by Mislav Marohnić # https://github.com/lostisland/faraday/blob/63cf47c95b573539f047c729bd9ad67560bc83ff/lib/faraday/adapter/em_http_ssl_patch.rb - # - # Updated by Ably, here’s why: - # - # We noticed that the existing verification mechanism is failing in the - # case where the certificate chain presented by the server contains a - # certificate that’s signed by an expired trust anchor. At the time of - # writing, this is the case with some Let’s Encrypt certificate chains, - # which contain a cross-sign by the expired DST Root X3 CA. - # - # This isn’t meant to be an issue; the certificate chain presented by the - # server still contains a certificate that’s a trust anchor in most - # modern systems. So in the case where this trust anchor exists, OpenSSL - # would instead construct a certification path that goes straight to that - # anchor, bypassing the expired certificate. - # - # Unfortunately, as described in - # https://github.com/eventmachine/eventmachine/issues/954#issue-1014842247, - # EventMachine misuses OpenSSL in a variety of ways. One of them is that - # it does not configure a list of trust anchors, meaning that OpenSSL is - # unable to construct the correct certification path in the manner - # described above. - # - # This means that we end up in a degenerate situation where - # ssl_verify_peer just receives the certificates in the chain provided by - # the peer. In the scenario described above, one of these certificates is - # expired and hence the existing verification mechanism, which "verifies" - # each certificate provided to ssl_verify_peer, fails. - # - # So, instead we remove the existing ad-hoc mechanism for verification - # (which did things I’m not sure it should have done, like putting - # non-trust-anchors into an OpenSSL::X509::Store) and instead employ - # OpenSSL (configured to use the system trust store, and hence able to - # construct the correct certification path) to do all the hard work of - # constructing the certification path and then verifying the peer - # certificate. (This is what, in my opinion, EventMachine ideally would - # be allowing OpenSSL to do in the first place. Instead, as far as I can - # tell, it pushes all of this responsibility onto its users, and then - # provides them with an insufficient API for meeting this - # responsibility.) def ssl_verify_peer(cert_string) - # We use ssl_verify_peer simply as a mechanism for gathering the - # certificate chain presented by the peer. In ssl_handshake_completed, - # we’ll make use of this information in order to verify the peer. - @peer_certificate_chain ||= [] + cert = nil begin cert = OpenSSL::X509::Certificate.new(cert_string) - @peer_certificate_chain << cert - true rescue OpenSSL::X509::CertificateError return false end - end - def ssl_handshake_completed - # It’s not great to have to perform the server certificate verification - # after the handshake has completed, because it means: - # - # - We have to be sure that we don’t send any data over the TLS - # connection until we’ve verified the certificate. Created - # https://github.com/ably/ably-ruby/issues/400 to understand whether - # there’s anything we need to change to be sure of this. - # - # - If verification does fail, we have no way of failing the handshake - # with a bad_certificate error. - # - # Unfortunately there doesn’t seem to be a better alternative within - # the TLS-related APIs provided to us by EventMachine. (Admittedly I am - # not familiar with EventMachine.) - # - # (Performing the verification post-handshake is not new to the Ably - # implementation of certificate verification; the previous - # implementation performed hostname verification after the handshake - # was complete.) - - # I was quite worried by the description in the aforementioned issue - # eventmachine/eventmachine#954 of how EventMachine "ignores all errors - # from the chain construction" and hence I don’t know if there is some - # weird scenario where, somehow, the calls to ssl_verify_peer terminate - # with some intermediate certificate instead of with the certificate of - # the server we’re communicating with. (It's quite possible that this - # can’t occur and I’m just being paranoid, but I think a bit of - # paranoia when it comes to security isn't a bad thing.) - # - # That's why, instead of the previous code which passed - # certificate_store.verify the final certificate received by - # ssl_verify_peer, I explicitly use the result of get_peer_cert, to be - # sure that the certificate that we’re verifying is the one that the - # server has demonstrated that they hold the private key for. - server_certificate = OpenSSL::X509::Certificate.new(get_peer_cert) - - # A sense check to confirm my understanding of what’s in @peer_certificate_chain. - # - # (As mentioned above, unless something has gone very wrong, these two - # certificates should be identical.) - unless server_certificate == @peer_certificate_chain.last - raise OpenSSL::SSL::SSLError.new(%(Peer certificate sense check failed for "#{host}")); - end + @last_seen_cert = cert - # Verify the server’s certificate against the default trust anchors, - # aided by the intermediate certificates provided by the server. - unless create_certificate_store.verify(server_certificate, @peer_certificate_chain[0...-1]) + if certificate_store.verify(@last_seen_cert) + begin + certificate_store.add_cert(@last_seen_cert) + rescue OpenSSL::X509::StoreError => e + raise e unless e.message == 'cert already in hash table' + end + true + else raise OpenSSL::SSL::SSLError.new(%(unable to verify the server certificate for "#{host}")) end + end + def ssl_handshake_completed unless verify_peer? warn "[WARNING; ably-em-http-request] TLS hostname validation is disabled (use 'tls: {verify_peer: true}'), see" + " CVE-2020-13482 and https://github.com/igrigorik/em-http-request/issues/339 for details" unless parent.connopts.tls.has_key?(:verify_peer) return true end - # Verify that the peer’s certificate matches the hostname. - unless OpenSSL::SSL.verify_certificate_identity(server_certificate, host) + unless OpenSSL::SSL.verify_certificate_identity(@last_seen_cert, host) raise OpenSSL::SSL::SSLError.new(%(host "#{host}" does not match the server certificate)) else true @@ -166,12 +84,14 @@ def host parent.connopts.host end - def create_certificate_store - store = OpenSSL::X509::Store.new - store.set_default_paths - ca_file = parent.connopts.tls[:cert_chain_file] - store.add_file(ca_file) if ca_file - store + def certificate_store + @certificate_store ||= begin + store = OpenSSL::X509::Store.new + store.set_default_paths + ca_file = parent.connopts.tls[:cert_chain_file] + store.add_file(ca_file) if ca_file + store + end end end