Merge pull request #228 from fluxcd/deny-cross-ns-flag

Allow disabling cross-namespace references to image repositories

api/go.mod

@@ -3,6 +3,7 @@ module github.com/fluxcd/image-reflector-controller/api
 go 1.17
 
 require (
+	github.com/fluxcd/pkg/apis/acl v0.0.3
 	github.com/fluxcd/pkg/apis/meta v0.10.2
 	k8s.io/apimachinery v0.23.1
 	sigs.k8s.io/controller-runtime v0.11.0

api/go.sum

@@ -121,6 +121,8 @@ github.com/evanphx/json-patch v0.5.2/go.mod h1:ZWS5hhDbVDyob71nXKNL0+PWn6ToqBHMi
 github.com/evanphx/json-patch v4.12.0+incompatible/go.mod h1:50XU6AFN0ol/bzJsmQLiYLvXMP4fmwYFNcr97nuDLSk=
 github.com/fatih/color v1.7.0/go.mod h1:Zm6kSWBoL9eyXnKyktHP6abPY2pDugNf5KwzbycvMj4=
 github.com/felixge/httpsnoop v1.0.1/go.mod h1:m8KPJKqk1gH5J9DgRY2ASl2lWCfGKXixSwevea8zH2U=
+github.com/fluxcd/pkg/apis/acl v0.0.3 h1:Lw0ZHdpnO4G7Zy9KjrzwwBmDZQuy4qEjaU/RvA6k1lc=
+github.com/fluxcd/pkg/apis/acl v0.0.3/go.mod h1:XPts6lRJ9C9fIF9xVWofmQwftvhY25n1ps7W9xw0XLU=
 github.com/fluxcd/pkg/apis/meta v0.10.2 h1:pnDBBEvfs4HaKiVAYgz+e/AQ8dLvcgmVfSeBroZ/KKI=
 github.com/fluxcd/pkg/apis/meta v0.10.2/go.mod h1:KQ2er9xa6koy7uoPMZjIjNudB5p4tXs+w0GO6fRcy7I=
 github.com/form3tech-oss/jwt-go v3.2.2+incompatible/go.mod h1:pbq4aXjuKjdthFRnoDwaVPLA+WlJuPGy+QneDUgJi2k=

api/v1beta1/imagerepository_types.go

@@ -21,6 +21,7 @@ import (
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
+	"github.com/fluxcd/pkg/apis/acl"
 	"github.com/fluxcd/pkg/apis/meta"
 )
 
@@ -71,15 +72,7 @@ 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:"namespaceSelectors,omitempty"`
-}
-
-type NamespaceSelector struct {
-	MatchLabels map[string]string `json:"matchLabels,omitempty"`
+	AccessFrom *acl.AccessFrom `json:"accessFrom,omitempty"`
 }
 
 type ScanResult struct {

config/crd/bases/image.toolkit.fluxcd.io_imagerepositories.yaml

@@ -394,14 +394,27 @@ spec:
                   labels.
                 properties:
                   namespaceSelectors:
+                    description: NamespaceSelectors is the list of namespace selectors
+                      to which this ACL applies. Items in this list are evaluated
+                      using a logical OR operation.
                     items:
+                      description: NamespaceSelector selects the namespaces to which
+                        this ACL applies. An empty map of MatchLabels matches all
+                        namespaces in a cluster.
                       properties:
                         matchLabels:
                           additionalProperties:
                             type: string
+                          description: MatchLabels is a map of {key,value} pairs.
+                            A single {key,value} in the matchLabels map is equivalent
+                            to an element of matchExpressions, whose key field is
+                            "key", the operator is "In", and the values array contains
+                            only "value". The requirements are ANDed.
                           type: object
                       type: object
                     type: array
+                required:
+                - namespaceSelectors
                 type: object
               certSecretRef:
                 description: "CertSecretRef can be given the name of a secret containing

controllers/imagepolicy_controller.go

@@ -21,10 +21,8 @@ import (
 	"fmt"
 	"time"
 
-	v1 "k8s.io/api/core/v1"
 	apimeta "k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	kuberecorder "k8s.io/client-go/tools/record"
@@ -36,7 +34,9 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 	"sigs.k8s.io/controller-runtime/pkg/source"
 
+	aclapi "github.com/fluxcd/pkg/apis/acl"
 	"github.com/fluxcd/pkg/apis/meta"
+	"github.com/fluxcd/pkg/runtime/acl"
 	"github.com/fluxcd/pkg/runtime/events"
 	"github.com/fluxcd/pkg/runtime/metrics"
 
@@ -57,6 +57,7 @@ type ImagePolicyReconciler struct {
 	ExternalEventRecorder *events.Recorder
 	MetricsRecorder       *metrics.Recorder
 	Database              DatabaseReader
+	ACLOptions            acl.Options
 }
 
 type ImagePolicyReconcilerOptions struct {
@@ -97,6 +98,23 @@ func (r *ImagePolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 	if pol.Spec.ImageRepositoryRef.Namespace != "" {
 		repoNamespacedName.Namespace = pol.Spec.ImageRepositoryRef.Namespace
 	}
+
+	// check if we're allowed to reference across namespaces, before trying to fetch it
+	if r.ACLOptions.NoCrossNamespaceRefs && repoNamespacedName.Namespace != pol.GetNamespace() {
+		err := fmt.Errorf("cannot access '%s/%s', cross-namespace references have been blocked", imagev1.ImageRepositoryKind, repoNamespacedName)
+		imagev1.SetImagePolicyReadiness(
+			&pol,
+			metav1.ConditionFalse,
+			aclapi.AccessDeniedReason,
+			err.Error(),
+		)
+		if err := r.patchStatus(ctx, req, pol.Status); err != nil {
+			return ctrl.Result{Requeue: true}, err
+		}
+		log.Error(err, "access denied to cross-namespace ImageRepository")
+		return ctrl.Result{}, nil // this cannot proceed until the spec changes, so no need to requeue explicitly
+	}
+
 	if err := r.Get(ctx, repoNamespacedName, &repo); err != nil {
 		if client.IgnoreNotFound(err) == nil {
 			imagev1.SetImagePolicyReadiness(
@@ -115,11 +133,13 @@ func (r *ImagePolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 	}
 
 	// check if we are allowed to use the referenced ImageRepository
-	if _, err := r.hasAccessToRepository(ctx, req, pol.Spec.ImageRepositoryRef, repo.Spec.AccessFrom); err != nil {
+
+	aclAuth := acl.NewAuthorization(r.Client)
+	if err := aclAuth.HasAccessToRef(ctx, &pol, repoNamespacedName, repo.Spec.AccessFrom); err != nil {
 		imagev1.SetImagePolicyReadiness(
 			&pol,
 			metav1.ConditionFalse,
-			"AccessDenied",
+			aclapi.AccessDeniedReason,
 			err.Error(),
 		)
 		if err := r.patchStatus(ctx, req, pol.Status); err != nil {
@@ -327,37 +347,3 @@ func (r *ImagePolicyReconciler) patchStatus(ctx context.Context, req ctrl.Reques
 
 	return r.Status().Patch(ctx, &res, patch)
 }
-
-func (r *ImagePolicyReconciler) hasAccessToRepository(ctx context.Context, policy ctrl.Request, repo meta.NamespacedObjectReference, acl *imagev1.AccessFrom) (bool, error) {
-	// grant access if the policy is in the same namespace as the repository
-	if repo.Namespace == "" || policy.Namespace == repo.Namespace {
-		return true, nil
-	}
-
-	// deny access if the repository has no ACL defined
-	if acl == nil {
-		return false, fmt.Errorf("ImageRepository '%s/%s' can't be accessed due to missing access list",
-			repo.Namespace, repo.Name)
-	}
-
-	// get the policy namespace labels
-	var policyNamespace v1.Namespace
-	if err := r.Get(ctx, types.NamespacedName{Name: policy.Namespace}, &policyNamespace); err != nil {
-		return false, err
-	}
-	policyLabels := policyNamespace.GetLabels()
-
-	// check if the policy namespace labels match any ACL
-	for _, selector := range acl.NamespaceSelectors {
-		sel, err := metav1.LabelSelectorAsSelector(&metav1.LabelSelector{MatchLabels: selector.MatchLabels})
-		if err != nil {
-			return false, err
-		}
-		if sel.Matches(labels.Set(policyLabels)) {
-			return true, nil
-		}
-	}
-
-	return false, fmt.Errorf("ImageRepository '%s/%s' can't be accessed due to labels mismatch on namespace '%s'",
-		repo.Namespace, repo.Name, policy.Namespace)
-}

controllers/policy_test.go

@@ -28,6 +28,8 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 
+	aclapi "github.com/fluxcd/pkg/apis/acl"
+
 	imagev1 "github.com/fluxcd/image-reflector-controller/api/v1beta1"
 	// +kubebuilder:scaffold:imports
 )
@@ -47,6 +49,72 @@ var _ = Describe("ImagePolicy controller", func() {
 		registryServer.Close()
 	})
 
+	When("cross-namespace refs disallowed", func() {
+		BeforeEach(func() {
+			imagePolicyReconciler.ACLOptions.NoCrossNamespaceRefs = true
+		})
+
+		AfterEach(func() {
+			imagePolicyReconciler.ACLOptions.NoCrossNamespaceRefs = false
+		})
+
+		It("fails to reconcile an ImagePolicy with a cross-ns ref", func() {
+			// a bona fide image repo is needed so that it _would_ succeed if not for the disallowed cross-ns ref.
+			versions := []string{"1.0.1", "1.0.2", "1.1.0-alpha"}
+			imgRepo := loadImages(registryServer, "test-semver-policy-"+randStringRunes(5), versions)
+
+			repo := imagev1.ImageRepository{
+				Spec: imagev1.ImageRepositorySpec{
+					Interval: metav1.Duration{Duration: reconciliationInterval},
+					Image:    imgRepo,
+				},
+			}
+			imageObjectName := types.NamespacedName{
+				Name:      "polimage-" + randStringRunes(5),
+				Namespace: "default",
+			}
+			repo.Name = imageObjectName.Name
+			repo.Namespace = imageObjectName.Namespace
+
+			ctx, cancel := context.WithTimeout(context.Background(), contextTimeout)
+			defer cancel()
+			Expect(k8sClient.Create(ctx, &repo)).To(Succeed())
+
+			ns := corev1.Namespace{}
+			ns.Name = "cross-ns-test-" + randStringRunes(5)
+			Expect(k8sClient.Create(ctx, &ns)).To(Succeed())
+
+			imagePolicyName := types.NamespacedName{
+				Namespace: ns.Name,
+				Name:      "policy-test-" + randStringRunes(5),
+			}
+			imagePolicy := imagev1.ImagePolicy{
+				Spec: imagev1.ImagePolicySpec{
+					ImageRepositoryRef: meta.NamespacedObjectReference{
+						Namespace: repo.Namespace,
+						Name:      repo.Name,
+					},
+					Policy: imagev1.ImagePolicyChoice{
+						SemVer: &imagev1.SemVerPolicy{
+							Range: "1.x",
+						},
+					},
+				},
+			}
+			imagePolicy.Namespace = imagePolicyName.Namespace
+			imagePolicy.Name = imagePolicyName.Name
+			Expect(k8sClient.Create(ctx, &imagePolicy)).To(Succeed())
+
+			var pol imagev1.ImagePolicy
+			Eventually(func() bool {
+				err := k8sClient.Get(ctx, imagePolicyName, &pol)
+				return err == nil && apimeta.IsStatusConditionFalse(pol.Status.Conditions, meta.ReadyCondition)
+			}, timeout, interval).Should(BeTrue())
+			ready := apimeta.FindStatusCondition(pol.Status.Conditions, meta.ReadyCondition)
+			Expect(ready.Reason).To(Equal(aclapi.AccessDeniedReason))
+		})
+	})
+
 	Context("Calculates an image from a repository's tags", func() {
 		When("Using SemVerPolicy", func() {
 			It("calculates an image from a repository's tags", func() {
@@ -480,7 +548,7 @@ var _ = Describe("ImagePolicy controller", func() {
 					Spec: imagev1.ImageRepositorySpec{
 						Interval:   metav1.Duration{Duration: reconciliationInterval},
 						Image:      imgRepo,
-						AccessFrom: &imagev1.AccessFrom{},
+						AccessFrom: nil,
 					},
 				}
 				repoObjectName := types.NamespacedName{
@@ -532,7 +600,7 @@ var _ = Describe("ImagePolicy controller", func() {
 					_ = r.Get(ctx, polObjectName, &pol)
 					return apimeta.IsStatusConditionFalse(pol.Status.Conditions, meta.ReadyCondition)
 				}, timeout, interval).Should(BeTrue())
-				Expect(apimeta.FindStatusCondition(pol.Status.Conditions, meta.ReadyCondition).Reason).To(Equal("AccessDenied"))
+				Expect(apimeta.FindStatusCondition(pol.Status.Conditions, meta.ReadyCondition).Reason).To(Equal(aclapi.AccessDeniedReason))
 
 				Expect(r.Delete(ctx, &pol)).To(Succeed())
 			})
@@ -553,8 +621,8 @@ var _ = Describe("ImagePolicy controller", func() {
 					Spec: imagev1.ImageRepositorySpec{
 						Interval: metav1.Duration{Duration: reconciliationInterval},
 						Image:    imgRepo,
-						AccessFrom: &imagev1.AccessFrom{
-							NamespaceSelectors: []imagev1.NamespaceSelector{
+						AccessFrom: &aclapi.AccessFrom{
+							NamespaceSelectors: []aclapi.NamespaceSelector{
 								{
 									MatchLabels: make(map[string]string),
 								},
@@ -636,8 +704,8 @@ var _ = Describe("ImagePolicy controller", func() {
 					Spec: imagev1.ImageRepositorySpec{
 						Interval: metav1.Duration{Duration: reconciliationInterval},
 						Image:    imgRepo,
-						AccessFrom: &imagev1.AccessFrom{
-							NamespaceSelectors: []imagev1.NamespaceSelector{
+						AccessFrom: &aclapi.AccessFrom{
+							NamespaceSelectors: []aclapi.NamespaceSelector{
 								{
 									MatchLabels: policyNamespace.Labels,
 								},
@@ -737,8 +805,8 @@ var _ = Describe("ImagePolicy controller", func() {
 					Spec: imagev1.ImageRepositorySpec{
 						Interval: metav1.Duration{Duration: reconciliationInterval},
 						Image:    imgRepo,
-						AccessFrom: &imagev1.AccessFrom{
-							NamespaceSelectors: []imagev1.NamespaceSelector{
+						AccessFrom: &aclapi.AccessFrom{
+							NamespaceSelectors: []aclapi.NamespaceSelector{
 								{
 									MatchLabels: map[string]string{
 										"tenant": "b",