Ensure OPA strict mode compliance

Hi Gatekeeper friends! 👋😃 In order to ensure the Gatekeeper library works with future updates of OPA, and to better catch some common mistakes in Rego policy in the future, this PR aims to fix all errors reported from `opa check --strict src`, and to make sure future changes are compliant with the OPA [strict mode](https://www.openpolicyagent.org/docs/latest/policy-language/#strict-mode) checks. The errors raised from strict mode, and which are fixed with this PR, include: * `input` must not be shadowed/assigned * Unused function args — use wildcards (`_`) in their place * Unused local assignment * Deprecated built-in functions * `re_match` -> `regex.match` * `all` -> these were not needed * `any` -> `strings.any_prefix_match` and `strings.any_suffix_match` as these were always used together with `startswith`/`endswith` The by far most noisy fix was the first one, i.e. `input` getting shadowed in basically all tests. Changed to use `inp` for a name as `in` is of course another name we don't want to shadow 😅 The changes here do not change semantics of the code. I did however also make some additional fixes where e.g. the unused function args error showed that a function wasn't needed in the first place, and so on. Naturally, all tests pass as before. Two things I'm not sure about: 1. The code introduces the `strings.any_prefix_match` and `strings.any_suffix_match` built-in functions in one policy. These were introduced in OPA v0.44.0, which is quite a few OPA and Gatekeeper versions ago. I could not find information on what the minimum version of OPA or Gatekeeper must be supported in the Rego code in this library. If needed, that change can be reverted. 2. https://github.com/open-policy-agent/gatekeeper-library mentions Versioning. Should I update the versions of each policy changed with this PR? To make sure the code stays compliant, I've added the `opa check --strict` command to execute with the `test.sh` script, before the unit tests run. Signed-off-by: Anders Eknert <[email protected]>
open-policy-agent · Oct 30, 2023 · 1839c6e · 1839c6e
1 parent 68b6d53
commit 1839c6e
Show file tree

Hide file tree

Showing 92 changed files with 1,953 additions and 2,035 deletions.
diff --git a/.github/workflows/workflow.yaml b/.github/workflows/workflow.yaml
@@ -45,7 +45,7 @@ jobs:
     strategy:
       matrix:
         os: [ "ubuntu-latest", "macos-latest" ]
-        opa: [ "v0.42.2" ]
+        opa: [ "v0.54.0" ]
     name: Unit test on ${{ matrix.os }} opa ${{ matrix.opa }}
     steps:
       - name: Harden Runner

diff --git a/library/general/allowedrepos/template.yaml b/library/general/allowedrepos/template.yaml
@@ -29,21 +29,18 @@ spec:
 
         violation[{"msg": msg}] {
           container := input.review.object.spec.containers[_]
-          satisfied := [good | repo = input.parameters.repos[_] ; good = startswith(container.image, repo)]
-          not any(satisfied)
+          not strings.any_prefix_match(container.image, input.parameters.repos)
           msg := sprintf("container <%v> has an invalid image repo <%v>, allowed repos are %v", [container.name, container.image, input.parameters.repos])
         }
 
         violation[{"msg": msg}] {
           container := input.review.object.spec.initContainers[_]
-          satisfied := [good | repo = input.parameters.repos[_] ; good = startswith(container.image, repo)]
-          not any(satisfied)
+          not strings.any_prefix_match(container.image, input.parameters.repos)
           msg := sprintf("initContainer <%v> has an invalid image repo <%v>, allowed repos are %v", [container.name, container.image, input.parameters.repos])
         }
 
         violation[{"msg": msg}] {
           container := input.review.object.spec.ephemeralContainers[_]
-          satisfied := [good | repo = input.parameters.repos[_] ; good = startswith(container.image, repo)]
-          not any(satisfied)
+          not strings.any_prefix_match(container.image, input.parameters.repos)
           msg := sprintf("ephemeralContainer <%v> has an invalid image repo <%v>, allowed repos are %v", [container.name, container.image, input.parameters.repos])
         }
diff --git a/library/general/containerlimits/template.yaml b/library/general/containerlimits/template.yaml
@@ -65,7 +65,7 @@ spec:
         canonify_cpu(orig) = new {
           not is_number(orig)
           not endswith(orig, "m")
-          re_match("^[0-9]+(\\.[0-9]+)?$", orig)
+          regex.match("^[0-9]+(\\.[0-9]+)?$", orig)
           new := to_number(orig) * 1000
         }
 
@@ -162,7 +162,7 @@ spec:
           not is_number(orig)
           suffix := get_suffix(orig)
           raw := replace(orig, suffix, "")
-          re_match("^[0-9]+(\\.[0-9]+)?$", raw)
+          regex.match("^[0-9]+(\\.[0-9]+)?$", raw)
           new := to_number(raw) * mem_multiple(suffix)
         }
 

