From cda20e39b1ed16ebbee8b7e26a899ad2166d9e2b Mon Sep 17 00:00:00 2001 From: Scott Miller Date: Wed, 11 Sep 2024 09:57:27 -0500 Subject: [PATCH] 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 | 9 ++ builtin/credential/cert/path_login_test.go | 93 +++++++++++++++----- changelog/28309.txt | 3 + sdk/helper/ocsp/client.go | 99 +++++++++++++++++----- sdk/helper/ocsp/ocsp_test.go | 4 +- 5 files changed, 163 insertions(+), 45 deletions(-) create mode 100644 changelog/28309.txt diff --git a/builtin/credential/cert/path_login.go b/builtin/credential/cert/path_login.go index 832036f12dc0..53571b26185e 100644 --- a/builtin/credential/cert/path_login.go +++ b/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 + } } } diff --git a/builtin/credential/cert/path_login_test.go b/builtin/credential/cert/path_login_test.go index 51d670ce8c40..ad1030464f35 100644 --- a/builtin/credential/cert/path_login_test.go +++ b/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, }, diff --git a/changelog/28309.txt b/changelog/28309.txt new file mode 100644 index 000000000000..94574b312103 --- /dev/null +++ b/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. +``` \ No newline at end of file diff --git a/sdk/helper/ocsp/client.go b/sdk/helper/ocsp/client.go index cef1f6896b48..872ca717da5e 100644 --- a/sdk/helper/ocsp/client.go +++ b/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 diff --git a/sdk/helper/ocsp/ocsp_test.go b/sdk/helper/ocsp/ocsp_test.go index 111576653428..063756d28cae 100644 --- a/sdk/helper/ocsp/ocsp_test.go +++ b/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) }