Add constraints on the Common Name for certificate-based authentication

[michaelansel]

While the signing chain gives you strong assertions of identity and validity, it doesn't always give you a strong definition of authorization. It is common to enforce constraints on the Common Name (and other fields) in the certificate at signing time, and then leverage those constrained fields at authorization time (standard website TLS is a perfect example). This change introduces one such constraint on the Common Name field: in addition to matching the signing chain (ensuring the validity of the certificate), we also require a matching Common Name before selecting the policies to assign.

This aligns the Cert auth backend more closely with the EC2 auth backend, in which you have both an assertion of validity (signed identity document) and an assertion of access (roles inside the identity document).

In the future (or now) we could also add similar matching for the O, OU, SANS, or specific required extensions (e.g. "must be a client cert, not a server cert"). It would seem beneficial to align this as close as possible with the signing constraints in the PKI backend, though CN is definitely unique (PKI backend handles CN prefix restriction by constraining the CN parameter in the signing request instead of in the role configuration).

Commits:

1. Refactor to put all the constraint logic in a single place (vs replicated between the NonCA and CA sections); added some comments along the way since the variable names are rather confusing; added an explicit failure if there are multiple matching ParsedCerts
2. Introduce prefix/suffix matching of the Common Name

I'm not married to the idea of prefix/suffix matching, but it seemed to meet my needs (host*.domain.parts) and was an easy way to gain familiarity with the code. Other ideas I came up with that I'm happy to implement:

• full regex
• split into hostname and domain components and support globbing on those (more like the PKI backend)
• split out user/domain components for email addresses as the CN

re: adding explicit failure on multiple chains -- this is a change in behavior (previously, one entry was picked semi-randomly), but seems like a better failure case than the previous behavior. Ideally we would test for this in pathCertWrite, but I'm not sure how feasible that is. I feel like detecting this case is even more important as we add constraints that could be overlapping. I can remove it if you feel otherwise.

[michaelansel]

Thinking about this more, I almost wonder if we should resolve the "multiple matches" problem by allowing the client to specify which role they would like to authenticate against, like in the other credential backends.

[jefferai]

Really multiple matches should be multiple successful chains since if only one chain is successful the desired behavior is obvious. (That may be what you coded, don't have time to look now.) But I'm into the idea of the client specifying the desired cert name to auth against (although for backwards compatibility this shouldn't be required, so handling the multiple matches issue is still necessary).

Probably globbing on a Name is good enough, although really you should check all DNS/Email SANs, not just CN. No full regexes, please...but you knew I was going to say that :-)

[michaelansel]

Really multiple matches should be multiple successful chains since if only one chain is successful the desired behavior is obvious.
I'm a little confused here. In the released state, if there are multiple matches, we succeed and pick a random set of policies with no notification. In the new state, we would fail with an error ("too many matches; we don't know what to do"). I could imagine this causing problems if someone was previously configuring both the root and intermediate with the same policy set, but that seems like a really weird intentional use case. Maybe we add a post-upgrade check that confirms there aren't any intersecting certificate chains (but only if the chains have no other constraints on them)?

Should I go ahead an also introduce explicit cert name selection in this PR, or would you like to keep that separate? I think it requires adding a new route and passing the specified cert name into b.loadTrustedCerts for filtering.

For the constraints, here is my next idea:

required_name: "host*domain.parts" (split on * and do prefix/suffix match; fail on multiple *; excludes doing a "contains" match)
allow_alt_names: true (at least one of CN, SANs must match the required_name)

[jefferai]

I'm a little confused here. In the released state, if there are multiple matches, we succeed and pick a random set of policies with no notification. In the new state, we would fail with an error ("too many matches; we don't know what to do").

If only one chain succeeds, there's no real question about what to do -- just use that chain's policies. If multiple succeed, you can either simply pick the first chain that succeeds opportunistically (which is what in theory happens now, except with a bug), or bail and say I don't know which one you want, so you need to specify it at login time. That would be a backwards-incompatible change in behavior from now. I think the right thing to do would be to fix the bug, and then also add the ability to specify the cert you want to match against. If you don't specify, opportunistically match. That's backwards compatible but also lets you pick if there are multiple matching paths.

I think it requires adding a new route

It doesn't, just requires adding a parameter to the login path.

required_name: "host*domain.parts" (split on * and do prefix/suffix match; fail on multiple *; excludes doing a "contains" match)

This seems complicated. Why require this split, why not just glob like with the PR you sent for the PKI backend?

allow_alt_names: true (at least one of CN, SANs must match the required_name)

General Internet behavior is to always allow alternate names. Is there a reason to ignore them?

[michaelansel]

• Understood and agreed on the backwards compatibility; I'll fix that
• 👍 on adding the new parameter to the login path. I'll get on that.
• I see your point on SANs. No need for the second param.

required_name: "host*domain.parts" (split on * and do prefix/suffix match; fail on multiple *; excludes doing a "contains" match)

This seems complicated. Why require this split, why not just glob like with the PR you sent for the PKI backend?

I presume you're thinking of #2517? The key difference is that I need to match that complex pattern from the 0.7.0 release blog post: myhostXXXXXX.mycorp.com, so I need both a prefix and a suffix match.

[michaelansel]

I probably should have read #2517 more carefully before commenting. Nevermind that. I've fixed it to use go-glob.

[michaelansel]

Pretty sure I addressed everything. I added the exact same version of go-glob as #2517, so it should merge cleanly.

I'm not sure how much of the documentation is generated from the code. Is there anything I need to update?

[michaelansel]

Oh, and I discovered that the basic_singleCert test was actually doing the exact same thing as basic_CA test, so I fixed it to do the expected thing.

[jefferai]

Hi @michaelansel ,

No documentation is generated from code, sadly. 😢

[michaelansel]

Docs + CLI support added

[michaelansel]

CI failure references Azure. I'm fairly certain I didn't change anything that would break that test...

# github.com/hashicorp/vault/physical
physical/azure_test.go:38: cleanupClient.GetBlobService().DeleteContainerIfExists undefined (type "github.com/hashicorp/vault/vendor/github.com/Azure/azure-storage-go".BlobStorageClient has no field or method DeleteContainerIfExists)

[jefferai]

Rebase -- it's due to vendored deps update.

[jefferai]

(Or merge :-) )

