From e0ff56d89fbd7d066e9c862b30337f6520f13f17 Mon Sep 17 00:00:00 2001 From: pasha-codefresh Date: Mon, 15 Apr 2024 10:20:08 +0300 Subject: [PATCH] Merge pull request from GHSA-2gvw-w6fj-7m3c * sec: validate a project before execute an action Signed-off-by: pashakostohrys * sec: validate a project before execute an action Signed-off-by: pashakostohrys --------- Signed-off-by: pashakostohrys --- .../application/v1alpha1/app_project_types.go | 18 ++ server/application/application.go | 185 +++++++++--------- server/application/application_test.go | 118 ++++++++++- util/argo/argo.go | 3 +- util/session/sessionmanager.go | 2 +- 5 files changed, 223 insertions(+), 103 deletions(-) diff --git a/pkg/apis/application/v1alpha1/app_project_types.go b/pkg/apis/application/v1alpha1/app_project_types.go index 5243ab7990266..81f95ab624a0d 100644 --- a/pkg/apis/application/v1alpha1/app_project_types.go +++ b/pkg/apis/application/v1alpha1/app_project_types.go @@ -17,6 +17,24 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" ) +type ErrApplicationNotAllowedToUseProject struct { + application string + namespace string + project string +} + +func NewErrApplicationNotAllowedToUseProject(application, namespace, project string) error { + return &ErrApplicationNotAllowedToUseProject{ + application: application, + namespace: namespace, + project: project, + } +} + +func (err *ErrApplicationNotAllowedToUseProject) Error() string { + return fmt.Sprintf("application '%s' in namespace '%s' is not allowed to use project %s", err.application, err.namespace, err.project) +} + // AppProjectList is list of AppProject resources // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object type AppProjectList struct { diff --git a/server/application/application.go b/server/application/application.go index 36daf3ee5d1f5..13689d4cd9dc2 100644 --- a/server/application/application.go +++ b/server/application/application.go @@ -148,7 +148,7 @@ func NewServer( // // If the user does provide a "project," we can respond more specifically. If the user does not have access to the given // app name in the given project, we return "permission denied." If the app exists, but the project is different from -func (s *Server) getAppEnforceRBAC(ctx context.Context, action, project, namespace, name string, getApp func() (*appv1.Application, error)) (*appv1.Application, error) { +func (s *Server) getAppEnforceRBAC(ctx context.Context, action, project, namespace, name string, getApp func() (*appv1.Application, error)) (*appv1.Application, *appv1.AppProject, error) { logCtx := log.WithFields(map[string]interface{}{ "application": name, "namespace": namespace, @@ -165,7 +165,7 @@ func (s *Server) getAppEnforceRBAC(ctx context.Context, action, project, namespa // but the app is in a different project" response. We don't want the user inferring the existence of the // app from response time. _, _ = getApp() - return nil, permissionDeniedErr + return nil, nil, permissionDeniedErr } } a, err := getApp() @@ -173,15 +173,15 @@ func (s *Server) getAppEnforceRBAC(ctx context.Context, action, project, namespa if apierr.IsNotFound(err) { if project != "" { // We know that the user was allowed to get the Application, but the Application does not exist. Return 404. - return nil, status.Errorf(codes.NotFound, apierr.NewNotFound(schema.GroupResource{Group: "argoproj.io", Resource: "applications"}, name).Error()) + return nil, nil, status.Errorf(codes.NotFound, apierr.NewNotFound(schema.GroupResource{Group: "argoproj.io", Resource: "applications"}, name).Error()) } // We don't know if the user was allowed to get the Application, and we don't want to leak information about // the Application's existence. Return 403. logCtx.Warn("application does not exist") - return nil, permissionDeniedErr + return nil, nil, permissionDeniedErr } logCtx.Errorf("failed to get application: %s", err) - return nil, permissionDeniedErr + return nil, nil, permissionDeniedErr } // Even if we performed an initial RBAC check (because the request was fully parameterized), we still need to // perform a second RBAC check to ensure that the user has access to the actual Application's project (not just the @@ -195,11 +195,11 @@ func (s *Server) getAppEnforceRBAC(ctx context.Context, action, project, namespa // The user specified a project. We would have returned a 404 if the user had access to the app, but the app // did not exist. So we have to return a 404 when the app does exist, but the user does not have access. // Otherwise, they could infer that the app exists based on the error code. - return nil, status.Errorf(codes.NotFound, apierr.NewNotFound(schema.GroupResource{Group: "argoproj.io", Resource: "applications"}, name).Error()) + return nil, nil, status.Errorf(codes.NotFound, apierr.NewNotFound(schema.GroupResource{Group: "argoproj.io", Resource: "applications"}, name).Error()) } // The user didn't specify a project. We always return permission denied for both lack of access and lack of // existence. - return nil, permissionDeniedErr + return nil, nil, permissionDeniedErr } effectiveProject := "default" if a.Spec.Project != "" { @@ -212,15 +212,20 @@ func (s *Server) getAppEnforceRBAC(ctx context.Context, action, project, namespa }).Warnf("user tried to %s application in project %s, but the application is in project %s", action, project, effectiveProject) // The user has access to the app, but the app is in a different project. Return 404, meaning "app doesn't // exist in that project". - return nil, status.Errorf(codes.NotFound, apierr.NewNotFound(schema.GroupResource{Group: "argoproj.io", Resource: "applications"}, name).Error()) + return nil, nil, status.Errorf(codes.NotFound, apierr.NewNotFound(schema.GroupResource{Group: "argoproj.io", Resource: "applications"}, name).Error()) } - return a, nil + // Get the app's associated project, and make sure all project restrictions are enforced. + proj, err := s.getAppProject(ctx, a, logCtx) + if err != nil { + return a, nil, err + } + return a, proj, nil } // getApplicationEnforceRBACInformer uses an informer to get an Application. If the app does not exist, permission is // denied, or any other error occurs when getting the app, we return a permission denied error to obscure any sensitive // information. -func (s *Server) getApplicationEnforceRBACInformer(ctx context.Context, action, project, namespace, name string) (*appv1.Application, error) { +func (s *Server) getApplicationEnforceRBACInformer(ctx context.Context, action, project, namespace, name string) (*appv1.Application, *appv1.AppProject, error) { namespaceOrDefault := s.appNamespaceOrDefault(namespace) return s.getAppEnforceRBAC(ctx, action, project, namespaceOrDefault, name, func() (*appv1.Application, error) { return s.appLister.Applications(namespaceOrDefault).Get(name) @@ -230,7 +235,7 @@ func (s *Server) getApplicationEnforceRBACInformer(ctx context.Context, action, // getApplicationEnforceRBACClient uses a client to get an Application. If the app does not exist, permission is denied, // or any other error occurs when getting the app, we return a permission denied error to obscure any sensitive // information. -func (s *Server) getApplicationEnforceRBACClient(ctx context.Context, action, project, namespace, name, resourceVersion string) (*appv1.Application, error) { +func (s *Server) getApplicationEnforceRBACClient(ctx context.Context, action, project, namespace, name, resourceVersion string) (*appv1.Application, *appv1.AppProject, error) { namespaceOrDefault := s.appNamespaceOrDefault(namespace) return s.getAppEnforceRBAC(ctx, action, project, namespaceOrDefault, name, func() (*appv1.Application, error) { if !s.isNamespaceEnabled(namespaceOrDefault) { @@ -314,7 +319,13 @@ func (s *Server) Create(ctx context.Context, q *application.ApplicationCreateReq if q.Validate != nil { validate = *q.Validate } - err := s.validateAndNormalizeApp(ctx, a, validate) + + proj, err := s.getAppProject(ctx, a, log.WithField("application", a.Name)) + if err != nil { + return nil, err + } + + err = s.validateAndNormalizeApp(ctx, a, proj, validate) if err != nil { return nil, fmt.Errorf("error while validating and normalizing app: %w", err) } @@ -370,7 +381,7 @@ func (s *Server) Create(ctx context.Context, q *application.ApplicationCreateReq return updated, nil } -func (s *Server) queryRepoServer(ctx context.Context, a *appv1.Application, action func( +func (s *Server) queryRepoServer(ctx context.Context, a *appv1.Application, proj *appv1.AppProject, action func( client apiclient.RepoServerServiceClient, repo *appv1.Repository, helmRepos []*appv1.Repository, @@ -397,13 +408,6 @@ func (s *Server) queryRepoServer(ctx context.Context, a *appv1.Application, acti if err != nil { return fmt.Errorf("error getting kustomize settings options: %w", err) } - proj, err := argo.GetAppProject(a, applisters.NewAppProjectLister(s.projInformer.GetIndexer()), s.ns, s.settingsMgr, s.db, ctx) - if err != nil { - if apierr.IsNotFound(err) { - return status.Errorf(codes.InvalidArgument, "application references project %s which does not exist", a.Spec.Project) - } - return fmt.Errorf("error getting application's project: %w", err) - } helmRepos, err := s.db.ListHelmRepositories(ctx) if err != nil { @@ -438,7 +442,7 @@ func (s *Server) GetManifests(ctx context.Context, q *application.ApplicationMan if q.Name == nil || *q.Name == "" { return nil, fmt.Errorf("invalid request: application name is missing") } - a, err := s.getApplicationEnforceRBACInformer(ctx, rbacpolicy.ActionGet, q.GetProject(), q.GetAppNamespace(), q.GetName()) + a, proj, err := s.getApplicationEnforceRBACInformer(ctx, rbacpolicy.ActionGet, q.GetProject(), q.GetAppNamespace(), q.GetName()) if err != nil { return nil, err } @@ -450,7 +454,7 @@ func (s *Server) GetManifests(ctx context.Context, q *application.ApplicationMan } var manifestInfo *apiclient.ManifestResponse - err = s.queryRepoServer(ctx, a, func( + err = s.queryRepoServer(ctx, a, proj, func( client apiclient.RepoServerServiceClient, repo *appv1.Repository, helmRepos []*appv1.Repository, helmCreds []*appv1.RepoCreds, helmOptions *appv1.HelmOptions, kustomizeOptions *appv1.KustomizeOptions, enableGenerateManifests map[string]bool) error { revision := source.TargetRevision if q.GetRevision() != "" { @@ -476,11 +480,6 @@ func (s *Server) GetManifests(ctx context.Context, q *application.ApplicationMan return fmt.Errorf("error getting API resources: %w", err) } - proj, err := argo.GetAppProject(a, applisters.NewAppProjectLister(s.projInformer.GetIndexer()), s.ns, s.settingsMgr, s.db, ctx) - if err != nil { - return fmt.Errorf("error getting app project: %w", err) - } - manifestInfo, err = client.GenerateManifest(ctx, &apiclient.ManifestRequest{ Repo: repo, Revision: revision, @@ -543,13 +542,13 @@ func (s *Server) GetManifestsWithFiles(stream application.ApplicationService_Get return fmt.Errorf("invalid request: application name is missing") } - a, err := s.getApplicationEnforceRBACInformer(ctx, rbacpolicy.ActionGet, query.GetProject(), query.GetAppNamespace(), query.GetName()) + a, proj, err := s.getApplicationEnforceRBACInformer(ctx, rbacpolicy.ActionGet, query.GetProject(), query.GetAppNamespace(), query.GetName()) if err != nil { return err } var manifestInfo *apiclient.ManifestResponse - err = s.queryRepoServer(ctx, a, func( + err = s.queryRepoServer(ctx, a, proj, func( client apiclient.RepoServerServiceClient, repo *appv1.Repository, helmRepos []*appv1.Repository, helmCreds []*appv1.RepoCreds, helmOptions *appv1.HelmOptions, kustomizeOptions *appv1.KustomizeOptions, enableGenerateManifests map[string]bool) error { appInstanceLabelKey, err := s.settingsMgr.GetAppInstanceLabelKey() @@ -660,7 +659,7 @@ func (s *Server) Get(ctx context.Context, q *application.ApplicationQuery) (*app // We must use a client Get instead of an informer Get, because it's common to call Get immediately // following a Watch (which is not yet powered by an informer), and the Get must reflect what was // previously seen by the client. - a, err := s.getApplicationEnforceRBACClient(ctx, rbacpolicy.ActionGet, project, appNs, appName, q.GetResourceVersion()) + a, proj, err := s.getApplicationEnforceRBACClient(ctx, rbacpolicy.ActionGet, project, appNs, appName, q.GetResourceVersion()) if err != nil { return nil, err } @@ -691,7 +690,7 @@ func (s *Server) Get(ctx context.Context, q *application.ApplicationQuery) (*app if refreshType == appv1.RefreshTypeHard { // force refresh cached application details - if err := s.queryRepoServer(ctx, a, func( + if err := s.queryRepoServer(ctx, a, proj, func( client apiclient.RepoServerServiceClient, repo *appv1.Repository, helmRepos []*appv1.Repository, @@ -743,7 +742,7 @@ func (s *Server) Get(ctx context.Context, q *application.ApplicationQuery) (*app // ListResourceEvents returns a list of event resources func (s *Server) ListResourceEvents(ctx context.Context, q *application.ApplicationResourceEventsQuery) (*v1.EventList, error) { - a, err := s.getApplicationEnforceRBACInformer(ctx, rbacpolicy.ActionGet, q.GetProject(), q.GetAppNamespace(), q.GetName()) + a, _, err := s.getApplicationEnforceRBACInformer(ctx, rbacpolicy.ActionGet, q.GetProject(), q.GetAppNamespace(), q.GetName()) if err != nil { return nil, err } @@ -811,12 +810,12 @@ func (s *Server) validateAndUpdateApp(ctx context.Context, newApp *appv1.Applica s.projectLock.RLock(newApp.Spec.GetProject()) defer s.projectLock.RUnlock(newApp.Spec.GetProject()) - app, err := s.getApplicationEnforceRBACClient(ctx, action, currentProject, newApp.Namespace, newApp.Name, "") + app, proj, err := s.getApplicationEnforceRBACClient(ctx, action, currentProject, newApp.Namespace, newApp.Name, "") if err != nil { return nil, err } - err = s.validateAndNormalizeApp(ctx, newApp, validate) + err = s.validateAndNormalizeApp(ctx, newApp, proj, validate) if err != nil { return nil, fmt.Errorf("error validating and normalizing app: %w", err) } @@ -915,7 +914,7 @@ func (s *Server) UpdateSpec(ctx context.Context, q *application.ApplicationUpdat if q.GetSpec() == nil { return nil, fmt.Errorf("error updating application spec: spec is nil in request") } - a, err := s.getApplicationEnforceRBACClient(ctx, rbacpolicy.ActionUpdate, q.GetProject(), q.GetAppNamespace(), q.GetName(), "") + a, _, err := s.getApplicationEnforceRBACClient(ctx, rbacpolicy.ActionUpdate, q.GetProject(), q.GetAppNamespace(), q.GetName(), "") if err != nil { return nil, err } @@ -934,7 +933,7 @@ func (s *Server) UpdateSpec(ctx context.Context, q *application.ApplicationUpdat // Patch patches an application func (s *Server) Patch(ctx context.Context, q *application.ApplicationPatchRequest) (*appv1.Application, error) { - app, err := s.getApplicationEnforceRBACClient(ctx, rbacpolicy.ActionGet, q.GetProject(), q.GetAppNamespace(), q.GetName(), "") + app, _, err := s.getApplicationEnforceRBACClient(ctx, rbacpolicy.ActionGet, q.GetProject(), q.GetAppNamespace(), q.GetName(), "") if err != nil { return nil, err } @@ -977,11 +976,35 @@ func (s *Server) Patch(ctx context.Context, q *application.ApplicationPatchReque return s.validateAndUpdateApp(ctx, newApp, false, true, rbacpolicy.ActionUpdate, q.GetProject()) } +func (s *Server) getAppProject(ctx context.Context, a *appv1.Application, logCtx *log.Entry) (*appv1.AppProject, error) { + proj, err := argo.GetAppProject(a, applisters.NewAppProjectLister(s.projInformer.GetIndexer()), s.ns, s.settingsMgr, s.db, ctx) + if err == nil { + return proj, nil + } + + // If there's a permission issue or the app doesn't exist, return a vague error to avoid letting the user enumerate project names. + vagueError := status.Errorf(codes.InvalidArgument, "app is not allowed in project %q, or the project does not exist", a.Spec.Project) + + if apierr.IsNotFound(err) { + return nil, vagueError + } + + if _, ok := err.(*appv1.ErrApplicationNotAllowedToUseProject); ok { + logCtx.WithFields(map[string]interface{}{ + "project": a.Spec.Project, + argocommon.SecurityField: argocommon.SecurityMedium, + }).Warnf("error getting app project: %s", err) + return nil, vagueError + } + + return nil, vagueError +} + // Delete removes an application and all associated resources func (s *Server) Delete(ctx context.Context, q *application.ApplicationDeleteRequest) (*application.ApplicationResponse, error) { appName := q.GetName() appNs := s.appNamespaceOrDefault(q.GetAppNamespace()) - a, err := s.getApplicationEnforceRBACClient(ctx, rbacpolicy.ActionGet, q.GetProject(), appNs, appName, "") + a, _, err := s.getApplicationEnforceRBACClient(ctx, rbacpolicy.ActionGet, q.GetProject(), appNs, appName, "") if err != nil { return nil, err } @@ -1136,16 +1159,7 @@ func (s *Server) Watch(q *application.ApplicationQuery, ws application.Applicati } } -func (s *Server) validateAndNormalizeApp(ctx context.Context, app *appv1.Application, validate bool) error { - proj, err := argo.GetAppProject(app, applisters.NewAppProjectLister(s.projInformer.GetIndexer()), s.ns, s.settingsMgr, s.db, ctx) - if err != nil { - if apierr.IsNotFound(err) { - // Offer no hint that the project does not exist. - log.Warnf("User attempted to create/update application in non-existent project %q", app.Spec.Project) - return permissionDeniedErr - } - return fmt.Errorf("error getting application's project: %w", err) - } +func (s *Server) validateAndNormalizeApp(ctx context.Context, app *appv1.Application, proj *appv1.AppProject, validate bool) error { if app.GetName() == "" { return fmt.Errorf("resource name may not be empty") } @@ -1249,7 +1263,7 @@ func (s *Server) getAppResources(ctx context.Context, a *appv1.Application) (*ap } func (s *Server) getAppLiveResource(ctx context.Context, action string, q *application.ApplicationResourceRequest) (*appv1.ResourceNode, *rest.Config, *appv1.Application, error) { - a, err := s.getApplicationEnforceRBACInformer(ctx, action, q.GetProject(), q.GetAppNamespace(), q.GetName()) + a, _, err := s.getApplicationEnforceRBACInformer(ctx, action, q.GetProject(), q.GetAppNamespace(), q.GetName()) if err != nil { return nil, nil, nil, err } @@ -1386,7 +1400,7 @@ func (s *Server) DeleteResource(ctx context.Context, q *application.ApplicationR } func (s *Server) ResourceTree(ctx context.Context, q *application.ResourcesQuery) (*appv1.ApplicationTree, error) { - a, err := s.getApplicationEnforceRBACInformer(ctx, rbacpolicy.ActionGet, q.GetProject(), q.GetAppNamespace(), q.GetApplicationName()) + a, _, err := s.getApplicationEnforceRBACInformer(ctx, rbacpolicy.ActionGet, q.GetProject(), q.GetAppNamespace(), q.GetApplicationName()) if err != nil { return nil, err } @@ -1395,7 +1409,7 @@ func (s *Server) ResourceTree(ctx context.Context, q *application.ResourcesQuery } func (s *Server) WatchResourceTree(q *application.ResourcesQuery, ws application.ApplicationService_WatchResourceTreeServer) error { - _, err := s.getApplicationEnforceRBACInformer(ws.Context(), rbacpolicy.ActionGet, q.GetProject(), q.GetAppNamespace(), q.GetApplicationName()) + _, _, err := s.getApplicationEnforceRBACInformer(ws.Context(), rbacpolicy.ActionGet, q.GetProject(), q.GetAppNamespace(), q.GetApplicationName()) if err != nil { return err } @@ -1412,7 +1426,7 @@ func (s *Server) WatchResourceTree(q *application.ResourcesQuery, ws application } func (s *Server) RevisionMetadata(ctx context.Context, q *application.RevisionMetadataQuery) (*appv1.RevisionMetadata, error) { - a, err := s.getApplicationEnforceRBACInformer(ctx, rbacpolicy.ActionGet, q.GetProject(), q.GetAppNamespace(), q.GetName()) + a, proj, err := s.getApplicationEnforceRBACInformer(ctx, rbacpolicy.ActionGet, q.GetProject(), q.GetAppNamespace(), q.GetName()) if err != nil { return nil, err } @@ -1422,12 +1436,6 @@ func (s *Server) RevisionMetadata(ctx context.Context, q *application.RevisionMe if err != nil { return nil, fmt.Errorf("error getting repository by URL: %w", err) } - // We need to get some information with the project associated to the app, - // so we'll know whether GPG signatures are enforced. - proj, err := argo.GetAppProject(a, applisters.NewAppProjectLister(s.projInformer.GetIndexer()), s.ns, s.settingsMgr, s.db, ctx) - if err != nil { - return nil, fmt.Errorf("error getting app project: %w", err) - } conn, repoClient, err := s.repoClientset.NewRepoServerClient() if err != nil { return nil, fmt.Errorf("error creating repo server client: %w", err) @@ -1442,7 +1450,7 @@ func (s *Server) RevisionMetadata(ctx context.Context, q *application.RevisionMe // RevisionChartDetails returns the helm chart metadata, as fetched from the reposerver func (s *Server) RevisionChartDetails(ctx context.Context, q *application.RevisionMetadataQuery) (*appv1.ChartDetails, error) { - a, err := s.getApplicationEnforceRBACInformer(ctx, rbacpolicy.ActionGet, q.GetProject(), q.GetAppNamespace(), q.GetName()) + a, _, err := s.getApplicationEnforceRBACInformer(ctx, rbacpolicy.ActionGet, q.GetProject(), q.GetAppNamespace(), q.GetName()) if err != nil { return nil, err } @@ -1473,7 +1481,7 @@ func isMatchingResource(q *application.ResourcesQuery, key kube.ResourceKey) boo } func (s *Server) ManagedResources(ctx context.Context, q *application.ResourcesQuery) (*application.ManagedResourcesResponse, error) { - a, err := s.getApplicationEnforceRBACInformer(ctx, rbacpolicy.ActionGet, q.GetProject(), q.GetAppNamespace(), q.GetApplicationName()) + a, _, err := s.getApplicationEnforceRBACInformer(ctx, rbacpolicy.ActionGet, q.GetProject(), q.GetAppNamespace(), q.GetApplicationName()) if err != nil { return nil, err } @@ -1530,7 +1538,7 @@ func (s *Server) PodLogs(q *application.ApplicationPodLogsQuery, ws application. } } - a, err := s.getApplicationEnforceRBACInformer(ws.Context(), rbacpolicy.ActionGet, q.GetProject(), q.GetAppNamespace(), q.GetName()) + a, _, err := s.getApplicationEnforceRBACInformer(ws.Context(), rbacpolicy.ActionGet, q.GetProject(), q.GetAppNamespace(), q.GetName()) if err != nil { return err } @@ -1722,19 +1730,11 @@ func isTheSelectedOne(currentNode *appv1.ResourceNode, q *application.Applicatio // Sync syncs an application to its target state func (s *Server) Sync(ctx context.Context, syncReq *application.ApplicationSyncRequest) (*appv1.Application, error) { - a, err := s.getApplicationEnforceRBACClient(ctx, rbacpolicy.ActionGet, syncReq.GetProject(), syncReq.GetAppNamespace(), syncReq.GetName(), "") + a, proj, err := s.getApplicationEnforceRBACClient(ctx, rbacpolicy.ActionGet, syncReq.GetProject(), syncReq.GetAppNamespace(), syncReq.GetName(), "") if err != nil { return nil, err } - proj, err := argo.GetAppProject(a, applisters.NewAppProjectLister(s.projInformer.GetIndexer()), s.ns, s.settingsMgr, s.db, ctx) - if err != nil { - if apierr.IsNotFound(err) { - return a, status.Errorf(codes.InvalidArgument, "application references project %s which does not exist", a.Spec.Project) - } - return a, fmt.Errorf("error getting app project: %w", err) - } - s.inferResourcesStatusHealth(a) if !proj.Spec.SyncWindows.Matches(a).CanSync(true) { @@ -1831,7 +1831,7 @@ func (s *Server) Sync(ctx context.Context, syncReq *application.ApplicationSyncR } func (s *Server) Rollback(ctx context.Context, rollbackReq *application.ApplicationRollbackRequest) (*appv1.Application, error) { - a, err := s.getApplicationEnforceRBACClient(ctx, rbacpolicy.ActionSync, rollbackReq.GetProject(), rollbackReq.GetAppNamespace(), rollbackReq.GetName(), "") + a, _, err := s.getApplicationEnforceRBACClient(ctx, rbacpolicy.ActionSync, rollbackReq.GetProject(), rollbackReq.GetAppNamespace(), rollbackReq.GetName(), "") if err != nil { return nil, err } @@ -1890,7 +1890,7 @@ func (s *Server) Rollback(ctx context.Context, rollbackReq *application.Applicat } func (s *Server) ListLinks(ctx context.Context, req *application.ListAppLinksRequest) (*application.LinksResponse, error) { - a, err := s.getApplicationEnforceRBACClient(ctx, rbacpolicy.ActionGet, req.GetProject(), req.GetNamespace(), req.GetName(), "") + a, proj, err := s.getApplicationEnforceRBACClient(ctx, rbacpolicy.ActionGet, req.GetProject(), req.GetNamespace(), req.GetName(), "") if err != nil { return nil, err } @@ -1905,7 +1905,7 @@ func (s *Server) ListLinks(ctx context.Context, req *application.ListAppLinksReq return nil, fmt.Errorf("failed to read application deep links from configmap: %w", err) } - clstObj, _, err := s.getObjectsForDeepLinks(ctx, a) + clstObj, _, err := s.getObjectsForDeepLinks(ctx, a, proj) if err != nil { return nil, err } @@ -1920,12 +1920,7 @@ func (s *Server) ListLinks(ctx context.Context, req *application.ListAppLinksReq return finalList, nil } -func (s *Server) getObjectsForDeepLinks(ctx context.Context, app *appv1.Application) (cluster *unstructured.Unstructured, project *unstructured.Unstructured, err error) { - proj, err := argo.GetAppProject(app, applisters.NewAppProjectLister(s.projInformer.GetIndexer()), s.ns, s.settingsMgr, s.db, ctx) - if err != nil { - return nil, nil, fmt.Errorf("error getting app project: %w", err) - } - +func (s *Server) getObjectsForDeepLinks(ctx context.Context, app *appv1.Application, proj *appv1.AppProject) (cluster *unstructured.Unstructured, project *unstructured.Unstructured, err error) { // sanitize project jwt tokens proj.Status = appv1.AppProjectStatus{} @@ -1988,7 +1983,12 @@ func (s *Server) ListResourceLinks(ctx context.Context, req *application.Applica return nil, err } - clstObj, projObj, err := s.getObjectsForDeepLinks(ctx, app) + proj, err := s.getAppProject(ctx, app, log.WithField("application", app.GetName())) + if err != nil { + return nil, err + } + + clstObj, projObj, err := s.getObjectsForDeepLinks(ctx, app, proj) if err != nil { return nil, err } @@ -2044,7 +2044,7 @@ func (s *Server) resolveRevision(ctx context.Context, app *appv1.Application, sy func (s *Server) TerminateOperation(ctx context.Context, termOpReq *application.OperationTerminateRequest) (*application.OperationTerminateResponse, error) { appName := termOpReq.GetName() appNs := s.appNamespaceOrDefault(termOpReq.GetAppNamespace()) - a, err := s.getApplicationEnforceRBACClient(ctx, rbacpolicy.ActionSync, termOpReq.GetProject(), appNs, appName, "") + a, _, err := s.getApplicationEnforceRBACClient(ctx, rbacpolicy.ActionSync, termOpReq.GetProject(), appNs, appName, "") if err != nil { return nil, err } @@ -2117,7 +2117,7 @@ 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 *appv1.ResourceNode, app *appv1.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()) + app, _, err = s.getApplicationEnforceRBACInformer(ctx, rbacRequest, q.GetProject(), q.GetAppNamespace(), q.GetName()) if err != nil { return nil, nil, nil, nil, err } @@ -2213,6 +2213,11 @@ func (s *Server) RunResourceAction(ctx context.Context, q *application.ResourceA } } + proj, err := s.getAppProject(ctx, a, log.WithField("application", a.Name)) + if err != nil { + return nil, err + } + // First, make sure all the returned resources are permitted, for each operation. // Also perform create with dry-runs for all create-operation resources. // This is performed separately to reduce the risk of only some of the resources being successfully created later. @@ -2220,7 +2225,7 @@ func (s *Server) RunResourceAction(ctx context.Context, q *application.ResourceA // the dry-run for relevant apply/delete operation would have to be invoked as well. for _, impactedResource := range newObjects { newObj := impactedResource.UnstructuredObj - err := s.verifyResourcePermitted(ctx, app, newObj) + err := s.verifyResourcePermitted(ctx, app, proj, newObj) if err != nil { return nil, err } @@ -2314,14 +2319,7 @@ func (s *Server) patchResource(ctx context.Context, config *rest.Config, liveObj return &application.ApplicationResponse{}, nil } -func (s *Server) verifyResourcePermitted(ctx context.Context, app *appv1.Application, obj *unstructured.Unstructured) error { - proj, err := argo.GetAppProject(app, applisters.NewAppProjectLister(s.projInformer.GetIndexer()), s.ns, s.settingsMgr, s.db, ctx) - if err != nil { - if apierr.IsNotFound(err) { - return fmt.Errorf("application references project %s which does not exist", app.Spec.Project) - } - return fmt.Errorf("failed to get project %s: %w", app.Spec.Project, err) - } +func (s *Server) verifyResourcePermitted(ctx context.Context, app *appv1.Application, proj *appv1.AppProject, obj *unstructured.Unstructured) error { permitted, err := proj.IsResourcePermitted(schema.GroupKind{Group: obj.GroupVersionKind().Group, Kind: obj.GroupVersionKind().Kind}, obj.GetNamespace(), app.Spec.Destination, func(project string) ([]*appv1.Cluster, error) { clusters, err := s.db.GetProjectClusters(context.TODO(), project) if err != nil { @@ -2381,16 +2379,11 @@ func splitStatusPatch(patch []byte) ([]byte, []byte, error) { } func (s *Server) GetApplicationSyncWindows(ctx context.Context, q *application.ApplicationSyncWindowsQuery) (*application.ApplicationSyncWindowsResponse, error) { - a, err := s.getApplicationEnforceRBACClient(ctx, rbacpolicy.ActionGet, q.GetProject(), q.GetAppNamespace(), q.GetName(), "") + a, proj, err := s.getApplicationEnforceRBACClient(ctx, rbacpolicy.ActionGet, q.GetProject(), q.GetAppNamespace(), q.GetName(), "") if err != nil { return nil, err } - proj, err := argo.GetAppProject(a, applisters.NewAppProjectLister(s.projInformer.GetIndexer()), s.ns, s.settingsMgr, s.db, ctx) - if err != nil { - return nil, fmt.Errorf("error getting app project: %w", err) - } - windows := proj.Spec.SyncWindows.Matches(a) sync := windows.CanSync(true) diff --git a/server/application/application_test.go b/server/application/application_test.go index 8e5cb59cd4263..03e38cf14c79e 100644 --- a/server/application/application_test.go +++ b/server/application/application_test.go @@ -1818,7 +1818,7 @@ func TestServer_GetApplicationSyncWindowsState(t *testing.T) { appServer := newTestAppServer(t, testApp) active, err := appServer.GetApplicationSyncWindows(context.Background(), &application.ApplicationSyncWindowsQuery{Name: &testApp.Name}) - assert.Contains(t, err.Error(), "not found") + assert.Contains(t, err.Error(), "not exist") assert.Nil(t, active) }) } @@ -2428,7 +2428,16 @@ func TestAppNamespaceRestrictions(t *testing.T) { t.Run("Get application in other namespace when allowed", func(t *testing.T) { testApp := newTestApp() testApp.Namespace = "argocd-1" - appServer := newTestAppServer(t, testApp) + testApp.Spec.Project = "other-ns" + otherNsProj := &appsv1.AppProject{ + ObjectMeta: metav1.ObjectMeta{Name: "other-ns", Namespace: "default"}, + Spec: appsv1.AppProjectSpec{ + SourceRepos: []string{"*"}, + Destinations: []appsv1.ApplicationDestination{{Server: "*", Namespace: "*"}}, + SourceNamespaces: []string{"argocd-1"}, + }, + } + appServer := newTestAppServer(t, testApp, otherNsProj) appServer.enabledNamespaces = []string{"argocd-1"} app, err := appServer.Get(context.TODO(), &application.ApplicationQuery{ Name: pointer.String("test-app"), @@ -2439,6 +2448,28 @@ func TestAppNamespaceRestrictions(t *testing.T) { require.Equal(t, "argocd-1", app.Namespace) require.Equal(t, "test-app", app.Name) }) + t.Run("Get application in other namespace when project is not allowed", func(t *testing.T) { + testApp := newTestApp() + testApp.Namespace = "argocd-1" + testApp.Spec.Project = "other-ns" + otherNsProj := &appsv1.AppProject{ + ObjectMeta: metav1.ObjectMeta{Name: "other-ns", Namespace: "default"}, + Spec: appsv1.AppProjectSpec{ + SourceRepos: []string{"*"}, + Destinations: []appsv1.ApplicationDestination{{Server: "*", Namespace: "*"}}, + SourceNamespaces: []string{"argocd-2"}, + }, + } + appServer := newTestAppServer(t, testApp, otherNsProj) + appServer.enabledNamespaces = []string{"argocd-1"} + app, err := appServer.Get(context.TODO(), &application.ApplicationQuery{ + Name: pointer.String("test-app"), + AppNamespace: pointer.String("argocd-1"), + }) + require.Error(t, err) + require.Nil(t, app) + require.ErrorContains(t, err, "app is not allowed in project") + }) t.Run("Create application in other namespace when allowed", func(t *testing.T) { testApp := newTestApp() testApp.Namespace = "argocd-1" @@ -2481,7 +2512,7 @@ func TestAppNamespaceRestrictions(t *testing.T) { }) require.Error(t, err) require.Nil(t, app) - require.ErrorContains(t, err, "not allowed to use project") + require.ErrorContains(t, err, "app is not allowed in project") }) t.Run("Create application in other namespace when not allowed by configuration", func(t *testing.T) { @@ -2505,5 +2536,84 @@ func TestAppNamespaceRestrictions(t *testing.T) { require.Nil(t, app) require.ErrorContains(t, err, "namespace 'argocd-1' is not permitted") }) - + t.Run("Get application sync window in other namespace when project is allowed", func(t *testing.T) { + testApp := newTestApp() + testApp.Namespace = "argocd-1" + testApp.Spec.Project = "other-ns" + otherNsProj := &appsv1.AppProject{ + ObjectMeta: metav1.ObjectMeta{Name: "other-ns", Namespace: "default"}, + Spec: appsv1.AppProjectSpec{ + SourceRepos: []string{"*"}, + Destinations: []appsv1.ApplicationDestination{{Server: "*", Namespace: "*"}}, + SourceNamespaces: []string{"argocd-1"}, + }, + } + appServer := newTestAppServer(t, testApp, otherNsProj) + appServer.enabledNamespaces = []string{"argocd-1"} + active, err := appServer.GetApplicationSyncWindows(context.TODO(), &application.ApplicationSyncWindowsQuery{Name: &testApp.Name, AppNamespace: &testApp.Namespace}) + assert.NoError(t, err) + assert.Equal(t, 0, len(active.ActiveWindows)) + }) + t.Run("Get application sync window in other namespace when project is not allowed", func(t *testing.T) { + testApp := newTestApp() + testApp.Namespace = "argocd-1" + testApp.Spec.Project = "other-ns" + otherNsProj := &appsv1.AppProject{ + ObjectMeta: metav1.ObjectMeta{Name: "other-ns", Namespace: "default"}, + Spec: appsv1.AppProjectSpec{ + SourceRepos: []string{"*"}, + Destinations: []appsv1.ApplicationDestination{{Server: "*", Namespace: "*"}}, + SourceNamespaces: []string{"argocd-2"}, + }, + } + appServer := newTestAppServer(t, testApp, otherNsProj) + appServer.enabledNamespaces = []string{"argocd-1"} + active, err := appServer.GetApplicationSyncWindows(context.TODO(), &application.ApplicationSyncWindowsQuery{Name: &testApp.Name, AppNamespace: &testApp.Namespace}) + require.Error(t, err) + require.Nil(t, active) + require.ErrorContains(t, err, "app is not allowed in project") + }) + t.Run("Get list of links in other namespace when project is not allowed", func(t *testing.T) { + testApp := newTestApp() + testApp.Namespace = "argocd-1" + testApp.Spec.Project = "other-ns" + otherNsProj := &appsv1.AppProject{ + ObjectMeta: metav1.ObjectMeta{Name: "other-ns", Namespace: "default"}, + Spec: appsv1.AppProjectSpec{ + SourceRepos: []string{"*"}, + Destinations: []appsv1.ApplicationDestination{{Server: "*", Namespace: "*"}}, + SourceNamespaces: []string{"argocd-2"}, + }, + } + appServer := newTestAppServer(t, testApp, otherNsProj) + appServer.enabledNamespaces = []string{"argocd-1"} + links, err := appServer.ListLinks(context.TODO(), &application.ListAppLinksRequest{ + Name: pointer.String("test-app"), + Namespace: pointer.String("argocd-1"), + }) + require.Error(t, err) + require.Nil(t, links) + require.ErrorContains(t, err, "app is not allowed in project") + }) + t.Run("Get list of links in other namespace when project is allowed", func(t *testing.T) { + testApp := newTestApp() + testApp.Namespace = "argocd-1" + testApp.Spec.Project = "other-ns" + otherNsProj := &appsv1.AppProject{ + ObjectMeta: metav1.ObjectMeta{Name: "other-ns", Namespace: "default"}, + Spec: appsv1.AppProjectSpec{ + SourceRepos: []string{"*"}, + Destinations: []appsv1.ApplicationDestination{{Server: "*", Namespace: "*"}}, + SourceNamespaces: []string{"argocd-1"}, + }, + } + appServer := newTestAppServer(t, testApp, otherNsProj) + appServer.enabledNamespaces = []string{"argocd-1"} + links, err := appServer.ListLinks(context.TODO(), &application.ListAppLinksRequest{ + Name: pointer.String("test-app"), + Namespace: pointer.String("argocd-1"), + }) + require.NoError(t, err) + assert.Equal(t, 0, len(links.Items)) + }) } diff --git a/util/argo/argo.go b/util/argo/argo.go index 36e513cf0f534..49862bb64ac39 100644 --- a/util/argo/argo.go +++ b/util/argo/argo.go @@ -694,8 +694,7 @@ func GetAppProject(app *argoappv1.Application, projLister applicationsv1.AppProj return nil, err } if !proj.IsAppNamespacePermitted(app, ns) { - return nil, fmt.Errorf("application '%s' in namespace '%s' is not allowed to use project '%s'", - app.Name, app.Namespace, proj.Name) + return nil, argoappv1.NewErrApplicationNotAllowedToUseProject(app.Name, app.Namespace, proj.Name) } return proj, nil } diff --git a/util/session/sessionmanager.go b/util/session/sessionmanager.go index af22ca0f2502e..e13075847c380 100644 --- a/util/session/sessionmanager.go +++ b/util/session/sessionmanager.go @@ -312,7 +312,7 @@ func expireOldFailedAttempts(maxAge time.Duration, failures map[string]LoginAtte return expiredCount } -// Protect admin user from login attempt reset caused by attempts to overflow cache in a brute force attack. Instead remove random non-admin to make room in cache. +// Protect admin user from login attempt reset caused by attempts to overflow cache in a brute force attack. Instead remove random non-admin to make room in cache. func pickRandomNonAdminLoginFailure(failures map[string]LoginAttempts, username string) *string { idx := rand.Intn(len(failures) - 1) i := 0