From e6f050dbe9fa8faf247f0489c25987c742f72ff3 Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Mon, 10 Feb 2020 19:52:42 +0200 Subject: [PATCH 1/5] Move ocsp.py to public api (#7744) We should move ocsp.py to public API, as an upcoming OCSP prefetching functionality in Apache plugin relies on it, and as the plugins are note released in lockstep with the Certbot core, we need to be careful when changing those APIs. * Move ocsp.py to public api * Fix type annotations, move to pointing to an interface and fix linting * Add certbot.ocsp to documentation table of contents * Modify tests to reflect the changes in ocsp.py * Add changelog entry * Fix notAfter mock for tests --- certbot/CHANGELOG.md | 2 + certbot/certbot/_internal/cert_manager.py | 2 +- certbot/certbot/{_internal => }/ocsp.py | 11 +++-- certbot/docs/api/certbot.ocsp.rst | 7 +++ certbot/docs/api/certbot.rst | 1 + certbot/tests/ocsp_test.py | 55 ++++++++++++----------- 6 files changed, 46 insertions(+), 32 deletions(-) rename certbot/certbot/{_internal => }/ocsp.py (97%) create mode 100644 certbot/docs/api/certbot.ocsp.rst diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md index 50a6bc7f5..ff3061e01 100644 --- a/certbot/CHANGELOG.md +++ b/certbot/CHANGELOG.md @@ -6,6 +6,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). ### Added +* Added certbot.ocsp Certbot's API. The certbot.ocsp module can be used to + determine the OCSP status of certificates. * Don't verify the existing certificate in HTTP01Response.simple_verify, for compatibility with the real-world ACME challenge checks. diff --git a/certbot/certbot/_internal/cert_manager.py b/certbot/certbot/_internal/cert_manager.py index 1def76a3d..298e7d269 100644 --- a/certbot/certbot/_internal/cert_manager.py +++ b/certbot/certbot/_internal/cert_manager.py @@ -11,8 +11,8 @@ from acme.magic_typing import List # pylint: disable=unused-import, no-name-in- from certbot import crypto_util from certbot import errors from certbot import interfaces +from certbot import ocsp from certbot import util -from certbot._internal import ocsp from certbot._internal import storage from certbot.compat import os from certbot.display import util as display_util diff --git a/certbot/certbot/_internal/ocsp.py b/certbot/certbot/ocsp.py similarity index 97% rename from certbot/certbot/_internal/ocsp.py rename to certbot/certbot/ocsp.py index 2f6543e5d..5c3cfdd59 100644 --- a/certbot/certbot/_internal/ocsp.py +++ b/certbot/certbot/ocsp.py @@ -21,7 +21,7 @@ from acme.magic_typing import Tuple # pylint: disable=unused-import, no-name-in from certbot import crypto_util from certbot import errors from certbot import util -from certbot._internal.storage import RenewableCert # pylint: disable=unused-import +from certbot.interfaces import RenewableCert # pylint: disable=unused-import try: # Only cryptography>=2.5 has ocsp module @@ -32,7 +32,6 @@ except (ImportError, AttributeError): # pragma: no cover ocsp = None # type: ignore - logger = logging.getLogger(__name__) @@ -64,12 +63,12 @@ class RevocationChecker(object): .. todo:: Make this a non-blocking call - :param `.storage.RenewableCert` cert: Certificate object + :param `.interfaces.RenewableCert` cert: Certificate object :returns: True if revoked; False if valid or the check failed or cert is expired. :rtype: bool """ - cert_path, chain_path = cert.cert, cert.chain + cert_path, chain_path = cert.cert_path, cert.chain_path if self.broken: return False @@ -78,7 +77,7 @@ class RevocationChecker(object): # so don't check OCSP if the cert is expired. # https://github.com/certbot/certbot/issues/7152 now = pytz.UTC.fromutc(datetime.utcnow()) - if cert.target_expiry <= now: + if crypto_util.notAfter(cert_path) <= now: return False url, host = _determine_ocsp_server(cert_path) @@ -296,5 +295,5 @@ def _translate_ocsp_query(cert_path, ocsp_output, ocsp_errors): return True else: logger.warning("Unable to properly parse OCSP output: %s\nstderr:%s", - ocsp_output, ocsp_errors) + ocsp_output, ocsp_errors) return False diff --git a/certbot/docs/api/certbot.ocsp.rst b/certbot/docs/api/certbot.ocsp.rst new file mode 100644 index 000000000..1266c328a --- /dev/null +++ b/certbot/docs/api/certbot.ocsp.rst @@ -0,0 +1,7 @@ +certbot.ocsp package +====================== + +.. automodule:: certbot.ocsp + :members: + :undoc-members: + :show-inheritance: diff --git a/certbot/docs/api/certbot.rst b/certbot/docs/api/certbot.rst index 6f5b4b403..e4245f80f 100644 --- a/certbot/docs/api/certbot.rst +++ b/certbot/docs/api/certbot.rst @@ -26,6 +26,7 @@ Submodules certbot.errors certbot.interfaces certbot.main + certbot.ocsp certbot.reverter certbot.util diff --git a/certbot/tests/ocsp_test.py b/certbot/tests/ocsp_test.py index 6e4ab52b8..6b05a8d3c 100644 --- a/certbot/tests/ocsp_test.py +++ b/certbot/tests/ocsp_test.py @@ -32,12 +32,12 @@ ocsp: Use -help for summary. class OCSPTestOpenSSL(unittest.TestCase): """ - OCSP revokation tests using OpenSSL binary. + OCSP revocation tests using OpenSSL binary. """ def setUp(self): - from certbot._internal import ocsp - with mock.patch('certbot._internal.ocsp.Popen') as mock_popen: + from certbot import ocsp + 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) @@ -48,8 +48,8 @@ class OCSPTestOpenSSL(unittest.TestCase): def tearDown(self): pass - @mock.patch('certbot._internal.ocsp.logger.info') - @mock.patch('certbot._internal.ocsp.Popen') + @mock.patch('certbot.ocsp.logger.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() @@ -57,7 +57,7 @@ class OCSPTestOpenSSL(unittest.TestCase): mock_popen.return_value = mock_communicate mock_exists.return_value = True - from certbot._internal import ocsp + from certbot import ocsp checker = ocsp.RevocationChecker(enforce_openssl_binary_usage=True) self.assertEqual(mock_popen.call_count, 1) self.assertEqual(checker.host_args("x"), ["Host=x"]) @@ -74,14 +74,15 @@ class OCSPTestOpenSSL(unittest.TestCase): self.assertEqual(mock_log.call_count, 1) self.assertEqual(checker.broken, True) - @mock.patch('certbot._internal.ocsp._determine_ocsp_server') + @mock.patch('certbot.ocsp._determine_ocsp_server') + @mock.patch('certbot.ocsp.crypto_util.notAfter') @mock.patch('certbot.util.run_script') - def test_ocsp_revoked(self, mock_run, mock_determine): + def test_ocsp_revoked(self, mock_run, mock_na, mock_determine): now = pytz.UTC.fromutc(datetime.utcnow()) cert_obj = mock.MagicMock() - cert_obj.cert = "x" - cert_obj.chain = "y" - cert_obj.target_expiry = now + timedelta(hours=2) + cert_obj.cert_path = "x" + cert_obj.chain_path = "y" + mock_na.return_value = now + timedelta(hours=2) self.checker.broken = True mock_determine.return_value = ("", "") @@ -99,7 +100,7 @@ class OCSPTestOpenSSL(unittest.TestCase): self.assertEqual(mock_run.call_count, 2) # cert expired - cert_obj.target_expiry = now + mock_na.return_value = now mock_determine.return_value = ("", "") count_before = mock_determine.call_count self.assertEqual(self.checker.ocsp_revoked(cert_obj), False) @@ -108,16 +109,16 @@ class OCSPTestOpenSSL(unittest.TestCase): def test_determine_ocsp_server(self): cert_path = test_util.vector_path('ocsp_certificate.pem') - from certbot._internal import ocsp + from certbot import ocsp result = ocsp._determine_ocsp_server(cert_path) self.assertEqual(('http://ocsp.test4.buypass.com', 'ocsp.test4.buypass.com'), result) - @mock.patch('certbot._internal.ocsp.logger') + @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._internal import ocsp + from certbot import ocsp self.assertEqual(ocsp._translate_ocsp_query(*openssl_happy), False) self.assertEqual(ocsp._translate_ocsp_query(*openssl_confused), False) self.assertEqual(mock_log.debug.call_count, 1) @@ -145,18 +146,22 @@ class OSCPTestCryptography(unittest.TestCase): """ def setUp(self): - from certbot._internal import ocsp + from certbot import ocsp self.checker = ocsp.RevocationChecker() self.cert_path = test_util.vector_path('ocsp_certificate.pem') self.chain_path = test_util.vector_path('ocsp_issuer_certificate.pem') self.cert_obj = mock.MagicMock() - self.cert_obj.cert = self.cert_path - self.cert_obj.chain = self.chain_path + self.cert_obj.cert_path = self.cert_path + self.cert_obj.chain_path = self.chain_path now = pytz.UTC.fromutc(datetime.utcnow()) - self.cert_obj.target_expiry = now + timedelta(hours=2) + self.mock_notAfter = mock.patch('certbot.ocsp.crypto_util.notAfter', + return_value=now + timedelta(hours=2)) + self.mock_notAfter.start() + # Ensure the mock.patch is stopped even if test raises an exception + self.addCleanup(self.mock_notAfter.stop) - @mock.patch('certbot._internal.ocsp._determine_ocsp_server') - @mock.patch('certbot._internal.ocsp._check_ocsp_cryptography') + @mock.patch('certbot.ocsp._determine_ocsp_server') + @mock.patch('certbot.ocsp._check_ocsp_cryptography') def test_ensure_cryptography_toggled(self, mock_revoke, mock_determine): mock_determine.return_value = ('http://example.com', 'example.com') self.checker.ocsp_revoked(self.cert_obj) @@ -263,7 +268,7 @@ class OSCPTestCryptography(unittest.TestCase): with _ocsp_mock(ocsp_lib.OCSPCertStatus.REVOKED, ocsp_lib.OCSPResponseStatus.SUCCESSFUL): # This mock is necessary to avoid the first call contained in _determine_ocsp_server # of the method cryptography.x509.Extensions.get_extension_for_class. - with mock.patch('certbot._internal.ocsp._determine_ocsp_server') as mock_server: + with mock.patch('certbot.ocsp._determine_ocsp_server') as mock_server: mock_server.return_value = ('https://example.com', 'example.com') with mock.patch('cryptography.x509.Extensions.get_extension_for_class', side_effect=x509.ExtensionNotFound( @@ -275,12 +280,12 @@ class OSCPTestCryptography(unittest.TestCase): @contextlib.contextmanager def _ocsp_mock(certificate_status, response_status, http_status_code=200, check_signature_side_effect=None): - with mock.patch('certbot._internal.ocsp.ocsp.load_der_ocsp_response') as mock_response: + with mock.patch('certbot.ocsp.ocsp.load_der_ocsp_response') as mock_response: mock_response.return_value = _construct_mock_ocsp_response( certificate_status, response_status) - with mock.patch('certbot._internal.ocsp.requests.post') as mock_post: + with mock.patch('certbot.ocsp.requests.post') as mock_post: mock_post.return_value = mock.Mock(status_code=http_status_code) - with mock.patch('certbot._internal.ocsp.crypto_util.verify_signed_payload') \ + with mock.patch('certbot.ocsp.crypto_util.verify_signed_payload') \ as mock_check: if check_signature_side_effect: mock_check.side_effect = check_signature_side_effect From 02bf7d7dfc0864b742f0efe9cc7074f525a872ab Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 10 Feb 2020 11:01:17 -0800 Subject: [PATCH 2/5] Print script output in case of a failure. (#7759) These tests failed at https://travis-ci.com/certbot/certbot/jobs/285202481 but do not include any output from the script about what went wrong because the string created from `subprocess.CalledProcessError` does not include value of output. This PR fixes that by printing these values which `pytest` will include in the output if the test fails. --- letsencrypt-auto-source/tests/auto_test.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/letsencrypt-auto-source/tests/auto_test.py b/letsencrypt-auto-source/tests/auto_test.py index 9c823fb55..805bb21af 100644 --- a/letsencrypt-auto-source/tests/auto_test.py +++ b/letsencrypt-auto-source/tests/auto_test.py @@ -167,6 +167,10 @@ def out_and_err(command, input=None, shell=False, env=None): if status: error = CalledProcessError(status, command) error.output = out + print('stdout output was:') + print(out) + print('stderr output was:') + print(err) raise error return out, err From b8856ac810ba89deff0bee012db1fd515dc75efa Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 11 Feb 2020 12:05:29 -0800 Subject: [PATCH 3/5] Fix unpinned tests (#7760) Our nightly tests failed last night due to a new release of `virtualenv` and `pip`'s lack of dependency resolution: https://travis-ci.com/certbot/certbot/jobs/285797857#L280. It looks like we were not the only ones affected by this problem: https://github.com/pypa/virtualenv/issues/1551 This fixes the problem by using `-I` to skip the logic where `pip` decides a dependency is already satisfied and has it reinstall/update the packages passed to `pip` and all of their dependencies. You can see our nightly tests passing with this change at https://github.com/certbot/certbot/runs/439231061. --- .travis.yml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 23d957a21..1eae66333 100644 --- a/.travis.yml +++ b/.travis.yml @@ -263,8 +263,10 @@ addons: # except in tests where the environment variable CERTBOT_NO_PIN is set. # virtualenv is listed here explicitly to make sure it is upgraded when # CERTBOT_NO_PIN is set to work around failures we've seen when using an older -# version of virtualenv. -install: 'tools/pip_install.py -U codecov tox virtualenv' +# version of virtualenv. The option "-I" is set so when CERTBOT_NO_PIN is also +# set, pip updates dependencies it thinks are already satisfied to avoid some +# problems with its lack of real dependency resolution. +install: 'tools/pip_install.py -I codecov tox virtualenv' # Most of the time TRAVIS_RETRY is an empty string, and has no effect on the # script command. It is set only to `travis_retry` during farm tests, in # order to trigger the Travis retry feature, and compensate the inherent From 605ef40656110e4b1e9f5d35c47b102ea2aa2b4b Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 11 Feb 2020 14:20:29 -0800 Subject: [PATCH 4/5] Remove duplicate pyparsing pin --- tools/oldest_constraints.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/tools/oldest_constraints.txt b/tools/oldest_constraints.txt index 6154b497a..85d058796 100644 --- a/tools/oldest_constraints.txt +++ b/tools/oldest_constraints.txt @@ -13,7 +13,6 @@ ply==3.4 pyasn1==0.1.9 pycparser==2.14 pyOpenSSL==0.13.1 -pyparsing==1.5.6 pyRFC3339==1.0 python-augeas==0.5.0 oauth2client==4.0.0 From 7d540fc33ad39ec6664807c1a6445426c5534097 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 11 Feb 2020 14:44:37 -0800 Subject: [PATCH 5/5] update pyparsing comment --- certbot-nginx/setup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot-nginx/setup.py b/certbot-nginx/setup.py index 3b75a3424..b180fe06a 100644 --- a/certbot-nginx/setup.py +++ b/certbot-nginx/setup.py @@ -13,7 +13,7 @@ install_requires = [ 'certbot>=1.1.0', 'mock', 'PyOpenSSL', - 'pyparsing>=1.5.5', # Python3 support; perhaps unnecessary? + 'pyparsing>=1.5.5', # Python3 support 'setuptools', 'zope.interface', ]