From bcbd45b380d2cf776cb3cd920f03291301cee998 Mon Sep 17 00:00:00 2001 From: Steven Clark Date: Mon, 20 Nov 2023 10:51:03 -0500 Subject: [PATCH] Handle expired OCSP responses from server (#24193) * Handle expired OCSP responses from server - If a server replied with what we considered an expired OCSP response (nextUpdate is now or in the past), and it was our only response we would panic due to missing error handling logic. * Add cl --- changelog/24193.txt | 3 ++ sdk/helper/ocsp/client.go | 11 ++++- sdk/helper/ocsp/ocsp_test.go | 89 ++++++++++++++++++++++++++++++++++++ 3 files changed, 102 insertions(+), 1 deletion(-) create mode 100644 changelog/24193.txt diff --git a/changelog/24193.txt b/changelog/24193.txt new file mode 100644 index 0000000000..67ea1d0ae9 --- /dev/null +++ b/changelog/24193.txt @@ -0,0 +1,3 @@ +```release-note:bug +auth/cert: Handle errors related to expired OCSP server responses +``` diff --git a/sdk/helper/ocsp/client.go b/sdk/helper/ocsp/client.go index c1b9c2fbc3..8bd9cea4ee 100644 --- a/sdk/helper/ocsp/client.go +++ b/sdk/helper/ocsp/client.go @@ -517,6 +517,11 @@ func (c *Client) GetRevocationStatus(ctx context.Context, subject, issuer *x509. } if isValidOCSPStatus(ret.code) { ocspStatuses[i] = ret + } else if ret.err != nil { + // This check needs to occur after the isValidOCSPStatus as the unknown + // status also sets an err value within ret. + errors[i] = ret.err + return ret.err } return nil } @@ -568,8 +573,12 @@ func (c *Client) GetRevocationStatus(ctx context.Context, subject, issuer *x509. if (ret == nil || ret.code == ocspStatusUnknown) && firstError != nil { return nil, firstError } - // otherwise ret should contain a response for the overall request + // An extra safety in case ret and firstError are both nil + if ret == nil { + return nil, fmt.Errorf("failed to extract a known response code or error from the OCSP server") + } + // otherwise ret should contain a response for the overall request if !isValidOCSPStatus(ret.code) { return ret, nil } diff --git a/sdk/helper/ocsp/ocsp_test.go b/sdk/helper/ocsp/ocsp_test.go index 2f3f1976d2..677dfa85aa 100644 --- a/sdk/helper/ocsp/ocsp_test.go +++ b/sdk/helper/ocsp/ocsp_test.go @@ -6,14 +6,20 @@ import ( "bytes" "context" "crypto" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" "crypto/tls" "crypto/x509" + "crypto/x509/pkix" "errors" "fmt" "io" "io/ioutil" + "math/big" "net" "net/http" + "net/http/httptest" "net/url" "testing" "time" @@ -21,6 +27,7 @@ import ( "github.com/hashicorp/go-hclog" "github.com/hashicorp/go-retryablehttp" lru "github.com/hashicorp/golang-lru" + "github.com/stretchr/testify/require" "golang.org/x/crypto/ocsp" ) @@ -201,6 +208,88 @@ func TestUnitCheckOCSPResponseCache(t *testing.T) { } } +func TestUnitExpiredOCSPResponse(t *testing.T) { + rootCaKey, rootCa, leafCert := createCaLeafCerts(t) + + expiredOcspResponse := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + now := time.Now() + ocspRes := ocsp.Response{ + SerialNumber: big.NewInt(2), + ThisUpdate: now.Add(-1 * time.Hour), + NextUpdate: now.Add(-30 * time.Minute), + Status: ocsp.Good, + } + response, err := ocsp.CreateResponse(rootCa, rootCa, ocspRes, rootCaKey) + if err != nil { + _, _ = w.Write(ocsp.InternalErrorErrorResponse) + t.Fatalf("failed generating OCSP response: %v", err) + } + _, _ = w.Write(response) + }) + ts := httptest.NewServer(expiredOcspResponse) + defer ts.Close() + + logFactory := func() hclog.Logger { + return hclog.NewNullLogger() + } + client := New(logFactory, 100) + + ctx := context.Background() + + config := &VerifyConfig{ + OcspEnabled: true, + OcspServersOverride: []string{ts.URL}, + OcspFailureMode: FailOpenFalse, + QueryAllServers: false, + } + + status, err := client.GetRevocationStatus(ctx, leafCert, rootCa, config) + require.ErrorContains(t, err, "invalid validity", + "Expected error got response: %v, %v", status, err) +} + +func createCaLeafCerts(t *testing.T) (*ecdsa.PrivateKey, *x509.Certificate, *x509.Certificate) { + rootCaKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err, "failed generated root key for CA") + + // Validate we reject CSRs that contain CN that aren't in the original order + cr := &x509.Certificate{ + Subject: pkix.Name{CommonName: "Root Cert"}, + SerialNumber: big.NewInt(1), + IsCA: true, + BasicConstraintsValid: true, + SignatureAlgorithm: x509.ECDSAWithSHA256, + NotBefore: time.Now().Add(-1 * time.Second), + NotAfter: time.Now().AddDate(1, 0, 0), + KeyUsage: x509.KeyUsageCertSign | x509.KeyUsageCRLSign, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageOCSPSigning}, + } + rootCaBytes, err := x509.CreateCertificate(rand.Reader, cr, cr, &rootCaKey.PublicKey, rootCaKey) + require.NoError(t, err, "failed generating root ca") + + rootCa, err := x509.ParseCertificate(rootCaBytes) + require.NoError(t, err, "failed parsing root ca") + + leafKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err, "failed generated leaf key") + + cr = &x509.Certificate{ + Subject: pkix.Name{CommonName: "Leaf Cert"}, + SerialNumber: big.NewInt(2), + SignatureAlgorithm: x509.ECDSAWithSHA256, + NotBefore: time.Now().Add(-1 * time.Second), + NotAfter: time.Now().AddDate(1, 0, 0), + KeyUsage: x509.KeyUsageDigitalSignature, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + } + leafCertBytes, err := x509.CreateCertificate(rand.Reader, cr, rootCa, &leafKey.PublicKey, rootCaKey) + require.NoError(t, err, "failed generating root ca") + + leafCert, err := x509.ParseCertificate(leafCertBytes) + require.NoError(t, err, "failed parsing root ca") + return rootCaKey, rootCa, leafCert +} + func TestUnitValidateOCSP(t *testing.T) { ocspRes := &ocsp.Response{} ost, err := validateOCSP(ocspRes)