Enhance verb (#5596)

* Add the cli parameters

* Tests and error messages

* Requested fixes

* Only handle SSL vhosts

* Interactive cert-name selection if not defined on CLI

* Address PR comments

* Address review comments

* Added tests and addressed review comments

* Move cert manager tests to correct file

* Add display ops tests

* Use display util constants instead of hardcoded values in tests

certbot-apache/certbot_apache/configurator.py

@@ -323,7 +323,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
             # Returned objects are guaranteed to be ssl vhosts
             return self._choose_vhosts_wildcard(domain, create_if_no_ssl)
         else:
-            return [self.choose_vhost(domain)]
+            return [self.choose_vhost(domain, create_if_no_ssl)]
 
     def _vhosts_for_wildcard(self, domain):
         """
@@ -475,20 +475,21 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         if chain_path is not None:
             self.save_notes += "\tSSLCertificateChainFile %s\n" % chain_path
 
-    def choose_vhost(self, target_name, temp=False):
+    def choose_vhost(self, target_name, create_if_no_ssl=True):
         """Chooses a virtual host based on the given domain name.
 
         If there is no clear virtual host to be selected, the user is prompted
         with all available choices.
 
-        The returned vhost is guaranteed to have TLS enabled unless temp is
-        True. If temp is True, there is no such guarantee and the result is
-        not cached.
+        The returned vhost is guaranteed to have TLS enabled unless
+        create_if_no_ssl is set to False, in which case there is no such guarantee
+        and the result is not cached.
 
         :param str target_name: domain name
-        :param bool temp: whether the vhost is only used temporarily
+        :param bool create_if_no_ssl: If found VirtualHost doesn't have a HTTPS
+            counterpart, should one get created
 
-        :returns: ssl vhost associated with name
+        :returns: vhost associated with name
         :rtype: :class:`~certbot_apache.obj.VirtualHost`
 
         :raises .errors.PluginError: If no vhost is available or chosen
@@ -501,7 +502,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         # Try to find a reasonable vhost
         vhost = self._find_best_vhost(target_name)
         if vhost is not None:
-            if temp:
+            if not create_if_no_ssl:
                 return vhost
             if not vhost.ssl:
                 vhost = self.make_vhost_ssl(vhost)
@@ -510,7 +511,9 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
             self.assoc[target_name] = vhost
             return vhost
 
-        return self._choose_vhost_from_list(target_name, temp)
+        # Negate create_if_no_ssl value to indicate if we want a SSL vhost
+        # to get created if a non-ssl vhost is selected.
+        return self._choose_vhost_from_list(target_name, temp=not create_if_no_ssl)
 
     def _choose_vhost_from_list(self, target_name, temp=False):
         # Select a vhost from a list
@@ -1505,7 +1508,20 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
             raise errors.PluginError(
                 "Unsupported enhancement: {0}".format(enhancement))
 
-        vhosts = self.choose_vhosts(domain, create_if_no_ssl=False)
+        matched_vhosts = self.choose_vhosts(domain, create_if_no_ssl=False)
+        # We should be handling only SSL vhosts for enhancements
+        vhosts = [vhost for vhost in matched_vhosts if vhost.ssl]
+
+        if not vhosts:
+            msg_tmpl = ("Certbot was not able to find SSL VirtualHost for a "
+                        "domain {0} for enabling enhancement \"{1}\". The requested "
+                        "enhancement was not configured.")
+            msg_enhancement = enhancement
+            if options:
+                msg_enhancement += ": " + options
+            msg = msg_tmpl.format(domain, msg_enhancement)
+            logger.warning(msg)
+            raise errors.PluginError(msg)
         try:
             for vhost in vhosts:
                 func(vhost, options)

certbot-apache/certbot_apache/tests/configurator_test.py

@@ -246,7 +246,7 @@ class MultipleVhostsTest(util.ApacheTest):
     @mock.patch("certbot_apache.display_ops.select_vhost")
     def test_choose_vhost_select_vhost_with_temp(self, mock_select):
         mock_select.return_value = self.vh_truth[0]
-        chosen_vhost = self.config.choose_vhost("none.com", temp=True)
+        chosen_vhost = self.config.choose_vhost("none.com", create_if_no_ssl=False)
         self.assertEqual(self.vh_truth[0], chosen_vhost)
 
     @mock.patch("certbot_apache.display_ops.select_vhost")
@@ -936,6 +936,22 @@ class MultipleVhostsTest(util.ApacheTest):
             errors.PluginError,
             self.config.enhance, "certbot.demo", "unknown_enhancement")
 
+    def test_enhance_no_ssl_vhost(self):
+        with mock.patch("certbot_apache.configurator.logger.warning") as mock_log:
+            self.assertRaises(errors.PluginError, self.config.enhance,
+                              "certbot.demo", "redirect")
+            # Check that correct logger.warning was printed
+            self.assertTrue("not able to find" in mock_log.call_args[0][0])
+            self.assertTrue("\"redirect\"" in mock_log.call_args[0][0])
+
+            mock_log.reset_mock()
+
+            self.assertRaises(errors.PluginError, self.config.enhance,
+                              "certbot.demo", "ensure-http-header", "Test")
+            # Check that correct logger.warning was printed
+            self.assertTrue("not able to find" in mock_log.call_args[0][0])
+            self.assertTrue("Test" in mock_log.call_args[0][0])
+
     @mock.patch("certbot.util.exe_exists")
     def test_ocsp_stapling(self, mock_exe):
         self.config.parser.update_runtime_variables = mock.Mock()
@@ -945,6 +961,7 @@ class MultipleVhostsTest(util.ApacheTest):
         mock_exe.return_value = True
 
         # This will create an ssl vhost for certbot.demo
+        self.config.choose_vhost("certbot.demo")
         self.config.enhance("certbot.demo", "staple-ocsp")
 
         # Get the ssl vhost for certbot.demo
@@ -971,6 +988,7 @@ class MultipleVhostsTest(util.ApacheTest):
         mock_exe.return_value = True
 
         # Checking the case with already enabled ocsp stapling configuration
+        self.config.choose_vhost("ocspvhost.com")
         self.config.enhance("ocspvhost.com", "staple-ocsp")
 
         # Get the ssl vhost for letsencrypt.demo
@@ -995,6 +1013,7 @@ class MultipleVhostsTest(util.ApacheTest):
         self.config.parser.modules.add("mod_ssl.c")
         self.config.parser.modules.add("socache_shmcb_module")
         self.config.get_version = mock.Mock(return_value=(2, 2, 0))
+        self.config.choose_vhost("certbot.demo")
 
         self.assertRaises(errors.PluginError,
                 self.config.enhance, "certbot.demo", "staple-ocsp")
@@ -1020,6 +1039,7 @@ class MultipleVhostsTest(util.ApacheTest):
         mock_exe.return_value = True
 
         # This will create an ssl vhost for certbot.demo
+        self.config.choose_vhost("certbot.demo")
         self.config.enhance("certbot.demo", "ensure-http-header",
                             "Strict-Transport-Security")
 
@@ -1039,7 +1059,8 @@ class MultipleVhostsTest(util.ApacheTest):
         # skip the enable mod
         self.config.parser.modules.add("headers_module")
 
-        # This will create an ssl vhost for certbot.demo
+        # This will create an ssl vhost for encryption-example.demo
+        self.config.choose_vhost("encryption-example.demo")
         self.config.enhance("encryption-example.demo", "ensure-http-header",
                             "Strict-Transport-Security")
 
@@ -1058,6 +1079,7 @@ class MultipleVhostsTest(util.ApacheTest):
         mock_exe.return_value = True
 
         # This will create an ssl vhost for certbot.demo
+        self.config.choose_vhost("certbot.demo")
         self.config.enhance("certbot.demo", "ensure-http-header",
                             "Upgrade-Insecure-Requests")
 
@@ -1079,7 +1101,8 @@ class MultipleVhostsTest(util.ApacheTest):
         # skip the enable mod
         self.config.parser.modules.add("headers_module")
 
-        # This will create an ssl vhost for certbot.demo
+        # This will create an ssl vhost for encryption-example.demo
+        self.config.choose_vhost("encryption-example.demo")
         self.config.enhance("encryption-example.demo", "ensure-http-header",
                             "Upgrade-Insecure-Requests")
 
@@ -1097,6 +1120,7 @@ class MultipleVhostsTest(util.ApacheTest):
         self.config.get_version = mock.Mock(return_value=(2, 2))
 
         # This will create an ssl vhost for certbot.demo
+        self.config.choose_vhost("certbot.demo")
         self.config.enhance("certbot.demo", "redirect")
 
         # These are not immediately available in find_dir even with save() and
