Skip to content

Commit

Permalink
Remove quiet/silent flags from authz checks (#37832)
Browse files Browse the repository at this point in the history 
* Always log role check errors to Trace level.

* Remove all quiet/silent flags from role checks.
  • Loading branch information
@Joerger
Joerger committed Feb 26, 2024
1 parent a24eee0 commit 1d00bcb
Show file tree
Hide file tree
Showing 18 changed files with 108 additions and 123 deletions.
15 changes: 7 additions & 8 deletions lib/auth/assist/assistv1/service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ func (a *Service) CreateAssistantConversation(ctx context.Context, req *assist.C
return nil, trace.Wrap(err)
}

if err := authCtx.CheckAccessToKind(true, types.KindAssistant, types.VerbCreate); err != nil {
if err := authCtx.CheckAccessToKind(types.KindAssistant, types.VerbCreate); err != nil {
return nil, trace.Wrap(err)
}

Expand All @@ -130,7 +130,7 @@ func (a *Service) UpdateAssistantConversationInfo(ctx context.Context, req *assi
return nil, trace.Wrap(err)
}

if err := authCtx.CheckAccessToKind(true, types.KindAssistant, types.VerbUpdate); err != nil {
if err := authCtx.CheckAccessToKind(types.KindAssistant, types.VerbUpdate); err != nil {
return nil, trace.Wrap(err)
}

Expand All @@ -153,7 +153,7 @@ func (a *Service) GetAssistantConversations(ctx context.Context, req *assist.Get
return nil, trace.Wrap(err)
}

if err := authCtx.CheckAccessToKind(true, types.KindAssistant, types.VerbList); err != nil {
if err := authCtx.CheckAccessToKind(types.KindAssistant, types.VerbList); err != nil {
return nil, trace.Wrap(err)
}

Expand All @@ -172,7 +172,7 @@ func (a *Service) DeleteAssistantConversation(ctx context.Context, req *assist.D
return nil, trace.Wrap(err)
}

if err := authCtx.CheckAccessToKind(true, types.KindAssistant, types.VerbDelete); err != nil {
if err := authCtx.CheckAccessToKind(types.KindAssistant, types.VerbDelete); err != nil {
return nil, trace.Wrap(err)
}

Expand All @@ -190,7 +190,7 @@ func (a *Service) GetAssistantMessages(ctx context.Context, req *assist.GetAssis
return nil, trace.Wrap(err)
}

if err := authCtx.CheckAccessToKind(true, types.KindAssistant, types.VerbRead); err != nil {
if err := authCtx.CheckAccessToKind(types.KindAssistant, types.VerbRead); err != nil {
return nil, trace.Wrap(err)
}

Expand All @@ -209,7 +209,7 @@ func (a *Service) CreateAssistantMessage(ctx context.Context, req *assist.Create
return nil, trace.Wrap(err)
}

if err := authCtx.CheckAccessToKind(true, types.KindAssistant, types.VerbCreate); err != nil {
if err := authCtx.CheckAccessToKind(types.KindAssistant, types.VerbCreate); err != nil {
return nil, trace.Wrap(err)
}

Expand Down Expand Up @@ -242,7 +242,6 @@ func (a *Service) IsAssistEnabled(ctx context.Context, _ *assist.IsAssistEnabled
checkErr := authCtx.Checker.CheckAccessToRule(
&services.Context{User: authCtx.User},
defaults.Namespace, types.KindAssistant, types.VerbRead,
false, /* silent */
)
if checkErr != nil {
return nil, trace.Wrap(err)
Expand Down Expand Up @@ -272,7 +271,7 @@ func (a *Service) GetAssistantEmbeddings(ctx context.Context, msg *assist.GetAss
return nil, trace.Wrap(err)
}

if err := authCtx.CheckAccessToKind(true, msg.Kind, types.VerbRead, types.VerbList); err != nil {
if err := authCtx.CheckAccessToKind(msg.Kind, types.VerbRead, types.VerbList); err != nil {
return nil, trace.Wrap(err)
}

Expand Down
41 changes: 17 additions & 24 deletions lib/auth/auth_with_roles.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ func (a *ServerWithRoles) actionWithContext(ctx *services.Context, namespace, re
}
var errs []error
for _, verb := range verbs {
errs = append(errs, a.context.Checker.CheckAccessToRule(ctx, namespace, resource, verb, false))
errs = append(errs, a.context.Checker.CheckAccessToRule(ctx, namespace, resource, verb))
}
if err := trace.NewAggregate(errs...); err != nil {
return err
Expand All @@ -103,18 +103,11 @@ func (a *ServerWithRoles) actionWithContext(ctx *services.Context, namespace, re
}

type actionConfig struct {
quiet bool
context authz.Context
}

type actionOption func(*actionConfig)

func quietAction(quiet bool) actionOption {
return func(cfg *actionConfig) {
cfg.quiet = quiet
}
}

func (a *ServerWithRoles) withOptions(opts ...actionOption) actionConfig {
cfg := actionConfig{context: a.context}
for _, opt := range opts {
Expand All @@ -130,7 +123,7 @@ func (c actionConfig) action(namespace, resource string, verbs ...string) error
}
var errs []error
for _, verb := range verbs {
errs = append(errs, c.context.Checker.CheckAccessToRule(&services.Context{User: c.context.User}, namespace, resource, verb, c.quiet))
errs = append(errs, c.context.Checker.CheckAccessToRule(&services.Context{User: c.context.User}, namespace, resource, verb))
}
if err := trace.NewAggregate(errs...); err != nil {
return err
Expand All @@ -150,16 +143,16 @@ func (a *ServerWithRoles) currentUserAction(username string) error {
return nil
}
return a.context.Checker.CheckAccessToRule(&services.Context{User: a.context.User},
apidefaults.Namespace, types.KindUser, types.VerbCreate, true)
apidefaults.Namespace, types.KindUser, types.VerbCreate)
}

// authConnectorAction is a special checker that grants access to auth
// connectors. It first checks if you have access to the specific connector.
// If not, it checks if the requester has the meta KindAuthConnector access
// (which grants access to all connectors).
func (a *ServerWithRoles) authConnectorAction(namespace string, resource string, verb string) error {
if err := a.context.Checker.CheckAccessToRule(&services.Context{User: a.context.User}, namespace, resource, verb, true); err != nil {
if err := a.context.Checker.CheckAccessToRule(&services.Context{User: a.context.User}, namespace, types.KindAuthConnector, verb, false); err != nil {
if err := a.context.Checker.CheckAccessToRule(&services.Context{User: a.context.User}, namespace, resource, verb); err != nil {
if err := a.context.Checker.CheckAccessToRule(&services.Context{User: a.context.User}, namespace, types.KindAuthConnector, verb); err != nil {
return trace.Wrap(err)
}
}
Expand All @@ -169,7 +162,7 @@ func (a *ServerWithRoles) authConnectorAction(namespace string, resource string,
// actionForListWithCondition extracts a restrictive filter condition to be
// added to a list query after a simple resource check fails.
func (a *ServerWithRoles) actionForListWithCondition(namespace, resource, identifier string) (*types.WhereExpr, error) {
origErr := a.withOptions(quietAction(true)).action(namespace, resource, types.VerbList)
origErr := a.action(namespace, resource, types.VerbList)
if origErr == nil || !trace.IsAccessDenied(origErr) {
return nil, trace.Wrap(origErr)
}
Expand All @@ -186,7 +179,7 @@ func (a *ServerWithRoles) actionForListWithCondition(namespace, resource, identi
// rule context after a simple resource check fails.
func (a *ServerWithRoles) actionWithExtendedContext(namespace, kind, verb string, extendContext func(*services.Context) error) error {
ruleCtx := &services.Context{User: a.context.User}
origErr := a.context.Checker.CheckAccessToRule(ruleCtx, namespace, kind, verb, true)
origErr := a.context.Checker.CheckAccessToRule(ruleCtx, namespace, kind, verb)
if origErr == nil || !trace.IsAccessDenied(origErr) {
return trace.Wrap(origErr)
}
Expand All @@ -195,7 +188,7 @@ func (a *ServerWithRoles) actionWithExtendedContext(namespace, kind, verb string
// Return the original AccessDenied to avoid leaking information.
return trace.Wrap(origErr)
}
return trace.Wrap(a.context.Checker.CheckAccessToRule(ruleCtx, namespace, kind, verb, false))
return trace.Wrap(a.context.Checker.CheckAccessToRule(ruleCtx, namespace, kind, verb))
}

// actionForKindSession is a special checker that grants access to session
Expand Down Expand Up @@ -443,13 +436,13 @@ func (a *ServerWithRoles) filterSessionTracker(ctx context.Context, joinerRoles
}

// Skip past it if there's a deny rule in place blocking access.
if err := a.context.Checker.CheckAccessToRule(ruleCtx, apidefaults.Namespace, types.KindSSHSession, verb, true /* silent */); err != nil {
if err := a.context.Checker.CheckAccessToRule(ruleCtx, apidefaults.Namespace, types.KindSSHSession, verb); err != nil {
return false
}
}

ruleCtx := &services.Context{User: a.context.User, SessionTracker: tracker}
if a.context.Checker.CheckAccessToRule(ruleCtx, apidefaults.Namespace, types.KindSessionTracker, types.VerbList, true /* silent */) == nil {
if a.context.Checker.CheckAccessToRule(ruleCtx, apidefaults.Namespace, types.KindSessionTracker, types.VerbList) == nil {
return true
}

Expand Down Expand Up @@ -965,7 +958,7 @@ func (a *ServerWithRoles) GetClusterAlerts(ctx context.Context, query types.GetC
// with permissions to view all resources of kind 'cluster_alert' can opt into viewing all alerts
// regardless of labels for management/debug purposes.
var resourceLevelPermit bool
if query.WithUntargeted && a.withOptions(quietAction(true)).action(apidefaults.Namespace, types.KindClusterAlert, types.VerbRead, types.VerbList) == nil {
if query.WithUntargeted && a.action(apidefaults.Namespace, types.KindClusterAlert, types.VerbRead, types.VerbList) == nil {
resourceLevelPermit = true
}

Expand Down Expand Up @@ -1004,7 +997,7 @@ Outer:
continue Verbs
}

if a.withOptions(quietAction(true)).action(apidefaults.Namespace, rv[0], rv[1]) == nil {
if a.action(apidefaults.Namespace, rv[0], rv[1]) == nil {
// user holds at least one of the resource:verb pairs specified by
// the verb-permit label.
filtered = append(filtered, alert)
Expand Down Expand Up @@ -1380,7 +1373,7 @@ func (a *ServerWithRoles) ListUnifiedResources(ctx context.Context, req *proto.L
actionVerbs = []string{types.VerbList}
}

resourceAccessMap[kind] = a.withOptions(quietAction(true)).action(apidefaults.Namespace, kind, actionVerbs...)
resourceAccessMap[kind] = a.action(apidefaults.Namespace, kind, actionVerbs...)
}

// Apply any requested additional search_as_roles and/or preview_as_roles
Expand Down Expand Up @@ -2310,7 +2303,7 @@ type accessChecker interface {
}

func (a *ServerWithRoles) GetAccessRequests(ctx context.Context, filter types.AccessRequestFilter) ([]types.AccessRequest, error) {
if err := a.withOptions(quietAction(true)).action(apidefaults.Namespace, types.KindAccessRequest, types.VerbList, types.VerbRead); err != nil {
if err := a.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
Expand Down Expand Up @@ -2495,12 +2488,12 @@ func (a *ServerWithRoles) GetPluginData(ctx context.Context, filter types.Plugin
case types.KindAccessRequest, types.KindAccessList:
// for backwards compatibility, we allow list/read against kinds to also grant list/read for
// access request related plugin data.
if a.withOptions(quietAction(true)).action(apidefaults.Namespace, filter.Kind, types.VerbList) != nil {
if a.action(apidefaults.Namespace, filter.Kind, types.VerbList) != nil {
if err := a.action(apidefaults.Namespace, types.KindAccessPluginData, types.VerbList); err != nil {
return nil, trace.Wrap(err)
}
}
if a.withOptions(quietAction(true)).action(apidefaults.Namespace, filter.Kind, types.VerbRead) != nil {
if a.action(apidefaults.Namespace, filter.Kind, types.VerbRead) != nil {
if err := a.action(apidefaults.Namespace, types.KindAccessPluginData, types.VerbRead); err != nil {
return nil, trace.Wrap(err)
}
Expand All @@ -2517,7 +2510,7 @@ func (a *ServerWithRoles) UpdatePluginData(ctx context.Context, params types.Plu
case types.KindAccessRequest, types.KindAccessList:
// for backwards compatibility, we allow update against access requests to also grant update for
// access request related plugin data.
if a.withOptions(quietAction(true)).action(apidefaults.Namespace, params.Kind, types.VerbUpdate) != nil {
if a.action(apidefaults.Namespace, params.Kind, types.VerbUpdate) != nil {
if err := a.action(apidefaults.Namespace, types.KindAccessPluginData, types.VerbUpdate); err != nil {
return trace.Wrap(err)
}
Expand Down
14 changes: 7 additions & 7 deletions lib/auth/discoveryconfig/discoveryconfigv1/service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ func (s *Service) ListDiscoveryConfigs(ctx context.Context, req *discoveryconfig
return nil, trace.Wrap(err)
}

if err := authCtx.CheckAccessToKind(true, types.KindDiscoveryConfig, types.VerbRead, types.VerbList); err != nil {
if err := authCtx.CheckAccessToKind(types.KindDiscoveryConfig, types.VerbRead, types.VerbList); err != nil {
return nil, trace.Wrap(err)
}

Expand All @@ -128,7 +128,7 @@ func (s *Service) GetDiscoveryConfig(ctx context.Context, req *discoveryconfigv1
return nil, trace.Wrap(err)
}

if err := authCtx.CheckAccessToKind(true, types.KindDiscoveryConfig, types.VerbRead); err != nil {
if err := authCtx.CheckAccessToKind(types.KindDiscoveryConfig, types.VerbRead); err != nil {
return nil, trace.Wrap(err)
}

Expand All @@ -147,7 +147,7 @@ func (s *Service) CreateDiscoveryConfig(ctx context.Context, req *discoveryconfi
return nil, trace.Wrap(err)
}

if err := authCtx.CheckAccessToKind(true, types.KindDiscoveryConfig, types.VerbCreate); err != nil {
if err := authCtx.CheckAccessToKind(types.KindDiscoveryConfig, types.VerbCreate); err != nil {
return nil, trace.Wrap(err)
}

Expand All @@ -171,7 +171,7 @@ func (s *Service) UpdateDiscoveryConfig(ctx context.Context, req *discoveryconfi
return nil, trace.Wrap(err)
}

if err := authCtx.CheckAccessToKind(true, types.KindDiscoveryConfig, types.VerbUpdate); err != nil {
if err := authCtx.CheckAccessToKind(types.KindDiscoveryConfig, types.VerbUpdate); err != nil {
return nil, trace.Wrap(err)
}

Expand All @@ -195,7 +195,7 @@ func (s *Service) UpsertDiscoveryConfig(ctx context.Context, req *discoveryconfi
return nil, trace.Wrap(err)
}

if err := authCtx.CheckAccessToKind(true, types.KindDiscoveryConfig, types.VerbCreate, types.VerbUpdate); err != nil {
if err := authCtx.CheckAccessToKind(types.KindDiscoveryConfig, types.VerbCreate, types.VerbUpdate); err != nil {
return nil, trace.Wrap(err)
}

Expand All @@ -219,7 +219,7 @@ func (s *Service) DeleteDiscoveryConfig(ctx context.Context, req *discoveryconfi
return nil, trace.Wrap(err)
}

if err := authCtx.CheckAccessToKind(true, types.KindDiscoveryConfig, types.VerbDelete); err != nil {
if err := authCtx.CheckAccessToKind(types.KindDiscoveryConfig, types.VerbDelete); err != nil {
return nil, trace.Wrap(err)
}

Expand All @@ -237,7 +237,7 @@ func (s *Service) DeleteAllDiscoveryConfigs(ctx context.Context, _ *discoverycon
return nil, trace.Wrap(err)
}

if err := authCtx.CheckAccessToKind(true, types.KindDiscoveryConfig, types.VerbDelete); err != nil {
if err := authCtx.CheckAccessToKind(types.KindDiscoveryConfig, types.VerbDelete); err != nil {
return nil, trace.Wrap(err)
}

Expand Down
6 changes: 3 additions & 3 deletions lib/auth/integration/integrationv1/awsoidc.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ func (s *Service) GenerateAWSOIDCToken(ctx context.Context, _ *integrationpb.Gen
return nil, trace.Wrap(err)
}

if err := authCtx.CheckAccessToKind(true, types.KindIntegration, types.VerbUse); err != nil {
if err := authCtx.CheckAccessToKind(types.KindIntegration, types.VerbUse); err != nil {
return nil, trace.Wrap(err)
}
return s.generateAWSOIDCTokenWithoutAuthZ(ctx)
Expand Down Expand Up @@ -207,7 +207,7 @@ func (s *AWSOIDCService) ListDatabases(ctx context.Context, req *integrationpb.L
return nil, trace.Wrap(err)
}

if err := authCtx.CheckAccessToKind(true, types.KindIntegration, types.VerbUse); err != nil {
if err := authCtx.CheckAccessToKind(types.KindIntegration, types.VerbUse); err != nil {
return nil, trace.Wrap(err)
}

Expand Down Expand Up @@ -254,7 +254,7 @@ func (s *AWSOIDCService) DeployDatabaseService(ctx context.Context, req *integra
return nil, trace.Wrap(err)
}

if err := authCtx.CheckAccessToKind(true, types.KindIntegration, types.VerbUse); err != nil {
if err := authCtx.CheckAccessToKind(types.KindIntegration, types.VerbUse); err != nil {
return nil, trace.Wrap(err)
}

Expand Down
12 changes: 6 additions & 6 deletions lib/auth/integration/integrationv1/service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ func (s *Service) ListIntegrations(ctx context.Context, req *integrationpb.ListI
return nil, trace.Wrap(err)
}

if err := authCtx.CheckAccessToKind(true, types.KindIntegration, types.VerbRead, types.VerbList); err != nil {
if err := authCtx.CheckAccessToKind(types.KindIntegration, types.VerbRead, types.VerbList); err != nil {
return nil, trace.Wrap(err)
}

Expand Down Expand Up @@ -164,7 +164,7 @@ func (s *Service) GetIntegration(ctx context.Context, req *integrationpb.GetInte
return nil, trace.Wrap(err)
}

if err := authCtx.CheckAccessToKind(true, types.KindIntegration, types.VerbRead); err != nil {
if err := authCtx.CheckAccessToKind(types.KindIntegration, types.VerbRead); err != nil {
return nil, trace.Wrap(err)
}
integration, err := s.cache.GetIntegration(ctx, req.GetName())
Expand All @@ -187,7 +187,7 @@ func (s *Service) CreateIntegration(ctx context.Context, req *integrationpb.Crea
return nil, trace.Wrap(err)
}

if err := authCtx.CheckAccessToKind(true, types.KindIntegration, types.VerbCreate); err != nil {
if err := authCtx.CheckAccessToKind(types.KindIntegration, types.VerbCreate); err != nil {
return nil, trace.Wrap(err)
}

Expand All @@ -211,7 +211,7 @@ func (s *Service) UpdateIntegration(ctx context.Context, req *integrationpb.Upda
return nil, trace.Wrap(err)
}

if err := authCtx.CheckAccessToKind(true, types.KindIntegration, types.VerbUpdate); err != nil {
if err := authCtx.CheckAccessToKind(types.KindIntegration, types.VerbUpdate); err != nil {
return nil, trace.Wrap(err)
}

Expand All @@ -235,7 +235,7 @@ func (s *Service) DeleteIntegration(ctx context.Context, req *integrationpb.Dele
return nil, trace.Wrap(err)
}

if err := authCtx.CheckAccessToKind(true, types.KindIntegration, types.VerbDelete); err != nil {
if err := authCtx.CheckAccessToKind(types.KindIntegration, types.VerbDelete); err != nil {
return nil, trace.Wrap(err)
}

Expand All @@ -253,7 +253,7 @@ func (s *Service) DeleteAllIntegrations(ctx context.Context, _ *integrationpb.De
return nil, trace.Wrap(err)
}

if err := authCtx.CheckAccessToKind(true, types.KindIntegration, types.VerbDelete); err != nil {
if err := authCtx.CheckAccessToKind(types.KindIntegration, types.VerbDelete); err != nil {
return nil, trace.Wrap(err)
}

Expand Down
Loading

0 comments on commit 1d00bcb

Please sign in to comment.