Merge pull request #5 from ably-forks/fix-location-of-verify-peer-check

Fix location of `verify_peer` check

.github/workflows/ci.yml

@@ -20,15 +20,3 @@ jobs:
           ruby-version: ${{ matrix.ruby }}
           bundler-cache: true
       - run: bundle exec rake spec
-  spec-legacy:
-    name: "RSpec / Ruby 2.2"
-    runs-on: ubuntu-20.04
-    steps:
-      - run: sudo apt-get install libcurl4-openssl-dev
-      - uses: actions/checkout@v4
-      - uses: ruby/setup-ruby@v1
-        with:
-          ruby-version: 2.2
-          bundler-cache: true
-      - name: rake spec
-        run: bundle exec rake spec

lib/em-http/http_connection.rb

@@ -94,6 +94,16 @@ def ssl_verify_peer(cert_string)
       end
 
       def ssl_handshake_completed
+        # Warning message updated by Ably — the previous message suggested that
+        # when verify_peer is false, the server certificate would be verified
+        # but not checked against the hostname. This is not true — when
+        # verify_peer is false, the server certificate is not verified at all.
+        unless verify_peer?
+          warn "[WARNING; ably-em-http-request] TLS server certificate 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
+
         # It’s not great to have to perform the server certificate verification
         # after the handshake has completed, because it means:
         #
@@ -134,7 +144,7 @@ def ssl_handshake_completed
         #
         # (As mentioned above, unless something has gone very wrong, these two
         # certificates should be identical.)
-        unless server_certificate == @peer_certificate_chain.last
+        unless server_certificate.to_der == @peer_certificate_chain.last.to_der
           raise OpenSSL::SSL::SSLError.new(%(Peer certificate sense check failed for "#{host}"));
         end
 
@@ -144,12 +154,6 @@ def ssl_handshake_completed
           raise OpenSSL::SSL::SSLError.new(%(unable to verify the server certificate for "#{host}"))
         end
 
-        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)
           raise OpenSSL::SSL::SSLError.new(%(host "#{host}" does not match the server certificate))

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

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