Add fix for Go x/crypto/ocsp failure case

When calling ocsp.ParseRequest(req, issue) with a non-nil issuer on a
ocsp request which _unknowingly_ contains an entry in the
BasicOCSPResponse's certs field, Go incorrectly assumes that the issuer
is a direct parent of the _first_ certificate in the certs field,
discarding the rest.

As documented in the Go issue, this is not a valid assumption and thus
causes OCSP verification to fail in Vault with an error like:

> bad OCSP signature: crypto/rsa: verification error

which ultimately leads to a cert auth login error of:

> no chain matching all constraints could be found for this login certificate

We address this by using the unsafe issuer=nil argument, taking on the
task of validating the OCSP response's signature as best we can in the
absence of full chain information on either side (both the trusted
certificate whose OCSP response we're verifying and the lack of any
additional certs the OCSP responder may have sent).

See also: golang/go#59641

Signed-off-by: Alexander Scheel <[email protected]>

Author: cipherboy

sdk/helper/ocsp/client.go

@@ -347,13 +347,75 @@ func (c *Client) retryOCSP(
 		// endpoint might return invalid results for e.g., GET but return
 		// valid results for POST on retry. This could happen if e.g., the
 		// server responds with JSON.
-		ocspRes, err = ocsp.ParseResponse(ocspResBytes, issuer)
+		ocspRes, err = ocsp.ParseResponse(ocspResBytes /*issuer = */, nil /* !!unsafe!! */)
 		if err != nil {
 			err = fmt.Errorf("error parsing %v OCSP response: %w", method, err)
 			retErr = multierror.Append(retErr, err)
 			continue
 		}
 
+		// Above, we use the unsafe issuer=nil parameter to ocsp.ParseResponse
+		// because Go's library does the wrong thing.
+		//
+		// Here, we lack a full chain, but we know we trust the parent issuer,
+		// so if the Go library incorrectly discards useful certificates, we
+		// likely cannot verify this without passing through the full chain
+		// back to the root.
+		//
+		// Instead, take one of two paths: 1. if there is no certificate in
+		// the ocspRes, verify the OCSP response directly with our trusted
+		// issuer certificate, or 2. if there is a certificate, either verify
+		// it directly matches our trusted issuer certificate, or verify it
+		// is signed by our trusted issuer certificate.
+		//
+		// See also: https://github.com/golang/go/issues/59641
+		//
+		// This addresses the !!unsafe!! behavior above.
+		if ocspRes.Certificate == nil {
+			if err := ocspRes.CheckSignatureFrom(issuer); err != nil {
+				err = fmt.Errorf("error directly verifying signature on %v OCSP response: %w", method, err)
+				retErr = multierror.Append(retErr, err)
+				continue
+			}
+		} else {
+			// Because we have at least one certificate here, we know that
+			// Go's ocsp library verified the signature from this certificate
+			// onto the response and it was valid. Now we need to know we trust
+			// this certificate. There's two ways we can do this:
+			//
+			// 1. Via confirming issuer == ocspRes.Certificate, or
+			// 2. Via confirming ocspRes.Certificate.CheckSignatureFrom(issuer).
+			if !bytes.Equal(issuer.Raw, ocspRes.Raw) {
+				// 1 must not hold, so 2 holds; verify the signature.
+				if err := ocspRes.Certificate.CheckSignatureFrom(issuer); err != nil {
+					err = fmt.Errorf("error checking chain of trust on %v OCSP response via %v failed: %w", method, issuer.Subject.String(), err)
+					retErr = multierror.Append(retErr, err)
+					continue
+				}
+
+				// Verify the OCSP responder certificate is still valid and
+				// contains the required EKU since it is a delegated OCSP
+				// responder certificate.
+				if ocspRes.Certificate.NotAfter.Before(time.Now()) {
+					err := fmt.Errorf("error checking delegated OCSP responder on %v OCSP response: certificate has expired", method)
+					retErr = multierror.Append(retErr, err)
+					continue
+				}
+				haveEKU := false
+				for _, ku := range ocspRes.Certificate.ExtKeyUsage {
+					if ku == x509.ExtKeyUsageOCSPSigning {
+						haveEKU = true
+						break
+					}
+				}
+				if !haveEKU {
+					err := fmt.Errorf("error checking delegated OCSP responder on %v OCSP response: certificate lacks the OCSP Signing EKU", method)
+					retErr = multierror.Append(retErr, err)
+					continue
+				}
+			}
+		}
+
 		// While we haven't validated the signature on the OCSP response, we
 		// got what we presume is a definitive answer and simply changing
 		// methods will likely not help us in that regard. Use this status