Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

acl: remove remaining unused nil ACL object handling #20456

Merged
merged 1 commit into from
Apr 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 2 additions & 6 deletions .semgrep/rpc_endpoint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -112,13 +112,9 @@ rules:
# authorization, as nil ACLs are always programmer errors.
- id: "rpc-authz-bypass"
patterns:
# Pattern that will accidentally bypass authorization checks.
# Pattern that may accidentally bypass authorization checks.
- pattern: |
...
if aclObj != nil && aclObj.$ACL_CHECK(...) {
...
}
...
aclObj == nil

message: "RPC method ACL check $ACL_CHECK appears to bypass authorization by first checking for nil ACLs"
languages:
Expand Down
3 changes: 1 addition & 2 deletions nomad/csi_endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -1795,8 +1795,7 @@ func (v *CSIPlugin) Get(args *structs.CSIPluginGetRequest, reply *structs.CSIPlu
return structs.ErrPermissionDenied
}

withAllocs := aclObj == nil ||
aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob)
withAllocs := aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob)

if args.ID == "" {
return fmt.Errorf("missing plugin ID")
Expand Down
2 changes: 1 addition & 1 deletion nomad/job_endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -1367,7 +1367,7 @@ func (j *Job) GetJobVersions(args *structs.JobVersionsRequest,
// Returns `nil` set if the token has access to all namespaces
// and ErrPermissionDenied if the token has no capabilities on any namespace.
func allowedNSes(aclObj *acl.ACL, state *state.StateStore, allow func(ns string) bool) (map[string]bool, error) {
if aclObj == nil || aclObj.IsManagement() {
if aclObj.IsManagement() {
return nil, nil
}

Expand Down
2 changes: 1 addition & 1 deletion nomad/namespace_endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ func (n *Namespace) ListNamespaces(args *structs.NamespaceListRequest, reply *st
ns := raw.(*structs.Namespace)

// Only return namespaces allowed by acl
if aclObj == nil || aclObj.AllowNamespace(ns.Name) {
if aclObj.AllowNamespace(ns.Name) {
reply.Namespaces = append(reply.Namespaces, ns)
}
}
Expand Down
15 changes: 6 additions & 9 deletions nomad/operator_endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -369,8 +369,7 @@ func (op *Operator) AutopilotGetConfiguration(args *structs.GenericRequest, repl
aclObj, err := op.srv.ResolveACL(args)
if err != nil {
return err
}
if aclObj != nil && !aclObj.AllowOperatorRead() {
} else if !aclObj.AllowOperatorRead() {
return structs.ErrPermissionDenied
}

Expand Down Expand Up @@ -404,8 +403,7 @@ func (op *Operator) AutopilotSetConfiguration(args *structs.AutopilotSetConfigRe
aclObj, err := op.srv.ResolveACL(args)
if err != nil {
return err
}
if aclObj != nil && !aclObj.AllowOperatorWrite() {
} else if !aclObj.AllowOperatorWrite() {
return structs.ErrPermissionDenied
}

Expand Down Expand Up @@ -447,8 +445,7 @@ func (op *Operator) ServerHealth(args *structs.GenericRequest, reply *structs.Op
aclObj, err := op.srv.ResolveACL(args)
if err != nil {
return err
}
if aclObj != nil && !aclObj.AllowOperatorRead() {
} else if !aclObj.AllowOperatorRead() {
return structs.ErrPermissionDenied
}

Expand Down Expand Up @@ -482,7 +479,7 @@ func (op *Operator) SchedulerSetConfiguration(args *structs.SchedulerSetConfigRe
aclObj, err := op.srv.ResolveACL(args)
if err != nil {
return err
} else if aclObj != nil && !aclObj.AllowOperatorWrite() {
} else if !aclObj.AllowOperatorWrite() {
return structs.ErrPermissionDenied
}

Expand Down Expand Up @@ -536,7 +533,7 @@ func (op *Operator) SchedulerGetConfiguration(args *structs.GenericRequest, repl
aclObj, err := op.srv.ResolveACL(args)
if err != nil {
return err
} else if aclObj != nil && !aclObj.AllowOperatorRead() {
} else if !aclObj.AllowOperatorRead() {
return structs.ErrPermissionDenied
}

Expand Down Expand Up @@ -807,7 +804,7 @@ func (op *Operator) UpgradeCheckVaultWorkloadIdentity(
aclObj, err := op.srv.ResolveACL(args)
if err != nil {
return err
} else if aclObj != nil && !aclObj.AllowOperatorRead() {
} else if !aclObj.AllowOperatorRead() {
return structs.ErrPermissionDenied
}

Expand Down
19 changes: 3 additions & 16 deletions nomad/search_endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,7 @@ func getResourceIter(context structs.Context, aclObj *acl.ACL, namespace, prefix
if err != nil {
return nil, err
}
if aclObj == nil || aclObj.IsManagement() {
if aclObj.IsManagement() {
return iter, nil
}
return memdb.NewFilterIterator(iter, nodePoolCapFilter(aclObj)), nil
Expand All @@ -410,18 +410,12 @@ func getResourceIter(context structs.Context, aclObj *acl.ACL, namespace, prefix
if err != nil {
return nil, err
}
if aclObj == nil {
return iter, nil
}
return memdb.NewFilterIterator(iter, nsCapFilter(aclObj)), nil
case structs.Variables:
iter, err := store.GetVariablesByPrefix(ws, prefix)
if err != nil {
return nil, err
}
if aclObj == nil {
return iter, nil
}
return memdb.NewFilterIterator(iter, nsCapFilter(aclObj)), nil
default:
return getEnterpriseResourceIter(context, aclObj, namespace, prefix, ws, store)
Expand Down Expand Up @@ -471,7 +465,7 @@ func getFuzzyResourceIterator(context structs.Context, aclObj *acl.ACL, namespac
return nil, err
}

if aclObj == nil || aclObj.IsManagement() {
if aclObj.IsManagement() {
return iter, nil
}
return memdb.NewFilterIterator(iter, nodePoolCapFilter(aclObj)), nil
Expand Down Expand Up @@ -499,9 +493,6 @@ func nsCapIterFilter(iter memdb.ResultIterator, err error, aclObj *acl.ACL) (mem
if err != nil {
return nil, err
}
if aclObj == nil {
return iter, nil
}
return memdb.NewFilterIterator(iter, nsCapFilter(aclObj)), nil
}

Expand Down Expand Up @@ -667,7 +658,7 @@ func (s *Search) PrefixSearch(args *structs.SearchRequest, reply *structs.Search
//
// Returns true if aclObj is nil or is for a management token
func sufficientSearchPerms(aclObj *acl.ACL, namespace string, context structs.Context) bool {
if aclObj == nil || aclObj.IsManagement() {
if aclObj.IsManagement() {
return true
}

Expand Down Expand Up @@ -893,10 +884,6 @@ func sufficientFuzzySearchPerms(aclObj *acl.ACL, namespace string, context struc
func filteredSearchContexts(aclObj *acl.ACL, namespace string, context structs.Context) []structs.Context {
desired := expandContext(context)

// If ACLs aren't enabled return all contexts
if aclObj == nil {
return desired
}
if aclObj.IsManagement() {
return desired
}
Expand Down
4 changes: 2 additions & 2 deletions nomad/stream/event_broker.go
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ func (e *EventBroker) handleACLUpdates(ctx context.Context) {
}

aclObj, expiryTime, err := aclObjFromSnapshotForTokenSecretID(e.aclDelegate.TokenProvider(), e.aclCache, tokenSecretID)
if err != nil || aclObj == nil {
if err != nil {
e.logger.Error("failed resolving ACL for secretID, closing subscriptions", "error", err)
e.subscriptions.closeSubscriptionsForTokens([]string{tokenSecretID})
continue
Expand Down Expand Up @@ -255,7 +255,7 @@ func (e *EventBroker) checkSubscriptionsAgainstACLChange() {
}

aclObj, expiryTime, err := aclObjFromSnapshotForTokenSecretID(aclSnapshot, e.aclCache, tokenSecretID)
if err != nil || aclObj == nil {
if err != nil {
e.logger.Debug("failed resolving ACL for secretID, closing subscriptions", "error", err)
e.subscriptions.closeSubscriptionsForTokens([]string{tokenSecretID})
continue
Expand Down
Loading