Skip to content

Commit

Permalink
acl: remove unused nil ACL object handling
Browse files Browse the repository at this point in the history 
As of #18754 which shipped in Nomad 1.7, we no longer need to nil-check the
object returned by `ResolveACL` if there's no error return, because in the case
where ACLs are disabled we return a special "ACLs disabled" ACL object. Checking
nil is not a bug but should be discouraged because it opens us up to future bugs
that would bypass ACLs.

While working on an unrelated feature @lindleywhite discovered that we missed
removing the nil check from several endpoints with our semgrep linter. This
changeset fixes that.

Co-Author: Lindley <[email protected]>
  • Loading branch information
@tgross
tgross committed Mar 18, 2024
1 parent 695bb7f commit 8577b80
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 47 deletions.
69 changes: 34 additions & 35 deletions nomad/acl_endpoint.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,9 +94,9 @@ func (a *ACL) UpsertPolicies(args *structs.ACLPolicyUpsertRequest, reply *struct
defer metrics.MeasureSince([]string{"nomad", "acl", "upsert_policies"}, time.Now())

// Check management level permissions
if acl, err := a.srv.ResolveACL(args); err != nil {
if aclObj, err := a.srv.ResolveACL(args); err != nil {
return err
} else if acl == nil || !acl.IsManagement() {
} else if !aclObj.IsManagement() {
return structs.ErrPermissionDenied
}

Expand Down Expand Up @@ -143,9 +143,9 @@ func (a *ACL) DeletePolicies(args *structs.ACLPolicyDeleteRequest, reply *struct
defer metrics.MeasureSince([]string{"nomad", "acl", "delete_policies"}, time.Now())

// Check management level permissions
if acl, err := a.srv.ResolveACL(args); err != nil {
if aclObj, err := a.srv.ResolveACL(args); err != nil {
return err
} else if acl == nil || !acl.IsManagement() {
} else if !aclObj.IsManagement() {
return structs.ErrPermissionDenied
}

Expand Down Expand Up @@ -632,9 +632,9 @@ func (a *ACL) UpsertTokens(args *structs.ACLTokenUpsertRequest, reply *structs.A
defer metrics.MeasureSince([]string{"nomad", "acl", "upsert_tokens"}, time.Now())

// Check management level permissions
if acl, err := a.srv.ResolveACL(args); err != nil {
if aclObj, err := a.srv.ResolveACL(args); err != nil {
return err
} else if acl == nil || !acl.IsManagement() {
} else if !aclObj.IsManagement() {
return structs.ErrPermissionDenied
}

Expand Down Expand Up @@ -786,9 +786,9 @@ func (a *ACL) DeleteTokens(args *structs.ACLTokenDeleteRequest, reply *structs.G
defer metrics.MeasureSince([]string{"nomad", "acl", "delete_tokens"}, time.Now())

// Check management level permissions
if acl, err := a.srv.ResolveACL(args); err != nil {
if aclObj, err := a.srv.ResolveACL(args); err != nil {
return err
} else if acl == nil || !acl.IsManagement() {
} else if !aclObj.IsManagement() {
return structs.ErrPermissionDenied
}

Expand Down Expand Up @@ -864,9 +864,9 @@ func (a *ACL) ListTokens(args *structs.ACLTokenListRequest, reply *structs.ACLTo
defer metrics.MeasureSince([]string{"nomad", "acl", "list_tokens"}, time.Now())

// Check management level permissions
if acl, err := a.srv.ResolveACL(args); err != nil {
if aclObj, err := a.srv.ResolveACL(args); err != nil {
return err
} else if acl == nil || !acl.IsManagement() {
} else if !aclObj.IsManagement() {
return structs.ErrPermissionDenied
}

Expand Down Expand Up @@ -1019,9 +1019,9 @@ func (a *ACL) GetTokens(args *structs.ACLTokenSetRequest, reply *structs.ACLToke
defer metrics.MeasureSince([]string{"nomad", "acl", "get_tokens"}, time.Now())

// Check management level permissions
if acl, err := a.srv.ResolveACL(args); err != nil {
if aclObj, err := a.srv.ResolveACL(args); err != nil {
return err
} else if acl == nil || !acl.IsManagement() {
} else if !aclObj.IsManagement() {
return structs.ErrPermissionDenied
}

Expand Down Expand Up @@ -1232,9 +1232,9 @@ func (a *ACL) ExpireOneTimeTokens(args *structs.OneTimeTokenExpireRequest, reply

// Check management level permissions
if a.srv.config.ACLEnabled {
if acl, err := a.srv.ResolveACL(args); err != nil {
if aclObj, err := a.srv.ResolveACL(args); err != nil {
return err
} else if acl == nil || !acl.IsManagement() {
} else if !aclObj.IsManagement() {
return structs.ErrPermissionDenied
}
}
Expand Down Expand Up @@ -1282,9 +1282,9 @@ func (a *ACL) UpsertRoles(
}

// Only management level permissions can create ACL roles.
if acl, err := a.srv.ResolveACL(args); err != nil {
if aclObj, err := a.srv.ResolveACL(args); err != nil {
return err
} else if acl == nil || !acl.IsManagement() {
} else if !aclObj.IsManagement() {
return structs.ErrPermissionDenied
}

Expand Down Expand Up @@ -1426,9 +1426,9 @@ func (a *ACL) DeleteRolesByID(
}

// Only management level permissions can create ACL roles.
if acl, err := a.srv.ResolveACL(args); err != nil {
if aclObj, err := a.srv.ResolveACL(args); err != nil {
return err
} else if acl == nil || !acl.IsManagement() {
} else if !aclObj.IsManagement() {
return structs.ErrPermissionDenied
}

Expand Down Expand Up @@ -1867,9 +1867,9 @@ func (a *ACL) UpsertAuthMethods(
}

// Check management level permissions
if acl, err := a.srv.ResolveACL(args); err != nil {
if aclObj, err := a.srv.ResolveACL(args); err != nil {
return err
} else if acl == nil || !acl.IsManagement() {
} else if !aclObj.IsManagement() {
return structs.ErrPermissionDenied
}

Expand Down Expand Up @@ -1970,9 +1970,9 @@ func (a *ACL) DeleteAuthMethods(
}

// Check management level permissions
if acl, err := a.srv.ResolveACL(args); err != nil {
if aclObj, err := a.srv.ResolveACL(args); err != nil {
return err
} else if acl == nil || !acl.IsManagement() {
} else if !aclObj.IsManagement() {
return structs.ErrPermissionDenied
}

Expand Down Expand Up @@ -2059,10 +2059,9 @@ func (a *ACL) GetAuthMethod(
defer metrics.MeasureSince([]string{"nomad", "acl", "get_auth_method_name"}, time.Now())

// Resolve the token and ensure it has some form of permissions.
acl, err := a.srv.ResolveACL(args)
if err != nil {
if aclObj, err := a.srv.ResolveACL(args); err != nil {
return err
} else if acl == nil || !acl.IsManagement() {
} else if !aclObj.IsManagement() {
return structs.ErrPermissionDenied
}

Expand Down Expand Up @@ -2224,9 +2223,9 @@ func (a *ACL) UpsertBindingRules(
}

// Check management level permissions
if acl, err := a.srv.ResolveACL(args); err != nil {
if aclObj, err := a.srv.ResolveACL(args); err != nil {
return err
} else if acl == nil || !acl.IsManagement() {
} else if !aclObj.IsManagement() {
return structs.ErrPermissionDenied
}

Expand Down Expand Up @@ -2351,9 +2350,9 @@ func (a *ACL) DeleteBindingRules(
}

// Check management level permissions.
if acl, err := a.srv.ResolveACL(args); err != nil {
if aclObj, err := a.srv.ResolveACL(args); err != nil {
return err
} else if acl == nil || !acl.IsManagement() {
} else if !aclObj.IsManagement() {
return structs.ErrPermissionDenied
}

Expand Down Expand Up @@ -2393,9 +2392,9 @@ func (a *ACL) ListBindingRules(
defer metrics.MeasureSince([]string{"nomad", "acl", "list_binding_rules"}, time.Now())

// Check management level permissions.
if acl, err := a.srv.ResolveACL(args); err != nil {
if aclObj, err := a.srv.ResolveACL(args); err != nil {
return err
} else if acl == nil || !acl.IsManagement() {
} else if !aclObj.IsManagement() {
return structs.ErrPermissionDenied
}

Expand Down Expand Up @@ -2452,9 +2451,9 @@ func (a *ACL) GetBindingRules(
defer metrics.MeasureSince([]string{"nomad", "acl", "get_rules"}, time.Now())

// Check management level permissions.
if acl, err := a.srv.ResolveACL(args); err != nil {
if aclObj, err := a.srv.ResolveACL(args); err != nil {
return err
} else if acl == nil || !acl.IsManagement() {
} else if !aclObj.IsManagement() {
return structs.ErrPermissionDenied
}

Expand Down Expand Up @@ -2507,9 +2506,9 @@ func (a *ACL) GetBindingRule(
defer metrics.MeasureSince([]string{"nomad", "acl", "get_binding_rule"}, time.Now())

// Check management level permissions.
if acl, err := a.srv.ResolveACL(args); err != nil {
if aclObj, err := a.srv.ResolveACL(args); err != nil {
return err
} else if acl == nil || !acl.IsManagement() {
} else if !aclObj.IsManagement() {
return structs.ErrPermissionDenied
}

Expand Down
24 changes: 12 additions & 12 deletions nomad/operator_endpoint.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -365,11 +365,11 @@ func (op *Operator) AutopilotGetConfiguration(args *structs.GenericRequest, repl
}

// This action requires operator read access.
rule, err := op.srv.ResolveACL(args)
aclObj, err := op.srv.ResolveACL(args)
if err != nil {
return err
}
if rule != nil && !rule.AllowOperatorRead() {
if aclObj != nil && !aclObj.AllowOperatorRead() {
return structs.ErrPermissionDenied
}

Expand Down Expand Up @@ -400,11 +400,11 @@ func (op *Operator) AutopilotSetConfiguration(args *structs.AutopilotSetConfigRe
}

// This action requires operator write access.
rule, err := op.srv.ResolveACL(args)
aclObj, err := op.srv.ResolveACL(args)
if err != nil {
return err
}
if rule != nil && !rule.AllowOperatorWrite() {
if aclObj != nil && !aclObj.AllowOperatorWrite() {
return structs.ErrPermissionDenied
}

Expand Down Expand Up @@ -443,11 +443,11 @@ func (op *Operator) ServerHealth(args *structs.GenericRequest, reply *structs.Op
}

// This action requires operator read access.
rule, err := op.srv.ResolveACL(args)
aclObj, err := op.srv.ResolveACL(args)
if err != nil {
return err
}
if rule != nil && !rule.AllowOperatorRead() {
if aclObj != nil && !aclObj.AllowOperatorRead() {
return structs.ErrPermissionDenied
}

Expand Down Expand Up @@ -478,10 +478,10 @@ func (op *Operator) SchedulerSetConfiguration(args *structs.SchedulerSetConfigRe
}

// This action requires operator write access.
rule, err := op.srv.ResolveACL(args)
aclObj, err := op.srv.ResolveACL(args)
if err != nil {
return err
} else if rule != nil && !rule.AllowOperatorWrite() {
} else if aclObj != nil && !aclObj.AllowOperatorWrite() {
return structs.ErrPermissionDenied
}

Expand Down Expand Up @@ -532,10 +532,10 @@ func (op *Operator) SchedulerGetConfiguration(args *structs.GenericRequest, repl
}

// This action requires operator read access.
rule, err := op.srv.ResolveACL(args)
aclObj, err := op.srv.ResolveACL(args)
if err != nil {
return err
} else if rule != nil && !rule.AllowOperatorRead() {
} else if aclObj != nil && !aclObj.AllowOperatorRead() {
return structs.ErrPermissionDenied
}

Expand Down Expand Up @@ -803,10 +803,10 @@ func (op *Operator) UpgradeCheckVaultWorkloadIdentity(
}

// This action requires operator read access.
rule, err := op.srv.ResolveACL(args)
aclObj, err := op.srv.ResolveACL(args)
if err != nil {
return err
} else if rule != nil && !rule.AllowOperatorRead() {
} else if aclObj != nil && !aclObj.AllowOperatorRead() {
return structs.ErrPermissionDenied
}

Expand Down

0 comments on commit 8577b80

Please sign in to comment.