diff --git a/library/general/containerrequests/template.yaml b/library/general/containerrequests/template.yaml
@@ -65,7 +65,7 @@ spec:
         canonify_cpu(orig) = new {
           not is_number(orig)
           not endswith(orig, "m")
-          re_match("^[0-9]+(\\.[0-9]+)?$", orig)
+          regex.match("^[0-9]+(\\.[0-9]+)?$", orig)
           new := to_number(orig) * 1000
         }
 
@@ -162,7 +162,7 @@ spec:
           not is_number(orig)
           suffix := get_suffix(orig)
           raw := replace(orig, suffix, "")
-          re_match("^[0-9]+(\\.[0-9]+)?$", raw)
+          regex.match("^[0-9]+(\\.[0-9]+)?$", raw)
           new := to_number(raw) * mem_multiple(suffix)
         }
 

diff --git a/library/general/containerresourceratios/template.yaml b/library/general/containerresourceratios/template.yaml
@@ -68,14 +68,14 @@ spec:
         canonify_cpu(orig) = new {
           not is_number(orig)
           not endswith(orig, "m")
-          re_match("^[0-9]+$", orig)
+          regex.match("^[0-9]+$", orig)
           new := to_number(orig) * 1000
         }
 
         canonify_cpu(orig) = new {
           not is_number(orig)
           not endswith(orig, "m")
-          re_match("^[0-9]+[.][0-9]+$", orig)
+          regex.match("^[0-9]+[.][0-9]+$", orig)
           new := to_number(orig) * 1000
         }
 
@@ -172,7 +172,7 @@ spec:
           not is_number(orig)
           suffix := get_suffix(orig)
           raw := replace(orig, suffix, "")
-          re_match("^[0-9]+(\\.[0-9]+)?$", raw)
+          regex.match("^[0-9]+(\\.[0-9]+)?$", raw)
           new := to_number(raw) * mem_multiple(suffix)
         }
 

diff --git a/library/general/disallowedtags/template.yaml b/library/general/disallowedtags/template.yaml
@@ -44,16 +44,15 @@ spec:
         violation[{"msg": msg}] {
             container := input_containers[_]
             not is_exempt(container)
-            tags := [forbid | tag = input.parameters.tags[_] ; forbid = endswith(container.image, concat(":", ["", tag]))]
-            any(tags)
+            tags := [tag_with_prefix | tag := input.parameters.tags[_]; tag_with_prefix := concat(":", ["", tag])]
+            strings.any_suffix_match(container.image, tags)
             msg := sprintf("container <%v> uses a disallowed tag <%v>; disallowed tags are %v", [container.name, container.image, input.parameters.tags])
         }
 
         violation[{"msg": msg}] {
             container := input_containers[_]
             not is_exempt(container)
-            tag := [contains(container.image, ":")]
-            not all(tag)
+            not contains(container.image, ":")
             msg := sprintf("container <%v> didn't specify an image tag <%v>", [container.name, container.image])
         }
 

diff --git a/library/general/ephemeralstoragelimit/template.yaml b/library/general/ephemeralstoragelimit/template.yaml
@@ -146,7 +146,7 @@ spec:
           not is_number(orig)
           suffix := get_suffix(orig)
           raw := replace(orig, suffix, "")
-          re_match("^[0-9]+(\\.[0-9]+)?$", raw)
+          regex.match("^[0-9]+(\\.[0-9]+)?$", raw)
           new := to_number(raw) * storage_multiple(suffix)
         }
 

diff --git a/library/general/httpsonly/template.yaml b/library/general/httpsonly/template.yaml
@@ -39,19 +39,19 @@ spec:
 
         violation[{"msg": msg}] {
           input.review.object.kind == "Ingress"
-          re_match("^(extensions|networking.k8s.io)/", input.review.object.apiVersion)
+          regex.match("^(extensions|networking.k8s.io)/", input.review.object.apiVersion)
           ingress := input.review.object
           not https_complete(ingress)
-          not tls_is_optional(ingress)
+          not tls_is_optional
           msg := sprintf("Ingress should be https. tls configuration and allow-http=false annotation are required for %v", [ingress.metadata.name])
         }
 
         violation[{"msg": msg}] {
           input.review.object.kind == "Ingress"
-          re_match("^(extensions|networking.k8s.io)/", input.review.object.apiVersion)
+          regex.match("^(extensions|networking.k8s.io)/", input.review.object.apiVersion)
           ingress := input.review.object
           not annotation_complete(ingress)
-          not tls_not_optional(ingress)
+          tls_is_optional
           msg := sprintf("Ingress should be https. The allow-http=false annotation is required for %v", [ingress.metadata.name])
         }
 
@@ -65,15 +65,7 @@ spec:
           ingress.metadata.annotations["kubernetes.io/ingress.allow-http"] == "false"
         }
 
