Strict namespace annotation regex #329
Conversation
|
Thanks for adding this @leosunmo, we'll try and take a look asap. |
|
this seems like a very sensible change, however there is the possibility that there are people currently relying on this "feature" so making it stricter could be a breaking change for them. Maybe we should add this behind a flag and look at making it default in the next major version? |
|
Agreed. I'd potentially control with a flag so it's also possible for people to disable if they prefer, but agreed, better to default to current behaviour for now and potentially change on a major release. |
|
Pulling in release v3.5. Didn't seem to change anything, will re-run tests. |
|
Tests still pass. At some point I could look at adding a flag, but I don't have a lot of free time to look at this. |
Set false for NewNamespacePermittedRoleNamePolicy by default except for where we are already doing strict regexp testing.
Tests failure with strict namespace regexp enabled as well as backwards compatibility when strict namespace regexp disabled.
StrictNamespaceRegexp adds start of line and end of line anchors to the namespace regexp annotation. This ensures that a vague namespace annotation regex such as 'red' doesn't allow roles that contain the letters "red" anywhere in their Role ARN to be assumed by pods. Instead the requested role will be checked against the regex '^red$' if strict namespace regexp is enabled.
|
This has been brought in line with master branch. Added the flag Let me know if the flag makes sense, the code looks good and the tests are sane. Still running this change (and #327) in production right now, and it's working well. |
|
I've been teeing up other changes for v4 that makes some breaking flag and deprecation changes. While reviewing issues and PRs I came back to this. @Joseph-Irving @leosunmo how do you feel about incorporating now? I feel like the default behaviour should now be the stricter form with the ability for people to revert to v3.x behaviour if necessary? |
|
I think making this a breaking change with a revert flag is a great idea. I will try to find time to look at this again asap. |
Since strict namespace regex will not be the default, we should assume it's enabled by default in all tests.
|
Okay, I've made the strict namespace regex behaviour default. The flag is a bit cumbersome but at least it's explicit and clear about its behaviour. Inverted the tests to hopefully catch the new expected default behaviour. |
|
Thanks again @leosunmo. I'm still keen to pull this into v4 (I'll mark it for the milestone) and will try and help with the merge (there's been a few other changes to help improve things and pull in ExternalID / Session Naming support). |
|
@pingles Okay went through and fixed merge conflicts. Haven't looked at the Let me know if there's something you'd like to see changed. |
| @@ -209,7 +209,7 @@ func (b *KiamServerBuilder) Build() (*KiamServer, error) { | |||
| credentialsProvider: credentialsCache, | |||
| assumePolicy: Policies( | |||
| NewRequestingAnnotatedRolePolicy(b.podCache, arnResolver), | |||
| NewNamespacePermittedRoleNamePolicy(b.namespaceCache, arnResolver), | |||
| NewNamespacePermittedRoleNamePolicy(!b.config.DisableStrictNamespaceRegexp, b.namespaceCache, arnResolver), | |||
There was a problem hiding this comment.
Generally don't like subtle things like this where you end up with a double-negative...
I felt like the code should deal with a strict bool that was by default true, but the flag should be very verbose to make sure you understand you are about to reduce safety.
This (rather crudely) adds beginning-of-string and end-of-string assertion for the namespace annotation regex.
Resolves: #328
This should stop really short or vague regex from passing.
I was afraid regex with pre-existing assertions might cause issues (https://github.com/leosunmo/kiam/blob/978092d031db57b4d7d884d1956822e2075e65d2/pkg/server/policy_test.go#L124), but it doesn't look like it. I had a good play around here and it seems fine to double-up on these assertions.
We could always check if the expression string starts and/or ends we these assertions and trim it.