PYTHON-5740 - Fix weak OCSP hashing algorithm#2748
PYTHON-5740 - Fix weak OCSP hashing algorithm#2748NoahStapp wants to merge 3 commits intomongodb:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to address weak hashing in OCSP handling by switching from SHA-1 to SHA-256.
Changes:
- Updated OCSP-related hashing in
pymongo/ocsp_support.pyto use SHA-256. - Updated the changelog to document the hashing change.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
pymongo/ocsp_support.py |
Swaps OCSP hashing from SHA-1 to SHA-256 in responder key hashing and OCSP request construction. |
doc/changelog.rst |
Adds a release note describing the OCSP hashing change. |
| elif isinstance(public_key, _EllipticCurvePublicKey): | ||
| pbytes = public_key.public_bytes(_Encoding.X962, _PublicFormat.UncompressedPoint) | ||
| else: | ||
| pbytes = public_key.public_bytes(_Encoding.DER, _PublicFormat.SubjectPublicKeyInfo) | ||
| digest = _Hash(_SHA1(), backend=_default_backend()) # noqa: S303 | ||
| digest = _Hash(_SHA256(), backend=_default_backend()) |
There was a problem hiding this comment.
_public_key_hash now computes a SHA-256 digest, but the OCSP ResponderID byKey KeyHash is defined as a SHA-1 hash in RFC2560/RFC6960. response.responder_key_hash from cryptography follows the RFC, so switching this to SHA-256 will prevent _get_certs_by_key_hash from ever finding a matching responder cert (and can cause valid delegated OCSP responses to be rejected). Keep SHA-1 for the responder KeyHash computation (and update the comment accordingly), even if the OCSP CertID/request hashing moves to SHA-256.
There was a problem hiding this comment.
I need to verify more, the robots have given different answers on each ask.
| def _build_ocsp_request(cert: Certificate, issuer: Certificate) -> OCSPRequest: | ||
| # https://cryptography.io/en/latest/x509/ocsp/#creating-requests | ||
| builder = _OCSPRequestBuilder() | ||
| builder = builder.add_certificate(cert, issuer, _SHA1()) # noqa: S303 | ||
| builder = builder.add_certificate(cert, issuer, _SHA256()) | ||
| return builder.build() |
There was a problem hiding this comment.
Changing the OCSP request CertID hash algorithm to SHA-256 updates response.issuer_key_hash (it is computed using the request's hash algorithm). The response ResponderID responder_key_hash is not derived from the CertID hash algorithm, so the existing responder-is-issuer shortcut (rkey_hash == ikey_hash) can stop working and may incorrectly treat an issuer responder as a delegate, causing verification failures when no responder cert is embedded. Update the issuer-responder detection logic to compare like-for-like (e.g., compare responder_key_hash against a SHA-1 KeyHash of the issuer cert per RFC, or rely on responder_name when present) rather than comparing to issuer_key_hash.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
PYTHON-5740
Changes in this PR
Use SHA-256 instead of SHA1 to align with modern practices for OCSP authentication.
Test Plan
Use existing tests. Requires mongodb-labs/drivers-evergreen-tools#762 to be merged first for test server compatibility.
Checklist
Checklist for Author
Checklist for Reviewer