diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md index 2f551c8ab..c6f6c95a3 100644 --- a/certbot/CHANGELOG.md +++ b/certbot/CHANGELOG.md @@ -25,6 +25,7 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). `resource` field in any requests or the `type` field when responding to challenges. * Fix nginx plugin crash when non-ASCII configuration file is being read (instead, the user will be warned that UTF-8 must be used). +* Fix hanging OCSP queries during revocation checking - added a 10 second timeout. More details about these changes can be found on our GitHub repo. diff --git a/certbot/certbot/ocsp.py b/certbot/certbot/ocsp.py index 9799c675c..1d5611b64 100644 --- a/certbot/certbot/ocsp.py +++ b/certbot/certbot/ocsp.py @@ -70,12 +70,13 @@ class RevocationChecker(object): """ return self.ocsp_revoked_by_paths(cert.cert_path, cert.chain_path) - def ocsp_revoked_by_paths(self, cert_path, chain_path): - # type: (str, str) -> bool + def ocsp_revoked_by_paths(self, cert_path, chain_path, timeout=10): + # type: (str, str, int) -> bool """Performs the OCSP revocation check :param str cert_path: Certificate filepath - :param str chain_path: Certificate chain filepath + :param str chain_path: Certificate chain + :param int timeout: Timeout (in seconds) for the OCSP query :returns: True if revoked; False if valid or the check failed or cert is expired. :rtype: bool @@ -96,11 +97,11 @@ class RevocationChecker(object): return False if self.use_openssl_binary: - return self._check_ocsp_openssl_bin(cert_path, chain_path, host, url) - return _check_ocsp_cryptography(cert_path, chain_path, url) + return self._check_ocsp_openssl_bin(cert_path, chain_path, host, url, timeout) + return _check_ocsp_cryptography(cert_path, chain_path, url, timeout) - def _check_ocsp_openssl_bin(self, cert_path, chain_path, host, url): - # type: (str, str, str, str) -> bool + def _check_ocsp_openssl_bin(self, cert_path, chain_path, host, url, timeout): + # type: (str, str, str, str, int) -> bool # jdkasten thanks "Bulletproof SSL and TLS - Ivan Ristic" for documenting this! cmd = ["openssl", "ocsp", "-no_nonce", @@ -110,6 +111,7 @@ class RevocationChecker(object): "-CAfile", chain_path, "-verify_other", chain_path, "-trust_other", + "-timeout", str(timeout), "-header"] + self.host_args(host) logger.debug("Querying OCSP for %s", cert_path) logger.debug(" ".join(cmd)) @@ -152,8 +154,8 @@ def _determine_ocsp_server(cert_path): return None, None -def _check_ocsp_cryptography(cert_path, chain_path, url): - # type: (str, str, str) -> bool +def _check_ocsp_cryptography(cert_path, chain_path, url, timeout): + # type: (str, str, str, int) -> bool # Retrieve OCSP response with open(chain_path, 'rb') as file_handler: issuer = x509.load_pem_x509_certificate(file_handler.read(), default_backend()) @@ -165,7 +167,8 @@ def _check_ocsp_cryptography(cert_path, chain_path, url): request_binary = request.public_bytes(serialization.Encoding.DER) try: response = requests.post(url, data=request_binary, - headers={'Content-Type': 'application/ocsp-request'}) + headers={'Content-Type': 'application/ocsp-request'}, + timeout=timeout) except requests.exceptions.RequestException: logger.info("OCSP check failed for %s (are we offline?)", cert_path, exc_info=True) return False diff --git a/certbot/tests/ocsp_test.py b/certbot/tests/ocsp_test.py index 6b05a8d3c..9eb49e115 100644 --- a/certbot/tests/ocsp_test.py +++ b/certbot/tests/ocsp_test.py @@ -162,11 +162,11 @@ class OSCPTestCryptography(unittest.TestCase): @mock.patch('certbot.ocsp._determine_ocsp_server') @mock.patch('certbot.ocsp._check_ocsp_cryptography') - def test_ensure_cryptography_toggled(self, mock_revoke, mock_determine): + def test_ensure_cryptography_toggled(self, mock_check, mock_determine): mock_determine.return_value = ('http://example.com', 'example.com') self.checker.ocsp_revoked(self.cert_obj) - mock_revoke.assert_called_once_with(self.cert_path, self.chain_path, 'http://example.com') + mock_check.assert_called_once_with(self.cert_path, self.chain_path, 'http://example.com', 10) def test_revoke(self): with _ocsp_mock(ocsp_lib.OCSPCertStatus.REVOKED, ocsp_lib.OCSPResponseStatus.SUCCESSFUL):