k8sallowedrepos: Fix Security Bypass for Image Pulling

[yakirk]

What this PR does / why we need it: This PR fixes security issues in the current "Allowed Repositories" (k8sallowedrepos) rule that can be bypassed in several ways. The previous rule only checked for a suffix match, so possible scenarios were:

1. The user defined allowing image downloads from a registry like "myregistry.io" and didn't add a '/' at the end. So, the rule could be bypassed by establishing a malicious registry under myregistry.io.malicious.com/malicious-image .
2. The user defined allowing image downloads from the repository of a specific username on docker.io, for example docker.io/mycompany. The rule could be bypassed by establishing a malicious repository on Docker Hub like docker.io/mycompanyevil/malicious-image.

This is why when a user adds a constraint to pull images from specific repositories or registries, the rule appends a '/' to the end if it's not already present. The PR also provides the option to allow only specific image names (because they require special checking). For example, in the previous rule, since the rule only checked for a suffix match, if the user defined to allow downloading only "ubuntu", it was possible to download images like "ubuntu-evil" and basically any image name that starts with the same prefix. This PR enhances security by preventing potential bypasses of the existing rule.

[yakirk]

Hey, is there anything I can do to make this PR merge? I am struggling to understand why the build failed.

[maxsmythe]

I think the DCO check is failing. Details for how to fix that should be in the "details" link next to the failed check: https://github.com/open-policy-agent/gatekeeper-library/pull/529/checks?check_run_id=25572805190

I haven't looked at the code yet, but some high-level observations:

• If we are switching to v2 (which this code would require, since this is a behavioral change), then we need to add a V2 suffix to the constraint kind.

• I don't know if we want to try to distinguish between repos and images. That leads us to a place where we are trying to replicate docker's image parsing logic in Rego, which seems complex. Would it be better to just rename the field from "repos" to "prefix" to make it clear to users that this is merely a string check? Users can then decide what makes sense for them.

[yakirk]

Hey Max, I appreciate the answer.

1. I will fix the DCO.
2. I will add the v2 to the constraint kind.
3. If we don't distinguish between repos and images, this opens the ground for bypasses and security issues, which I will explain: Imagine scenarios where a user adds a constraint to allow only images from the organization "openpolicyagent" and defines the policy without a slash ("openpolicyagent"). In this case, we want to add a "/" to ensure that the pulled images are from Docker Hub and only from the repos of the user "openpolicyagent," not a different user. But lets think about the current case - if we define a constraint to allow only the "ubuntu" image, we cannot add "/", because it will break the pulling. However, if we keep using the prefix approach, an attacker can create a username on Docker Hub like "ubuntumyusername123/evil-image" and download any image they wish. This is one of the reasons why it is important to separate the cases of registry/repository and image name. Furthermore, for the image name, we need to check if the next character doesn't exist or if it contains ":" or "@". I agree that the Docker image parsing logic is complicated, but this is exactly what I did in the PR. I used the documentation of Kubernetes regarding image names to ensure we are aligned with this policy too - Kubernetes Documentation.

[yakirk]

I will close this PR and open a new, clean one. The DCO caused some issues.