Fix permitted dns domain handling (#4905)

It should not require a period to indicate subdomains being allowed Fixes #4863
hashicorp · Jul 11, 2018 · 0883dc3 · 0883dc3
1 parent dffd687
commit 0883dc3
Show file tree

Hide file tree

Showing 3 changed files with 21 additions and 14 deletions.
diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go
@@ -1849,7 +1849,7 @@ func TestBackend_Permitted_DNS_Domains(t *testing.T) {
 	_, 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"},
+		"permitted_dns_domains": []string{"foobar.com", "zipzap.com"},
 	})
 	if err != nil {
 		t.Fatal(err)
@@ -1862,6 +1862,7 @@ func TestBackend_Permitted_DNS_Domains(t *testing.T) {
 
 	path := "root/"
 	checkIssue := func(valid bool, args ...interface{}) {
+		t.Helper()
 		argMap := map[string]interface{}{}
 		var currString string
 		for i, arg := range args {
@@ -1930,8 +1931,8 @@ func TestBackend_Permitted_DNS_Domains(t *testing.T) {
 	}
 
 	// Check issuing and signing against root's permitted domains
-	checkIssue(false, "common_name", "zipzap.com")
-	checkIssue(false, "common_name", "host.foobar.com")
+	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")
 
@@ -1945,7 +1946,7 @@ func TestBackend_Permitted_DNS_Domains(t *testing.T) {
 	_, 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"},
+		"permitted_dns_domains": []string{"abc.com", "xyz.com"},
 		"ttl": "5h",
 	})
 	if err == nil {
@@ -1954,7 +1955,7 @@ func TestBackend_Permitted_DNS_Domains(t *testing.T) {
 	_, 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"},
+		"permitted_dns_domains": []string{"abc.com", "xyz.com"},
 		"ttl": "5h",
 	})
 	if err == nil {
@@ -1965,7 +1966,7 @@ func TestBackend_Permitted_DNS_Domains(t *testing.T) {
 	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"},
+		"permitted_dns_domains": []string{"abc.com", "xyz.com"},
 		"ttl": "5h",
 	})
 	if err != nil {
@@ -1980,8 +1981,9 @@ func TestBackend_Permitted_DNS_Domains(t *testing.T) {
 
 	// Check enforcement with the intermediate's set values
 	path = "int/"
-	checkIssue(false, "common_name", "host.abc.com")
-	checkIssue(false, "common_name", "xyz.com")
+	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")
 }

diff --git a/builtin/logical/pki/cert_util.go b/builtin/logical/pki/cert_util.go
@@ -1482,12 +1482,18 @@ NameCheck:
 	for name := range namesToCheck {
 		for _, perm := range ca.PermittedDNSDomains {
 			switch {
-			case strings.HasPrefix(perm, ".") && strings.HasSuffix(name, perm):
-				// .example.com matches my.host.example.com and
-				// host.example.com but does not match example.com
-				break NameCheck
 			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

diff --git a/website/source/api/secret/pki/index.html.md b/website/source/api/secret/pki/index.html.md
@@ -1076,8 +1076,7 @@ existing cert/key with new values.
 
 - `permitted_dns_domains` `(string: "")` – A comma separated string (or, string
   array) containing DNS domains for which certificates are allowed to be issued
-  or signed by this CA certificate. Supports subdomains via a `.` in front of
-  the domain, as per
+  or signed by this CA certificate. Note that subdomains are allowed, as per
   [RFC](https://tools.ietf.org/html/rfc5280#section-4.2.1.10).
 
 - `ou` `(string: "")` – Specifies the OU (OrganizationalUnit) values in the