From 8577b80cf5d49d1eac4d97920d8f2d57e9236564 Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Mon, 18 Mar 2024 09:25:43 -0400 Subject: [PATCH] acl: remove unused nil ACL object handling 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 --- nomad/acl_endpoint.go | 69 +++++++++++++++++++------------------- nomad/operator_endpoint.go | 24 ++++++------- 2 files changed, 46 insertions(+), 47 deletions(-) diff --git a/nomad/acl_endpoint.go b/nomad/acl_endpoint.go index 302ccd91872f..78c300e6b6b1 100644 --- a/nomad/acl_endpoint.go +++ b/nomad/acl_endpoint.go @@ -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 } @@ -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 } @@ -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 } @@ -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 } @@ -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 } @@ -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 } @@ -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 } } @@ -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 } @@ -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 } @@ -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 } @@ -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 } @@ -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 } @@ -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 } @@ -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 } @@ -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 } @@ -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 } @@ -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 } diff --git a/nomad/operator_endpoint.go b/nomad/operator_endpoint.go index 0bdc8a60766f..de85ca10521d 100644 --- a/nomad/operator_endpoint.go +++ b/nomad/operator_endpoint.go @@ -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 } @@ -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 } @@ -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 } @@ -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 } @@ -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 } @@ -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 }