Forbidden sysctls cel #521
Conversation
ritazh
force-pushed
the
forbidden-sysctls-cel
branch
5 times, most recently
from
8d2d872
to
f9b0657
Compare
ritazh
force-pushed
the
forbidden-sysctls-cel
branch
2 times, most recently
from
9887541
to
8bab861
Compare
| expression: | | ||
| !has(variables.params.forbiddenSysctls) ? [] : | ||
| variables.params.forbiddenSysctls.filter(sysctl, !sysctl.endsWith("*")) | ||
| - name: allAllowed |
There was a problem hiding this comment.
I don't think the Rego code considers undefined allowedSysctls to mean "all allowed". I think it just denies all sysctls. Worth creating a test case to validate this edge case.
There was a problem hiding this comment.
Looking at the history: https://github.com/open-policy-agent/gatekeeper-library/pull/253/files#diff-b14d9c092cc66bb50ced20769a8508a11132aa8425a27e3a65666fb580b52414R117-R119
comment in the test:
Empty allowedSysctls means none are allowed
unspecified allowedSysctls which does not place any restrictions by itself
The rego code also does not put any restriction on unspecified allowedSysctls.
If we want to change this behavior, it is definitely not backward compatible. IMO we should keep the same logic to reduce confusion. WDYT? @maxsmythe
There was a problem hiding this comment.
Definitely no need to change the behavior. My reading of the Rego code was that empty allowedSysctls leads to a violation (despite what the test comment says).
Maybe my reading is wrong? In either case, adding a test suite for the edge case would validate uniform behavior.
But yeah, to re-iterate, VAP-CEL should behave like the Rego, just want to ensure that is the case.
There was a problem hiding this comment.
yes I adding 2 new tests (one for empty allowedSysctls and one for unspecified allowedSysctls, which showed existing rego has the same behavior as the test comment due to:
not allowed_sysctl(sysctl) and allowed_sysctl(_) never actually checks for not input.parameters.allowedSysctls
There was a problem hiding this comment.
Ah, I think I see why. The message expression below fails if the parameter is unspecified vs. empty:
msg := sprintf("The sysctl %v is not explicitly allowed, pod: %v. Allowed sysctls: %v", [sysctl, input.review.object.metadata.name, input.parameters.allowedSysctls])
which will cause the violation not to be thrown. I imaging replacing the message with a constant would change the behavior of that edge case. Fun times.
Sorry for the extra work. If we add the undefined vs. empty list test, at least we can make sure the edge-case behavior doesn't change in the future. Worth doing?
There was a problem hiding this comment.
updated for cel.
e.g. output:
The sysctl is not allowed for pod: nginx-forbidden-sysctls-disallowed, forbidden: kernel.*, allowed: unspecified
and
The sysctl is not allowed for pod: nginx-forbidden-sysctls-disallowed, forbidden: kernel.*, allowed: empty
There was a problem hiding this comment.
re: rego bug, I propose to fix the message and add condition for no violations for unspecified allowedSysctls to ensure backward compat. wdyt?
There was a problem hiding this comment.
Is the rego not WAI? It will simply not return a message/violation if allowedSysctls is unspecified, which is the expected behavior.
I am definitely down for making the test for allowedSysctls explicit in the Rego... having an implicit logic gate buried in a string formatting statement seems a bit hard to catch for readers.
| expression: | | ||
| !has(variables.params.forbiddenSysctls) ? [] : | ||
| variables.params.forbiddenSysctls.filter(sysctl, !sysctl.endsWith("*")) | ||
| - name: allAllowed |
There was a problem hiding this comment.
Ah, I think I see why. The message expression below fails if the parameter is unspecified vs. empty:
msg := sprintf("The sysctl %v is not explicitly allowed, pod: %v. Allowed sysctls: %v", [sysctl, input.review.object.metadata.name, input.parameters.allowedSysctls])
which will cause the violation not to be thrown. I imaging replacing the message with a constant would change the behavior of that edge case. Fun times.
Sorry for the extra work. If we add the undefined vs. empty list test, at least we can make sure the edge-case behavior doesn't change in the future. Worth doing?
...y/pod-security-policy/forbidden-sysctls/1.2.0/samples/psp-forbidden-sysctls/constraint3.yaml
Outdated
Show resolved
Hide resolved
Signed-off-by: Rita Zhang <[email protected]>
ritazh
force-pushed
the
forbidden-sysctls-cel
branch
3 times, most recently
from
fd9ff56
to
76bd55b
Compare
Signed-off-by: Rita Zhang <[email protected]>
ritazh
force-pushed
the
forbidden-sysctls-cel
branch
from
76bd55b
to
0a7d73d
Compare
| apiVersion: v1 | ||
| kind: Pod | ||
| metadata: | ||
| name: nginx-forbidden-sysctls-disallowed |
There was a problem hiding this comment.
| name: nginx-forbidden-sysctls-disallowed | |
| name: nginx-forbidden-sysctls-allowed |
|
This issue/PR has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions. |
What this PR does / why we need it:
Which issue(s) does this PR fix (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when the PR gets merged):Fixes #541
Special notes for your reviewer:
will rebase once #519 is merged