-        tls_is_optional(ingress) = true {
+        tls_is_optional {
           parameters := object.get(input, "parameters", {})
-          tlsOptional := object.get(parameters, "tlsOptional", false)
-          is_boolean(tlsOptional)
-          true == tlsOptional
-        }
-
-        tls_not_optional(ingress) = true {
-          parameters := object.get(input, "parameters", {})
-          tlsOptional := object.get(parameters, "tlsOptional", false)
-          true != tlsOptional
+          object.get(parameters, "tlsOptional", false) == true
         }
diff --git a/library/general/imagedigests/template.yaml b/library/general/imagedigests/template.yaml
@@ -42,24 +42,21 @@ spec:
         violation[{"msg": msg}] {
           container := input.review.object.spec.containers[_]
           not is_exempt(container)
-          satisfied := [re_match("@[a-z0-9]+([+._-][a-z0-9]+)*:[a-zA-Z0-9=_-]+", container.image)]
-          not all(satisfied)
+          not regex.match("@[a-z0-9]+([+._-][a-z0-9]+)*:[a-zA-Z0-9=_-]+", container.image)
           msg := sprintf("container <%v> uses an image without a digest <%v>", [container.name, container.image])
         }
 
         violation[{"msg": msg}] {
           container := input.review.object.spec.initContainers[_]
           not is_exempt(container)
-          satisfied := [re_match("@[a-z0-9]+([+._-][a-z0-9]+)*:[a-zA-Z0-9=_-]+", container.image)]
-          not all(satisfied)
+          not regex.match("@[a-z0-9]+([+._-][a-z0-9]+)*:[a-zA-Z0-9=_-]+", container.image)
           msg := sprintf("initContainer <%v> uses an image without a digest <%v>", [container.name, container.image])
         }
 
         violation[{"msg": msg}] {
           container := input.review.object.spec.ephemeralContainers[_]
           not is_exempt(container)
-          satisfied := [re_match("@[a-z0-9]+([+._-][a-z0-9]+)*:[a-zA-Z0-9=_-]+", container.image)]
-          not all(satisfied)
+          not regex.match("@[a-z0-9]+([+._-][a-z0-9]+)*:[a-zA-Z0-9=_-]+", container.image)
           msg := sprintf("ephemeralContainer <%v> uses an image without a digest <%v>", [container.name, container.image])
         }
       libs:

diff --git a/library/general/noupdateserviceaccount/template.yaml b/library/general/noupdateserviceaccount/template.yaml
@@ -30,13 +30,15 @@ spec:
     rego: |
       package noupdateserviceaccount
 
-      privileged(userInfo, allowedUsers, allowedGroups) {
+      privileged(userInfo, allowedUsers, _) {
         # Allow if the user is in allowedUsers.
         # Use object.get so omitted parameters can't cause policy bypass by
         # evaluating to undefined.
         username := object.get(userInfo, "username", "")
         allowedUsers[_] == username
-      } {
+      }
+
+      privileged(userInfo, _, allowedGroups) {
         # Allow if the user's groups intersect allowedGroups.
         # Use object.get so omitted parameters can't cause policy bypass by
         # evaluating to undefined.

diff --git a/library/general/replicalimits/template.yaml b/library/general/replicalimits/template.yaml
@@ -46,7 +46,7 @@ spec:
         }
 
         input_replica_limit(spec) {
-            provided := input.review.object.spec.replicas
+            provided := spec.replicas
             count(input.parameters.ranges) > 0
             range := input.parameters.ranges[_]
             value_within_range(range, provided)

diff --git a/library/general/requiredannotations/template.yaml b/library/general/requiredannotations/template.yaml
@@ -54,6 +54,6 @@ spec:
           expected := input.parameters.annotations[_]
           expected.key == key
           expected.allowedRegex != ""
-          not re_match(expected.allowedRegex, value)
+          not regex.match(expected.allowedRegex, value)
           msg := sprintf("Annotation <%v: %v> does not satisfy allowed regex: %v", [key, value, expected.allowedRegex])
         }
diff --git a/library/general/requiredlabels/template.yaml b/library/general/requiredlabels/template.yaml
@@ -41,14 +41,11 @@ spec:
       rego: |
         package k8srequiredlabels
 
-        get_message(parameters, _default) = msg {
+        get_message(parameters, _default) := _default {
           not parameters.message
-          msg := _default
         }
 
-        get_message(parameters, _default) = msg {
-          msg := parameters.message
-        }
+        get_message(parameters, _) := parameters.message
 
         violation[{"msg": msg, "details": {"missing_labels": missing}}] {
           provided := {label | input.review.object.metadata.labels[label]}
@@ -65,7 +62,7 @@ spec:
           expected.key == key
           # do not match if allowedRegex is not defined, or is an empty string
           expected.allowedRegex != ""
-          not re_match(expected.allowedRegex, value)
+          not regex.match(expected.allowedRegex, value)
           def_msg := sprintf("Label <%v: %v> does not satisfy allowed regex: %v", [key, value, expected.allowedRegex])
           msg := get_message(input.parameters, def_msg)
         }
diff --git a/library/general/uniqueingresshost/template.yaml b/library/general/uniqueingresshost/template.yaml
@@ -42,10 +42,10 @@ spec:
 
         violation[{"msg": msg}] {
           input.review.kind.kind == "Ingress"
-          re_match("^(extensions|networking.k8s.io)$", input.review.kind.group)
+          regex.match("^(extensions|networking.k8s.io)$", input.review.kind.group)
           host := input.review.object.spec.rules[_].host
           other := data.inventory.namespace[_][otherapiversion]["Ingress"][name]
-          re_match("^(extensions|networking.k8s.io)/.+$", otherapiversion)
+          regex.match("^(extensions|networking.k8s.io)/.+$", otherapiversion)
           other.spec.rules[_].host == host
           not identical(other, input.review)
           msg := sprintf("ingress host conflicts with an existing ingress <%v>", [host])

diff --git a/library/pod-security-policy/capabilities/template.yaml b/library/pod-security-policy/capabilities/template.yaml
@@ -125,14 +125,11 @@ spec:
           count(all - dropped) > 0
         }
 
-        get_default(obj, param, _default) = out {
-          out = obj[param]
-        }
+        get_default(obj, param, _) := obj[param]
 
-        get_default(obj, param, _default) = out {
+        get_default(obj, param, _default) := _default {
           not obj[param]
           not obj[param] == false
-          out = _default
         }
       libs:
         - |

diff --git a/library/pod-security-policy/forbidden-sysctls/template.yaml b/library/pod-security-policy/forbidden-sysctls/template.yaml
@@ -63,7 +63,7 @@ spec:
         }
 
         # * may be used to forbid all sysctls
-        forbidden_sysctl(sysctl) {
+        forbidden_sysctl(_) {
             input.parameters.forbiddenSysctls[_] == "*"
         }
 
@@ -78,7 +78,7 @@ spec:
         }
 
         # * may be used to allow all sysctls
-        allowed_sysctl(sysctl) {
+        allowed_sysctl(_) {
             input.parameters.allowedSysctls[_] == "*"
         }
 

diff --git a/library/pod-security-policy/fsgroup/template.yaml b/library/pod-security-policy/fsgroup/template.yaml
@@ -58,7 +58,7 @@ spec:
             msg := sprintf("The provided pod spec fsGroup is not allowed, pod: %v. Allowed fsGroup: %v", [input.review.object.metadata.name, input.parameters])
         }
 
-        input_fsGroup_allowed(spec) {
+        input_fsGroup_allowed(_) {
             # RunAsAny - No range is required. Allows any fsGroup ID to be specified.
             input.parameters.rule == "RunAsAny"
         }

diff --git a/library/pod-security-policy/host-filesystem/template.yaml b/library/pod-security-policy/host-filesystem/template.yaml
@@ -54,7 +54,7 @@ spec:
             msg := sprintf("HostPath volume %v is not allowed, pod: %v. Allowed path: %v", [volume, input.review.object.metadata.name, allowedPaths])
         }
 
-        input_hostpath_violation(allowedPaths, volume) {
+        input_hostpath_violation(allowedPaths, _) {
             # An empty list means all host paths are blocked
             allowedPaths == []
         }

diff --git a/library/pod-security-policy/host-network-ports/template.yaml b/library/pod-security-policy/host-network-ports/template.yaml
@@ -65,12 +65,12 @@ spec:
             o.spec.hostNetwork
         }
 
-        input_share_hostnetwork(o) {
+        input_share_hostnetwork(_) {
             hostPort := input_containers[_].ports[_].hostPort
             hostPort < input.parameters.min
         }
 
-        input_share_hostnetwork(o) {
+        input_share_hostnetwork(_) {
             hostPort := input_containers[_].ports[_].hostPort
             hostPort > input.parameters.max
         }

diff --git a/library/pod-security-policy/proc-mount/template.yaml b/library/pod-security-policy/proc-mount/template.yaml
@@ -68,7 +68,7 @@ spec:
             allowedProcMount == "default"
             lower(c.securityContext.procMount) == "default"
         }
-        input_proc_mount_type_allowed(allowedProcMount, c) {
+        input_proc_mount_type_allowed(allowedProcMount, _) {
             allowedProcMount == "unmasked"
         }
 

diff --git a/library/pod-security-policy/seccomp/template.yaml b/library/pod-security-policy/seccomp/template.yaml
@@ -98,7 +98,7 @@ spec:
             msg := get_message(result.profile, result.file, name, result.location, allowed_profiles)
         }
 
-        get_message(profile, file, name, location, allowed_profiles) = message {
+        get_message(profile, _, name, location, allowed_profiles) = message {
             not profile == "Localhost"
             message := sprintf("Seccomp profile '%v' is not allowed for container '%v'. Found at: %v. Allowed profiles: %v", [profile, name, location, allowed_profiles])
         }
@@ -121,7 +121,7 @@ spec:
         }
 
         # Simple allowed Profiles
-        allowed_profile(profile, file, allowed) {
+        allowed_profile(profile, _, allowed) {
             not startswith(lower(profile), "localhost")
             profile == allowed[_]
         }
@@ -136,20 +136,20 @@ spec:
         }
 
         # seccomp Localhost with wildcard
-        allowed_profile(profile, file, allowed) {
+        allowed_profile(profile, _, allowed) {
             profile == "Localhost"
             input_wildcard_allowed_files
             profile == allowed[_]
         }
 
         # annotation localhost with wildcard
-        allowed_profile(profile, file, allowed) {
+        allowed_profile(profile, _, allowed) {
             "localhost/*" == allowed[_]
             startswith(profile, "localhost/")
         }
 
         # annotation localhost without wildcard
-        allowed_profile(profile, file, allowed) {
+        allowed_profile(profile, _, allowed) {
             startswith(profile, "localhost/")
             profile == allowed[_]
         }

diff --git a/library/pod-security-policy/selinux/template.yaml b/library/pod-security-policy/selinux/template.yaml
@@ -94,7 +94,7 @@ spec:
         field_allowed(field, options, params) {
             params[field] == options[field]
         }
-        field_allowed(field, options, params) {
+        field_allowed(field, options, _) {
             not has_field(options, field)
         }