From d119060fe46fda27eaa731a034f01232c5373026 Mon Sep 17 00:00:00 2001 From: Alexander Scheel Date: Thu, 24 Feb 2022 07:41:56 -0600 Subject: [PATCH] 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 --- 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 + 7 files changed, 367 insertions(+), 212 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`) +```