Skip to content

Conversation

jamis
Copy link
Contributor

@jamis jamis commented Aug 6, 2025

The driver was incorrectly using the last certificate in the chain as the CA when performing OCSP verification. This worked for the case where the chain had only two elements, but for longer chains, it was causing the verification to fail.

This change searches the cert chain for the issuer of the peer certificate, and uses that as the CA for the verification request.

@jamis jamis requested a review from a team as a code owner August 6, 2025 15:07
@jamis jamis requested a review from comandeo-mongo August 6, 2025 15:07
@@ -460,7 +460,7 @@ def verify_ocsp_endpoint!(socket, timeout = nil)
end

cert = socket.peer_cert
ca_cert = socket.peer_cert_chain.last
ca_cert = socket.peer_cert_chain[1]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we rely on this certificate always working or should we fall back to check all other certificates as well?

Copy link
Contributor Author

@jamis jamis Aug 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two points for why I think this is correct (but I am absolutely not well-versed in SSL lore, and may have this all wrong).

  • Per the Suggested OCSP Behavior section of our OCSP spec, item 8 says "If any unvalidated intermediate certificates remain and those certificates have OCSP endpoints, for each certificate, the driver SHOULD NOT reach out to the OCSP endpoint specified and attempt to validate that certificate." So, we don't try to validate the OCSP endpoints of any intermediate certs. (Not sure if that's what you were referring to, but figured I'd be thorough.)
  • Given that we saw the warnings/errors happening because we were trying to validate using the last item in the chain, I'm assuming that using subsequent certificates to validate here would be similarly problematic. Or, is the correct behavior that we're supposed to try each certificate in the chain until one of them gives the green light, ignoring error messages along the way?

alexbevi
alexbevi previously approved these changes Aug 6, 2025
Copy link
Contributor

@alexbevi alexbevi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@alexbevi alexbevi self-requested a review August 7, 2025 14:05
Copy link
Contributor

@alexbevi alexbevi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approach LGTM

@jamis jamis merged commit 842b794 into mongodb:master Aug 7, 2025
202 of 206 checks passed
@jamis jamis deleted the 3694-fix-ocsp-verification branch August 7, 2025 14:17
jamis added a commit to jamis/mongo-ruby-driver that referenced this pull request Aug 7, 2025
* use the next cert in the chain as the CA when verifying OCSP

* don't assume the issuer is at a particular position in the chain

* Fix incorrect method description
@jamis jamis added the bug label Aug 7, 2025
jamis added a commit that referenced this pull request Aug 19, 2025
* RUBY-3694 Use correct CA when verifying OCSP endpoint  (#2944)

* use the next cert in the chain as the CA when verifying OCSP

* don't assume the issuer is at a particular position in the chain

* Fix incorrect method description

* https for submodules

* fix submodule syntax

* update test configuration

* bump drivers-evergreen-tools for DB version lookups

* allow prepare_server to compute the distro in the format it needs

* more evergreen config tweaks

* more test tweaks

* fix broken resolver specs

* don't try to use mock_dns on ruby 2.7

* make sure and definie the minimum_mri_version constraint

* use https to clone git repos

* maybe we need to skip sooner to avoid the around hooks...

* Gracefully handle the case where the issuer can't be found (#2946)

* no need to test against latest; 2.20.x only supports up to 7.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants