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 +}