From 6f362d68c0c02757907360cd30d332c55b800424 Mon Sep 17 00:00:00 2001 From: dennis Date: Thu, 30 Jul 2026 15:24:50 -0700 Subject: [PATCH] feat: allow a PodAccessRequest to override the container image By default the Pod that Oz launches runs the same image as the workload its PodAccessTemplate points at. That image is often the wrong tool for the job - a distroless production image has no shell, and a schema migration may need tooling deliberately kept out of the production build. Today the only way around it is for an administrator to author a dedicated PodAccessTemplate (or hand-write a patchSpecOperations JSON patch against /spec/containers/N/image), which puts a human in the loop for what is otherwise a self-service operation. This adds an optional `spec.image` to PodAccessRequest, plus an `--image` flag to `ozctl create PodAccessRequest`, so a developer can ask for a different image against an existing template. The image is only honored if it matches an allow-list configured on the controller at deploy time (`--allowed-image-patterns`, exposed as the `controllerManager.manager.allowedImagePatterns` Helm value). The allow-list lives on the controller rather than on a PodAccessTemplate on purpose: the set of registries an organization will run code from is a cluster-wide security boundary, and letting template authors widen it would defeat the point of having one. With no patterns configured the feature is off and any `spec.image` is rejected, so this is inert until an administrator opts in. Pattern matching confines `*` to a single path segment and offers `**` for matching across segments. This is what stops a leading wildcard from swallowing an untrusted registry host: under naive glob semantics `*.dkr.ecr.*.amazonaws.com/team/*` would also match `evil.example.com/x.dkr.ecr.us-west-2.amazonaws.com/team/backdoor`. Enforcement happens in two places. The validating webhook rejects the request at admission time so the developer gets immediate feedback, and the PodAccessBuilder re-validates before building the Pod. The second check is not redundant: the ValidatingWebhookConfiguration is optional (the chart's `webhook.create` value can disable it), so the builder is the only check guaranteed to run. Notes on the implementation: - `spec.image` is immutable. The Pod is built once, on first reconcile, so a later edit would silently do nothing while leaving the resource claiming to run an image it does not. - The override resolves the "default" container against the *unmutated* PodSpec, matching what PatchPodTemplateSpec() already does. A template's patchSpecOperations may rename containers, so resolving afterwards can fail to find a container that existed when the template author named it. getDefaultContainerID() is extracted into an exported GetDefaultContainerID() to share that logic with callers that have no PodTemplateSpecMutationConfig at all. - Only the image is replaced. The Pod still inherits the target workload's service account, secrets, environment and network identity, so any allow-listed image can run code with that workload's privileges - documented prominently in the chart values and README. Co-Authored-By: Claude Opus 5 (1M context) --- API.md | 50 +++++ README.md | 54 +++++ charts/oz/README.md | 1 + charts/oz/templates/deployment.yaml | 3 + charts/oz/values.yaml | 22 ++ .../crds.wizardofoz.co_podaccessrequests.yaml | 28 +++ examples/pod_access_request.yaml | 9 + .../api/v1alpha1/pod_access_request_test.go | 64 ++++++ .../api/v1alpha1/pod_access_request_types.go | 25 +++ .../v1alpha1/pod_access_request_webhook.go | 37 ++- .../api/v1alpha1/pod_spec_mutation_config.go | 31 ++- .../create_access_resources.go | 79 +++++++ .../create_access_resources_test.go | 94 ++++++++ internal/cmd/manager/main.go | 32 +++ .../ozctl/cmd/create_pod_access_request.go | 14 ++ internal/imagepolicy/imagepolicy.go | 210 ++++++++++++++++++ internal/imagepolicy/imagepolicy_test.go | 204 +++++++++++++++++ 17 files changed, 953 insertions(+), 4 deletions(-) create mode 100644 internal/imagepolicy/imagepolicy.go create mode 100644 internal/imagepolicy/imagepolicy_test.go diff --git a/API.md b/API.md index 43564747..65ec4e06 100644 --- a/API.md +++ b/API.md @@ -776,6 +776,31 @@ time has expired, the resouce will be automatically deleted on the next reconcil

Valid time units are “s”, “m”, “h”.

+ + +image
+ +string + + + +

Image optionally overrides the container image used for the “default” +container of the Pod that Oz launches. This lets a developer run a +purpose-built debugging, shell or migration image without an +administrator having to author a dedicated PodAccessTemplate for it.

+

The image must match one of the patterns that the Oz controller was +deployed with (--allowed-image-patterns, set through the +controllerManager.manager.allowedImagePatterns Helm value). If the +controller has no patterns configured then image overrides are disabled +entirely and this field is rejected.

+

The rest of the PodSpec - volumes, environment, service account - is +still inherited from the template’s target controller, so the overriding +image runs with the same identity and secrets as the workload it is +standing in for. Note also that imagePullSecrets are inherited, so an +image from a registry the workload cannot pull from will fail to start.

+

This field is immutable; request a new PodAccessRequest to change it.

+ + @@ -835,6 +860,31 @@ time has expired, the resouce will be automatically deleted on the next reconcil

Valid time units are “s”, “m”, “h”.

+ + +image
+ +string + + + +

Image optionally overrides the container image used for the “default” +container of the Pod that Oz launches. This lets a developer run a +purpose-built debugging, shell or migration image without an +administrator having to author a dedicated PodAccessTemplate for it.

+

The image must match one of the patterns that the Oz controller was +deployed with (--allowed-image-patterns, set through the +controllerManager.manager.allowedImagePatterns Helm value). If the +controller has no patterns configured then image overrides are disabled +entirely and this field is rejected.

+

The rest of the PodSpec - volumes, environment, service account - is +still inherited from the template’s target controller, so the overriding +image runs with the same identity and secrets as the workload it is +standing in for. Note also that imagePullSecrets are inherited, so an +image from a registry the workload cannot pull from will fail to start.

+

This field is immutable; request a new PodAccessRequest to change it.

+ +

PodAccessRequestStatus diff --git a/README.md b/README.md index f6ed7e85..9ac98655 100644 --- a/README.md +++ b/README.md @@ -297,6 +297,60 @@ spec: maxStorage: 1Gi ``` +#### Overriding the container image + +By default the Pod that *Oz* launches runs the same image as the workload the +[`PodAccessTemplate`][pod_access_template] points at. Sometimes that image is +not what a developer needs - a distroless production image has no shell, and a +schema migration may need tooling that is deliberately kept out of the +production build. + +A [`PodAccessRequest`][pod_access_request] can therefore ask for a different +image, without an administrator authoring a dedicated template for it: + +```yaml +apiVersion: crds.wizardofoz.co/v1alpha1 +kind: PodAccessRequest +metadata: + name: deployment-example +spec: + templateName: deployment-example + duration: 5m + image: registry.example.com/team/debug:v1 +``` + +or from the CLI: + +```bash +$ ozctl create PodAccessRequest deployment-example \ + --image registry.example.com/team/debug:v1 +``` + +This is off by default. The cluster administrator opts in by allow-listing one +or more image patterns when installing the controller, using the +`controllerManager.manager.allowedImagePatterns` Helm value: + +```yaml +controllerManager: + manager: + allowedImagePatterns: + - registry.example.com/team/* +``` + +`*` matches within a single path segment and `**` matches across segments, so +the pattern above permits `registry.example.com/team/debug:v1` but not +`evil.example.com/registry.example.com/team/debug:v1`. Patterns are compared +against the reference exactly as the developer wrote it - *Oz* does not expand a +bare `nginx` into `docker.io/library/nginx` - so write patterns for the fully +qualified form your developers use. + +Treat this as a security boundary. Only the image is replaced; the Pod still +inherits the target workload's service account, secrets, environment and +network identity, so any image you allow-list can run code with that workload's +privileges. Restrict the patterns to registries you control. Note also that +`imagePullSecrets` are inherited from the target workload, so an image from a +registry that workload cannot pull from will simply fail to start. + #### [`ExecAccessTemplate`][exec_access_template] ### Exec Access into Existing Pods diff --git a/charts/oz/README.md b/charts/oz/README.md index a3cfdeab..ccbcaf35 100644 --- a/charts/oz/README.md +++ b/charts/oz/README.md @@ -32,6 +32,7 @@ Kubernetes: `>=1.26.0-0` | Key | Type | Default | Description | |-----|------|---------|-------------| +| controllerManager.manager.allowedImagePatterns | `[]string` | `[]` | Glob patterns describing which container images a `PodAccessRequest` is allowed to select through its `spec.image` field. This lets developers launch a purpose-built debugging or migration image without an administrator authoring a dedicated `PodAccessTemplate` for it. `*` matches within a single path segment and `**` matches across segments, so `registry.example.com/team/*` permits `registry.example.com/team/debug:v1` but not `evil.example.com/registry.example.com/team/debug:v1`. Patterns are compared against the reference exactly as the developer wrote it - Oz does not expand a bare `nginx` into `docker.io/library/nginx` - so write patterns for the fully qualified form your developers use. **This is a security boundary.** An overriding image runs with the target workload's service account, secrets and network identity, so restrict this to registries you control. The default of `[]` disables image overrides entirely. | | controllerManager.manager.image.repository | `string` | `"ghcr.io/diranged/oz"` | Docker Image repository and name to use for the controller. | | controllerManager.manager.image.tag | `string` | `nil` | If set, overrides the .Chart.AppVersion field to set the target image version for the Oz controller. | | controllerManager.manager.resources.limits.cpu | string | `"500m"` | | diff --git a/charts/oz/templates/deployment.yaml b/charts/oz/templates/deployment.yaml index 5fc206da..0b3b25b1 100644 --- a/charts/oz/templates/deployment.yaml +++ b/charts/oz/templates/deployment.yaml @@ -62,6 +62,9 @@ spec: - --health-probe-bind-address=:8081 - --metrics-bind-address=:8443 - --leader-elect + {{- with .Values.controllerManager.manager.allowedImagePatterns }} + - --allowed-image-patterns={{ join "," . }} + {{- end }} securityContext: allowPrivilegeEscalation: false capabilities: diff --git a/charts/oz/values.yaml b/charts/oz/values.yaml index 41e5b19d..c607cf09 100644 --- a/charts/oz/values.yaml +++ b/charts/oz/values.yaml @@ -21,6 +21,28 @@ controllerManager: # target image version for the Oz controller. tag: + # -- (`[]string`) Glob patterns describing which container images a + # `PodAccessRequest` is allowed to select through its `spec.image` field. + # This lets developers launch a purpose-built debugging or migration image + # without an administrator authoring a dedicated `PodAccessTemplate` for + # it. + # + # `*` matches within a single path segment and `**` matches across + # segments, so `registry.example.com/team/*` permits + # `registry.example.com/team/debug:v1` but not + # `evil.example.com/registry.example.com/team/debug:v1`. + # + # Patterns are compared against the reference exactly as the developer + # wrote it - Oz does not expand a bare `nginx` into + # `docker.io/library/nginx` - so write patterns for the fully qualified + # form your developers use. + # + # **This is a security boundary.** An overriding image runs with the target + # workload's service account, secrets and network identity, so restrict + # this to registries you control. The default of `[]` disables image + # overrides entirely. + allowedImagePatterns: [] + resources: limits: cpu: 500m diff --git a/config/crd/bases/crds.wizardofoz.co_podaccessrequests.yaml b/config/crd/bases/crds.wizardofoz.co_podaccessrequests.yaml index a4f5f48a..5c304f59 100644 --- a/config/crd/bases/crds.wizardofoz.co_podaccessrequests.yaml +++ b/config/crd/bases/crds.wizardofoz.co_podaccessrequests.yaml @@ -27,6 +27,11 @@ spec: jsonPath: .status.ready name: Ready type: boolean + - description: Overridden container image + jsonPath: .spec.image + name: Image + priority: 1 + type: string name: v1alpha1 schema: openAPIV3Schema: @@ -62,6 +67,29 @@ spec: Valid time units are "s", "m", "h". pattern: ^[0-9]+(s|m|h)$ type: string + image: + description: |- + Image optionally overrides the container image used for the "default" + container of the Pod that Oz launches. This lets a developer run a + purpose-built debugging, shell or migration image without an + administrator having to author a dedicated `PodAccessTemplate` for it. + + The image must match one of the patterns that the Oz controller was + deployed with (`--allowed-image-patterns`, set through the + `controllerManager.manager.allowedImagePatterns` Helm value). If the + controller has no patterns configured then image overrides are disabled + entirely and this field is rejected. + + The rest of the PodSpec - volumes, environment, service account - is + still inherited from the template's target controller, so the overriding + image runs with the same identity and secrets as the workload it is + standing in for. Note also that `imagePullSecrets` are inherited, so an + image from a registry the workload cannot pull from will fail to start. + + This field is immutable; request a new `PodAccessRequest` to change it. + maxLength: 512 + pattern: ^[a-zA-Z0-9][a-zA-Z0-9._:/@+-]*$ + type: string templateName: description: |- Defines the name of the `ExecAcessTemplate` that should be used to grant access to the target diff --git a/examples/pod_access_request.yaml b/examples/pod_access_request.yaml index 461b1dcd..4010f0db 100644 --- a/examples/pod_access_request.yaml +++ b/examples/pod_access_request.yaml @@ -5,3 +5,12 @@ metadata: spec: templateName: deployment-example duration: 5m + + # Optionally run a different image than the one the target workload uses - + # handy when the production image has no shell, or when you need migration + # tooling that is not baked into it. + # + # This only works if the cluster administrator allow-listed the registry via + # the controller's --allowed-image-patterns flag + # (controllerManager.manager.allowedImagePatterns in the Helm chart). + # image: registry.example.com/team/debug:v1 diff --git a/internal/api/v1alpha1/pod_access_request_test.go b/internal/api/v1alpha1/pod_access_request_test.go index a962d146..9165e5d6 100644 --- a/internal/api/v1alpha1/pod_access_request_test.go +++ b/internal/api/v1alpha1/pod_access_request_test.go @@ -19,6 +19,7 @@ import ( "k8s.io/client-go/tools/clientcmd/api" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + "github.com/diranged/oz/internal/imagepolicy" "github.com/diranged/oz/internal/testing/utils" ) @@ -206,6 +207,69 @@ var _ = Describe("PodAccessRequest", Ordered, func() { Expect(err).To(Not(HaveOccurred())) }) + It("Create with a disallowed image override...", func() { + // No policy configured - the default state of the process - so any + // image override at all must be refused. + imagepolicy.SetActive(nil) + + imageRequest := &PodAccessRequest{ + Spec: PodAccessRequestSpec{ + TemplateName: "", + Image: "registry.example.com/team/debug:v1", + }, + } + _, err = imageRequest.ValidateCreate(*admissionRequest) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("image overrides are disabled")) + }) + + It("Create with an allowed image override...", func() { + policy, policyErr := imagepolicy.New([]string{"registry.example.com/team/*"}) + Expect(policyErr).ToNot(HaveOccurred()) + imagepolicy.SetActive(policy) + DeferCleanup(func() { imagepolicy.SetActive(nil) }) + + imageRequest := &PodAccessRequest{ + Spec: PodAccessRequestSpec{ + TemplateName: "", + Image: "registry.example.com/team/debug:v1", + }, + } + _, err = imageRequest.ValidateCreate(*admissionRequest) + Expect(err).ToNot(HaveOccurred()) + + // ... but an image outside the allow-list is still refused. + imageRequest.Spec.Image = "evil.example.com/backdoor:v1" + _, err = imageRequest.ValidateCreate(*admissionRequest) + Expect(err).To(HaveOccurred()) + }) + + It("Update that changes the image is rejected...", func() { + oldRequest := &PodAccessRequest{ + Spec: PodAccessRequestSpec{Image: "registry.example.com/team/debug:v1"}, + } + newRequest := &PodAccessRequest{ + Spec: PodAccessRequestSpec{Image: "registry.example.com/team/other:v2"}, + } + _, err = newRequest.ValidateUpdate(*admissionRequest, oldRequest) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("spec.image is immutable")) + }) + + It("Update that leaves the image alone is allowed...", func() { + oldRequest := &PodAccessRequest{ + Spec: PodAccessRequestSpec{Image: "registry.example.com/team/debug:v1"}, + } + newRequest := &PodAccessRequest{ + Spec: PodAccessRequestSpec{ + Image: "registry.example.com/team/debug:v1", + Duration: "2h", + }, + } + _, err = newRequest.ValidateUpdate(*admissionRequest, oldRequest) + Expect(err).ToNot(HaveOccurred()) + }) + It("Update without UserInfo...", func() { requestBytes, _ := json.Marshal(request) admissionRequest = &admission.Request{ diff --git a/internal/api/v1alpha1/pod_access_request_types.go b/internal/api/v1alpha1/pod_access_request_types.go index b8a654f6..c95ea71f 100644 --- a/internal/api/v1alpha1/pod_access_request_types.go +++ b/internal/api/v1alpha1/pod_access_request_types.go @@ -47,6 +47,30 @@ type PodAccessRequestSpec struct { // +kubebuilder:validation:Optional // +kubebuilder:validation:Pattern="^[0-9]+(s|m|h)$" Duration string `json:"duration,omitempty"` + + // Image optionally overrides the container image used for the "default" + // container of the Pod that Oz launches. This lets a developer run a + // purpose-built debugging, shell or migration image without an + // administrator having to author a dedicated `PodAccessTemplate` for it. + // + // The image must match one of the patterns that the Oz controller was + // deployed with (`--allowed-image-patterns`, set through the + // `controllerManager.manager.allowedImagePatterns` Helm value). If the + // controller has no patterns configured then image overrides are disabled + // entirely and this field is rejected. + // + // The rest of the PodSpec - volumes, environment, service account - is + // still inherited from the template's target controller, so the overriding + // image runs with the same identity and secrets as the workload it is + // standing in for. Note also that `imagePullSecrets` are inherited, so an + // image from a registry the workload cannot pull from will fail to start. + // + // This field is immutable; request a new `PodAccessRequest` to change it. + // + // +kubebuilder:validation:Optional + // +kubebuilder:validation:MaxLength=512 + // +kubebuilder:validation:Pattern="^[a-zA-Z0-9][a-zA-Z0-9._:/@+-]*$" + Image string `json:"image,omitempty"` } // PodAccessRequestStatus defines the observed state of AccessRequest @@ -65,6 +89,7 @@ type PodAccessRequestStatus struct { // +kubebuilder:printcolumn:name="Template",type="string",JSONPath=".spec.templateName",description="Access Template" // +kubebuilder:printcolumn:name="Pod",type="string",JSONPath=".status.podName",description="Target Pod Name" // +kubebuilder:printcolumn:name="Ready",type="boolean",JSONPath=".status.ready",description="Is request ready?" +// +kubebuilder:printcolumn:name="Image",type="string",JSONPath=".spec.image",description="Overridden container image",priority=1 type PodAccessRequest struct { metav1.TypeMeta `json:",inline"` metav1.ObjectMeta `json:"metadata,omitempty"` diff --git a/internal/api/v1alpha1/pod_access_request_webhook.go b/internal/api/v1alpha1/pod_access_request_webhook.go index c50bbf4f..0b03d0de 100644 --- a/internal/api/v1alpha1/pod_access_request_webhook.go +++ b/internal/api/v1alpha1/pod_access_request_webhook.go @@ -17,6 +17,7 @@ limitations under the License. package v1alpha1 import ( + "errors" "fmt" "k8s.io/apimachinery/pkg/runtime" @@ -24,6 +25,7 @@ import ( logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + "github.com/diranged/oz/internal/imagepolicy" "github.com/diranged/oz/internal/webhook" ) @@ -65,6 +67,7 @@ func (r *PodAccessRequest) ValidateCreate(req admission.Request) (admission.Warn if req.UserInfo.Username != "" { podaccessrequestlog.Info( fmt.Sprintf("Create PodAccessRequest from %s", req.UserInfo.Username), + "image", r.Spec.Image, ) } else { // TODO: Make this fail, after we have confidence in the code in a live environment. @@ -72,11 +75,26 @@ func (r *PodAccessRequest) ValidateCreate(req admission.Request) (admission.Warn warnings = append(warnings, w) podaccessrequestlog.Info(w) } + + // Reject an image override that the cluster's policy does not permit. This + // is also enforced when the Pod is built (see the PodAccessBuilder), + // because this webhook is optional - the chart's `webhook.create` setting + // can turn it off. + if err := imagepolicy.Active().Validate(r.Spec.Image); err != nil { + podaccessrequestlog.Info( + "Denied PodAccessRequest image override", + "user", req.UserInfo.Username, + "image", r.Spec.Image, + "reason", err.Error(), + ) + return warnings, fmt.Errorf("spec.image is not allowed: %w", err) + } + return warnings, nil } // ValidateUpdate implements webhook.IContextuallyValidatableObject so a webhook will be registered for the type -func (r *PodAccessRequest) ValidateUpdate(req admission.Request, _ runtime.Object) (admission.Warnings, error) { +func (r *PodAccessRequest) ValidateUpdate(req admission.Request, old runtime.Object) (admission.Warnings, error) { warnings := admission.Warnings{} if req.UserInfo.Username != "" { podaccessrequestlog.Info( @@ -88,6 +106,23 @@ func (r *PodAccessRequest) ValidateUpdate(req admission.Request, _ runtime.Objec warnings = append(warnings, w) podaccessrequestlog.Info(w) } + + // Spec.image is immutable. The Pod is built once, when the request is + // first reconciled, so editing the image afterwards would silently do + // nothing - and worse, would leave the resource claiming to run an image + // that it does not. Reject the edit instead. + if oldRequest, ok := old.(*PodAccessRequest); ok && oldRequest != nil { + if oldRequest.Spec.Image != r.Spec.Image { + return warnings, fmt.Errorf( + "spec.image is immutable (%q -> %q): create a new PodAccessRequest instead", + oldRequest.Spec.Image, + r.Spec.Image, + ) + } + } else if old != nil { + return warnings, errors.New("could not decode the previous PodAccessRequest to validate spec.image") + } + return warnings, nil } diff --git a/internal/api/v1alpha1/pod_spec_mutation_config.go b/internal/api/v1alpha1/pod_spec_mutation_config.go index 9155a636..4c554097 100644 --- a/internal/api/v1alpha1/pod_spec_mutation_config.go +++ b/internal/api/v1alpha1/pod_spec_mutation_config.go @@ -161,6 +161,31 @@ type JSONPatchOperation struct { func (c *PodTemplateSpecMutationConfig) getDefaultContainerID( ctx context.Context, pod corev1.PodTemplateSpec, +) (int, error) { + return GetDefaultContainerID(ctx, pod, c.DefaultContainerName) +} + +// GetDefaultContainerID returns the numerical identifier of the container +// within the PodSpec.Containers[] list that Oz considers the "default" +// container - the one that mutations and overrides apply to. +// +// The name is resolved in order of preference: the explicitly supplied +// name (typically PodTemplateSpecMutationConfig.DefaultContainerName), then the +// well-known DefaultContainerAnnotationKey annotation on the Pod, and finally +// the first container in the list. +// +// This is exported (and takes the name as a parameter rather than reading it +// off a config struct) so that callers which have no +// PodTemplateSpecMutationConfig at all - a PodAccessTemplate is not required to +// define one - can still resolve the same container. +// +// Returns: +// +// int: The identifier in the PodSpec.Containers[] list of the "default" container. +func GetDefaultContainerID( + ctx context.Context, + pod corev1.PodTemplateSpec, + defaultContainerName string, ) (int, error) { logger := log.FromContext(ctx) logger.V(1).Info("Determining \"default\" container ID from PodTemplateSpec...") @@ -170,7 +195,7 @@ func (c *PodTemplateSpecMutationConfig) getDefaultContainerID( // If the user did not supply a DefaultContainerName spec, then try to find // the well known annotation. - if c.DefaultContainerName == "" { + if defaultContainerName == "" { if val, ok := pod.Annotations[DefaultContainerAnnotationKey]; ok { if ok { logger.V(1). @@ -179,8 +204,8 @@ func (c *PodTemplateSpecMutationConfig) getDefaultContainerID( } } } else { - logger.V(1).Info(fmt.Sprintf("Using template-supplied value %s", c.DefaultContainerName)) - defContName = c.DefaultContainerName + logger.V(1).Info(fmt.Sprintf("Using template-supplied value %s", defaultContainerName)) + defContName = defaultContainerName } // At this point, if we didn't find the user supplied value OR the default diff --git a/internal/builders/podaccessbuilder/create_access_resources.go b/internal/builders/podaccessbuilder/create_access_resources.go index 54987ec7..2ddc3a42 100644 --- a/internal/builders/podaccessbuilder/create_access_resources.go +++ b/internal/builders/podaccessbuilder/create_access_resources.go @@ -11,9 +11,12 @@ import ( "github.com/diranged/oz/internal/api/v1alpha1" bldutil "github.com/diranged/oz/internal/builders/utils" + "github.com/diranged/oz/internal/imagepolicy" ) // CreateAccessResources implements the IBuilder interface +// +// revive:disable:cyclomatic A linear sequence of build steps, each with its own error check func (b *PodAccessBuilder) CreateAccessResources( ctx context.Context, client client.Client, @@ -34,6 +37,14 @@ func (b *PodAccessBuilder) CreateAccessResources( return "", err } + // Keep a reference to the unmutated spec. An image override needs it to + // resolve the "default" container the same way PatchPodTemplateSpec() does + // - the mutation config's JSON patches are free to rename containers, so + // resolving the name against the mutated spec could fail to find a + // container that existed when the template author named it. + // PatchPodTemplateSpec() deep-copies its input, so this stays intact. + origPodTemplateSpec := podTemplateSpec + // Run the PodSpec through the optional mutation config mutator := podTmpl.Spec.ControllerTargetMutationConfig if mutator != nil { @@ -44,6 +55,14 @@ func (b *PodAccessBuilder) CreateAccessResources( } } + // Finally, apply the requester's image override, if they asked for one. + // This happens last so that it wins over anything the template's mutation + // config did to the image. + if err := applyImageOverride(ctx, podReq, mutator, origPodTemplateSpec, &podTemplateSpec); err != nil { + log.Error(err, "Failed to apply image override for PodAccessRequest") + return statusString, err + } + // Generate a Pod for the user to access pod, err := bldutil.CreatePod(ctx, client, podReq, podTemplateSpec) if err != nil { @@ -111,3 +130,63 @@ func (b *PodAccessBuilder) CreateAccessResources( ) return statusString, nil } + +// applyImageOverride replaces the image of the "default" container with the one +// requested in Spec.image, after re-checking it against the cluster's image +// policy. +// +// The policy is deliberately re-validated here rather than trusted from the +// admission webhook. The ValidatingWebhookConfiguration is an optional part of +// the deployment (see the chart's `webhook.create` value), so this is the only +// check that is guaranteed to run - and it is the last point before an +// arbitrary image would be handed to the Kubernetes API. +func applyImageOverride( + ctx context.Context, + req *v1alpha1.PodAccessRequest, + mutator *v1alpha1.PodTemplateSpecMutationConfig, + origPodTemplateSpec corev1.PodTemplateSpec, + podTemplateSpec *corev1.PodTemplateSpec, +) error { + if req.Spec.Image == "" { + return nil + } + + log := logf.FromContext(ctx).WithName("applyImageOverride") + + if err := imagepolicy.Active().Validate(req.Spec.Image); err != nil { + return fmt.Errorf("spec.image is not allowed: %w", err) + } + + // A PodAccessTemplate is not required to define a mutation config, so fall + // back to an empty name and let the shared helper resolve the container + // from the well-known annotation or position. + var defaultContainerName string + if mutator != nil { + defaultContainerName = mutator.DefaultContainerName + } + + id, err := v1alpha1.GetDefaultContainerID(ctx, origPodTemplateSpec, defaultContainerName) + if err != nil { + return err + } + + // The container was resolved against the pre-mutation PodSpec, and a + // template's patchSpecOperations are free to add or remove containers, so + // the index is not guaranteed to still be in range. + if id < 0 || id >= len(podTemplateSpec.Spec.Containers) { + return fmt.Errorf( + "cannot apply spec.image: container %d no longer exists after the template's mutations were applied", + id, + ) + } + + log.Info( + "Overriding container image", + "container", podTemplateSpec.Spec.Containers[id].Name, + "from", podTemplateSpec.Spec.Containers[id].Image, + "to", req.Spec.Image, + ) + podTemplateSpec.Spec.Containers[id].Image = req.Spec.Image + + return nil +} diff --git a/internal/builders/podaccessbuilder/create_access_resources_test.go b/internal/builders/podaccessbuilder/create_access_resources_test.go index 33e3869b..aaf6227a 100644 --- a/internal/builders/podaccessbuilder/create_access_resources_test.go +++ b/internal/builders/podaccessbuilder/create_access_resources_test.go @@ -18,6 +18,7 @@ import ( "github.com/diranged/oz/internal/api/v1alpha1" bldutil "github.com/diranged/oz/internal/builders/utils" + "github.com/diranged/oz/internal/imagepolicy" "github.com/diranged/oz/internal/testing/utils" ) @@ -275,6 +276,99 @@ var _ = Describe("RequestReconciler", Ordered, func() { Expect(foundRoleBinding.Subjects[0].Name).To(Equal("testGroupA")) }) + It("CreateAccessResources() should apply an allowed image override", func() { + By("Configuring an image policy for the duration of this test") + policy, err := imagepolicy.New([]string{"registry.example.com/team/*"}) + Expect(err).ToNot(HaveOccurred()) + imagepolicy.SetActive(policy) + DeferCleanup(func() { imagepolicy.SetActive(nil) }) + + By("Creating a request that asks for a different image") + imageRequest := &v1alpha1.PodAccessRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "createaccessresource-image-test", + Namespace: ns.GetName(), + }, + Spec: v1alpha1.PodAccessRequestSpec{ + TemplateName: template.GetName(), + Image: "registry.example.com/team/debug:v1", + }, + } + err = k8sClient.Create(ctx, imageRequest) + Expect(err).ToNot(HaveOccurred()) + + // Execute + _, err = builder.CreateAccessResources(ctx, k8sClient, imageRequest, template) + Expect(err).ToNot(HaveOccurred()) + + // VERIFY: The Pod runs the requested image, and the rest of the + // template's mutation config is still applied. + foundPod := &corev1.Pod{} + err = k8sClient.Get(ctx, types.NamespacedName{ + Name: bldutil.GenerateResourceName(imageRequest), + Namespace: ns.GetName(), + }, foundPod) + Expect(err).ToNot(HaveOccurred()) + Expect(foundPod.Spec.Containers[0].Image).To(Equal("registry.example.com/team/debug:v1")) + Expect(foundPod.Spec.Containers[0].Command[0]).To(Equal("/bin/sleep")) + }) + + It("CreateAccessResources() should reject a disallowed image override", func() { + By("Configuring an image policy that does not cover the request") + policy, err := imagepolicy.New([]string{"registry.example.com/team/*"}) + Expect(err).ToNot(HaveOccurred()) + imagepolicy.SetActive(policy) + DeferCleanup(func() { imagepolicy.SetActive(nil) }) + + denyRequest := &v1alpha1.PodAccessRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "createaccessresource-image-deny-test", + Namespace: ns.GetName(), + }, + Spec: v1alpha1.PodAccessRequestSpec{ + TemplateName: template.GetName(), + Image: "evil.example.com/backdoor:v1", + }, + } + err = k8sClient.Create(ctx, denyRequest) + Expect(err).ToNot(HaveOccurred()) + + // Execute - the builder must refuse even though the object was + // accepted by the API (the validating webhook is optional). + _, err = builder.CreateAccessResources(ctx, k8sClient, denyRequest, template) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("is not permitted on this cluster")) + + // VERIFY: No Pod was created at all. + foundPod := &corev1.Pod{} + err = k8sClient.Get(ctx, types.NamespacedName{ + Name: bldutil.GenerateResourceName(denyRequest), + Namespace: ns.GetName(), + }, foundPod) + Expect(err).To(HaveOccurred()) + }) + + It("CreateAccessResources() should reject an image override when no policy is configured", func() { + imagepolicy.SetActive(nil) + + disabledRequest := &v1alpha1.PodAccessRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "createaccessresource-image-disabled-test", + Namespace: ns.GetName(), + }, + Spec: v1alpha1.PodAccessRequestSpec{ + TemplateName: template.GetName(), + Image: "registry.example.com/team/debug:v1", + }, + } + err := k8sClient.Create(ctx, disabledRequest) + Expect(err).ToNot(HaveOccurred()) + + _, err = builder.CreateAccessResources(ctx, k8sClient, disabledRequest, template) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("image overrides are disabled")) + }) + It("CreateAccessResources() should succeed with Rollout", func() { rolloutRequest.Status.PodName = "" diff --git a/internal/cmd/manager/main.go b/internal/cmd/manager/main.go index 45c89ca0..b92de879 100644 --- a/internal/cmd/manager/main.go +++ b/internal/cmd/manager/main.go @@ -21,6 +21,7 @@ import ( "context" "flag" "os" + "strings" "time" rolloutsv1alpha1 "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" @@ -39,6 +40,7 @@ import ( "github.com/diranged/oz/internal/controllers/podwatcher" "github.com/diranged/oz/internal/controllers/requestcontroller" "github.com/diranged/oz/internal/controllers/templatecontroller" + "github.com/diranged/oz/internal/imagepolicy" "sigs.k8s.io/controller-runtime/pkg/metrics/filters" metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" //+kubebuilder:scaffold:imports @@ -72,6 +74,7 @@ func Main() { var enableLeaderElection bool var requestReconciliationInterval int var templateReconciliationInterval int + var allowedImagePatterns string // Boilerplate flag.StringVar( @@ -109,6 +112,18 @@ func Main() { defaultReconciliationInterval, "Access Template reconciliation interval (in minutes)", ) + flag.StringVar( + &allowedImagePatterns, + "allowed-image-patterns", + "", + // Note: the flag package treats backquotes in a usage string as the + // argument placeholder, so they are deliberately avoided here. + "Comma-separated list of glob patterns for container images that a PodAccessRequest may "+ + "select through its spec.image field. A single star matches within one path segment, "+ + "a double star matches across segments (eg "+ + "\"registry.example.com/team/*,ghcr.io/example/**\"). If empty, image overrides are "+ + "disabled entirely.", + ) // Reconfigure the default logger. Get rid of the JSON log and switch to a LogFmt logger // configLog := uzap.NewProductionEncoderConfig() @@ -129,6 +144,23 @@ func Main() { rootLogger := zap.New(zap.UseFlagOptions(&opts)) ctrl.SetLogger(rootLogger) + // Install the image-override allow-list before anything can serve an + // admission request or reconcile a PodAccessRequest. An invalid pattern is + // fatal rather than ignored - silently dropping a pattern could either + // break access for developers or, far worse, be misread as "configured" + // when nothing is actually being restricted. + policy, err := imagepolicy.New(strings.Split(allowedImagePatterns, ",")) + if err != nil { + setupLog.Error(err, "invalid --allowed-image-patterns") + os.Exit(1) + } + imagepolicy.SetActive(policy) + if policy.Enabled() { + setupLog.Info("PodAccessRequest image overrides enabled", "patterns", policy.Patterns()) + } else { + setupLog.Info("PodAccessRequest image overrides disabled - no --allowed-image-patterns set") + } + mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{ Scheme: scheme, Metrics: metricsserver.Options{ diff --git a/internal/cmd/ozctl/cmd/create_pod_access_request.go b/internal/cmd/ozctl/cmd/create_pod_access_request.go index af681ff6..b513bc27 100644 --- a/internal/cmd/ozctl/cmd/create_pod_access_request.go +++ b/internal/cmd/ozctl/cmd/create_pod_access_request.go @@ -11,6 +11,11 @@ import ( api "github.com/diranged/oz/internal/api/v1alpha1" ) +// Holder for the value of the --image flag. Unlike --duration and --wait, this +// is only meaningful for a PodAccessRequest, so it is not shared with the +// ExecAccessRequest command. +var image = "" + var createPodAccessRequestExample = ` A PodAccessRequest always generates a new Pod for you to do your work in. You simply run: @@ -19,6 +24,12 @@ $ ozctl create PodAccessRequest Success, your access request is ready! Here are your access instructions: kubectl exec -ti -n default user-vd9r9-a217f263 -- /bin/sh + +If your cluster administrator has allow-listed one or more image registries, +you can also run a different image than the one the target workload uses - +handy for a container with debugging or migration tooling baked in: + +$ ozctl create PodAccessRequest --image registry.example.com/team/debug:v1 ` // createPodAccessRequestCmd represents the create command @@ -71,6 +82,7 @@ var createPodAccessRequestCmd = &cobra.Command{ Spec: api.PodAccessRequestSpec{ TemplateName: templateName, Duration: duration, + Image: image, }, } @@ -92,6 +104,8 @@ func init() { StringVarP(&waitTime, "wait", "w", "5m", "Duration to wait for the access request to be fully ready. Valid time units are: ns, us, ms, s, m, h.") createPodAccessRequestCmd.Flags(). StringVarP(&requestNamePrefix, "request-name", "N", usernameEnv, "Prefix name to use when creating the `AccessRequest` objects.") + createPodAccessRequestCmd.Flags(). + StringVarP(&image, "image", "i", "", "Override the container image for the Pod. Must match an image pattern allowed by the cluster administrator; if none are configured, this is rejected.") kubeConfigFlags.AddFlags(createPodAccessRequestCmd.Flags()) diff --git a/internal/imagepolicy/imagepolicy.go b/internal/imagepolicy/imagepolicy.go new file mode 100644 index 00000000..50652eaa --- /dev/null +++ b/internal/imagepolicy/imagepolicy.go @@ -0,0 +1,210 @@ +/* +Copyright 2022 Matt Wise. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// Package imagepolicy implements the operator-wide allow-list that governs +// which container images a PodAccessRequest may run via its `spec.image` +// override. +// +// The allow-list is deliberately a deployment-time setting on the controller +// itself (see the `--allowed-image-patterns` flag) rather than a field on a +// PodAccessTemplate. The set of registries an organization is willing to run +// code from is a cluster-wide security boundary, and letting template authors +// widen it would defeat the purpose of having one. +// +// The zero value of a Policy allows nothing, so an operator that is deployed +// without any configuration rejects every image override. +package imagepolicy + +import ( + "errors" + "fmt" + "regexp" + "strings" + "sync" +) + +const ( + // maxImageLength is an upper bound on the length of an image reference we + // are willing to evaluate. The OCI distribution spec caps a repository + // name at 255 characters; 512 leaves generous room for a registry host, + // a port and a digest without accepting unbounded input. + maxImageLength = 512 +) + +// validImageChars matches the characters that may legally appear in an image +// reference. This is checked before any pattern matching so that neither a +// hostile nor a fat-fingered value can smuggle newlines or shell +// metacharacters into a PodSpec. +var validImageChars = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9._:/@+-]*$`) + +// ErrDisabled is returned when a request asks for an image override but the +// controller has no allow-list patterns configured at all. +var ErrDisabled = errors.New( + "image overrides are disabled on this cluster: the Oz controller was deployed without any --allowed-image-patterns", +) + +// Policy is an immutable, compiled set of allow-list patterns. +type Policy struct { + patterns []string + matchers []*regexp.Regexp +} + +// New compiles a Policy from a list of glob patterns. +// +// Patterns are matched against the image reference exactly as the user wrote +// it - Oz does not normalize a bare `nginx` into `docker.io/library/nginx`. +// Operators should therefore write patterns for the fully-qualified form their +// developers actually use. +// +// Two wildcards are supported: +// +// "*" matches any run of characters except the path separator "/" +// "**" matches any run of characters, including "/" +// +// The distinction matters for security. Under `*`-matches-everything +// semantics, a pattern like `*.dkr.ecr.*.amazonaws.com/team/*` would also +// match `evil.example.com/x.dkr.ecr.us-west-2.amazonaws.com/team/backdoor`, +// because the leading wildcard would happily swallow a registry host that the +// operator never intended to trust. Confining `*` to a single path segment +// closes that hole; `**` remains available when an operator genuinely wants to +// match across segments. +// +// Empty and whitespace-only patterns are ignored, which keeps a Helm value of +// `[]` (rendered as an empty flag) from being mistaken for a real pattern. +func New(patterns []string) (*Policy, error) { + p := &Policy{} + for _, pattern := range patterns { + pattern = strings.TrimSpace(pattern) + if pattern == "" { + continue + } + + matcher, err := compile(pattern) + if err != nil { + return nil, fmt.Errorf("invalid image pattern %q: %w", pattern, err) + } + + p.patterns = append(p.patterns, pattern) + p.matchers = append(p.matchers, matcher) + } + return p, nil +} + +// compile turns a glob pattern into an anchored regular expression. +func compile(pattern string) (*regexp.Regexp, error) { + var b strings.Builder + b.WriteString("^") + for i := 0; i < len(pattern); i++ { + switch { + case pattern[i] == '*' && i+1 < len(pattern) && pattern[i+1] == '*': + // "**" - cross path separators. + b.WriteString(".*") + i++ + case pattern[i] == '*': + // "*" - stay within a single path segment. + b.WriteString("[^/]*") + default: + b.WriteString(regexp.QuoteMeta(string(pattern[i]))) + } + } + b.WriteString("$") + return regexp.Compile(b.String()) +} + +// Enabled reports whether any patterns are configured. When false, every +// non-empty image override is rejected. +func (p *Policy) Enabled() bool { + return p != nil && len(p.matchers) > 0 +} + +// Patterns returns the configured patterns, for logging and error messages. +func (p *Policy) Patterns() []string { + if p == nil { + return nil + } + return append([]string(nil), p.patterns...) +} + +// Validate checks an image reference against the policy. +// +// An empty image is always allowed: it means the caller did not ask for an +// override, and the image inherited from the target controller's PodSpec will +// be used instead. +func (p *Policy) Validate(image string) error { + if image == "" { + return nil + } + + if !p.Enabled() { + return ErrDisabled + } + + if len(image) > maxImageLength { + return fmt.Errorf( + "invalid image reference: must be %d characters or fewer, got %d", + maxImageLength, + len(image), + ) + } + + if !validImageChars.MatchString(image) { + return fmt.Errorf( + "invalid image reference %q: must begin with an alphanumeric character and contain only alphanumerics and the characters . _ : / @ + -", + image, + ) + } + + for _, matcher := range p.matchers { + if matcher.MatchString(image) { + return nil + } + } + + return fmt.Errorf( + "image %q is not permitted on this cluster: it must match one of the allowed patterns %v", + image, + p.patterns, + ) +} + +// The active Policy is process-global state, set once during controller +// startup. A global is used because the admission webhook handlers are +// dispatched on the API type itself (see internal/webhook) and have no +// constructor to inject configuration through. +var ( + activeMu sync.RWMutex + active = &Policy{} +) + +// SetActive installs the Policy used by the admission webhooks and the access +// builders. It is intended to be called once, from the controller's main() +// before the manager starts. +func SetActive(p *Policy) { + activeMu.Lock() + defer activeMu.Unlock() + if p == nil { + p = &Policy{} + } + active = p +} + +// Active returns the Policy installed by SetActive. It never returns nil; the +// default denies every image override. +func Active() *Policy { + activeMu.RLock() + defer activeMu.RUnlock() + return active +} diff --git a/internal/imagepolicy/imagepolicy_test.go b/internal/imagepolicy/imagepolicy_test.go new file mode 100644 index 00000000..c55ab36c --- /dev/null +++ b/internal/imagepolicy/imagepolicy_test.go @@ -0,0 +1,204 @@ +package imagepolicy + +import ( + "errors" + "strings" + "testing" +) + +const ( + ecrPattern = "*.dkr.ecr.*.amazonaws.com/team/*" + ghcrPattern = "ghcr.io/example/*" + matchAll = "**" + unqualified = "nginx:latest" + digestLength = 64 + + // A reference that ecrPattern is expected to allow. + allowedECRImage = "1234567890.dkr.ecr.us-west-2.amazonaws.com/team/web:abc123" +) + +func TestNewIgnoresEmptyPatterns(t *testing.T) { + p, err := New([]string{"", " ", ghcrPattern}) + if err != nil { + t.Fatalf("New() returned an unexpected error: %s", err) + } + if got := p.Patterns(); len(got) != 1 || got[0] != ghcrPattern { + t.Errorf("Patterns() = %v, want [%s]", got, ghcrPattern) + } +} + +func TestNewTreatsRegexMetacharactersAsLiterals(t *testing.T) { + // A stray "(" would be a regex syntax error if it were not escaped. + if _, err := New([]string{"ghcr.io/example(1)/*"}); err != nil { + t.Fatalf("New() should treat regex metacharacters as literals, got: %s", err) + } +} + +func TestDisabledPolicyRejectsEverything(t *testing.T) { + for _, p := range []*Policy{nil, {}, mustNew(t, nil), mustNew(t, []string{""})} { + if p.Enabled() { + t.Errorf("Enabled() = true for a policy with no patterns") + } + if err := p.Validate(""); err != nil { + t.Errorf("Validate(\"\") = %s, want nil - an empty image is not an override", err) + } + if err := p.Validate(unqualified); !errors.Is(err, ErrDisabled) { + t.Errorf("Validate() = %v, want ErrDisabled", err) + } + } +} + +func TestValidate(t *testing.T) { + tests := []struct { + name string + patterns []string + image string + wantErr bool + }{ + { + name: "empty image is always allowed", + patterns: []string{ecrPattern}, + image: "", + }, + { + name: "matching registry and tag", + patterns: []string{ecrPattern}, + image: allowedECRImage, + }, + { + name: "matching registry and digest", + patterns: []string{ecrPattern}, + image: "1234567890.dkr.ecr.us-west-2.amazonaws.com/team/web@sha256:" + + strings.Repeat("a", digestLength), + }, + { + name: "matching registry with no tag", + patterns: []string{ecrPattern}, + image: "1234567890.dkr.ecr.us-west-2.amazonaws.com/team/web", + }, + { + name: "any of several patterns may match", + patterns: []string{ghcrPattern, ecrPattern}, + image: "ghcr.io/example/debug:v1", + }, + { + name: "wrong repository is rejected", + patterns: []string{ecrPattern}, + image: "1234567890.dkr.ecr.us-west-2.amazonaws.com/someone-else/web:abc123", + wantErr: true, + }, + { + name: "wrong registry is rejected", + patterns: []string{ecrPattern}, + image: "docker.io/team/web:abc123", + wantErr: true, + }, + { + name: "unqualified image is rejected", + patterns: []string{ecrPattern}, + image: unqualified, + wantErr: true, + }, + { + // The important one. A leading "*" must not swallow a path + // separator, or an attacker-controlled registry host could be + // prefixed onto an otherwise-trusted-looking reference. + name: "registry prefix smuggling is rejected", + patterns: []string{ecrPattern}, + image: "evil.example.com/" + allowedECRImage, + wantErr: true, + }, + { + // The same attack, but hiding the extra segment in the middle. + name: "extra path segment is rejected by a single-star pattern", + patterns: []string{ecrPattern}, + image: "1234567890.dkr.ecr.us-west-2.amazonaws.com/team/../evil/web:abc123", + wantErr: true, + }, + { + name: "double star crosses path separators", + patterns: []string{"*.dkr.ecr.*.amazonaws.com/team/**"}, + image: "1234567890.dkr.ecr.us-west-2.amazonaws.com/team/sub/web:abc123", + }, + { + name: "double star still anchors the registry host", + patterns: []string{"*.dkr.ecr.*.amazonaws.com/team/**"}, + image: "evil.example.com/x.dkr.ecr.us-west-2.amazonaws.com/team/web:abc123", + wantErr: true, + }, + { + name: "single star does not cross path separators", + patterns: []string{ecrPattern}, + image: "1234567890.dkr.ecr.us-west-2.amazonaws.com/team/sub/web:abc123", + wantErr: true, + }, + { + name: "dots in the pattern are literal", + patterns: []string{ghcrPattern}, + image: "ghcrxio/example/debug:v1", + wantErr: true, + }, + { + name: "newline in the image reference is rejected", + patterns: []string{matchAll}, + image: "ghcr.io/example/debug:v1\nmalicious: true", + wantErr: true, + }, + { + name: "leading dash in the image reference is rejected", + patterns: []string{matchAll}, + image: "-ghcr.io/example/debug:v1", + wantErr: true, + }, + { + name: "over-long image reference is rejected", + patterns: []string{matchAll}, + image: "ghcr.io/example/" + strings.Repeat("a", maxImageLength), + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := mustNew(t, tt.patterns).Validate(tt.image) + if tt.wantErr && err == nil { + t.Errorf("Validate(%q) = nil, want an error", tt.image) + } + if !tt.wantErr && err != nil { + t.Errorf("Validate(%q) = %s, want nil", tt.image, err) + } + }) + } +} + +func TestActiveDefaultsToDenyAll(t *testing.T) { + // Deliberately not calling SetActive() first - this is the state of the + // process before main() configures anything. + if err := Active().Validate(unqualified); !errors.Is(err, ErrDisabled) { + t.Errorf("Active().Validate() = %v, want ErrDisabled", err) + } +} + +func TestSetActive(t *testing.T) { + t.Cleanup(func() { SetActive(nil) }) + + SetActive(mustNew(t, []string{ecrPattern})) + if err := Active().Validate(allowedECRImage); err != nil { + t.Errorf("Active().Validate() = %s, want nil", err) + } + + // A nil Policy must fall back to deny-all rather than panic. + SetActive(nil) + if err := Active().Validate(unqualified); !errors.Is(err, ErrDisabled) { + t.Errorf("Active().Validate() after SetActive(nil) = %v, want ErrDisabled", err) + } +} + +func mustNew(t *testing.T, patterns []string) *Policy { + t.Helper() + p, err := New(patterns) + if err != nil { + t.Fatalf("New(%v) returned an unexpected error: %s", patterns, err) + } + return p +}