diff --git a/controller/appcontroller.go b/controller/appcontroller.go index 9f3e32ba0b6e6..d65c0b6411ded 100644 --- a/controller/appcontroller.go +++ b/controller/appcontroller.go @@ -1803,6 +1803,12 @@ func (ctrl *ApplicationController) shouldSelfHeal(app *appv1.Application) (bool, return retryAfter <= 0, retryAfter } +// isAppNamespaceAllowed returns whether the application is allowed in the +// namespace it's residing in. +func (ctrl *ApplicationController) isAppNamespaceAllowed(app *appv1.Application) bool { + return app.Namespace == ctrl.namespace || glob.MatchStringInList(ctrl.applicationNamespaces, app.Namespace, false) +} + func (ctrl *ApplicationController) canProcessApp(obj interface{}) bool { app, ok := obj.(*appv1.Application) if !ok { @@ -1811,7 +1817,7 @@ func (ctrl *ApplicationController) canProcessApp(obj interface{}) bool { // Only process given app if it exists in a watched namespace, or in the // control plane's namespace. - if app.Namespace != ctrl.namespace && !glob.MatchStringInList(ctrl.applicationNamespaces, app.Namespace, false) { + if !ctrl.isAppNamespaceAllowed(app) { return false } @@ -1862,7 +1868,7 @@ func (ctrl *ApplicationController) newApplicationInformerAndLister() (cache.Shar } newItems := []appv1.Application{} for _, app := range appList.Items { - if ctrl.namespace == app.Namespace || glob.MatchStringInList(ctrl.applicationNamespaces, app.Namespace, false) { + if ctrl.isAppNamespaceAllowed(&app) { newItems = append(newItems, app) } } @@ -1879,20 +1885,24 @@ func (ctrl *ApplicationController) newApplicationInformerAndLister() (cache.Shar cache.NamespaceIndex: func(obj interface{}) ([]string, error) { app, ok := obj.(*appv1.Application) if ok { - // This call to 'ValidateDestination' ensures that the .spec.destination field of all Applications - // returned by the informer/lister will have server field set (if not already set) based on the name. - // (or, if not found, an error app condition) - - // If the server field is not set, set it based on the cluster name; if the cluster name can't be found, - // log an error as an App Condition. - if err := argo.ValidateDestination(context.Background(), &app.Spec.Destination, ctrl.db); err != nil { - ctrl.setAppCondition(app, appv1.ApplicationCondition{Type: appv1.ApplicationConditionInvalidSpecError, Message: err.Error()}) - } - - // If the application is not allowed to use the project, - // log an error. - if _, err := ctrl.getAppProj(app); err != nil { - ctrl.setAppCondition(app, ctrl.projectErrorToCondition(err, app)) + // We only generally work with applications that are in one + // the allowed namespaces. + if ctrl.isAppNamespaceAllowed(app) { + // If the application is not allowed to use the project, + // log an error. + if _, err := ctrl.getAppProj(app); err != nil { + ctrl.setAppCondition(app, ctrl.projectErrorToCondition(err, app)) + } else { + // This call to 'ValidateDestination' ensures that the .spec.destination field of all Applications + // returned by the informer/lister will have server field set (if not already set) based on the name. + // (or, if not found, an error app condition) + + // If the server field is not set, set it based on the cluster name; if the cluster name can't be found, + // log an error as an App Condition. + if err := argo.ValidateDestination(context.Background(), &app.Spec.Destination, ctrl.db); err != nil { + ctrl.setAppCondition(app, appv1.ApplicationCondition{Type: appv1.ApplicationConditionInvalidSpecError, Message: err.Error()}) + } + } } } @@ -1904,7 +1914,11 @@ func (ctrl *ApplicationController) newApplicationInformerAndLister() (cache.Shar return nil, nil } - proj, err := applisters.NewAppProjectLister(ctrl.projInformer.GetIndexer()).AppProjects(ctrl.namespace).Get(app.Spec.GetProject()) + if !ctrl.isAppNamespaceAllowed(app) { + return nil, nil + } + + proj, err := ctrl.getAppProj(app) if err != nil { return nil, nil } diff --git a/server/application/application.go b/server/application/application.go index afc2e0c734670..12484685e52b3 100644 --- a/server/application/application.go +++ b/server/application/application.go @@ -233,6 +233,9 @@ func (s *Server) getApplicationEnforceRBACInformer(ctx context.Context, action, func (s *Server) getApplicationEnforceRBACClient(ctx context.Context, action, project, namespace, name, resourceVersion string) (*appv1.Application, error) { namespaceOrDefault := s.appNamespaceOrDefault(namespace) return s.getAppEnforceRBAC(ctx, action, project, namespaceOrDefault, name, func() (*appv1.Application, error) { + if !s.isNamespaceEnabled(namespaceOrDefault) { + return nil, security.NamespaceNotPermittedError(namespaceOrDefault) + } return s.appclientset.ArgoprojV1alpha1().Applications(namespaceOrDefault).Get(ctx, name, metav1.GetOptions{ ResourceVersion: resourceVersion, }) @@ -644,6 +647,7 @@ func (s *Server) Get(ctx context.Context, q *application.ApplicationQuery) (*app } else if len(projects) > 1 { return nil, status.Errorf(codes.InvalidArgument, "multiple projects specified - the get endpoint accepts either zero or one project") } + // 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. diff --git a/server/application/application_test.go b/server/application/application_test.go index 57b740a6f1ea4..56be539e48ac0 100644 --- a/server/application/application_test.go +++ b/server/application/application_test.go @@ -170,6 +170,7 @@ func newTestAppServerWithEnforcerConfigure(f func(*rbac.Enforcer), t *testing.T, Destinations: []appsv1.ApplicationDestination{{Server: "*", Namespace: "*"}}, }, } + myProj := &appsv1.AppProject{ ObjectMeta: metav1.ObjectMeta{Name: "my-proj", Namespace: "default"}, Spec: appsv1.AppProjectSpec{ @@ -2343,3 +2344,145 @@ func TestIsApplicationPermitted(t *testing.T) { assert.True(t, permitted) }) } + +func TestAppNamespaceRestrictions(t *testing.T) { + t.Run("List applications in controller namespace", func(t *testing.T) { + testApp := newTestApp() + appServer := newTestAppServer(t, testApp) + apps, err := appServer.List(context.TODO(), &application.ApplicationQuery{}) + require.NoError(t, err) + require.Len(t, apps.Items, 1) + }) + + t.Run("List applications with non-allowed apps existing", func(t *testing.T) { + testApp1 := newTestApp() + testApp1.Namespace = "argocd-1" + appServer := newTestAppServer(t, testApp1) + apps, err := appServer.List(context.TODO(), &application.ApplicationQuery{}) + require.NoError(t, err) + require.Len(t, apps.Items, 0) + }) + + t.Run("List applications with non-allowed apps existing and explicit ns request", func(t *testing.T) { + testApp1 := newTestApp() + testApp2 := newTestApp() + testApp2.Namespace = "argocd-1" + appServer := newTestAppServer(t, testApp1, testApp2) + apps, err := appServer.List(context.TODO(), &application.ApplicationQuery{AppNamespace: pointer.String("argocd-1")}) + require.NoError(t, err) + require.Len(t, apps.Items, 0) + }) + + t.Run("List applications with allowed apps in other namespaces", func(t *testing.T) { + testApp1 := newTestApp() + testApp1.Namespace = "argocd-1" + appServer := newTestAppServer(t, testApp1) + appServer.enabledNamespaces = []string{"argocd-1"} + apps, err := appServer.List(context.TODO(), &application.ApplicationQuery{}) + require.NoError(t, err) + require.Len(t, apps.Items, 1) + }) + + t.Run("Get application in control plane namespace", func(t *testing.T) { + testApp := newTestApp() + appServer := newTestAppServer(t, testApp) + app, err := appServer.Get(context.TODO(), &application.ApplicationQuery{ + Name: pointer.String("test-app"), + }) + require.NoError(t, err) + assert.Equal(t, "test-app", app.GetName()) + }) + t.Run("Get application in other namespace when forbidden", func(t *testing.T) { + testApp := newTestApp() + testApp.Namespace = "argocd-1" + appServer := newTestAppServer(t, testApp) + app, err := appServer.Get(context.TODO(), &application.ApplicationQuery{ + Name: pointer.String("test-app"), + AppNamespace: pointer.String("argocd-1"), + }) + require.Error(t, err) + require.ErrorContains(t, err, "permission denied") + require.Nil(t, app) + }) + t.Run("Get application in other namespace when allowed", func(t *testing.T) { + testApp := newTestApp() + testApp.Namespace = "argocd-1" + appServer := newTestAppServer(t, testApp) + appServer.enabledNamespaces = []string{"argocd-1"} + app, err := appServer.Get(context.TODO(), &application.ApplicationQuery{ + Name: pointer.String("test-app"), + AppNamespace: pointer.String("argocd-1"), + }) + require.NoError(t, err) + require.NotNil(t, app) + require.Equal(t, "argocd-1", app.Namespace) + require.Equal(t, "test-app", app.Name) + }) + t.Run("Create application in other namespace when 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, otherNsProj) + appServer.enabledNamespaces = []string{"argocd-1"} + app, err := appServer.Create(context.TODO(), &application.ApplicationCreateRequest{ + Application: testApp, + }) + require.NoError(t, err) + require.NotNil(t, app) + assert.Equal(t, "test-app", app.Name) + assert.Equal(t, "argocd-1", app.Namespace) + }) + + t.Run("Create application in other namespace when not allowed by project", 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{}, + }, + } + appServer := newTestAppServer(t, otherNsProj) + appServer.enabledNamespaces = []string{"argocd-1"} + app, err := appServer.Create(context.TODO(), &application.ApplicationCreateRequest{ + Application: testApp, + }) + require.Error(t, err) + require.Nil(t, app) + require.ErrorContains(t, err, "not allowed to use project") + }) + + t.Run("Create application in other namespace when not allowed by configuration", 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, otherNsProj) + appServer.enabledNamespaces = []string{"argocd-2"} + app, err := appServer.Create(context.TODO(), &application.ApplicationCreateRequest{ + Application: testApp, + }) + require.Error(t, err) + require.Nil(t, app) + require.ErrorContains(t, err, "namespace 'argocd-1' is not permitted") + }) + +}