Skip to content

Commit

Permalink
Add config setting to preserve V2 update/delete RBAC (argoproj#19988)
Browse files Browse the repository at this point in the history 
A breaking change was introduced in a previous commit that is planned to
be a part of the next major version of Argo CD (v3) where it's okay to
introduce breaking changes. We want this feature before we hit v3, so
we add a config setting that allows us to explicitly turn this new v3
behavior on in v2. The current v2 behavior is the default, so this
change will not affect folks who do not explicitly opt in.

This commit to add the gating code is added separately so it will be
easy to either cherry pick that pervious commit or revert this one.

(see also argoproj#18124, argoproj#20600)

Signed-off-by: Matt Finkel <[email protected]>
  • Loading branch information
@fffinkel
fffinkel committed Nov 5, 2024
1 parent f1f2b9e commit b80c8b3
Show file tree
Hide file tree
Showing 4 changed files with 352 additions and 9 deletions.
34 changes: 29 additions & 5 deletions docs/operator-manual/rbac.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -138,12 +138,36 @@ p, example-user, applications, delete, default/prod-app, deny
p, example-user, applications, delete/*/Pod/*, default/prod-app, allow
```

If we want to explicitly allow updates to the application, but deny updates to any sub-resources:
!!! note

```csv
p, example-user, applications, update, default/prod-app, allow
p, example-user, applications, update/*, default/prod-app, deny
```
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
```

!!! note

In v3, RBAC will have a breaking change. The `udpate` and `delete` actions
(without a `/*`) will no longer include sub-resources. This allows you to
explicitly allow or deny access to an application without affecting its
sub-resources. For example, you may want to allow enable/disable of auto-sync
by allowing update on an application, but disallow the editing of deployment
manifests for that application.

To enable this behavior before v3, you can set the config value
`server.rbac.enablev3` to `true` in the Argo CD ConfigMap argocd-cm.

Once you do so, you can explicitly allow updates to the application, but deny
updates to any sub-resources:

```csv
p, example-user, applications, update, default/prod-app, allow
p, example-user, applications, update/*, default/prod-app, deny
```

#### The `action` action

Expand Down
11 changes: 10 additions & 1 deletion server/application/application.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1333,10 +1333,19 @@ 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) {
if action == rbacpolicy.ActionDelete || action == rbacpolicy.ActionUpdate {
enableV3, err := s.settingsMgr.GetServerRBACEnableV3()
if err != nil {
return nil, nil, nil, errors.New("asdfasdfasdfasdf: " + err.Error())
}

if enableV3 && (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())
if !enableV3 && err != nil && errors.Is(err, permissionDeniedErr) && (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())
}
if err != nil {
return nil, nil, nil, err
}
Expand Down
301 changes: 298 additions & 3 deletions server/application/application_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1650,7 +1650,12 @@ func TestDeleteResourcesRBAC(t *testing.T) {
// nolint:staticcheck
ctx = context.WithValue(ctx, "claims", &jwt.RegisteredClaims{Subject: "test-user"})
testApp := newTestApp()
appServer := newTestAppServer(t, testApp)
f := func(enf *rbac.Enforcer) {
_ = enf.SetBuiltinPolicy(assets.BuiltinPolicyCSV)
enf.SetDefaultRole("role:admin")
}
argoCM := map[string]string{"server.rbac.enablev3": "true"}
appServer := newTestAppServerWithEnforcerConfigure(t, f, argoCM, testApp)
appServer.enf.SetDefaultRole("")

req := application.ApplicationResourceDeleteRequest{
Expand Down Expand Up @@ -1708,14 +1713,82 @@ p, test-user, applications, delete/fake.io/PodTest/*, default/test-app, deny
})
}

func TestPatchResourcesRBAC(t *testing.T) {
func TestDeleteResourcesRBACV2(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("")

req := application.ApplicationResourceDeleteRequest{
Name: &testApp.Name,
AppNamespace: &testApp.Namespace,
Group: strToPtr("fake.io"),
Kind: strToPtr("PodTest"),
Namespace: strToPtr("fake-ns"),
ResourceName: strToPtr("my-pod-test"),
}

expectedErrorWhenDeleteAllowed := "rpc error: code = InvalidArgument desc = PodTest fake.io my-pod-test not found as part of application test-app"

t.Run("delete with application permission", func(t *testing.T) {
_ = appServer.enf.SetBuiltinPolicy(`
p, test-user, applications, delete, default/test-app, allow
`)
_, err := appServer.DeleteResource(ctx, &req)
assert.Equal(t, expectedErrorWhenDeleteAllowed, err.Error())
})

t.Run("delete with application permission but deny subresource", func(t *testing.T) {
_ = appServer.enf.SetBuiltinPolicy(`
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())
})

t.Run("delete with subresource", func(t *testing.T) {
_ = appServer.enf.SetBuiltinPolicy(`
p, test-user, applications, delete/*, default/test-app, allow
`)
_, err := appServer.DeleteResource(ctx, &req)
assert.Equal(t, expectedErrorWhenDeleteAllowed, err.Error())
})

t.Run("delete with subresource but deny applications", func(t *testing.T) {
_ = appServer.enf.SetBuiltinPolicy(`
p, test-user, applications, delete, default/test-app, deny
p, test-user, applications, delete/*, default/test-app, allow
`)
_, err := appServer.DeleteResource(ctx, &req)
assert.Equal(t, expectedErrorWhenDeleteAllowed, err.Error())
})

t.Run("delete with specific subresource denied", func(t *testing.T) {
_ = appServer.enf.SetBuiltinPolicy(`
p, test-user, applications, delete/*, default/test-app, allow
p, test-user, applications, delete/fake.io/PodTest/*, default/test-app, deny
`)
_, err := appServer.DeleteResource(ctx, &req)
assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String())
})
}

func TestPatchResourcesRBAC(t *testing.T) {
ctx := context.Background()
// nolint:staticcheck
ctx = context.WithValue(ctx, "claims", &jwt.RegisteredClaims{Subject: "test-user"})
testApp := newTestApp()
f := func(enf *rbac.Enforcer) {
_ = enf.SetBuiltinPolicy(assets.BuiltinPolicyCSV)
enf.SetDefaultRole("role:admin")
}
argoCM := map[string]string{"server.rbac.enablev3": "true"}
appServer := newTestAppServerWithEnforcerConfigure(t, f, argoCM, testApp)
appServer.enf.SetDefaultRole("")

req := application.ApplicationResourcePatchRequest{
Name: &testApp.Name,
AppNamespace: &testApp.Namespace,
Expand Down Expand Up @@ -1771,7 +1844,70 @@ p, test-user, applications, update/fake.io/PodTest/*, default/test-app, deny
})
}

func TestUpdateApplicationRBAC(t *testing.T) {
func TestPatchResourcesRBACV2(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("")

req := 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("patch with application permission", func(t *testing.T) {
_ = appServer.enf.SetBuiltinPolicy(`
p, test-user, applications, update, default/test-app, allow
`)
_, err := appServer.PatchResource(ctx, &req)
assert.Equal(t, expectedErrorWhenUpdateAllowed, err.Error())
})

t.Run("patch with application permission but deny subresource", func(t *testing.T) {
_ = appServer.enf.SetBuiltinPolicy(`
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())
})

t.Run("patch with subresource", func(t *testing.T) {
_ = appServer.enf.SetBuiltinPolicy(`
p, test-user, applications, update/*, default/test-app, allow
`)
_, err := appServer.PatchResource(ctx, &req)
assert.Equal(t, expectedErrorWhenUpdateAllowed, err.Error())
})

t.Run("patch with subresource but deny applications", func(t *testing.T) {
_ = appServer.enf.SetBuiltinPolicy(`
p, test-user, applications, update, default/test-app, deny
p, test-user, applications, update/*, default/test-app, allow
`)
_, err := appServer.PatchResource(ctx, &req)
assert.Equal(t, expectedErrorWhenUpdateAllowed, err.Error())
})

t.Run("patch with specific subresource denied", 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.PatchResource(ctx, &req)
assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String())
})
}

func TestUpdateApplicationRBACV2(t *testing.T) {
ctx := context.Background()
// nolint:staticcheck
ctx = context.WithValue(ctx, "claims", &jwt.RegisteredClaims{Subject: "test-user"})
Expand Down Expand Up @@ -1813,6 +1949,165 @@ p, test-user, applications, update/*, default/test-app, allow

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 and resource 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, expectedErrorWhenUpdateAllowed, err.Error())
})

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, expectedErrorWhenUpdateAllowed, err.Error())
})

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())
})
}

// this test tests the V3 RBAC functionality
func TestUpdateApplicationRBAC(t *testing.T) {
ctx := context.Background()
// nolint:staticcheck
ctx = context.WithValue(ctx, "claims", &jwt.RegisteredClaims{Subject: "test-user"})
testApp := newTestApp()
f := func(enf *rbac.Enforcer) {
_ = enf.SetBuiltinPolicy(assets.BuiltinPolicyCSV)
enf.SetDefaultRole("role:admin")
}
argoCM := map[string]string{"server.rbac.enablev3": "true"}
appServer := newTestAppServerWithEnforcerConfigure(t, f, argoCM, testApp)
appServer.enf.SetDefaultRole("")
testApp.Spec.Project = ""

// TODO someohow set v3

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
Expand Down
Loading

0 comments on commit b80c8b3

Please sign in to comment.