Enable fine-grained update/delete RBAC enforcement by default (argopr…

…oj#19988) Change applications resource RBAC to use fine-grained update/delete enforcement by default. This allows us to enforce RBAC on the application itself, separately from the sub-resources related to it. (see also argoproj#18124, argoproj#20600) Signed-off-by: Matt Finkel <[email protected]>
fffinkel · Nov 5, 2024 · f1f2b9e · f1f2b9e
1 parent 8a4e7e0
commit f1f2b9e
Show file tree

Hide file tree

Showing 4 changed files with 166 additions and 18 deletions.
diff --git a/assets/builtin-policy.csv b/assets/builtin-policy.csv
@@ -17,7 +17,9 @@ p, role:readonly, logs, get, */*, allow
 
 p, role:admin, applications, create, */*, allow
 p, role:admin, applications, update, */*, allow
+p, role:admin, applications, update/*, */*, allow
 p, role:admin, applications, delete, */*, allow
+p, role:admin, applications, delete/*, */*, allow
 p, role:admin, applications, sync, */*, allow
 p, role:admin, applications, override, */*, allow
 p, role:admin, applications, action/*, */*, allow
@@ -43,4 +45,4 @@ p, role:admin, gpgkeys, delete, *, allow
 p, role:admin, exec, create, */*, allow
 
 g, role:admin, role:readonly
-g, admin, role:admin
+g, admin, role:admin
diff --git a/docs/operator-manual/rbac.md b/docs/operator-manual/rbac.md
@@ -114,7 +114,7 @@ The `applications` resource is an [Application-Specific Policy](#application-spe
 
 #### Fine-grained Permissions for `update`/`delete` action
 
-The `update` and `delete` actions, when granted on an application, will allow the user to perform the operation on the application itself **and** all of its resources.
+The `update` and `delete` actions, when granted on an application, will allow the user to perform the operation on the application itself but not its resources.
 It can be desirable to only allow `update` or `delete` on specific resources within an application.
 
 To do so, when the action if performed on an application's resource, the `<action>` will have the `<action>/<group>/<kind>/<ns>/<name>` format.
@@ -138,15 +138,12 @@ p, example-user, applications, delete, default/prod-app, deny
 p, example-user, applications, delete/*/Pod/*, default/prod-app, allow
 ```
 
-!!! note
-
-    It is not possible to deny fine-grained permissions for a sub-resource if the action was **explicitly allowed on the application**.
-    For instance, the following policies will **allow** a user to delete the Pod and any other resources in the application:
+If we want to explicitly allow updates to the application, but deny updates to any sub-resources:
 
-    ```csv
-    p, example-user, applications, delete, default/prod-app, allow
-    p, example-user, applications, delete/*/Pod/*, default/prod-app, deny
-    ```
+```csv
+p, example-user, applications, update, default/prod-app, allow
+p, example-user, applications, update/*, default/prod-app, deny
+```
 
 #### The `action` action
 

diff --git a/server/application/application.go b/server/application/application.go
@@ -1333,12 +1333,10 @@ 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())
-	if err != nil && errors.Is(err, permissionDeniedErr) && (action == rbacpolicy.ActionDelete || action == rbacpolicy.ActionUpdate) {
-		// If users dont have permission on the whole applications, maybe they have fine-grained access to the specific resources
+	if action == rbacpolicy.ActionDelete || action == rbacpolicy.ActionUpdate {
 		action = fmt.Sprintf("%s/%s/%s/%s/%s", action, q.GetGroup(), q.GetKind(), q.GetNamespace(), q.GetResourceName())
-		a, _, err = s.getApplicationEnforceRBACInformer(ctx, action, q.GetProject(), q.GetAppNamespace(), q.GetName())
 	}
+	a, _, err := s.getApplicationEnforceRBACInformer(ctx, action, q.GetProject(), q.GetAppNamespace(), q.GetName())
 	if err != nil {
 		return nil, nil, nil, err
 	}

diff --git a/server/application/application_test.go b/server/application/application_test.go
@@ -1669,7 +1669,7 @@ func TestDeleteResourcesRBAC(t *testing.T) {
 p, test-user, applications, delete, default/test-app, allow
 `)
 		_, err := appServer.DeleteResource(ctx, &req)
-		assert.Equal(t, expectedErrorWhenDeleteAllowed, err.Error())
+		assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String())
 	})
 
 	t.Run("delete with application permission but deny subresource", func(t *testing.T) {
@@ -1678,7 +1678,7 @@ p, test-user, applications, delete, default/test-app, allow
 p, test-user, applications, delete/*, default/test-app, deny
 `)
 		_, err := appServer.DeleteResource(ctx, &req)
-		assert.Equal(t, expectedErrorWhenDeleteAllowed, err.Error())
+		assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String())
 	})
 
 	t.Run("delete with subresource", func(t *testing.T) {
@@ -1732,7 +1732,7 @@ func TestPatchResourcesRBAC(t *testing.T) {
 p, test-user, applications, update, default/test-app, allow
 `)
 		_, err := appServer.PatchResource(ctx, &req)
-		assert.Equal(t, expectedErrorWhenUpdateAllowed, err.Error())
+		assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String())
 	})
 
 	t.Run("patch with application permission but deny subresource", func(t *testing.T) {
@@ -1741,7 +1741,7 @@ p, test-user, applications, update, default/test-app, allow
 p, test-user, applications, update/*, default/test-app, deny
 `)
 		_, err := appServer.PatchResource(ctx, &req)
-		assert.Equal(t, expectedErrorWhenUpdateAllowed, err.Error())
+		assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String())
 	})
 
 	t.Run("patch with subresource", func(t *testing.T) {
@@ -1771,6 +1771,157 @@ p, test-user, applications, update/fake.io/PodTest/*, default/test-app, deny
 	})
 }
 
+func TestUpdateApplicationRBAC(t *testing.T) {
+	ctx := context.Background()
+	// nolint:staticcheck
+	ctx = context.WithValue(ctx, "claims", &jwt.RegisteredClaims{Subject: "test-user"})
+	testApp := newTestApp()
+	appServer := newTestAppServer(t, testApp)
+	appServer.enf.SetDefaultRole("")
+	testApp.Spec.Project = ""
+
+	appSpecReq := application.ApplicationUpdateSpecRequest{
+		Name:         &testApp.Name,
+		AppNamespace: &testApp.Namespace,
+		Spec:         &testApp.Spec,
+	}
+
+	t.Run("can update application spec with update allow", func(t *testing.T) {
+		_ = appServer.enf.SetBuiltinPolicy(`
+p, test-user, applications, update, */*, allow
+`)
+		_, err := appServer.UpdateSpec(ctx, &appSpecReq)
+		require.NoError(t, err)
+	})
+
+	t.Run("cannot update application spec with sub-resource update allow", func(t *testing.T) {
+		_ = appServer.enf.SetBuiltinPolicy(`
+p, test-user, applications, update/*, default/test-app, allow
+`)
+		_, err := appServer.UpdateSpec(ctx, &appSpecReq)
+		assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String())
+	})
+
+	resourceReq := application.ApplicationResourcePatchRequest{
+		Name:         &testApp.Name,
+		AppNamespace: &testApp.Namespace,
+		Group:        strToPtr("fake.io"),
+		Kind:         strToPtr("PodTest"),
+		Namespace:    strToPtr("fake-ns"),
+		ResourceName: strToPtr("my-pod-test"),
+	}
+
+	expectedErrorWhenUpdateAllowed := "rpc error: code = InvalidArgument desc = PodTest fake.io my-pod-test not found as part of application test-app"
+
+	t.Run("can update application spec with update allow, sub-resource update deny", func(t *testing.T) {
+		_ = appServer.enf.SetBuiltinPolicy(`
+p, test-user, applications, update, default/test-app, allow
+p, test-user, applications, update/fake.io/PodTest/*, default/test-app, deny
+`)
+		_, err := appServer.UpdateSpec(ctx, &appSpecReq)
+		require.NoError(t, err)
+		_, err = appServer.PatchResource(ctx, &resourceReq)
+		assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String())
+	})
+
+	t.Run("can update application spec with update allow, sub-resource update allow", func(t *testing.T) {
+		_ = appServer.enf.SetBuiltinPolicy(`
+p, test-user, applications, update, default/test-app, allow
+p, test-user, applications, update/fake.io/PodTest/*, default/test-app, allow
+`)
+		_, err := appServer.UpdateSpec(ctx, &appSpecReq)
+		require.NoError(t, err)
+		_, err = appServer.PatchResource(ctx, &resourceReq)
+		assert.Equal(t, expectedErrorWhenUpdateAllowed, err.Error())
+	})
+
+	t.Run("cannot update application spec with update deny, sub-resource update allow", func(t *testing.T) {
+		_ = appServer.enf.SetBuiltinPolicy(`
+p, test-user, applications, update, default/test-app, deny
+p, test-user, applications, update/fake.io/PodTest/*, default/test-app, allow
+`)
+		_, err := appServer.UpdateSpec(ctx, &appSpecReq)
+		assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String())
+		_, err = appServer.PatchResource(ctx, &resourceReq)
+		assert.Equal(t, expectedErrorWhenUpdateAllowed, err.Error())
+	})
+
+	t.Run("cannot update application spec with update deny, sub-resource update deny", func(t *testing.T) {
+		_ = appServer.enf.SetBuiltinPolicy(`
+p, test-user, applications, update, default/test-app, deny
+p, test-user, applications, update/fake.io/PodTest/*, default/test-app, deny
+`)
+		_, err := appServer.UpdateSpec(ctx, &appSpecReq)
+		assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String())
+		_, err = appServer.PatchResource(ctx, &resourceReq)
+		assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String())
+	})
+
+	appReq := application.ApplicationUpdateRequest{
+		Application: testApp,
+	}
+
+	t.Run("update application with generic permission", func(t *testing.T) {
+		_ = appServer.enf.SetBuiltinPolicy(`
+p, test-user, applications, update, default/test-app, allow
+`)
+		_, err := appServer.Update(ctx, &appReq)
+		require.NoError(t, err)
+	})
+
+	t.Run("cannot update application with sub-resource permission", func(t *testing.T) {
+		_ = appServer.enf.SetBuiltinPolicy(`
+p, test-user, applications, update/*, default/test-app, allow
+`)
+		_, err := appServer.Update(ctx, &appReq)
+		assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String())
+	})
+
+	t.Run("can update application with update allow, sub-resource update deny", func(t *testing.T) {
+		_ = appServer.enf.SetBuiltinPolicy(`
+p, test-user, applications, update, default/test-app, allow
+p, test-user, applications, update/fake.io/PodTest/*, default/test-app, deny
+`)
+		_, err := appServer.Update(ctx, &appReq)
+		require.NoError(t, err)
+		_, err = appServer.PatchResource(ctx, &resourceReq)
+		assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String())
+	})
+
+	t.Run("can update application with update allow, sub-resource update allow", func(t *testing.T) {
+		_ = appServer.enf.SetBuiltinPolicy(`
+p, test-user, applications, update, default/test-app, allow
+p, test-user, applications, update/fake.io/PodTest/*, default/test-app, allow
+`)
+		_, err := appServer.Update(ctx, &appReq)
+		require.NoError(t, err)
+		_, err = appServer.PatchResource(ctx, &resourceReq)
+		assert.Equal(t, expectedErrorWhenUpdateAllowed, err.Error())
+	})
+
+	t.Run("cannot update application with update deny, sub-resource update allow", func(t *testing.T) {
+		_ = appServer.enf.SetBuiltinPolicy(`
+p, test-user, applications, update, default/test-app, deny
+p, test-user, applications, update/fake.io/PodTest/*, default/test-app, allow
+`)
+		_, err := appServer.Update(ctx, &appReq)
+		assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String())
+		_, err = appServer.PatchResource(ctx, &resourceReq)
+		assert.Equal(t, expectedErrorWhenUpdateAllowed, err.Error())
+	})
+
+	t.Run("cannot update application with update deny, sub-resource update deny", func(t *testing.T) {
+		_ = appServer.enf.SetBuiltinPolicy(`
+p, test-user, applications, update, default/test-app, deny
+p, test-user, applications, update/fake.io/PodTest/*, default/test-app, deny
+`)
+		_, err := appServer.Update(ctx, &appReq)
+		assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String())
+		_, err = appServer.PatchResource(ctx, &resourceReq)
+		assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String())
+	})
+}
+
 func TestSyncAndTerminate(t *testing.T) {
 	ctx := context.Background()
 	appServer := newTestAppServer(t)