diff --git a/lib/auth/access_request_test.go b/lib/auth/access_request_test.go index ee634728f06ce..8cafda37cd4f4 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(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) }