diff --git a/certbot/main.py b/certbot/main.py index 3557f65b3..dd497d14d 100644 --- a/certbot/main.py +++ b/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) diff --git a/certbot/renewal.py b/certbot/renewal.py index 339d7b7ff..5e57c3e20 100644 --- a/certbot/renewal.py +++ b/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 diff --git a/certbot/storage.py b/certbot/storage.py index 5ca2ff6a9..c740657d8 100644 --- a/certbot/storage.py +++ b/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? diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py index 6cbbea631..39a54258a 100644 --- a/certbot/tests/cli_test.py +++ b/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) diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index 69291871a..fab1065c5 100644 --- a/certbot/tests/main_test.py +++ b/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): diff --git a/certbot/tests/storage_test.py b/certbot/tests/storage_test.py index 7ac7771da..9566e0aec 100644 --- a/certbot/tests/storage_test.py +++ b/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: