Command-line UX overhaul (#8852)

Streamline and reorganize Certbot's CLI output. This change is a substantial command-line UX overhaul, based on previous user research. The main goal was to streamline and clarify output. To see more verbose output, use the -v or -vv flags. --- * nginx,apache: CLI logging changes - Add "Successfully deployed ..." message using display_util - Remove IReporter usage and replace with display_util - Standardize "... could not find a VirtualHost ..." error This changes also bumps the version of certbot required by certbot-nginx and certbot-apache to take use of the new display_util function. * fix certbot_compatibility_test since the http plugins now require IDisplay, we need to inject it * fix dependency version on certbot * use better asserts * try fix oldest deps because certbot 1.10.0 depends on acme>=1.8.0, we need to use acme==1.8.0 in the -oldest tests * cli: redesign output of new certificate reporting Changes the output of run, certonly and certonly --csr. No longer uses IReporter. * cli: redesign output of failed authz reporting * fix problem sorting to be stable between py2 & 3 * add some catch-all error text * cli: dont use IReporter for EFF donation prompt * add per-authenticator hints * pass achalls to auth_hint, write some tests * exclude static auth hints from coverage * dont call auth_hint unless derived from .Plugin * dns fallback hint: dont assume --dns-blah works --dns-blah won't work for third-party plugins, they need to be specified using --authenticator dns-blah. * add code comments about the auth_hint interface * renew: don't restart the installer for dry-runs Prevents Certbot from superfluously invoking the installer restart during dry-run renewals. (This does not affect authenticator restarts). Additionally removes some CLI output that was reporting the fullchain path of the renewed certificate. * update CHANGELOG.md * cli: redesign output when cert installation failed - Display a message when certificate installation begins. - Don't use IReporter, just log errors immediately if restart/rollback fails. - Prompt the user with a command to retry the installation process once they have fixed any underlying problems. * vary by preconfigured_renewal and move expiry date to be above the renewal advice * update code comment Co-authored-by: ohemorange <ebportnoy@gmail.com> * update code comment Co-authored-by: ohemorange <ebportnoy@gmail.com> * fix lint * derve cert name from cert_path, if possible * fix type annotation * text change in nginx hint Co-authored-by: ohemorange <ebportnoy@gmail.com> * print message when restarting server after renewal * log: print "advice" when exiting with an error When running in non-quiet mode. * try fix -oldest lock_test.py * fix docstring * s/Restarting/Reloading/ when notifying the user * fix test name Co-authored-by: ohemorange <ebportnoy@gmail.com> * type annotations * s/using the {} plugin/installer: {}/ * copy: avoid "plugin" where possible * link to user guide#automated-renewals when not running with --preconfigured-renewal * cli: reduce default logging verbosity * fix lock_test: -vv is needed to see logger.debug * Change comment in log.py to match the change to default verbosity * Audit and adjust logging levels in apache module * Audit and adjust logging levels in nginx module * Audit, adjust logging levels, and improve logging calls in certbot module * Fix tests to mock correct methods and classes * typo in non-preconfigured-renewal message Co-authored-by: ohemorange <ebportnoy@gmail.com> * fix test * revert acme version bump * catch up to python3 changes * Revert "revert acme version bump" This reverts commit fa83d6a51c. * Change ocsp check error to warning since it's non-fatal * Update storage_test in parallel with last change * get rid of leading newline on "Deploying [...]" * shrink renewal and installation success messages * print logfile rather than logdir in exit handler * Decrease logging level to info for idempotent operation where enhancement is already set * Display cert not yet due for renewal message when renewing and no other action will be taken, and change cert to certificate * also write to logger so it goes in the log file * Don't double write to log file; fix main test * cli: remove trailing newline on new cert reporting * ignore type error * revert accidental changes to dependencies * Pass tests in any timezone by using utcfromtimestamp * Add changelog entry * fix nits * Improve wording of try again message * minor wording change to changelog * hooks: send hook stdout to CLI stdout includes both --manual and --{pre,post,renew} hooks * update docstrings and remove TODO * add a pending deprecation on execute_command * add test coverage for both * update deprecation text Co-authored-by: ohemorange <ebportnoy@gmail.com> Co-authored-by: Alex Zorin <alex@zorin.id.au> Co-authored-by: alexzorin <alex@zor.io>
2026-07-28 08:05:31 +02:00 · 2021-05-25 10:47:39 +10:00
parent 099c6c8b24
commit 6f27c32db1
55 changed files with 1010 additions and 386 deletions
@@ -25,6 +25,7 @@ from certbot import util
 from certbot.achallenges import KeyAuthorizationAnnotatedChallenge  # pylint: disable=unused-import
 from certbot.compat import filesystem
 from certbot.compat import os
+from certbot.display import util as display_util
 from certbot.plugins import common
 from certbot.plugins.enhancements import AutoHSTSEnhancement
 from certbot.plugins.util import path_surgery
@@ -515,6 +516,8 @@ class ApacheConfigurator(common.Installer):
        vhosts = self.choose_vhosts(domain)
        for vhost in vhosts:
            self._deploy_cert(vhost, cert_path, key_path, chain_path, fullchain_path)
+            display_util.notify("Successfully deployed certificate for {} to {}"
+                                .format(domain, vhost.filep))

    def choose_vhosts(self, domain, create_if_no_ssl=True):
        """
@@ -553,6 +556,19 @@ class ApacheConfigurator(common.Installer):

        return list(matched)

+    def _raise_no_suitable_vhost_error(self, target_name: str):
+        """
+        Notifies the user that Certbot could not find a vhost to secure
+        and raises an error.
+        :param str target_name: The server name that could not be mapped
+        :raises errors.PluginError: Raised unconditionally
+        """
+        raise errors.PluginError(
+            "Certbot could not find a VirtualHost for {0} in the Apache "
+            "configuration. Please create a VirtualHost with a ServerName "
+            "matching {0} and try again.".format(target_name)
+        )
+
    def _in_wildcard_scope(self, name, domain):
        """
        Helper method for _vhosts_for_wildcard() that makes sure that the domain
@@ -590,12 +606,7 @@ class ApacheConfigurator(common.Installer):
        dialog_output = display_ops.select_vhost_multiple(list(dialog_input))

        if not dialog_output:
-            logger.error(
-                "No vhost exists with servername or alias for domain %s. "
-                "No vhost was selected. Please specify ServerName or ServerAlias "
-                "in the Apache config.",
-                domain)
-            raise errors.PluginError("No vhost selected")
+            self._raise_no_suitable_vhost_error(domain)

        # Make sure we create SSL vhosts for the ones that are HTTP only
        # if requested.
@@ -719,12 +730,7 @@ class ApacheConfigurator(common.Installer):
        # Select a vhost from a list
        vhost = display_ops.select_vhost(target_name, self.vhosts)
        if vhost is None:
-            logger.error(
-                "No vhost exists with servername or alias of %s. "
-                "No vhost was selected. Please specify ServerName or ServerAlias "
-                "in the Apache config.",
-                target_name)
-            raise errors.PluginError("No vhost selected")
+            self._raise_no_suitable_vhost_error(target_name)
        if temp:
            return vhost
        if not vhost.ssl:
@@ -1532,12 +1538,11 @@ class ApacheConfigurator(common.Installer):
            raise errors.PluginError("Unable to write/read in make_vhost_ssl")

        if sift:
-            reporter = zope.component.getUtility(interfaces.IReporter)
-            reporter.add_message(
-                "Some rewrite rules copied from {0} were disabled in the "
-                "vhost for your HTTPS site located at {1} because they have "
-                "the potential to create redirection loops.".format(
-                    vhost.filep, ssl_fp), reporter.MEDIUM_PRIORITY)
+            display_util.notify(
+                f"Some rewrite rules copied from {vhost.filep} were disabled in the "
+                f"vhost for your HTTPS site located at {ssl_fp} because they have "
+                "the potential to create redirection loops."
+            )
        self.parser.aug.set("/augeas/files%s/mtime" % (self._escape(ssl_fp)), "0")
        self.parser.aug.set("/augeas/files%s/mtime" % (self._escape(vhost.filep)), "0")

@@ -1866,13 +1871,13 @@ class ApacheConfigurator(common.Installer):
            if options:
                msg_enhancement += ": " + options
            msg = msg_tmpl.format(domain, msg_enhancement)
-            logger.warning(msg)
+            logger.error(msg)
            raise errors.PluginError(msg)
        try:
            for vhost in vhosts:
                func(vhost, options)
        except errors.PluginError:
-            logger.warning("Failed %s for %s", enhancement, domain)
+            logger.error("Failed %s for %s", enhancement, domain)
            raise

    def _autohsts_increase(self, vhost, id_str, nextstep):
@@ -2436,7 +2441,7 @@ class ApacheConfigurator(common.Installer):
        try:
            util.run_script(self.options.restart_cmd)
        except errors.SubprocessError as err:
-            logger.info("Unable to restart apache using %s",
+            logger.warning("Unable to restart apache using %s",
                        self.options.restart_cmd)
            alt_restart = self.options.restart_cmd_alt
            if alt_restart:
@@ -2500,6 +2505,11 @@ class ApacheConfigurator(common.Installer):
                version=".".join(str(i) for i in self.version))
        )

+    def auth_hint(self, failed_achalls): # pragma: no cover
+        return ("The Certificate Authority failed to verify the temporary Apache configuration "
+                "changes made by Certbot. Ensure that the listed domains point to this Apache "
+                "server and that it is accessible from the internet.")
+
    ###########################################################################
    # Challenges Section
    ###########################################################################
@@ -2593,7 +2603,7 @@ class ApacheConfigurator(common.Installer):
                msg_tmpl = ("Certbot was not able to find SSL VirtualHost for a "
                            "domain {0} for enabling AutoHSTS enhancement.")
                msg = msg_tmpl.format(d)
-                logger.warning(msg)
+                logger.error(msg)
                raise errors.PluginError(msg)
            for vh in vhosts:
                try:
@@ -2679,7 +2689,7 @@ class ApacheConfigurator(common.Installer):
                except errors.PluginError:
                    msg = ("Could not find VirtualHost with ID {0}, disabling "
                           "AutoHSTS for this VirtualHost").format(id_str)
-                    logger.warning(msg)
+                    logger.error(msg)
                    # Remove the orphaned AutoHSTS entry from pluginstorage
                    self._autohsts.pop(id_str)
                    continue
@@ -2719,7 +2729,7 @@ class ApacheConfigurator(common.Installer):
                except errors.PluginError:
                    msg = ("VirtualHost with id {} was not found, unable to "
                           "make HSTS max-age permanent.").format(id_str)
-                    logger.warning(msg)
+                    logger.error(msg)
                    self._autohsts.pop(id_str)
                    continue
                if self._autohsts_vhost_in_lineage(vhost, lineage):
@@ -119,7 +119,7 @@ def _vhost_menu(domain, vhosts):
            "guidance in non-interactive mode. Certbot may need "
            "vhosts to be explicitly labelled with ServerName or "
            "ServerAlias directives.".format(domain))
-        logger.warning(msg)
+        logger.error(msg)
        raise errors.MissingCommandlineFlag(msg)

    return code, tag
@@ -58,7 +58,7 @@ class DebianConfigurator(configurator.ApacheConfigurator):
                # Already in shape
                vhost.enabled = True
                return None
-            logger.warning(
+            logger.error(
                "Could not symlink %s to %s, got error: %s", enabled_path,
                vhost.filep, err.strerror)
            errstring = ("Encountered error while trying to enable a " +