Provide the flag for disable unsafe builtins #1191

becky-hd · 2021-03-19T04:31:13Z

Signed-off-by: Becky Huang beckyhd@google.com

What this PR does / why we need it:
Provide the flag allowing us to set --disable-builtin=http.send to toggle off opa http.send builtin

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
Fixes #1137

Special notes for your reviewer:
Reused the flag struct implemented by exempt-namespace, it's now moved to util package.

codecov-io · 2021-03-19T05:08:06Z

Codecov Report

Merging #1191 (92b03d8) into master (2380fac) will decrease coverage by 0.12%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #1191      +/-   ##
==========================================
- Coverage   49.98%   49.86%   -0.13%     
==========================================
  Files          64       65       +1     
  Lines        4469     4470       +1     
==========================================
- Hits         2234     2229       -5     
- Misses       1926     1932       +6     
  Partials      309      309

Flag	Coverage Δ
unittests	`49.86% <0.00%> (-0.13%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/util/flagSet.go	`0.00% <0.00%> (ø)`
pkg/webhook/namespacelabel.go	`72.00% <ø> (+4.25%)`	⬆️
...onstrainttemplate/constrainttemplate_controller.go	`53.72% <0.00%> (-0.65%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2380fac...92b03d8. Read the comment docs.

sozercan · 2021-03-19T18:49:02Z

Can we add a test in e2e to validate this? Although, this might be hard to do since there is no easy way to add flag without helm charts etc

maxsmythe · 2021-03-19T20:29:14Z

We add flags in parts of the Makefile.

What do you think about:

Modifying the e2e tests to install GK with the http builtin disabled
making sure a CT that leverages the HTTP built in errors out?

maxsmythe · 2021-03-19T20:31:44Z

main.go

@@ -109,6 +110,7 @@ func init() {
 	_ = statusv1beta1.AddToScheme(scheme)
 	_ = mutationsv1alpha1.AddToScheme(scheme)
 	// +kubebuilder:scaffold:scheme
+	flag.Var(disabledBuiltin, "disable-builtin", "disable opa built-in function, this flag can be declared more than once.")


nit: disabledBuiltins plural for the variable name, as it's a slice. The flag name is fine.

Any guidance we can provide on what valid builtins are for this flag? Can we link to OPA documentation for example?

maxsmythe · 2021-03-19T20:35:38Z

main.go

@@ -231,7 +233,7 @@ func setupControllers(mgr ctrl.Manager, sw *watch.ControllerSwitch, tracker *rea
 	<-setupFinished

 	// initialize OPA
-	driver := local.New(local.Tracing(false))
+	driver := local.New(local.Tracing(false), local.DisableBuiltins(disabledBuiltin.ToSlice()...))


Do we need to worry about duplicate strings in the slice, or is that handled elsewhere?

becky-hd · 2021-03-19T20:44:45Z

We add flags in parts of the Makefile.

What do you think about:

Modifying the e2e tests to install GK with the http builtin disabled

making sure a CT that leverages the HTTP built in errors out?

I was thinking adding the disable-builtin flag in deploy yaml used by e2e OR patch the deployment in the test runtime.

sozercan · 2021-03-19T21:37:27Z

main.go

@@ -109,6 +110,7 @@ func init() {
 	_ = statusv1beta1.AddToScheme(scheme)
 	_ = mutationsv1alpha1.AddToScheme(scheme)
 	// +kubebuilder:scaffold:scheme
+	flag.Var(disabledBuiltin, "disable-builtin", "disable opa built-in function, this flag can be declared more than once.")


should the flag be called disable-opa-builtin to be more explicit?

ritazh · 2021-03-20T00:18:56Z

Is this only a runtime error that is returned when the constraintTemplate/rego is executed? How does the error get bubbled up? Is it only visible in GK logs? Is there any validation for this when the constraintTemplate is deployed so that the policy owner can act on this?

maxsmythe · 2021-03-20T00:26:59Z

I think this would wind up being a compile-time error and be reported on the CT's status?

maxsmythe · 2021-03-20T00:28:18Z

We add flags in parts of the Makefile.
What do you think about:

Modifying the e2e tests to install GK with the http builtin disabled

making sure a CT that leverages the HTTP built in errors out?

I was thinking adding the disable-builtin flag in deploy yaml used by e2e OR patch the deployment in the test runtime.

Modifying the deploy yaml would probably be less flaky. Fewer moving parts at that time.

ritazh · 2021-03-23T06:52:18Z

I think this would wind up being a compile-time error and be reported on the CT's status?

Thinking about the user experience. Is it possible to block/validate the CT from being created/updated or at least give an error/warning if an opa builtin is found in the rego but it's part of this disable list?

maxsmythe · 2021-03-23T23:10:54Z

That seems like a significant scope increase. Currently any unrecognized function only fails at compilation time and gets reported on status:

  - errors:
    - code: ingest_error
      message: 'Could not ingest Rego: 1 error occurred: __modset_templates["admission.k8s.gatekeeper.sh"]["K8sRequiredLabels"]_idx_0:13:
        rego_type_error: undefined function unknown_func'
    id: gatekeeper-controller-manager-c9fdc8c4-zk8fk
    observedGeneration: 1
    operations:
    - webhook
    templateUID: 6159f75f-cfbf-40b0-a400-333f6a1a4ff7
  - errors:
    - code: ingest_error
      message: 'Could not ingest Rego: 1 error occurred: __modset_templates["admission.k8s.gatekeeper.sh"]["K8sRequiredLabels"]_idx_0:13:
        rego_type_error: undefined function unknown_func'

Running compilation as part of the webhook would put us at risk of timing out (compilation can take a while)

ritazh · 2021-03-24T02:27:41Z

Running compilation as part of the webhook would put us at risk of timing out (compilation can take a while)

Definitely not suggesting this. I was thinking if we could statically identify this is used in the rego. Like say look for specific string values?

maxsmythe · 2021-03-25T23:45:02Z

I don't think we can reliably do static analysis without compilation here, unfortunately.

Simple string matching risks matching against non-active code (e.x. denying a constraint template because there is a comment: # we must cache data because using http.send() is insecure and therefore disabled), substrings, alternative implementations of functions written in pure Rego, and potentially other places.

Removing the unsafe function from Rego's lexicon and relying on the compiler to find unbound references is the only reliable method AFAIK.

maxsmythe

LGTM after the Helm chart issues are sorted.

I defer to @ritazh @sozercan and @shomron's opinions for that.

maxsmythe · 2021-04-24T05:05:13Z

cmd/build/helmify/kustomize-for-helm.yaml

@@ -69,6 +69,7 @@ spec:
            - --exempt-namespace={{ .Release.Namespace }}
            - --operation=webhook
            - --enable-mutation={{ .Values.experimentalEnableMutation}}
+            - --disable-opa-builtin={{ .Values.builtinHttpSend }}


What happens here if no value is provided? Will this still work?

It works with unset value in this case, but seems like a good idea to remove empty string in FlagSet.ToSlice().

maxsmythe · 2021-04-24T05:07:43Z

test/bats/tests/templates/use_http_send_template.yaml

+        package k8sdenynamehttpsend
+        violation[{"msg": msg}] {
+          input.review.object.metadata.name == input.parameters.invalidName
+          response := http.send({"method": "get", "url": "https://github.com/"})


Ironically, any kind of "get" in our tests makes me nervous. It's a good thing this is disabled!

maxsmythe · 2021-04-24T05:09:33Z

cmd/build/helmify/static/values.yaml

@@ -70,3 +70,4 @@ pdb:
  controllerManager:
    minAvailable: 1
 service: {}
+builtinHttpSend:


We probably want to name this something like disabledBuiltins and allow it to take a list of disabled builtins. Helm chart users may want to disable more than HTTP send

+1 on naming and we need a default

Changed the variable into a list, that solves the unset --disable-opa-builtin for us.

maxsmythe · 2021-04-24T05:14:48Z

It looks like the upgrade test is failing because --disable-opa-builtin isn't getting set for that test.

maxsmythe

LGTM @sozercan @ritazh or @shomron, LGTY?

charts/gatekeeper/templates/gatekeeper-controller-manager-deployment.yaml

Signed-off-by: Becky Huang <beckyhd@google.com>

sozercan

LGTM

becky-hd force-pushed the http-1137 branch 2 times, most recently from fc1c58f to ff909dd Compare March 19, 2021 05:04

becky-hd force-pushed the http-1137 branch from ff909dd to 3f1b811 Compare March 19, 2021 18:22

maxsmythe requested changes Mar 19, 2021

View reviewed changes

sozercan reviewed Mar 19, 2021

View reviewed changes

becky-hd force-pushed the http-1137 branch 3 times, most recently from 92b03d8 to 093e48e Compare March 23, 2021 03:36

becky-hd force-pushed the http-1137 branch from 093e48e to 13bf873 Compare March 24, 2021 22:04

maxsmythe changed the title ~~Provide the flag for diable unsafe builtins~~ Provide the flag for disable unsafe builtins Apr 17, 2021

becky-hd force-pushed the http-1137 branch 3 times, most recently from 47c15cd to d94f572 Compare April 23, 2021 20:56

maxsmythe approved these changes Apr 24, 2021

View reviewed changes

becky-hd force-pushed the http-1137 branch from d94f572 to 3987e07 Compare April 27, 2021 03:14

maxsmythe approved these changes Apr 28, 2021

View reviewed changes

maxsmythe requested review from sozercan and shomron April 28, 2021 01:05

maxsmythe requested a review from ritazh April 28, 2021 01:05

sozercan reviewed Apr 28, 2021

View reviewed changes

charts/gatekeeper/templates/gatekeeper-controller-manager-deployment.yaml Outdated Show resolved Hide resolved

Provide the flag for diable unsafe builtins

96202d7

Signed-off-by: Becky Huang <beckyhd@google.com>

becky-hd force-pushed the http-1137 branch from 3987e07 to 96202d7 Compare April 28, 2021 03:13

sozercan approved these changes Apr 28, 2021

View reviewed changes

sozercan merged commit d883817 into open-policy-agent:master Apr 28, 2021

julianKatz pushed a commit to julianKatz/gatekeeper that referenced this pull request May 11, 2021

Provide the flag for disable unsafe builtins (open-policy-agent#1191)

9cfb321

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide the flag for disable unsafe builtins #1191

Provide the flag for disable unsafe builtins #1191

becky-hd commented Mar 19, 2021

codecov-io commented Mar 19, 2021 •

edited

sozercan commented Mar 19, 2021 •

edited

maxsmythe commented Mar 19, 2021

maxsmythe Mar 19, 2021

shomron Mar 22, 2021

maxsmythe Mar 19, 2021

becky-hd commented Mar 19, 2021

sozercan Mar 19, 2021

ritazh commented Mar 20, 2021

maxsmythe commented Mar 20, 2021

maxsmythe commented Mar 20, 2021

ritazh commented Mar 23, 2021 •

edited

maxsmythe commented Mar 23, 2021 •

edited

ritazh commented Mar 24, 2021 •

edited

maxsmythe commented Mar 25, 2021

maxsmythe left a comment

maxsmythe Apr 24, 2021

becky-hd Apr 27, 2021

maxsmythe Apr 24, 2021

maxsmythe Apr 24, 2021

sozercan Apr 26, 2021

becky-hd Apr 27, 2021

maxsmythe commented Apr 24, 2021

maxsmythe left a comment

sozercan left a comment

Provide the flag for disable unsafe builtins #1191

Provide the flag for disable unsafe builtins #1191

Conversation

becky-hd commented Mar 19, 2021

codecov-io commented Mar 19, 2021 • edited

Codecov Report

sozercan commented Mar 19, 2021 • edited

maxsmythe commented Mar 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

becky-hd commented Mar 19, 2021

Choose a reason for hiding this comment

ritazh commented Mar 20, 2021

maxsmythe commented Mar 20, 2021

maxsmythe commented Mar 20, 2021

ritazh commented Mar 23, 2021 • edited

maxsmythe commented Mar 23, 2021 • edited

ritazh commented Mar 24, 2021 • edited

maxsmythe commented Mar 25, 2021

maxsmythe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxsmythe commented Apr 24, 2021

maxsmythe left a comment

Choose a reason for hiding this comment

sozercan left a comment

Choose a reason for hiding this comment

codecov-io commented Mar 19, 2021 •

edited

sozercan commented Mar 19, 2021 •

edited

ritazh commented Mar 23, 2021 •

edited

maxsmythe commented Mar 23, 2021 •

edited

ritazh commented Mar 24, 2021 •

edited