Skip to content

Commit

Permalink
Use Go's in-built permitted DNS domain logic (#4908)
Browse files Browse the repository at this point in the history
Fixes #4863
  • Loading branch information
jefferai authored Jul 11, 2018
1 parent 57b77dc commit 9680045
Show file tree
Hide file tree
Showing 4 changed files with 2 additions and 233 deletions.
4 changes: 2 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ BUG FIXES:

* core: Fix returning 500 instead of 503 if a rekey is attempted when Vault is
sealed [GH-4874]
* secrets/pki: Fix permitted DNS domains requiring a prefix of `.` to allow
subdomains [GH-4863]
* secrets/pki: Fix permitted DNS domains performing improper validation
[GH-4863]
* secrets/database: Fix panic during DB creds revocation [GH-4846]

## 0.10.3 (June 20th, 2018)
Expand Down
1 change: 0 additions & 1 deletion builtin/credential/cert/backend_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,6 @@ func TestBackend_PermittedDNSDomainsIntermediateCA(t *testing.T) {
// Create a new api client with the desired TLS configuration
newClient := getAPIClient(cores[0].Listeners[0].Address.Port, cores[0].TLSConfig)

// Set the intermediate CA cert as a trusted certificate in the backend
secret, err = newClient.Logical().Write("auth/cert/login", map[string]interface{}{
"name": "myvault-dot-com",
})
Expand Down
178 changes: 0 additions & 178 deletions builtin/logical/pki/backend_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1810,184 +1810,6 @@ func TestBackend_Root_Idempotency(t *testing.T) {
}
}

func TestBackend_Permitted_DNS_Domains(t *testing.T) {
coreConfig := &vault.CoreConfig{
LogicalBackends: map[string]logical.Factory{
"pki": Factory,
},
}
cluster := vault.NewTestCluster(t, coreConfig, &vault.TestClusterOptions{
HandlerFunc: vaulthttp.Handler,
})
cluster.Start()
defer cluster.Cleanup()

client := cluster.Cores[0].Client
var err error
err = client.Sys().Mount("root", &api.MountInput{
Type: "pki",
Config: api.MountConfigInput{
DefaultLeaseTTL: "16h",
MaxLeaseTTL: "32h",
},
})
if err != nil {
t.Fatal(err)
}
err = client.Sys().Mount("int", &api.MountInput{
Type: "pki",
Config: api.MountConfigInput{
DefaultLeaseTTL: "4h",
MaxLeaseTTL: "20h",
},
})
if err != nil {
t.Fatal(err)
}

// Direct issuing from root
_, err = client.Logical().Write("root/root/generate/internal", map[string]interface{}{
"ttl": "40h",
"common_name": "myvault.com",
"permitted_dns_domains": []string{"foobar.com", "zipzap.com"},
})
if err != nil {
t.Fatal(err)
}

clientKey, err := rsa.GenerateKey(rand.Reader, 2048)
if err != nil {
t.Fatal(err)
}

path := "root/"
checkIssue := func(valid bool, args ...interface{}) {
t.Helper()
argMap := map[string]interface{}{}
var currString string
for i, arg := range args {
if i%2 == 0 {
currString = arg.(string)
} else {
argMap[currString] = arg
}
}
// We do this to ensure writing a key type of any is invalid when
// issuing and valid when signing
_, err = client.Logical().Write(path+"roles/example", map[string]interface{}{
"allowed_domains": "foobar.com,zipzap.com,abc.com,xyz.com",
"allow_subdomains": true,
"allow_bare_domains": true,
"max_ttl": "2h",
"key_type": "any",
})
if err != nil {
t.Fatal(err)
}
_, err = client.Logical().Write(path+"issue/example", argMap)
if err == nil {
t.Fatal("expected err from key_type any")
}
// Now put it back
_, err = client.Logical().Write(path+"roles/example", map[string]interface{}{
"allowed_domains": "foobar.com,zipzap.com,abc.com,xyz.com",
"allow_subdomains": true,
"allow_bare_domains": true,
"max_ttl": "2h",
"key_type": "rsa",
})
if err != nil {
t.Fatal(err)
}
_, err = client.Logical().Write(path+"issue/example", argMap)
switch {
case valid && err != nil:
t.Fatal(err)
case !valid && err == nil:
t.Fatal("expected error")
}

csr, err := x509.CreateCertificateRequest(rand.Reader, &x509.CertificateRequest{
Subject: pkix.Name{
CommonName: argMap["common_name"].(string),
},
}, clientKey)
if err != nil {
t.Fatal(err)
}
delete(argMap, "common_name")
argMap["csr"] = strings.TrimSpace(string(pem.EncodeToMemory(&pem.Block{
Type: "CERTIFICATE REQUEST",
Bytes: csr,
})))

_, err = client.Logical().Write(path+"sign/example", argMap)
switch {
case valid && err != nil:
t.Fatal(err)
case !valid && err == nil:
t.Fatal("expected error")
}
}

// Check issuing and signing against root's permitted domains
checkIssue(false, "common_name", "zpzap.com")
checkIssue(false, "common_name", "hostfoobar.com")
checkIssue(true, "common_name", "host.zipzap.com")
checkIssue(true, "common_name", "foobar.com")

// Verify that root also won't issue an intermediate outside of its permitted domains
resp, err := client.Logical().Write("int/intermediate/generate/internal", map[string]interface{}{
"common_name": "issuer.abc.com",
})
if err != nil {
t.Fatal(err)
}
_, err = client.Logical().Write("root/root/sign-intermediate", map[string]interface{}{
"common_name": "issuer.abc.com",
"csr": resp.Data["csr"],
"permitted_dns_domains": []string{"abc.com", "xyz.com"},
"ttl": "5h",
})
if err == nil {
t.Fatal("expected error")
}
_, err = client.Logical().Write("root/root/sign-intermediate", map[string]interface{}{
"use_csr_values": true,
"csr": resp.Data["csr"],
"permitted_dns_domains": []string{"abc.com", "xyz.com"},
"ttl": "5h",
})
if err == nil {
t.Fatal("expected error")
}

// Sign a valid intermediate
resp, err = client.Logical().Write("root/root/sign-intermediate", map[string]interface{}{
"common_name": "issuer.zipzap.com",
"csr": resp.Data["csr"],
"permitted_dns_domains": []string{"abc.com", "xyz.com"},
"ttl": "5h",
})
if err != nil {
t.Fatal(err)
}
resp, err = client.Logical().Write("int/intermediate/set-signed", map[string]interface{}{
"certificate": resp.Data["certificate"],
})
if err != nil {
t.Fatal(err)
}

// Check enforcement with the intermediate's set values
path = "int/"
checkIssue(false, "common_name", "hostabc.com")
checkIssue(true, "common_name", "host.abc.com")
checkIssue(true, "common_name", "xyz.com")
checkIssue(true, "common_name", "abc.com")
checkIssue(true, "common_name", "host.xyz.com")
}

