feat!: limit controller secret access only to project namespaces (#2761)

Signed-off-by: Kent Rancourt <[email protected]>
Co-authored-by: Faeka Ansari <[email protected]>

Tiltfile

@@ -106,6 +106,7 @@ k8s_resource(
     'kargo-controller:serviceaccount',
     'kargo-controller-argocd:clusterrole',
     'kargo-controller-argocd:clusterrolebinding',
+    'kargo-controller-read-secrets:clusterrole',
     'kargo-controller-rollouts:clusterrole',
     'kargo-controller-rollouts:clusterrolebinding'
   ],

charts/kargo/templates/controller/cluster-roles.yaml

@@ -14,6 +14,7 @@ rules:
   verbs:
   - create
   - patch
+{{- if .Values.controller.serviceAccount.clusterWideSecretReadingEnabled }}
 - apiGroups:
   - ""
   resources:
@@ -22,6 +23,7 @@ rules:
   - get
   - list
   - watch
+{{- end }}
 - apiGroups:
   - kargo.akuity.io
   resources:
@@ -68,6 +70,25 @@ rules:
   - warehouses/status
   verbs:
   - patch
+{{- if not .Values.controller.serviceAccount.clusterWideSecretReadingEnabled }}
+---
+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRole
+metadata:
+  name: kargo-controller-read-secrets
+  labels:
+    {{- include "kargo.labels" . | nindent 4 }}
+    {{- include "kargo.controller.labels" . | nindent 4 }}
+rules:
+- apiGroups:
+  - ""
+  resources:
+  - secrets
+  verbs:
+  - get
+  - list
+  - watch
+{{- end }}
 {{- if and .Values.controller.argocd.integrationEnabled (not .Values.controller.argocd.watchArgocdNamespaceOnly) }}
 ---
 apiVersion: rbac.authorization.k8s.io/v1

charts/kargo/templates/management-controller/configmap.yaml

@@ -9,6 +9,9 @@ metadata:
     {{- include "kargo.managementController.labels" . | nindent 4 }}
 data:
   KARGO_NAMESPACE: {{ .Release.Namespace }}
+  {{- if .Values.controller.serviceAccount.clusterWideSecretReadingEnabled }}
+  MANAGE_CONTROLLER_ROLE_BINDINGS: "false"
+  {{- end }}
   LOG_LEVEL: {{ quote .Values.managementController.logLevel }}
   {{- if .Values.kubeconfigSecrets.kargo }}
   KUBECONFIG: /etc/kargo/kubeconfigs/kubeconfig.yaml

charts/kargo/values.yaml

@@ -369,10 +369,12 @@ controller:
   serviceAccount:
     ## @param controller.serviceAccount.iamRole Specifies the ARN of an AWS IAM role to be used by the controller in an IRSA-enabled EKS cluster.
     iamRole: ""
+    ## @param controller.serviceAccount.clusterWideSecretReadingEnabled Specifies whether the controller's ServiceAccount should be granted read permissions to Secrets CLUSTER-WIDE in the Kargo control plane's cluster. Enabling this is highly discouraged and you do so at your own peril. When this is NOT enabled, the Kargo management controller will dynamically expand and contract the controller's permissions to read Secrets on a Project-by-Project basis.
+    clusterWideSecretReadingEnabled: true
 
   ## All settings relating to shared credentials (used across multiple kargo projects)
   globalCredentials:
-    ## @param controller.globalCredentials.namespaces List of namespaces to look for shared credentials.
+    ## @param controller.globalCredentials.namespaces List of namespaces to look for shared credentials. Note that as of v1.0.0, the Kargo controller does not have cluster-wide access to Secrets. The controller receives read-only permission for Secrets on a per-Project basis as Projects are created. If you designate some namespaces as homes for "global" credentials, you will need to manually grant the controller permission to read Secrets in those namespaces.
     namespaces: []
 
   gitClient:

cmd/controlplane/controller.go

@@ -7,7 +7,6 @@ import (
 
 	"github.com/spf13/cobra"
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/cache"
@@ -18,7 +17,6 @@ import (
 	kargoapi "github.com/akuity/kargo/api/v1alpha1"
 	"github.com/akuity/kargo/internal/api/kubernetes"
 	libargocd "github.com/akuity/kargo/internal/argocd"
-	"github.com/akuity/kargo/internal/controller"
 	argocd "github.com/akuity/kargo/internal/controller/argocd/api/v1alpha1"
 	"github.com/akuity/kargo/internal/controller/promotions"
 	rollouts "github.com/akuity/kargo/internal/controller/rollouts/api/v1alpha1"
@@ -172,29 +170,23 @@ func (o *controllerOptions) setupKargoManager(
 		}
 	}
 
-	secretReq, err := controller.GetCredentialsRequirement()
-	if err != nil {
-		return nil, stagesReconcilerCfg, fmt.Errorf("error getting label requirement for credentials Secrets: %w", err)
-	}
-
-	cacheOpts := cache.Options{
-		ByObject: map[client.Object]cache.ByObject{
-			// Only watch Secrets matching the label requirements
-			// for credentials.
-			&corev1.Secret{}: {
-				Label: labels.NewSelector().Add(*secretReq),
-			},
-		},
-	}
-
 	mgr, err := ctrl.NewManager(
 		restCfg,
 		ctrl.Options{
 			Scheme: scheme,
 			Metrics: server.Options{
 				BindAddress: "0",
 			},
-			Cache: cacheOpts,
+			Client: client.Options{
+				Cache: &client.CacheOptions{
+					// The controller does not have cluster-wide permissions, to
+					// get/list/watch Secrets. Its access to Secrets grows and shrinks
+					// dynamically as Projects are created and deleted. We disable caching
+					// here since the underlying informer will not be able to watch
+					// Secrets in all namespaces.
+					DisableFor: []client.Object{&corev1.Secret{}},
+				},
+			},
 		},
 	)
 	return mgr, stagesReconcilerCfg, err

cmd/controlplane/management_controller.go

@@ -9,13 +9,16 @@ import (
 	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/cache"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	"sigs.k8s.io/controller-runtime/pkg/metrics/server"
 
 	kargoapi "github.com/akuity/kargo/api/v1alpha1"
 	"github.com/akuity/kargo/internal/api/kubernetes"
 	"github.com/akuity/kargo/internal/controller/management/namespaces"
 	"github.com/akuity/kargo/internal/controller/management/projects"
+	"github.com/akuity/kargo/internal/controller/management/serviceaccounts"
 	"github.com/akuity/kargo/internal/logging"
 	"github.com/akuity/kargo/internal/os"
 	versionpkg "github.com/akuity/kargo/internal/version"
@@ -78,6 +81,15 @@ func (o *managementControllerOptions) run(ctx context.Context) error {
 		return fmt.Errorf("error setting up Projects reconciler: %w", err)
 	}
 
+	if os.GetEnv("MANAGE_CONTROLLER_ROLE_BINDINGS", "true") == "true" {
+		if err := serviceaccounts.SetupReconcilerWithManager(
+			kargoMgr,
+			serviceaccounts.ReconcilerConfigFromEnv(),
+		); err != nil {
+			return fmt.Errorf("error setting up ServiceAccount reconciler: %w", err)
+		}
+	}
+
 	if err := kargoMgr.Start(ctx); err != nil {
 		return fmt.Errorf("error starting kargo manager: %w", err)
 	}
@@ -118,6 +130,15 @@ func (o *managementControllerOptions) setupManager(ctx context.Context) (manager
 			Metrics: server.Options{
 				BindAddress: "0",
 			},
+			Cache: cache.Options{
+				ByObject: map[client.Object]cache.ByObject{
+					&corev1.ServiceAccount{}: {
+						Namespaces: map[string]cache.Config{
+							os.GetEnv("KARGO_NAMESPACE", "kargo"): {},
+						},
+					},
+				},
+			},
 		},
 	)
 }

internal/controller/labels.go

@@ -9,7 +9,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
 
 	kargoapi "github.com/akuity/kargo/api/v1alpha1"
-	"github.com/akuity/kargo/internal/credentials"
 )
 
 // GetShardPredicate constructs a predicate used as an event filter for various
