From 3eebbcb33b9d1684d967ea91459bd16235cb6e04 Mon Sep 17 00:00:00 2001 From: Alexy Mantha Date: Thu, 2 Apr 2026 05:48:29 -0400 Subject: [PATCH] feat: use impersonation for server operations (logs, delete, etc) #22996 (#26898) Signed-off-by: Alexy Mantha --- controller/appcontroller.go | 2 +- controller/sync.go | 44 +-- controller/sync_test.go | 39 +-- .../app-sync-using-impersonation.md | 2 +- docs/operator-manual/upgrading/3.4-3.5.md | 40 +++ docs/operator-manual/upgrading/overview.md | 1 + mkdocs.yml | 1 + server/application/application.go | 43 ++- server/application/application_test.go | 126 ++++++++ util/settings/impersonation.go | 50 ++++ util/settings/impersonation_test.go | 268 ++++++++++++++++++ 11 files changed, 542 insertions(+), 74 deletions(-) create mode 100644 docs/operator-manual/upgrading/3.4-3.5.md create mode 100644 util/settings/impersonation.go create mode 100644 util/settings/impersonation_test.go diff --git a/controller/appcontroller.go b/controller/appcontroller.go index 788abd8af6..7bd895995a 100644 --- a/controller/appcontroller.go +++ b/controller/appcontroller.go @@ -2690,7 +2690,7 @@ func (ctrl *ApplicationController) applyImpersonationConfig(config *rest.Config, if !impersonationEnabled { return nil } - user, err := deriveServiceAccountToImpersonate(proj, app, destCluster) + user, err := settings_util.DeriveServiceAccountToImpersonate(proj, app, destCluster) if err != nil { return fmt.Errorf("error deriving service account to impersonate: %w", err) } diff --git a/controller/sync.go b/controller/sync.go index bd182610f9..9afbbe6ba8 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -6,7 +6,6 @@ import ( "fmt" "os" "strconv" - "strings" "time" "k8s.io/apimachinery/pkg/util/strategicpatch" @@ -33,20 +32,16 @@ import ( applog "github.com/argoproj/argo-cd/v3/util/app/log" "github.com/argoproj/argo-cd/v3/util/argo" "github.com/argoproj/argo-cd/v3/util/argo/diff" - "github.com/argoproj/argo-cd/v3/util/glob" kubeutil "github.com/argoproj/argo-cd/v3/util/kube" logutils "github.com/argoproj/argo-cd/v3/util/log" "github.com/argoproj/argo-cd/v3/util/lua" + "github.com/argoproj/argo-cd/v3/util/settings" ) const ( // EnvVarSyncWaveDelay is an environment variable which controls the delay in seconds between // each sync-wave EnvVarSyncWaveDelay = "ARGOCD_SYNC_WAVE_DELAY" - - // serviceAccountDisallowedCharSet contains the characters that are not allowed to be present - // in a DefaultServiceAccount configured for a DestinationServiceAccount - serviceAccountDisallowedCharSet = "!*[]{}\\/" ) func (m *appStateManager) getOpenAPISchema(server *v1alpha1.Cluster) (openapi.Resources, error) { @@ -288,7 +283,7 @@ func (m *appStateManager) SyncAppState(app *v1alpha1.Application, project *v1alp return } if impersonationEnabled { - serviceAccountToImpersonate, err := deriveServiceAccountToImpersonate(project, app, destCluster) + serviceAccountToImpersonate, err := settings.DeriveServiceAccountToImpersonate(project, app, destCluster) if err != nil { state.Phase = common.OperationError state.Message = fmt.Sprintf("failed to find a matching service account to impersonate: %v", err) @@ -558,41 +553,6 @@ func syncWindowPreventsSync(app *v1alpha1.Application, proj *v1alpha1.AppProject return !canSync, nil } -// deriveServiceAccountToImpersonate determines the service account to be used for impersonation for the sync operation. -// The returned service account will be fully qualified including namespace and the service account name in the format system:serviceaccount:: -func deriveServiceAccountToImpersonate(project *v1alpha1.AppProject, application *v1alpha1.Application, destCluster *v1alpha1.Cluster) (string, error) { - // spec.Destination.Namespace is optional. If not specified, use the Application's - // namespace - serviceAccountNamespace := application.Spec.Destination.Namespace - if serviceAccountNamespace == "" { - serviceAccountNamespace = application.Namespace - } - // Loop through the destinationServiceAccounts and see if there is any destination that is a candidate. - // if so, return the service account specified for that destination. - for _, item := range project.Spec.DestinationServiceAccounts { - dstServerMatched, err := glob.MatchWithError(item.Server, destCluster.Server) - if err != nil { - return "", fmt.Errorf("invalid glob pattern for destination server: %w", err) - } - dstNamespaceMatched, err := glob.MatchWithError(item.Namespace, application.Spec.Destination.Namespace) - if err != nil { - return "", fmt.Errorf("invalid glob pattern for destination namespace: %w", err) - } - if dstServerMatched && dstNamespaceMatched { - if strings.Trim(item.DefaultServiceAccount, " ") == "" || strings.ContainsAny(item.DefaultServiceAccount, serviceAccountDisallowedCharSet) { - return "", fmt.Errorf("default service account contains invalid chars '%s'", item.DefaultServiceAccount) - } else if strings.Contains(item.DefaultServiceAccount, ":") { - // service account is specified along with its namespace. - return "system:serviceaccount:" + item.DefaultServiceAccount, nil - } - // service account needs to be prefixed with a namespace - return fmt.Sprintf("system:serviceaccount:%s:%s", serviceAccountNamespace, item.DefaultServiceAccount), nil - } - } - // if there is no match found in the AppProject.Spec.DestinationServiceAccounts, use the default service account of the destination namespace. - return "", fmt.Errorf("no matching service account found for destination server %s and namespace %s", application.Spec.Destination.Server, serviceAccountNamespace) -} - // validateSyncPermissions checks whether the given resource is permitted by the project's // allow/deny lists and destination rules. It returns an error if the API resource info is nil // (preventing a nil-pointer panic), if the resource's group/kind is not permitted, or if diff --git a/controller/sync_test.go b/controller/sync_test.go index 5079c997e8..4a450eb80a 100644 --- a/controller/sync_test.go +++ b/controller/sync_test.go @@ -22,6 +22,7 @@ import ( "github.com/argoproj/argo-cd/v3/test" "github.com/argoproj/argo-cd/v3/util/argo/diff" "github.com/argoproj/argo-cd/v3/util/argo/normalizers" + "github.com/argoproj/argo-cd/v3/util/settings" ) func TestPersistRevisionHistory(t *testing.T) { @@ -726,7 +727,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when - sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster) + sa, err := settings.DeriveServiceAccountToImpersonate(f.project, f.application, f.cluster) assert.Equal(t, expectedSA, sa) // then, there should be an error saying no valid match was found @@ -750,7 +751,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when - sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster) + sa, err := settings.DeriveServiceAccountToImpersonate(f.project, f.application, f.cluster) // then, there should be no error and should use the right service account for impersonation require.NoError(t, err) @@ -789,7 +790,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when - sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster) + sa, err := settings.DeriveServiceAccountToImpersonate(f.project, f.application, f.cluster) // then, there should be no error and should use the right service account for impersonation require.NoError(t, err) @@ -828,7 +829,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when - sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster) + sa, err := settings.DeriveServiceAccountToImpersonate(f.project, f.application, f.cluster) // then, there should be no error and it should use the first matching service account for impersonation require.NoError(t, err) @@ -862,7 +863,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when - sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster) + sa, err := settings.DeriveServiceAccountToImpersonate(f.project, f.application, f.cluster) // then, there should not be any error and should use the first matching glob pattern service account for impersonation require.NoError(t, err) @@ -897,7 +898,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when - sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster) + sa, err := settings.DeriveServiceAccountToImpersonate(f.project, f.application, f.cluster) // then, there should be an error saying no match was found require.EqualError(t, err, expectedErrMsg) @@ -925,7 +926,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when - sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster) + sa, err := settings.DeriveServiceAccountToImpersonate(f.project, f.application, f.cluster) // then, there should not be any error and the service account configured for with empty namespace should be used. require.NoError(t, err) @@ -959,7 +960,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when - sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster) + sa, err := settings.DeriveServiceAccountToImpersonate(f.project, f.application, f.cluster) // then, there should not be any error and the catch all service account should be returned require.NoError(t, err) @@ -983,7 +984,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when - sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster) + sa, err := settings.DeriveServiceAccountToImpersonate(f.project, f.application, f.cluster) // then, there must be an error as the glob pattern is invalid. require.ErrorContains(t, err, "invalid glob pattern for destination namespace") @@ -1017,7 +1018,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when - sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster) + sa, err := settings.DeriveServiceAccountToImpersonate(f.project, f.application, f.cluster) assert.Equal(t, expectedSA, sa) // then, there should not be any error and the service account with its namespace should be returned. @@ -1045,7 +1046,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { f.application.Spec.Destination.Name = f.cluster.Name // when - sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster) + sa, err := settings.DeriveServiceAccountToImpersonate(f.project, f.application, f.cluster) assert.Equal(t, expectedSA, sa) // then, there should not be any error and the service account with its namespace should be returned. @@ -1128,7 +1129,7 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when - sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster) + sa, err := settings.DeriveServiceAccountToImpersonate(f.project, f.application, f.cluster) // then, there should not be any error and the right service account must be returned. require.NoError(t, err) @@ -1167,7 +1168,7 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when - sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster) + sa, err := settings.DeriveServiceAccountToImpersonate(f.project, f.application, f.cluster) // then, there should not be any error and first matching service account should be used require.NoError(t, err) @@ -1201,7 +1202,7 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when - sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster) + sa, err := settings.DeriveServiceAccountToImpersonate(f.project, f.application, f.cluster) assert.Equal(t, expectedSA, sa) // then, there should not be any error and the service account of the glob pattern, being the first match should be returned. @@ -1236,7 +1237,7 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when - sa, err := deriveServiceAccountToImpersonate(f.project, f.application, &v1alpha1.Cluster{Server: destinationServerURL}) + sa, err := settings.DeriveServiceAccountToImpersonate(f.project, f.application, &v1alpha1.Cluster{Server: destinationServerURL}) // then, there an error with appropriate message must be returned require.EqualError(t, err, expectedErr) @@ -1270,7 +1271,7 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when - sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster) + sa, err := settings.DeriveServiceAccountToImpersonate(f.project, f.application, f.cluster) // then, there should not be any error and the service account of the glob pattern match must be returned. require.NoError(t, err) @@ -1294,7 +1295,7 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when - sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster) + sa, err := settings.DeriveServiceAccountToImpersonate(f.project, f.application, f.cluster) // then, there must be an error as the glob pattern is invalid. require.ErrorContains(t, err, "invalid glob pattern for destination server") @@ -1328,7 +1329,7 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when - sa, err := deriveServiceAccountToImpersonate(f.project, f.application, &v1alpha1.Cluster{Server: destinationServerURL}) + sa, err := settings.DeriveServiceAccountToImpersonate(f.project, f.application, &v1alpha1.Cluster{Server: destinationServerURL}) // then, there should not be any error and the service account with the given namespace prefix must be returned. require.NoError(t, err) @@ -1356,7 +1357,7 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) { f.application.Spec.Destination.Name = f.cluster.Name // when - sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster) + sa, err := settings.DeriveServiceAccountToImpersonate(f.project, f.application, f.cluster) assert.Equal(t, expectedSA, sa) // then, there should not be any error and the service account with its namespace should be returned. diff --git a/docs/operator-manual/app-sync-using-impersonation.md b/docs/operator-manual/app-sync-using-impersonation.md index f2aa4f7c89..1f86f6023e 100644 --- a/docs/operator-manual/app-sync-using-impersonation.md +++ b/docs/operator-manual/app-sync-using-impersonation.md @@ -30,7 +30,7 @@ Impersonation requests first authenticate as the requesting user, then switch to ### Feature scope -Impersonation is currently only supported for the lifecycle of objects managed by an Application directly, which includes sync operations (creation, update and pruning of resources) and deletion as part of Application finalizer logic. This *does not* includes operations triggered via ArgoCD's UI, which will still be executed with Argo CD's control-plane service account. +Impersonation is supported for the lifecycle of objects managed by an Application directly, which includes sync operations (creation, update and pruning of resources) and deletion as part of Application finalizer logic. It is also supported for UI operations triggered by the user. ## Prerequisites diff --git a/docs/operator-manual/upgrading/3.4-3.5.md b/docs/operator-manual/upgrading/3.4-3.5.md new file mode 100644 index 0000000000..d56a00b5ca --- /dev/null +++ b/docs/operator-manual/upgrading/3.4-3.5.md @@ -0,0 +1,40 @@ +# v3.4 to 3.5 + +## Breaking Changes + +## Behavioral Improvements / Fixes + +### Impersonation extended to server operations + +When [impersonation](../app-sync-using-impersonation.md) is enabled, it now applies to all API server operations, not just sync operations. This means that actions triggered through the UI or API (viewing logs, listing events, deleting resources, running resource actions, etc.) will use the impersonated service account derived from the AppProject's `destinationServiceAccounts` configuration. + +Previously, impersonation only applied to sync operations. + +**Affected operations and required permissions:** + +| Operation | Kubernetes API call | Required RBAC verbs | +|---|---|---| +| Get resource | `GET` on the target resource | `get` | +| Patch resource | `PATCH` on the target resource | `get`, `patch` | +| Delete resource | `DELETE` on the target resource | `delete` | +| List resource events | `LIST` on `events` (core/v1) | `list` | +| View pod logs | `GET` on `pods` and `pods/log` | `get` | +| Run resource action | `GET`, `CREATE`, `PATCH` on the target resource | `get`, `create`, `patch` | + +This list covers built-in operations. Custom resource actions may require additional permissions depending on what Kubernetes API calls they make. + +Users with impersonation enabled must ensure the service accounts configured in `destinationServiceAccounts` have permissions for these operations. + +No action is required for users who do not have impersonation enabled. + +## API Changes + +## Security Changes + +## Deprecated Items + +## Kustomize Upgraded + +## Helm Upgraded + +## Custom Healthchecks Added diff --git a/docs/operator-manual/upgrading/overview.md b/docs/operator-manual/upgrading/overview.md index cdd92063de..0a8eb6f126 100644 --- a/docs/operator-manual/upgrading/overview.md +++ b/docs/operator-manual/upgrading/overview.md @@ -39,6 +39,7 @@ kubectl apply -n argocd --server-side --force-conflicts -f https://raw.githubuse
+- [v3.4 to v3.5](./3.4-3.5.md) - [v3.3 to v3.4](./3.3-3.4.md) - [v3.2 to v3.3](./3.2-3.3.md) - [v3.1 to v3.2](./3.1-3.2.md) diff --git a/mkdocs.yml b/mkdocs.yml index 0e99b9a713..09ca532a24 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -136,6 +136,7 @@ nav: - operator-manual/server-commands/additional-configuration-method.md - Upgrading: - operator-manual/upgrading/overview.md + - operator-manual/upgrading/3.4-3.5.md - operator-manual/upgrading/3.3-3.4.md - operator-manual/upgrading/3.2-3.3.md - operator-manual/upgrading/3.1-3.2.md diff --git a/server/application/application.go b/server/application/application.go index c373cb4698..7029a57bd8 100644 --- a/server/application/application.go +++ b/server/application/application.go @@ -508,7 +508,7 @@ func (s *Server) GetManifests(ctx context.Context, q *application.ApplicationMan return fmt.Errorf("error getting app instance label key from settings: %w", err) } - config, err := s.getApplicationClusterConfig(ctx, a) + config, err := s.getApplicationClusterConfig(ctx, a, proj) if err != nil { return fmt.Errorf("error getting application cluster config: %w", err) } @@ -670,7 +670,7 @@ func (s *Server) GetManifestsWithFiles(stream application.ApplicationService_Get return fmt.Errorf("error getting trackingMethod from settings: %w", err) } - config, err := s.getApplicationClusterConfig(ctx, a) + config, err := s.getApplicationClusterConfig(ctx, a, proj) if err != nil { return fmt.Errorf("error getting application cluster config: %w", err) } @@ -879,7 +879,7 @@ func (s *Server) Get(ctx context.Context, q *application.ApplicationQuery) (*v1a // ListResourceEvents returns a list of event resources func (s *Server) ListResourceEvents(ctx context.Context, q *application.ApplicationResourceEventsQuery) (*corev1.EventList, error) { - a, _, err := s.getApplicationEnforceRBACInformer(ctx, rbac.ActionGet, q.GetProject(), q.GetAppNamespace(), q.GetName()) + a, p, err := s.getApplicationEnforceRBACInformer(ctx, rbac.ActionGet, q.GetProject(), q.GetAppNamespace(), q.GetName()) if err != nil { return nil, err } @@ -918,7 +918,7 @@ func (s *Server) ListResourceEvents(ctx context.Context, q *application.Applicat namespace = q.GetResourceNamespace() var config *rest.Config - config, err = s.getApplicationClusterConfig(ctx, a) + config, err = s.getApplicationClusterConfig(ctx, a, p) if err != nil { return nil, fmt.Errorf("error getting application cluster config: %w", err) } @@ -1377,7 +1377,7 @@ func (s *Server) validateAndNormalizeApp(ctx context.Context, app *v1alpha1.Appl return nil } -func (s *Server) getApplicationClusterConfig(ctx context.Context, a *v1alpha1.Application) (*rest.Config, error) { +func (s *Server) getApplicationClusterConfig(ctx context.Context, a *v1alpha1.Application, p *v1alpha1.AppProject) (*rest.Config, error) { cluster, err := argo.GetDestinationCluster(ctx, a.Spec.Destination, s.db) if err != nil { return nil, fmt.Errorf("error validating destination: %w", err) @@ -1387,6 +1387,24 @@ func (s *Server) getApplicationClusterConfig(ctx context.Context, a *v1alpha1.Ap return nil, fmt.Errorf("error getting cluster REST config: %w", err) } + impersonationEnabled, err := s.settingsMgr.IsImpersonationEnabled() + if err != nil { + return nil, fmt.Errorf("error getting impersonation setting: %w", err) + } + + if !impersonationEnabled { + return config, nil + } + + user, err := settings.DeriveServiceAccountToImpersonate(p, a, cluster) + if err != nil { + return nil, fmt.Errorf("error deriving service account to impersonate: %w", err) + } + + config.Impersonate = rest.ImpersonationConfig{ + UserName: user, + } + return config, err } @@ -1437,7 +1455,7 @@ func (s *Server) getAppLiveResource(ctx context.Context, action string, q *appli if fineGrainedInheritanceDisabled && (action == rbac.ActionDelete || action == rbac.ActionUpdate) { action = fmt.Sprintf("%s/%s/%s/%s/%s", action, q.GetGroup(), q.GetKind(), q.GetNamespace(), q.GetResourceName()) } - a, _, err := s.getApplicationEnforceRBACInformer(ctx, action, q.GetProject(), q.GetAppNamespace(), q.GetName()) + a, p, err := s.getApplicationEnforceRBACInformer(ctx, action, q.GetProject(), q.GetAppNamespace(), q.GetName()) if !fineGrainedInheritanceDisabled && err != nil && errors.Is(err, argocommon.PermissionDeniedAPIError) && (action == rbac.ActionDelete || action == rbac.ActionUpdate) { action = fmt.Sprintf("%s/%s/%s/%s/%s", action, q.GetGroup(), q.GetKind(), q.GetNamespace(), q.GetResourceName()) a, _, err = s.getApplicationEnforceRBACInformer(ctx, action, q.GetProject(), q.GetAppNamespace(), q.GetName()) @@ -1455,10 +1473,11 @@ func (s *Server) getAppLiveResource(ctx context.Context, action string, q *appli if found == nil || found.UID == "" { return nil, nil, nil, status.Errorf(codes.InvalidArgument, "%s %s %s not found as part of application %s", q.GetKind(), q.GetGroup(), q.GetResourceName(), q.GetName()) } - config, err := s.getApplicationClusterConfig(ctx, a) + config, err := s.getApplicationClusterConfig(ctx, a, p) if err != nil { return nil, nil, nil, fmt.Errorf("error getting application cluster config: %w", err) } + return found, config, a, nil } @@ -1571,6 +1590,7 @@ func (s *Server) DeleteResource(ctx context.Context, q *application.ApplicationR propagationPolicy := metav1.DeletePropagationForeground deleteOption = metav1.DeleteOptions{PropagationPolicy: &propagationPolicy} } + err = s.kubectl.DeleteResource(ctx, config, res.GroupKindVersion(), res.Name, res.Namespace, deleteOption) if err != nil { return nil, fmt.Errorf("error deleting resource: %w", err) @@ -1826,7 +1846,7 @@ func (s *Server) PodLogs(q *application.ApplicationPodLogsQuery, ws application. } } - a, _, err := s.getApplicationEnforceRBACInformer(ws.Context(), rbac.ActionGet, q.GetProject(), q.GetAppNamespace(), q.GetName()) + a, p, err := s.getApplicationEnforceRBACInformer(ws.Context(), rbac.ActionGet, q.GetProject(), q.GetAppNamespace(), q.GetName()) if err != nil { return err } @@ -1840,7 +1860,7 @@ func (s *Server) PodLogs(q *application.ApplicationPodLogsQuery, ws application. return fmt.Errorf("error getting app resource tree: %w", err) } - config, err := s.getApplicationClusterConfig(ws.Context(), a) + config, err := s.getApplicationClusterConfig(ws.Context(), a, p) if err != nil { return fmt.Errorf("error getting application cluster config: %w", err) } @@ -2515,7 +2535,8 @@ func (s *Server) ListResourceActions(ctx context.Context, q *application.Applica func (s *Server) getUnstructuredLiveResourceOrApp(ctx context.Context, rbacRequest string, q *application.ApplicationResourceRequest) (obj *unstructured.Unstructured, res *v1alpha1.ResourceNode, app *v1alpha1.Application, config *rest.Config, err error) { if q.GetKind() == applicationType.ApplicationKind && q.GetGroup() == applicationType.Group && q.GetName() == q.GetResourceName() { - app, _, err = s.getApplicationEnforceRBACInformer(ctx, rbacRequest, q.GetProject(), q.GetAppNamespace(), q.GetName()) + var p *v1alpha1.AppProject + app, p, err = s.getApplicationEnforceRBACInformer(ctx, rbacRequest, q.GetProject(), q.GetAppNamespace(), q.GetName()) if err != nil { return nil, nil, nil, nil, err } @@ -2523,7 +2544,7 @@ func (s *Server) getUnstructuredLiveResourceOrApp(ctx context.Context, rbacReque if err != nil { return nil, nil, nil, nil, err } - config, err = s.getApplicationClusterConfig(ctx, app) + config, err = s.getApplicationClusterConfig(ctx, app, p) if err != nil { return nil, nil, nil, nil, fmt.Errorf("error getting application cluster config: %w", err) } diff --git a/server/application/application_test.go b/server/application/application_test.go index 2dcd4ae077..82c7033625 100644 --- a/server/application/application_test.go +++ b/server/application/application_test.go @@ -4644,3 +4644,129 @@ func TestTerminateOperationWithConflicts(t *testing.T) { require.NoError(t, err) assert.GreaterOrEqual(t, updateCallCount, 2, "Update should be called at least twice (once with conflict, once with success)") } + +func TestGetApplicationClusterConfig(t *testing.T) { + t.Run("ImpersonationDisabled", func(t *testing.T) { + app := newTestApp() + appServer := newTestAppServer(t, app) + + project := &v1alpha1.AppProject{ + ObjectMeta: metav1.ObjectMeta{Name: "default", Namespace: "default"}, + Spec: v1alpha1.AppProjectSpec{ + SourceRepos: []string{"*"}, + Destinations: []v1alpha1.ApplicationDestination{{Server: "*", Namespace: "*"}}, + }, + } + + config, err := appServer.getApplicationClusterConfig(t.Context(), app, project) + require.NoError(t, err) + assert.Empty(t, config.Impersonate.UserName) + }) + + t.Run("ImpersonationEnabledWithMatch", func(t *testing.T) { + f := func(enf *rbac.Enforcer) { + _ = enf.SetBuiltinPolicy(assets.BuiltinPolicyCSV) + enf.SetDefaultRole("role:admin") + } + + projWithSA := &v1alpha1.AppProject{ + ObjectMeta: metav1.ObjectMeta{Name: "proj-impersonate", Namespace: "default"}, + Spec: v1alpha1.AppProjectSpec{ + SourceRepos: []string{"*"}, + Destinations: []v1alpha1.ApplicationDestination{{Server: "*", Namespace: "*"}}, + DestinationServiceAccounts: []v1alpha1.ApplicationDestinationServiceAccount{ + { + Server: "https://cluster-api.example.com", + Namespace: test.FakeDestNamespace, + DefaultServiceAccount: "test-sa", + }, + }, + }, + } + + app := newTestApp(func(a *v1alpha1.Application) { + a.Spec.Project = "proj-impersonate" + }) + + appServer := newTestAppServerWithEnforcerConfigure(t, f, + map[string]string{"application.sync.impersonation.enabled": "true"}, + app, projWithSA, + ) + + config, err := appServer.getApplicationClusterConfig(t.Context(), app, projWithSA) + require.NoError(t, err) + assert.Equal(t, "system:serviceaccount:"+test.FakeDestNamespace+":test-sa", config.Impersonate.UserName) + }) + + t.Run("ImpersonationEnabledWithNoMatch", func(t *testing.T) { + f := func(enf *rbac.Enforcer) { + _ = enf.SetBuiltinPolicy(assets.BuiltinPolicyCSV) + enf.SetDefaultRole("role:admin") + } + + app := newTestApp() + appServer := newTestAppServerWithEnforcerConfigure(t, f, + map[string]string{"application.sync.impersonation.enabled": "true"}, + app, + ) + + // "default" project has no DestinationServiceAccounts + project := &v1alpha1.AppProject{ + ObjectMeta: metav1.ObjectMeta{Name: "default", Namespace: "default"}, + Spec: v1alpha1.AppProjectSpec{ + SourceRepos: []string{"*"}, + Destinations: []v1alpha1.ApplicationDestination{{Server: "*", Namespace: "*"}}, + }, + } + + config, err := appServer.getApplicationClusterConfig(t.Context(), app, project) + assert.Nil(t, config) + assert.ErrorContains(t, err, "no matching service account found") + }) +} + +func TestGetUnstructuredLiveResourceOrAppWithImpersonation(t *testing.T) { + f := func(enf *rbac.Enforcer) { + _ = enf.SetBuiltinPolicy(assets.BuiltinPolicyCSV) + enf.SetDefaultRole("role:admin") + } + + projWithSA := &v1alpha1.AppProject{ + ObjectMeta: metav1.ObjectMeta{Name: "proj-impersonate", Namespace: "default"}, + Spec: v1alpha1.AppProjectSpec{ + SourceRepos: []string{"*"}, + Destinations: []v1alpha1.ApplicationDestination{{Server: "*", Namespace: "*"}}, + DestinationServiceAccounts: []v1alpha1.ApplicationDestinationServiceAccount{ + { + Server: "https://cluster-api.example.com", + Namespace: test.FakeDestNamespace, + DefaultServiceAccount: "test-sa", + }, + }, + }, + } + + app := newTestApp(func(a *v1alpha1.Application) { + a.Spec.Project = "proj-impersonate" + }) + + appServer := newTestAppServerWithEnforcerConfigure(t, f, + map[string]string{"application.sync.impersonation.enabled": "true"}, + app, projWithSA, + ) + + appName := app.Name + group := "argoproj.io" + kind := "Application" + project := "proj-impersonate" + + _, _, _, config, err := appServer.getUnstructuredLiveResourceOrApp(t.Context(), rbac.ActionGet, &application.ApplicationResourceRequest{ + Name: &appName, + ResourceName: &appName, + Group: &group, + Kind: &kind, + Project: &project, + }) + require.NoError(t, err) + assert.Equal(t, "system:serviceaccount:"+test.FakeDestNamespace+":test-sa", config.Impersonate.UserName) +} diff --git a/util/settings/impersonation.go b/util/settings/impersonation.go new file mode 100644 index 0000000000..2310601d8a --- /dev/null +++ b/util/settings/impersonation.go @@ -0,0 +1,50 @@ +package settings + +import ( + "fmt" + "strings" + + "github.com/argoproj/argo-cd/v3/pkg/apis/application/v1alpha1" + "github.com/argoproj/argo-cd/v3/util/glob" +) + +const ( + // serviceAccountDisallowedCharSet contains the characters that are not allowed to be present + // in a DefaultServiceAccount configured for a DestinationServiceAccount + serviceAccountDisallowedCharSet = "!*[]{}\\/" +) + +// DeriveServiceAccountToImpersonate determines the service account to be used for impersonation for the sync operation. +// The returned service account will be fully qualified including namespace and the service account name in the format system:serviceaccount:: +func DeriveServiceAccountToImpersonate(project *v1alpha1.AppProject, application *v1alpha1.Application, destCluster *v1alpha1.Cluster) (string, error) { + // spec.Destination.Namespace is optional. If not specified, use the Application's + // namespace + serviceAccountNamespace := application.Spec.Destination.Namespace + if serviceAccountNamespace == "" { + serviceAccountNamespace = application.Namespace + } + // Loop through the destinationServiceAccounts and see if there is any destination that is a candidate. + // if so, return the service account specified for that destination. + for _, item := range project.Spec.DestinationServiceAccounts { + dstServerMatched, err := glob.MatchWithError(item.Server, destCluster.Server) + if err != nil { + return "", fmt.Errorf("invalid glob pattern for destination server: %w", err) + } + dstNamespaceMatched, err := glob.MatchWithError(item.Namespace, application.Spec.Destination.Namespace) + if err != nil { + return "", fmt.Errorf("invalid glob pattern for destination namespace: %w", err) + } + if dstServerMatched && dstNamespaceMatched { + if strings.Trim(item.DefaultServiceAccount, " ") == "" || strings.ContainsAny(item.DefaultServiceAccount, serviceAccountDisallowedCharSet) { + return "", fmt.Errorf("default service account contains invalid chars '%s'", item.DefaultServiceAccount) + } else if strings.Contains(item.DefaultServiceAccount, ":") { + // service account is specified along with its namespace. + return "system:serviceaccount:" + item.DefaultServiceAccount, nil + } + // service account needs to be prefixed with a namespace + return fmt.Sprintf("system:serviceaccount:%s:%s", serviceAccountNamespace, item.DefaultServiceAccount), nil + } + } + // if there is no match found in the AppProject.Spec.DestinationServiceAccounts, use the default service account of the destination namespace. + return "", fmt.Errorf("no matching service account found for destination server %s and namespace %s", application.Spec.Destination.Server, serviceAccountNamespace) +} diff --git a/util/settings/impersonation_test.go b/util/settings/impersonation_test.go new file mode 100644 index 0000000000..a15be1c380 --- /dev/null +++ b/util/settings/impersonation_test.go @@ -0,0 +1,268 @@ +package settings + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/argoproj/argo-cd/v3/pkg/apis/application/v1alpha1" +) + +func TestDeriveServiceAccountToImpersonate(t *testing.T) { + t.Run("MatchingServerAndNamespace", func(t *testing.T) { + project := &v1alpha1.AppProject{ + Spec: v1alpha1.AppProjectSpec{ + DestinationServiceAccounts: []v1alpha1.ApplicationDestinationServiceAccount{ + {Server: "https://cluster-api.example.com", Namespace: "dest-ns", DefaultServiceAccount: "test-sa"}, + }, + }, + } + app := &v1alpha1.Application{ + Spec: v1alpha1.ApplicationSpec{ + Destination: v1alpha1.ApplicationDestination{ + Server: "https://cluster-api.example.com", + Namespace: "dest-ns", + }, + }, + } + cluster := &v1alpha1.Cluster{Server: "https://cluster-api.example.com"} + + user, err := DeriveServiceAccountToImpersonate(project, app, cluster) + require.NoError(t, err) + assert.Equal(t, "system:serviceaccount:dest-ns:test-sa", user) + }) + + t.Run("MatchingWithGlobPatterns", func(t *testing.T) { + project := &v1alpha1.AppProject{ + Spec: v1alpha1.AppProjectSpec{ + DestinationServiceAccounts: []v1alpha1.ApplicationDestinationServiceAccount{ + {Server: "*", Namespace: "*", DefaultServiceAccount: "test-sa"}, + }, + }, + } + app := &v1alpha1.Application{ + Spec: v1alpha1.ApplicationSpec{ + Destination: v1alpha1.ApplicationDestination{ + Server: "https://cluster-api.example.com", + Namespace: "any-ns", + }, + }, + } + cluster := &v1alpha1.Cluster{Server: "https://cluster-api.example.com"} + + user, err := DeriveServiceAccountToImpersonate(project, app, cluster) + require.NoError(t, err) + assert.Equal(t, "system:serviceaccount:any-ns:test-sa", user) + }) + + t.Run("MatchingWithNamespacedServiceAccount", func(t *testing.T) { + project := &v1alpha1.AppProject{ + Spec: v1alpha1.AppProjectSpec{ + DestinationServiceAccounts: []v1alpha1.ApplicationDestinationServiceAccount{ + {Server: "https://cluster-api.example.com", Namespace: "dest-ns", DefaultServiceAccount: "other-ns:deploy-sa"}, + }, + }, + } + app := &v1alpha1.Application{ + Spec: v1alpha1.ApplicationSpec{ + Destination: v1alpha1.ApplicationDestination{ + Server: "https://cluster-api.example.com", + Namespace: "dest-ns", + }, + }, + } + cluster := &v1alpha1.Cluster{Server: "https://cluster-api.example.com"} + + user, err := DeriveServiceAccountToImpersonate(project, app, cluster) + require.NoError(t, err) + assert.Equal(t, "system:serviceaccount:other-ns:deploy-sa", user) + }) + + t.Run("FallbackToAppNamespaceWhenDestEmpty", func(t *testing.T) { + project := &v1alpha1.AppProject{ + Spec: v1alpha1.AppProjectSpec{ + DestinationServiceAccounts: []v1alpha1.ApplicationDestinationServiceAccount{ + // Namespace pattern matches empty string via glob "*" + {Server: "*", Namespace: "", DefaultServiceAccount: "test-sa"}, + }, + }, + } + app := &v1alpha1.Application{ + ObjectMeta: metav1.ObjectMeta{Namespace: "app-ns"}, + Spec: v1alpha1.ApplicationSpec{ + Destination: v1alpha1.ApplicationDestination{ + Server: "https://cluster-api.example.com", + Namespace: "", + }, + }, + } + cluster := &v1alpha1.Cluster{Server: "https://cluster-api.example.com"} + + user, err := DeriveServiceAccountToImpersonate(project, app, cluster) + require.NoError(t, err) + // Should use app.Namespace ("app-ns") as the SA namespace since Destination.Namespace is empty + assert.Equal(t, "system:serviceaccount:app-ns:test-sa", user) + }) + + t.Run("NoMatchingEntry", func(t *testing.T) { + project := &v1alpha1.AppProject{ + Spec: v1alpha1.AppProjectSpec{ + DestinationServiceAccounts: []v1alpha1.ApplicationDestinationServiceAccount{ + {Server: "https://other-server.com", Namespace: "other-ns", DefaultServiceAccount: "test-sa"}, + }, + }, + } + app := &v1alpha1.Application{ + Spec: v1alpha1.ApplicationSpec{ + Destination: v1alpha1.ApplicationDestination{ + Server: "https://cluster-api.example.com", + Namespace: "dest-ns", + }, + }, + } + cluster := &v1alpha1.Cluster{Server: "https://cluster-api.example.com"} + + user, err := DeriveServiceAccountToImpersonate(project, app, cluster) + assert.Empty(t, user) + assert.ErrorContains(t, err, "no matching service account found") + }) + + t.Run("EmptyDestinationServiceAccounts", func(t *testing.T) { + project := &v1alpha1.AppProject{ + Spec: v1alpha1.AppProjectSpec{ + DestinationServiceAccounts: []v1alpha1.ApplicationDestinationServiceAccount{}, + }, + } + app := &v1alpha1.Application{ + Spec: v1alpha1.ApplicationSpec{ + Destination: v1alpha1.ApplicationDestination{ + Server: "https://cluster-api.example.com", + Namespace: "dest-ns", + }, + }, + } + cluster := &v1alpha1.Cluster{Server: "https://cluster-api.example.com"} + + user, err := DeriveServiceAccountToImpersonate(project, app, cluster) + assert.Empty(t, user) + assert.ErrorContains(t, err, "no matching service account found") + }) + + t.Run("InvalidServiceAccountChars", func(t *testing.T) { + project := &v1alpha1.AppProject{ + Spec: v1alpha1.AppProjectSpec{ + DestinationServiceAccounts: []v1alpha1.ApplicationDestinationServiceAccount{ + {Server: "*", Namespace: "*", DefaultServiceAccount: "bad*sa"}, + }, + }, + } + app := &v1alpha1.Application{ + Spec: v1alpha1.ApplicationSpec{ + Destination: v1alpha1.ApplicationDestination{ + Server: "https://cluster-api.example.com", + Namespace: "dest-ns", + }, + }, + } + cluster := &v1alpha1.Cluster{Server: "https://cluster-api.example.com"} + + user, err := DeriveServiceAccountToImpersonate(project, app, cluster) + assert.Empty(t, user) + assert.ErrorContains(t, err, "default service account contains invalid chars") + }) + + t.Run("BlankServiceAccount", func(t *testing.T) { + project := &v1alpha1.AppProject{ + Spec: v1alpha1.AppProjectSpec{ + DestinationServiceAccounts: []v1alpha1.ApplicationDestinationServiceAccount{ + {Server: "*", Namespace: "*", DefaultServiceAccount: " "}, + }, + }, + } + app := &v1alpha1.Application{ + Spec: v1alpha1.ApplicationSpec{ + Destination: v1alpha1.ApplicationDestination{ + Server: "https://cluster-api.example.com", + Namespace: "dest-ns", + }, + }, + } + cluster := &v1alpha1.Cluster{Server: "https://cluster-api.example.com"} + + user, err := DeriveServiceAccountToImpersonate(project, app, cluster) + assert.Empty(t, user) + assert.ErrorContains(t, err, "default service account contains invalid chars") + }) + + t.Run("InvalidServerGlobPattern", func(t *testing.T) { + project := &v1alpha1.AppProject{ + Spec: v1alpha1.AppProjectSpec{ + DestinationServiceAccounts: []v1alpha1.ApplicationDestinationServiceAccount{ + {Server: "[", Namespace: "dest-ns", DefaultServiceAccount: "test-sa"}, + }, + }, + } + app := &v1alpha1.Application{ + Spec: v1alpha1.ApplicationSpec{ + Destination: v1alpha1.ApplicationDestination{ + Server: "https://cluster-api.example.com", + Namespace: "dest-ns", + }, + }, + } + cluster := &v1alpha1.Cluster{Server: "https://cluster-api.example.com"} + + user, err := DeriveServiceAccountToImpersonate(project, app, cluster) + assert.Empty(t, user) + assert.ErrorContains(t, err, "invalid glob pattern for destination server") + }) + + t.Run("InvalidNamespaceGlobPattern", func(t *testing.T) { + project := &v1alpha1.AppProject{ + Spec: v1alpha1.AppProjectSpec{ + DestinationServiceAccounts: []v1alpha1.ApplicationDestinationServiceAccount{ + {Server: "*", Namespace: "[", DefaultServiceAccount: "test-sa"}, + }, + }, + } + app := &v1alpha1.Application{ + Spec: v1alpha1.ApplicationSpec{ + Destination: v1alpha1.ApplicationDestination{ + Server: "https://cluster-api.example.com", + Namespace: "dest-ns", + }, + }, + } + cluster := &v1alpha1.Cluster{Server: "https://cluster-api.example.com"} + + user, err := DeriveServiceAccountToImpersonate(project, app, cluster) + assert.Empty(t, user) + assert.ErrorContains(t, err, "invalid glob pattern for destination namespace") + }) + + t.Run("FirstMatchWins", func(t *testing.T) { + project := &v1alpha1.AppProject{ + Spec: v1alpha1.AppProjectSpec{ + DestinationServiceAccounts: []v1alpha1.ApplicationDestinationServiceAccount{ + {Server: "*", Namespace: "dest-ns", DefaultServiceAccount: "first-sa"}, + {Server: "*", Namespace: "*", DefaultServiceAccount: "second-sa"}, + }, + }, + } + app := &v1alpha1.Application{ + Spec: v1alpha1.ApplicationSpec{ + Destination: v1alpha1.ApplicationDestination{ + Server: "https://cluster-api.example.com", + Namespace: "dest-ns", + }, + }, + } + cluster := &v1alpha1.Cluster{Server: "https://cluster-api.example.com"} + + user, err := DeriveServiceAccountToImpersonate(project, app, cluster) + require.NoError(t, err) + assert.Equal(t, "system:serviceaccount:dest-ns:first-sa", user) + }) +}