From 15d2a0ffde27f9563ca272d78629b7d23e99d685 Mon Sep 17 00:00:00 2001 From: James Kasten Date: Wed, 21 Dec 2016 14:36:51 -0800 Subject: [PATCH 01/23] Import OCSP code from the historical cert_manager branch (This is pde committing jdkasten's code) --- certbot/ocsp.py | 59 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 certbot/ocsp.py diff --git a/certbot/ocsp.py b/certbot/ocsp.py new file mode 100644 index 000000000..d7fc06e0d --- /dev/null +++ b/certbot/ocsp.py @@ -0,0 +1,59 @@ +import logging + +from letsencrypt import errors +from letsencrypt import le_util + + +logger = logging.getLogger(__name__) + + +REV_LABEL = "**Revoked**" +EXP_LABEL = "**Expired**" + +def revoked_status(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 chain certificate + + """ + url, _ = le_util.run_script( + ["openssl", "x509", "-in", cert_path, "-noout", "-ocsp_uri"]) + + url = url.rstrip() + host = url.partition("://")[2].rstrip("/") + if not host: + raise errors.Error( + "Unable to get OCSP host from cert, url - %s", url) + + # This was a PITA... + # Thanks to "Bulletproof SSL and TLS - Ivan Ristic" for helping me out + try: + output, _ = le_util.run_script( + ["openssl", "ocsp", + "-no_nonce", "-header", "Host", host, + "-issuer", chain_path, + "-cert", cert_path, + "-url", url, + "-CAfile", chain_path]) + except errors.SubprocessError: + return "(OCSP Failure)" + + return _translate_ocsp_query(cert_path, output) + + +def _translate_ocsp_query(cert_path, ocsp_output): + """Returns a label string out of the query.""" + if not "Response verify OK": + return "Revocation Unknown" + if cert_path + ": good" in ocsp_output: + return "" + elif cert_path + ": revoked" in ocsp_output: + return REV_LABEL + else: + raise errors.Error( + "Unable to properly parse OCSP output: %s", ocsp_output) + + From 40e29bb95f5ee64f55bea25be94a3b3341c99b53 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 21 Dec 2016 14:38:20 -0800 Subject: [PATCH 02/23] begin implementing OCSP checking for "certificates" --- certbot/cert_manager.py | 8 ++++++++ certbot/ocsp.py | 14 ++++++++------ 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/certbot/cert_manager.py b/certbot/cert_manager.py index 35b12e1bb..1b6d441c7 100644 --- a/certbot/cert_manager.py +++ b/certbot/cert_manager.py @@ -8,6 +8,7 @@ import zope.component from certbot import errors from certbot import interfaces +from certbot import ocsp from certbot import storage from certbot import util @@ -170,11 +171,17 @@ def _report_human_readable(parsed_certs): certinfo = [] for cert in parsed_certs: now = pytz.UTC.fromutc(datetime.datetime.utcnow()) + expiration_text = "" if cert.is_test_cert: expiration_text = "INVALID: TEST CERT" elif cert.target_expiry <= now: expiration_text = "INVALID: EXPIRED" else: + revoked = ocsp.revoked_status(cert.cert, cert.chain) + if revoked: + expiration_text = "INVALID: " + revoked + + if not expiration_text: diff = cert.target_expiry - now if diff.days == 1: expiration_text = "VALID: 1 day" @@ -182,6 +189,7 @@ def _report_human_readable(parsed_certs): expiration_text = "VALID: {0} hour(s)".format(diff.seconds // 3600) else: 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" diff --git a/certbot/ocsp.py b/certbot/ocsp.py index d7fc06e0d..cb3dd0610 100644 --- a/certbot/ocsp.py +++ b/certbot/ocsp.py @@ -1,8 +1,8 @@ +"""Tools for checking certificate revocation.""" import logging -from letsencrypt import errors -from letsencrypt import le_util - +from certbot import errors +from certbot import util logger = logging.getLogger(__name__) @@ -10,6 +10,9 @@ logger = logging.getLogger(__name__) REV_LABEL = "**Revoked**" EXP_LABEL = "**Expired**" +INSTALL_LABEL = "(Installed)" + + def revoked_status(cert_path, chain_path): """Get revoked status for a particular cert version. @@ -19,7 +22,7 @@ def revoked_status(cert_path, chain_path): :param str chain_path: Path to chain certificate """ - url, _ = le_util.run_script( + url, _ = util.run_script( ["openssl", "x509", "-in", cert_path, "-noout", "-ocsp_uri"]) url = url.rstrip() @@ -31,7 +34,7 @@ def revoked_status(cert_path, chain_path): # This was a PITA... # Thanks to "Bulletproof SSL and TLS - Ivan Ristic" for helping me out try: - output, _ = le_util.run_script( + output, _ = util.run_script( ["openssl", "ocsp", "-no_nonce", "-header", "Host", host, "-issuer", chain_path, @@ -56,4 +59,3 @@ def _translate_ocsp_query(cert_path, ocsp_output): raise errors.Error( "Unable to properly parse OCSP output: %s", ocsp_output) - From ac02cd9cb87ad8cd9e8080f13d7ccd36d9f0702b Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Mon, 19 Dec 2016 17:36:37 -0800 Subject: [PATCH 03/23] ocsp checking needs -verify_other https://community.letsencrypt.org/t/unable-to-verify-ocsp-response/7264 --- certbot/ocsp.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/certbot/ocsp.py b/certbot/ocsp.py index cb3dd0610..f4c986609 100644 --- a/certbot/ocsp.py +++ b/certbot/ocsp.py @@ -40,9 +40,10 @@ def revoked_status(cert_path, chain_path): "-issuer", chain_path, "-cert", cert_path, "-url", url, - "-CAfile", chain_path]) + "-CAfile", chain_path, + "-verify_other", chain_path]) except errors.SubprocessError: - return "(OCSP Failure)" + return "OCSP Failure" return _translate_ocsp_query(cert_path, output) From 245b84ab783488002a11f82c735b1fd6d31614c3 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Mon, 19 Dec 2016 17:45:21 -0800 Subject: [PATCH 04/23] Format CLI to keep modern openssls happy - This is somewhat ominous --- certbot/ocsp.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/certbot/ocsp.py b/certbot/ocsp.py index f4c986609..5a66ba48d 100644 --- a/certbot/ocsp.py +++ b/certbot/ocsp.py @@ -7,8 +7,7 @@ from certbot import util logger = logging.getLogger(__name__) -REV_LABEL = "**Revoked**" -EXP_LABEL = "**Expired**" +REV_LABEL = "REVOKED" INSTALL_LABEL = "(Installed)" @@ -36,7 +35,7 @@ def revoked_status(cert_path, chain_path): try: output, _ = util.run_script( ["openssl", "ocsp", - "-no_nonce", "-header", "Host", host, + "-no_nonce", "-header", "Host="+host, "-issuer", chain_path, "-cert", cert_path, "-url", url, From fe36e336a8bca509c9657904d7e9972682b028e8 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Mon, 19 Dec 2016 18:37:01 -0800 Subject: [PATCH 05/23] Run with both old and new versions of openssl --- certbot/ocsp.py | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/certbot/ocsp.py b/certbot/ocsp.py index 5a66ba48d..21b5d3be6 100644 --- a/certbot/ocsp.py +++ b/certbot/ocsp.py @@ -1,6 +1,8 @@ """Tools for checking certificate revocation.""" import logging +from subprocess import Popen, PIPE + from certbot import errors from certbot import util @@ -21,6 +23,7 @@ def revoked_status(cert_path, chain_path): :param str chain_path: Path to chain certificate """ + url, _ = util.run_script( ["openssl", "x509", "-in", cert_path, "-noout", "-ocsp_uri"]) @@ -30,12 +33,21 @@ def revoked_status(cert_path, chain_path): raise errors.Error( "Unable to get OCSP host from cert, url - %s", url) - # This was a PITA... - # Thanks to "Bulletproof SSL and TLS - Ivan Ristic" for helping me out + # New versions of openssl want -header var=val, old ones want -header var val + test_host_format = Popen(["openssl", "ocsp", "-header", "var", "val"], + stdout=PIPE, stderr=PIPE) + _out, err = test_host_format.communicate() + if "Missing =" in err: + host_arg = ["Host=" + host] + else: + host_arg = ["Host", host] + + # jdkasten thanks "Bulletproof SSL and TLS - Ivan Ristic" for documenting this! try: output, _ = util.run_script( ["openssl", "ocsp", - "-no_nonce", "-header", "Host="+host, + "-no_nonce", + "-header"] + host_arg + [ "-issuer", chain_path, "-cert", cert_path, "-url", url, From 7a18a124cec768f3405ea82667a1bdcec7566c10 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Mon, 19 Dec 2016 19:45:39 -0800 Subject: [PATCH 06/23] Better error handling --- certbot/cert_manager.py | 2 +- certbot/ocsp.py | 61 +++++++++++++++++++++++++++-------------- certbot/util.py | 7 +++-- 3 files changed, 45 insertions(+), 25 deletions(-) diff --git a/certbot/cert_manager.py b/certbot/cert_manager.py index 1b6d441c7..4b2fc069f 100644 --- a/certbot/cert_manager.py +++ b/certbot/cert_manager.py @@ -172,12 +172,12 @@ def _report_human_readable(parsed_certs): for cert in parsed_certs: now = pytz.UTC.fromutc(datetime.datetime.utcnow()) expiration_text = "" + revoked = ocsp.revoked_status(cert.cert, cert.chain) if cert.is_test_cert: expiration_text = "INVALID: TEST CERT" elif cert.target_expiry <= now: expiration_text = "INVALID: EXPIRED" else: - revoked = ocsp.revoked_status(cert.cert, cert.chain) if revoked: expiration_text = "INVALID: " + revoked diff --git a/certbot/ocsp.py b/certbot/ocsp.py index 21b5d3be6..f1b2d7d32 100644 --- a/certbot/ocsp.py +++ b/certbot/ocsp.py @@ -24,50 +24,69 @@ def revoked_status(cert_path, chain_path): """ - url, _ = util.run_script( - ["openssl", "x509", "-in", cert_path, "-noout", "-ocsp_uri"]) + if revoked_status.broken: + return False + + if not util.exe_exists("openssl"): + logging.info("openssl not installed, can't check revocation") + revoked_status.broken = True + return False + + try: + url, err = util.run_script( + ["openssl", "x509", "-in", cert_path, "-noout", "-ocsp_uri"], + log=logging.debug) + except errors.SubprocessError: + logger.info("Cannot extract OCSP URI from %s", cert_path) + return False url = url.rstrip() host = url.partition("://")[2].rstrip("/") if not host: - raise errors.Error( - "Unable to get OCSP host from cert, url - %s", url) + logger.info("Cannot process OCSP host from URL (%s) in cert at %s", url, cert_path) + return False # New versions of openssl want -header var=val, old ones want -header var val test_host_format = Popen(["openssl", "ocsp", "-header", "var", "val"], stdout=PIPE, stderr=PIPE) _out, err = test_host_format.communicate() if "Missing =" in err: - host_arg = ["Host=" + host] + host_args = ["Host=" + host] else: - host_arg = ["Host", host] + host_args = ["Host", host] # jdkasten thanks "Bulletproof SSL and TLS - Ivan Ristic" for documenting this! try: - output, _ = util.run_script( - ["openssl", "ocsp", - "-no_nonce", - "-header"] + host_arg + [ - "-issuer", chain_path, - "-cert", cert_path, - "-url", url, - "-CAfile", chain_path, - "-verify_other", chain_path]) + cmd = ["openssl", "ocsp", + "-no_nonce", + "-issuer", chain_path, + "-cert", cert_path, + "-url", url, + "-CAfile", chain_path, + "-verify_other", chain_path, + "-header"] + host_args + output, err = util.run_script(cmd, log=logging.debug) except errors.SubprocessError: - return "OCSP Failure" + logger.info("OCSP querying seems to be broken, assuming nothing is revoked...") + logger.debug("Command was:\n%s\nError was:\n%s", " ".join(cmd), err) + revoked_status.broken = True + return False - return _translate_ocsp_query(cert_path, output) + return _translate_ocsp_query(cert_path, output, err) +revoked_status.broken = False -def _translate_ocsp_query(cert_path, ocsp_output): +def _translate_ocsp_query(cert_path, ocsp_output, ocsp_errors): """Returns a label string out of the query.""" if not "Response verify OK": - return "Revocation Unknown" + logger.info("Revocation status for %s is unknown", cert_path) + logger.debug("Uncertain ouput:\n%s\nstderr:\n%s", ocsp_output, ocsp_errors) + return "" if cert_path + ": good" in ocsp_output: return "" elif cert_path + ": revoked" in ocsp_output: return REV_LABEL else: - raise errors.Error( - "Unable to properly parse OCSP output: %s", ocsp_output) + logger.warn("Unable to properly parse OCSP output: %s", ocsp_output) + return "" diff --git a/certbot/util.py b/certbot/util.py index cbcfa3314..81a9beca1 100644 --- a/certbot/util.py +++ b/certbot/util.py @@ -38,10 +38,11 @@ ANSI_SGR_RED = "\033[31m" ANSI_SGR_RESET = "\033[0m" -def run_script(params): +def run_script(params, log=logger.error): """Run the script with the given params. :param list params: List of parameters to pass to Popen + :param logging.Logger log: Logger to use for errors """ try: @@ -51,7 +52,7 @@ def run_script(params): except (OSError, ValueError): msg = "Unable to run the command: %s" % " ".join(params) - logger.error(msg) + log(msg) raise errors.SubprocessError(msg) stdout, stderr = proc.communicate() @@ -60,7 +61,7 @@ def run_script(params): msg = "Error while running %s.\n%s\n%s" % ( " ".join(params), stdout, stderr) # Enter recovery routine... - logger.error(msg) + log(msg) raise errors.SubprocessError(msg) return stdout, stderr From 840c584cbdf8128c3e8715324bf27c59bd7f5bda Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 20 Dec 2016 09:29:05 -0800 Subject: [PATCH 07/23] Make the OCSP checker a class (Since it contains a reasonable amount of system state) --- certbot/cert_manager.py | 7 ++- certbot/cli.py | 4 ++ certbot/ocsp.py | 108 ++++++++++++++++++++++++---------------- 3 files changed, 73 insertions(+), 46 deletions(-) diff --git a/certbot/cert_manager.py b/certbot/cert_manager.py index 4b2fc069f..59b46fe07 100644 --- a/certbot/cert_manager.py +++ b/certbot/cert_manager.py @@ -169,11 +169,14 @@ def _report_lines(msgs): def _report_human_readable(parsed_certs): """Format a results report for a parsed cert""" certinfo = [] + checker = ocsp.RevocationChecker() for cert in parsed_certs: now = pytz.UTC.fromutc(datetime.datetime.utcnow()) expiration_text = "" - revoked = ocsp.revoked_status(cert.cert, cert.chain) - if cert.is_test_cert: + revoked = checker.check_ocsp(cert.cert, cert.chain) + if revoked: + expiration_text = "INVALID: " + revoked + elif cert.is_test_cert: expiration_text = "INVALID: TEST CERT" elif cert.target_expiry <= now: expiration_text = "INVALID: EXPIRED" diff --git a/certbot/cli.py b/certbot/cli.py index 0adb7a4b5..fbcc8ff42 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -943,6 +943,10 @@ 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( + ["testing", "certificates"], "--check-ocsp", default="if otherwise valid", + help='Whether to check OCSP for listed certs. Can be set to "never", "always",' + 'or "if 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 f1b2d7d32..f2fe1188f 100644 --- a/certbot/ocsp.py +++ b/certbot/ocsp.py @@ -13,50 +13,43 @@ REV_LABEL = "REVOKED" INSTALL_LABEL = "(Installed)" +class RevocationChecker(object): + "This class figures out OCSP checking on this system, and performs it." -def revoked_status(cert_path, chain_path): - """Get revoked status for a particular cert version. + def __init__(self): + self.broken = False - .. todo:: Make this a non-blocking call + if not util.exe_exists("openssl"): + logging.info("openssl not installed, can't check revocation") + self.broken = True - :param str cert_path: Path to certificate - :param str chain_path: Path to chain certificate + # New versions of openssl want -header var=val, old ones want -header var val + test_host_format = Popen(["openssl", "ocsp", "-header", "var", "val"], + stdout=PIPE, stderr=PIPE) + _out, err = test_host_format.communicate() + if "Missing =" in err: + self.host_args = lambda host: ["Host=" + host] + else: + self.host_args = lambda host: ["Host", host] - """ - if revoked_status.broken: - return False + def check_ocsp(self, cert_path, chain_path): + """Get revoked status for a particular cert version. - if not util.exe_exists("openssl"): - logging.info("openssl not installed, can't check revocation") - revoked_status.broken = True - return False + .. todo:: Make this a non-blocking call - try: - url, err = util.run_script( - ["openssl", "x509", "-in", cert_path, "-noout", "-ocsp_uri"], - log=logging.debug) - except errors.SubprocessError: - logger.info("Cannot extract OCSP URI from %s", cert_path) - return False + :param str cert_path: Path to certificate + :param str chain_path: Path to intermediate cert - url = url.rstrip() - host = url.partition("://")[2].rstrip("/") - if not host: - logger.info("Cannot process OCSP host from URL (%s) in cert at %s", url, cert_path) - return False + """ + if self.broken: + return False - # New versions of openssl want -header var=val, old ones want -header var val - test_host_format = Popen(["openssl", "ocsp", "-header", "var", "val"], - stdout=PIPE, stderr=PIPE) - _out, err = test_host_format.communicate() - if "Missing =" in err: - host_args = ["Host=" + host] - else: - host_args = ["Host", host] - - # jdkasten thanks "Bulletproof SSL and TLS - Ivan Ristic" for documenting this! - try: + logger.debug("Querying OCSP for %s", cert_path) + url, host = self.determine_ocsp_server(cert_path) + if not host: + return False + # jdkasten thanks "Bulletproof SSL and TLS - Ivan Ristic" for documenting this! cmd = ["openssl", "ocsp", "-no_nonce", "-issuer", chain_path, @@ -64,16 +57,43 @@ def revoked_status(cert_path, chain_path): "-url", url, "-CAfile", chain_path, "-verify_other", chain_path, - "-header"] + host_args - output, err = util.run_script(cmd, log=logging.debug) - except errors.SubprocessError: - logger.info("OCSP querying seems to be broken, assuming nothing is revoked...") - logger.debug("Command was:\n%s\nError was:\n%s", " ".join(cmd), err) - revoked_status.broken = True - return False + "-header"] + self.host_args(host) + try: + output, err = util.run_script(cmd, log=logging.debug) + except errors.SubprocessError, e: + logger.info("We're offline, or OCSP querying is broken. " + "Assuming nothing is revoked...") + logger.debug("Command was:\n%s\nError was:\n%s", " ".join(cmd), e) + self.broken = True + return False - return _translate_ocsp_query(cert_path, output, err) -revoked_status.broken = False + return _translate_ocsp_query(cert_path, output, err) + + + def determine_ocsp_server(self, cert_path): + """Extract the OCSP server host from a certificate. + + :param str cert_path: Path to the cert we're checking OCSP for + :rtype tuple: + :returns: (OCSP server URL or None, OCSP server host or None) + + """ + try: + url, err = util.run_script( + ["openssl", "x509", "-in", cert_path, "-noout", "-ocsp_uri"], + log=logging.debug) + except errors.SubprocessError: + logger.info("Cannot extract OCSP URI from %s", cert_path) + logger.debug("Error was:\n%s", err) + return None, None + + url = url.rstrip() + host = url.partition("://")[2].rstrip("/") + if host: + return url, host + else: + 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): From e5e5db24d72456632d6724ab6693a1e751385329 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 20 Dec 2016 14:20:15 -0800 Subject: [PATCH 08/23] CLI flag for controlling ocsp checking now works --- certbot/cert_manager.py | 36 +++++++++++++----------------- certbot/cli.py | 4 ++-- certbot/ocsp.py | 27 +++++++++++++++++++++- certbot/tests/cert_manager_test.py | 12 +++++----- 4 files changed, 51 insertions(+), 28 deletions(-) diff --git a/certbot/cert_manager.py b/certbot/cert_manager.py index 59b46fe07..ed8c6e76d 100644 --- a/certbot/cert_manager.py +++ b/certbot/cert_manager.py @@ -81,7 +81,7 @@ def certificates(config): parse_failures.append(renewal_file) # Describe all the certs - _describe_certs(parsed_certs, parse_failures) + _describe_certs(config, parsed_certs, parse_failures) def delete(config): """Delete Certbot files associated with a certificate lineage.""" @@ -166,34 +166,30 @@ def _report_lines(msgs): """Format a results report for a category of single-line renewal outcomes""" return " " + "\n ".join(str(msg) for msg in msgs) -def _report_human_readable(parsed_certs): +def _report_human_readable(config, parsed_certs): """Format a results report for a parsed cert""" certinfo = [] - checker = ocsp.RevocationChecker() + checker = ocsp.RevocationChecker(config) for cert in parsed_certs: now = pytz.UTC.fromutc(datetime.datetime.utcnow()) - expiration_text = "" - revoked = checker.check_ocsp(cert.cert, cert.chain) - if revoked: - expiration_text = "INVALID: " + revoked - elif cert.is_test_cert: - expiration_text = "INVALID: TEST CERT" - elif cert.target_expiry <= now: - expiration_text = "INVALID: EXPIRED" + status = "" + if cert.is_test_cert: + status = "INVALID: TEST_CERT" + if cert.target_expiry <= now: + status = status + ",EXPIRED" if status else "INVALID: EXPIRED" else: - if revoked: - expiration_text = "INVALID: " + revoked + status = checker.ocsp_status(cert.cert, cert.chain, status) - if not expiration_text: + if not status: diff = cert.target_expiry - now if diff.days == 1: - expiration_text = "VALID: 1 day" + status = "VALID: 1 day" elif diff.days < 1: - expiration_text = "VALID: {0} hour(s)".format(diff.seconds // 3600) + status = "VALID: {0} hour(s)".format(diff.seconds // 3600) else: - expiration_text = "VALID: {0} days".format(diff.days) + status = "VALID: {0} days".format(diff.days) - valid_string = "{0} ({1})".format(cert.target_expiry, expiration_text) + valid_string = "{0} ({1})".format(cert.target_expiry, status) certinfo.append(" Certificate Name: {0}\n" " Domains: {1}\n" " Expiry Date: {2}\n" @@ -206,7 +202,7 @@ def _report_human_readable(parsed_certs): cert.privkey)) return "\n".join(certinfo) -def _describe_certs(parsed_certs, parse_failures): +def _describe_certs(config, parsed_certs, parse_failures): """Print information about the certs we know about""" out = [] @@ -217,7 +213,7 @@ def _describe_certs(parsed_certs, parse_failures): else: if parsed_certs: notify("Found the following certs:") - notify(_report_human_readable(parsed_certs)) + notify(_report_human_readable(config, parsed_certs)) if parse_failures: notify("\nThe following renewal configuration files " "were invalid:") diff --git a/certbot/cli.py b/certbot/cli.py index fbcc8ff42..cea8af51b 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -944,9 +944,9 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis help=config_help("no_verify_ssl"), default=flag_default("no_verify_ssl")) helpful.add( - ["testing", "certificates"], "--check-ocsp", default="if otherwise valid", + ["certificates", "testing"], "--check-ocsp", default="always", help='Whether to check OCSP for listed certs. Can be set to "never", "always",' - 'or "if otherwise valid".') + ' 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 f2fe1188f..c04afcc82 100644 --- a/certbot/ocsp.py +++ b/certbot/ocsp.py @@ -16,8 +16,9 @@ INSTALL_LABEL = "(Installed)" class RevocationChecker(object): "This class figures out OCSP checking on this system, and performs it." - def __init__(self): + def __init__(self, config): self.broken = False + self.config = config if not util.exe_exists("openssl"): logging.info("openssl not installed, can't check revocation") @@ -33,6 +34,30 @@ 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.lower() == "never": + return status_in + elif self.config.check_ocsp.lower() == "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): """Get revoked status for a particular cert version. diff --git a/certbot/tests/cert_manager_test.py b/certbot/tests/cert_manager_test.py index ae673fba1..7f4261fd0 100644 --- a/certbot/tests/cert_manager_test.py +++ b/certbot/tests/cert_manager_test.py @@ -189,31 +189,33 @@ class CertificatesTest(BaseCertManagerTest): cert.names.return_value = ["nameone", "nametwo"] cert.is_test_cert = False parsed_certs = [cert] + + mock_config = mock.MagicMock() # pylint: disable=protected-access - out = cert_manager._report_human_readable(parsed_certs) + out = cert_manager._report_human_readable(mock_config, parsed_certs) 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) + out = cert_manager._report_human_readable(mock_config, parsed_certs) 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) + out = cert_manager._report_human_readable(mock_config, 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) + out = cert_manager._report_human_readable(mock_config, 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) + out = cert_manager._report_human_readable(mock_config, parsed_certs) self.assertTrue('INVALID: TEST CERT' in out) From 03f312e653e62b41ae639cc2878b5bf6536718c3 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 20 Dec 2016 14:20:35 -0800 Subject: [PATCH 09/23] Allow filtering of "certbot certificates output" with --config-name or -d --- certbot/cert_manager.py | 7 ++++++- certbot/cli.py | 10 ++++++---- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/certbot/cert_manager.py b/certbot/cert_manager.py index ed8c6e76d..86b5eedd9 100644 --- a/certbot/cert_manager.py +++ b/certbot/cert_manager.py @@ -171,6 +171,10 @@ def _report_human_readable(config, parsed_certs): certinfo = [] checker = ocsp.RevocationChecker(config) 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: @@ -212,7 +216,8 @@ def _describe_certs(config, parsed_certs, parse_failures): notify("No certs found.") else: if parsed_certs: - notify("Found the following certs:") + match = "matching " if config.certname or config.domains else "" + notify("Found the following {0}certs:".format(match)) notify(_report_human_readable(config, parsed_certs)) if parse_failures: notify("\nThe following renewal configuration files " diff --git a/certbot/cli.py b/certbot/cli.py index cea8af51b..8e6d7910f 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -350,8 +350,10 @@ VERB_HELP = [ "usage": "\n\n certbot renew [--cert-name NAME] [options]\n\n" }), ("certificates", { - "short": "List all certificates managed by Certbot", - "opts": "List all certificates managed by Certbot" + "short": "List certificates managed by Certbot", + "opts": "List certificates managed by Certbot", + "usage": ("\n\n certbot certificates [options] ...\n\n" + "Print information about the status of certificates managed by Certbot.") }), ("delete", { "short": "Clean up all files related to a certificate", @@ -824,14 +826,14 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis "being run in a terminal. This flag cannot be used with the " "renew subcommand.") helpful.add( - [None, "run", "certonly"], + [None, "run", "certonly", "certificates"], "-d", "--domains", "--domain", dest="domains", metavar="DOMAIN", action=_DomainsAction, default=[], 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. (default: Ask)") helpful.add( - [None, "run", "certonly", "manage", "rename", "delete"], + [None, "run", "certonly", "manage", "rename", "delete", "certificates"], "--cert-name", dest="certname", metavar="CERTNAME", default=None, help="Certificate name to apply. Only one certificate name can be used " From 15ed372df617caa8342493763b461b144eeb66b8 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 20 Dec 2016 14:32:39 -0800 Subject: [PATCH 10/23] Fix existing tests --- certbot/tests/cert_manager_test.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/certbot/tests/cert_manager_test.py b/certbot/tests/cert_manager_test.py index 7f4261fd0..4fd66c34a 100644 --- a/certbot/tests/cert_manager_test.py +++ b/certbot/tests/cert_manager_test.py @@ -179,7 +179,9 @@ class CertificatesTest(BaseCertManagerTest): self.assertTrue(mock_utility.called) shutil.rmtree(tempdir) - def test_report_human_readable(self): + @mock.patch('certbot.cert_manager.ocsp.RevocationChecker.ocsp_status') + def test_report_human_readable(self, mock_ocsp): + mock_ocsp.side_effect = lambda _cert, _chain, status: status from certbot import cert_manager import datetime, pytz expiry = pytz.UTC.fromutc(datetime.datetime.utcnow()) @@ -190,7 +192,7 @@ class CertificatesTest(BaseCertManagerTest): cert.is_test_cert = False parsed_certs = [cert] - mock_config = mock.MagicMock() + mock_config = mock.MagicMock(certname=None, lineagename=None) # pylint: disable=protected-access out = cert_manager._report_human_readable(mock_config, parsed_certs) self.assertTrue("INVALID: EXPIRED" in out) @@ -216,7 +218,7 @@ class CertificatesTest(BaseCertManagerTest): cert.is_test_cert = True out = cert_manager._report_human_readable(mock_config, parsed_certs) - self.assertTrue('INVALID: TEST CERT' in out) + self.assertTrue('INVALID: TEST_CERT' in out) class SearchLineagesTest(BaseCertManagerTest): From bf6084db615afa28c5316c81ea146f869fd32d30 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 20 Dec 2016 14:41:34 -0800 Subject: [PATCH 11/23] With mixed staging/prod lineages, it might not be correct to stop OCSPing - One lineage might fail, and a later one succeed --- certbot/ocsp.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/certbot/ocsp.py b/certbot/ocsp.py index c04afcc82..56074fa45 100644 --- a/certbot/ocsp.py +++ b/certbot/ocsp.py @@ -86,10 +86,8 @@ class RevocationChecker(object): try: output, err = util.run_script(cmd, log=logging.debug) except errors.SubprocessError, e: - logger.info("We're offline, or OCSP querying is broken. " - "Assuming nothing is revoked...") + logger.info("We're offline, or OCSP querying is broken. ") logger.debug("Command was:\n%s\nError was:\n%s", " ".join(cmd), e) - self.broken = True return False return _translate_ocsp_query(cert_path, output, err) From 011f6055d4a4a011e166af195af5681be19c395a Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 20 Dec 2016 14:43:35 -0800 Subject: [PATCH 12/23] Better message --- certbot/ocsp.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot/ocsp.py b/certbot/ocsp.py index 56074fa45..c26c8c1f3 100644 --- a/certbot/ocsp.py +++ b/certbot/ocsp.py @@ -86,7 +86,7 @@ class RevocationChecker(object): try: output, err = util.run_script(cmd, log=logging.debug) except errors.SubprocessError, e: - logger.info("We're offline, or OCSP querying is broken. ") + 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 From fcf7387c3d901ebe9223147805fab70c533c9805 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 20 Dec 2016 14:54:48 -0800 Subject: [PATCH 13/23] Don't crash if openssl is missing --- certbot/ocsp.py | 1 + 1 file changed, 1 insertion(+) diff --git a/certbot/ocsp.py b/certbot/ocsp.py index c26c8c1f3..92dc6f872 100644 --- a/certbot/ocsp.py +++ b/certbot/ocsp.py @@ -23,6 +23,7 @@ class RevocationChecker(object): if not util.exe_exists("openssl"): logging.info("openssl not installed, can't check revocation") self.broken = True + return # New versions of openssl want -header var=val, old ones want -header var val test_host_format = Popen(["openssl", "ocsp", "-header", "var", "val"], From 7d02b8dbd541e2f35b325a5404b768d29aa36f48 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 20 Dec 2016 15:00:21 -0800 Subject: [PATCH 14/23] py3fix --- certbot/ocsp.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot/ocsp.py b/certbot/ocsp.py index 92dc6f872..758fb2e2f 100644 --- a/certbot/ocsp.py +++ b/certbot/ocsp.py @@ -86,7 +86,7 @@ class RevocationChecker(object): "-header"] + self.host_args(host) try: output, err = util.run_script(cmd, log=logging.debug) - except errors.SubprocessError, e: + 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 From 509f4029bb0b8c68a4c7eb26b1a6d3145734ba04 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 20 Dec 2016 15:07:43 -0800 Subject: [PATCH 15/23] more py3 fixes --- certbot/ocsp.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot/ocsp.py b/certbot/ocsp.py index 758fb2e2f..9049ddc01 100644 --- a/certbot/ocsp.py +++ b/certbot/ocsp.py @@ -27,7 +27,7 @@ class RevocationChecker(object): # New versions of openssl want -header var=val, old ones want -header var val test_host_format = Popen(["openssl", "ocsp", "-header", "var", "val"], - stdout=PIPE, stderr=PIPE) + stdout=PIPE, stderr=PIPE, universal_newlines=True) _out, err = test_host_format.communicate() if "Missing =" in err: self.host_args = lambda host: ["Host=" + host] From f495863da975341474afc2389e5c1022e8cac330 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 20 Dec 2016 15:38:37 -0800 Subject: [PATCH 16/23] Check --check-ocsp flags, and test those checks --- certbot/cli.py | 4 ++++ certbot/tests/cli_test.py | 4 ++++ certbot/util.py | 3 ++- 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/certbot/cli.py b/certbot/cli.py index 8e6d7910f..19d917739 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -529,6 +529,10 @@ 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( diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py index e3bd28a5e..1714dcdbb 100644 --- a/certbot/tests/cli_test.py +++ b/certbot/tests/cli_test.py @@ -268,6 +268,10 @@ 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/util.py b/certbot/util.py index 81a9beca1..cc0a74bd2 100644 --- a/certbot/util.py +++ b/certbot/util.py @@ -48,7 +48,8 @@ def run_script(params, log=logger.error): try: proc = subprocess.Popen(params, stdout=subprocess.PIPE, - stderr=subprocess.PIPE) + stderr=subprocess.PIPE, + universal_newlines=True) except (OSError, ValueError): msg = "Unable to run the command: %s" % " ".join(params) From 76b8c53566cb979569b2368a6394e8bc95fd6c09 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 20 Dec 2016 16:22:14 -0800 Subject: [PATCH 17/23] Tests for ocsp.py, and associated fixes --- certbot/ocsp.py | 10 +-- certbot/tests/cli_test.py | 2 +- certbot/tests/ocsp_test.py | 137 +++++++++++++++++++++++++++++++++++++ 3 files changed, 143 insertions(+), 6 deletions(-) create mode 100644 certbot/tests/ocsp_test.py diff --git a/certbot/ocsp.py b/certbot/ocsp.py index 9049ddc01..f24d94266 100644 --- a/certbot/ocsp.py +++ b/certbot/ocsp.py @@ -45,9 +45,9 @@ class RevocationChecker(object): :returns: a new status including revocation, if the cert is revoked.""" - if self.config.check_ocsp.lower() == "never": + if self.config.check_ocsp == "never": return status_in - elif self.config.check_ocsp.lower() == "lazy" and status_in: + elif self.config.check_ocsp == "lazy" and status_in: return status_in revoked = self.check_ocsp(cert_path, chain_path) @@ -106,9 +106,9 @@ class RevocationChecker(object): url, err = util.run_script( ["openssl", "x509", "-in", cert_path, "-noout", "-ocsp_uri"], log=logging.debug) - except errors.SubprocessError: + except errors.SubprocessError as e: logger.info("Cannot extract OCSP URI from %s", cert_path) - logger.debug("Error was:\n%s", err) + logger.debug("Error was:\n%s", e) return None, None url = url.rstrip() @@ -122,7 +122,7 @@ class RevocationChecker(object): def _translate_ocsp_query(cert_path, ocsp_output, ocsp_errors): """Returns a label string out of the query.""" - if not "Response verify OK": + 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 "" diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py index 1714dcdbb..b5c81bee4 100644 --- a/certbot/tests/cli_test.py +++ b/certbot/tests/cli_test.py @@ -270,7 +270,7 @@ class ParseTest(unittest.TestCase): def test_check_ocsp(self): self.assertRaises(errors.Error, self.parse, "certificates --check-ocsp bogus".split()) - self.parse("certificates --check-ocsp lazy".split()) + self.parse("certificates --check-ocsp LaZy".split()) class DefaultTest(unittest.TestCase): diff --git a/certbot/tests/ocsp_test.py b/certbot/tests/ocsp_test.py new file mode 100644 index 000000000..0bc947021 --- /dev/null +++ b/certbot/tests/ocsp_test.py @@ -0,0 +1,137 @@ + +"""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. +""" + +class OCSPTest(unittest.TestCase): + + _multiprocess_can_split_ = True + + def setUp(self): + from certbot import ocsp + self.config = mock.MagicMock() + self.checker = ocsp.RevocationChecker(self.config) + + def tearDown(self): + pass + + @mock.patch('certbot.ocsp.logging.info') + @mock.patch('certbot.ocsp.Popen') + @mock.patch('certbot.util.exe_exists') + def test_init(self, mock_exists, mock_popen, mock_log): + mock_communicate = mock.MagicMock() + mock_communicate.communicate.return_value = (None, out) + mock_popen.return_value = mock_communicate + mock_exists.return_value = True + + from certbot import ocsp + checker = ocsp.RevocationChecker(self.config) + 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) + 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) + 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" + + 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) + + 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') + def test_determine_ocsp_server(self, mock_run, mock_info, mock_debug): + 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)) + mock_run.return_value = "ftp:/" + host + "/", "" + self.assertEquals(self.checker.determine_ocsp_server("beep"), (None, None)) + self.assertEquals(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.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 + 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") + + +openssl_confused = ("", """ +/etc/letsencrypt/live/example.org/cert.pem: good + This Update: Dec 17 00:00:00 2016 GMT + Next Update: Dec 24 00:00:00 2016 GMT +""", +""" +Response Verify Failure +139903674214048:error:27069065:OCSP routines:OCSP_basic_verify:certificate verify error:ocsp_vfy.c:138:Verify error:unable to get local issuer certificate +""") + +openssl_happy = ("blah.pem", """ +blah.pem: good + This Update: Dec 20 18:00:00 2016 GMT + Next Update: Dec 27 18:00:00 2016 GMT +""", +"Response verify OK") + +openssl_revoked = ("blah.pem", """ +blah.pem: revoked + This Update: Dec 20 01:00:00 2016 GMT + Next Update: Dec 27 01:00:00 2016 GMT + Revocation Time: Dec 20 01:46:34 2016 GMT +""", +"""Response verify OK""") + +openssl_broken = ("", "tentacles", "Response verify OK") + +if __name__ == '__main__': + unittest.main() # pragma: no cover From 0ed3213989c095455d4c17c91cd1795e794374ea Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 21 Dec 2016 14:25:16 -0800 Subject: [PATCH 18/23] Remove --check-ocsp flag - Might have been occasionally useful, but simplicity - Add some missing tests, remove some obsolete ones --- certbot/cert_manager.py | 21 +++++---- certbot/cli.py | 8 ---- certbot/ocsp.py | 57 ++++++----------------- certbot/tests/cert_manager_test.py | 4 +- certbot/tests/cli_test.py | 4 -- certbot/tests/ocsp_test.py | 74 +++++++++++++++--------------- 6 files changed, 65 insertions(+), 103 deletions(-) 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 From e2d8630f5e3658218ce8fa3256dae841a1257ffe Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 21 Dec 2016 14:42:35 -0800 Subject: [PATCH 19/23] py3fix --- certbot/tests/ocsp_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot/tests/ocsp_test.py b/certbot/tests/ocsp_test.py index ce3ade2e8..285b0379b 100644 --- a/certbot/tests/ocsp_test.py +++ b/certbot/tests/ocsp_test.py @@ -86,7 +86,7 @@ class OCSPTest(unittest.TestCase): c = "confusion" mock_run.side_effect = errors.SubprocessError(c) self.assertEqual(self.checker.determine_ocsp_server("beep"), (None, None)) - self.assertTrue(c in mock_debug.call_args[0][1]) + self.assertTrue(c in repr(mock_debug.call_args[0][1])) @mock.patch('certbot.ocsp.logger') @mock.patch('certbot.util.run_script') From 61e822a89724a3430777a9840404e4348a5aa1ff Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 21 Dec 2016 21:50:19 -0800 Subject: [PATCH 20/23] Add a few more tests --- certbot/tests/cert_manager_test.py | 35 +++++++++++++++++++++++++----- certbot/tests/ocsp_test.py | 3 +++ 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/certbot/tests/cert_manager_test.py b/certbot/tests/cert_manager_test.py index dc83d0952..003eb1470 100644 --- a/certbot/tests/cert_manager_test.py +++ b/certbot/tests/cert_manager_test.py @@ -1,6 +1,7 @@ """Tests for certbot.cert_manager.""" # pylint disable=protected-access import os +import re import shutil import tempfile import unittest @@ -181,7 +182,7 @@ class CertificatesTest(BaseCertManagerTest): @mock.patch('certbot.cert_manager.ocsp.RevocationChecker.ocsp_revoked') def test_report_human_readable(self, mock_ocsp): - mock_ocsp.side_effect = lambda _cert, _chain: None + mock_ocsp.return_value = None from certbot import cert_manager import datetime, pytz expiry = pytz.UTC.fromutc(datetime.datetime.utcnow()) @@ -191,35 +192,57 @@ class CertificatesTest(BaseCertManagerTest): cert.names.return_value = ["nameone", "nametwo"] cert.is_test_cert = False parsed_certs = [cert] + get_report = lambda: cert_manager._report_human_readable(mock_config, parsed_certs) mock_config = mock.MagicMock(certname=None, lineagename=None) # pylint: disable=protected-access - out = cert_manager._report_human_readable(mock_config, parsed_certs) + out = get_report() self.assertTrue("INVALID: EXPIRED" in out) cert.target_expiry += datetime.timedelta(hours=2) # pylint: disable=protected-access - out = cert_manager._report_human_readable(mock_config, parsed_certs) + out = get_report() 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(mock_config, parsed_certs) + out = get_report() 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(mock_config, parsed_certs) + out = get_report() 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(mock_config, parsed_certs) + out = get_report() self.assertTrue('INVALID: TEST_CERT' in out) + cert = mock.MagicMock(lineagename="indescribable") + cert.target_expiry = expiry + cert.names.return_value = ["nameone", "thrice.named"] + cert.is_test_cert = True + parsed_certs.append(cert) + + out = get_report() + self.assertEqual(len(re.findall("INVALID:", out)), 2) + mock_config.domains = ["thrice.named"] + out = get_report() + self.assertEqual(len(re.findall("INVALID:", out)), 1) + mock_config.domains = ["nameone"] + out = get_report() + self.assertEqual(len(re.findall("INVALID:", out)), 2) + mock_config.certname = "indescribable" + out = get_report() + self.assertEqual(len(re.findall("INVALID:", out)), 1) + mock_config.certname = "horror" + out = get_report() + self.assertEqual(len(re.findall("INVALID:", out)), 0) + class SearchLineagesTest(BaseCertManagerTest): """Tests for certbot.cert_manager._search_lineages.""" diff --git a/certbot/tests/ocsp_test.py b/certbot/tests/ocsp_test.py index 285b0379b..e136978d8 100644 --- a/certbot/tests/ocsp_test.py +++ b/certbot/tests/ocsp_test.py @@ -69,6 +69,9 @@ class OCSPTest(unittest.TestCase): mock_determine.return_value = ("http://x.co", "x.co") self.assertEqual(self.checker.ocsp_revoked("blah.pem", "chain.pem"), False) + mock_run.side_effect = errors.SubprocessError("Unable to load certificate launcher") + self.assertEqual(self.checker.ocsp_revoked("x", "y"), None) + self.assertEqual(mock_run.call_count, 2) @mock.patch('certbot.ocsp.logger.debug') From 7014ab5fd0f02530b2319373f5fd45824a4d7a29 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 21 Dec 2016 23:20:19 -0800 Subject: [PATCH 21/23] lint --- certbot/tests/cert_manager_test.py | 4 +++- certbot/tests/ocsp_test.py | 3 +-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/certbot/tests/cert_manager_test.py b/certbot/tests/cert_manager_test.py index 003eb1470..6caaab878 100644 --- a/certbot/tests/cert_manager_test.py +++ b/certbot/tests/cert_manager_test.py @@ -1,5 +1,5 @@ """Tests for certbot.cert_manager.""" -# pylint disable=protected-access +# pylint: disable=protected-access import os import re import shutil @@ -192,6 +192,8 @@ class CertificatesTest(BaseCertManagerTest): cert.names.return_value = ["nameone", "nametwo"] cert.is_test_cert = False parsed_certs = [cert] + + # pylint: disable=protected-access get_report = lambda: cert_manager._report_human_readable(mock_config, parsed_certs) mock_config = mock.MagicMock(certname=None, lineagename=None) diff --git a/certbot/tests/ocsp_test.py b/certbot/tests/ocsp_test.py index e136978d8..c4517174d 100644 --- a/certbot/tests/ocsp_test.py +++ b/certbot/tests/ocsp_test.py @@ -1,5 +1,4 @@ - -"""Tests for hooks.py""" +"""Tests for ocsp.py""" # pylint: disable=protected-access import unittest From 19143d83036cdcbfc98c16f202a207c888ce18e3 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Thu, 22 Dec 2016 13:07:00 -0800 Subject: [PATCH 22/23] Increase test coverage --- certbot/tests/cert_manager_test.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/certbot/tests/cert_manager_test.py b/certbot/tests/cert_manager_test.py index 6caaab878..07f7cedaa 100644 --- a/certbot/tests/cert_manager_test.py +++ b/certbot/tests/cert_manager_test.py @@ -181,8 +181,8 @@ class CertificatesTest(BaseCertManagerTest): shutil.rmtree(tempdir) @mock.patch('certbot.cert_manager.ocsp.RevocationChecker.ocsp_revoked') - def test_report_human_readable(self, mock_ocsp): - mock_ocsp.return_value = None + def test_report_human_readable(self, mock_revoked): + mock_revoked.return_value = None from certbot import cert_manager import datetime, pytz expiry = pytz.UTC.fromutc(datetime.datetime.utcnow()) @@ -221,8 +221,9 @@ class CertificatesTest(BaseCertManagerTest): self.assertTrue('VALID' in out and not 'INVALID' in out) cert.is_test_cert = True + mock_revoked.return_value = True out = get_report() - self.assertTrue('INVALID: TEST_CERT' in out) + self.assertTrue('INVALID: TEST_CERT, REVOKED' in out) cert = mock.MagicMock(lineagename="indescribable") cert.target_expiry = expiry From 9aa93c05c1a4b88ab939660737db3d5e70d1b86d Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Thu, 22 Dec 2016 15:35:29 -0800 Subject: [PATCH 23/23] Simplify the ocsp_revoked() return type - we weren't reacting to None, so call it False instead --- certbot/ocsp.py | 12 ++++++------ certbot/tests/ocsp_test.py | 10 +++++----- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/certbot/ocsp.py b/certbot/ocsp.py index a5df5743d..2e0514a44 100644 --- a/certbot/ocsp.py +++ b/certbot/ocsp.py @@ -37,17 +37,17 @@ class RevocationChecker(object): :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 + :returns: True if revoked; False if valid or the check failed """ if self.broken: - return None + return False logger.debug("Querying OCSP for %s", cert_path) url, host = self.determine_ocsp_server(cert_path) if not host: - return None + return False # jdkasten thanks "Bulletproof SSL and TLS - Ivan Ristic" for documenting this! cmd = ["openssl", "ocsp", "-no_nonce", @@ -62,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 None + return False return _translate_ocsp_query(cert_path, output, err) @@ -98,12 +98,12 @@ def _translate_ocsp_query(cert_path, ocsp_output, ocsp_errors): 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 None + return False if cert_path + ": good" in ocsp_output: return False elif cert_path + ": revoked" in ocsp_output: return True else: logger.warn("Unable to properly parse OCSP output: %s", ocsp_output) - return None + return False diff --git a/certbot/tests/ocsp_test.py b/certbot/tests/ocsp_test.py index c4517174d..ff79bb01e 100644 --- a/certbot/tests/ocsp_test.py +++ b/certbot/tests/ocsp_test.py @@ -59,17 +59,17 @@ class OCSPTest(unittest.TestCase): 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) + self.assertEqual(self.checker.ocsp_revoked("x", "y"), False) self.checker.broken = False mock_run.return_value = tuple(openssl_happy[1:]) - self.assertEqual(self.checker.ocsp_revoked("x", "y"), None) + self.assertEqual(self.checker.ocsp_revoked("x", "y"), False) 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) mock_run.side_effect = errors.SubprocessError("Unable to load certificate launcher") - self.assertEqual(self.checker.ocsp_revoked("x", "y"), None) + self.assertEqual(self.checker.ocsp_revoked("x", "y"), False) self.assertEqual(mock_run.call_count, 2) @@ -97,10 +97,10 @@ class OCSPTest(unittest.TestCase): mock_run.return_value = openssl_confused from certbot import ocsp self.assertEqual(ocsp._translate_ocsp_query(*openssl_happy), False) - self.assertEqual(ocsp._translate_ocsp_query(*openssl_confused), None) + self.assertEqual(ocsp._translate_ocsp_query(*openssl_confused), False) 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(ocsp._translate_ocsp_query(*openssl_broken), False) self.assertEqual(mock_log.warn.call_count, 1) self.assertEqual(ocsp._translate_ocsp_query(*openssl_revoked), True)