Add globbing support to the PKI backend's allowed_domains list #2517

spectrumjade · 2017-03-21T20:19:00Z

This change adds simple glob support to the "allowed_domains" list in the PKI backend. This is useful if you would like role policies to allow more flexible name patterns without allowing any name or any subdomain.

It uses an asterisk as the glob character, which I think works for this use case because an asterisk cannot be used in a DNS domain name and in a certificate the wildcard can only be in one specific place (a single wildcard can be present in the left-most name position, and it must be the only component of the dot boundary). A PKI policy of "*.domain.com" will still allow the explicit name "*.domain.com". It will also allow anything ending in ".domain.com", however this is consistent with threat modeling around the DNS hierarchy.

jefferai · 2017-03-26T18:22:49Z

@justintime32 I think for now I'd prefer not to make these changes to email addresses, unless you have a particular use-case in mind. Both the operational and threat model are different for email vs. DNS and I don't see a need for it so would prefer to avoid expanding its scope.

spectrumjade · 2017-03-26T19:37:10Z

Hey @jefferai - I don't have a use case for email domain globbing. I'll go ahead and revert that part of the change.

jefferai

I think this is good to go after that change.

jefferai · 2017-03-28T21:05:53Z

builtin/logical/pki/cert_util.go

@@ -330,8 +331,8 @@ func validateNames(req *logical.Request, names []string, role *roleEntry) string
 					}
 				}

-				if strings.HasSuffix(sanitizedName, "."+req.DisplayName) ||


I think this use case is also not needed. It's difficult to reason about a use-case where you would want to make a token display name a globbing CN.

spectrumjade · 2017-03-29T01:27:54Z

Done!

spectrumjade · 2017-04-03T18:43:01Z

@jefferai any other thoughts?

jefferai · 2017-04-03T21:20:16Z

No, just haven't gotten back to this yet.

jefferai · 2017-04-17T16:13:41Z

Hi @spectrumjade ,

I've taken a while to get back to this because every time I look at it something about it eats away at me and makes me really uncomfortable, mostly for two reasons:

It changes behavior for the end user in a way they may not be expecting
It works around the point of the checks; e.g. if you're doing a check on the bare domain, checking for a glob of the bare domain is really no longer respecting the AllowBareDomains flag.

I want to go a slightly different direction instead: adding an explicit role parameter AllowGlobbedDomains. If this parameter is set to true, then along with the existing checks as-is, we simply have another check in the verify logic that does a straight-up glob check against the given role domain and the supplied SAN.

This is much less magic, is explicit to the user, and makes it easier to reason about the right thing to do in the BareDomain/Subdomain checking code.

It's a little extra work but not much. Hopefully it's understandable why I want to go this way.

michaelansel · 2017-04-17T18:04:49Z

@jefferai What do you think about instead adding an allowed_names parameter that is a comma-separated list of globs? Basically a PKI complement to #2595. That seems to me like it would protect backwards-compatibility, maintain the meaning of allowed_domains, and provide the functionality @spectrumjade is looking for (full name matching on all requested names).

jefferai · 2017-04-17T18:23:02Z

I think that will be very confusing, honestly. If we're going to add a parameter in either case, what does it buy you over simply explicitly enabling globbing of the existing fields with a bool?

michaelansel · 2017-04-17T18:29:48Z

Reading through the code closer, I think I was misunderstanding the scope of allowed_domains -- it appears to be matching the full name and not just the domain portion. I'll go with "I was confused" and retract my comment ;-)

jefferai · 2017-04-17T18:32:40Z

@michaelansel what it matches basically depends on what flags you have toggled as acceptable things to match (bare domain, subdomains, etc.). But yeah, it doesn't remove hostnames or anything.

spectrumjade · 2017-04-17T20:19:37Z

Hey @jefferai I think that's a fine approach. I'm preparing the change now. One thought I have, I think we should require that allow_bare_domains be enabled for globbing support to be enabled, because performing a glob check on an allowed_domain with no glob pattern in it would implicitly allow bare domains.

jefferai · 2017-04-17T21:13:46Z

@spectrumjade That can be worked around by using .example.com as the name to match rather than example.com. Or we can just not glob-match if there is no asterisk contained within the name.

spectrumjade · 2017-04-17T22:01:39Z

@jefferai Oops, I missed your comment before adding the new commit. Do you think one of those options would be better than the current approach (requiring allow_bare_domains to be set)?

Could you elaborate on the .example.com approach? I'm not sure I understand that part.

jefferai · 2017-04-18T16:20:48Z

I don't like the idea of requiring bare domains to be allowed just to enable globbing, because then you are...allowing bare domains, which you may not want to do.

Why not just require any glob check to have an asterisk in it? Then if there isn't an asterisk, you treat it like a non-globbed domain (must match the allowed subdomains/bare domains settings). That way you can easily mix and match bare domains and allowed globs but control their acceptability separately.

in the allowed_name.

spectrumjade · 2017-04-19T21:45:30Z

@jefferai I've implemented the wildcard check and removed the allow_bare_domains requirement. What are you thoughts on the current change?

jefferai · 2017-04-20T11:43:25Z

I like it. Super simple, super understandable.

chrishoffman

Overall this looks good to me. The only think I would like to see is a few unit tests to cover the new globbing use case.

jefferai · 2017-04-24T15:00:02Z

@spectrumjade Can you add some unit tests?

jefferai · 2017-04-28T16:59:44Z

@spectrumjade 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!

briankassouf

Code looks good to me, as soon as there is a unit test I'd say this is ready to go!

vishalnayak · 2017-04-30T16:16:30Z

builtin/logical/pki/cert_util.go

@@ -361,6 +362,13 @@ func validateNames(req *logical.Request, names []string, role *roleEntry) string
 						break
 					}
 				}
+
+				if role.AllowGlobDomains &&
+					strings.Contains(currDomain, "*") &&


Do we really need the strings.Contains(currDomain, "*") check here?

Read back on the conversation on the PR and understood how not having this check would implicitly allow bare domain. Ignore my previous comment.

jefferai · 2017-05-01T14:40:05Z

Thanks for all of your work on this!

Add globbing support to the PKI backend's allowed_domains list.

9aa4882

jefferai added this to the 0.7.1 milestone Mar 22, 2017

Don't allow glob patterns in email domains in the PKI backend.

bf06985

jefferai requested changes Mar 28, 2017

View reviewed changes

Don't support glob patterns for token display name matching

261e36b

michaelansel mentioned this pull request Apr 15, 2017

Add constraints on the Common Name for certificate-based authentication #2595

Merged

Add a PKI role flag to enable domain globbing

1166eca

Only allow PKI allowed_domains globbing if the glob character appears

9743261

in the allowed_name.

jefferai requested review from chrishoffman and briankassouf April 20, 2017 11:43

chrishoffman suggested changes Apr 20, 2017

View reviewed changes

briankassouf suggested changes Apr 28, 2017

View reviewed changes

Add test case for PKI glob domains

a8d12c3

vishalnayak reviewed Apr 30, 2017

View reviewed changes

jefferai merged commit 2e8e9ed into hashicorp:master May 1, 2017

spectrumjade deleted the pki_glob branch May 1, 2017 17:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add globbing support to the PKI backend's allowed_domains list #2517

Add globbing support to the PKI backend's allowed_domains list #2517

spectrumjade commented Mar 21, 2017 •

edited

Loading

jefferai commented Mar 26, 2017

spectrumjade commented Mar 26, 2017

jefferai left a comment

jefferai Mar 28, 2017

spectrumjade commented Mar 29, 2017

spectrumjade commented Apr 3, 2017

jefferai commented Apr 3, 2017

jefferai commented Apr 17, 2017

michaelansel commented Apr 17, 2017

jefferai commented Apr 17, 2017

michaelansel commented Apr 17, 2017

jefferai commented Apr 17, 2017

spectrumjade commented Apr 17, 2017 •

edited

Loading

jefferai commented Apr 17, 2017

spectrumjade commented Apr 17, 2017

jefferai commented Apr 18, 2017

spectrumjade commented Apr 19, 2017

jefferai commented Apr 20, 2017

chrishoffman left a comment

jefferai commented Apr 24, 2017

jefferai commented Apr 28, 2017

briankassouf left a comment

vishalnayak Apr 30, 2017

vishalnayak Apr 30, 2017 •

edited

Loading

jefferai commented May 1, 2017

Add globbing support to the PKI backend's allowed_domains list #2517

Add globbing support to the PKI backend's allowed_domains list #2517

Conversation

spectrumjade commented Mar 21, 2017 • edited Loading

jefferai commented Mar 26, 2017

spectrumjade commented Mar 26, 2017

jefferai left a comment

Choose a reason for hiding this comment

jefferai Mar 28, 2017

Choose a reason for hiding this comment

spectrumjade commented Mar 29, 2017

spectrumjade commented Apr 3, 2017

jefferai commented Apr 3, 2017

jefferai commented Apr 17, 2017

michaelansel commented Apr 17, 2017

jefferai commented Apr 17, 2017

michaelansel commented Apr 17, 2017

jefferai commented Apr 17, 2017

spectrumjade commented Apr 17, 2017 • edited Loading

jefferai commented Apr 17, 2017

spectrumjade commented Apr 17, 2017

jefferai commented Apr 18, 2017

spectrumjade commented Apr 19, 2017

jefferai commented Apr 20, 2017

chrishoffman left a comment

Choose a reason for hiding this comment

jefferai commented Apr 24, 2017

jefferai commented Apr 28, 2017

briankassouf left a comment

Choose a reason for hiding this comment

vishalnayak Apr 30, 2017

Choose a reason for hiding this comment

vishalnayak Apr 30, 2017 • edited Loading

Choose a reason for hiding this comment

jefferai commented May 1, 2017

spectrumjade commented Mar 21, 2017 •

edited

Loading

spectrumjade commented Apr 17, 2017 •

edited

Loading

vishalnayak Apr 30, 2017 •

edited

Loading