Renew symlink safety (#3560)

Re-do the fix for #3497 to ensure it works in all cases.

* If lineages are in an inconsistent (non-deployed) state, deploy them

* Test new _handle_identical_cert case

* Move lineage.has_pending_deployment() check up to _auth_from_domains

Less conceptually nice, but in the "renew" verb case it wasn't being called :(

* Swap _auth_from_domains return type

 * It now matches _treat_as_renewal & _handle_identical_cert_request etc

* Revert "Move lineage.has_pending_deployment() check up to _auth_from_domains"

This reverts commit a7fe734d73.

* Move test back to handle_identical_cert_request

* We need to check for non-deployment on two separate code paths

 - Once high up in "renew" (because failure to be deployed stops us from
   divind down the stack)
 - Once way down in _handle_identical_cert_request (because that's where it
   makes the most sense for run / certonly)
 - So refactor that work into storage.py

* We don't necessarily reinstall

certbot/main.py

@@ -67,16 +67,12 @@ def _report_successful_dry_run(config):
                                   reporter_util.HIGH_PRIORITY, on_crash=False)
 
 
-
 def _auth_from_domains(le_client, config, domains, lineage=None):
-    """Authenticate and enroll certificate."""
-    # 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).
+    """Authenticate and enroll certificate.
 
+    :returns: Tuple of (str action, cert_or_None) as per _treat_as_renewal
+              action can be: "newcert" | "renew" | "reinstall"
+    """
     # If lineage is specified, use that one instead of looking around for
     # a matching one.
     if lineage is None:
@@ -91,7 +87,7 @@ def _auth_from_domains(le_client, config, domains, lineage=None):
         # The lineage already exists; allow the caller to try installing
         # it without getting a new certificate at all.
         logger.info("Keeping the existing certificate")
-        return lineage, "reinstall"
+        return "reinstall", lineage
 
     hooks.pre_hook(config)
     try:
@@ -110,7 +106,7 @@ def _auth_from_domains(le_client, config, domains, lineage=None):
     if not config.dry_run and not config.verb == "renew":
         _report_new_cert(config, lineage.cert, lineage.fullchain)
 
-    return lineage, action
+    return action, lineage
 
 
 def _handle_subset_cert_request(config, domains, cert):
@@ -165,10 +161,7 @@ def _handle_identical_cert_request(config, lineage):
     :rtype: tuple
 
     """
-    if lineage.has_pending_deployment():
-        logger.warn("Found a new cert /archive/ that was not linked to in /live/; "
-                    "fixing and reinstalling..")
-        lineage.update_all_links_to(lineage.latest_common_version())
+    if not lineage.ensure_deployed():
         return "reinstall", lineage
     if renewal.should_renew(config, lineage):
         return "renew", lineage
@@ -515,7 +508,7 @@ def run(config, plugins):  # pylint: disable=too-many-branches,too-many-locals
     # TODO: Handle errors from _init_le_client?
     le_client = _init_le_client(config, authenticator, installer)
 
-    lineage, action = _auth_from_domains(le_client, config, domains)
+    action, lineage = _auth_from_domains(le_client, config, domains)
 
     le_client.deploy_certificate(
         domains, lineage.privkey, lineage.cert,
@@ -567,7 +560,7 @@ def obtain_cert(config, plugins, lineage=None):
     # SHOWTIME: Possibly obtain/renew a cert, and set action to renew | newcert | reinstall
     if config.csr is None: # the common case
         domains = _find_domains(config, installer)
-        _, action = _auth_from_domains(le_client, config, domains, lineage)
+        action, _ = _auth_from_domains(le_client, config, domains, lineage)
     else:
         assert lineage is None, "Did not expect a CSR with a RenewableCert"
         _csr_obtain_cert(config, le_client)

certbot/renewal.py

@@ -341,6 +341,7 @@ def renew_all_lineages(config):
             else:
                 # XXX: ensure that each call here replaces the previous one
                 zope.component.provideUtility(lineage_config)
+                renewal_candidate.ensure_deployed()
                 if should_renew(lineage_config, renewal_candidate):
                     plugins = plugins_disco.PluginsRegistry.find_all()
                     from certbot import main

certbot/storage.py

@@ -520,6 +520,22 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
         # for the others.
         return max(self.newest_available_version(x) for x in ALL_FOUR) + 1
 
+    def ensure_deployed(self):
+        """Make sure we've deployed the latest version.
+
+        :returns: False if a change was needed, True otherwise
+        :rtype: bool
+
+        May need to recover from rare interrupted / crashed states."""
+
+        if self.has_pending_deployment():
+            logger.warn("Found a new cert /archive/ that was not linked to in /live/; "
+                        "fixing...")
+            self.update_all_links_to(self.latest_common_version())
+            return False
+        return True
+
+
     def has_pending_deployment(self):
         """Is there a later version of all of the managed items?
 

certbot/tests/cli_test.py

@@ -177,7 +177,7 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
         args = ["--standalone", "certonly", "-m", "none@none.com",
                 "-d", "example.com", '--agree-tos'] + self.standard_args
         det.return_value = mock.MagicMock(), None
-        afd.return_value = mock.MagicMock(), "newcert"
+        afd.return_value = "newcert", mock.MagicMock()
 
         with mock.patch('certbot.main.client.acme_client.ClientNetwork') as acme_net:
             self._call_no_clientmock(args)

certbot/tests/main_test.py

@@ -17,17 +17,13 @@ class MainTest(unittest.TestCase):
     def tearDown(self):
         pass
 
-    @mock.patch("certbot.main.logger")
-    def test_handle_identical_cert_request_pending(self, _mock_logger):
-        # For now, just test has_pending_deployment_branch; other
-        # coverage is in cli_test.py...
+    def test_handle_identical_cert_request_pending(self):
         from certbot import main
         mock_lineage = mock.Mock()
-        mock_lineage.has_pending_deployment.return_value = True
+        mock_lineage.ensure_deployed.return_value = False
         # pylint: disable=protected-access
         ret = main._handle_identical_cert_request(mock.Mock(), mock_lineage)
         self.assertEqual(ret, ("reinstall", mock_lineage))
-        self.assertEqual(mock_lineage.update_all_links_to.call_count, 1)
 
 class ObtainCertTest(unittest.TestCase):
     """Tests for certbot.main.obtain_cert."""
@@ -55,7 +51,7 @@ class ObtainCertTest(unittest.TestCase):
     def test_no_reinstall_text_pause(self, mock_auth):
         mock_notification = self.mock_get_utility().notification
         mock_notification.side_effect = self._assert_no_pause
-        mock_auth.return_value = (mock.ANY, 'reinstall')
+        mock_auth.return_value = ('reinstall', mock.ANY)
         self._call('certonly --webroot -d example.com -t'.split())
 
     def _assert_no_pause(self, message, height=42, pause=True):

certbot/tests/storage_test.py

@@ -258,6 +258,23 @@ class RenewableCertTests(BaseRenewableCertTest):
         self.assertEqual(self.test_rc.latest_common_version(), 17)
         self.assertEqual(self.test_rc.next_free_version(), 18)
 
+    @mock.patch("certbot.storage.logger")
+    def test_ensure_deployed(self, mock_logger):
+        mock_update = self.test_rc.update_all_links_to = mock.Mock()
+        mock_has_pending = self.test_rc.has_pending_deployment = mock.Mock()
+        self.test_rc.latest_common_version = mock.Mock()
+
+        mock_has_pending.return_value = False
+        self.assertEqual(self.test_rc.ensure_deployed(), True)
+        self.assertEqual(mock_update.call_count, 0)
+        self.assertEqual(mock_logger.warn.call_count, 0)
+
+        mock_has_pending.return_value = True
+        self.assertEqual(self.test_rc.ensure_deployed(), False)
+        self.assertEqual(mock_update.call_count, 1)
+        self.assertEqual(mock_logger.warn.call_count, 1)
+
+
     def test_update_link_to(self):
         for ver in six.moves.range(1, 6):
             for kind in ALL_FOUR: