Skip to content

Commit

Permalink
backport of commit 17a2827 (#20199)
Browse files Browse the repository at this point in the history 
Co-authored-by: Alexander Scheel <alex.scheel@hashicorp.com>
  • Loading branch information
@hc-github-team-secure-vault-core @cipherboy
hc-github-team-secure-vault-core and cipherboy authored Apr 17, 2023
1 parent 242d8f3 commit b8997a7
Show file tree
Hide file tree
Showing 3 changed files with 234 additions and 1 deletion.
4 changes: 4 additions & 0 deletions changelog/20181.txt
View file
Original file line number Diff line number Diff line change
@@ -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.
```
64 changes: 63 additions & 1 deletion sdk/helper/ocsp/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
167 changes: 167 additions & 0 deletions sdk/helper/ocsp/ocsp_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"crypto"
"crypto/tls"
"crypto/x509"
"encoding/pem"
"errors"
"fmt"
"io"
Expand All @@ -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"
)

Expand Down Expand Up @@ -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 {
Expand Down

0 comments on commit b8997a7

Please sign in to comment.