From ee04385b9403be008460bfd4a561258b9b092e45 Mon Sep 17 00:00:00 2001 From: Nic Klaassen Date: Thu, 9 Nov 2023 16:49:21 -0800 Subject: [PATCH 1/2] fix!: respect deny rules for access requests Access Request follow their own set of RBAC rules. Usually, none of the typical create/read/list/delete verbs are required in any user's roles. Access is handled via custom rules based on the allow.request, deny.request, allow.review_requests, and deny.review_requests role fields. The create/read/list/delete verbs commonly used for other resources are usually all or nothing (barring `where` expressions), but a more nuanced set of rules apply to access requests. E.g. users should always be allowed to see access requests that they created or are allowed to review, without being allowed to see other access requests in the cluster. This seemed mostly logical once you thought about it long enough, but one detail that has been lacking so far is that explicit deny rules in the user's roles have no effect at all, even though explicit allow rules grant god-mode access to create or view any access requests in the cluster. Even with the following role, you could still create and view access requests: ```yaml kind: role version: v6 metadata: name: example spec: allow: request: roles: ["*"] review_requests: roles: ["*"] deny: rules: - resources: ["access_request"] verbs: ["create", "read", "list"] ``` This commit makes any explicit deny rules actually take effect. Fixes https://github.com/gravitational/customer-sensitive-requests/issues/103 changelog: Respect explicit deny rules for Access Requests. --- lib/auth/access_request_test.go | 173 ++++++++++++++++++++++++++++++++ lib/auth/auth_with_roles.go | 35 ++++--- lib/authz/permissions.go | 3 +- lib/services/role.go | 43 +++++++- 4 files changed, 237 insertions(+), 17 deletions(-) diff --git a/lib/auth/access_request_test.go b/lib/auth/access_request_test.go index ee634728f06ce..855df38b85df4 100644 --- a/lib/auth/access_request_test.go +++ b/lib/auth/access_request_test.go @@ -28,6 +28,7 @@ import ( "github.com/jonboulle/clockwork" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "golang.org/x/exp/maps" "github.com/gravitational/teleport" "github.com/gravitational/teleport/api/client/proto" @@ -186,6 +187,178 @@ func TestAccessRequest(t *testing.T) { t.Run("multi", func(t *testing.T) { testMultiAccessRequests(t, testPack) }) t.Run("role refresh with bogus request ID", func(t *testing.T) { testRoleRefreshWithBogusRequestID(t, testPack) }) t.Run("bot user approver", func(t *testing.T) { testBotAccessRequestReview(t, testPack) }) + t.Run("deny", func(t *testing.T) { testAccessRequestDenyRules(t, testPack) }) +} + +func testAccessRequestDenyRules(t *testing.T, testPack *accessRequestTestPack) { + t.Parallel() + + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + + userName := "denied" + + accessRequest, err := services.NewAccessRequest(userName, "admins") + require.NoError(t, err) + + for _, tc := range []struct { + desc string + roles map[string]types.RoleSpecV6 + expectGetDenied bool + expectCreateDenied bool + }{ + { + desc: "all allowed", + roles: map[string]types.RoleSpecV6{ + "allow": types.RoleSpecV6{ + Allow: types.RoleConditions{ + Request: &types.AccessRequestConditions{ + Roles: []string{"admins"}, + }, + ReviewRequests: &types.AccessReviewConditions{ + Roles: []string{"admins"}, + }, + }, + }, + }, + }, + { + desc: "all denied", + roles: map[string]types.RoleSpecV6{ + "allow": types.RoleSpecV6{ + Allow: types.RoleConditions{ + Request: &types.AccessRequestConditions{ + Roles: []string{"admins"}, + }, + ReviewRequests: &types.AccessReviewConditions{ + Roles: []string{"admins"}, + }, + }, + }, + "deny": types.RoleSpecV6{ + Deny: types.RoleConditions{ + Rules: []types.Rule{ + { + Resources: []string{"access_request"}, + Verbs: []string{"read", "create", "list"}, + }, + }, + }, + }, + }, + expectGetDenied: true, + expectCreateDenied: true, + }, + { + desc: "create denied", + roles: map[string]types.RoleSpecV6{ + "allow": types.RoleSpecV6{ + Allow: types.RoleConditions{ + Request: &types.AccessRequestConditions{ + Roles: []string{"admins"}, + }, + ReviewRequests: &types.AccessReviewConditions{ + Roles: []string{"admins"}, + }, + }, + }, + "deny": types.RoleSpecV6{ + Deny: types.RoleConditions{ + Rules: []types.Rule{ + { + Resources: []string{"access_request"}, + Verbs: []string{"create"}, + }, + }, + }, + }, + }, + expectCreateDenied: true, + }, + { + desc: "get denied", + roles: map[string]types.RoleSpecV6{ + "allow": types.RoleSpecV6{ + Allow: types.RoleConditions{ + Request: &types.AccessRequestConditions{ + Roles: []string{"admins"}, + }, + ReviewRequests: &types.AccessReviewConditions{ + Roles: []string{"admins"}, + }, + }, + }, + "deny": types.RoleSpecV6{ + Deny: types.RoleConditions{ + Rules: []types.Rule{ + { + Resources: []string{"access_request"}, + Verbs: []string{"read"}, + }, + }, + }, + }, + }, + expectGetDenied: true, + }, + { + desc: "list denied", + roles: map[string]types.RoleSpecV6{ + "allow": types.RoleSpecV6{ + Allow: types.RoleConditions{ + Request: &types.AccessRequestConditions{ + Roles: []string{"admins"}, + }, + ReviewRequests: &types.AccessReviewConditions{ + Roles: []string{"admins"}, + }, + }, + }, + "deny": types.RoleSpecV6{ + Deny: types.RoleConditions{ + Rules: []types.Rule{ + { + Resources: []string{"access_request"}, + Verbs: []string{"list"}, + }, + }, + }, + }, + }, + expectGetDenied: true, + }, + } { + t.Run(tc.desc, func(t *testing.T) { + for roleName, roleSpec := range tc.roles { + role, err := types.NewRole(roleName, roleSpec) + require.NoError(t, err) + _, err = testPack.tlsServer.Auth().UpsertRole(ctx, role) + require.NoError(t, err) + } + user, err := types.NewUser(userName) + require.NoError(t, err) + user.SetRoles(maps.Keys(tc.roles)) + _, err = testPack.tlsServer.Auth().UpsertUser(ctx, user) + require.NoError(t, err) + + client, err := testPack.tlsServer.NewClient(TestUser(userName)) + require.NoError(t, err) + + _, err = client.GetAccessRequests(ctx, types.AccessRequestFilter{}) + if tc.expectGetDenied { + assert.True(t, trace.IsAccessDenied(err), "want access denied, got %v", err) + } else { + assert.NoError(t, err) + } + + _, err = client.CreateAccessRequestV2(ctx, accessRequest) + if tc.expectCreateDenied { + assert.True(t, trace.IsAccessDenied(err), "want access denied, got %v", err) + } else { + assert.NoError(t, err) + } + }) + } } func testSingleAccessRequests(t *testing.T, testPack *accessRequestTestPack) { diff --git a/lib/auth/auth_with_roles.go b/lib/auth/auth_with_roles.go index c19345c9a07e1..342f01566569e 100644 --- a/lib/auth/auth_with_roles.go +++ b/lib/auth/auth_with_roles.go @@ -101,9 +101,8 @@ func (a *ServerWithRoles) actionWithContext(ctx *services.Context, namespace, re for _, verb := range verbs { errs = append(errs, a.context.Checker.CheckAccessToRule(ctx, namespace, resource, verb, false)) } - // Convert generic aggregate error to AccessDenied. if err := trace.NewAggregate(errs...); err != nil { - return trace.AccessDenied(err.Error()) + return err } return nil } @@ -137,9 +136,8 @@ func (c actionConfig) action(namespace, resource string, verbs ...string) error for _, verb := range verbs { errs = append(errs, c.context.Checker.CheckAccessToRule(&services.Context{User: c.context.User}, namespace, resource, verb, c.quiet)) } - // Convert generic aggregate error to AccessDenied. if err := trace.NewAggregate(errs...); err != nil { - return trace.AccessDenied(err.Error()) + return err } return nil } @@ -2525,16 +2523,24 @@ type accessChecker interface { } func (a *ServerWithRoles) GetAccessRequests(ctx context.Context, filter types.AccessRequestFilter) ([]types.AccessRequest, error) { - // users can always view their own access requests - if filter.User != "" && a.currentUserAction(filter.User) == nil { + if err := a.withOptions(quietAction(true)).action(apidefaults.Namespace, types.KindAccessRequest, types.VerbList, types.VerbRead); err != nil { + // Users are allowed to read + list their own access requests and + // requests they are allowed to review, unless access was *explicitly* + // denied. This means deny rules block the action but allow rules are + // not required. + if services.IsAccessExplicitlyDenied(err) { + return nil, trace.Wrap(err) + } + } else { + // nil err means the user has explicit read + list permissions and can + // get all requests. return a.authServer.GetAccessRequests(ctx, filter) } - // users with read + list permissions can get all requests - if a.withOptions(quietAction(true)).action(apidefaults.Namespace, types.KindAccessRequest, types.VerbList) == nil { - if a.withOptions(quietAction(true)).action(apidefaults.Namespace, types.KindAccessRequest, types.VerbRead) == nil { - return a.authServer.GetAccessRequests(ctx, filter) - } + // users can always view their own access requests unless the read or list + // verbs are explicitly denied + if filter.User != "" && a.currentUserAction(filter.User) == nil { + return a.authServer.GetAccessRequests(ctx, filter) } // user does not have read/list permissions and is not specifically requesting only @@ -2590,9 +2596,10 @@ func (a *ServerWithRoles) GetAccessRequests(ctx context.Context, filter types.Ac } func (a *ServerWithRoles) CreateAccessRequestV2(ctx context.Context, req types.AccessRequest) (types.AccessRequest, error) { - // An exception is made to allow users to create access *pending* requests for themselves. - if !req.GetState().IsPending() || a.currentUserAction(req.GetUser()) != nil { - if err := a.action(apidefaults.Namespace, types.KindAccessRequest, types.VerbCreate); err != nil { + if err := a.action(apidefaults.Namespace, types.KindAccessRequest, types.VerbCreate); err != nil { + // An exception is made to allow users to create *pending* access requests + // for themselves unless the create verb was explicitly denied. + if services.IsAccessExplicitlyDenied(err) || !req.GetState().IsPending() || a.currentUserAction(req.GetUser()) != nil { return nil, trace.Wrap(err) } } diff --git a/lib/authz/permissions.go b/lib/authz/permissions.go index c09c4aeacb15d..bc4253085423c 100644 --- a/lib/authz/permissions.go +++ b/lib/authz/permissions.go @@ -1197,9 +1197,8 @@ func AuthorizeContextWithVerbs(ctx context.Context, log logrus.FieldLogger, auth errs[i] = authCtx.Checker.CheckAccessToRule(ruleCtx, defaults.Namespace, kind, verb, quiet) } - // Convert generic aggregate error to AccessDenied (auth_with_roles also does this). if err := trace.NewAggregate(errs...); err != nil { - return nil, trace.AccessDenied(err.Error()) + return nil, err } return authCtx, nil } diff --git a/lib/services/role.go b/lib/services/role.go index 70dfe426a36a1..87f77fcb603a8 100644 --- a/lib/services/role.go +++ b/lib/services/role.go @@ -19,6 +19,7 @@ package services import ( "context" "encoding/json" + "errors" "fmt" "path" "regexp" @@ -2848,7 +2849,43 @@ type checkAccessParams struct { silent bool } -func (set RoleSet) checkAccessToRuleImpl(p checkAccessParams) error { +type accessExplicitlyDenied struct { + inner error +} + +// AccessExplicitlyDenied is an error type that indicates an AccessDenied error +// where a deny rule matched and access is explicitly denied, in contrast to +// cases where there is no matching deny or allow rule and access is only +// implicitly denied. +func AccessExplicitlyDenied(inner error) error { + return &accessExplicitlyDenied{inner} +} + +// IsAccessExplicitlyDenied returns true if any of the errors in err's chain is +// an AccessExplicitlyDenied error. +func IsAccessExplicitlyDenied(err error) bool { + var target *accessExplicitlyDenied + return errors.As(err, &target) +} + +func (a *accessExplicitlyDenied) Error() string { + return a.inner.Error() +} + +func (a *accessExplicitlyDenied) Unwrap() error { + return a.inner +} + +func (set RoleSet) checkAccessToRuleImpl(p checkAccessParams) (err error) { + // Every unknown error, which could be due to a bad role or an expression + // that can't parse, should be considered an explicit denial. + explicitDeny := true + defer func() { + if explicitDeny && err != nil { + err = AccessExplicitlyDenied(err) + } + }() + actionsParser, err := NewActionsParser(p.ctx) if err != nil { return trace.Wrap(err) @@ -2894,6 +2931,10 @@ func (set RoleSet) checkAccessToRuleImpl(p checkAccessParams) error { }).Infof("Access to %v %v in namespace %v denied to %v: no allow rule matched.", p.verb, p.resource, p.namespace, set) } + + // At this point no deny rule has matched and there are no more unknown + // errors, so this is only an implicit denial. + explicitDeny = false return trace.AccessDenied("access denied to perform action %q on %q", p.verb, p.resource) } From 9dfec47066ba82f5a0c61b1e860da5157ab3acf9 Mon Sep 17 00:00:00 2001 From: Nic Klaassen Date: Tue, 14 Nov 2023 13:55:21 -0800 Subject: [PATCH 2/2] fixes --- lib/auth/access_request_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/auth/access_request_test.go b/lib/auth/access_request_test.go index 855df38b85df4..8cafda37cd4f4 100644 --- a/lib/auth/access_request_test.go +++ b/lib/auth/access_request_test.go @@ -332,13 +332,13 @@ func testAccessRequestDenyRules(t *testing.T, testPack *accessRequestTestPack) { for roleName, roleSpec := range tc.roles { role, err := types.NewRole(roleName, roleSpec) require.NoError(t, err) - _, err = testPack.tlsServer.Auth().UpsertRole(ctx, role) + err = testPack.tlsServer.Auth().UpsertRole(ctx, role) require.NoError(t, err) } user, err := types.NewUser(userName) require.NoError(t, err) user.SetRoles(maps.Keys(tc.roles)) - _, err = testPack.tlsServer.Auth().UpsertUser(ctx, user) + err = testPack.tlsServer.Auth().UpsertUser(user) require.NoError(t, err) client, err := testPack.tlsServer.NewClient(TestUser(userName))