Skip to content

Commit

Permalink
fix: Better enforce application namespace restrictions
Browse files Browse the repository at this point in the history 
Signed-off-by: jannfis <jann@mistrust.net>
  • Loading branch information
@jannfis
jannfis committed Sep 8, 2023
1 parent 696e6e8 commit 5c4b129
Show file tree
Hide file tree
Showing 3 changed files with 167 additions and 7 deletions.
27 changes: 20 additions & 7 deletions controller/appcontroller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
}

Expand Down Expand Up @@ -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)
}
}
Expand All @@ -1879,6 +1885,14 @@ func (ctrl *ApplicationController) newApplicationInformerAndLister() (cache.Shar
cache.NamespaceIndex: func(obj interface{}) ([]string, error) {
app, ok := obj.(*appv1.Application)
if ok {
// If the application is not allowed to use the project,
// log an error.
if ctrl.isAppNamespaceAllowed(app) {
if _, err := ctrl.getAppProj(app); err != nil {
ctrl.setAppCondition(app, ctrl.projectErrorToCondition(err, app))
}
}

// 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)
Expand All @@ -1889,11 +1903,6 @@ func (ctrl *ApplicationController) newApplicationInformerAndLister() (cache.Shar
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))
}
}

return cache.MetaNamespaceIndexFunc(obj)
Expand All @@ -1904,6 +1913,10 @@ func (ctrl *ApplicationController) newApplicationInformerAndLister() (cache.Shar
return nil, nil
}

if !ctrl.isAppNamespaceAllowed(app) {
return nil, nil
}

proj, err := applisters.NewAppProjectLister(ctrl.projInformer.GetIndexer()).AppProjects(ctrl.namespace).Get(app.Spec.GetProject())
if err != nil {
return nil, nil
Expand Down
4 changes: 4 additions & 0 deletions server/application/application.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
})
Expand Down Expand Up @@ -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.
Expand Down
143 changes: 143 additions & 0 deletions server/application/application_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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")
})

}

0 comments on commit 5c4b129

Please sign in to comment.