@@ -63,18 +62,3 @@ func GetShardRequirement(shard string) (*labels.Requirement, error) {
 
 	return req, nil
 }
-
-// GetCredentialsRequirement returns a label requirement that matches only
-// resources that have a credential type label set to one of the supported
-// credential types.
-func GetCredentialsRequirement() (*labels.Requirement, error) {
-	req, err := labels.NewRequirement(kargoapi.CredentialTypeLabelKey, selection.In, []string{
-		credentials.TypeGit.String(),
-		credentials.TypeHelm.String(),
-		credentials.TypeImage.String(),
-	})
-	if err != nil {
-		return nil, fmt.Errorf("error creating credentials label selector: %w", err)
-	}
-	return req, nil
-}

internal/controller/labels_test.go

@@ -5,12 +5,10 @@ import (
 
 	"github.com/stretchr/testify/require"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/labels"
 	"sigs.k8s.io/controller-runtime/pkg/event"
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
 
 	kargoapi "github.com/akuity/kargo/api/v1alpha1"
-	"github.com/akuity/kargo/internal/credentials"
 )
 
 func TestGetShardPredicate(t *testing.T) {
@@ -60,62 +58,3 @@ func TestGetShardPredicate(t *testing.T) {
 		})
 	}
 }
-
-func TestGetCredentialsRequirement(t *testing.T) {
-	testCases := []struct {
-		name    string
-		labels  labels.Set
-		matches bool
-	}{
-		{
-			name: "credential type label set to git",
-			labels: labels.Set{
-				kargoapi.CredentialTypeLabelKey: credentials.TypeGit.String(),
-			},
-			matches: true,
-		},
-		{
-			name: "credential type label set to helm and other labels",
-			labels: labels.Set{
-				kargoapi.CredentialTypeLabelKey: credentials.TypeHelm.String(),
-				"other":                         "label",
-			},
-			matches: true,
-		},
-		{
-			name: "credential type label set to image",
-			labels: labels.Set{
-				kargoapi.CredentialTypeLabelKey: credentials.TypeImage.String(),
-			},
-			matches: true,
-		},
-		{
-			name: "credential type label set to unknown type",
-			labels: labels.Set{
-				kargoapi.CredentialTypeLabelKey: "unknown",
-			},
-			matches: false,
-		},
-		{
-			name: "with other labels but no credential type label",
-			labels: labels.Set{
-				"other":   "label",
-				"another": "label",
-			},
-			matches: false,
-		},
-		{
-			name:    "no labels",
-			labels:  nil,
-			matches: false,
-		},
-	}
-	for _, testCase := range testCases {
-		t.Run(testCase.name, func(t *testing.T) {
-			got, err := GetCredentialsRequirement()
-			require.NoError(t, err)
-			require.NotNil(t, got)
-			require.Equal(t, testCase.matches, got.Matches(testCase.labels))
-		})
-	}
-}