From e1d7b798fe58fc9af19b2bc3154528930f432860 Mon Sep 17 00:00:00 2001 From: Alexander Scheel Date: Thu, 24 Feb 2022 10:56:49 -0600 Subject: [PATCH] Backport 1.9.x - Add role parameter to restrict issuance of wildcard certificates + Clarify documentation around certificate issuance (#14251) * Add role parameter to restrict issuance of wildcard certificates (#14238) * Add new AllowWildcardCertificate field to PKI role This field allows the PKI role to control whether or not issuance of wildcard certificates are allowed. We default (both on migration and new role creation) to the less secure true value for backwards compatibility with existing Vault versions. Signed-off-by: Alexander Scheel * Refactor sanitizedName to reducedName Per comment, this variable name was confusing during the reproduction and subsequent fix of the earlier vulnerability and associated bug report. Because the common name isn't necessarily _sanitized_ in any way (and indeed must be considered in relation to other parts or the whole), but portions of the entire name are removed, reducedName appears to make the most sense. Signed-off-by: Alexander Scheel * Enforce AllowWildcardCertificates during issuance This commit adds the bulk of correctly validating wildcard certificate Common Names during issuance according to RFC 6125 Section 6.4.3 semantics. As part of this, support for RFC 2818-conforming wildcard certificates (wherein there are almost no restrictions on issuance) has been removed. Note that this flag does take precedence over AllowAnyName, giving a little more safety in wildcard issuance in this case. Signed-off-by: Alexander Scheel * Update test cases to conform with RFC 6125 Test cases 19, 70+71, and 83+84 didn't conform with the RFC 6125, and so should've been rejected under strict conformance. For 70+71 and 83+84, we previously conditioned around the value of AllowSubdomains (allowing issuance when true), but they likely should've been rejected either way. Additionally, update the notes about globs matching wildcard certificates to notate this is indeed the case. Signed-off-by: Alexander Scheel * Check AllowWildcardCertifciates in issuance tests This allows for regression tests to cover the new AllowWildcardCertificate conditional. We add additional test cases ensuring that wildcard issuance is properly forbidden in all relevant scenarios, while allowing the existing test cases to validate that wildcard status doesn't affect non-wildcard certificates. Signed-off-by: Alexander Scheel * Add Wildcard allowance during signing operations When using sign-verbatim, sign-intermediate, or getting certificate generation parameters, set AllowWildcardCertificates to mirror existing policies. Signed-off-by: Alexander Scheel * Add changelog entry Signed-off-by: Alexander Scheel * Clarify documentation around certificate issuance (#14236) We note that: - allow_bare_domains, allow_glob_domains, and allow_subdomains are all independent, - enforce_hostnames and allow_wildcard_certificates take precedence over allow_any_name, - We limit to RFC 6125 wildcards. - Clarify that both allow_bare_domains and allow_glob_domains will permit wildcard issuance in certain scenarios. Co-authored-by: mickael-hc <86245626+mickael-hc@users.noreply.github.com> Co-authored-by: Kit Haines Signed-off-by: Alexander Scheel Co-authored-by: mickael-hc <86245626+mickael-hc@users.noreply.github.com> Co-authored-by: Kit Haines Co-authored-by: mickael-hc <86245626+mickael-hc@users.noreply.github.com> Co-authored-by: Kit Haines --- builtin/logical/pki/backend_test.go | 281 +++++++++++++----------- builtin/logical/pki/ca_util.go | 38 ++-- builtin/logical/pki/cert_util.go | 164 +++++++++++--- builtin/logical/pki/path_issue_sign.go | 28 +-- builtin/logical/pki/path_roles.go | 29 +++ builtin/logical/pki/path_root.go | 36 +-- changelog/14238.txt | 3 + website/content/api-docs/secret/pki.mdx | 59 ++++- 8 files changed, 419 insertions(+), 219 deletions(-) create mode 100644 changelog/14238.txt diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index 550b1dbf5025..9b269f8c3071 100644 --- a/builtin/logical/pki/backend_test.go +++ b/builtin/logical/pki/backend_test.go @@ -692,12 +692,15 @@ func generateCSRSteps(t *testing.T, caCert, caKey string, intdata, reqdata map[s // Generates steps to test out various role permutations func generateRoleSteps(t *testing.T, useCSRs bool) []logicaltest.TestStep { roleVals := roleEntry{ - MaxTTL: 12 * time.Hour, - KeyType: "rsa", - KeyBits: 2048, - RequireCN: true, - SignatureBits: 256, + MaxTTL: 12 * time.Hour, + KeyType: "rsa", + KeyBits: 2048, + RequireCN: true, + SignatureBits: 256, + AllowWildcardCertificates: new(bool), } + *roleVals.AllowWildcardCertificates = true + issueVals := certutil.IssueData{} ret := []logicaltest.TestStep{} @@ -3294,13 +3297,14 @@ func (o MultiBool) ToValues() []bool { } type IssuanceRegression struct { - AllowedDomains []string - AllowBareDomains MultiBool - AllowGlobDomains MultiBool - AllowSubdomains MultiBool - AllowLocalhost MultiBool - CommonName string - Issued bool + AllowedDomains []string + AllowBareDomains MultiBool + AllowGlobDomains MultiBool + AllowSubdomains MultiBool + AllowLocalhost MultiBool + AllowWildcardCertificates MultiBool + CommonName string + Issued bool } func RoleIssuanceRegressionHelper(t *testing.T, client *api.Client, index int, test IssuanceRegression) int { @@ -3309,36 +3313,38 @@ func RoleIssuanceRegressionHelper(t *testing.T, client *api.Client, index int, t for _, AllowGlobDomains := range test.AllowGlobDomains.ToValues() { for _, AllowSubdomains := range test.AllowSubdomains.ToValues() { for _, AllowLocalhost := range test.AllowLocalhost.ToValues() { + for _, AllowWildcardCertificates := range test.AllowWildcardCertificates.ToValues() { + role := fmt.Sprintf("issuance-regression-%d-bare-%v-glob-%v-subdomains-%v-localhost-%v-wildcard-%v", index, AllowBareDomains, AllowGlobDomains, AllowSubdomains, AllowLocalhost, AllowWildcardCertificates) + resp, err := client.Logical().Write("pki/roles/"+role, map[string]interface{}{ + "allowed_domains": test.AllowedDomains, + "allow_bare_domains": AllowBareDomains, + "allow_glob_domains": AllowGlobDomains, + "allow_subdomains": AllowSubdomains, + "allow_localhost": AllowLocalhost, + "allow_wildcard_certificates": AllowWildcardCertificates, + // TODO: test across this vector as well. Currently certain wildcard + // matching is broken with it enabled (such as x*x.foo). + "enforce_hostnames": false, + "key_type": "ec", + "key_bits": 256, + }) + if err != nil { + t.Fatal(err) + } - role := fmt.Sprintf("issuance-regression-%d-bare-%v-glob-%v-subdomains-%v-localhost-%v", index, AllowBareDomains, AllowGlobDomains, AllowSubdomains, AllowLocalhost) - resp, err := client.Logical().Write("pki/roles/"+role, map[string]interface{}{ - "allowed_domains": test.AllowedDomains, - "allow_bare_domains": AllowBareDomains, - "allow_glob_domains": AllowGlobDomains, - "allow_subdomains": AllowSubdomains, - "allow_localhost": AllowLocalhost, - // TODO: test across this vector as well. Currently certain wildcard - // matching is broken with it enabled (such as x*x.foo). - "enforce_hostnames": false, - "key_type": "ec", - "key_bits": 256, - }) - if err != nil { - t.Fatal(err) - } + resp, err = client.Logical().Write("pki/issue/"+role, map[string]interface{}{ + "common_name": test.CommonName, + }) - resp, err = client.Logical().Write("pki/issue/"+role, map[string]interface{}{ - "common_name": test.CommonName, - }) + haveErr := err != nil || resp == nil + expectErr := !test.Issued - haveErr := err != nil || resp == nil - expectErr := !test.Issued + if haveErr != expectErr { + t.Fatalf("issuance regression test [%d] failed: haveErr: %v, expectErr: %v, err: %v, resp: %v, test case: %v, role: %v", index, haveErr, expectErr, err, resp, test, role) + } - if haveErr != expectErr { - t.Fatalf("issuance regression test [%d] failed: haveErr: %v, expectErr: %v, err: %v, resp: %v, test case: %v, role: %v", index, haveErr, expectErr, err, resp, test, role) + tested += 1 } - - tested += 1 } } } @@ -3350,144 +3356,155 @@ func RoleIssuanceRegressionHelper(t *testing.T, client *api.Client, index int, t func TestBackend_Roles_IssuanceRegression(t *testing.T) { // Regression testing of role's issuance policy. testCases := []IssuanceRegression{ - // allowed, bare, glob, subdomains, localhost, cn, issued + // allowed, bare, glob, subdomains, localhost, wildcards, cn, issued + // === Globs not allowed but used === // // Allowed contains globs, but globbing not allowed, resulting in all // issuances failing. Note that tests against issuing a wildcard with // a bare domain will be covered later. - /* 0 */ {[]string{"*.*.foo"}, MAny, MFalse, MAny, MAny, "baz.fud.bar.foo", false}, - /* 1 */ {[]string{"*.*.foo"}, MAny, MFalse, MAny, MAny, "*.fud.bar.foo", false}, - /* 2 */ {[]string{"*.*.foo"}, MAny, MFalse, MAny, MAny, "fud.bar.foo", false}, - /* 3 */ {[]string{"*.*.foo"}, MAny, MFalse, MAny, MAny, "*.bar.foo", false}, - /* 4 */ {[]string{"*.*.foo"}, MAny, MFalse, MAny, MAny, "bar.foo", false}, - /* 5 */ {[]string{"*.*.foo"}, MAny, MFalse, MAny, MAny, "*.foo", false}, - /* 6 */ {[]string{"*.foo"}, MAny, MFalse, MAny, MAny, "foo", false}, - /* 7 */ {[]string{"*.foo"}, MAny, MFalse, MAny, MAny, "baz.fud.bar.foo", false}, - /* 8 */ {[]string{"*.foo"}, MAny, MFalse, MAny, MAny, "*.fud.bar.foo", false}, - /* 9 */ {[]string{"*.foo"}, MAny, MFalse, MAny, MAny, "fud.bar.foo", false}, - /* 10 */ {[]string{"*.foo"}, MAny, MFalse, MAny, MAny, "*.bar.foo", false}, - /* 11 */ {[]string{"*.foo"}, MAny, MFalse, MAny, MAny, "bar.foo", false}, - /* 12 */ {[]string{"*.foo"}, MAny, MFalse, MAny, MAny, "foo", false}, - + /* 0 */ {[]string{"*.*.foo"}, MAny, MFalse, MAny, MAny, MAny, "baz.fud.bar.foo", false}, + /* 1 */ {[]string{"*.*.foo"}, MAny, MFalse, MAny, MAny, MAny, "*.fud.bar.foo", false}, + /* 2 */ {[]string{"*.*.foo"}, MAny, MFalse, MAny, MAny, MAny, "fud.bar.foo", false}, + /* 3 */ {[]string{"*.*.foo"}, MAny, MFalse, MAny, MAny, MAny, "*.bar.foo", false}, + /* 4 */ {[]string{"*.*.foo"}, MAny, MFalse, MAny, MAny, MAny, "bar.foo", false}, + /* 5 */ {[]string{"*.*.foo"}, MAny, MFalse, MAny, MAny, MAny, "*.foo", false}, + /* 6 */ {[]string{"*.foo"}, MAny, MFalse, MAny, MAny, MAny, "foo", false}, + /* 7 */ {[]string{"*.foo"}, MAny, MFalse, MAny, MAny, MAny, "baz.fud.bar.foo", false}, + /* 8 */ {[]string{"*.foo"}, MAny, MFalse, MAny, MAny, MAny, "*.fud.bar.foo", false}, + /* 9 */ {[]string{"*.foo"}, MAny, MFalse, MAny, MAny, MAny, "fud.bar.foo", false}, + /* 10 */ {[]string{"*.foo"}, MAny, MFalse, MAny, MAny, MAny, "*.bar.foo", false}, + /* 11 */ {[]string{"*.foo"}, MAny, MFalse, MAny, MAny, MAny, "bar.foo", false}, + /* 12 */ {[]string{"*.foo"}, MAny, MFalse, MAny, MAny, MAny, "foo", false}, + + // === Localhost sanity === // // Localhost forbidden, not matching allowed domains -> not issued - /* 13 */ {[]string{"*.*.foo"}, MAny, MAny, MAny, MFalse, "localhost", false}, + /* 13 */ {[]string{"*.*.foo"}, MAny, MAny, MAny, MFalse, MAny, "localhost", false}, // Localhost allowed, not matching allowed domains -> issued - /* 14 */ {[]string{"*.*.foo"}, MAny, MAny, MAny, MTrue, "localhost", true}, + /* 14 */ {[]string{"*.*.foo"}, MAny, MAny, MAny, MTrue, MAny, "localhost", true}, // Localhost allowed via allowed domains (and bare allowed), not by AllowLocalhost -> issued - /* 15 */ {[]string{"localhost"}, MTrue, MAny, MAny, MFalse, "localhost", true}, + /* 15 */ {[]string{"localhost"}, MTrue, MAny, MAny, MFalse, MAny, "localhost", true}, // Localhost allowed via allowed domains (and bare not allowed), not by AllowLocalhost -> not issued - /* 16 */ {[]string{"localhost"}, MFalse, MAny, MAny, MFalse, "localhost", false}, - // Localhost allowed via allowed domains, and by AllowLocalhost -> issued - /* 17 */ {[]string{"localhost"}, MAny, MAny, MAny, MTrue, "localhost", true}, + /* 16 */ {[]string{"localhost"}, MFalse, MAny, MAny, MFalse, MAny, "localhost", false}, + // Localhost allowed via allowed domains (but bare not allowed), and by AllowLocalhost -> issued + /* 17 */ {[]string{"localhost"}, MFalse, MAny, MAny, MTrue, MAny, "localhost", true}, + // === Bare wildcard issuance == // // allowed_domains contains one or more wildcards and bare domains allowed, // resulting in the cert being issued. - /* 18 */ {[]string{"*.foo"}, MTrue, MAny, MAny, MAny, "*.foo", true}, - /* 19 */ {[]string{"*.*.foo"}, MTrue, MAny, MAny, MAny, "*.*.foo", true}, + /* 18 */ {[]string{"*.foo"}, MTrue, MAny, MAny, MAny, MTrue, "*.foo", true}, + /* 19 */ {[]string{"*.*.foo"}, MTrue, MAny, MAny, MAny, MAny, "*.*.foo", false}, // Does not conform to RFC 6125 // === Double Leading Glob Testing === // - // Allowed contains globs, but glob allowed so certain matches work. // The value of bare and localhost does not impact these results. - /* 20 */ {[]string{"*.*.foo"}, MAny, MTrue, MFalse, MAny, "baz.fud.bar.foo", true}, // glob domains allow infinite subdomains - /* 21 */ {[]string{"*.*.foo"}, MAny, MTrue, MFalse, MAny, "*.fud.bar.foo", true}, // ???? does glob domain allow wildcard of subdomains? - /* 22 */ {[]string{"*.*.foo"}, MAny, MTrue, MFalse, MAny, "fud.bar.foo", true}, - /* 23 */ {[]string{"*.*.foo"}, MAny, MTrue, MFalse, MAny, "*.bar.foo", true}, // Regression fix: Vault#13530 - /* 24 */ {[]string{"*.*.foo"}, MAny, MTrue, MFalse, MAny, "bar.foo", false}, - /* 25 */ {[]string{"*.*.foo"}, MAny, MTrue, MFalse, MAny, "*.foo", false}, - /* 26 */ {[]string{"*.*.foo"}, MAny, MTrue, MFalse, MAny, "foo", false}, + /* 20 */ {[]string{"*.*.foo"}, MAny, MTrue, MFalse, MAny, MAny, "baz.fud.bar.foo", true}, // glob domains allow infinite subdomains + /* 21 */ {[]string{"*.*.foo"}, MAny, MTrue, MFalse, MAny, MTrue, "*.fud.bar.foo", true}, // glob domain allows wildcard of subdomains + /* 22 */ {[]string{"*.*.foo"}, MAny, MTrue, MFalse, MAny, MAny, "fud.bar.foo", true}, + /* 23 */ {[]string{"*.*.foo"}, MAny, MTrue, MFalse, MAny, MTrue, "*.bar.foo", true}, // Regression fix: Vault#13530 + /* 24 */ {[]string{"*.*.foo"}, MAny, MTrue, MFalse, MAny, MAny, "bar.foo", false}, + /* 25 */ {[]string{"*.*.foo"}, MAny, MTrue, MFalse, MAny, MAny, "*.foo", false}, + /* 26 */ {[]string{"*.*.foo"}, MAny, MTrue, MFalse, MAny, MAny, "foo", false}, // Allowed contains globs, but glob and subdomain both work, so we expect // wildcard issuance to work as well. The value of bare and localhost does // not impact these results. - /* 27 */ {[]string{"*.*.foo"}, MAny, MTrue, MTrue, MAny, "baz.fud.bar.foo", true}, - /* 28 */ {[]string{"*.*.foo"}, MAny, MTrue, MTrue, MAny, "*.fud.bar.foo", true}, - /* 29 */ {[]string{"*.*.foo"}, MAny, MTrue, MTrue, MAny, "fud.bar.foo", true}, - /* 30 */ {[]string{"*.*.foo"}, MAny, MTrue, MTrue, MAny, "*.bar.foo", true}, // Regression fix: Vault#13530 - /* 31 */ {[]string{"*.*.foo"}, MAny, MTrue, MTrue, MAny, "bar.foo", false}, - /* 32 */ {[]string{"*.*.foo"}, MAny, MTrue, MTrue, MAny, "*.foo", false}, - /* 33 */ {[]string{"*.*.foo"}, MAny, MTrue, MTrue, MAny, "foo", false}, + /* 27 */ {[]string{"*.*.foo"}, MAny, MTrue, MTrue, MAny, MAny, "baz.fud.bar.foo", true}, + /* 28 */ {[]string{"*.*.foo"}, MAny, MTrue, MTrue, MAny, MTrue, "*.fud.bar.foo", true}, + /* 29 */ {[]string{"*.*.foo"}, MAny, MTrue, MTrue, MAny, MAny, "fud.bar.foo", true}, + /* 30 */ {[]string{"*.*.foo"}, MAny, MTrue, MTrue, MAny, MTrue, "*.bar.foo", true}, // Regression fix: Vault#13530 + /* 31 */ {[]string{"*.*.foo"}, MAny, MTrue, MTrue, MAny, MAny, "bar.foo", false}, + /* 32 */ {[]string{"*.*.foo"}, MAny, MTrue, MTrue, MAny, MAny, "*.foo", false}, + /* 33 */ {[]string{"*.*.foo"}, MAny, MTrue, MTrue, MAny, MAny, "foo", false}, // === Single Leading Glob Testing === // - // Allowed contains globs, but glob allowed so certain matches work. // The value of bare and localhost does not impact these results. - /* 34 */ {[]string{"*.foo"}, MAny, MTrue, MFalse, MAny, "baz.fud.bar.foo", true}, // glob domains allow infinite subdomains - /* 35 */ {[]string{"*.foo"}, MAny, MTrue, MFalse, MAny, "*.fud.bar.foo", true}, // ???? does glob domain allow wildcard of subdomains? - /* 36 */ {[]string{"*.foo"}, MAny, MTrue, MFalse, MAny, "fud.bar.foo", true}, // glob domains allow infinite subdomains - /* 37 */ {[]string{"*.foo"}, MAny, MTrue, MFalse, MAny, "*.bar.foo", true}, // ???? does glob domain allow wildcards of subdomains? - /* 38 */ {[]string{"*.foo"}, MAny, MTrue, MFalse, MAny, "bar.foo", true}, - /* 39 */ {[]string{"*.foo"}, MAny, MTrue, MFalse, MAny, "foo", false}, + /* 34 */ {[]string{"*.foo"}, MAny, MTrue, MFalse, MAny, MAny, "baz.fud.bar.foo", true}, // glob domains allow infinite subdomains + /* 35 */ {[]string{"*.foo"}, MAny, MTrue, MFalse, MAny, MTrue, "*.fud.bar.foo", true}, // glob domain allows wildcard of subdomains + /* 36 */ {[]string{"*.foo"}, MAny, MTrue, MFalse, MAny, MAny, "fud.bar.foo", true}, // glob domains allow infinite subdomains + /* 37 */ {[]string{"*.foo"}, MAny, MTrue, MFalse, MAny, MTrue, "*.bar.foo", true}, // glob domain allows wildcards of subdomains + /* 38 */ {[]string{"*.foo"}, MAny, MTrue, MFalse, MAny, MAny, "bar.foo", true}, + /* 39 */ {[]string{"*.foo"}, MAny, MTrue, MFalse, MAny, MAny, "foo", false}, // Allowed contains globs, but glob and subdomain both work, so we expect // wildcard issuance to work as well. The value of bare and localhost does // not impact these results. - /* 40 */ {[]string{"*.foo"}, MAny, MTrue, MTrue, MAny, "baz.fud.bar.foo", true}, - /* 41 */ {[]string{"*.foo"}, MAny, MTrue, MTrue, MAny, "*.fud.bar.foo", true}, - /* 42 */ {[]string{"*.foo"}, MAny, MTrue, MTrue, MAny, "fud.bar.foo", true}, - /* 43 */ {[]string{"*.foo"}, MAny, MTrue, MTrue, MAny, "*.bar.foo", true}, - /* 44 */ {[]string{"*.foo"}, MAny, MTrue, MTrue, MAny, "bar.foo", true}, - /* 45 */ {[]string{"*.foo"}, MAny, MTrue, MTrue, MAny, "foo", false}, + /* 40 */ {[]string{"*.foo"}, MAny, MTrue, MTrue, MAny, MAny, "baz.fud.bar.foo", true}, + /* 41 */ {[]string{"*.foo"}, MAny, MTrue, MTrue, MAny, MTrue, "*.fud.bar.foo", true}, + /* 42 */ {[]string{"*.foo"}, MAny, MTrue, MTrue, MAny, MAny, "fud.bar.foo", true}, + /* 43 */ {[]string{"*.foo"}, MAny, MTrue, MTrue, MAny, MTrue, "*.bar.foo", true}, + /* 44 */ {[]string{"*.foo"}, MAny, MTrue, MTrue, MAny, MAny, "bar.foo", true}, + /* 45 */ {[]string{"*.foo"}, MAny, MTrue, MTrue, MAny, MAny, "foo", false}, // === Only base domain name === // - // Allowed contains only domain components, but subdomains not allowed. This // results in most issuances failing unless we allow bare domains, in which // case only the final issuance for "foo" will succeed. - /* 46 */ {[]string{"foo"}, MAny, MAny, MFalse, MAny, "baz.fud.bar.foo", false}, - /* 47 */ {[]string{"foo"}, MAny, MAny, MFalse, MAny, "*.fud.bar.foo", false}, - /* 48 */ {[]string{"foo"}, MAny, MAny, MFalse, MAny, "fud.bar.foo", false}, - /* 49 */ {[]string{"foo"}, MAny, MAny, MFalse, MAny, "*.bar.foo", false}, - /* 50 */ {[]string{"foo"}, MAny, MAny, MFalse, MAny, "bar.foo", false}, - /* 51 */ {[]string{"foo"}, MAny, MAny, MFalse, MAny, "*.foo", false}, - /* 52 */ {[]string{"foo"}, MFalse, MAny, MFalse, MAny, "foo", false}, - /* 53 */ {[]string{"foo"}, MTrue, MAny, MFalse, MAny, "foo", true}, + /* 46 */ {[]string{"foo"}, MAny, MAny, MFalse, MAny, MAny, "baz.fud.bar.foo", false}, + /* 47 */ {[]string{"foo"}, MAny, MAny, MFalse, MAny, MAny, "*.fud.bar.foo", false}, + /* 48 */ {[]string{"foo"}, MAny, MAny, MFalse, MAny, MAny, "fud.bar.foo", false}, + /* 49 */ {[]string{"foo"}, MAny, MAny, MFalse, MAny, MAny, "*.bar.foo", false}, + /* 50 */ {[]string{"foo"}, MAny, MAny, MFalse, MAny, MAny, "bar.foo", false}, + /* 51 */ {[]string{"foo"}, MAny, MAny, MFalse, MAny, MAny, "*.foo", false}, + /* 52 */ {[]string{"foo"}, MFalse, MAny, MFalse, MAny, MAny, "foo", false}, + /* 53 */ {[]string{"foo"}, MTrue, MAny, MFalse, MAny, MAny, "foo", true}, // Allowed contains only domain components, and subdomains are now allowed. // This results in most issuances succeeding, with the exception of the // base foo, which is still governed by base's value. - /* 54 */ {[]string{"foo"}, MAny, MAny, MTrue, MAny, "baz.fud.bar.foo", true}, - /* 55 */ {[]string{"foo"}, MAny, MAny, MTrue, MAny, "*.fud.bar.foo", true}, - /* 56 */ {[]string{"foo"}, MAny, MAny, MTrue, MAny, "fud.bar.foo", true}, - /* 57 */ {[]string{"foo"}, MAny, MAny, MTrue, MAny, "*.bar.foo", true}, - /* 58 */ {[]string{"foo"}, MAny, MAny, MTrue, MAny, "bar.foo", true}, - /* 59 */ {[]string{"foo"}, MAny, MAny, MTrue, MAny, "*.foo", true}, - /* 60 */ {[]string{"foo"}, MAny, MAny, MTrue, MAny, "x*x.foo", true}, // internal wildcards should be allowed per RFC 6125/6.4.3 - /* 61 */ {[]string{"foo"}, MAny, MAny, MTrue, MAny, "*x.foo", true}, // prefix wildcards should be allowed per RFC 6125/6.4.3 - /* 62 */ {[]string{"foo"}, MAny, MAny, MTrue, MAny, "x*.foo", true}, // suffix wildcards should be allowed per RFC 6125/6.4.3 - /* 63 */ {[]string{"foo"}, MFalse, MAny, MTrue, MAny, "foo", false}, - /* 64 */ {[]string{"foo"}, MTrue, MAny, MTrue, MAny, "foo", true}, + /* 54 */ {[]string{"foo"}, MAny, MAny, MTrue, MAny, MAny, "baz.fud.bar.foo", true}, + /* 55 */ {[]string{"foo"}, MAny, MAny, MTrue, MAny, MTrue, "*.fud.bar.foo", true}, + /* 56 */ {[]string{"foo"}, MAny, MAny, MTrue, MAny, MAny, "fud.bar.foo", true}, + /* 57 */ {[]string{"foo"}, MAny, MAny, MTrue, MAny, MTrue, "*.bar.foo", true}, + /* 58 */ {[]string{"foo"}, MAny, MAny, MTrue, MAny, MAny, "bar.foo", true}, + /* 59 */ {[]string{"foo"}, MAny, MAny, MTrue, MAny, MTrue, "*.foo", true}, + /* 60 */ {[]string{"foo"}, MAny, MAny, MTrue, MAny, MTrue, "x*x.foo", true}, // internal wildcards should be allowed per RFC 6125/6.4.3 + /* 61 */ {[]string{"foo"}, MAny, MAny, MTrue, MAny, MTrue, "*x.foo", true}, // prefix wildcards should be allowed per RFC 6125/6.4.3 + /* 62 */ {[]string{"foo"}, MAny, MAny, MTrue, MAny, MTrue, "x*.foo", true}, // suffix wildcards should be allowed per RFC 6125/6.4.3 + /* 63 */ {[]string{"foo"}, MFalse, MAny, MTrue, MAny, MAny, "foo", false}, + /* 64 */ {[]string{"foo"}, MTrue, MAny, MTrue, MAny, MAny, "foo", true}, // === Internal Glob Matching === // // Basic glob matching requirements - /* 65 */ {[]string{"x*x.foo"}, MAny, MTrue, MAny, MAny, "xerox.foo", true}, - /* 66 */ {[]string{"x*x.foo"}, MAny, MTrue, MAny, MAny, "xylophone.files.pyrex.foo", true}, // globs can match across subdomains - /* 67 */ {[]string{"x*x.foo"}, MAny, MTrue, MAny, MAny, "xercex.bar.foo", false}, // x.foo isn't matched - /* 68 */ {[]string{"x*x.foo"}, MAny, MTrue, MAny, MAny, "bar.foo", false}, // x*x isn't matched. - /* 69 */ {[]string{"x*x.foo"}, MAny, MTrue, MAny, MAny, "*.foo", false}, // unrelated wildcard - /* 70 */ {[]string{"x*x.foo"}, MAny, MTrue, MFalse, MAny, "*.x*x.foo", false}, // ???? double wildcard doesn't match glob without subdomains enabled - /* 71 */ {[]string{"x*x.foo"}, MAny, MTrue, MTrue, MAny, "*.x*x.foo", true}, // ???? as above, but with subdomains enabled - /* 72 */ {[]string{"x*x.foo"}, MAny, MTrue, MAny, MAny, "*.xyx.foo", false}, // ???? single wildcard matching glob fails (even with subdomains=true) + /* 65 */ {[]string{"x*x.foo"}, MAny, MTrue, MAny, MAny, MAny, "xerox.foo", true}, + /* 66 */ {[]string{"x*x.foo"}, MAny, MTrue, MAny, MAny, MAny, "xylophone.files.pyrex.foo", true}, // globs can match across subdomains + /* 67 */ {[]string{"x*x.foo"}, MAny, MTrue, MAny, MAny, MAny, "xercex.bar.foo", false}, // x.foo isn't matched + /* 68 */ {[]string{"x*x.foo"}, MAny, MTrue, MAny, MAny, MAny, "bar.foo", false}, // x*x isn't matched. + /* 69 */ {[]string{"x*x.foo"}, MAny, MTrue, MAny, MAny, MAny, "*.foo", false}, // unrelated wildcard + /* 70 */ {[]string{"x*x.foo"}, MAny, MTrue, MAny, MAny, MAny, "*.x*x.foo", false}, // Does not conform to RFC 6125 + /* 71 */ {[]string{"x*x.foo"}, MAny, MTrue, MAny, MAny, MAny, "*.xyx.foo", false}, // Globs and Subdomains do not layer per docs. // Various requirements around x*x.foo wildcard matching. - /* 73 */ {[]string{"x*x.foo"}, MFalse, MFalse, MAny, MAny, "x*x.foo", false}, // base disabled, shouldn't match wildcard - /* 74 */ {[]string{"x*x.foo"}, MFalse, MTrue, MAny, MAny, "x*x.foo", true}, // base disallowed, but globbing allowed and should match - /* 75 */ {[]string{"x*x.foo"}, MTrue, MAny, MAny, MAny, "x*x.foo", true}, // base allowed, should match wildcard + /* 72 */ {[]string{"x*x.foo"}, MFalse, MFalse, MAny, MAny, MAny, "x*x.foo", false}, // base disabled, shouldn't match wildcard + /* 73 */ {[]string{"x*x.foo"}, MFalse, MTrue, MAny, MAny, MTrue, "x*x.foo", true}, // base disallowed, but globbing allowed and should match + /* 74 */ {[]string{"x*x.foo"}, MTrue, MAny, MAny, MAny, MTrue, "x*x.foo", true}, // base allowed, should match wildcard // Basic glob matching requirements with internal dots. - /* 76 */ {[]string{"x.*.x.foo"}, MAny, MTrue, MAny, MAny, "xerox.foo", false}, // missing dots - /* 77 */ {[]string{"x.*.x.foo"}, MAny, MTrue, MAny, MAny, "x.ero.x.foo", true}, - /* 78 */ {[]string{"x.*.x.foo"}, MAny, MTrue, MAny, MAny, "xylophone.files.pyrex.foo", false}, // missing dots - /* 79 */ {[]string{"x.*.x.foo"}, MAny, MTrue, MAny, MAny, "x.ylophone.files.pyre.x.foo", true}, // globs can match across subdomains - /* 80 */ {[]string{"x.*.x.foo"}, MAny, MTrue, MAny, MAny, "xercex.bar.foo", false}, // x.foo isn't matched - /* 81 */ {[]string{"x.*.x.foo"}, MAny, MTrue, MAny, MAny, "bar.foo", false}, // x.*.x isn't matched. - /* 82 */ {[]string{"x.*.x.foo"}, MAny, MTrue, MAny, MAny, "*.foo", false}, // unrelated wildcard - /* 83 */ {[]string{"x.*.x.foo"}, MAny, MTrue, MFalse, MAny, "*.x.*.x.foo", false}, // ???? double wildcard doesn't match glob without subdomains enabled - /* 84 */ {[]string{"x.*.x.foo"}, MAny, MTrue, MTrue, MAny, "*.x.*.x.foo", true}, // ???? as above, but with subdomains enabled - /* 85 */ {[]string{"x.*.x.foo"}, MAny, MTrue, MAny, MAny, "*.x.y.x.foo", false}, // ???? single wildcard with internal glob match fails (even with subdomains=true) - } - - if len(testCases) != 86 { + /* 75 */ {[]string{"x.*.x.foo"}, MAny, MTrue, MAny, MAny, MAny, "xerox.foo", false}, // missing dots + /* 76 */ {[]string{"x.*.x.foo"}, MAny, MTrue, MAny, MAny, MAny, "x.ero.x.foo", true}, + /* 77 */ {[]string{"x.*.x.foo"}, MAny, MTrue, MAny, MAny, MAny, "xylophone.files.pyrex.foo", false}, // missing dots + /* 78 */ {[]string{"x.*.x.foo"}, MAny, MTrue, MAny, MAny, MAny, "x.ylophone.files.pyre.x.foo", true}, // globs can match across subdomains + /* 79 */ {[]string{"x.*.x.foo"}, MAny, MTrue, MAny, MAny, MAny, "xercex.bar.foo", false}, // x.foo isn't matched + /* 80 */ {[]string{"x.*.x.foo"}, MAny, MTrue, MAny, MAny, MAny, "bar.foo", false}, // x.*.x isn't matched. + /* 81 */ {[]string{"x.*.x.foo"}, MAny, MTrue, MAny, MAny, MAny, "*.foo", false}, // unrelated wildcard + /* 82 */ {[]string{"x.*.x.foo"}, MAny, MTrue, MAny, MAny, MAny, "*.x.*.x.foo", false}, // Does not conform to RFC 6125 + /* 83 */ {[]string{"x.*.x.foo"}, MAny, MTrue, MAny, MAny, MAny, "*.x.y.x.foo", false}, // Globs and Subdomains do not layer per docs. + + // === Wildcard restriction testing === // + /* 84 */ {[]string{"*.foo"}, MAny, MTrue, MFalse, MAny, MFalse, "*.fud.bar.foo", false}, // glob domain allows wildcard of subdomains + /* 85 */ {[]string{"*.foo"}, MAny, MTrue, MFalse, MAny, MFalse, "*.bar.foo", false}, // glob domain allows wildcards of subdomains + /* 86 */ {[]string{"foo"}, MAny, MAny, MTrue, MAny, MFalse, "*.fud.bar.foo", false}, + /* 87 */ {[]string{"foo"}, MAny, MAny, MTrue, MAny, MFalse, "*.bar.foo", false}, + /* 88 */ {[]string{"foo"}, MAny, MAny, MTrue, MAny, MFalse, "*.foo", false}, + /* 89 */ {[]string{"foo"}, MAny, MAny, MTrue, MAny, MFalse, "x*x.foo", false}, + /* 90 */ {[]string{"foo"}, MAny, MAny, MTrue, MAny, MFalse, "*x.foo", false}, + /* 91 */ {[]string{"foo"}, MAny, MAny, MTrue, MAny, MFalse, "x*.foo", false}, + /* 92 */ {[]string{"x*x.foo"}, MTrue, MAny, MAny, MAny, MFalse, "x*x.foo", false}, + /* 93 */ {[]string{"*.foo"}, MFalse, MFalse, MAny, MAny, MAny, "*.foo", false}, // Bare and globs forbidden despite (potentially) allowing wildcards. + /* 94 */ {[]string{"x.*.x.foo"}, MAny, MAny, MAny, MAny, MAny, "x.*.x.foo", false}, // Does not conform to RFC 6125 + } + + if len(testCases) != 95 { t.Fatalf("misnumbered test case entries will make it hard to find bugs: %v", len(testCases)) } diff --git a/builtin/logical/pki/ca_util.go b/builtin/logical/pki/ca_util.go index ea6b083c7eed..0db79c102df3 100644 --- a/builtin/logical/pki/ca_util.go +++ b/builtin/logical/pki/ca_util.go @@ -30,25 +30,27 @@ func (b *backend) getGenerationParams( } role = &roleEntry{ - TTL: time.Duration(data.Get("ttl").(int)) * time.Second, - KeyType: data.Get("key_type").(string), - KeyBits: data.Get("key_bits").(int), - SignatureBits: data.Get("signature_bits").(int), - AllowLocalhost: true, - AllowAnyName: true, - AllowIPSANs: true, - EnforceHostnames: false, - AllowedURISANs: []string{"*"}, - AllowedOtherSANs: []string{"*"}, - AllowedSerialNumbers: []string{"*"}, - OU: data.Get("ou").([]string), - Organization: data.Get("organization").([]string), - Country: data.Get("country").([]string), - Locality: data.Get("locality").([]string), - Province: data.Get("province").([]string), - StreetAddress: data.Get("street_address").([]string), - PostalCode: data.Get("postal_code").([]string), + TTL: time.Duration(data.Get("ttl").(int)) * time.Second, + KeyType: data.Get("key_type").(string), + KeyBits: data.Get("key_bits").(int), + SignatureBits: data.Get("signature_bits").(int), + AllowLocalhost: true, + AllowAnyName: true, + AllowIPSANs: true, + AllowWildcardCertificates: new(bool), + EnforceHostnames: false, + AllowedURISANs: []string{"*"}, + AllowedOtherSANs: []string{"*"}, + AllowedSerialNumbers: []string{"*"}, + OU: data.Get("ou").([]string), + Organization: data.Get("organization").([]string), + Country: data.Get("country").([]string), + Locality: data.Get("locality").([]string), + Province: data.Get("province").([]string), + StreetAddress: data.Get("street_address").([]string), + PostalCode: data.Get("postal_code").([]string), } + *role.AllowWildcardCertificates = true if role.KeyType == "rsa" && role.KeyBits < 2048 { errorResp = logical.ErrorResponse("RSA keys < 2048 bits are unsafe and not supported") diff --git a/builtin/logical/pki/cert_util.go b/builtin/logical/pki/cert_util.go index 205174aab956..bc022009bab6 100644 --- a/builtin/logical/pki/cert_util.go +++ b/builtin/logical/pki/cert_util.go @@ -38,11 +38,32 @@ type inputBundle struct { } var ( + // labelRegex is a single label from a valid domain name and was extracted + // from hostnameRegex below for use in leftWildLabelRegex, without any + // label separators (`.`). + labelRegex = `([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]*[a-zA-Z0-9])` + // A note on hostnameRegex: although we set the StrictDomainName option // when doing the idna conversion, this appears to only affect output, not // input, so it will allow e.g. host^123.example.com straight through. So // we still need to use this to check the output. - hostnameRegex = regexp.MustCompile(`^(\*\.)?(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]*[a-zA-Z0-9])\.)*([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9\-]*[A-Za-z0-9])\.?$`) + hostnameRegex = regexp.MustCompile(`^(\*\.)?(` + labelRegex + `\.)*` + labelRegex + `\.?$`) + + // Left Wildcard Label Regex is equivalent to a single domain label + // component from hostnameRegex above, but with additional wildcard + // characters added. There are four possibilities here: + // + // 1. Entire label is a wildcard, + // 2. Wildcard exists at the start, + // 3. Wildcard exists at the end, + // 4. Wildcard exists in the middle. + allWildRegex = `\*` + startWildRegex = `\*` + labelRegex + endWildRegex = labelRegex + `\*` + middleWildRegex = labelRegex + `\*` + labelRegex + leftWildLabelRegex = regexp.MustCompile(`^(` + allWildRegex + `|` + startWildRegex + `|` + endWildRegex + `|` + middleWildRegex + `)$`) + + // OIDs for X.509 certificate extensions used below. oidExtensionBasicConstraints = []int{2, 5, 29, 19} oidExtensionSubjectAltName = []int{2, 5, 29, 17} ) @@ -181,8 +202,16 @@ func fetchCertBySerial(ctx context.Context, req *logical.Request, prefix, serial // If one does not pass, it is returned in the string argument. func validateNames(b *backend, data *inputBundle, names []string) string { for _, name := range names { - sanitizedName := name - emailDomain := sanitizedName + // Previously, reducedName was called sanitizedName but this made + // little sense under the previous interpretation of wildcards, + // leading to two bugs in this implementation. We presently call it + // "reduced" to indicate that it is still untrusted input (potentially + // different from the bare Common Name entry we're validating), it + // might have been modified such as by the removal of wildcard labels + // or the email prefix. + reducedName := name + emailDomain := reducedName + wildcardLabel := "" isEmail := false isWildcard := false @@ -192,48 +221,119 @@ func validateNames(b *backend, data *inputBundle, names []string) string { // ends up being problematic for users, I guess that could be separated // into dns_names and email_names in the future to be explicit, but I // don't think this is likely. - if strings.Contains(sanitizedName, "@") { - splitEmail := strings.Split(sanitizedName, "@") + if strings.Contains(reducedName, "@") { + splitEmail := strings.Split(reducedName, "@") if len(splitEmail) != 2 { return name } - sanitizedName = splitEmail[1] + reducedName = splitEmail[1] emailDomain = splitEmail[1] isEmail = true } - // If we have an asterisk as the first part of the domain name, mark it - // as wildcard and set the sanitized name to the remainder of the - // domain - if strings.HasPrefix(sanitizedName, "*.") { - sanitizedName = sanitizedName[2:] + // Per RFC 6125 Section 6.4.3, and explicitly contradicting the earlier + // RFC 2818 which no modern client will validate against, there are two + // main types of wildcards, each with a single wildcard specifier (`*`, + // functionally different from the `*` used as a glob from the + // AllowGlobDomains parsing path) in the left-most label: + // + // 1. Entire label is a single wildcard character (most common and + // well-supported), + // 2. Part of the label contains a single wildcard character (e.g. per + /// RFC 6125: baz*.example.net, *baz.example.net, or b*z.example.net). + // + // We permit issuance of both but not the older RFC 2818 style under + // the new AllowWildcardCertificates option. However, anything with a + // glob character is technically a wildcard. + if strings.Contains(reducedName, "*") { + // Regardless of later rejections below, this common name contains + // a wildcard character and is thus technically a wildcard name. isWildcard = true + + // Additionally, if AllowWildcardCertificates is explicitly + // forbidden, it takes precedence over AllowAnyName, thus we should + // reject the name now. + // + // We expect the role to have been correctly migrated but guard for + // safety. + if data.role.AllowWildcardCertificates != nil && !*data.role.AllowWildcardCertificates { + return name + } + + if strings.Count(reducedName, "*") > 1 { + // As mentioned above, only one wildcard character is permitted + // under RFC 6125 semantics. + return name + } + + // Split the Common Name into two parts: a left-most label and the + // remaining segments (if present). + splitLabels := strings.SplitN(reducedName, ".", 2) + if len(splitLabels) != 2 { + // We've been given a single-part domain name that consists + // entirely of a wildcard. This is a little tricky to handle, + // but EnforceHostnames validates both the wildcard-containing + // label and the reduced name, but _only_ the latter if it is + // non-empty. This allows us to still validate the only label + // component matches hostname expectations still. + wildcardLabel = splitLabels[0] + reducedName = "" + } else { + // We have a (at least) two label domain name. But before we can + // update our names, we need to validate the wildcard ended up + // in the segment we expected it to. While this is (kinda) + // validated under EnforceHostnames's leftWildLabelRegex, we + // still need to validate it in the non-enforced mode. + // + // By validated assumption above, we know there's strictly one + // wildcard in this domain so we only need to check the wildcard + // label or the reduced name (as one is equivalent to the other). + // Because we later assume reducedName _lacks_ wildcard segments, + // we validate that. + wildcardLabel = splitLabels[0] + reducedName = splitLabels[1] + if strings.Contains(reducedName, "*") { + return name + } + } } // Email addresses using wildcard domain names do not make sense + // in a Common Name field. if isEmail && isWildcard { return name } // AllowAnyName is checked after this because EnforceHostnames still - // applies when allowing any name. Also, we check the sanitized name to + // applies when allowing any name. Also, we check the reduced name to // ensure that we are not either checking a full email address or a // wildcard prefix. if data.role.EnforceHostnames { - p := idna.New( - idna.StrictDomainName(true), - idna.VerifyDNSLength(true), - ) - converted, err := p.ToASCII(sanitizedName) - if err != nil { - return name + if reducedName != "" { + // See note above about splitLabels having only one segment + // and setting reducedName to the empty string. + p := idna.New( + idna.StrictDomainName(true), + idna.VerifyDNSLength(true), + ) + converted, err := p.ToASCII(reducedName) + if err != nil { + return name + } + if !hostnameRegex.MatchString(converted) { + return name + } } - if !hostnameRegex.MatchString(converted) { + + // When a wildcard is specified, we additionally need to validate + // the label with the wildcard is correctly formed. + if isWildcard && !leftWildLabelRegex.MatchString(wildcardLabel) { return name } } - // Self-explanatory + // Self-explanatory, but validations from EnforceHostnames and + // AllowWildcardCertificates take precedence. if data.role.AllowAnyName { continue } @@ -255,8 +355,8 @@ func validateNames(b *backend, data *inputBundle, names []string) string { // Variances are noted in-line if data.role.AllowLocalhost { - if sanitizedName == "localhost" || - sanitizedName == "localdomain" || + if reducedName == "localhost" || + reducedName == "localdomain" || (isEmail && emailDomain == "localhost") || (isEmail && emailDomain == "localdomain") { continue @@ -264,14 +364,14 @@ func validateNames(b *backend, data *inputBundle, names []string) string { if data.role.AllowSubdomains { // It is possible, if unlikely, to have a subdomain of "localhost" - if strings.HasSuffix(sanitizedName, ".localhost") || - (isWildcard && sanitizedName == "localhost") { + if strings.HasSuffix(reducedName, ".localhost") || + (isWildcard && reducedName == "localhost") { continue } // A subdomain of "localdomain" is also not entirely uncommon - if strings.HasSuffix(sanitizedName, ".localdomain") || - (isWildcard && sanitizedName == "localdomain") { + if strings.HasSuffix(reducedName, ".localdomain") || + (isWildcard && reducedName == "localdomain") { continue } } @@ -293,15 +393,15 @@ func validateNames(b *backend, data *inputBundle, names []string) string { // Compare the sanitized name against the hostname // portion of the email address in the broken // display name - if strings.HasSuffix(sanitizedName, "."+splitDisplay[1]) { + if strings.HasSuffix(reducedName, "."+splitDisplay[1]) { continue } } } } - if strings.HasSuffix(sanitizedName, "."+data.req.DisplayName) || - (isWildcard && sanitizedName == data.req.DisplayName) { + if strings.HasSuffix(reducedName, "."+data.req.DisplayName) || + (isWildcard && reducedName == data.req.DisplayName) { continue } } @@ -337,8 +437,8 @@ func validateNames(b *backend, data *inputBundle, names []string) string { } if data.role.AllowSubdomains { - if strings.HasSuffix(sanitizedName, "."+currDomain) || - (isWildcard && strings.EqualFold(sanitizedName, currDomain)) { + if strings.HasSuffix(reducedName, "."+currDomain) || + (isWildcard && strings.EqualFold(reducedName, currDomain)) { valid = true break } diff --git a/builtin/logical/pki/path_issue_sign.go b/builtin/logical/pki/path_issue_sign.go index ce3bcd2cd4b1..5b00bbcabf02 100644 --- a/builtin/logical/pki/path_issue_sign.go +++ b/builtin/logical/pki/path_issue_sign.go @@ -154,20 +154,22 @@ func (b *backend) pathSignVerbatim(ctx context.Context, req *logical.Request, da } entry := &roleEntry{ - AllowLocalhost: true, - AllowAnyName: true, - AllowIPSANs: true, - EnforceHostnames: false, - KeyType: "any", - UseCSRCommonName: true, - UseCSRSANs: true, - AllowedURISANs: []string{"*"}, - AllowedSerialNumbers: []string{"*"}, - GenerateLease: new(bool), - KeyUsage: data.Get("key_usage").([]string), - ExtKeyUsage: data.Get("ext_key_usage").([]string), - ExtKeyUsageOIDs: data.Get("ext_key_usage_oids").([]string), + AllowLocalhost: true, + AllowAnyName: true, + AllowIPSANs: true, + AllowWildcardCertificates: new(bool), + EnforceHostnames: false, + KeyType: "any", + UseCSRCommonName: true, + UseCSRSANs: true, + AllowedURISANs: []string{"*"}, + AllowedSerialNumbers: []string{"*"}, + GenerateLease: new(bool), + KeyUsage: data.Get("key_usage").([]string), + ExtKeyUsage: data.Get("ext_key_usage").([]string), + ExtKeyUsageOIDs: data.Get("ext_key_usage_oids").([]string), } + *entry.AllowWildcardCertificates = true *entry.GenerateLease = false diff --git a/builtin/logical/pki/path_roles.go b/builtin/logical/pki/path_roles.go index 4cfa666cfcc4..9c929563fe9a 100644 --- a/builtin/logical/pki/path_roles.go +++ b/builtin/logical/pki/path_roles.go @@ -107,6 +107,14 @@ can include glob patterns, e.g. "ftp*.example.com". See the documentation for more information.`, }, + "allow_wildcard_certificates": { + Type: framework.TypeBool, + Description: `If set, allows certificates with wildcards in +the common name to be issued, conforming to RFC 6125's Section 6.4.3; e.g., +"*.example.net" or "b*z.example.net". See the documentation for more +information.`, + }, + "allow_any_name": { Type: framework.TypeBool, Description: `If set, clients can request certificates for @@ -458,6 +466,15 @@ func (b *backend) getRole(ctx context.Context, s logical.Storage, n string) (*ro result.AllowedBaseDomain = "" modified = true } + if result.AllowWildcardCertificates == nil { + // While not the most secure default, when AllowWildcardCertificates isn't + // explicitly specified in the stored Role, we automatically upgrade it to + // true to preserve compatibility with previous versions of Vault. Once this + // field is set, this logic will not be triggered any more. + result.AllowWildcardCertificates = new(bool) + *result.AllowWildcardCertificates = true + modified = true + } // Upgrade generate_lease in role if result.GenerateLease == nil { @@ -557,6 +574,7 @@ func (b *backend) pathRoleCreate(ctx context.Context, req *logical.Request, data AllowBareDomains: data.Get("allow_bare_domains").(bool), AllowSubdomains: data.Get("allow_subdomains").(bool), AllowGlobDomains: data.Get("allow_glob_domains").(bool), + AllowWildcardCertificates: new(bool), // Handled specially below AllowAnyName: data.Get("allow_any_name").(bool), EnforceHostnames: data.Get("enforce_hostnames").(bool), AllowIPSANs: data.Get("allow_ip_sans").(bool), @@ -644,6 +662,15 @@ func (b *backend) pathRoleCreate(ctx context.Context, req *logical.Request, data } } + allow_wildcard_certificates, present := data.GetOk("allow_wildcard_certificates") + if !present { + // While not the most secure default, when AllowWildcardCertificates isn't + // explicitly specified in the request, we automatically set it to true to + // preserve compatibility with previous versions of Vault. + allow_wildcard_certificates = true + } + *entry.AllowWildcardCertificates = allow_wildcard_certificates.(bool) + // Store it jsonEntry, err := logical.StorageEntryJSON("role/"+name, entry) if err != nil { @@ -752,6 +779,7 @@ type roleEntry struct { AllowTokenDisplayName bool `json:"allow_token_displayname" mapstructure:"allow_token_displayname"` AllowSubdomains bool `json:"allow_subdomains" mapstructure:"allow_subdomains"` AllowGlobDomains bool `json:"allow_glob_domains" mapstructure:"allow_glob_domains"` + AllowWildcardCertificates *bool `json:"allow_wildcard_certificates,omitempty" mapstructure:"allow_wildcard_certificates"` AllowAnyName bool `json:"allow_any_name" mapstructure:"allow_any_name"` EnforceHostnames bool `json:"enforce_hostnames" mapstructure:"enforce_hostnames"` AllowIPSANs bool `json:"allow_ip_sans" mapstructure:"allow_ip_sans"` @@ -803,6 +831,7 @@ func (r *roleEntry) ToResponseData() map[string]interface{} { "allow_token_displayname": r.AllowTokenDisplayName, "allow_subdomains": r.AllowSubdomains, "allow_glob_domains": r.AllowGlobDomains, + "allow_wildcard_certificates": r.AllowWildcardCertificates, "allow_any_name": r.AllowAnyName, "enforce_hostnames": r.EnforceHostnames, "allow_ip_sans": r.AllowIPSANs, diff --git a/builtin/logical/pki/path_root.go b/builtin/logical/pki/path_root.go index 773444245971..f8213494f814 100644 --- a/builtin/logical/pki/path_root.go +++ b/builtin/logical/pki/path_root.go @@ -267,23 +267,25 @@ func (b *backend) pathCASignIntermediate(ctx context.Context, req *logical.Reque } role := &roleEntry{ - OU: data.Get("ou").([]string), - Organization: data.Get("organization").([]string), - Country: data.Get("country").([]string), - Locality: data.Get("locality").([]string), - Province: data.Get("province").([]string), - StreetAddress: data.Get("street_address").([]string), - PostalCode: data.Get("postal_code").([]string), - TTL: time.Duration(data.Get("ttl").(int)) * time.Second, - AllowLocalhost: true, - AllowAnyName: true, - AllowIPSANs: true, - EnforceHostnames: false, - KeyType: "any", - AllowedURISANs: []string{"*"}, - AllowedSerialNumbers: []string{"*"}, - AllowExpirationPastCA: true, - } + OU: data.Get("ou").([]string), + Organization: data.Get("organization").([]string), + Country: data.Get("country").([]string), + Locality: data.Get("locality").([]string), + Province: data.Get("province").([]string), + StreetAddress: data.Get("street_address").([]string), + PostalCode: data.Get("postal_code").([]string), + TTL: time.Duration(data.Get("ttl").(int)) * time.Second, + AllowLocalhost: true, + AllowAnyName: true, + AllowIPSANs: true, + AllowWildcardCertificates: new(bool), + EnforceHostnames: false, + KeyType: "any", + AllowedURISANs: []string{"*"}, + AllowedSerialNumbers: []string{"*"}, + AllowExpirationPastCA: true, + } + *role.AllowWildcardCertificates = true if cn := data.Get("common_name").(string); len(cn) == 0 { role.UseCSRCommonName = true diff --git a/changelog/14238.txt b/changelog/14238.txt new file mode 100644 index 000000000000..8c794eb53ec8 --- /dev/null +++ b/changelog/14238.txt @@ -0,0 +1,3 @@ +```release-note:improvement +secrets/pki: Restrict issuance of wildcard certificates via role parameter (`allow_wildcard_certificates`) +``` diff --git a/website/content/api-docs/secret/pki.mdx b/website/content/api-docs/secret/pki.mdx index db0240ac8e15..c471a72bbc86 100644 --- a/website/content/api-docs/secret/pki.mdx +++ b/website/content/api-docs/secret/pki.mdx @@ -751,10 +751,27 @@ request is denied. certificates for `localhost` as one of the requested common names. This is useful for testing and to allow clients on a single host to talk securely. -- `allowed_domains` `(list: [])` – Specifies the domains of the role. This is - used with the `allow_bare_domains` and `allow_subdomains` options. +~> **Note**: This strictly applies to `localhost` and `localdomain` when this + option is enabled. Additionally, even if this option is disabled, if either + name is included in `allowed_domains`, the match rules for that option + could permit issuance of a certificate for `localhost`. -- `allowed_domains_template` `()bool: false)` – When set, `allowed_domains` +- `allowed_domains` `(list: [])` – Specifies the domains of the role. This is + used with the `allow_bare_domains`, `allow_subdomains`, and `allow_glob_domains` + options to determine the type of matching between these domains and the + values of common name, DNS-typed SAN entries, and Email-typed SAN entries. + When `allow_any_name` is used, this attribute has no effect. + +~> **Note**: The three options `allow_bare_domains`, `allow_subdomains`, and + `allow_glob_domains` are each independent of each other. That is, at least + one type of allowed matching must describe the relationship between the + `allowed_domains` list and the names on the issued certificate. For example, + given `allowed_domain=foo.*.example.com` and `allow_subdomains=true` and + `allow_glob_domains=true`, a request for `bar.foo.baz.example.com` won't + be permitted, even though it `foo.baz.example.com` matches the glob + `foo.*.example.com` and `bar` is a subdomain of that. + +- `allowed_domains_template` `(bool: false)` – When set, `allowed_domains` may contain templates, as with [ACL Path Templating](/docs/concepts/policies). - `allow_bare_domains` `(bool: false)` – Specifies if clients can request @@ -762,23 +779,51 @@ request is denied. configured domain set with `allowed_domains` is `example.com`, this allows clients to actually request a certificate containing the name `example.com` as one of the DNS values on the final certificate. In some scenarios, this can be - considered a security risk. + considered a security risk. Note that when an `allowed_domain` field contains + a potential wildcard character (for example, `allowed_domains=*.example.com`) + and `allow_bare_domains` and `allow_wildcard_certificates` are both enabled, + issuance of a wildcard certificate for `*.example.com` will be permitted. - `allow_subdomains` `(bool: false)` – Specifies if clients can request certificates with CNs that are subdomains of the CNs allowed by the other role options. _This includes wildcard subdomains._ For example, an `allowed_domains` value of `example.com` with this option set to true will - allow `foo.example.com` and `bar.example.com` as well as `*.example.com`. This - is redundant when using the `allow_any_name` option. + allow `foo.example.com` and `bar.example.com` as well as `*.example.com`. To + restrict issuance of wildcards by this option, see `allow_wildcard_certificates` + below. This option is redundant when using the `allow_any_name` option. - `allow_glob_domains` `(bool: false)` - Allows names specified in `allowed_domains` to contain glob patterns (e.g. `ftp*.example.com`). Clients will be allowed to request certificates with names matching the glob patterns. +~> **Note**: These globs behave like shell-style globs and can match + across multiple domain parts. For example, `allowed_domains=*.example.com` + with `allow_glob_domains` enabled will match not only `foo.example.com` but + also `baz.bar.foo.example.com`. + +~> **Warning**: Glob patterns will match wildcard domains and permit their + issuance unless otherwise restricted by `allow_wildcard_certificates`. For + instance, with `allowed_domains=*.*.example.com` and both `allow_glob_domains` + and `allow_wildcard_certificates` enabled, we will permit the issuance of + a wildcard certificate for `*.foo.example.com`. + +- `allow_wildcard_certificates` `(bool: true)` - Allows the issuance of + certificates with [RFC 6125](https://tools.ietf.org/html/rfc6125) wildcards + in the CN field. When set to `false`, this prevents wildcards from being + issued even if they would've been allowed by an option above. We support + the following four wildcard types: + + - `*.example.com`, a single wildcard as the entire left-most label, + - `foo*.example.com`, a single suffixed wildcard in the left-most label, + - `*foo.example.com`, a single prefixed wildcard in the left-most label, and + - `f*o.example.com`, a single interior wildcard in the left-most label. + - `allow_any_name` `(bool: false)` – Specifies if clients can request any CN. Useful in some circumstances, but make sure you understand whether it is - appropriate for your installation before enabling it. + appropriate for your installation before enabling it. Note that both + `enforce_hostnames` and `allow_wildcard_certificates` are still checked, + which may introduce limitations on issuance with this option. - `enforce_hostnames` `(bool: true)` – Specifies if only valid host names are allowed for CNs, DNS SANs, and the host part of email addresses.