Add fix for Go x/crypto/ocsp failure case (hashicorp#20181)

* Add fix for Go x/crypto/ocsp failure case When calling ocsp.ParseRequest(req, issue) with a non-nil issuer on a ocsp request which _unknowingly_ contains an entry in the BasicOCSPResponse's certs field, Go incorrectly assumes that the issuer is a direct parent of the _first_ certificate in the certs field, discarding the rest. As documented in the Go issue, this is not a valid assumption and thus causes OCSP verification to fail in Vault with an error like: > bad OCSP signature: crypto/rsa: verification error which ultimately leads to a cert auth login error of: > no chain matching all constraints could be found for this login certificate We address this by using the unsafe issuer=nil argument, taking on the task of validating the OCSP response's signature as best we can in the absence of full chain information on either side (both the trusted certificate whose OCSP response we're verifying and the lack of any additional certs the OCSP responder may have sent). See also: golang/go#59641 Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com> * Add test case with Vault PKI Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com> * Add changelog entry Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com> --------- Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
fopina · Apr 17, 2023 · 17a2827 · 17a2827
1 parent 3acbedd
commit 17a2827
Show file tree

Hide file tree

Showing 3 changed files with 234 additions and 1 deletion.
diff --git a/changelog/20181.txt b/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.
+```
diff --git a/sdk/helper/ocsp/client.go b/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

diff --git a/sdk/helper/ocsp/ocsp_test.go b/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 {