Merge pull request #777 from letsencrypt/fix_771

Make 'auth' and 'run' use the same code (fixes #771)

letsencrypt/cli.py

@@ -173,6 +173,7 @@ def _find_duplicative_certs(domains, config, renew_config):
     identical_names_cert, subset_names_cert = None, None
 
     configs_dir = renew_config.renewal_configs_dir
+    # Verify the directory is there
     le_util.make_or_verify_dir(configs_dir, mode=0o755, uid=os.geteuid())
 
     cli_config = configuration.RenewerConfiguration(config)
@@ -199,8 +200,106 @@ def _find_duplicative_certs(domains, config, renew_config):
     return identical_names_cert, subset_names_cert
 
 
+def _treat_as_renewal(config, domains):
+    """Determine whether or not the call should be treated as a renewal.
+
+    :returns: RenewableCert or None if renewal shouldn't occur.
+    :rtype: :class:`.storage.RenewableCert`
+
+    :raises .Error: If the user would like to rerun the client again.
+
+    """
+    renewal = False
+
+    # Considering the possibility that the requested certificate is
+    # related to an existing certificate.  (config.duplicate, which
+    # is set with --duplicate, skips all of this logic and forces any
+    # kind of certificate to be obtained with renewal = False.)
+    if not config.duplicate:
+        ident_names_cert, subset_names_cert = _find_duplicative_certs(
+            domains, config, configuration.RenewerConfiguration(config))
+        # I am not sure whether that correctly reads the systemwide
+        # configuration file.
+        question = None
+        if ident_names_cert is not None:
+            question = (
+                "You have an existing certificate that contains exactly the "
+                "same domains you requested (ref: {0}){br}{br}Do you want to "
+                "renew and replace this certificate with a newly-issued one?"
+            ).format(ident_names_cert.configfile.filename, br=os.linesep)
+        elif subset_names_cert is not None:
+            question = (
+                "You have an existing certificate that contains a portion of "
+                "the domains you requested (ref: {0}){br}{br}It contains these "
+                "names: {1}{br}{br}You requested these names for the new "
+                "certificate: {2}.{br}{br}Do you want to replace this existing "
+                "certificate with the new certificate?"
+            ).format(subset_names_cert.configfile.filename,
+                     ", ".join(subset_names_cert.names()),
+                     ", ".join(domains),
+                     br=os.linesep)
+        if question is None:
+            # We aren't in a duplicative-names situation at all, so we don't
+            # have to tell or ask the user anything about this.
+            pass
+        elif config.renew_by_default or zope.component.getUtility(
+                interfaces.IDisplay).yesno(question, "Replace", "Cancel"):
+            renewal = True
+        else:
+            reporter_util = zope.component.getUtility(interfaces.IReporter)
+            reporter_util.add_message(
+                "To obtain a new certificate that {0} an existing certificate "
+                "in its domain-name coverage, you must use the --duplicate "
+                "option.{br}{br}For example:{br}{br}{1} --duplicate {2}".format(
+                    "duplicates" if ident_names_cert is not None else
+                    "overlaps with",
+                    sys.argv[0], " ".join(sys.argv[1:]),
+                    br=os.linesep
+                ),
+                reporter_util.HIGH_PRIORITY)
+            raise errors.Error(
+                "User did not use proper CLI and would like "
+                "to reinvoke the client.")
+
+        if renewal:
+            return ident_names_cert if ident_names_cert is not None else subset_names_cert
+
+    return None
+
+
+def _auth_from_domains(le_client, config, domains, plugins):
+    """Authenticate and enroll certificate."""
+    # Note: This can raise errors... caught above us though.
+    lineage = _treat_as_renewal(config, domains)
+
+    if lineage is not None:
+        # TODO: schoen wishes to reuse key - discussion
+        # https://github.com/letsencrypt/letsencrypt/pull/777/files#r40498574
+        new_certr, new_chain, new_key, _ = le_client.obtain_certificate(domains)
+        # TODO: Check whether it worked! <- or make sure errors are thrown (jdk)
+        lineage.save_successor(
+            lineage.latest_common_version(), OpenSSL.crypto.dump_certificate(
+                OpenSSL.crypto.FILETYPE_PEM, new_certr.body),
+            new_key.pem, crypto_util.dump_pyopenssl_chain(new_chain))
+
+        lineage.update_all_links_to(lineage.latest_common_version())
+        # TODO: Check return value of save_successor
+        # TODO: Also update lineage renewal config with any relevant
+        #       configuration values from this attempt? <- Absolutely (jdkasten)
+    else:
+        # TREAT AS NEW REQUEST
+        lineage = le_client.obtain_and_enroll_certificate(domains, plugins)
+        if not lineage:
+            raise errors.Error("Certificate could not be obtained")
+
+    return lineage
+
+
+# TODO: Make run as close to auth + install as possible
+# Possible difficulties: args.csr was hacked into auth
 def run(args, config, plugins):  # pylint: disable=too-many-branches,too-many-locals
     """Obtain a certificate and install."""
+    # Begin authenticator and installer setup
     if args.configurator is not None and (args.installer is not None or
                                           args.authenticator is not None):
         return ("Either --configurator or --authenticator/--installer"
@@ -219,92 +318,28 @@ def run(args, config, plugins):  # pylint: disable=too-many-branches,too-many-lo
 
     if installer is None or authenticator is None:
         return "Configurator could not be determined"
+    # End authenticator and installer setup
 
     domains = _find_domains(args, installer)
 
-    treat_as_renewal = False
-
-    # Considering the possibility that the requested certificate is
-    # related to an existing certificate.  (config.duplicate, which
-    # is set with --duplicate, skips all of this logic and forces any
-    # kind of certificate to be obtained with treat_as_renewal = False.)
-    if not config.duplicate:
-        identical_names_cert, subset_names_cert = _find_duplicative_certs(
-            domains, config, configuration.RenewerConfiguration(config))
-        # I am not sure whether that correctly reads the systemwide
-        # configuration file.
-        question = None
-        if identical_names_cert is not None:
-            question = (
-                "You have an existing certificate that contains exactly the "
-                "same domains you requested (ref: {0})\n\nDo you want to "
-                "renew and replace this certificate with a newly-issued one?"
-            ).format(identical_names_cert.configfile.filename)
-        elif subset_names_cert is not None:
-            question = (
-                "You have an existing certificate that contains a portion of "
-                "the domains you requested (ref: {0})\n\nIt contains these "
-                "names: {1}\n\nYou requested these names for the new "
-                "certificate: {2}.\n\nDo you want to replace this existing "
-                "certificate with the new certificate?"
-            ).format(subset_names_cert.configfile.filename,
-                     ", ".join(subset_names_cert.names()),
-                     ", ".join(domains))
-        if question is None:
-            # We aren't in a duplicative-names situation at all, so we don't
-            # have to tell or ask the user anything about this.
-            pass
-        elif zope.component.getUtility(interfaces.IDisplay).yesno(
-                question, "Replace", "Cancel"):
-            treat_as_renewal = True
-        else:
-            reporter_util = zope.component.getUtility(interfaces.IReporter)
-            reporter_util.add_message(
-                "To obtain a new certificate that {0} an existing certificate "
-                "in its domain-name coverage, you must use the --duplicate "
-                "option.\n\nFor example:\n\n{1} --duplicate {2}".format(
-                    "duplicates" if identical_names_cert is not None else
-                    "overlaps with", sys.argv[0], " ".join(sys.argv[1:])),
-                reporter_util.HIGH_PRIORITY)
-            return 1
-
-    # Attempting to obtain the certificate
     # TODO: Handle errors from _init_le_client?
     le_client = _init_le_client(args, config, authenticator, installer)
-    if treat_as_renewal:
-        lineage = identical_names_cert if identical_names_cert is not None else subset_names_cert
-        # TODO: Use existing privkey instead of generating a new one
-        new_certr, new_chain, new_key, _ = le_client.obtain_certificate(domains)
-        # TODO: Check whether it worked!
-        lineage.save_successor(
-            lineage.latest_common_version(), OpenSSL.crypto.dump_certificate(
-                OpenSSL.crypto.FILETYPE_PEM, new_certr.body),
-            new_key.pem, crypto_util.dump_pyopenssl_chain(new_chain))
 
-        lineage.update_all_links_to(lineage.latest_common_version())
+    lineage = _auth_from_domains(le_client, config, domains, plugins)
-        # TODO: Check return value of save_successor
+
-        # TODO: Also update lineage renewal config with any relevant
+    # TODO: We also need to pass the fullchain (for Nginx)
-        #       configuration values from this attempt?
+    le_client.deploy_certificate(
-        le_client.deploy_certificate(
+        domains, lineage.privkey, lineage.cert, lineage.chain)
-            domains, lineage.privkey, lineage.cert, lineage.chain)
+    le_client.enhance_config(domains, args.redirect)
-        display_ops.success_renewal(domains)
+
-    else:
+    if len(lineage.available_versions("cert")) == 1:
-        # TREAT AS NEW REQUEST
-        lineage = le_client.obtain_and_enroll_certificate(
-            domains, authenticator, installer, plugins)
-        if not lineage:
-            return "Certificate could not be obtained"
-        # TODO: This treats the key as changed even when it wasn't
-        # TODO: We also need to pass the fullchain (for Nginx)
-        le_client.deploy_certificate(
-            domains, lineage.privkey, lineage.cert, lineage.chain)
-        le_client.enhance_config(domains, args.redirect)
         display_ops.success_installation(domains)
+    else:
+        display_ops.success_renewal(domains)
 
 
 def auth(args, config, plugins):
     """Authenticate & obtain cert, but do not install it."""
-    # XXX: Update for renewer / RenewableCert
 
     if args.domains is not None and args.csr is not None:
         # TODO: --csr could have a priority, when --domains is
@@ -324,6 +359,7 @@ def auth(args, config, plugins):
     # TODO: Handle errors from _init_le_client?
     le_client = _init_le_client(args, config, authenticator, installer)
 
+    # This is a special case; cert and chain are simply saved
     if args.csr is not None:
         certr, chain = le_client.obtain_certificate_from_csr(le_util.CSR(
             file=args.csr[0], data=args.csr[1], form="der"))
@@ -331,9 +367,7 @@ def auth(args, config, plugins):
             certr, chain, args.cert_path, args.chain_path)
     else:
         domains = _find_domains(args, installer)
-        if not le_client.obtain_and_enroll_certificate(
+        _auth_from_domains(le_client, config, domains, plugins)
-                domains, authenticator, installer, plugins):
-            return "Certificate could not be obtained"
 
 
 def install(args, config, plugins):
@@ -621,8 +655,9 @@ def create_parser(plugins, args):
         version="%(prog)s {0}".format(letsencrypt.__version__),
         help="show program's version number and exit")
     helpful.add(
-        "automation", "--no-confirm", dest="no_confirm", action="store_true",
+        "automation", "--renew-by-default", action="store_true",
-        help="Turn off confirmation screens, currently used for --revoke")
+        help="Select renewal by default when domains are a superset of a "
+             "a previously attained cert")
     helpful.add(
         "automation", "--agree-eula", dest="eula", action="store_true",
         help="Agree to the Let's Encrypt Developer Preview EULA")
@@ -832,7 +867,8 @@ def _handle_exception(exc_type, exc_value, trace, args):
 
     """
     logger.debug(
-        "Exiting abnormally:\n%s",
+        "Exiting abnormally:%s%s",
+        os.linesep,
         "".join(traceback.format_exception(exc_type, exc_value, trace)))
 
     if issubclass(exc_type, Exception) and (args is None or not args.debug):

letsencrypt/client.py

@@ -111,6 +111,8 @@ class Client(object):
     :ivar .AuthHandler auth_handler: Authorizations handler that will
         dispatch DV and Continuity challenges to appropriate
         authenticators (providing `.IAuthenticator` interface).
+    :ivar .IAuthenticator dv_auth: Prepared (`.IAuthenticator.prepare`)
+        authenticator that can solve the `.constants.DV_CHALLENGES`.
     :ivar .IInstaller installer: Installer.
     :ivar acme.client.Client acme: Optional ACME client API handle.
        You might already have one from `register`.
@@ -118,14 +120,10 @@ class Client(object):
     """
 
     def __init__(self, config, account_, dv_auth, installer, acme=None):
-        """Initialize a client.
+        """Initialize a client."""
-
-        :param .IAuthenticator dv_auth: Prepared (`.IAuthenticator.prepare`)
-            authenticator that can solve the `.constants.DV_CHALLENGES`.
-
-        """
         self.config = config
         self.account = account_
+        self.dv_auth = dv_auth
         self.installer = installer
 
         # Initialize ACME if account is provided
@@ -215,8 +213,7 @@ class Client(object):
 
         return self._obtain_certificate(domains, csr) + (key, csr)
 
-    def obtain_and_enroll_certificate(
+    def obtain_and_enroll_certificate(self, domains, plugins):
-            self, domains, authenticator, installer, plugins):
         """Obtain and enroll certificate.
 
         Get a new certificate for the specified domains using the specified
@@ -224,12 +221,6 @@ class Client(object):
         containing it.
 
         :param list domains: Domains to request.
-        :param authenticator: The authenticator to use.
-        :type authenticator: :class:`letsencrypt.interfaces.IAuthenticator`
-
-        :param installer: The installer to use.
-        :type installer: :class:`letsencrypt.interfaces.IInstaller`
-
         :param plugins: A PluginsFactory object.
 
         :returns: A new :class:`letsencrypt.storage.RenewableCert` instance
@@ -241,9 +232,10 @@ class Client(object):
 
         # TODO: remove this dirty hack
         self.config.namespace.authenticator = plugins.find_init(
-            authenticator).name
+            self.dv_auth).name
-        if installer is not None:
+        if self.installer is not None:
-            self.config.namespace.installer = plugins.find_init(installer).name
+            self.config.namespace.installer = plugins.find_init(
+                self.installer).name
 
         # XXX: We clearly need a more general and correct way of getting
         # options into the configobj for the RenewableCert instance.

letsencrypt/tests/cli_test.py

@@ -176,7 +176,8 @@ class DuplicativeCertsTest(renewer_test.BaseRenewableCertTest):
     def tearDown(self):
         shutil.rmtree(self.tempdir)
 
-    def test_find_duplicative_names(self):
+    @mock.patch("letsencrypt.le_util.make_or_verify_dir")
+    def test_find_duplicative_names(self, unused_makedir):
         from letsencrypt.cli import _find_duplicative_certs
         test_cert = test_util.load_vector("cert-san.pem")
         with open(self.test_rc.cert, "w") as f:
@@ -206,5 +207,5 @@ class DuplicativeCertsTest(renewer_test.BaseRenewableCertTest):
         self.assertEqual(result, (None, None))
 
 
-if __name__ == '__main__':
+if __name__ == "__main__":
     unittest.main()  # pragma: no cover

letsencrypt/tests/configuration_test.py

@@ -57,9 +57,9 @@ class RenewerConfigurationTest(unittest.TestCase):
 
     @mock.patch('letsencrypt.configuration.constants')
     def test_dynamic_dirs(self, constants):
-        constants.ARCHIVE_DIR = "a"
+        constants.ARCHIVE_DIR = 'a'
         constants.LIVE_DIR = 'l'
-        constants.RENEWAL_CONFIGS_DIR = "renewal_configs"
+        constants.RENEWAL_CONFIGS_DIR = 'renewal_configs'
         constants.RENEWER_CONFIG_FILENAME = 'r.conf'
 
         self.assertEqual(self.config.archive_dir, '/tmp/config/a')

letsencrypt/tests/crypto_util_test.py

@@ -6,7 +6,9 @@ import unittest
 
 import OpenSSL
 import mock
+import zope.component
 
+from letsencrypt import interfaces
 from letsencrypt.tests import test_util
 
 
@@ -20,6 +22,8 @@ class InitSaveKeyTest(unittest.TestCase):
     """Tests for letsencrypt.crypto_util.init_save_key."""
     def setUp(self):
         logging.disable(logging.CRITICAL)
+        zope.component.provideUtility(
+            mock.Mock(strict_permissions=True), interfaces.IConfig)
         self.key_dir = tempfile.mkdtemp('key_dir')
 
     def tearDown(self):
@@ -48,6 +52,8 @@ class InitSaveCSRTest(unittest.TestCase):
     """Tests for letsencrypt.crypto_util.init_save_csr."""
 
     def setUp(self):
+        zope.component.provideUtility(
+            mock.Mock(strict_permissions=True), interfaces.IConfig)
         self.csr_dir = tempfile.mkdtemp('csr_dir')
 
     def tearDown(self):

letsencrypt/tests/renewer_test.py

@@ -33,7 +33,12 @@ def fill_with_sample_data(rc_object):
 
 
 class BaseRenewableCertTest(unittest.TestCase):
+    """Base class for setting up Renewable Cert tests.
 
+    .. note:: It may be required to write out self.config for
+    your test.  Check :class:`.cli_test.DuplicateCertTest` for an example.
+
+    """
     def setUp(self):
         from letsencrypt import storage
         self.tempdir = tempfile.mkdtemp()

tests/integration/_common.sh

@@ -23,6 +23,7 @@ letsencrypt_test () {
         --agree-eula \
         --agree-tos \
         --email "" \
+        --renew-by-default \
         --debug \
         -vvvvvvv \
         "$@"