From cc86ff2a216ce6686cb4f826ae0892589e9f744d Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 7 Dec 2016 16:02:13 -0800 Subject: [PATCH] Improve the "certbot certificates" output (#3846) * Begin making "certbot certificates" future safe * Handle the case where a renewal conf file has no "server" entry --- certbot/cert_manager.py | 14 ++++++++------ certbot/renewal.py | 8 ++------ certbot/storage.py | 12 +++++++++++- certbot/tests/cert_manager_test.py | 12 ++++++++++-- certbot/tests/storage_test.py | 13 +++++++++++++ certbot/util.py | 12 ++++++++++++ 6 files changed, 56 insertions(+), 15 deletions(-) diff --git a/certbot/cert_manager.py b/certbot/cert_manager.py index a3237253e..f16d19201 100644 --- a/certbot/cert_manager.py +++ b/certbot/cert_manager.py @@ -39,20 +39,22 @@ def _report_human_readable(parsed_certs): certinfo = [] for cert in parsed_certs: now = pytz.UTC.fromutc(datetime.datetime.utcnow()) - if cert.target_expiry <= now: - expiration_text = "EXPIRED" + if cert.is_test_cert: + expiration_text = "INVALID: TEST CERT" + elif cert.target_expiry <= now: + expiration_text = "INVALID: EXPIRED" else: diff = cert.target_expiry - now if diff.days == 1: - expiration_text = "1 day" + expiration_text = "VALID: 1 day" elif diff.days < 1: - expiration_text = "under 1 day" + expiration_text = "VALID: {0} hour(s)".format(diff.seconds // 3600) else: - expiration_text = "{0} days".format(diff.days) + expiration_text = "VALID: {0} days".format(diff.days) valid_string = "{0} ({1})".format(cert.target_expiry, expiration_text) certinfo.append(" Certificate Name: {0}\n" " Domains: {1}\n" - " Valid Until: {2}\n" + " Expiry Date: {2}\n" " Certificate Path: {3}\n" " Private Key Path: {4}".format( cert.lineagename, diff --git a/certbot/renewal.py b/certbot/renewal.py index aa39c5fad..09a58ed1b 100644 --- a/certbot/renewal.py +++ b/certbot/renewal.py @@ -13,7 +13,6 @@ import OpenSSL from certbot import configuration from certbot import cli -from certbot import constants from certbot import crypto_util from certbot import errors @@ -209,9 +208,6 @@ def should_renew(config, 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 - # Some lineages may have begun with --staging, but then had production certs # added to them latest_cert = OpenSSL.crypto.load_certificate( @@ -220,8 +216,8 @@ def _avoid_invalidating_lineage(config, lineage, original_server): # we should test more methodically now_valid = "fake" not in repr(latest_cert.get_issuer()).lower() - if _is_staging(config.server): - if not _is_staging(original_server) or now_valid: + if util.is_staging(config.server): + if not util.is_staging(original_server) or now_valid: if not config.break_my_certs: names = ", ".join(lineage.names()) raise errors.Error( diff --git a/certbot/storage.py b/certbot/storage.py index 1fc13a5df..2536e59ca 100644 --- a/certbot/storage.py +++ b/certbot/storage.py @@ -172,7 +172,8 @@ def relevant_values(all_values): if _relevant(option) and cli.option_was_set(option, value)) -class RenewableCert(object): # pylint: disable=too-many-instance-attributes +class RenewableCert(object): + # pylint: disable=too-many-instance-attributes,too-many-public-methods """Renewable certificate. Represents a lineage of certificates that is under the management of @@ -281,6 +282,15 @@ class RenewableCert(object): # pylint: disable=too-many-instance-attributes return os.path.join( self.cli_config.default_archive_dir, self.lineagename) + @property + def is_test_cert(self): + """Returns true if this is a test cert from a staging server.""" + server = self.configuration["renewalparams"].get("server", None) + if server: + return util.is_staging(server) + else: + return False + def _check_symlinks(self): """Raises an exception if a symlink doesn't exist""" for kind in ALL_FOUR: diff --git a/certbot/tests/cert_manager_test.py b/certbot/tests/cert_manager_test.py index c67ab5e50..68e095cd6 100644 --- a/certbot/tests/cert_manager_test.py +++ b/certbot/tests/cert_manager_test.py @@ -157,26 +157,34 @@ class CertificatesTest(BaseCertManagerTest): cert = mock.MagicMock(lineagename="nameone") cert.target_expiry = expiry cert.names.return_value = ["nameone", "nametwo"] + cert.is_test_cert = False parsed_certs = [cert] # pylint: disable=protected-access out = cert_manager._report_human_readable(parsed_certs) - self.assertTrue('EXPIRED' in out) + self.assertTrue("INVALID: EXPIRED" in out) cert.target_expiry += datetime.timedelta(hours=2) # pylint: disable=protected-access out = cert_manager._report_human_readable(parsed_certs) - self.assertTrue('under 1 day' in out) + self.assertTrue('1 hour(s)' in out) + self.assertTrue('VALID' in out and not 'INVALID' in out) cert.target_expiry += datetime.timedelta(days=1) # pylint: disable=protected-access out = cert_manager._report_human_readable(parsed_certs) self.assertTrue('1 day' in out) self.assertFalse('under' in out) + self.assertTrue('VALID' in out and not 'INVALID' in out) cert.target_expiry += datetime.timedelta(days=2) # pylint: disable=protected-access out = cert_manager._report_human_readable(parsed_certs) self.assertTrue('3 days' in out) + self.assertTrue('VALID' in out and not 'INVALID' in out) + + cert.is_test_cert = True + out = cert_manager._report_human_readable(parsed_certs) + self.assertTrue('INVALID: TEST CERT' in out) if __name__ == "__main__": unittest.main() # pragma: no cover diff --git a/certbot/tests/storage_test.py b/certbot/tests/storage_test.py index 1b212449e..ebe7d2243 100644 --- a/certbot/tests/storage_test.py +++ b/certbot/tests/storage_test.py @@ -705,6 +705,19 @@ class RenewableCertTests(BaseRenewableCertTest): self.assertEqual(storage.add_time_interval(base_time, interval), excepted) + def test_is_test_cert(self): + self.test_rc.configuration["renewalparams"] = {} + rp = self.test_rc.configuration["renewalparams"] + self.assertEqual(self.test_rc.is_test_cert, False) + rp["server"] = "https://acme-staging.api.letsencrypt.org/directory" + self.assertEqual(self.test_rc.is_test_cert, True) + rp["server"] = "https://staging.someotherca.com/directory" + self.assertEqual(self.test_rc.is_test_cert, True) + rp["server"] = "https://acme-v01.api.letsencrypt.org/directory" + self.assertEqual(self.test_rc.is_test_cert, False) + rp["server"] = "https://acme-v02.api.letsencrypt.org/directory" + self.assertEqual(self.test_rc.is_test_cert, False) + def test_missing_cert(self): from certbot import storage self.assertRaises(errors.CertStorageError, diff --git a/certbot/util.py b/certbot/util.py index 220795237..1597c5bd8 100644 --- a/certbot/util.py +++ b/certbot/util.py @@ -17,6 +17,7 @@ import sys import configargparse +from certbot import constants from certbot import errors @@ -499,3 +500,14 @@ def get_strict_version(normalized): # strict version ending with "a" and a number designates a pre-release # pylint: disable=no-member return distutils.version.StrictVersion(normalized.replace(".dev", "a")) + + +def is_staging(srv): + """ + Determine whether a given ACME server is a known test / staging server. + + :param str srv: the URI for the ACME server + :returns: True iff srv is a known test / staging server + :rtype bool: + """ + return srv == constants.STAGING_URI or "staging" in srv