@@ -1147,6 +1171,7 @@ class MultipleVhostsTest(util.ApacheTest):
         self.config.save()
 
         # This will create an ssl vhost for certbot.demo
+        self.config.choose_vhost("certbot.demo")
         self.config.enhance("certbot.demo", "redirect")
 
         # These are not immediately available in find_dir even with save() and
@@ -1213,6 +1238,9 @@ class MultipleVhostsTest(util.ApacheTest):
         self.config.parser.modules.add("rewrite_module")
         self.config.get_version = mock.Mock(return_value=(2, 3, 9))
 
+        # Creates ssl vhost for the domain
+        self.config.choose_vhost("red.blue.purple.com")
+
         self.config.enhance("red.blue.purple.com", "redirect")
         verify_no_redirect = ("certbot_apache.configurator."
                               "ApacheConfigurator._verify_no_certbot_redirect")
@@ -1224,7 +1252,7 @@ class MultipleVhostsTest(util.ApacheTest):
         # Skip the enable mod
         self.config.parser.modules.add("rewrite_module")
         self.config.get_version = mock.Mock(return_value=(2, 3, 9))
-
+        self.config.choose_vhost("red.blue.purple.com")
         self.config.enhance("red.blue.purple.com", "redirect")
         # Clear state about enabling redirect on this run
         # pylint: disable=protected-access
@@ -1446,6 +1474,7 @@ class MultipleVhostsTest(util.ApacheTest):
         # pylint: disable=protected-access
         self.config.parser.modules.add("mod_ssl.c")
         self.config.parser.modules.add("headers_module")
+        self.vh_truth[3].ssl = True
         self.config._wildcard_vhosts["*.certbot.demo"] = [self.vh_truth[3]]
         self.config.enhance("*.certbot.demo", "ensure-http-header",
                             "Upgrade-Insecure-Requests")
@@ -1453,6 +1482,7 @@ class MultipleVhostsTest(util.ApacheTest):
 
     @mock.patch("certbot_apache.configurator.ApacheConfigurator._choose_vhosts_wildcard")
     def test_enhance_wildcard_no_install(self, mock_choose):
+        self.vh_truth[3].ssl = True
         mock_choose.return_value = [self.vh_truth[3]]
         self.config.parser.modules.add("mod_ssl.c")
         self.config.parser.modules.add("headers_module")

certbot-apache/certbot_apache/tests/debian_test.py

@@ -161,6 +161,8 @@ class MultipleVhostsTestDebian(util.ApacheTest):
         self.config.parser.modules.add("mod_ssl.c")
         self.config.get_version = mock.Mock(return_value=(2, 4, 7))
         mock_exe.return_value = True
+        # This will create an ssl vhost for certbot.demo
+        self.config.choose_vhost("certbot.demo")
         self.config.enhance("certbot.demo", "staple-ocsp")
         self.assertTrue("socache_shmcb_module" in self.config.parser.modules)
 
@@ -172,6 +174,7 @@ class MultipleVhostsTestDebian(util.ApacheTest):
         mock_exe.return_value = True
 
         # This will create an ssl vhost for certbot.demo
+        self.config.choose_vhost("certbot.demo")
         self.config.enhance("certbot.demo", "ensure-http-header",
                             "Strict-Transport-Security")
         self.assertTrue("headers_module" in self.config.parser.modules)
@@ -183,6 +186,7 @@ class MultipleVhostsTestDebian(util.ApacheTest):
         mock_exe.return_value = True
         self.config.get_version = mock.Mock(return_value=(2, 2))
         # This will create an ssl vhost for certbot.demo
+        self.config.choose_vhost("certbot.demo")
         self.config.enhance("certbot.demo", "redirect")
         self.assertTrue("rewrite_module" in self.config.parser.modules)
 

certbot-apache/certbot_apache/tls_sni_01.py

@@ -123,7 +123,8 @@ class ApacheTlsSni01(common.TLSSNI01):
             self.configurator.config.tls_sni_01_port)))
 
         try:
-            vhost = self.configurator.choose_vhost(achall.domain, temp=True)
+            vhost = self.configurator.choose_vhost(achall.domain,
+                                                   create_if_no_ssl=False)
         except (PluginError, MissingCommandlineFlag):
             # We couldn't find the virtualhost for this domain, possibly
             # because it's a new vhost that's not configured yet