fix(rbac): handle malformed rbac policy #11964
Conversation
zekth
force-pushed
the
fix/rbac-missing-comma
branch
from
78a0650 to
c029254
Compare
util/rbac/rbac.go
Outdated
| @@ -472,6 +472,12 @@ func loadPolicyLine(line string, model model.Model) error { | |||
| return fmt.Errorf("invalid RBAC policy: %s", line) | |||
| } | |||
|
|
|||
| for i := 0; i < len(tokens); i++ { | |||
There was a problem hiding this comment.
I wonder if a role could contain a space. Like `p, some spacey role, applications, , myapp/, allow
There was a problem hiding this comment.
I made the assumption that not indeed.
There was a problem hiding this comment.
Want to limit this check to everything besides the role field, so it's an easy-merge?
There was a problem hiding this comment.
Can't we just assume that there are two types of policies right now:
- Grants (
g), which has two fields (the grantee and the role granted) - Policy (
p), which has five fields (the role, the resource type, the verb, the resource name pattern and the action)
So wouldn't it make more sense to evaluate the policy type and check whether it has the right amount of fields set?
There was a problem hiding this comment.
I think we might already do that. But the fields are split on commas, not spaces.
There was a problem hiding this comment.
I ment, instead of checking whether a field contains white space, couldn't we just check for len(tokens) according to the type of policy (g or p) of the currently evaluated record?
There was a problem hiding this comment.
Oh, sure, since we wouldn't hit the right number of tokens when there's a comma missing. I do like that better.
There was a problem hiding this comment.
Will push the fix torward that approach then, i wasn't sure if there was special cases that i was not was about.
There was a problem hiding this comment.
@alexmt Are you aware of RBAC policy definitions that have a different pattern, e.g. a variable number of fields in a permission definition?
zekth
force-pushed
the
fix/rbac-missing-comma
branch
2 times, most recently
from
1473415 to
6e96579
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #11964 +/- ##
==========================================
+ Coverage 48.98% 49.06% +0.08%
==========================================
Files 249 249
Lines 43066 43127 +61
==========================================
+ Hits 21094 21160 +66
+ Misses 19864 19855 -9
- Partials 2108 2112 +4
☔ View full report in Codecov by Sentry. |
zekth
force-pushed
the
fix/rbac-missing-comma
branch
4 times, most recently
from
a726b44 to
4fef3a9
Compare
zekth
force-pushed
the
fix/rbac-missing-comma
branch
from
4fef3a9 to
1462282
Compare
|
@crenshaw-dev any update on this? |
zekth
force-pushed
the
fix/rbac-missing-comma
branch
from
1462282 to
0ceec2f
Compare
zekth
force-pushed
the
fix/rbac-missing-comma
branch
from
0ceec2f to
160e57d
Compare
zekth
force-pushed
the
fix/rbac-missing-comma
branch
from
a152e4f to
4e2fa06
Compare
zekth
force-pushed
the
fix/rbac-missing-comma
branch
from
4e2fa06 to
ed216a2
Compare
zekth
force-pushed
the
fix/rbac-missing-comma
branch
from
ed216a2 to
2d2c5ca
Compare
zekth
force-pushed
the
fix/rbac-missing-comma
branch
from
2d2c5ca to
4065522
Compare
Signed-off-by: Vincent Le Goff <vincent.legoff@konghq.com>
zekth
force-pushed
the
fix/rbac-missing-comma
branch
from
4065522 to
fb4571d
Compare
|
@crenshaw-dev fixed |
Signed-off-by: Vincent Le Goff <vincent.legoff@konghq.com>
Signed-off-by: Vincent Le Goff <vincent.legoff@konghq.com>
The rbac parser happens to swallow some malformed entries like the one i added in test:
p, role:Myrole, applications, *, myapp/* allowThe outcome of this, the whole rbac was broken and then no apps, no clusters were showing up in argo-cd UI. This PR addresses this problem.
Checklist: