From c85cb82e786fa51961e5c9d0cf85f5fd69341d61 Mon Sep 17 00:00:00 2001 From: pasha-codefresh Date: Mon, 15 Apr 2024 10:20:07 +0300 Subject: [PATCH] Merge pull request from GHSA-2gvw-w6fj-7m3c * chore: Update USERS.md (#17683) Add Shield.com as one of the users in the USER.md file Signed-off-by: suhas-chikkanna <162577490+suhas-chikkanna@users.noreply.github.com> sec: validate project before execute action Signed-off-by: pashakostohrys * sec: validate a project before execute an action Signed-off-by: pashakostohrys --------- Signed-off-by: pashakostohrys Co-authored-by: suhas-chikkanna <162577490+suhas-chikkanna@users.noreply.github.com> --- .../application/v1alpha1/app_project_types.go | 18 ++ server/application/application.go | 188 +++++++++--------- server/application/application_test.go | 118 ++++++++++- util/argo/argo.go | 3 +- 4 files changed, 223 insertions(+), 104 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 6753db1888f8f..f7842989ebf5e 100644 --- a/server/application/application.go +++ b/server/application/application.go @@ -150,7 +150,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) { user := session.Username(ctx) if user == "" { user = "Unknown user" @@ -172,7 +172,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() @@ -180,15 +180,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 @@ -202,11 +202,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 != "" { @@ -219,15 +219,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) @@ -237,7 +242,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) { @@ -321,7 +326,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) } @@ -377,7 +388,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, proj *appv1.AppProject, action func( client apiclient.RepoServerServiceClient, helmRepos []*appv1.Repository, helmCreds []*appv1.RepoCreds, @@ -391,14 +402,6 @@ func (s *Server) queryRepoServer(ctx context.Context, a *appv1.Application, acti } defer ioutil.Close(closer) - 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 { return fmt.Errorf("error listing helm repositories: %w", err) @@ -432,7 +435,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 } @@ -442,7 +445,7 @@ func (s *Server) GetManifests(ctx context.Context, q *application.ApplicationMan } manifestInfos := make([]*apiclient.ManifestResponse, 0) - err = s.queryRepoServer(ctx, a, func( + err = s.queryRepoServer(ctx, proj, func( client apiclient.RepoServerServiceClient, helmRepos []*appv1.Repository, helmCreds []*appv1.RepoCreds, helmOptions *appv1.HelmOptions, enableGenerateManifests map[string]bool) error { appInstanceLabelKey, err := s.settingsMgr.GetAppInstanceLabelKey() @@ -464,12 +467,7 @@ func (s *Server) GetManifests(ctx context.Context, q *application.ApplicationMan if err != nil { 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) - } - + sources := make([]appv1.ApplicationSource, 0) if a.Spec.HasMultipleSources() { numOfSources := int64(len(a.Spec.GetSources())) @@ -581,13 +579,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, proj, func( client apiclient.RepoServerServiceClient, helmRepos []*appv1.Repository, helmCreds []*appv1.RepoCreds, helmOptions *appv1.HelmOptions, enableGenerateManifests map[string]bool) error { appInstanceLabelKey, err := s.settingsMgr.GetAppInstanceLabelKey() @@ -712,7 +710,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 } @@ -743,7 +741,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, proj, func( client apiclient.RepoServerServiceClient, helmRepos []*appv1.Repository, _ []*appv1.RepoCreds, @@ -805,7 +803,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 } @@ -873,12 +871,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) } @@ -977,7 +975,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 } @@ -996,7 +994,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 } @@ -1039,11 +1037,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 } @@ -1198,16 +1220,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") } @@ -1311,7 +1324,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 } @@ -1448,7 +1461,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 } @@ -1457,7 +1470,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 } @@ -1474,7 +1487,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 } @@ -1484,12 +1497,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) @@ -1504,7 +1511,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 } @@ -1535,7 +1542,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 } @@ -1592,7 +1599,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 } @@ -1789,19 +1796,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) { @@ -1898,7 +1897,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 } @@ -1957,7 +1956,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 } @@ -1972,7 +1971,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 } @@ -1987,12 +1986,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{} @@ -2055,7 +2049,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 } @@ -2111,7 +2110,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 } @@ -2184,7 +2183,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 } @@ -2280,6 +2279,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. @@ -2287,7 +2291,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 } @@ -2381,14 +2385,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 { @@ -2448,16 +2445,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 58e51d4075b46..e82a011895544 100644 --- a/server/application/application_test.go +++ b/server/application/application_test.go @@ -1819,7 +1819,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) }) } @@ -2531,7 +2531,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"), @@ -2542,6 +2551,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" @@ -2584,7 +2615,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) { @@ -2608,5 +2639,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 ccc4fe81e94d2..031f1dac6408c 100644 --- a/util/argo/argo.go +++ b/util/argo/argo.go @@ -700,8 +700,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 }