From b204e512635d24bcca09848abc450953ac534bed Mon Sep 17 00:00:00 2001 From: Alexander Scheel Date: Wed, 17 May 2023 15:54:37 -0400 Subject: [PATCH] ACME tests for Intermediate CA issuance prevention (#20633) * Do not set use_csr_values when issuing ACME certs Signed-off-by: Alexander Scheel * Ensure CSRs with Basic Constraints are rejected Signed-off-by: Alexander Scheel * Add test to ensure CA certificates cannot be issued Signed-off-by: Alexander Scheel * Update builtin/logical/pkiext/pkiext_binary/acme_test.go Co-authored-by: Steven Clark * Update builtin/logical/pkiext/pkiext_binary/acme_test.go Co-authored-by: Steven Clark * 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 Co-authored-by: Steven Clark --- builtin/logical/pki/path_acme_order.go | 22 +++- .../logical/pkiext/pkiext_binary/acme_test.go | 106 ++++++++++++++++-- sdk/helper/certutil/helpers.go | 8 +- 3 files changed, 122 insertions(+), 14 deletions(-) diff --git a/builtin/logical/pki/path_acme_order.go b/builtin/logical/pki/path_acme_order.go index 22ba2917a..a8d491b30 100644 --- a/builtin/logical/pki/path_acme_order.go +++ b/builtin/logical/pki/path_acme_order.go @@ -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 } diff --git a/builtin/logical/pkiext/pkiext_binary/acme_test.go b/builtin/logical/pkiext/pkiext_binary/acme_test.go index 946fc37ee..d52ad1cd5 100644 --- a/builtin/logical/pkiext/pkiext_binary/acme_test.go +++ b/builtin/logical/pkiext/pkiext_binary/acme_test.go @@ -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) diff --git a/sdk/helper/certutil/helpers.go b/sdk/helper/certutil/helpers.go index ab1aecd5a..b26a628f3 100644 --- a/sdk/helper/certutil/helpers.go +++ b/sdk/helper/certutil/helpers.go @@ -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) } }