diff --git a/certbot/cert_manager.py b/certbot/cert_manager.py index 86b5eedd9..09798e3bc 100644 --- a/certbot/cert_manager.py +++ b/certbot/cert_manager.py @@ -169,22 +169,25 @@ def _report_lines(msgs): def _report_human_readable(config, parsed_certs): """Format a results report for a parsed cert""" certinfo = [] - checker = ocsp.RevocationChecker(config) + checker = ocsp.RevocationChecker() for cert in parsed_certs: if config.certname and cert.lineagename != config.certname: continue if config.domains and not set(config.domains).issubset(cert.names()): continue now = pytz.UTC.fromutc(datetime.datetime.utcnow()) - status = "" - if cert.is_test_cert: - status = "INVALID: TEST_CERT" - if cert.target_expiry <= now: - status = status + ",EXPIRED" if status else "INVALID: EXPIRED" - else: - status = checker.ocsp_status(cert.cert, cert.chain, status) - if not status: + reasons = [] + if cert.is_test_cert: + reasons.append('TEST_CERT') + if cert.target_expiry <= now: + reasons.append('EXPIRED') + if checker.ocsp_revoked(cert.cert, cert.chain): + reasons.append('REVOKED') + + if reasons: + status = "INVALID: " + ", ".join(reasons) + else: diff = cert.target_expiry - now if diff.days == 1: status = "VALID: 1 day" diff --git a/certbot/cli.py b/certbot/cli.py index 19d917739..68eb67b35 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -529,10 +529,6 @@ class HelpfulArgumentParser(object): constants.FORCE_INTERACTIVE_FLAG)) parsed_args.noninteractive_mode = True - parsed_args.check_ocsp = parsed_args.check_ocsp.lower() - if parsed_args.check_ocsp not in ("always", "never", "lazy"): - raise errors.Error('--check-ocsp must be "always", "never", or "lazy"') - if parsed_args.force_interactive and parsed_args.noninteractive_mode: raise errors.Error( "Flag for non-interactive mode and {0} conflict".format( @@ -949,10 +945,6 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis "testing", "--no-verify-ssl", action="store_true", help=config_help("no_verify_ssl"), default=flag_default("no_verify_ssl")) - helpful.add( - ["certificates", "testing"], "--check-ocsp", default="always", - help='Whether to check OCSP for listed certs. Can be set to "never", "always",' - ' or "lazy" (ie, only for certs that are otherwise valid).') helpful.add( ["testing", "standalone", "apache", "nginx"], "--tls-sni-01-port", type=int, default=flag_default("tls_sni_01_port"), diff --git a/certbot/ocsp.py b/certbot/ocsp.py index f24d94266..a5df5743d 100644 --- a/certbot/ocsp.py +++ b/certbot/ocsp.py @@ -8,17 +8,11 @@ from certbot import util logger = logging.getLogger(__name__) - -REV_LABEL = "REVOKED" - -INSTALL_LABEL = "(Installed)" - class RevocationChecker(object): "This class figures out OCSP checking on this system, and performs it." - def __init__(self, config): + def __init__(self): self.broken = False - self.config = config if not util.exe_exists("openssl"): logging.info("openssl not installed, can't check revocation") @@ -35,46 +29,25 @@ class RevocationChecker(object): self.host_args = lambda host: ["Host", host] - def ocsp_status(self, cert_path, chain_path, status_in): - """Helper function: updates a cert status string with revocation information - - :param str cert_path: path to a cert to check - :param str chain_path: issuing intermediate for the cert - :param str status_in: a string that is either empty, if the cert is otherwise - believed to be valid, or 'INVALID: $REASON'. - - :returns: a new status including revocation, if the cert is revoked.""" - - if self.config.check_ocsp == "never": - return status_in - elif self.config.check_ocsp == "lazy" and status_in: - return status_in - - revoked = self.check_ocsp(cert_path, chain_path) - if not revoked: - return status_in - elif status_in: - return status_in + ",REVOKED" - else: - return "INVALID: REVOKED" - - - def check_ocsp(self, cert_path, chain_path): + def ocsp_revoked(self, cert_path, chain_path): """Get revoked status for a particular cert version. .. todo:: Make this a non-blocking call :param str cert_path: Path to certificate :param str chain_path: Path to intermediate cert + :rtype bool or None: + :returns: False if valid; True if revoked; None if check itself failed """ if self.broken: - return False + return None + logger.debug("Querying OCSP for %s", cert_path) url, host = self.determine_ocsp_server(cert_path) if not host: - return False + return None # jdkasten thanks "Bulletproof SSL and TLS - Ivan Ristic" for documenting this! cmd = ["openssl", "ocsp", "-no_nonce", @@ -89,7 +62,7 @@ class RevocationChecker(object): except errors.SubprocessError as e: logger.info("OCSP check failed for %s (are we offline?)", cert_path) logger.debug("Command was:\n%s\nError was:\n%s", " ".join(cmd), e) - return False + return None return _translate_ocsp_query(cert_path, output, err) @@ -103,7 +76,7 @@ class RevocationChecker(object): """ try: - url, err = util.run_script( + url, _err = util.run_script( ["openssl", "x509", "-in", cert_path, "-noout", "-ocsp_uri"], log=logging.debug) except errors.SubprocessError as e: @@ -119,18 +92,18 @@ class RevocationChecker(object): logger.info("Cannot process OCSP host from URL (%s) in cert at %s", url, cert_path) return None, None - def _translate_ocsp_query(cert_path, ocsp_output, ocsp_errors): - """Returns a label string out of the query.""" + """Parse openssl's weird output to work out what it means.""" + if not "Response verify OK" in ocsp_errors: logger.info("Revocation status for %s is unknown", cert_path) logger.debug("Uncertain ouput:\n%s\nstderr:\n%s", ocsp_output, ocsp_errors) - return "" + return None if cert_path + ": good" in ocsp_output: - return "" + return False elif cert_path + ": revoked" in ocsp_output: - return REV_LABEL + return True else: logger.warn("Unable to properly parse OCSP output: %s", ocsp_output) - return "" + return None diff --git a/certbot/tests/cert_manager_test.py b/certbot/tests/cert_manager_test.py index 4fd66c34a..dc83d0952 100644 --- a/certbot/tests/cert_manager_test.py +++ b/certbot/tests/cert_manager_test.py @@ -179,9 +179,9 @@ class CertificatesTest(BaseCertManagerTest): self.assertTrue(mock_utility.called) shutil.rmtree(tempdir) - @mock.patch('certbot.cert_manager.ocsp.RevocationChecker.ocsp_status') + @mock.patch('certbot.cert_manager.ocsp.RevocationChecker.ocsp_revoked') def test_report_human_readable(self, mock_ocsp): - mock_ocsp.side_effect = lambda _cert, _chain, status: status + mock_ocsp.side_effect = lambda _cert, _chain: None from certbot import cert_manager import datetime, pytz expiry = pytz.UTC.fromutc(datetime.datetime.utcnow()) diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py index b5c81bee4..e3bd28a5e 100644 --- a/certbot/tests/cli_test.py +++ b/certbot/tests/cli_test.py @@ -268,10 +268,6 @@ class ParseTest(unittest.TestCase): self.assertRaises( errors.Error, self.parse, "-n --force-interactive".split()) - def test_check_ocsp(self): - self.assertRaises(errors.Error, self.parse, "certificates --check-ocsp bogus".split()) - self.parse("certificates --check-ocsp LaZy".split()) - class DefaultTest(unittest.TestCase): """Tests for certbot.cli._Default.""" diff --git a/certbot/tests/ocsp_test.py b/certbot/tests/ocsp_test.py index 0bc947021..ce3ade2e8 100644 --- a/certbot/tests/ocsp_test.py +++ b/certbot/tests/ocsp_test.py @@ -2,13 +2,11 @@ """Tests for hooks.py""" # pylint: disable=protected-access -import os import unittest import mock from certbot import errors -from certbot import hooks out = """Missing = in header key=value ocsp: Use -help for summary. @@ -20,8 +18,13 @@ class OCSPTest(unittest.TestCase): def setUp(self): from certbot import ocsp - self.config = mock.MagicMock() - self.checker = ocsp.RevocationChecker(self.config) + with mock.patch('certbot.ocsp.Popen') as mock_popen: + with mock.patch('certbot.util.exe_exists') as mock_exists: + mock_communicate = mock.MagicMock() + mock_communicate.communicate.return_value = (None, out) + mock_popen.return_value = mock_communicate + mock_exists.return_value = True + self.checker = ocsp.RevocationChecker() def tearDown(self): pass @@ -36,44 +39,38 @@ class OCSPTest(unittest.TestCase): mock_exists.return_value = True from certbot import ocsp - checker = ocsp.RevocationChecker(self.config) + checker = ocsp.RevocationChecker() self.assertEqual(mock_popen.call_count, 1) self.assertEqual(checker.host_args("x"), ["Host=x"]) mock_communicate.communicate.return_value = (None, out.partition("\n")[2]) - checker = ocsp.RevocationChecker(self.config) + checker = ocsp.RevocationChecker() self.assertEqual(checker.host_args("x"), ["Host", "x"]) self.assertEqual(checker.broken, False) mock_exists.return_value = False mock_popen.call_count = 0 - checker = ocsp.RevocationChecker(self.config) + checker = ocsp.RevocationChecker() self.assertEqual(mock_popen.call_count, 0) self.assertEqual(mock_log.call_count, 1) self.assertEqual(checker.broken, True) - def test_ocsp_status(self): - from certbot import ocsp - checker = self.checker - checker.check_ocsp = mock.MagicMock() - checker.check_ocsp.return_value = "octopus found in certificate" + @mock.patch('certbot.ocsp.RevocationChecker.determine_ocsp_server') + @mock.patch('certbot.util.run_script') + def test_ocsp_revoked(self, mock_run, mock_determine): + self.checker.broken = True + mock_determine.return_value = ("", "") + self.assertEqual(self.checker.ocsp_revoked("x", "y"), None) - checker.config.check_ocsp = "never" - self.assertEqual(checker.ocsp_status("a", "a", "xyz"), "xyz") - self.assertEqual(checker.ocsp_status("a", "a", ""), "") - self.assertEqual(checker.check_ocsp.call_count, 0) + self.checker.broken = False + mock_run.return_value = tuple(openssl_happy[1:]) + self.assertEqual(self.checker.ocsp_revoked("x", "y"), None) + self.assertEqual(mock_run.call_count, 0) + + mock_determine.return_value = ("http://x.co", "x.co") + self.assertEqual(self.checker.ocsp_revoked("blah.pem", "chain.pem"), False) - checker.config.check_ocsp = "lazy" - self.assertEqual(checker.ocsp_status("a", "a", "xyz"), "xyz") - self.assertEqual(checker.check_ocsp.call_count, 0) - self.assertEqual(checker.ocsp_status("a", "a", ""), "INVALID: REVOKED") - checker.config.check_ocsp = "always" - self.assertEqual(checker.ocsp_status("a", "a", "xyz"), "xyz,REVOKED") - checker.check_ocsp.return_value = "" - self.assertEqual(checker.ocsp_status("a", "a", "xyz"), "xyz") - - @mock.patch('certbot.ocsp.logger.debug') @mock.patch('certbot.ocsp.logger.info') @mock.patch('certbot.util.run_script') @@ -81,31 +78,32 @@ class OCSPTest(unittest.TestCase): uri = "http://ocsp.stg-int-x1.letsencrypt.org/" host = "ocsp.stg-int-x1.letsencrypt.org" mock_run.return_value = uri, "" - self.assertEquals(self.checker.determine_ocsp_server("beep"), (uri, host)) + self.assertEqual(self.checker.determine_ocsp_server("beep"), (uri, host)) mock_run.return_value = "ftp:/" + host + "/", "" - self.assertEquals(self.checker.determine_ocsp_server("beep"), (None, None)) - self.assertEquals(mock_info.call_count, 1) + self.assertEqual(self.checker.determine_ocsp_server("beep"), (None, None)) + self.assertEqual(mock_info.call_count, 1) c = "confusion" mock_run.side_effect = errors.SubprocessError(c) - self.assertEquals(self.checker.determine_ocsp_server("beep"), (None, None)) + self.assertEqual(self.checker.determine_ocsp_server("beep"), (None, None)) self.assertTrue(c in mock_debug.call_args[0][1]) @mock.patch('certbot.ocsp.logger') @mock.patch('certbot.util.run_script') def test_translate_ocsp(self, mock_run, mock_log): - # pylint: disable=protected-access + # pylint: disable=protected-access,star-args mock_run.return_value = openssl_confused from certbot import ocsp - self.assertEquals(ocsp._translate_ocsp_query(*openssl_happy), "") - self.assertEquals(ocsp._translate_ocsp_query(*openssl_confused), "") - self.assertEquals(mock_log.debug.call_count, 1) - self.assertEquals(mock_log.warn.call_count, 0) - self.assertEquals(ocsp._translate_ocsp_query(*openssl_broken), "") - self.assertEquals(mock_log.warn.call_count, 1) - self.assertEquals(ocsp._translate_ocsp_query(*openssl_revoked), "REVOKED") + self.assertEqual(ocsp._translate_ocsp_query(*openssl_happy), False) + self.assertEqual(ocsp._translate_ocsp_query(*openssl_confused), None) + self.assertEqual(mock_log.debug.call_count, 1) + self.assertEqual(mock_log.warn.call_count, 0) + self.assertEqual(ocsp._translate_ocsp_query(*openssl_broken), None) + self.assertEqual(mock_log.warn.call_count, 1) + self.assertEqual(ocsp._translate_ocsp_query(*openssl_revoked), True) +# pylint: disable=line-too-long openssl_confused = ("", """ /etc/letsencrypt/live/example.org/cert.pem: good This Update: Dec 17 00:00:00 2016 GMT