From d5849c3416087efeefa3fd6f8f71aeac0229cc56 Mon Sep 17 00:00:00 2001
From: Seth Schoen <schoen@eff.org>
Date: Wed, 9 Dec 2015 10:45:28 -0800
Subject: [PATCH 01/31] WIP on cli.py

---
 letsencrypt/cli.py | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index 348818368..457661d1b 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -219,6 +219,17 @@ def _treat_as_renewal(config, domains):
     :raises .Error: If the user would like to rerun the client again.
 
     """
+    # TODO: This now needs to return 3 different cases plus the .Error
+    #       case: reinstall, renew, fresh cert
+    #       Probably the return value should be a tuple of (result, cert)
+    #       like ("reinstall", RenewableCert_instance)
+    #       ("renew", RenewableCert_instance)
+    #       ("newcert", None)
+    #       We could also return ("cancel", None) instead of raising the
+    #       error, but the error-handling mechanism seems to work
+    # TODO: Find out whether a RenewableCert instance is enough information
+    #       for the installer to try to reinstall it when we return "reinstall"
+    # TODO: Also address superset case
     renewal = False
 
     # Considering the possibility that the requested certificate is
@@ -234,9 +245,21 @@ def _treat_as_renewal(config, domains):
         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?"
+                "same domains you requested (ref: {0}){br}{br}Note that the "
+                "Let's Encrypt certificate authority limits the number of "
+                "certificates that can be issued for the same domain name per "
+                "week!{br}{br}Do you want to reinstall this existing "
+                "certificate, or renew and replace this certificate with a "
+                "newly-issued one?"
             ).format(ident_names_cert.configfile.filename, br=os.linesep)
+            print(zope.component.getUtility(interfaces.IDisplay).menu(
+                question, ["Attempt to reinstall this existing certificate",
+                           "Obtain a new certificate for these domains",
+                           "Cancel this operation and do nothing"],
+                "OK", "Cancel"))
+            # TODO: Analyze the result and make a code path that does the
+            #       right thing with it
+            sys.exit(1)
         elif subset_names_cert is not None:
             question = (
                 "You have an existing certificate that contains a portion of "

From 79c61a80987fd9711324bb536f359ca90c38cdda Mon Sep 17 00:00:00 2001
From: Seth Schoen <schoen@eff.org>
Date: Wed, 9 Dec 2015 16:37:58 -0800
Subject: [PATCH 02/31] Basic updated logic for #1546 behavior

---
 letsencrypt/cli.py | 82 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 58 insertions(+), 24 deletions(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index 457661d1b..c24f870f5 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -219,16 +219,14 @@ def _treat_as_renewal(config, domains):
     :raises .Error: If the user would like to rerun the client again.
 
     """
-    # TODO: This now needs to return 3 different cases plus the .Error
-    #       case: reinstall, renew, fresh cert
-    #       Probably the return value should be a tuple of (result, cert)
-    #       like ("reinstall", RenewableCert_instance)
-    #       ("renew", RenewableCert_instance)
-    #       ("newcert", None)
+    #       This will now instead return 3 different cases plus the .Error
+    #       case (which raises an exception): reinstall, renew, newcert
+    #       The return value will be a tuple of (result, cert), viz.
+    #       either ("reinstall", RenewableCert_instance)
+    #       or     ("renew", RenewableCert_instance)
+    #       or     ("newcert", None)
     #       We could also return ("cancel", None) instead of raising the
-    #       error, but the error-handling mechanism seems to work
-    # TODO: Find out whether a RenewableCert instance is enough information
-    #       for the installer to try to reinstall it when we return "reinstall"
+    #       error, but the error-handling mechanism seems to work OK.
     # TODO: Also address superset case
     renewal = False
 
@@ -243,23 +241,48 @@ def _treat_as_renewal(config, domains):
         # configuration file.
         question = None
         if ident_names_cert is not None:
+            # TODO: I bet this question is confusing to people who don't know
+            #       how the backend repreentation of certificates work. The
+            #       distinction is renewal updates existing lineage with new
+            #       cert (eventually maybe preserving the privkey), while
+            #       newcert creates a new lineage. And reinstall doesn't cause
+            #       a new issuance at all.
             question = (
                 "You have an existing certificate that contains exactly the "
                 "same domains you requested (ref: {0}){br}{br}Note that the "
                 "Let's Encrypt certificate authority limits the number of "
                 "certificates that can be issued for the same domain name per "
-                "week!{br}{br}Do you want to reinstall this existing "
-                "certificate, or renew and replace this certificate with a "
-                "newly-issued one?"
+                "week, including renewal certificates!{br}{br}Do you want to "
+                "reinstall this existing certificate, renew and replace this "
+                "certificate with a newly-issued one, or get a completely new "
+                "certificate?"
             ).format(ident_names_cert.configfile.filename, br=os.linesep)
-            print(zope.component.getUtility(interfaces.IDisplay).menu(
+            response = zope.component.getUtility(interfaces.IDisplay).menu(
                 question, ["Attempt to reinstall this existing certificate",
-                           "Obtain a new certificate for these domains",
+                           "Renew this certificate, replacing it with the updated one",
+                           "Obtain a completely new certificate for these domains",
                            "Cancel this operation and do nothing"],
-                "OK", "Cancel"))
-            # TODO: Analyze the result and make a code path that does the
-            #       right thing with it
-            sys.exit(1)
+                "OK", "Cancel")
+            if response[0] == "cancel" or response[1] == 3:
+                # TODO: Add notification related to command-line options for
+                #       skipping the menu for this case.
+                raise errors.Error(
+                    "User did not use proper CLI and would like "
+                    "to reinvoke the client.")
+            elif response[1] == 0:
+                # Reinstall
+                return "reinstall", ident_names_cert
+            elif response[1] == 1:
+                # Renew
+                return "renew", ident_names_cert
+            elif response[1] == 2:
+                # New cert
+                return "newcert", None
+            else:
+                assert 0
+                # NOTREACHED
+        # TODO: Since the rest of the function deals only with the subset
+        #       case, we could now simplify it considerably!
         elif subset_names_cert is not None:
             question = (
                 "You have an existing certificate that contains a portion of "
@@ -295,9 +318,9 @@ def _treat_as_renewal(config, domains):
                 "to reinvoke the client.")
 
         if renewal:
-            return ident_names_cert if ident_names_cert is not None else subset_names_cert
+            return "renew", ident_names_cert if ident_names_cert is not None else subset_names_cert
 
-    return None
+    return "newcert", None
 
 
 def _report_new_cert(cert_path, fullchain_path):
@@ -337,10 +360,20 @@ def _suggest_donate():
 
 def _auth_from_domains(le_client, config, domains):
     """Authenticate and enroll certificate."""
-    # Note: This can raise errors... caught above us though.
-    lineage = _treat_as_renewal(config, domains)
+    # Note: This can raise errors... caught above us though. This is now
+    # a three-way case: reinstall (which results in a no-op here because
+    # although there is a relevant lineage, we don't do anything to it
+    # inside this function -- we don't obtain a new certificate), renew
+    # (which results in treating the request as a renewal), or newcert
+    # (which results in treating the request as a new certificate request).
 
-    if lineage is not None:
+    action, lineage = _treat_as_renewal(config, domains)
+    print action, lineage
+    if action == "reinstall":
+        # The lineage already exists; allow the caller to try installing
+        # it without getting a new certificate at all.
+        return lineage
+    elif action == "renew":
         # 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)
@@ -354,7 +387,7 @@ def _auth_from_domains(le_client, config, domains):
         # TODO: Check return value of save_successor
         # TODO: Also update lineage renewal config with any relevant
         #       configuration values from this attempt? <- Absolutely (jdkasten)
-    else:
+    elif action == "newcert":
         # TREAT AS NEW REQUEST
         lineage = le_client.obtain_and_enroll_certificate(domains)
         if not lineage:
@@ -508,6 +541,7 @@ def run(args, config, plugins):  # pylint: disable=too-many-branches,too-many-lo
 
 def obtain_cert(args, config, plugins):
     """Authenticate & obtain cert, but do not install it."""
+    # TODO: Is this now dead code? What calls it?
 
     if args.domains and args.csr is not None:
         # TODO: --csr could have a priority, when --domains is

From ae0f35d48da3a86ae0f264588ffe66495ad1c484 Mon Sep 17 00:00:00 2001
From: Seth Schoen <schoen@eff.org>
Date: Thu, 10 Dec 2015 15:23:10 -0800
Subject: [PATCH 03/31] Taking "newcert" option out of the menu

---
 letsencrypt/cli.py | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index c24f870f5..b1d3f3d87 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -227,6 +227,12 @@ def _treat_as_renewal(config, domains):
     #       or     ("newcert", None)
     #       We could also return ("cancel", None) instead of raising the
     #       error, but the error-handling mechanism seems to work OK.
+    #       Note that the "newcert" option is not suggested in the UI menu
+    #       when the requested cert has precisely the same names as an
+    #       existing cert (it's considered an "advanced" option in this
+    #       situation, so it would have to be selected with a command-line
+    #       flag).
+    #
     # TODO: Also address superset case
     renewal = False
 
@@ -250,20 +256,17 @@ def _treat_as_renewal(config, domains):
             question = (
                 "You have an existing certificate that contains exactly the "
                 "same domains you requested (ref: {0}){br}{br}Note that the "
-                "Let's Encrypt certificate authority limits the number of "
-                "certificates that can be issued for the same domain name per "
-                "week, including renewal certificates!{br}{br}Do you want to "
-                "reinstall this existing certificate, renew and replace this "
-                "certificate with a newly-issued one, or get a completely new "
-                "certificate?"
+                "Let's Encrypt CA limits how many certificates can be issued "
+                "for the same domain name per week, including renewal "
+                "certificates!{br}{br}What would you like to do?"
             ).format(ident_names_cert.configfile.filename, br=os.linesep)
             response = zope.component.getUtility(interfaces.IDisplay).menu(
                 question, ["Attempt to reinstall this existing certificate",
                            "Renew this certificate, replacing it with the updated one",
-                           "Obtain a completely new certificate for these domains",
+#                           "Obtain a completely new certificate for these domains",
                            "Cancel this operation and do nothing"],
                 "OK", "Cancel")
-            if response[0] == "cancel" or response[1] == 3:
+            if response[0] == "cancel" or response[1] == 2:
                 # TODO: Add notification related to command-line options for
                 #       skipping the menu for this case.
                 raise errors.Error(
@@ -275,9 +278,9 @@ def _treat_as_renewal(config, domains):
             elif response[1] == 1:
                 # Renew
                 return "renew", ident_names_cert
-            elif response[1] == 2:
-                # New cert
-                return "newcert", None
+#            elif response[1] == 2:
+#                # New cert
+#                return "newcert", None
             else:
                 assert 0
                 # NOTREACHED

From 748a7fe2bcca45906085b85280bbcf678f87d27c Mon Sep 17 00:00:00 2001
From: Seth Schoen <schoen@eff.org>
Date: Thu, 10 Dec 2015 15:41:27 -0800
Subject: [PATCH 04/31] Partially update documentation for _treat_as_renewal

---
 letsencrypt/cli.py | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index b1d3f3d87..ecee15c2e 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -211,10 +211,11 @@ def _find_duplicative_certs(config, domains):
 
 
 def _treat_as_renewal(config, domains):
-    """Determine whether or not the call should be treated as a renewal.
+    """Determine whether there are duplicated names and how to handle them.
 
-    :returns: RenewableCert or None if renewal shouldn't occur.
-    :rtype: :class:`.storage.RenewableCert`
+    :returns: Two-element tuple containing desired new-certificate
+              behavior as a string token, plus either a RenewableCert
+              instance or None if renewal shouldn't occur.
 
     :raises .Error: If the user would like to rerun the client again.
 

From 95d01130988c88b7f13e9a8f437ec317ef536077 Mon Sep 17 00:00:00 2001
From: Seth Schoen <schoen@eff.org>
Date: Thu, 10 Dec 2015 15:49:33 -0800
Subject: [PATCH 05/31] Make --renew-by-default apply in exact-duplicate case

---
 letsencrypt/cli.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index ecee15c2e..c3a8f93b2 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -254,6 +254,8 @@ def _treat_as_renewal(config, domains):
             #       cert (eventually maybe preserving the privkey), while
             #       newcert creates a new lineage. And reinstall doesn't cause
             #       a new issuance at all.
+            if config.renew_by_default:
+                return "renew", ident_names_cert
             question = (
                 "You have an existing certificate that contains exactly the "
                 "same domains you requested (ref: {0}){br}{br}Note that the "

From 859fcea4ece78af66d248117569724c6d89cdb88 Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Thu, 10 Dec 2015 19:08:08 -0800
Subject: [PATCH 06/31] Try to minimise verbiage.

---
 letsencrypt/cli.py | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index c3a8f93b2..86910acc7 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -258,14 +258,11 @@ def _treat_as_renewal(config, domains):
                 return "renew", ident_names_cert
             question = (
                 "You have an existing certificate that contains exactly the "
-                "same domains you requested (ref: {0}){br}{br}Note that the "
-                "Let's Encrypt CA limits how many certificates can be issued "
-                "for the same domain name per week, including renewal "
-                "certificates!{br}{br}What would you like to do?"
+                "same domains you requested.{br}(ref: {0}){br}{br}What would you like to do?"
             ).format(ident_names_cert.configfile.filename, br=os.linesep)
             response = zope.component.getUtility(interfaces.IDisplay).menu(
                 question, ["Attempt to reinstall this existing certificate",
-                           "Renew this certificate, replacing it with the updated one",
+                           "Renew & replace the cert (limit 5 per 7 days)",
 #                           "Obtain a completely new certificate for these domains",
                            "Cancel this operation and do nothing"],
                 "OK", "Cancel")

From d3f5df5b023793183b3150e297d73ed0d94cb576 Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Thu, 10 Dec 2015 19:12:29 -0800
Subject: [PATCH 07/31] Hedge on the rate limit slightly, because it may change
 after a release

---
 letsencrypt/cli.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index 86910acc7..3d24c7a06 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -262,7 +262,7 @@ def _treat_as_renewal(config, domains):
             ).format(ident_names_cert.configfile.filename, br=os.linesep)
             response = zope.component.getUtility(interfaces.IDisplay).menu(
                 question, ["Attempt to reinstall this existing certificate",
-                           "Renew & replace the cert (limit 5 per 7 days)",
+                           "Renew & replace the cert (limit ~5 per 7 days)",
 #                           "Obtain a completely new certificate for these domains",
                            "Cancel this operation and do nothing"],
                 "OK", "Cancel")

From 090ada2aca22d383db1255362aecf2792823e810 Mon Sep 17 00:00:00 2001
From: Noah Swartz <noah@eff.org>
Date: Fri, 11 Dec 2015 15:46:35 -0800
Subject: [PATCH 08/31] fix issue with mocked _treat_as_renewal code

---
 letsencrypt/tests/cli_test.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py
index 462d37a87..433671b38 100644
--- a/letsencrypt/tests/cli_test.py
+++ b/letsencrypt/tests/cli_test.py
@@ -389,7 +389,7 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
 
     def _certonly_new_request_common(self, mock_client):
         with mock.patch('letsencrypt.cli._treat_as_renewal') as mock_renewal:
-            mock_renewal.return_value = None
+            mock_renewal.return_value = ("newcert", None)
             with mock.patch('letsencrypt.cli._init_le_client') as mock_init:
                 mock_init.return_value = mock_client
                 self._call(['-d', 'foo.bar', '-a', 'standalone', 'certonly'])
@@ -405,7 +405,7 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
         mock_lineage = mock.MagicMock(cert=cert_path, fullchain=chain_path)
         mock_cert = mock.MagicMock(body='body')
         mock_key = mock.MagicMock(pem='pem_key')
-        mock_renewal.return_value = mock_lineage
+        mock_renewal.return_value = ("renew", mock_lineage)
         mock_client = mock.MagicMock()
         mock_client.obtain_certificate.return_value = (mock_cert, 'chain',
                                                        mock_key, 'csr')

From 8fdff540b5ab46fc1d6fd66f54c209eecb8ac6eb Mon Sep 17 00:00:00 2001
From: Seth Schoen <schoen@eff.org>
Date: Fri, 11 Dec 2015 22:02:46 -0800
Subject: [PATCH 09/31] Add interactive flag to should_auto{renew,deploy}

---
 letsencrypt/storage.py | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/letsencrypt/storage.py b/letsencrypt/storage.py
index 7e2802b14..f457fe13e 100644
--- a/letsencrypt/storage.py
+++ b/letsencrypt/storage.py
@@ -471,7 +471,7 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
         return ("autodeploy" not in self.configuration or
                 self.configuration.as_bool("autodeploy"))
 
-    def should_autodeploy(self):
+    def should_autodeploy(self, interactive=False):
         """Should this lineage now automatically deploy a newer version?
 
         This is a policy question and does not only depend on whether
@@ -480,12 +480,16 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
         exists, and whether the time interval for autodeployment has
         been reached.)
 
+        :param bool interactive: set to True to examine the question
+            regardless of whether the renewal configuration allows
+            automated deployment (for interactive use). Default False.
+
         :returns: whether the lineage now ought to autodeploy an
             existing newer cert version
         :rtype: bool
 
         """
-        if self.autodeployment_is_enabled():
+        if interactive or self.autodeployment_is_enabled():
             if self.has_pending_deployment():
                 interval = self.configuration.get("deploy_before_expiry",
                                                   "5 days")
@@ -529,7 +533,7 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
         return ("autorenew" not in self.configuration or
                 self.configuration.as_bool("autorenew"))
 
-    def should_autorenew(self):
+    def should_autorenew(self, interactive=False):
         """Should we now try to autorenew the most recent cert version?
 
         This is a policy question and does not only depend on whether
@@ -540,12 +544,16 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
         Note that this examines the numerically most recent cert version,
         not the currently deployed version.
 
+        :param bool interactive: set to True to examine the question
+            regardless of whether the renewal configuration allows
+            automated renewal (for interactive use). Default False.
+
         :returns: whether an attempt should now be made to autorenew the
             most current cert version in this lineage
         :rtype: bool
 
         """
-        if self.autorenewal_is_enabled():
+        if interactive or self.autorenewal_is_enabled():
             # Consider whether to attempt to autorenew this cert now
 
             # Renewals on the basis of revocation

From 621bef35c966a7bb912d8b83c173a03dbce656b7 Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Sat, 12 Dec 2015 10:11:28 -0800
Subject: [PATCH 10/31] Close to expiry, default to renewal

- Also move the identical-cert-names case into its own function
---
 letsencrypt/cli.py | 69 ++++++++++++++++++++++------------------------
 1 file changed, 33 insertions(+), 36 deletions(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index 3d24c7a06..135bb7255 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -248,42 +248,7 @@ def _treat_as_renewal(config, domains):
         # configuration file.
         question = None
         if ident_names_cert is not None:
-            # TODO: I bet this question is confusing to people who don't know
-            #       how the backend repreentation of certificates work. The
-            #       distinction is renewal updates existing lineage with new
-            #       cert (eventually maybe preserving the privkey), while
-            #       newcert creates a new lineage. And reinstall doesn't cause
-            #       a new issuance at all.
-            if config.renew_by_default:
-                return "renew", ident_names_cert
-            question = (
-                "You have an existing certificate that contains exactly the "
-                "same domains you requested.{br}(ref: {0}){br}{br}What would you like to do?"
-            ).format(ident_names_cert.configfile.filename, br=os.linesep)
-            response = zope.component.getUtility(interfaces.IDisplay).menu(
-                question, ["Attempt to reinstall this existing certificate",
-                           "Renew & replace the cert (limit ~5 per 7 days)",
-#                           "Obtain a completely new certificate for these domains",
-                           "Cancel this operation and do nothing"],
-                "OK", "Cancel")
-            if response[0] == "cancel" or response[1] == 2:
-                # TODO: Add notification related to command-line options for
-                #       skipping the menu for this case.
-                raise errors.Error(
-                    "User did not use proper CLI and would like "
-                    "to reinvoke the client.")
-            elif response[1] == 0:
-                # Reinstall
-                return "reinstall", ident_names_cert
-            elif response[1] == 1:
-                # Renew
-                return "renew", ident_names_cert
-#            elif response[1] == 2:
-#                # New cert
-#                return "newcert", None
-            else:
-                assert 0
-                # NOTREACHED
+            return _handle_identical_cert_request(ident_names_cert)
         # TODO: Since the rest of the function deals only with the subset
         #       case, we could now simplify it considerably!
         elif subset_names_cert is not None:
@@ -325,6 +290,38 @@ def _treat_as_renewal(config, domains):
 
     return "newcert", None
 
+def _handle_identical_cert_request(cert):
+    """Figure out what to do if a cert has the same names as a perviously obtained one
+
+    :param storage.RenewableCert cert:
+
+    :returns: Tuple of (string, cert_or_None) as per _treat_as_renewal
+    """
+    if config.renew_by_default or cert.should_autorenew(interactive=True):
+        return "renew", cert
+    display = zope.component.getUtility(interfaces.IDisplay)
+    question = (
+        "You have an existing certificate that contains exactly the same "
+        "domains you requested.{br}(ref: {0}){br}{br}What would you like to do?"
+    ).format(cert.configfile.filename, br=os.linesep)
+    response = display.menu(
+        question, ["Attempt to reinstall this existing certificate",
+                   "Renew & replace the cert (limit ~5 per 7 days)",
+                   "Cancel this operation and do nothing"],
+        "OK", "Cancel")
+    if response[0] == "cancel" or response[1] == 2:
+        # TODO: Add notification related to command-line options for
+        #       skipping the menu for this case.
+        raise errors.Error(
+            "User did not use proper CLI and would like "
+            "to reinvoke the client.")
+    elif response[1] == 0:
+        return "reinstall", cert
+    elif response[1] == 1:
+        return "renew", cert
+    else:
+        assert False, "This is impossible"
+
 
 def _report_new_cert(cert_path, fullchain_path):
     """Reports the creation of a new certificate to the user.

From 6351194fc2936bad1a1e2a031831ae900e5250d3 Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Sat, 12 Dec 2015 10:20:34 -0800
Subject: [PATCH 11/31] Simplify construction of the "renew" case

---
 letsencrypt/cli.py | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index 135bb7255..8aac90c6d 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -235,7 +235,6 @@ def _treat_as_renewal(config, domains):
     #       flag).
     #
     # TODO: Also address superset case
-    renewal = False
 
     # Considering the possibility that the requested certificate is
     # related to an existing certificate.  (config.duplicate, which
@@ -249,8 +248,6 @@ def _treat_as_renewal(config, domains):
         question = None
         if ident_names_cert is not None:
             return _handle_identical_cert_request(ident_names_cert)
-        # TODO: Since the rest of the function deals only with the subset
-        #       case, we could now simplify it considerably!
         elif subset_names_cert is not None:
             question = (
                 "You have an existing certificate that contains a portion of "
@@ -268,7 +265,7 @@ def _treat_as_renewal(config, domains):
             pass
         elif config.renew_by_default or zope.component.getUtility(
                 interfaces.IDisplay).yesno(question, "Replace", "Cancel"):
-            renewal = True
+            return "renew", subset_names_cert
         else:
             reporter_util = zope.component.getUtility(interfaces.IReporter)
             reporter_util.add_message(
@@ -285,9 +282,6 @@ def _treat_as_renewal(config, domains):
                 "User did not use proper CLI and would like "
                 "to reinvoke the client.")
 
-        if renewal:
-            return "renew", ident_names_cert if ident_names_cert is not None else subset_names_cert
-
     return "newcert", None
 
 def _handle_identical_cert_request(cert):

From 439a2d05eb89ff91441a05a6cec122c3b4dc10ab Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Sat, 12 Dec 2015 10:37:11 -0800
Subject: [PATCH 12/31] Simplifications:

 - Handle the base "newcert" cases at the top, rather than with a very long if
   statement
 - Avoid having a state-tracking "question variable"
---
 letsencrypt/cli.py | 48 +++++++++++++++++++++-------------------------
 1 file changed, 22 insertions(+), 26 deletions(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index 8aac90c6d..acba13441 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -240,30 +240,28 @@ def _treat_as_renewal(config, domains):
     # 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(
-            config, domains)
-        # I am not sure whether that correctly reads the systemwide
-        # configuration file.
-        question = None
-        if ident_names_cert is not None:
-            return _handle_identical_cert_request(ident_names_cert)
-        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(
+    if config.duplicate:
+        return "newcert", None
+    ident_names_cert, subset_names_cert = _find_duplicative_certs(config, domains)
+    # I am not sure whether that correctly reads the systemwide
+    # configuration file.
+    if not (ident_names_cert or subset_names_cert):
+        return "newcert", None
+
+    if ident_names_cert is not None:
+        return _handle_identical_cert_request(ident_names_cert)
+    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 config.renew_by_default or zope.component.getUtility(
                 interfaces.IDisplay).yesno(question, "Replace", "Cancel"):
             return "renew", subset_names_cert
         else:
@@ -282,8 +280,6 @@ def _treat_as_renewal(config, domains):
                 "User did not use proper CLI and would like "
                 "to reinvoke the client.")
 
-    return "newcert", None
-
 def _handle_identical_cert_request(cert):
     """Figure out what to do if a cert has the same names as a perviously obtained one
 

From f9e7d880bf8501703f8288a9eaded26abc2d0465 Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Sat, 12 Dec 2015 11:00:38 -0800
Subject: [PATCH 13/31] Remove transient commentary

---
 letsencrypt/cli.py | 97 ++++++++++++++++++++++------------------------
 1 file changed, 47 insertions(+), 50 deletions(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index acba13441..f88a053c8 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -213,79 +213,38 @@ def _find_duplicative_certs(config, domains):
 def _treat_as_renewal(config, domains):
     """Determine whether there are duplicated names and how to handle them.
 
-    :returns: Two-element tuple containing desired new-certificate
-              behavior as a string token, plus either a RenewableCert
-              instance or None if renewal shouldn't occur.
+    :returns: Two-element tuple containing desired new-certificate behavior as
+              a string token ("reinstall", "renew", or "newcert), plus either a
+              RenewableCert instance or None if renewal shouldn't occur.
 
     :raises .Error: If the user would like to rerun the client again.
 
     """
-    #       This will now instead return 3 different cases plus the .Error
-    #       case (which raises an exception): reinstall, renew, newcert
-    #       The return value will be a tuple of (result, cert), viz.
-    #       either ("reinstall", RenewableCert_instance)
-    #       or     ("renew", RenewableCert_instance)
-    #       or     ("newcert", None)
-    #       We could also return ("cancel", None) instead of raising the
-    #       error, but the error-handling mechanism seems to work OK.
-    #       Note that the "newcert" option is not suggested in the UI menu
-    #       when the requested cert has precisely the same names as an
-    #       existing cert (it's considered an "advanced" option in this
-    #       situation, so it would have to be selected with a command-line
-    #       flag).
-    #
-    # TODO: Also address superset case
-
     # 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 config.duplicate:
         return "newcert", None
+    # TODO: Also address superset case
     ident_names_cert, subset_names_cert = _find_duplicative_certs(config, domains)
-    # I am not sure whether that correctly reads the systemwide
+    # XXX ^ schoen is not sure whether that correctly reads the systemwide
     # configuration file.
     if not (ident_names_cert or subset_names_cert):
         return "newcert", None
 
     if ident_names_cert is not None:
-        return _handle_identical_cert_request(ident_names_cert)
+        return _handle_identical_cert_request(config, ident_names_cert)
     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 config.renew_by_default or zope.component.getUtility(
-                interfaces.IDisplay).yesno(question, "Replace", "Cancel"):
-            return "renew", subset_names_cert
-        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.")
+        return _handle_subset_cert_request(config, domains, subset_names_cert)
 
-def _handle_identical_cert_request(cert):
+def _handle_identical_cert_request(config, cert):
     """Figure out what to do if a cert has the same names as a perviously obtained one
 
     :param storage.RenewableCert cert:
 
     :returns: Tuple of (string, cert_or_None) as per _treat_as_renewal
+
     """
     if config.renew_by_default or cert.should_autorenew(interactive=True):
         return "renew", cert
@@ -312,6 +271,44 @@ def _handle_identical_cert_request(cert):
     else:
         assert False, "This is impossible"
 
+def _handle_subset_cert_request(config, domains, cert):
+    """Figure out what to do if a previous cert had a subset of the names now requested
+
+    :param storage.RenewableCert cert:
+
+    :returns: Tuple of (string, cert_or_None) as per _treat_as_renewal
+
+    """
+    existing = ", ".join(cert.names())
+    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(cert.configfile.filename,
+             existing,
+             ", ".join(domains),
+             br=os.linesep)
+    if config.renew_by_default or zope.component.getUtility(
+            interfaces.IDisplay).yesno(question, "Replace", "Cancel"):
+        return "renew", cert
+    else:
+        reporter_util = zope.component.getUtility(interfaces.IReporter)
+        reporter_util.add_message(
+            "To obtain a new certificate that contains these names without "
+            "replacing your existing certificate for {0}, you must use the "
+            "--duplicate option.{br}{br}"
+            "For example:{br}{br}{1} --duplicate {2}".format(
+                existing,
+                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.")
+
 
 def _report_new_cert(cert_path, fullchain_path):
     """Reports the creation of a new certificate to the user.

From 2b3f217ce2d44c55ce46579604dbdbc2fb6101b1 Mon Sep 17 00:00:00 2001
From: Seth Schoen <schoen@eff.org>
Date: Sat, 12 Dec 2015 11:03:19 -0800
Subject: [PATCH 14/31] Attempt to fix #1856

---
 letsencrypt/cli.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index f88a053c8..bebbd24ad 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -205,7 +205,12 @@ def _find_duplicative_certs(config, domains):
         if candidate_names == set(domains):
             identical_names_cert = candidate_lineage
         elif candidate_names.issubset(set(domains)):
-            subset_names_cert = candidate_lineage
+            # This logic finds and returns the largest subset-names cert
+            # in the case where there are several available.
+            if subset_names_cert is None:
+                subset_names_cert = candidate_lineage
+            elif len(candidate_names) > len(subset_names_cert.names()):
+                subset_names_cert = candidate_lineage
 
     return identical_names_cert, subset_names_cert
 

From dfbf572bc418393ae0bd5cbe6ad7b85f13b7ad52 Mon Sep 17 00:00:00 2001
From: Seth Schoen <schoen@eff.org>
Date: Sat, 12 Dec 2015 11:06:20 -0800
Subject: [PATCH 15/31] Add missing quotation mark in documentation

---
 letsencrypt/cli.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index bebbd24ad..6c0209660 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -219,8 +219,8 @@ def _treat_as_renewal(config, domains):
     """Determine whether there are duplicated names and how to handle them.
 
     :returns: Two-element tuple containing desired new-certificate behavior as
-              a string token ("reinstall", "renew", or "newcert), plus either a
-              RenewableCert instance or None if renewal shouldn't occur.
+              a string token ("reinstall", "renew", or "newcert"), plus either
+              a RenewableCert instance or None if renewal shouldn't occur.
 
     :raises .Error: If the user would like to rerun the client again.
 

From d92a32b9d7156f071a74d8fc479330e1e0430a9a Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Sat, 12 Dec 2015 12:09:36 -0800
Subject: [PATCH 16/31] Refuse to update valid lineages with staging certs

---
 letsencrypt/cli.py | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index 6c0209660..950629282 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -366,6 +366,8 @@ def _auth_from_domains(le_client, config, domains):
         # it without getting a new certificate at all.
         return lineage
     elif action == "renew":
+        orginal_server = lineage.configuration["renewalparams"]["server"]
+        _avoid_invalidating_lineage(config, lineage, original_server)
         # 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)
@@ -389,6 +391,17 @@ def _auth_from_domains(le_client, config, domains):
 
     return lineage
 
+def _avoid_invalidating_lineage(config, lineage, original_server):
+    "Do not renew a valid cert with one from a staging server!"
+    def is_staging(srv):
+        return (srv == constants.STAGING_URI or "staging" in srv)
+
+    if is_staging(config.server) and not is_staging(original_server):
+        if not config.break_my_certs:
+            raise errors.Error(
+                "You're trying to renew/replace a valid certificiate with "
+                "a test certificate. We will not do that unless you use the "
+                "--break-my-certs flag!")
 
 def set_configurator(previously, now):
     """
@@ -959,7 +972,10 @@ def prepare_and_parse_args(plugins, args):
     helpful.add(
         "testing", "--http-01-port", type=int, dest="http01_port",
         default=flag_default("http01_port"), help=config_help("http01_port"))
-
+    helpful.add(
+        "testing", "--break-my-certs", action="store_true",
+        help="Be willing to replace or renew valid certs with invalid "
+             "(testing/staging) certs")
     helpful.add_group(
         "security", description="Security parameters & server settings")
     helpful.add(

From 1cbf78284f7aa4fe05666ad8f15fe235a9be483e Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Sat, 12 Dec 2015 12:14:52 -0800
Subject: [PATCH 17/31] Lint, bugfix, helpful domain list

---
 letsencrypt/cli.py | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index 950629282..69a259d5b 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -366,7 +366,7 @@ def _auth_from_domains(le_client, config, domains):
         # it without getting a new certificate at all.
         return lineage
     elif action == "renew":
-        orginal_server = lineage.configuration["renewalparams"]["server"]
+        original_server = lineage.configuration["renewalparams"]["server"]
         _avoid_invalidating_lineage(config, lineage, original_server)
         # TODO: schoen wishes to reuse key - discussion
         # https://github.com/letsencrypt/letsencrypt/pull/777/files#r40498574
@@ -393,15 +393,16 @@ def _auth_from_domains(le_client, config, domains):
 
 def _avoid_invalidating_lineage(config, lineage, original_server):
     "Do not renew a valid cert with one from a staging server!"
-    def is_staging(srv):
-        return (srv == constants.STAGING_URI or "staging" in srv)
+    def _is_staging(srv):
+        return srv == constants.STAGING_URI or "staging" in srv
 
-    if is_staging(config.server) and not is_staging(original_server):
+    if _is_staging(config.server) and not _is_staging(original_server):
         if not config.break_my_certs:
+            names = ", ".join(lineage.names())
             raise errors.Error(
-                "You're trying to renew/replace a valid certificiate with "
-                "a test certificate. We will not do that unless you use the "
-                "--break-my-certs flag!")
+                "You've asked to renew/replace a valid certificiate with "
+                "a test certificate (domains: {0}). We will not do that "
+                "unless you use the --break-my-certs flag!".format(names))
 
 def set_configurator(previously, now):
     """

From d290cd36d59f0b922b0ab9f6effbd4acde2289ca Mon Sep 17 00:00:00 2001
From: Seth Schoen <schoen@eff.org>
Date: Sat, 12 Dec 2015 12:18:01 -0800
Subject: [PATCH 18/31] Correct typo

---
 letsencrypt/cli.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index ca92595d7..4cf27aa81 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -400,7 +400,7 @@ def _avoid_invalidating_lineage(config, lineage, original_server):
         if not config.break_my_certs:
             names = ", ".join(lineage.names())
             raise errors.Error(
-                "You've asked to renew/replace a valid certificiate with "
+                "You've asked to renew/replace a valid certificate with "
                 "a test certificate (domains: {0}). We will not do that "
                 "unless you use the --break-my-certs flag!".format(names))
 

From 411b5093e972edf532a3845e16093f42c0aebfc0 Mon Sep 17 00:00:00 2001
From: Seth Schoen <schoen@eff.org>
Date: Sat, 12 Dec 2015 12:23:16 -0800
Subject: [PATCH 19/31] Add parallel --reinstall command-line flag

---
 letsencrypt/cli.py | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index 4cf27aa81..c4e0dd2de 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -252,7 +252,13 @@ def _handle_identical_cert_request(config, cert):
 
     """
     if config.renew_by_default or cert.should_autorenew(interactive=True):
+        # Set with --renew-by-default, force an identical certificate to
+        # be renewed without further prompting.
         return "renew", cert
+    if config.reinstall:
+        # Set with --reinstall, force an identical certificate to be
+        # reinstalled without further prompting.
+        return "reinstall", cert
     display = zope.component.getUtility(interfaces.IDisplay)
     question = (
         "You have an existing certificate that contains exactly the same "
@@ -943,6 +949,9 @@ def prepare_and_parse_args(plugins, args):
     helpful.add(
         None, "--duplicate", dest="duplicate", action="store_true",
         help="Allow getting a certificate that duplicates an existing one")
+    helpful.add(
+        None, "--reinstall", dest="reinstall", action="store_true",
+        help="Try to reinstall an existing certificate without prompting")
 
     helpful.add_group(
         "automation",

From 0e19f43757eb90189cdfc72dc996ec2f1c06863d Mon Sep 17 00:00:00 2001
From: Seth Schoen <schoen@eff.org>
Date: Sat, 12 Dec 2015 12:35:28 -0800
Subject: [PATCH 20/31] Let's not blame the user in the cancel message

---
 letsencrypt/cli.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index c4e0dd2de..3cf01d3c1 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -273,8 +273,8 @@ def _handle_identical_cert_request(config, cert):
         # TODO: Add notification related to command-line options for
         #       skipping the menu for this case.
         raise errors.Error(
-            "User did not use proper CLI and would like "
-            "to reinvoke the client.")
+            "User chose to cancel the operation and may "
+            "reinvoke the client.")
     elif response[1] == 0:
         return "reinstall", cert
     elif response[1] == 1:
@@ -317,8 +317,8 @@ def _handle_subset_cert_request(config, domains, cert):
             ),
             reporter_util.HIGH_PRIORITY)
         raise errors.Error(
-            "User did not use proper CLI and would like "
-            "to reinvoke the client.")
+            "User chose to cancel the operation and may "
+            "reinvoke the client.")
 
 
 def _report_new_cert(cert_path, fullchain_path):

From 723d9fe048184b195ed6bcf4230fe045c345f6b0 Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Sat, 12 Dec 2015 12:44:24 -0800
Subject: [PATCH 21/31] Handle lineages that were upgraded from staging ->
 production

---
 letsencrypt/cli.py | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index ca92595d7..953fb8700 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -396,13 +396,22 @@ def _avoid_invalidating_lineage(config, lineage, original_server):
     def _is_staging(srv):
         return srv == constants.STAGING_URI or "staging" in srv
 
-    if _is_staging(config.server) and not _is_staging(original_server):
-        if not config.break_my_certs:
-            names = ", ".join(lineage.names())
-            raise errors.Error(
-                "You've asked to renew/replace a valid certificiate with "
-                "a test certificate (domains: {0}). We will not do that "
-                "unless you use the --break-my-certs flag!".format(names))
+    # Some lineages may have begun with --staging, but then had production certs
+    # added to them
+    latest_cert = OpenSSL.crypto.load_certificate(OpenSSL.crypto.FILETYPE_PEM,
+                                                  open(lineage.cert).read())
+    # all our test certs are from happy hacker fake CA, though maybe one day
+    # we should test more methodically
+    now_valid = not ("fake" in repr(latest_cert.get_issuer()).lower())
+
+    if _is_staging(config.server):
+        if not _is_staging(original_server) or now_valid:
+            if not config.break_my_certs:
+                names = ", ".join(lineage.names())
+                raise errors.Error(
+                    "You've asked to renew/replace a seemingly valid certificiate with "
+                    "a test certificate (domains: {0}). We will not do that "
+                    "unless you use the --break-my-certs flag!".format(names))
 
 def set_configurator(previously, now):
     """

From cd1c4e1e8e4038a6eb1803e968149304b29b07b8 Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Sat, 12 Dec 2015 13:02:22 -0800
Subject: [PATCH 22/31] Fix existing test

---
 letsencrypt/tests/cli_test.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py
index e865099e8..ccf16f5b5 100644
--- a/letsencrypt/tests/cli_test.py
+++ b/letsencrypt/tests/cli_test.py
@@ -413,7 +413,7 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
     @mock.patch('letsencrypt.cli._treat_as_renewal')
     @mock.patch('letsencrypt.cli._init_le_client')
     def test_certonly_renewal(self, mock_init, mock_renewal, mock_get_utility, _suggest):
-        cert_path = '/etc/letsencrypt/live/foo.bar/cert.pem'
+        cert_path = 'letsencrypt/tests/testdata/cert.pem'
         chain_path = '/etc/letsencrypt/live/foo.bar/fullchain.pem'
 
         mock_lineage = mock.MagicMock(cert=cert_path, fullchain=chain_path)

From eb289bcf8158be2bc1ff9f98cc88a01133a80ec3 Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Sat, 12 Dec 2015 13:06:58 -0800
Subject: [PATCH 23/31] Remove debugging printf

---
 letsencrypt/cli.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index 3cda04009..6ea7e9b55 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -366,7 +366,6 @@ def _auth_from_domains(le_client, config, domains):
     # (which results in treating the request as a new certificate request).
 
     action, lineage = _treat_as_renewal(config, domains)
-    print action, lineage
     if action == "reinstall":
         # The lineage already exists; allow the caller to try installing
         # it without getting a new certificate at all.

From 97f987ca41afd58e37867943d761148c355819ce Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Sat, 12 Dec 2015 13:14:48 -0800
Subject: [PATCH 24/31] lint

---
 letsencrypt/cli.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index 6ea7e9b55..fe36b0007 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -407,7 +407,7 @@ def _avoid_invalidating_lineage(config, lineage, original_server):
                                                   open(lineage.cert).read())
     # all our test certs are from happy hacker fake CA, though maybe one day
     # we should test more methodically
-    now_valid = not ("fake" in repr(latest_cert.get_issuer()).lower())
+    now_valid = not "fake" in repr(latest_cert.get_issuer()).lower()
 
     if _is_staging(config.server):
         if not _is_staging(original_server) or now_valid:

From 49e596785860674159c6584c803f850b0fbd31df Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Sat, 12 Dec 2015 14:09:33 -0800
Subject: [PATCH 25/31] UI and flag tuning

 * Add --expand or --replace, which in most cases is what people want to add
   new names to lineages without always forcing renewal
 * --reinstall is now also --keep, and the UI is aware if it's in certonly or
   run mode
 * Explain --duplicate better
---
 letsencrypt/cli.py | 54 ++++++++++++++++++++++++++++++----------------
 1 file changed, 36 insertions(+), 18 deletions(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index fe36b0007..ae76fcae6 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -251,24 +251,33 @@ def _handle_identical_cert_request(config, cert):
     :returns: Tuple of (string, cert_or_None) as per _treat_as_renewal
 
     """
-    if config.renew_by_default or cert.should_autorenew(interactive=True):
-        # Set with --renew-by-default, force an identical certificate to
-        # be renewed without further prompting.
+    if config.renew_by_default:
+        logger.info("Auto-renewal forced with --renew-by-default...")
+        return "renew", cert
+    if cert.should_autorenew(interactive=True):
+        logger.info("Cert is due for renewal, auto-renewing...")
         return "renew", cert
     if config.reinstall:
         # Set with --reinstall, force an identical certificate to be
         # reinstalled without further prompting.
         return "reinstall", cert
-    display = zope.component.getUtility(interfaces.IDisplay)
+
     question = (
         "You have an existing certificate that contains exactly the same "
-        "domains you requested.{br}(ref: {0}){br}{br}What would you like to do?"
+        "domains you requested and isn't close to expiry."
+        "{br}(ref: {0}){br}{br}What would you like to do?"
     ).format(cert.configfile.filename, br=os.linesep)
-    response = display.menu(
-        question, ["Attempt to reinstall this existing certificate",
-                   "Renew & replace the cert (limit ~5 per 7 days)",
-                   "Cancel this operation and do nothing"],
-        "OK", "Cancel")
+
+    if config.verb == "run":
+        keep_opt = "Attempt to reinstall this existing certificate"
+    elif config.verb == "certonly":
+        keep_opt = "Keep the existing certificate for now"
+    choices = [keep_opt,
+               "Renew & replace the cert (limit ~5 per 7 days)",
+               "Cancel this operation and do nothing"]
+
+    display = zope.component.getUtility(interfaces.IDisplay)
+    response = display.menu(question, choices, "OK", "Cancel")
     if response[0] == "cancel" or response[1] == 2:
         # TODO: Add notification related to command-line options for
         #       skipping the menu for this case.
@@ -301,7 +310,7 @@ def _handle_subset_cert_request(config, domains, cert):
              existing,
              ", ".join(domains),
              br=os.linesep)
-    if config.renew_by_default or zope.component.getUtility(
+    if config.expand or config.renew_by_default or zope.component.getUtility(
             interfaces.IDisplay).yesno(question, "Replace", "Cancel"):
         return "renew", cert
     else:
@@ -772,6 +781,7 @@ class HelpfulArgumentParser(object):
         """
         parsed_args = self.parser.parse_args(self.args)
         parsed_args.func = self.VERBS[self.verb]
+        parsed_args.verb = self.verb
 
         # Do any post-parsing homework here
 
@@ -955,12 +965,11 @@ def prepare_and_parse_args(plugins, args):
                 "multiple -d flags or enter a comma separated list of domains "
                 "as a parameter.")
     helpful.add(
-        None, "--duplicate", dest="duplicate", action="store_true",
-        help="Allow getting a certificate that duplicates an existing one")
-    helpful.add(
-        None, "--reinstall", dest="reinstall", action="store_true",
-        help="Try to reinstall an existing certificate without prompting")
-
+        None, "--keep-until-expiring", "--keep", "--reinstall",
+        dest="reinstall", action="store_true",
+        help="If the requested cert matches an existing cert, keep the "
+             "existing one by default until it is due for renewal (for the "
+             "'run' subcommand this means reinstall the existing cert)")
     helpful.add_group(
         "automation",
         description="Arguments for automating execution & other tweaks")
@@ -968,16 +977,25 @@ def prepare_and_parse_args(plugins, args):
         "automation", "--version", action="version",
         version="%(prog)s {0}".format(letsencrypt.__version__),
         help="show program's version number and exit")
+    helpful.add(
+        "automation", "--expand", "--expand-existing-certs", "--replace", action="store_true",
+        help="If an existing cert covers some subset of the requested names, "
+             "expand and replace it with the additional names.")
     helpful.add(
         "automation", "--renew-by-default", action="store_true",
         help="Select renewal by default when domains are a superset of a "
-             "previously attained cert")
+             "previously attained cert (often --keep-until-expiring is "
+             "more appropriate). Implies --expand.")
     helpful.add(
         "automation", "--agree-tos", dest="tos", action="store_true",
         help="Agree to the Let's Encrypt Subscriber Agreement")
     helpful.add(
         "automation", "--account", metavar="ACCOUNT_ID",
         help="Account ID to use")
+    helpful.add(
+        "automation", "--duplicate", dest="duplicate", action="store_true",
+        help="Allow making a certificate lineage that duplicates an existing one "
+             "(mostly useful for multiple webservers with distinct keys)")
 
     helpful.add_group(
         "testing", description="The following flags are meant for "

From 8ed70c18cc46f4cc54b5c69dcd7f85f31996443f Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Sat, 12 Dec 2015 14:14:18 -0800
Subject: [PATCH 26/31] Tweak docs for --duplicate

---
 letsencrypt/cli.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index ae76fcae6..81a5f4f65 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -995,7 +995,7 @@ def prepare_and_parse_args(plugins, args):
     helpful.add(
         "automation", "--duplicate", dest="duplicate", action="store_true",
         help="Allow making a certificate lineage that duplicates an existing one "
-             "(mostly useful for multiple webservers with distinct keys)")
+             "(both can be renewed in parallel)")
 
     helpful.add_group(
         "testing", description="The following flags are meant for "

From 685ec6b5398342b99422cf50b37e78dd61aedf1d Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Sat, 12 Dec 2015 14:21:05 -0800
Subject: [PATCH 27/31] obtain_cert isn't dead; maybe it should be called
 certonly though...

---
 letsencrypt/cli.py | 2 --
 1 file changed, 2 deletions(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index 81a5f4f65..ed0bc230d 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -570,8 +570,6 @@ def run(args, config, plugins):  # pylint: disable=too-many-branches,too-many-lo
 
 def obtain_cert(args, config, plugins):
     """Authenticate & obtain cert, but do not install it."""
-    # TODO: Is this now dead code? What calls it?
-
     if args.domains and args.csr is not None:
         # TODO: --csr could have a priority, when --domains is
         # supplied, check if CSR matches given domains?

From b34887437246b372a9687ff9222a1c7bed77b02e Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Sat, 12 Dec 2015 16:39:52 -0800
Subject: [PATCH 28/31] Address review comments, fine tune flag names

---
 letsencrypt/cli.py | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index ed0bc230d..317702497 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -235,7 +235,7 @@ def _treat_as_renewal(config, domains):
     ident_names_cert, subset_names_cert = _find_duplicative_certs(config, domains)
     # XXX ^ schoen is not sure whether that correctly reads the systemwide
     # configuration file.
-    if not (ident_names_cert or subset_names_cert):
+    if ident_names_cert is None and subset_names_cert is None:
         return "newcert", None
 
     if ident_names_cert is not None:
@@ -249,6 +249,7 @@ def _handle_identical_cert_request(config, cert):
     :param storage.RenewableCert cert:
 
     :returns: Tuple of (string, cert_or_None) as per _treat_as_renewal
+    :rtype: tuple
 
     """
     if config.renew_by_default:
@@ -297,6 +298,7 @@ def _handle_subset_cert_request(config, domains, cert):
     :param storage.RenewableCert cert:
 
     :returns: Tuple of (string, cert_or_None) as per _treat_as_renewal
+    :rtype: tuple
 
     """
     existing = ", ".join(cert.names())
@@ -962,23 +964,23 @@ def prepare_and_parse_args(plugins, args):
                 help="Domain names to apply. For multiple domains you can use "
                 "multiple -d flags or enter a comma separated list of domains "
                 "as a parameter.")
-    helpful.add(
-        None, "--keep-until-expiring", "--keep", "--reinstall",
-        dest="reinstall", action="store_true",
-        help="If the requested cert matches an existing cert, keep the "
-             "existing one by default until it is due for renewal (for the "
-             "'run' subcommand this means reinstall the existing cert)")
     helpful.add_group(
         "automation",
         description="Arguments for automating execution & other tweaks")
+    helpful.add(
+        "automation", "--keep-until-expiring", "-k", "--reinstall",
+        dest="reinstall", action="store_true",
+        help="If the requested cert matches an existing cert, always keep the "
+             "existing one until it is due for renewal (for the "
+             "'run' subcommand this means reinstall the existing cert)")
+    helpful.add(
+        "automation", "--expand", action="store_true",
+        help="If an existing cert covers some subset of the requested names, "
+             "always expand and replace it with the additional names.")
     helpful.add(
         "automation", "--version", action="version",
         version="%(prog)s {0}".format(letsencrypt.__version__),
         help="show program's version number and exit")
-    helpful.add(
-        "automation", "--expand", "--expand-existing-certs", "--replace", action="store_true",
-        help="If an existing cert covers some subset of the requested names, "
-             "expand and replace it with the additional names.")
     helpful.add(
         "automation", "--renew-by-default", action="store_true",
         help="Select renewal by default when domains are a superset of a "

From 173ea94e17bbcdc321ab4607e78491a2f0481753 Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Sat, 12 Dec 2015 17:01:43 -0800
Subject: [PATCH 29/31] Further flag tweaking

---
 letsencrypt/cli.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index 317702497..5e06d00d6 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -306,14 +306,14 @@ def _handle_subset_cert_request(config, domains, cert):
         "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: {2}.{br}{br}Do you want to expand and replace this existing "
         "certificate with the new certificate?"
     ).format(cert.configfile.filename,
              existing,
              ", ".join(domains),
              br=os.linesep)
     if config.expand or config.renew_by_default or zope.component.getUtility(
-            interfaces.IDisplay).yesno(question, "Replace", "Cancel"):
+            interfaces.IDisplay).yesno(question, "Expand", "Cancel"):
         return "renew", cert
     else:
         reporter_util = zope.component.getUtility(interfaces.IReporter)
@@ -968,7 +968,7 @@ def prepare_and_parse_args(plugins, args):
         "automation",
         description="Arguments for automating execution & other tweaks")
     helpful.add(
-        "automation", "--keep-until-expiring", "-k", "--reinstall",
+        "automation", "--keep-until-expiring", "--keep", "--reinstall",
         dest="reinstall", action="store_true",
         help="If the requested cert matches an existing cert, always keep the "
              "existing one until it is due for renewal (for the "

From aee25fb05ee63ce42ed23beef08a30904c9039c8 Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Sat, 12 Dec 2015 17:21:24 -0800
Subject: [PATCH 30/31] Attempt to fix ridiculous log message

---
 letsencrypt/storage.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/letsencrypt/storage.py b/letsencrypt/storage.py
index f457fe13e..01ff0677f 100644
--- a/letsencrypt/storage.py
+++ b/letsencrypt/storage.py
@@ -567,8 +567,8 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
                 "cert", self.latest_common_version()))
             now = pytz.UTC.fromutc(datetime.datetime.utcnow())
             if expiry < add_time_interval(now, interval):
-                logger.debug("Should renew, certificate "
-                             "has been expired since %s.",
+                logger.debug("Should renew, less than %r days before certificate "
+                             "expiry %s.", interval,
                              expiry.strftime("%Y-%m-%d %H:%M:%S %Z"))
                 return True
         return False

From a99f1d1395d6ffab7b0b41cc8a9b6a4bb3386890 Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Sat, 12 Dec 2015 17:24:05 -0800
Subject: [PATCH 31/31] This should be maximally legible.

---
 letsencrypt/storage.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/letsencrypt/storage.py b/letsencrypt/storage.py
index 01ff0677f..3b2b548b0 100644
--- a/letsencrypt/storage.py
+++ b/letsencrypt/storage.py
@@ -567,7 +567,7 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
                 "cert", self.latest_common_version()))
             now = pytz.UTC.fromutc(datetime.datetime.utcnow())
             if expiry < add_time_interval(now, interval):
-                logger.debug("Should renew, less than %r days before certificate "
+                logger.debug("Should renew, less than %s before certificate "
                              "expiry %s.", interval,
                              expiry.strftime("%Y-%m-%d %H:%M:%S %Z"))
                 return True