func TestBackend_SignIntermediate_AllowedPastCA(t *testing.T) {
coreConfig := &vault.CoreConfig{
LogicalBackends: map[string]logical.Factory{
Expand Down
52 changes: 0 additions & 52 deletions builtin/logical/pki/cert_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -556,7 +556,6 @@ func generateCert(ctx context.Context,

if isCA {
data.params.IsCA = isCA

data.params.PermittedDNSDomains = data.apiData.Get("permitted_dns_domains").([]string)

if data.signingBundle == nil {
Expand Down Expand Up @@ -1217,11 +1216,6 @@ func createCertificate(data *dataBundle) (*certutil.ParsedCertBundle, error) {
caCert := data.signingBundle.Certificate
certTemplate.AuthorityKeyId = caCert.SubjectKeyId

err = checkPermittedDNSDomains(certTemplate, caCert)
if err != nil {
return nil, errutil.UserError{Err: err.Error()}
}

certBytes, err = x509.CreateCertificate(rand.Reader, certTemplate, caCert, result.PrivateKey.Public(), data.signingBundle.PrivateKey)
} else {
// Creating a self-signed root
Expand Down Expand Up @@ -1443,10 +1437,6 @@ func signCertificate(data *dataBundle) (*certutil.ParsedCertBundle, error) {
certTemplate.PermittedDNSDomains = data.params.PermittedDNSDomains
certTemplate.PermittedDNSDomainsCritical = true
}
err = checkPermittedDNSDomains(certTemplate, caCert)
if err != nil {
return nil, errutil.UserError{Err: err.Error()}
}

certBytes, err = x509.CreateCertificate(rand.Reader, certTemplate, caCert, data.csr.PublicKey, data.signingBundle.PrivateKey)

Expand All @@ -1465,48 +1455,6 @@ func signCertificate(data *dataBundle) (*certutil.ParsedCertBundle, error) {
return result, nil
}

func checkPermittedDNSDomains(template, ca *x509.Certificate) error {
if len(ca.PermittedDNSDomains) == 0 {
return nil
}

namesToCheck := map[string]struct{}{
template.Subject.CommonName: struct{}{},
}
for _, name := range template.DNSNames {
namesToCheck[name] = struct{}{}
}

var badName string
NameCheck:
for name := range namesToCheck {
for _, perm := range ca.PermittedDNSDomains {
switch {
case perm == name:
break NameCheck
case strings.HasSuffix(name, perm):
// https://tools.ietf.org/html/rfc5280#section-4.2.1.10 says
// zero or more labels are valid. The zero value is handled by
// the previous case, so now we need to make sure it's actually
// a label. We can do that by trimming the suffix and ensuring
// that the last character in the remaining prefix is a period.
pre := strings.TrimSuffix(name, perm)
if strings.HasSuffix(pre, ".") {
break NameCheck
}
}
}
badName = name
break
}

if badName == "" {
return nil
}

return fmt.Errorf("name %q disallowed by CA's permitted DNS domains", badName)
}

func convertRespToPKCS8(resp *logical.Response) error {
privRaw, ok := resp.Data["private_key"]
if !ok {
Expand Down

0 comments on commit 9680045

Please sign in to comment.