[michaelansel]

no dice. same failure: https://travis-ci.org/hashicorp/vault/builds/222882326#L370

[jefferai]

Oops. I can't test now but I think it's now fixed.

[michaelansel]

Sweet. Looks like we are all clean now.

[vishalnayak]

Code changes are looking good to me. Feature wise, I wonder if required_name can be changed to required_names (TypeStringSlice/TypeCommaStringSlice) listing many globs. That way, globs of CN, Email addresses and Domain names can be specified, requiring any one to match.

[vishalnayak]

s/connState.PeerCertificates[0]/clientCert

[vishalnayak]

s/connState.PeerCertificates[0]/clientCert. This would need increasing the scope of clientCert above.

[jefferai]

I like the required_names suggestion, it makes sense.

[michaelansel]

Done. I had originally shied away from multiple names because I thought it would be confusing (my instinct reaction was that required_names would be an ANDed list of names), but maybe I was just being overly paranoid. I don't have a strong preference either way.

[vishalnayak]

You might want to use the new TypeCommaStringSlice type here.

[vishalnayak]

@michaelansel The name required_names seems to mean an AND by intuition, however an OR seems reasonable for now. We could have a way to specify how we want to evaluate the constraints, AND or OR. But this can be deferred for now and can be done if there are pressing needs and it looks like the code can easily transition to that with backwards compatibility.

[vishalnayak]

@michaelansel Jeff and I were talking about this and he suggested that it might be a good idea to rename required_names to allowed_names to make it align with the behavior. We can have required_names going forward if needed and it can honor ANDing behavior.

[michaelansel]

Totally missed the addition of TypeCommaStringSlice; I'll make the change

allowed_names makes sense, I'll update to match. I was originally thinking we would just add require_all_names or something, but I think the pair of lists is more flexible.

[jefferai]

@michaelansel we're looking to release 0.7.1 next week, so please keep that in mind if you want to see this in that release!

[michaelansel]

Thanks for the heads up, @jefferai! I've got the rename complete, but I'm struggling with the new data type.

Commit: michaelansel@1c59f04

Failing to compile:

$ make test TEST=./builtin/credential/cert
go generate
CGO_ENABLED=0 VAULT_TOKEN= VAULT_ACC= go test -tags='vault' ./builtin/credential/cert  -timeout=20m -parallel=4
# github.com/hashicorp/vault/builtin/credential/cert
builtin/credential/cert/path_login.go:219: cannot use allowedName (type rune) as type string in argument to glob.Glob
builtin/credential/cert/path_login.go:224: cannot use allowedName (type rune) as type string in argument to glob.Glob
builtin/credential/cert/path_login.go:230: cannot use allowedName (type rune) as type string in argument to glob.Glob
FAIL	github.com/hashicorp/vault/builtin/credential/cert [build failed]
make: *** [test] Error 2

I think this means that the loop is iterating over runes in a string instead of the expected array of strings that the unit tests suggest should be returned. This could easily be caused by my lack of Go knowledge; I couldn't find any real usage examples to copy :-(

[jefferai]

@michaelansel The reason is that you changed it to iterate over a string, and each iterative step over a string is an individual rune, whereas before it was iterating over a split string (e.g. a []string).

[michaelansel]

Doesn't the new data type automatically do the comma-split? Or does it just validate that it is a comma-separated string?

[michaelansel]

(I'm basing my understanding entirely off of this test case: https://github.com/hashicorp/vault/blob/master/logical/framework/field_data_test.go#L194-L202)

[jefferai]

It does, except you didn't change the Config entry to []string! So you're taking an interface and saying it's a string rather than a []string and saving that; then when you iterate over it it's iterating over the runes.

[michaelansel]

Bingo. All fixed. Thanks for the help!

[jefferai]

Sounds good, LMK when you're ready for another look.

[michaelansel]

Pretty sure this is all done at this point.

[jefferai]

LGTM! @vishalnayak can you give another once-over?