From 2d967eeda0ea3d66eb67ccd30fa53a0fab188a3e Mon Sep 17 00:00:00 2001 From: Steve Clark Date: Mon, 20 Nov 2023 09:53:08 -0500 Subject: [PATCH 1/2] 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. --- sdk/helper/ocsp/client.go | 11 ++++- sdk/helper/ocsp/ocsp_test.go | 89 ++++++++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 1 deletion(-) diff --git a/sdk/helper/ocsp/client.go b/sdk/helper/ocsp/client.go index c1b9c2fbc37a..8bd9cea4ee8f 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 2f3f1976d2a8..677dfa85aa5e 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) From 237ca3aff13642f7df4db0bf8e0564c896e08494 Mon Sep 17 00:00:00 2001 From: Steve Clark Date: Mon, 20 Nov 2023 09:59:21 -0500 Subject: [PATCH 2/2] Add cl --- changelog/24193.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/24193.txt diff --git a/changelog/24193.txt b/changelog/24193.txt new file mode 100644 index 000000000000..67ea1d0ae974 --- /dev/null +++ b/changelog/24193.txt @@ -0,0 +1,3 @@ +```release-note:bug +auth/cert: Handle errors related to expired OCSP server responses +```