Only verify required ports are available (#3608)

* only verify port is available when you actually need it

* refactor code to create achalls

* Test port checks are based on achall

* test that only the port for the requested challenge is checked in standalone

certbot/plugins/standalone.py

@@ -194,15 +194,6 @@ class Authenticator(common.Plugin):
         return [challenges.Challenge.TYPES[name] for name in
                 self.conf("supported-challenges").split(",")]
 
-    @property
-    def _necessary_ports(self):
-        necessary_ports = set()
-        if challenges.HTTP01 in self.supported_challenges:
-            necessary_ports.add(self.config.http01_port)
-        if challenges.TLSSNI01 in self.supported_challenges:
-            necessary_ports.add(self.config.tls_sni_01_port)
-        return necessary_ports
-
     def more_info(self):  # pylint: disable=missing-docstring
         return("This authenticator creates its own ephemeral TCP listener "
                "on the necessary port in order to respond to incoming "
@@ -217,12 +208,30 @@ class Authenticator(common.Plugin):
         # pylint: disable=unused-argument,missing-docstring
         return self.supported_challenges
 
-    def perform(self, achalls):  # pylint: disable=missing-docstring
+    def _verify_ports_are_available(self, achalls):
-        renewer = self.config.verb == "renew"
+        """Confirm the ports are available to solve all achalls.
-        if any(util.already_listening(port, renewer) for port in self._necessary_ports):
+
+        :param list achalls: list of
+            :class:`~certbot.achallenges.AnnotatedChallenge`
+
+        :raises .errors.MisconfigurationError: if required port is
+            unavailable
+
+        """
+        ports = []
+        if any(isinstance(ac.chall, challenges.HTTP01) for ac in achalls):
+            ports.append(self.config.http01_port)
+        if any(isinstance(ac.chall, challenges.TLSSNI01) for ac in achalls):
+            ports.append(self.config.tls_sni_01_port)
+
+        renewer = (self.config.verb == "renew")
+
+        if any(util.already_listening(port, renewer) for port in ports):
             raise errors.MisconfigurationError(
-                "At least one of the (possibly) required ports is "
+                "At least one of the required ports is already taken.")
-                "already taken.")
+
+    def perform(self, achalls):  # pylint: disable=missing-docstring
+        self._verify_ports_are_available(achalls)
 
         try:
             return self.perform2(achalls)

certbot/plugins/standalone_test.py

@@ -137,20 +137,33 @@ class AuthenticatorTest(unittest.TestCase):
         self.assertEqual(self.auth.get_chall_pref(domain=None),
                          [challenges.TLSSNI01])
 
+    @classmethod
+    def _get_achalls(cls):
+        domain = b'localhost'
+        key = jose.JWK.load(test_util.load_vector('rsa512_key.pem'))
+        http_01 = achallenges.KeyAuthorizationAnnotatedChallenge(
+            challb=acme_util.HTTP01_P, domain=domain, account_key=key)
+        tls_sni_01 = achallenges.KeyAuthorizationAnnotatedChallenge(
+            challb=acme_util.TLSSNI01_P, domain=domain, account_key=key)
+
+        return [http_01, tls_sni_01]
+
     @mock.patch("certbot.plugins.standalone.util")
     def test_perform_already_listening(self, mock_util):
-        for chall, port in ((challenges.TLSSNI01.typ, 1234),
+        http_01, tls_sni_01 = self._get_achalls()
-                            (challenges.HTTP01.typ, 4321)):
+
+        for achall, port in ((http_01, self.config.http01_port,),
+                             (tls_sni_01, self.config.tls_sni_01_port)):
             mock_util.already_listening.return_value = True
-            self.config.standalone_supported_challenges = chall
             self.assertRaises(
-                errors.MisconfigurationError, self.auth.perform, [])
+                errors.MisconfigurationError, self.auth.perform, [achall])
             mock_util.already_listening.assert_called_once_with(port, False)
             mock_util.already_listening.reset_mock()
 
     @mock.patch("certbot.plugins.standalone.zope.component.getUtility")
     def test_perform(self, unused_mock_get_utility):
-        achalls = [1, 2, 3]
+        achalls = self._get_achalls()
+
         self.auth.perform2 = mock.Mock(return_value=mock.sentinel.responses)
         self.assertEqual(mock.sentinel.responses, self.auth.perform(achalls))
         self.auth.perform2.assert_called_once_with(achalls)
@@ -181,12 +194,7 @@ class AuthenticatorTest(unittest.TestCase):
             socket.errno.ENOTCONN, [])
 
     def test_perform2(self):
-        domain = b'localhost'
+        http_01, tls_sni_01 = self._get_achalls()
-        key = jose.JWK.load(test_util.load_vector('rsa512_key.pem'))
-        http_01 = achallenges.KeyAuthorizationAnnotatedChallenge(
-            challb=acme_util.HTTP01_P, domain=domain, account_key=key)
-        tls_sni_01 = achallenges.KeyAuthorizationAnnotatedChallenge(
-            challb=acme_util.TLSSNI01_P, domain=domain, account_key=key)
 
         self.auth.servers = mock.MagicMock()
 

tests/boulder-integration.sh

@@ -33,8 +33,17 @@ common() {
         "$@"
 }
 
-common --domains le1.wtf --standalone-supported-challenges tls-sni-01 auth
+# We start a server listening on the port for the
-common --domains le2.wtf --standalone-supported-challenges http-01 run
+# unrequested challenge to prevent regressions in #3601.
+python -m SimpleHTTPServer $http_01_port &
+python_server_pid=$!
+common --domains le1.wtf --preferred-challenges tls-sni-01 auth
+kill $python_server_pid
+python -m SimpleHTTPServer $tls_sni_01_port &
+python_server_pid=$!
+common --domains le2.wtf --preferred-challenges http-01 run
+kill $python_server_pid
+
 common -a manual -d le.wtf auth --rsa-key-size 4096
 
 export CSR_PATH="${root}/csr.der" KEY_PATH="${root}/key.pem" \

tests/integration/_common.sh

@@ -9,7 +9,9 @@ then
 fi
 store_flags="--config-dir $root/conf --work-dir $root/work"
 store_flags="$store_flags --logs-dir $root/logs"
-export root store_flags
+tls_sni_01_port=5001
+http_01_port=5002
+export root store_flags tls_sni_01_port http_01_port
 
 certbot_test () {
     certbot_test_no_force_renew \
@@ -21,8 +23,8 @@ certbot_test_no_force_renew () {
     certbot \
         --server "${SERVER:-http://localhost:4000/directory}" \
         --no-verify-ssl \
-        --tls-sni-01-port 5001 \
+        --tls-sni-01-port $tls_sni_01_port \
-        --http-01-port 5002 \
+        --http-01-port $http_01_port \
         --manual-test-mode \
         $store_flags \
         --non-interactive \