luxolus
/
open-vault
2
0
Fork 0
Code Issues 1 Pull Requests Releases Activity

ACME tests for Intermediate CA issuance prevention (#20633) 

* Do not set use_csr_values when issuing ACME certs

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Ensure CSRs with Basic Constraints are rejected

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Add test to ensure CA certificates cannot be issued

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Update builtin/logical/pkiext/pkiext_binary/acme_test.go

Co-authored-by: Steven Clark <steven.clark@hashicorp.com>

* Update builtin/logical/pkiext/pkiext_binary/acme_test.go

Co-authored-by: Steven Clark <steven.clark@hashicorp.com>

* Update acme_test.go to include certutil

* Update acme_test.go - unused imports, reformat

* Update acme_test.go - hex really was used

This is why I can't use the GH web editor. :-)

---------

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
Co-authored-by: Steven Clark <steven.clark@hashicorp.com>
This commit is contained in:
Alexander Scheel 2023-05-17 15:54:37 -04:00 committed by GitHub
parent 143a785c21
commit b204e51263
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 122 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

22
builtin/logical/pki/path_acme_order.go
View File

@ -471,7 +471,20 @@ func issueCertFromCsr(ac *acmeContext, csr *x509.CertificateRequest) (*certutil.
return nil, "", fmt.Errorf("%w: Refusing to sign CSR with empty PublicKey", ErrBadCSR)
}
parsedBundle, _, err := signCert(ac.sc.Backend, input, signingBundle, false, true)
// UseCSRValues as defined in certutil/helpers.go accepts the following
// fields off of the CSR:
//
// 1. Subject fields,
// 2. SANs,
// 3. Extensions (except for a BasicConstraint extension)
//
// Because we have stricter validation of subject parameters, and no way
// to validate or allow extensions, we do not wish to use the CSR's
// parameters for these values. If a CSR sets, e.g., an organizational
// unit, we have no way of validating this (via ACME here, without perhaps
// an external policy engine), and thus should not be setting it on our
// final issued certificate.
parsedBundle, _, err := signCert(ac.sc.Backend, input, signingBundle, false /* is_ca=false */, false /* use_csr_values */)
if err != nil {
return nil, "", fmt.Errorf("%w: refusing to sign CSR: %s", ErrBadCSR, err.Error())
}
@ -507,6 +520,13 @@ func parseCsrFromFinalize(data map[string]interface{}) (*x509.CertificateRequest
if csr.PublicKey == nil || csr.PublicKeyAlgorithm == x509.UnknownPublicKeyAlgorithm {
return nil, fmt.Errorf("%w: failed to parse csr no public key info or unknown key algorithm used", ErrBadCSR)
}
for _, ext := range csr.Extensions {
if ext.Id.Equal(certutil.ExtensionBasicConstraintsOID) {
return nil, fmt.Errorf("%w: refusing to accept CSR with Basic Constraints extension", ErrBadCSR)
}
}
return csr, nil
}

106
builtin/logical/pkiext/pkiext_binary/acme_test.go
View File

@ -12,6 +12,7 @@ import (
"crypto/tls"
"crypto/x509"
"crypto/x509/pkix"
"encoding/hex"
"net"
"net/http"
"path"
@ -21,6 +22,7 @@ import (
"golang.org/x/crypto/acme"
"github.com/hashicorp/vault/builtin/logical/pkiext"
"github.com/hashicorp/vault/sdk/helper/certutil"
hDocker "github.com/hashicorp/vault/sdk/helper/docker"
"github.com/stretchr/testify/require"
)
@ -33,9 +35,10 @@ func Test_ACME(t *testing.T) {
defer cluster.Cleanup()
tc := map[string]func(t *testing.T, cluster *VaultPkiCluster){
"certbot": SubtestACMECertbot,
"acme ip sans": SubtestACMEIPAndDNS,
"acme wildcard": SubtestACMEWildcardDNS,
"certbot": SubtestACMECertbot,
"acme ip sans": SubtestACMEIPAndDNS,
"acme wildcard": SubtestACMEWildcardDNS,
"acme prevents ica": SubtestACMEPreventsICADNS,
}
// Wrap the tests within an outer group, so that we run all tests
@ -255,7 +258,7 @@ func SubtestACMEIPAndDNS(t *testing.T, cluster *VaultPkiCluster) {
return challengesToAccept
}
acmeCert := doAcmeValidationWithGoLibrary(t, directoryUrl, acmeOrderIdentifiers, cr, provisioningFunc)
acmeCert := doAcmeValidationWithGoLibrary(t, directoryUrl, acmeOrderIdentifiers, cr, provisioningFunc, "")
require.Len(t, acmeCert.IPAddresses, 1, "expected only a single ip address in cert")
require.Equal(t, ipAddr, acmeCert.IPAddresses[0].String())
@ -278,7 +281,7 @@ func SubtestACMEIPAndDNS(t *testing.T, cluster *VaultPkiCluster) {
IPAddresses: []net.IP{net.ParseIP(ipAddr)},
}
acmeCert = doAcmeValidationWithGoLibrary(t, directoryUrl, acmeOrderIdentifiers, cr, provisioningFunc)
acmeCert = doAcmeValidationWithGoLibrary(t, directoryUrl, acmeOrderIdentifiers, cr, provisioningFunc, "")
require.Len(t, acmeCert.IPAddresses, 1, "expected only a single ip address in cert")
require.Equal(t, ipAddr, acmeCert.IPAddresses[0].String())
@ -288,7 +291,7 @@ func SubtestACMEIPAndDNS(t *testing.T, cluster *VaultPkiCluster) {
type acmeGoValidatorProvisionerFunc func(acmeClient *acme.Client, auths []*acme.Authorization) []*acme.Challenge
func doAcmeValidationWithGoLibrary(t *testing.T, directoryUrl string, acmeOrderIdentifiers []acme.AuthzID, cr *x509.CertificateRequest, provisioningFunc acmeGoValidatorProvisionerFunc) *x509.Certificate {
func doAcmeValidationWithGoLibrary(t *testing.T, directoryUrl string, acmeOrderIdentifiers []acme.AuthzID, cr *x509.CertificateRequest, provisioningFunc acmeGoValidatorProvisionerFunc, expectedFailure string) *x509.Certificate {
// Since we are contacting Vault through the host ip/port, the certificate will not validate properly
tr := &http.Transport{
TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
@ -343,8 +346,19 @@ func doAcmeValidationWithGoLibrary(t *testing.T, directoryUrl string, acmeOrderI
csr, err := x509.CreateCertificateRequest(rand.Reader, cr, csrKey)
require.NoError(t, err, "failed generating csr")
t.Logf("[TEST-LOG] Created CSR: %v", hex.EncodeToString(csr))
certs, _, err := acmeClient.CreateOrderCert(testCtx, order.FinalizeURL, csr, false)
require.NoError(t, err, "failed to get a certificate back from ACME")
if err != nil {
if expectedFailure != "" {
require.Contains(t, err.Error(), expectedFailure, "got a unexpected failure not matching expected value")
return nil
}
require.NoError(t, err, "failed to get a certificate back from ACME")
} else if expectedFailure != "" {
t.Fatalf("expected failure containing: %s got none", expectedFailure)
}
acmeCert, err := x509.ParseCertificate(certs[0])
require.NoError(t, err, "failed parsing acme cert bytes")
@ -402,7 +416,7 @@ func SubtestACMEWildcardDNS(t *testing.T, cluster *VaultPkiCluster) {
return challengesToAccept
}
acmeCert := doAcmeValidationWithGoLibrary(t, directoryUrl, acmeOrderIdentifiers, cr, provisioningFunc)
acmeCert := doAcmeValidationWithGoLibrary(t, directoryUrl, acmeOrderIdentifiers, cr, provisioningFunc, "")
require.Contains(t, acmeCert.DNSNames, hostname)
require.Contains(t, acmeCert.DNSNames, wildcard)
require.Equal(t, wildcard, acmeCert.Subject.CommonName)
@ -419,13 +433,87 @@ func SubtestACMEWildcardDNS(t *testing.T, cluster *VaultPkiCluster) {
require.NoError(t, err, "failed creating role wildcard")
directoryUrl = basePath + "/roles/wildcard/acme/directory"
acmeCert = doAcmeValidationWithGoLibrary(t, directoryUrl, acmeOrderIdentifiers, cr, provisioningFunc)
acmeCert = doAcmeValidationWithGoLibrary(t, directoryUrl, acmeOrderIdentifiers, cr, provisioningFunc, "")
require.Contains(t, acmeCert.DNSNames, hostname)
require.Contains(t, acmeCert.DNSNames, wildcard)
require.Equal(t, wildcard, acmeCert.Subject.CommonName)
pki.RemoveDNSRecordsForDomain(hostname)
}
func SubtestACMEPreventsICADNS(t *testing.T, cluster *VaultPkiCluster) {
pki, err := cluster.CreateAcmeMount("pki-dns-ica")
require.NoError(t, err, "failed setting up acme mount")
// Since we interact with ACME from outside the container network the ACME
// configuration needs to be updated to use the host port and not the internal
// docker ip.
basePath, err := pki.UpdateClusterConfigLocalAddr()
require.NoError(t, err, "failed updating cluster config")
hostname := "go-lang-intermediate-ca-cert.dadgarcorp.com"
// Do validation without a role first.
directoryUrl := basePath + "/acme/directory"
acmeOrderIdentifiers := []acme.AuthzID{
{Type: "dns", Value: hostname},
}
cr := &x509.CertificateRequest{
Subject: pkix.Name{CommonName: hostname},
DNSNames: []string{hostname},
ExtraExtensions: []pkix.Extension{
// Basic Constraint with IsCA asserted to true.
{
Id: certutil.ExtensionBasicConstraintsOID,
Critical: true,
Value: []byte{0x30, 0x03, 0x01, 0x01, 0xFF},
},
},
}
provisioningFunc := func(acmeClient *acme.Client, auths []*acme.Authorization) []*acme.Challenge {
// For each dns-01 challenge, place the record in the associated DNS resolver.
var challengesToAccept []*acme.Challenge
for _, auth := range auths {
for _, challenge := range auth.Challenges {
if challenge.Status != acme.StatusPending {
t.Logf("ignoring challenge not in status pending: %v", challenge)
continue
}
if challenge.Type == "dns-01" {
challengeBody, err := acmeClient.DNS01ChallengeRecord(challenge.Token)
require.NoError(t, err, "failed generating challenge response")
err = pki.AddDNSRecord("_acme-challenge."+auth.Identifier.Value, "TXT", challengeBody)
require.NoError(t, err, "failed setting DNS record")
challengesToAccept = append(challengesToAccept, challenge)
}
}
}
require.GreaterOrEqual(t, len(challengesToAccept), 1, "Need at least one challenge, got none")
return challengesToAccept
}
doAcmeValidationWithGoLibrary(t, directoryUrl, acmeOrderIdentifiers, cr, provisioningFunc, "refusing to accept CSR with Basic Constraints extension")
pki.RemoveDNSRecordsForDomain(hostname)
// Redo validation with a role this time.
err = pki.UpdateRole("ica", map[string]interface{}{
"key_type": "any",
"allowed_domains": "go-lang-intermediate-ca-cert.dadgarcorp.com",
"allow_subdomains": true,
"allow_bare_domains": true,
"allow_wildcard_certificates": true,
})
require.NoError(t, err, "failed creating role wildcard")
directoryUrl = basePath + "/roles/ica/acme/directory"
doAcmeValidationWithGoLibrary(t, directoryUrl, acmeOrderIdentifiers, cr, provisioningFunc, "refusing to accept CSR with Basic Constraints extension")
pki.RemoveDNSRecordsForDomain(hostname)
}
func getDockerLog(t *testing.T) (func(s string), *pkiext.LogConsumerWriter, *pkiext.LogConsumerWriter) {
logConsumer := func(s string) {
t.Logf(s)

8
sdk/helper/certutil/helpers.go
View File

@ -1042,8 +1042,8 @@ func selectSignatureAlgorithmForECDSA(pub crypto.PublicKey, signatureBits int) x
}
var (
oidExtensionBasicConstraints = []int{2, 5, 29, 19}
oidExtensionSubjectAltName = []int{2, 5, 29, 17}
ExtensionBasicConstraintsOID = []int{2, 5, 29, 19}
ExtensionSubjectAltNameOID = []int{2, 5, 29, 17}
)
// CreateCSR creates a CSR with the default rand.Reader to
@ -1098,7 +1098,7 @@ func createCSR(data *CreationBundle, addBasicConstraints bool, randReader io.Rea
return nil, errutil.InternalError{Err: errwrap.Wrapf("error marshaling basic constraints: {{err}}", err).Error()}
}
ext := pkix.Extension{
Id: oidExtensionBasicConstraints,
Id: ExtensionBasicConstraintsOID,
Value: val,
Critical: true,
}
@ -1219,7 +1219,7 @@ func signCertificate(data *CreationBundle, randReader io.Reader) (*ParsedCertBun
certTemplate.URIs = data.CSR.URIs
for _, name := range data.CSR.Extensions {
if !name.Id.Equal(oidExtensionBasicConstraints) && !(len(data.Params.OtherSANs) > 0 && name.Id.Equal(oidExtensionSubjectAltName)) {
if !name.Id.Equal(ExtensionBasicConstraintsOID) && !(len(data.Params.OtherSANs) > 0 && name.Id.Equal(ExtensionSubjectAltNameOID)) {
certTemplate.ExtraExtensions = append(certTemplate.ExtraExtensions, name)
}
}