Ferry ocsp_ca_certificates over the OCSP ValidationConf (#28309)

* Ferry ocsp_ca_certificates over the OCSP ValidationConf

* changelog

* First check issuer, then check extraCAS

* Use the correct cert when the signature validation from issuer succeeds

* Validate via extraCas in the cert missing case as well

* dedupe logic

* remove CA test

builtin/credential/cert/path_login.go

@@ -693,6 +693,15 @@ func (b *backend) loadTrustedCerts(ctx context.Context, storage logical.Storage,
 			conf.QueryAllServers = conf.QueryAllServers || entry.OcspQueryAllServers
 			conf.OcspThisUpdateMaxAge = entry.OcspThisUpdateMaxAge
 			conf.OcspMaxRetries = entry.OcspMaxRetries
+
+			if len(entry.OcspCaCertificates) > 0 {
+				certs, err := certutil.ParseCertsPEM([]byte(entry.OcspCaCertificates))
+				if err != nil {
+					b.Logger().Error("failed to parse ocsp_ca_certificates", "name", name, "error", err)
+					continue
+				}
+				conf.ExtraCas = certs
+			}
 		}
 	}
 

builtin/credential/cert/path_login_test.go

@@ -5,6 +5,7 @@ package cert
 
 import (
 	"context"
+	"crypto"
 	"crypto/tls"
 	"crypto/x509"
 	"crypto/x509/pkix"
@@ -281,19 +282,6 @@ func TestCert_RoleResolve_RoleDoesNotExist(t *testing.T) {
 }
 
 func TestCert_RoleResolveOCSP(t *testing.T) {
-	cases := []struct {
-		name        string
-		failOpen    bool
-		certStatus  int
-		errExpected bool
-	}{
-		{"failFalseGoodCert", false, ocsp.Good, false},
-		{"failFalseRevokedCert", false, ocsp.Revoked, true},
-		{"failFalseUnknownCert", false, ocsp.Unknown, true},
-		{"failTrueGoodCert", true, ocsp.Good, false},
-		{"failTrueRevokedCert", true, ocsp.Revoked, true},
-		{"failTrueUnknownCert", true, ocsp.Unknown, false},
-	}
 	certTemplate := &x509.Certificate{
 		Subject: pkix.Name{
 			CommonName: "example.com",
@@ -332,15 +320,76 @@ func TestCert_RoleResolveOCSP(t *testing.T) {
 		t.Fatalf("err: %v", err)
 	}
 
+	tempDir, connState2, err := generateTestCertAndConnState(t, certTemplate)
+	if err != nil {
+		t.Fatalf("error testing connection state: %v", err)
+	}
+	ca2, err := ioutil.ReadFile(filepath.Join(tempDir, "ca_cert.pem"))
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
+	issuer2 := parsePEM(ca2)
+	pkf2, err := ioutil.ReadFile(filepath.Join(tempDir, "ca_key.pem"))
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+	pk2, err := certutil.ParsePEMBundle(string(pkf2))
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
+	type caData struct {
+		privateKey crypto.Signer
+		caBytes    []byte
+		caChain    []*x509.Certificate
+		connState  tls.ConnectionState
+	}
+
+	ca1Data := caData{
+		pk.PrivateKey,
+		ca,
+		issuer,
+		connState,
+	}
+	ca2Data := caData{
+		pk2.PrivateKey,
+		ca2,
+		issuer2,
+		connState2,
+	}
+
+	cases := []struct {
+		name        string
+		failOpen    bool
+		certStatus  int
+		errExpected bool
+		caData      caData
+		ocspCaCerts string
+	}{
+		{name: "failFalseGoodCert", certStatus: ocsp.Good, caData: ca1Data},
+		{name: "failFalseRevokedCert", certStatus: ocsp.Revoked, errExpected: true, caData: ca1Data},
+		{name: "failFalseUnknownCert", certStatus: ocsp.Unknown, errExpected: true, caData: ca1Data},
+		{name: "failTrueGoodCert", failOpen: true, certStatus: ocsp.Good, caData: ca1Data},
+		{name: "failTrueRevokedCert", failOpen: true, certStatus: ocsp.Revoked, errExpected: true, caData: ca1Data},
+		{name: "failTrueUnknownCert", failOpen: true, certStatus: ocsp.Unknown, caData: ca1Data},
+		{name: "failFalseGoodCertExtraCas", certStatus: ocsp.Good, caData: ca2Data, ocspCaCerts: string(pkf2)},
+		{name: "failFalseRevokedCertExtraCas", certStatus: ocsp.Revoked, errExpected: true, caData: ca2Data, ocspCaCerts: string(pkf2)},
+		{name: "failFalseUnknownCertExtraCas", certStatus: ocsp.Unknown, errExpected: true, caData: ca2Data, ocspCaCerts: string(pkf2)},
+		{name: "failTrueGoodCertExtraCas", failOpen: true, certStatus: ocsp.Good, caData: ca2Data, ocspCaCerts: string(pkf2)},
+		{name: "failTrueRevokedCertExtraCas", failOpen: true, certStatus: ocsp.Revoked, errExpected: true, caData: ca2Data, ocspCaCerts: string(pkf2)},
+		{name: "failTrueUnknownCertExtraCas", failOpen: true, certStatus: ocsp.Unknown, caData: ca2Data, ocspCaCerts: string(pkf2)},
+	}
+
 	for _, c := range cases {
 		t.Run(c.name, func(t *testing.T) {
-			resp, err := ocsp.CreateResponse(issuer[0], issuer[0], ocsp.Response{
+			resp, err := ocsp.CreateResponse(c.caData.caChain[0], c.caData.caChain[0], ocsp.Response{
 				Status:       c.certStatus,
 				SerialNumber: certTemplate.SerialNumber,
 				ProducedAt:   time.Now(),
 				ThisUpdate:   time.Now(),
 				NextUpdate:   time.Now().Add(time.Hour),
-			}, pk.PrivateKey)
+			}, c.caData.privateKey)
 			if err != nil {
 				t.Fatal(err)
 			}
@@ -351,18 +400,18 @@ func TestCert_RoleResolveOCSP(t *testing.T) {
 			var resolveStep logicaltest.TestStep
 			var loginStep logicaltest.TestStep
 			if c.errExpected {
-				loginStep = testAccStepLoginWithNameInvalid(t, connState, "web")
-				resolveStep = testAccStepResolveRoleOCSPFail(t, connState, "web")
+				loginStep = testAccStepLoginWithNameInvalid(t, c.caData.connState, "web")
+				resolveStep = testAccStepResolveRoleOCSPFail(t, c.caData.connState, "web")
 			} else {
-				loginStep = testAccStepLoginWithName(t, connState, "web")
-				resolveStep = testAccStepResolveRoleWithName(t, connState, "web")
+				loginStep = testAccStepLoginWithName(t, c.caData.connState, "web")
+				resolveStep = testAccStepResolveRoleWithName(t, c.caData.connState, "web")
 			}
 			logicaltest.Test(t, logicaltest.TestCase{
 				CredentialBackend: b,
 				Steps: []logicaltest.TestStep{
-					testAccStepCertWithExtraParams(t, "web", ca, "foo", allowed{dns: "example.com"}, false,
-						map[string]interface{}{"ocsp_enabled": true, "ocsp_fail_open": c.failOpen}),
-					testAccStepReadCertPolicy(t, "web", false, map[string]interface{}{"ocsp_enabled": true, "ocsp_fail_open": c.failOpen}),
+					testAccStepCertWithExtraParams(t, "web", c.caData.caBytes, "foo", allowed{dns: "example.com"}, false,
+						map[string]interface{}{"ocsp_enabled": true, "ocsp_fail_open": c.failOpen, "ocsp_ca_certificates": c.ocspCaCerts}),
+					testAccStepReadCertPolicy(t, "web", false, map[string]interface{}{"ocsp_enabled": true, "ocsp_fail_open": c.failOpen, "ocsp_ca_certificates": c.ocspCaCerts}),
 					loginStep,
 					resolveStep,
 				},

changelog/28309.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+auth/cert: ocsp_ca_certificates field was not honored when validating OCSP responses signed by a CA that did not issue the certificate.
+```

sdk/helper/ocsp/client.go

@@ -321,6 +321,7 @@ func (c *Client) retryOCSP(
 	reqBody []byte,
 	subject,
 	issuer *x509.Certificate,
+	extraCas []*x509.Certificate,
 ) (ocspRes *ocsp.Response, ocspResBytes []byte, ocspS *ocspStatus, retErr error) {
 	doRequest := func(request *retryablehttp.Request) (*http.Response, error) {
 		if request != nil {
@@ -391,7 +392,7 @@ func (c *Client) retryOCSP(
 			continue
 		}
 
-		if err := validateOCSPParsedResponse(ocspRes, subject, issuer); err != nil {
+		if err := validateOCSPParsedResponse(ocspRes, subject, issuer, extraCas); err != nil {
 			err = fmt.Errorf("error validating %v OCSP response: %w", method, err)
 
 			if IsOcspVerificationError(err) {
@@ -443,7 +444,7 @@ func IsOcspVerificationError(err error) bool {
 	return errors.As(err, &errOcspIssuer)
 }
 
-func validateOCSPParsedResponse(ocspRes *ocsp.Response, subject, issuer *x509.Certificate) error {
+func validateOCSPParsedResponse(ocspRes *ocsp.Response, subject, issuer *x509.Certificate, extraCas []*x509.Certificate) error {
 	// Above, we use the unsafe issuer=nil parameter to ocsp.ParseResponse
 	// because Go's library does the wrong thing.
 	//
@@ -462,38 +463,59 @@ func validateOCSPParsedResponse(ocspRes *ocsp.Response, subject, issuer *x509.Ce
 	//
 	// This addresses the !!unsafe!! behavior above.
 	if ocspRes.Certificate == nil {
+		// With no certificate, we need to validate that the response is signed by the issuer or an extra CA
 		if err := ocspRes.CheckSignatureFrom(issuer); err != nil {
-			return &ErrOcspIssuerVerification{fmt.Errorf("error directly verifying signature: %w", err)}
+			if len(extraCas) > 0 {
+				// Perhaps it was signed by one of the extra configured OCSP CAs
+				matchedCA, overallErr := verifySignature(ocspRes, extraCas)
+
+				if overallErr != nil {
+					return &ErrOcspIssuerVerification{fmt.Errorf("error checking chain of trust %v failed: %w", issuer.Subject.String(), overallErr)}
+				}
+
+				err := validateSigner(matchedCA)
+				if err != nil {
+					return err
+				}
+			} else {
+				return &ErrOcspIssuerVerification{fmt.Errorf("error directly verifying signature: %w", err)}
+			}
 		}
 	} 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:
+		// this certificate. There are three ways we can do this:
 		//
 		// 1. Via confirming issuer == ocspRes.Certificate, or
 		// 2. Via confirming ocspRes.Certificate.CheckSignatureFrom(issuer).
+		// 3. Trusting extra configured OCSP CAs
 		if !bytes.Equal(issuer.Raw, ocspRes.Raw) {
-			// 1 must not hold, so 2 holds; verify the signature.
+			var overallErr error
+			var matchedCA *x509.Certificate
+
+			// Assumption 1 failed, try 2
 			if err := ocspRes.Certificate.CheckSignatureFrom(issuer); err != nil {
-				return &ErrOcspIssuerVerification{fmt.Errorf("error checking chain of trust %v failed: %w", issuer.Subject.String(), err)}
-			}
+				// Assumption 2 failed, try 3
+				overallErr = multierror.Append(overallErr, err)
 
-			// 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()) {
-				return &ErrOcspIssuerVerification{fmt.Errorf("error checking delegated OCSP responder OCSP response: certificate has expired")}
-			}
-			haveEKU := false
-			for _, ku := range ocspRes.Certificate.ExtKeyUsage {
-				if ku == x509.ExtKeyUsageOCSPSigning {
-					haveEKU = true
-					break
+				m, err := verifySignature(ocspRes, extraCas)
+				if err != nil {
+					overallErr = multierror.Append(overallErr, err)
+				} else {
+					matchedCA = m
 				}
+			} else {
+				matchedCA = ocspRes.Certificate
 			}
-			if !haveEKU {
-				return &ErrOcspIssuerVerification{fmt.Errorf("error checking delegated OCSP responder: certificate lacks the OCSP Signing EKU")}
+
+			if overallErr != nil {
+				return &ErrOcspIssuerVerification{fmt.Errorf("error checking chain of trust %v failed: %w", issuer.Subject.String(), overallErr)}
+			}
+
+			err := validateSigner(matchedCA)
+			if err != nil {
+				return err
 			}
 		}
 	}
@@ -512,6 +534,41 @@ func validateOCSPParsedResponse(ocspRes *ocsp.Response, subject, issuer *x509.Ce
 	return nil
 }
 
+func verifySignature(res *ocsp.Response, extraCas []*x509.Certificate) (*x509.Certificate, error) {
+	var overallErr error
+	var matchedCA *x509.Certificate
+	for _, ca := range extraCas {
+		if err := res.CheckSignatureFrom(ca); err != nil {
+			overallErr = multierror.Append(overallErr, err)
+		} else {
+			matchedCA = ca
+			overallErr = nil
+			break
+		}
+	}
+	return matchedCA, overallErr
+}
+
+func validateSigner(matchedCA *x509.Certificate) error {
+	// Verify the OCSP responder certificate is still valid and
+	// contains the required EKU since it is a delegated OCSP
+	// responder certificate.
+	if matchedCA.NotAfter.Before(time.Now()) {
+		return &ErrOcspIssuerVerification{fmt.Errorf("error checking delegated OCSP responder OCSP response: certificate has expired")}
+	}
+	haveEKU := false
+	for _, ku := range matchedCA.ExtKeyUsage {
+		if ku == x509.ExtKeyUsageOCSPSigning {
+			haveEKU = true
+			break
+		}
+	}
+	if !haveEKU {
+		return &ErrOcspIssuerVerification{fmt.Errorf("error checking delegated OCSP responder: certificate lacks the OCSP Signing EKU")}
+	}
+	return nil
+}
+
 // GetRevocationStatus checks the certificate revocation status for subject using issuer certificate.
 func (c *Client) GetRevocationStatus(ctx context.Context, subject, issuer *x509.Certificate, conf *VerifyConfig) (*ocspStatus, error) {
 	status, ocspReq, encodedCertID, err := c.validateWithCache(subject, issuer, conf)
@@ -564,7 +621,7 @@ func (c *Client) GetRevocationStatus(ctx context.Context, subject, issuer *x509.
 				defer wg.Done()
 			}
 			ocspRes, _, ocspS, err := c.retryOCSP(
-				ctx, ocspClient, retryablehttp.NewRequest, u, headers, ocspReq, subject, issuer)
+				ctx, ocspClient, retryablehttp.NewRequest, u, headers, ocspReq, subject, issuer, conf.ExtraCas)
 			ocspResponses[i] = ocspRes
 			if err != nil {
 				errors[i] = err

sdk/helper/ocsp/ocsp_test.go

@@ -677,7 +677,7 @@ func TestOCSPRetry(t *testing.T) {
 		context.TODO(),
 		client, fakeRequestFunc,
 		dummyOCSPHost,
-		make(map[string]string), []byte{0}, certs[0], certs[len(certs)-1])
+		make(map[string]string), []byte{0}, certs[0], certs[len(certs)-1], nil)
 	if err == nil {
 		fmt.Printf("should fail: %v, %v, %v\n", res, b, st)
 	}
@@ -692,7 +692,7 @@ func TestOCSPRetry(t *testing.T) {
 		context.TODO(),
 		client, fakeRequestFunc,
 		dummyOCSPHost,
-		make(map[string]string), []byte{0}, certs[0], certs[len(certs)-1])
+		make(map[string]string), []byte{0}, certs[0], certs[len(certs)-1], nil)
 	if err == nil {
 		fmt.Printf("should fail: %v, %v, %v\n", res, b, st)
 	}