acme: socket timeout for HTTP standalone servers (#7388)

* acme: socket timeout for HTTP standalone servers

Adds a default 30 second timeout to the StreamRequestHandler for clients
connecting to standalone HTTP-01 servers. This should prevent most cases
of an idle client connection from preventing the standalone server from
shutting down.

Fixes #7386

* use idiomatic kwargs default value

* move HTTP01Server lower to fix mypy forward ref.

* fix test crash on macOS due to socket double-close

* maybe its not an OSError?

* disable coverage check on useless branch

acme/acme/standalone.py

@@ -176,10 +176,10 @@ class HTTPServer(BaseHTTPServer.HTTPServer):
 class HTTP01Server(HTTPServer, ACMEServerMixin):
     """HTTP01 Server."""
 
-    def __init__(self, server_address, resources, ipv6=False):
+    def __init__(self, server_address, resources, ipv6=False, timeout=30):
         HTTPServer.__init__(
             self, server_address, HTTP01RequestHandler.partial_init(
-                simple_http_resources=resources), ipv6=ipv6)
+                simple_http_resources=resources, timeout=timeout), ipv6=ipv6)
 
 
 class HTTP01DualNetworkedServers(BaseDualNetworkedServers):
@@ -204,6 +204,7 @@ class HTTP01RequestHandler(BaseHTTPServer.BaseHTTPRequestHandler):
 
     def __init__(self, *args, **kwargs):
         self.simple_http_resources = kwargs.pop("simple_http_resources", set())
+        self.timeout = kwargs.pop('timeout', 30)
         BaseHTTPServer.BaseHTTPRequestHandler.__init__(self, *args, **kwargs)
 
     def log_message(self, format, *args):  # pylint: disable=redefined-builtin
@@ -253,7 +254,7 @@ class HTTP01RequestHandler(BaseHTTPServer.BaseHTTPRequestHandler):
                          self.path)
 
     @classmethod
-    def partial_init(cls, simple_http_resources):
+    def partial_init(cls, simple_http_resources, timeout):
         """Partially initialize this handler.
 
         This is useful because `socketserver.BaseServer` takes
@@ -262,7 +263,8 @@ class HTTP01RequestHandler(BaseHTTPServer.BaseHTTPRequestHandler):
 
         """
         return functools.partial(
-            cls, simple_http_resources=simple_http_resources)
+            cls, simple_http_resources=simple_http_resources,
+            timeout=timeout)
 
 
 class _BaseRequestHandlerWithLogging(socketserver.BaseRequestHandler):

acme/tests/standalone_test.py

@@ -85,6 +85,28 @@ class HTTP01ServerTest(unittest.TestCase):
     def test_http01_not_found(self):
         self.assertFalse(self._test_http01(add=False))
 
+    def test_timely_shutdown(self):
+        from acme.standalone import HTTP01Server
+        server = HTTP01Server(('', 0), resources=set(), timeout=0.05)
+        server_thread = threading.Thread(target=server.serve_forever)
+        server_thread.start()
+
+        client = socket.socket()
+        client.connect(('localhost', server.socket.getsockname()[1]))
+
+        stop_thread = threading.Thread(target=server.shutdown)
+        stop_thread.start()
+        server_thread.join(5.)
+
+        is_hung = server_thread.is_alive()
+        try:
+            client.shutdown(socket.SHUT_RDWR)
+        except: # pragma: no cover, pylint: disable=bare-except
+            # may raise error because socket could already be closed
+            pass
+
+        self.assertFalse(is_hung, msg='Server shutdown should not be hung')
+
 
 @unittest.skipIf(not challenges.TLSALPN01.is_supported(), "pyOpenSSL too old")
 class TLSALPN01ServerTest(unittest.TestCase):

certbot/CHANGELOG.md

@@ -26,6 +26,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
 * Fix nginx plugin crash when non-ASCII configuration file is being read (instead,
   the user will be warned that UTF-8 must be used).
 * Fix hanging OCSP queries during revocation checking - added a 10 second timeout.
+* Standalone servers now have a default socket timeout of 30 seconds, fixing
+  cases where an idle connection can cause the standalone plugin to hang.
 
 More details about these changes can be found on our GitHub repo.