Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Add ACL support for allowing cross-namespace access to image repository #162

Merged
merged 6 commits into from
Aug 9, 2021

Conversation

stefanprodan
Copy link
Member

@stefanprodan stefanprodan commented Aug 6, 2021

Problem

In the current implementation, users are forced into placing all the image automation objects in the same namespace because the ImagePolicy can refer to an ImageRepository only in the same namespace as itself. This means users have to copy the image pull secrets from their original namespaces into flux-system or copy the Git auth secret in all apps namespaces.

Initial proposal: fluxcd/flux2#582 (comment)
Issues: fluxcd/image-automation-controller#85 fluxcd/image-automation-controller#151

Proposed solution

To allow an ImagePolicy to refer an ImageRepository across the cluster, the ImageRepositoryRef type changes from LocaObjectReference to NamespacedObjectReference.

type ImagePolicySpec struct {
	// ImageRepositoryRef points at the object specifying the image being scanned
	// +required
	ImageRepositoryRef meta.NamespacedObjectReference `json:"imageRepositoryRef"`
}

The ImageRepositoryRef.Namespace is optional, when not specified, the controller behaves like before: it looks for the
ImageRepository in the same namespace as the ImagePolicy.

To grant access to an ImageRepository to policies in other namespaces, the owner of the ImageRepository has to specify a list of label selectors that match the namespaces of the ImagePolicy objects.

type ImageRepositorySpec struct {
	// AccessFrom defines an ACL for allowing cross-namespace references
	// to the ImageRepository object based on the caller's namespace labels.
	// +optional
	AccessFrom *AccessFrom `json:"accessFrom,omitempty"`
}

type AccessFrom struct {
	NamespaceSelectors []NamespaceSelector `json:"namespaceSelector,omitempty"`
}

type NamespaceSelector struct {
	MatchLabels map[string]string `json:"matchLabels,omitempty"`
}

When a policy refers to a repository in a different namespace, the controller checks if the policy namespace labels match any of the selectors defined on the ImageRepository object. If the namespace where the policy resides doesn't have labels or the labels don't match the repository ACL, then the controller sets the policy Ready status to false and the reason to AccessDenied. The access denied error message is set on the Ready condition message and logged before the controller rejects the policy.

Example

Assuming you have an apps namespace where the image pull secrets are. Instead of creating all image automation objects in the apps namespace and copy the Git auth secret from flux-system into apps, you can only define the ImageRepository in apps and grant access to ImagePolicies defined in flux-system namespace:

apiVersion: v1
kind: Namespace
metadata:
  name: apps
---
apiVersion: image.toolkit.fluxcd.io/v1beta1
kind: ImageRepository
metadata:
  name: app1
  namespace: apps
spec:
  interval: 5m
  image: docker.io/org/image
  secretRef:
    name: regcred
  accessFrom:
    namespaceSelectors:
      - matchLabels:
          app.kubernetes.io/instance: flux-system

The ImagePolicies defined in flux-system namespace can now reference the app1 repository from apps namespace:

apiVersion: image.toolkit.fluxcd.io/v1beta1
kind: ImagePolicy
metadata:
  name: app1
  namespace: flux-system
spec:
  imageRepositoryRef:
    name: app1
    namespace: apps
  policy:
    semver:
      range: 1.0.x

If the owner of an ImageRepository wants to grant access to the repo for all namespaces in a cluster, this can be done with an empty matchLabels:

apiVersion: image.toolkit.fluxcd.io/v1beta1
kind: ImageRepository
metadata:
  name: app1
  namespace: apps
spec:
  interval: 5m
  image: docker.io/org/image
  secretRef:
    name: regcred
  accessFrom:
    namespaceSelectors:
      - matchLabels: {}

Feedback & Testing

Please comment on this PR and let us know your thoughts about this feature.

To install the controller with the ACL capability:

  1. install the controllers
flux install --components-extra=image-reflector-controller,image-automation-controller
  1. apply the CRDs from this branch
kubectl apply -k https:/fluxcd/image-reflector-controller/config/crd?ref=image-acl
  1. allow the controller to list namespaces
cat << EOF | kubectl apply -f -
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
  name: crd-controller-list-ns
rules:
- apiGroups:
  - ""
  resources:
  - namespaces
  verbs:
  - get
  - list
  - watch
---
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
  name: crd-controller-list-ns
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: ClusterRole
  name: crd-controller-list-ns
subjects:
  - kind: ServiceAccount
    name: image-reflector-controller
    namespace: flux-system
EOF
  1. deploy image-reflector-controller build of this branch
kubectl -n flux-system set image deployment/image-reflector-controller \
manager=ghcr.io/fluxcd/image-reflector-controller:acl-3f77178a

- add `AccessFrom` to ImageRepositorySpec for granting cross-namespace access to repositories
- change `ImageRepositoryRef` type from local reference to namespaced reference

Signed-off-by: Stefan Prodan <[email protected]>
When a policy refers to a repository in a different namespace, the controller checks if the policy namespace labels match any of the selectors defined on the ImageRepository object. If the namespace where the policy resides, doesn't have labels or the labels don't match the repository ACL, then the controller sets the policy ready status to false and the reason to AccessDenied. The access denied error message is set on the ready condition message and logged before the controller rejects the policy.

Signed-off-by: Stefan Prodan <[email protected]>
@stefanprodan stefanprodan added the enhancement New feature or request label Aug 6, 2021
@squaremo
Copy link
Member

squaremo commented Aug 6, 2021

Is it possible to gain access to an ImageRepository by creating a namespace with the requisite labels? Yes, you would have to know what those are, and yes, you would need the ability to create namespaces -- but this does seem like it's violating the spirit of granting access.

@stefanprodan
Copy link
Member Author

Is it possible to gain access to an ImageRepository by creating a namespace with the requisite labels?

Yes in the same way it's possible to access an app endpoint cross-namespace guarded by a network policy. I should've specify that this proposal is inspired by the Kubernetes NetworkPolicy API.

Yes, you would have to know what those are, and yes, you would need the ability to create namespaces

Given our tenancy approach showcased here https:/fluxcd/flux2-multi-tenancy this is not possible, a tenant can't create its own namespaces nor it can change the labels of an existing namespace. In regards to fluxcd/flux2#582, a tenant has access to namespaced objects (due to the role binding) so it can't alter namespaces unless the cluster admins explicitly grants this permission.

@squaremo
Copy link
Member

squaremo commented Aug 6, 2021

this proposal is inspired by the Kubernetes NetworkPolicy API

Seems to be controversial there, too: kubernetes/kubernetes#88253

There is a decent solution that arose in that issue: let people select namespaces by name. That way, if you care you can at least close that loophole (and if you don't, just select everything). Namespaces have a label with their name, in Kubernetes 1.21.

@stefanprodan
Copy link
Member Author

stefanprodan commented Aug 6, 2021

There is a decent solution that arose in that issue: let people select namespaces by name.

Yes, I don't expect everyone to be on 1.21 but we should document this in the API spec docs and tell people to use the readonly name label if they upgrade.

Copy link
Member

@squaremo squaremo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably much of this can move to fluxcd/pkg/ at some point, since it's pretty general. In the meantime, I think

  • the meaning of no selectors; and,
  • using metav1 for doing the matching
    are worth addressing (either with changes or comments here ..)

controllers/imagepolicy_controller.go Show resolved Hide resolved
controllers/imagepolicy_controller.go Outdated Show resolved Hide resolved
controllers/imagepolicy_controller.go Show resolved Hide resolved
controllers/policy_test.go Outdated Show resolved Hide resolved
controllers/policy_test.go Show resolved Hide resolved
@stefanprodan
Copy link
Member Author

@squaremo I've added the Kubernetes readony label to docs, hopefully people will change their selectors once they upgrade to 1.21.

Copy link
Member

@squaremo squaremo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One typo, and the remaining question over what no selectors means.

docs/spec/v1beta1/imagerepositories.md Show resolved Hide resolved
controllers/imagepolicy_controller.go Outdated Show resolved Hide resolved
controllers/imagepolicy_controller.go Outdated Show resolved Hide resolved
@wavemoran
Copy link

I'm building up some new clusters and migrating our workloads from flux v1 to v2. This route makes more logical sense to me as I can now keep a set of apps, repo secrets, and associated ImagePolicy and ImageUpdateAutomation within the same namespace.

It is also helpful as we restrict engineer teams by namespace. This will help avoid needing access across namespaces or relying on another team to manage flux.

Copy link
Member

@squaremo squaremo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK cool, looks good to me. I'm glad to have an answer for the people who ask after this! Thank you Stefan 🍇

Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looking forward to seeing this translated to a fluxcd/pkg contribution :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants