[v14] fix!: respect deny rules for access requests

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) {

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

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
 }

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