Fix broken interactions between glob_domains and wildcards (#14235)
* Allow issuance of wildcard via glob match From Vault v1.8.0 onwards, we would incorrectly disallow issuance of a wildcard certificate when allow_glob_domain was enabled with a multi-part glob domain in allowed_domains (such as *.*.foo) when attempting to issue a wildcard for a subdomain (such as *.bar.foo). This fixes that by reverting an errant change in the case insensitivity patch. Here, when validating against a very powerful glob construct, we leave the wildcard prefix (*.) from the raw common_name element, to allow multi-part globs to match wildcard entries. It is important to note that "sanitizedName" is an incorrect variable naming here. Wildcard parsing (per RFC 6125 which supercedes RFC 2818) must be in the left-most segment of the domain, but we lack validation to ensure no internal wildcards exist. Additionally per item 3 of section 6.4.3 of RFC 6125, wildcards MAY be internal to a domain segment, in which case sanitizedName again leaves the wildcard in place. Resolves: #13530 Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com> * Remove duplicate email address check As pointed out by Steven Clark (author of the removed conditional in 70012cd865b3dcdab376dba0c0e0abc88c48f508), this is duplicate from the now-reintroduced comparison against name (versus the erroneous sanitizedName at the time of his commit). This is a reversion of the changes to builtin/logical/pki/cert_util.go, but keeping the additional valuable test cases. Co-authored-by: Steven Clark <steven.clark@hashicorp.com> Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com> * Add multi-dimensional PKI role issuance tests This commit introduces multi-dimensional testing of PKI secrets engine's role-based certificate issuance with the intent of preventing future regressions. Here, dimensions of testing include: - AllowedDomains to decide which domains are approved for issuance, - AllowBareDomains to decide if raw entries of AllowedDomains are permitted, - AllowGlobDomains to decide if glob patterns in AllowedDomains are parsed, - AllowSubdomains to decide if subdomains of AllowedDomains are permitted, - AllowLocalhost to decide if localhost identifiers are permitted, and - CommonName of the certificate to request. Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com> * Add changelog entry Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com> Co-authored-by: Steven Clark <steven.clark@hashicorp.com>
This commit is contained in:
Alexander Scheel
GitHub
parent
36430a34ac
commit
1877fc16d7
|
|
@ -3904,6 +3904,275 @@ func TestBackend_Root_FullCAChain(t *testing.T) {
|
|||
}
|
||||
}
|
||||
|
||||
type MultiBool int
|
||||
|
||||
const (
|
||||
MFalse MultiBool = iota
|
||||
MTrue MultiBool = iota
|
||||
MAny MultiBool = iota
|
||||
)
|
||||
|
||||
func (o MultiBool) ToValues() []bool {
|
||||
if o == MTrue {
|
||||
return []bool{true}
|
||||
}
|
||||
|
||||
if o == MFalse {
|
||||
return []bool{false}
|
||||
}
|
||||
|
||||
if o == MAny {
|
||||
return []bool{true, false}
|
||||
}
|
||||
|
||||
return []bool{}
|
||||
}
|
||||
|
||||
type IssuanceRegression struct {
|
||||
AllowedDomains []string
|
||||
AllowBareDomains MultiBool
|
||||
AllowGlobDomains MultiBool
|
||||
AllowSubdomains MultiBool
|
||||
AllowLocalhost MultiBool
|
||||
CommonName string
|
||||
Issued bool
|
||||
}
|
||||
|
||||
func RoleIssuanceRegressionHelper(t *testing.T, client *api.Client, index int, test IssuanceRegression) int {
|
||||
tested := 0
|
||||
for _, AllowBareDomains := range test.AllowBareDomains.ToValues() {
|
||||
for _, AllowGlobDomains := range test.AllowGlobDomains.ToValues() {
|
||||
for _, AllowSubdomains := range test.AllowSubdomains.ToValues() {
|
||||
for _, AllowLocalhost := range test.AllowLocalhost.ToValues() {
|
||||
|
||||
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,
|
||||
})
|
||||
|
||||
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)
|
||||
}
|
||||
|
||||
tested += 1
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return tested
|
||||
}
|
||||
|
||||
func TestBackend_Roles_IssuanceRegression(t *testing.T) {
|
||||
// Regression testing of role's issuance policy.
|
||||
testCases := []IssuanceRegression{
|
||||
// allowed, bare, glob, subdomains, localhost, cn, issued
|
||||
|
||||
// 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},
|
||||
|
||||
// Localhost forbidden, not matching allowed domains -> not issued
|
||||
/* 13 */ {[]string{"*.*.foo"}, MAny, MAny, MAny, MFalse, "localhost", false},
|
||||
// Localhost allowed, not matching allowed domains -> issued
|
||||
/* 14 */ {[]string{"*.*.foo"}, MAny, MAny, MAny, MTrue, "localhost", true},
|
||||
// Localhost allowed via allowed domains (and bare allowed), not by AllowLocalhost -> issued
|
||||
/* 15 */ {[]string{"localhost"}, MTrue, MAny, MAny, MFalse, "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},
|
||||
|
||||
// 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},
|
||||
|
||||
// === 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},
|
||||
|
||||
// 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},
|
||||
|
||||
// === 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},
|
||||
|
||||
// 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},
|
||||
|
||||
// === 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},
|
||||
|
||||
// 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},
|
||||
|
||||
// === 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)
|
||||
|
||||
// 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
|
||||
|
||||
// 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 {
|
||||
t.Fatalf("misnumbered test case entries will make it hard to find bugs: %v", len(testCases))
|
||||
}
|
||||
|
||||
coreConfig := &vault.CoreConfig{
|
||||
LogicalBackends: map[string]logical.Factory{
|
||||
"pki": Factory,
|
||||
},
|
||||
}
|
||||
cluster := vault.NewTestCluster(t, coreConfig, &vault.TestClusterOptions{
|
||||
HandlerFunc: vaulthttp.Handler,
|
||||
})
|
||||
cluster.Start()
|
||||
defer cluster.Cleanup()
|
||||
|
||||
client := cluster.Cores[0].Client
|
||||
var err error
|
||||
|
||||
// Generate a root CA at /pki to use for our tests
|
||||
err = client.Sys().Mount("pki", &api.MountInput{
|
||||
Type: "pki",
|
||||
Config: api.MountConfigInput{
|
||||
DefaultLeaseTTL: "12h",
|
||||
MaxLeaseTTL: "128h",
|
||||
},
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
resp, err := client.Logical().Write("pki/root/generate/exported", map[string]interface{}{
|
||||
"common_name": "myvault.com",
|
||||
"ttl": "128h",
|
||||
"key_type": "ec",
|
||||
"key_bits": 256,
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if resp == nil {
|
||||
t.Fatal("expected ca info")
|
||||
}
|
||||
|
||||
tested := 0
|
||||
for index, test := range testCases {
|
||||
tested += RoleIssuanceRegressionHelper(t, client, index, test)
|
||||
}
|
||||
|
||||
t.Log(fmt.Sprintf("Issuance regression expanded matrix test scenarios: %d", tested))
|
||||
}
|
||||
|
||||
var (
|
||||
initTest sync.Once
|
||||
rsaCAKey string
|
||||
|
|
|
|||
|
|
@ -353,10 +353,8 @@ func validateNames(b *backend, data *inputBundle, names []string) string {
|
|||
// First, allow an exact match of the base domain if that role flag
|
||||
// is enabled
|
||||
if data.role.AllowBareDomains &&
|
||||
(strings.EqualFold(sanitizedName, currDomain) ||
|
||||
(isEmail && strings.EqualFold(emailDomain, currDomain)) ||
|
||||
// Handle the use case of AllowedDomain being an email address
|
||||
(isEmail && strings.EqualFold(name, currDomain))) {
|
||||
(strings.EqualFold(name, currDomain) ||
|
||||
(isEmail && strings.EqualFold(emailDomain, currDomain))) {
|
||||
valid = true
|
||||
break
|
||||
}
|
||||
|
|
@ -371,7 +369,7 @@ func validateNames(b *backend, data *inputBundle, names []string) string {
|
|||
|
||||
if data.role.AllowGlobDomains &&
|
||||
strings.Contains(currDomain, "*") &&
|
||||
glob.Glob(currDomain, sanitizedName) {
|
||||
glob.Glob(currDomain, name) {
|
||||
valid = true
|
||||
break
|
||||
}
|
||||
|
|
|
|||
|
|
@ -0,0 +1,3 @@
|
|||
```release-note:bug
|
||||
secrets/pki: Fix issuance of wildcard certificates matching glob patterns
|
||||
```
|
||||
Loading…
Reference in New Issue