Add fix for Go x/crypto/ocsp failure case

changelog/20181.txt

@@ -0,0 +1,4 @@
+```release-note:bug
+sdk/helper/ocsp: Workaround bug in Go's ocsp.ParseResponse(...), causing validation to fail with embedded CA certificates.
+auth/cert: Fix OCSP validation against Vault's PKI engine.
+```

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

sdk/helper/ocsp/ocsp_test.go

@@ -8,6 +8,7 @@ import (
 	"crypto"
 	"crypto/tls"
 	"crypto/x509"
+	"encoding/pem"
 	"errors"
 	"fmt"
 	"io"
@@ -18,9 +19,16 @@ import (
 	"testing"
 	"time"
 
+	"github.com/hashicorp/vault/api"
+	"github.com/hashicorp/vault/builtin/logical/pki"
+	vaulthttp "github.com/hashicorp/vault/http"
+	"github.com/hashicorp/vault/sdk/logical"
+	"github.com/hashicorp/vault/vault"
+
 	"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"
 )
 
@@ -424,6 +432,165 @@ func TestCanEarlyExitForOCSP(t *testing.T) {
 	}
 }
 
+func TestWithVaultPKI(t *testing.T) {
+	t.Parallel()
+
+	coreConfig := &vault.CoreConfig{
+		LogicalBackends: map[string]logical.Factory{
+			"pki": pki.Factory,
+		},
+	}
+	cluster := vault.NewTestCluster(t, coreConfig, &vault.TestClusterOptions{
+		HandlerFunc: vaulthttp.Handler,
+	})
+
+	cluster.Start()
+	defer cluster.Cleanup()
+	cores := cluster.Cores
+	vault.TestWaitActive(t, cores[0].Core)
+	client := cores[0].Client
+
+	err := client.Sys().Mount("pki", &api.MountInput{
+		Type: "pki",
+		Config: api.MountConfigInput{
+			DefaultLeaseTTL: "16h",
+			MaxLeaseTTL:     "32h",
+		},
+	})
+	require.NoError(t, err)
+
+	resp, err := client.Logical().Write("pki/root/generate/internal", map[string]interface{}{
+		"ttl":         "40h",
+		"common_name": "Root R1",
+		"key_type":    "ec",
+	})
+	require.NoError(t, err)
+	require.NotNil(t, resp)
+	require.NotNil(t, resp.Data)
+	require.NotEmpty(t, resp.Data["issuer_id"])
+	rootIssuerId := resp.Data["issuer_id"].(string)
+
+	// Set URLs pointing to the issuer.
+	_, err = client.Logical().Write("pki/config/cluster", map[string]interface{}{
+		"path":     client.Address() + "/v1/pki",
+		"aia_path": client.Address() + "/v1/pki",
+	})
+	require.NoError(t, err)
+
+	_, err = client.Logical().Write("pki/config/urls", map[string]interface{}{
+		"enable_templating":       true,
+		"crl_distribution_points": "{{cluster_aia_path}}/issuer/{{issuer_id}}/crl/der",
+		"issuing_certificates":    "{{cluster_aia_path}}/issuer/{{issuer_id}}/der",
+		"ocsp_servers":            "{{cluster_aia_path}}/ocsp",
+	})
+	require.NoError(t, err)
+
+	// Build an intermediate CA
+	resp, err = client.Logical().Write("pki/intermediate/generate/internal", map[string]interface{}{
+		"common_name": "Int X1",
+		"key_type":    "ec",
+	})
+	require.NoError(t, err)
+	require.NotNil(t, resp)
+	require.NotNil(t, resp.Data)
+	require.NotEmpty(t, resp.Data["csr"])
+	intermediateCSR := resp.Data["csr"].(string)
+
+	resp, err = client.Logical().Write("pki/root/sign-intermediate", map[string]interface{}{
+		"csr": intermediateCSR,
+		"ttl": "20h",
+	})
+	require.NoError(t, err)
+	require.NotNil(t, resp)
+	require.NotNil(t, resp.Data)
+	require.NotEmpty(t, resp.Data["certificate"])
+	intermediateCert := resp.Data["certificate"]
+
+	resp, err = client.Logical().Write("pki/intermediate/set-signed", map[string]interface{}{
+		"certificate": intermediateCert,
+	})
+	require.NoError(t, err)
+	require.NotNil(t, resp)
+	require.NotNil(t, resp.Data)
+	require.NotEmpty(t, resp.Data["imported_issuers"])
+	rawImportedIssuers := resp.Data["imported_issuers"].([]interface{})
+	require.Equal(t, len(rawImportedIssuers), 1)
+	importedIssuer := rawImportedIssuers[0].(string)
+	require.NotEmpty(t, importedIssuer)
+
+	// Set intermediate as default.
+	_, err = client.Logical().Write("pki/config/issuers", map[string]interface{}{
+		"default": importedIssuer,
+	})
+	require.NoError(t, err)
+
+	// Setup roles for root, intermediate.
+	_, err = client.Logical().Write("pki/roles/example-root", map[string]interface{}{
+		"allowed_domains":  "example.com",
+		"allow_subdomains": "true",
+		"max_ttl":          "1h",
+		"key_type":         "ec",
+		"issuer_ref":       rootIssuerId,
+	})
+	require.NoError(t, err)
+
+	_, err = client.Logical().Write("pki/roles/example-int", map[string]interface{}{
+		"allowed_domains":  "example.com",
+		"allow_subdomains": "true",
+		"max_ttl":          "1h",
+		"key_type":         "ec",
+	})
+	require.NoError(t, err)
+
+	// Issue certs and validate them against OCSP.
+	for _, path := range []string{"pki/issue/example-int", "pki/issue/example-root"} {
+		t.Logf("Validating against path: %v", path)
+		resp, err = client.Logical().Write(path, map[string]interface{}{
+			"common_name": "test.example.com",
+			"ttl":         "5m",
+		})
+		require.NoError(t, err)
+		require.NotNil(t, resp)
+		require.NotNil(t, resp.Data)
+		require.NotEmpty(t, resp.Data["certificate"])
+		require.NotEmpty(t, resp.Data["issuing_ca"])
+		require.NotEmpty(t, resp.Data["serial_number"])
+
+		certPEM := resp.Data["certificate"].(string)
+		certBlock, _ := pem.Decode([]byte(certPEM))
+		require.NotNil(t, certBlock)
+		cert, err := x509.ParseCertificate(certBlock.Bytes)
+		require.NoError(t, err)
+		require.NotNil(t, cert)
+
+		issuerPEM := resp.Data["issuing_ca"].(string)
+		issuerBlock, _ := pem.Decode([]byte(issuerPEM))
+		require.NotNil(t, issuerBlock)
+		issuer, err := x509.ParseCertificate(issuerBlock.Bytes)
+		require.NoError(t, err)
+		require.NotNil(t, issuer)
+
+		serialNumber := resp.Data["serial_number"].(string)
+
+		conf := &VerifyConfig{
+			OcspFailureMode: FailOpenFalse,
+			ExtraCas:        []*x509.Certificate{cluster.CACert},
+		}
+		ocspClient := New(testLogFactory, 10)
+
+		err = ocspClient.VerifyLeafCertificate(context.Background(), cert, issuer, conf)
+		require.NoError(t, err)
+
+		_, err = client.Logical().Write("pki/revoke", map[string]interface{}{
+			"serial_number": serialNumber,
+		})
+		require.NoError(t, err)
+
+		err = ocspClient.VerifyLeafCertificate(context.Background(), cert, issuer, conf)
+		require.Error(t, err)
+	}
+}
+
 var testLogger = hclog.New(hclog.DefaultOptions)
 
 func testLogFactory() hclog.Logger {