make authorizing client wrapper check all admin-designated namespaces…

… for SA mappings (akuity#1214)

Signed-off-by: Kent <kent.rancourt@gmail.com>

cmd/controlplane/api.go

@@ -51,12 +51,15 @@ func newAPICommand() *cobra.Command {
 			if err != nil {
 				return errors.Wrap(err, "create internal Kubernetes client")
 			}
-			kubeClient, err := kubernetes.NewClient(ctx, restCfg, kubernetes.ClientOptions{
-				KargoNamespace: cfg.KargoNamespace,
+			kubeClientOptions := kubernetes.ClientOptions{
 				NewInternalClient: func(context.Context, *rest.Config, *runtime.Scheme) (client.Client, error) {
 					return internalClient, nil
 				},
-			})
+			}
+			if cfg.OIDCConfig != nil {
+				kubeClientOptions.GlobalServiceAccountNamespaces = cfg.OIDCConfig.GlobalServiceAccountNamespaces
+			}
+			kubeClient, err := kubernetes.NewClient(ctx, restCfg, kubeClientOptions)
 			if err != nil {
 				return errors.Wrap(err, "create Kubernetes client")
 			}

internal/api/kubernetes/client.go

@@ -31,9 +31,9 @@ import (
 // ClientOptions specifies options for customizing the client returned by the
 // NewClient function.
 type ClientOptions struct {
-	// KargoNamespace is the namespace in which the Kargo components
-	// are running.
-	KargoNamespace string
+	// GlobalServiceAccountNamespaces is a list of namespaces in which we should
+	// always look for ServiceAccounts when attempting to authorize a user.
+	GlobalServiceAccountNamespaces []string
 	// NewInternalClient may be used to take control of how the client's own
 	// internal/underlying controller-runtime client is created. This is mainly
 	// useful for tests wherein one may, for instance, wish to inject a custom
@@ -148,10 +148,10 @@ func NewClient(
 		internalClient: internalClient,
 		statusWriter: &authorizingStatusWriterWrapper{
 			internalClient:        internalClient,
-			getAuthorizedClientFn: getAuthorizedClient(opts.KargoNamespace),
+			getAuthorizedClientFn: getAuthorizedClient(opts.GlobalServiceAccountNamespaces),
 		},
 		internalDynamicClient: internalDynamicClient,
-		getAuthorizedClientFn: getAuthorizedClient(opts.KargoNamespace),
+		getAuthorizedClientFn: getAuthorizedClient(opts.GlobalServiceAccountNamespaces),
 	}, nil
 }
 
@@ -516,7 +516,7 @@ func gvrAndKeyFromObj(
 // found therein to attempt to identify or build an appropriate client for
 // performing the desired operation. If it is unable to do so, it amounts to the
 // operation being unauthorized and an error is returned.
-func getAuthorizedClient(kargoNamespace string) func(
+func getAuthorizedClient(globalServiceAccountNamespaces []string) func(
 	context.Context,
 	libClient.Client,
 	string,
@@ -552,21 +552,29 @@ func getAuthorizedClient(kargoNamespace string) func(
 			Namespace:   key.Namespace,
 			Name:        key.Name,
 		}
+
 		if userInfo.Username != "" {
-			for _, ns := range []string{key.Namespace, kargoNamespace} {
-				if ns == "" {
-					continue
-				}
-				accounts, ok := userInfo.ServiceAccounts[ns]
-				if !ok {
-					continue
+			var namespacesToCheck []string
+			if key.Namespace != "" {
+				namespacesToCheck = make([]string, 0, len(globalServiceAccountNamespaces)+1)
+				namespacesToCheck = append(namespacesToCheck, key.Namespace)
+				namespacesToCheck = append(namespacesToCheck, globalServiceAccountNamespaces...)
+			} else {
+				namespacesToCheck = make([]string, len(userInfo.ServiceAccountsByNamespace))
+				var i int
+				for ns := range userInfo.ServiceAccountsByNamespace {
+					namespacesToCheck[i] = ns
+					i++
 				}
-				for sa := range accounts {
+			}
+			for _, namespaceToCheck := range namespacesToCheck {
+				serviceAccountsToCheck := userInfo.ServiceAccountsByNamespace[namespaceToCheck]
+				for serviceAccountToCheck := range serviceAccountsToCheck {
 					err := reviewSubjectAccess(
 						ctx,
 						internalClient.Scheme(),
 						ra,
-						withServiceAccount(sa),
+						withServiceAccount(serviceAccountToCheck),
 					)
 					if err == nil {
 						return internalClient, nil
@@ -576,29 +584,6 @@ func getAuthorizedClient(kargoNamespace string) func(
 					}
 				}
 			}
-			// If the operation is related to cluster-scoped resources
-			// (e.g. Project(Namespace)), all ServiceAccounts are candidates.
-			if key.Namespace == "" {
-				for ns, accounts := range userInfo.ServiceAccounts {
-					if ns == key.Namespace || ns == kargoNamespace {
-						continue
-					}
-					for sa := range accounts {
-						err := reviewSubjectAccess(
-							ctx,
-							internalClient.Scheme(),
-							ra,
-							withServiceAccount(sa),
-						)
-						if err == nil {
-							return internalClient, nil
-						}
-						if !apierrors.IsForbidden(err) {
-							return nil, errors.Wrap(err, "review subject access")
-						}
-					}
-				}
-			}
 			return nil, newForbiddenError(ra)
 		}
 

internal/api/kubernetes/client_test.go

@@ -471,7 +471,7 @@ func TestGetAuthorizedClient(t *testing.T) {
 			if testCase.userInfo != nil {
 				ctx := user.ContextWithInfo(context.Background(), *testCase.userInfo)
 				testCase.assert(
-					getAuthorizedClient("")(
+					getAuthorizedClient(nil)(
 						ctx,
 						testInternalClient,
 						"", // Verb doesn't matter for these tests

internal/api/option/auth.go

@@ -257,6 +257,7 @@ func (a *authInterceptor) WrapStreamingHandler(
 		return next(ctx, conn)
 	}
 }
+
 func (a *authInterceptor) listServiceAccounts(
 	ctx context.Context,
 	username string,
@@ -272,35 +273,46 @@ func (a *authInterceptor) listServiceAccounts(
 			kubeclient.ServiceAccountsByGroupIndexField: group,
 		})
 	}
+	// allowedNamespaces is a set of all namespaces in which to search for
+	// ServiceAccounts the user may be mapped to. These will includes all project
+	// namespaces and any additional namespaces that the Kargo admin has
+	// designated.
 	allowedNamespaces := make(map[string]struct{})
 	if a.cfg.OIDCConfig != nil {
+		// Add namespaces designated by the Kargo admin to the set.
 		for _, ns := range a.cfg.OIDCConfig.GlobalServiceAccountNamespaces {
 			allowedNamespaces[ns] = struct{}{}
 		}
 	}
+	// Find all project namespaces.
 	nsList := &corev1.NamespaceList{}
 	if err := a.internalClient.List(ctx, nsList, libClient.MatchingLabels{
 		kargoapi.LabelProjectKey: kargoapi.LabelTrueValue,
 	}); err != nil {
 		return nil, errors.Wrap(err, "list namespaces")
 	}
+	// Add all project namespaces to the set.
 	for _, ns := range nsList.Items {
 		allowedNamespaces[ns.GetName()] = struct{}{}
 	}
+	// Now search all identified namespaces for ServiceAccounts that the user may
+	// be mapped to.
 	accounts := make(map[string]map[types.NamespacedName]struct{})
-	for _, q := range queries {
+	for _, query := range queries {
+		// List ALL ServiceAccounts matching the query.
 		list := &corev1.ServiceAccountList{}
-		if err := a.internalClient.List(ctx, list, q); err != nil {
+		if err := a.internalClient.List(ctx, list, query); err != nil {
 			return nil, errors.Wrap(err, "list service accounts")
 		}
 		for _, sa := range list.Items {
+			// Skip if it's not in a namespace we care about.
+			if _, ok := allowedNamespaces[sa.GetNamespace()]; !ok {
+				continue
+			}
 			key := types.NamespacedName{
 				Namespace: sa.GetNamespace(),
 				Name:      sa.GetName(),
 			}
-			if _, ok := allowedNamespaces[key.Namespace]; !ok {
-				continue
-			}
 			if _, ok := accounts[key.Namespace]; !ok {
 				accounts[key.Namespace] = make(map[types.NamespacedName]struct{})
 			}
@@ -382,9 +394,9 @@ func (a *authInterceptor) authenticate(
 			return user.ContextWithInfo(
 				ctx,
 				user.Info{
-					Username:        username,
-					Groups:          groups,
-					ServiceAccounts: sa,
+					Username:                   username,
+					Groups:                     groups,
+					ServiceAccountsByNamespace: sa,
 				},
 			), nil
 		}

internal/api/user/user.go

@@ -29,9 +29,9 @@ type Info struct {
 	// constructing an ad-hoc Kubernetes client, this token will be used directly.
 	// When this is non-empty, all other fields should have an empty value.
 	BearerToken string
-	// ServiceAccounts is the map of the ServiceAccounts that are mapped to the user
-	// by `rbac.kargo.akuity.io` annotations.
-	ServiceAccounts map[string]map[types.NamespacedName]struct{}
+	// ServiceAccountsByNamespace is the mapping of namespace names to sets of
+	// ServiceAccounts that a user has been mapped to.
+	ServiceAccountsByNamespace map[string]map[types.NamespacedName]struct{}
 }
 
 // ContextWithInfo returns a context.Context that has been augmented with