mirror of
https://github.com/certbot/certbot.git
synced 2026-05-28 04:34:11 -04:00
Remove --check-ocsp flag
- Might have been occasionally useful, but simplicity - Add some missing tests, remove some obsolete ones
This commit is contained in:
Peter Eckersley
parent
76b8c53566
commit
0ed3213989
6 changed files with 65 additions and 103 deletions
|
|
@ -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"
|
||||
|
|
|
|||
|
|
@ -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"),
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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())
|
||||
|
|
|
|||
|
|
@ -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."""
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Reference in a new issue