[#499][#500] Handle servers that don't return an HTTP response

sslyze/connection_helpers/http_response_parser.py

@@ -10,6 +10,10 @@ def makefile(self, *args, **kw):  # type: ignore
         return self
 
 
+class NotAValidHttpResponseError(Exception):
+    pass
+
+
 class HttpResponseParser:
     """Utility to parse HTTP responses - http://pythonwise.blogspot.com/2010/02/parse-http-response.html.
     """
@@ -34,4 +38,9 @@ def _parse(read_method: Callable) -> HTTPResponse:
         fake_sock = _FakeSocket(response)
         response = HTTPResponse(fake_sock)  # type: ignore
         response.begin()
+
+        if response.version == 9:
+            # HTTP 0.9 => Probably not an HTTP response
+            raise NotAValidHttpResponseError()
+
         return response

sslyze/plugins/http_headers_plugin.py

@@ -1,8 +1,10 @@
 import logging
+import socket
 from concurrent.futures._base import Future
 from http.client import HTTPResponse
 
 from dataclasses import dataclass
+from traceback import TracebackException
 from urllib.parse import urlsplit
 
 from sslyze.plugins.plugin_base import (
@@ -15,7 +17,7 @@
 )
 from sslyze.server_connectivity import ServerConnectivityInfo
 from sslyze.connection_helpers.http_request_generator import HttpRequestGenerator
-from sslyze.connection_helpers.http_response_parser import HttpResponseParser
+from sslyze.connection_helpers.http_response_parser import HttpResponseParser, NotAValidHttpResponseError
 from typing import List, Optional
 
 
@@ -76,20 +78,27 @@ class StrictTransportSecurityHeader:
 class HttpHeadersScanResult(ScanCommandResult):
     """The result of testing a server for the presence of security-related HTTP headers.
 
-    Each HTTP header described below will be ``None`` if the server did not return it.
+    Each HTTP header described below will be ``None`` if the server did not return a valid HTTP response, or if the
+    server returned an HTTP response without the HTTP header.
 
     Attributes:
+        http_request_sent: The initial HTTP request sent to the server by SSLyze.
+        http_error_trace: An error the server returned after receiving the initial HTTP request. If this field is set,
+            all the subsequent fields will be ``None`` as SSLyze did not receive a valid HTTP response from the server.
+        http_path_redirected_to: The path SSLyze was eventually redirected to after sending the initial HTTP request.
         strict_transport_security_header: The Strict-Transport-Security header returned by the server.
         public_key_pins_header: The Public-Key-Pins header returned by the server.
         public_key_pins_report_only_header: The Public-Key-Pins-Report-Only header returned by the server.
         expect_ct_header: The Expect-CT header returned by the server.
     """
 
-    strict_transport_security_header: Optional[StrictTransportSecurityHeader]
+    http_request_sent: str
+    http_error_trace: Optional[TracebackException]
 
+    http_path_redirected_to: Optional[str]
+    strict_transport_security_header: Optional[StrictTransportSecurityHeader]
     public_key_pins_header: Optional[PublicKeyPinsHeader]
     public_key_pins_report_only_header: Optional[PublicKeyPinsHeader]
-
     expect_ct_header: Optional[ExpectCtHeader]
 
 
@@ -102,6 +111,16 @@ class _HttpHeadersCliConnector(ScanCommandCliConnector[HttpHeadersScanResult, No
     def result_to_console_output(cls, result: HttpHeadersScanResult) -> List[str]:
         result_as_txt = [cls._format_title("HTTP Security Headers")]
 
+        # If an error occurred after sending the HTTP request, just display it
+        if result.http_error_trace:
+            result_as_txt.append(
+                cls._format_subtitle("Error - Server did not return a valid HTTP response. Is it an HTTP server?")
+            )
+            result_as_txt.append("")
+            for trace_line in result.http_error_trace.format(chain=False):
+                result_as_txt.append(f"       {trace_line.strip()}")
+            return result_as_txt
+
         # HSTS
         result_as_txt.append(cls._format_subtitle("Strict-Transport-Security Header"))
         if not result.strict_transport_security_header:
@@ -174,26 +193,34 @@ def _retrieve_and_analyze_http_response(server_info: ServerConnectivityInfo) ->
     _logger.info(f"Retrieving HTTP headers from {server_info}")
     redirections_count = 0
     next_location_path: Optional[str] = "/"
+    http_error_trace = None
+
     while next_location_path and redirections_count < 4:
-        _logger.info(f"Sending request to {next_location_path}")
+        _logger.info(f"Sending HTTP request to {next_location_path}")
+        http_path_redirected_to = next_location_path
+
+        # Perform the TLS handshake
         ssl_connection = server_info.get_preconfigured_tls_connection()
-        try:
-            # Perform the TLS handshake
-            ssl_connection.connect()
+        ssl_connection.connect()
 
+        try:
             # Send an HTTP GET request to the server
             ssl_connection.ssl_client.write(
                 HttpRequestGenerator.get_request(
                     host=server_info.network_configuration.tls_server_name_indication, path=next_location_path
                 )
             )
             http_response = HttpResponseParser.parse_from_ssl_connection(ssl_connection.ssl_client)
+
+        except (socket.timeout, ConnectionError, NotAValidHttpResponseError) as e:
+            # The server didn't return a proper HTTP response
+            http_error_trace = TracebackException.from_exception(e)
+
         finally:
             ssl_connection.close()
 
-        if http_response.version == 9:
-            # HTTP 0.9 => Probably not an HTTP response
-            raise ValueError("Server did not return an HTTP response")
+        if http_error_trace:
+            break
 
         # Handle redirection if there is one
         next_location_path = _detect_http_redirection(
@@ -203,13 +230,33 @@ def _retrieve_and_analyze_http_response(server_info: ServerConnectivityInfo) ->
         )
         redirections_count += 1
 
-    # Parse and return each header
-    return HttpHeadersScanResult(
-        strict_transport_security_header=_parse_hsts_header_from_http_response(http_response),
-        public_key_pins_header=_parse_hpkp_header_from_http_response(http_response),
-        public_key_pins_report_only_header=_parse_hpkp_report_only_header_from_http_response(http_response),
-        expect_ct_header=_parse_expect_ct_header_from_http_response(http_response),
-    )
+    # Prepare the results
+    initial_http_request = HttpRequestGenerator.get_request(
+        host=server_info.network_configuration.tls_server_name_indication, path="/"
+    ).decode("ascii")
+
+    if http_error_trace:
+        # If the server errored when receiving an HTTP request, return the error as the result
+        return HttpHeadersScanResult(
+            http_request_sent=initial_http_request,
+            http_error_trace=http_error_trace,
+            http_path_redirected_to=None,
+            strict_transport_security_header=None,
+            public_key_pins_header=None,
+            public_key_pins_report_only_header=None,
+            expect_ct_header=None,
+        )
+    else:
+        # If no HTTP error happened, parse and return each header
+        return HttpHeadersScanResult(
+            http_request_sent=initial_http_request,
+            http_path_redirected_to=http_path_redirected_to,
+            http_error_trace=None,
+            strict_transport_security_header=_parse_hsts_header_from_http_response(http_response),
+            public_key_pins_header=_parse_hpkp_header_from_http_response(http_response),
+            public_key_pins_report_only_header=_parse_hpkp_report_only_header_from_http_response(http_response),
+            expect_ct_header=_parse_expect_ct_header_from_http_response(http_response),
+        )
 
 
 def _detect_http_redirection(http_response: HTTPResponse, server_host_name: str, server_port: int) -> Optional[str]:

tests/openssl_server/__init__.py

@@ -10,7 +10,7 @@
 import logging
 import time
 from threading import Thread
-from typing import Optional, List
+from typing import Optional, List, IO
 
 
 class NotOnLinux64Error(EnvironmentError):
@@ -31,30 +31,34 @@ class _OpenSslServerIOManager:
     """Thread to log all output from s_server and reply to incoming connections.
     """
 
-    def __init__(self, s_server_stdout, s_server_stdin):
-        self.s_server_stdout = s_server_stdout
-        self.s_server_stdin = s_server_stdin
+    def __init__(
+        self, s_server_stdout: IO[bytes], s_server_stdin: IO[bytes], should_reply_to_http_requests: bool
+    ) -> None:
+        self._s_server_stdout = s_server_stdout
+        self._s_server_stdin = s_server_stdin
+        self._should_reply_to_http_requests = should_reply_to_http_requests
         self.is_server_ready = False
 
         def read_and_log_and_reply():
             while True:
-                s_server_out = self.s_server_stdout.readline()
+                s_server_out = self._s_server_stdout.readline()
                 if s_server_out:
                     logging.warning(f"s_server output: {s_server_out}")
 
                     if b"ACCEPT" in s_server_out:
-                        # S_server is ready to receive connections
+                        # The s_server process is ready to receive connections
                         self.is_server_ready = True
 
                     if _OpenSslServer.HELLO_MSG in s_server_out:
                         # When receiving the special message, we want s_server to reply
-                        self.s_server_stdin.write(b"Hey there")
-                        self.s_server_stdin.flush()
-
-                    if b"Connection: close\r\n" in s_server_out:
-                        # We "Connection: close" to detect an HTTP request being sent and we return an HTTP response
-                        self.s_server_stdin.write(b"HTTP/1.1 200 OK\r\nContent-Length: 0\r\n\r\n")
-                        self.s_server_stdin.flush()
+                        self._s_server_stdin.write(b"Hey there")
+                        self._s_server_stdin.flush()
+
+                    if self._should_reply_to_http_requests:
+                        if b"Connection: close\r\n" in s_server_out:
+                            # We "Connection: close" to detect an HTTP request being sent and we return an HTTP response
+                            self._s_server_stdin.write(b"HTTP/1.1 200 OK\r\nContent-Length: 0\r\n\r\n")
+                            self._s_server_stdin.flush()
                 else:
                     break
 
@@ -125,6 +129,7 @@ def __init__(
         client_auth_config: ClientAuthConfigEnum,
         should_enable_server_cipher_preference: bool,
         openssl_cipher_string: Optional[str],
+        should_reply_to_http_requests: bool = True,
         extra_openssl_args: Optional[List[str]] = None,
     ) -> None:
         if not self.is_platform_supported():
@@ -138,6 +143,7 @@ def __init__(
         self.ip_address = "127.0.0.1"
         self._server_certificate_path = server_certificate_path
         self._server_key_path = server_key_path
+        self._should_reply_to_http_requests = should_reply_to_http_requests
 
         # Retrieve one of the available local ports; set.pop() is thread safe
         self.port = self._AVAILABLE_LOCAL_PORTS.pop()
@@ -161,11 +167,13 @@ def __enter__(self):
             self._process = subprocess.Popen(
                 args, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.STDOUT
             )
-            self._server_io_manager = _OpenSslServerIOManager(self._process.stdout, self._process.stdin)
+            self._server_io_manager = _OpenSslServerIOManager(
+                self._process.stdout, self._process.stdin, self._should_reply_to_http_requests
+            )
 
             # Block until s_server is ready to accept requests
             while not self._server_io_manager.is_server_ready:
-                time.sleep(1)
+                time.sleep(0.5)
                 if self._process.poll() is not None:
                     # s_server has terminated early
                     raise RuntimeError("Could not start s_server")
@@ -270,6 +278,7 @@ def __init__(
         client_auth_config: ClientAuthConfigEnum = ClientAuthConfigEnum.DISABLED,
         should_enable_server_cipher_preference: bool = False,
         openssl_cipher_string: Optional[str] = None,
+        should_reply_to_http_requests: bool = True,
         max_early_data: Optional[int] = None,
         groups: Optional[str] = None,
     ) -> None:
@@ -288,4 +297,5 @@ def __init__(
             should_enable_server_cipher_preference=should_enable_server_cipher_preference,
             server_certificate_path=server_certificate_path,
             server_key_path=server_key_path,
+            should_reply_to_http_requests=should_reply_to_http_requests,
         )

tests/plugins_tests/test_http_headers_plugin.py

@@ -17,7 +17,7 @@
     ClientAuthenticationCredentials,
 )
 from tests.markers import can_only_run_on_linux_64
-from tests.openssl_server import ClientAuthConfigEnum, LegacyOpenSslServer
+from tests.openssl_server import ClientAuthConfigEnum, LegacyOpenSslServer, ModernOpenSslServer
 
 
 class TestHttpHeadersPlugin:
@@ -30,6 +30,8 @@ def test_hsts_enabled(self):
         result: HttpHeadersScanResult = HttpHeadersImplementation.scan_server(server_info)
 
         # And only HSTS is detected
+        assert result.http_request_sent
+        assert result.http_path_redirected_to
         assert result.strict_transport_security_header
         assert not result.public_key_pins_header
         assert not result.public_key_pins_report_only_header
@@ -47,6 +49,8 @@ def test_hsts_and_hpkp_disabled(self):
         result: HttpHeadersScanResult = HttpHeadersImplementation.scan_server(server_info)
 
         # And no headers are detected
+        assert result.http_request_sent
+        assert result.http_path_redirected_to
         assert not result.strict_transport_security_header
         assert not result.public_key_pins_header
         assert not result.public_key_pins_report_only_header
@@ -70,6 +74,33 @@ def test_expect_ct_enabled(self):
         # And a CLI output can be generated
         assert HttpHeadersImplementation.cli_connector_cls.result_to_console_output(result)
 
+    def test_http_error(self):
+        # Given a server to scan
+        with ModernOpenSslServer(
+            # And the server will trigger an error when receiving an HTTP request
+            should_reply_to_http_requests=False
+        ) as server:
+            server_location = ServerNetworkLocationViaDirectConnection(
+                hostname=server.hostname, ip_address=server.ip_address, port=server.port
+            )
+            server_info = ServerConnectivityTester().perform(server_location)
+
+            # When scanning for HTTP headers, it succeeds
+            result: HttpHeadersScanResult = HttpHeadersImplementation.scan_server(server_info)
+
+        # And the result mention the error returned by the server when sending an HTTP request
+        assert result.http_error_trace
+        assert result.http_request_sent
+
+        # And the other result fields are not set
+        assert not result.http_path_redirected_to
+        assert not result.public_key_pins_header
+        assert not result.public_key_pins_report_only_header
+        assert not result.expect_ct_header
+
+        # And a CLI output can be generated
+        assert HttpHeadersImplementation.cli_connector_cls.result_to_console_output(result)
+
     @can_only_run_on_linux_64
     def test_fails_when_client_auth_failed(self):
         # Given a server that requires client authentication