Skip to content

Commit

Permalink
Fix broken interactions between glob_domains and wildcards (#14241)
Browse files Browse the repository at this point in the history 
* 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 <[email protected]>

* Remove duplicate email address check

As pointed out by Steven Clark (author of the removed conditional in
70012cd), 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 <[email protected]>
Signed-off-by: Alexander Scheel <[email protected]>

* 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 <[email protected]>

* Add changelog entry

Signed-off-by: Alexander Scheel <[email protected]>

Co-authored-by: Steven Clark <[email protected]>

Co-authored-by: Steven Clark <[email protected]>
  • Loading branch information
@cipherboy @stevendpclark
cipherboy and stevendpclark authored Feb 24, 2022
1 parent 56619cf commit 850981d
Show file tree
Hide file tree
Showing 3 changed files with 275 additions and 5 deletions.
269 changes: 269 additions & 0 deletions builtin/logical/pki/backend_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3269,6 +3269,275 @@ func TestBackend_RevokePlusTidy_Intermediate(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
Expand Down
8 changes: 3 additions & 5 deletions builtin/logical/pki/cert_util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -330,10 +330,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
}
Expand All @@ -348,7 +346,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
}
Expand Down
3 changes: 3 additions & 0 deletions changelog/14235.txt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
secrets/pki: Fix issuance of wildcard certificates matching glob patterns
```

0 comments on commit 850981d

Please sign in to comment.