Feature/regexp policy matcher

[drbig]

Addresses directly this issue.
Reworked from prefix matching to regexp matching, while trying to preserve the current behavior.

Our motivation is due to Metronome prefixing task_name with a unique id, hence breaking the simple prefix matching.

2019-08-28: Run full tests and all pass.

[drbig]

Some thoughts/considerations:

1. By trying to make existing syntax work the "rules" as to what to put where become rather loose

E.g:

"mesos": {
  "regexp": "mesos:framework:\\.*",
  ...
}

Is equivalent to:

"mesos:framework:*": {
  ...
}

Is equivalent to:

"mesos:*": {
  "regexp": "framework:\\.*",
  ...
}

Can't honestly say whether this is a feature or a bug, but generally would lean that if we want to enforce one set of rules then no wildcards at all at the key level - either a an exact matching path and no regexp defined, or some placeholder key e.g. <re> and then regexp must be defined (but that then defeats the radix tree...)...

2. Previously the * was in fact a prefix-matching. Currently there is no enforcement of that, but at least adding this would be trivial (regexp.Compile("^" + v.Regexp)).

Of course this in current state would make the rules even more "non-intuitive".

[nemosupremo]

Thanks for the contribution! When I first saw #76 I was a bit perplexed on how I would design this feature. I think your approach is a bit more powerful, but also more "optional" and has resulted in a cleaner design than what I was thinking.

Can't honestly say whether this is a feature or a bug, but generally would lean that if we want to enforce one set of rules then no wildcards at all at the key level - either a an exact matching path and no regexp defined, or some placeholder key e.g. and then regexp must be defined (but that then defeats the radix tree...)...

I thought about this, while I don't like the fact that there are multiple ways to specify a rule and I prefer prefix matching, I understand that the regex matching is necessary and that we should prefer one way of doing things. However, to your point about leaning towards regex, I would argue the other way. Introducing regex also means introducing a couple more footguns; an improper regex can mean you accidentally make a policy too broad. If someone wants to be conservative, it's easier to do that with prefix matching.

I prefer how you've currently implemented things - use prefix matching where appropriate (which I believe is most cases), and dropdown to regex if you need something more powerful and opt into the footguns. If your problem isn't solved by prefix-matching, then falldown into regex - hopefully you don't need to, but if you do it's there and if you don't there's a more straightforward, less powerful to accomplish a more general task.

[jsynowiec]

Of course this in current state would make the rules even more "non-intuitive".

Maybe at first. But after you do a few, then it's straightforward.

I thought about this, while I don't like the fact that there are multiple ways to specify a rule (...) and that we should prefer one way of doing things.

I definitely see this concerns as a feature. You can achieve the same outcome in multiple ways, so you can choose the one that suits your case the most.

I understand that the regex matching is necessary

Definitely. In addition to #76 I already have a few other use cases that require either wildcard/glob inside the rule or regular expression to work, like giving a set of policies to all services inside a group.

an improper regex can mean you accidentally make a policy too broad. If someone wants to be conservative, it's easier to do that with prefix matching.

I think you shouldn't be too protective for the users. It's they decision on what and how they want to use. Vault policies could be abused on their own - one can just allow anything and then just match mesos:* in Gatekeeper 🤷‍♂️

[drbig]

@nemosupremo Well put argument for "explicit path match, then wildcard, and only then if really needed use a regexp". Thought it over and it is indeed saner than "explicit path match or regexp". So 👍 on this point.

With the above said I don't see any straightforward way to "enforce" "either this way or that way", so I'm personally OK with "you can do the same in (at least) three ways, if you wish".

I was a bit surprised that you've already merged :) I still see as outstanding:

1. Should we enforce the prefix-matching behavior? By always prefixing v.Regexp with ^? I don't have enough experience to tell if this may be a problem or not (somewhere down the line...).
2. The test case I did was copy&paste, there could be more and definitely cleaner.
3. Didn't touch the documentation.

I think I should be able to find time to do a PR for 1 and/or 2 if there is a consensus. As from the POV of our business needs the current state is already good enough though.