From 32a3adbe51b8f5fa0f46d08faafd34a417825651 Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Thu, 12 Oct 2023 17:09:46 -0400 Subject: [PATCH] auth: use `ACLsDisabledACL` when ACLs are disabled The RPC handlers expect to see `nil` ACL objects whenever ACLs are disabled. By using `nil` as a sentinel value, we have the risk of nil pointer exceptions and improper handling of `nil` when returned from our various auth methods that can lead to privilege escalation bugs. This is the final patch in a series to eliminate the use of `nil` ACLs as a sentinel value for when ACLs are disabled. This patch adds a new virtual ACL policy field for when ACLs are disabled and updates our authentication logic to use it. Included: * Extends auth package tests to demonstrate that nil ACLs are treated as failed auth and disabled ACLs succeed auth. * Adds a new `AllowDebug` ACL check for the weird special casing we have for pprof debugging when ACLs are disabled. * Removes the remaining unexported methods (and repeated tests) from the `nomad/acl.go` file. * Update the semgrep rules to detect improper nil ACL checking and remove the old invalid ACL checks. * Update the contributing guide for RPC authentication. Ref: https://github.com/hashicorp/nomad-enterprise/pull/1218 Ref: https://github.com/hashicorp/nomad/pull/18703 Ref: https://github.com/hashicorp/nomad/pull/18715 Ref: https://github.com/hashicorp/nomad/pull/16799 Ref: https://github.com/hashicorp/nomad/pull/18730 Ref: https://github.com/hashicorp/nomad/pull/18744 --- .semgrep/rpc_endpoint.yml | 79 ++---- acl/acl.go | 167 +++++++---- acl/acl_test.go | 65 +++++ acl/virtual.go | 10 + client/acl.go | 2 +- client/acl_test.go | 5 +- client/agent_endpoint.go | 10 +- client/alloc_endpoint.go | 16 +- client/alloc_watcher_e2e_test.go | 1 + client/client_stats_endpoint.go | 2 +- client/fs_endpoint.go | 23 +- command/agent/agent_endpoint.go | 21 +- command/agent/http_test.go | 4 +- command/agent/job_endpoint.go | 12 +- contributing/checklist-rpc-endpoint.md | 13 +- nomad/acl.go | 4 - nomad/acl_test.go | 369 ------------------------- nomad/alloc_endpoint.go | 12 +- nomad/auth/auth.go | 49 ++-- nomad/auth/auth_test.go | 83 ++++-- nomad/client_agent_endpoint.go | 14 +- nomad/client_agent_endpoint_test.go | 3 +- nomad/client_alloc_endpoint.go | 14 +- nomad/client_fs_endpoint.go | 4 +- nomad/client_meta_endpoint.go | 4 +- nomad/client_stats_endpoint.go | 2 +- nomad/csi_endpoint.go | 2 +- nomad/deployment_endpoint.go | 17 +- nomad/eval_endpoint.go | 2 +- nomad/job_endpoint.go | 147 +++++----- nomad/keyring_endpoint.go | 10 +- nomad/namespace_endpoint.go | 8 +- nomad/node_endpoint.go | 39 +-- nomad/operator_endpoint.go | 12 +- nomad/periodic_endpoint.go | 2 +- nomad/scaling_endpoint.go | 4 +- nomad/status_endpoint.go | 2 +- nomad/structs/structs.go | 31 ++- nomad/system_endpoint.go | 8 +- nomad/variables_endpoint.go | 28 +- 40 files changed, 535 insertions(+), 765 deletions(-) diff --git a/.semgrep/rpc_endpoint.yml b/.semgrep/rpc_endpoint.yml index c4ad201d9f3e..ae9f2696d316 100644 --- a/.semgrep/rpc_endpoint.yml +++ b/.semgrep/rpc_endpoint.yml @@ -2,9 +2,7 @@ # SPDX-License-Identifier: BUSL-1.1 rules: - # Check potentially unauthenticated RPC endpoints. Technically more - # authorization (authz) oriented than authn, but before Nomad 1.4/1.5 that - # distinction wasn't as important. + # Check potentially RPC endpoints with missing authentication/authorization. - id: "rpc-potentially-unauthenticated" patterns: - pattern: | @@ -40,7 +38,6 @@ rules: } ... - # Pattern used by endpoints that are used only for client-to-server. # Authorization can be done after forwarding, but must check the # AllowClientOp policy @@ -56,7 +53,6 @@ rules: } ... - # Pattern used by endpoints that are used only for client-to-server. # Authorization can be done after forwarding, but must check the # AllowClientOp policy. This should not be added to any new endpoints. @@ -72,7 +68,8 @@ rules: } ... - # Pattern used by ACL endpoints that need to interact with the token directly + # Pattern used by ACL endpoints that need to interact with the token + # directly. - pattern-not-inside: | authErr := $A.$B.Authenticate($A.ctx, args) ... @@ -82,52 +79,7 @@ rules: ... ... := args.GetIdentity().GetACLToken() ... - # Pattern used by endpoints called exclusively between agents - # (server -> server or client -> server) - - pattern-not-inside: | - authErr := $A.$B.Authenticate($A.ctx, args) - ... - ... := validateTLSCertificateLevel(...) - ... - if done, err := $A.$B.forward($METHOD, ...); done { - return err - } - # Pattern used by endpoints that support both normal ACLs and workload - # identity but break authentication and authorization up - # TODO: currently this is just for Variables and should be removed once - # https://github.com/hashicorp/nomad/issues/15875 is complete. - - pattern-not-inside: | - authErr := $A.$B.Authenticate($A.ctx, args) - ... - if done, err := $A.$B.forward($METHOD, ...); done { - return err - } - ... - ... := $T.handleMixedAuthEndpoint(...) - ... - # Second pattern used by endpoints that support both normal ACLs and - # workload identity but break authentication and authorization up - # TODO: currently this is just for Variables and should be removed once - # https://github.com/hashicorp/nomad/issues/15875 is complete. - - pattern-not-inside: | - authErr := $A.$B.Authenticate($A.ctx, args) - ... - if done, err := $A.$B.forward($METHOD, ...); done { - return err - } - ... - ... := svePreApply($A, args, args.Var) - ... - # Pattern used by some Node endpoints. - - pattern-not-inside: | - authErr := $A.$B.Authenticate($A.ctx, args) - ... - if done, err := $A.$B.forward($METHOD, ...); done { - return err - } - ... - return $A.deregister(...) - ... + - metavariable-pattern: metavariable: $METHOD patterns: @@ -142,8 +94,6 @@ rules: - pattern-not: 'structs.ACLOIDCAuthURLRPCMethod' - pattern-not: 'structs.ACLOIDCCompleteAuthRPCMethod' - pattern-not: 'structs.ACLLoginRPCMethod' - - pattern-not: '"CSIPlugin.Get"' - - pattern-not: '"CSIPlugin.List"' - pattern-not: '"Status.Leader"' - pattern-not: '"Status.Peers"' - pattern-not: '"Status.Version"' @@ -155,3 +105,24 @@ rules: paths: include: - "nomad/*_endpoint.go" + + + # ACL objects should never be nil-checked in RPC handlers before checking + # authorization, as nil ACLs are always programmer errors. + - id: "rpc-authz-bypass" + patterns: + # Pattern that will accidentally bypass authorization checks. + - pattern: | + ... + if aclObj != nil && aclObj.$ACL_CHECK(...) { + ... + } + ... + + message: "RPC method ACL check $ACL_CHECK appears to bypass authorization by first checking for nil ACLs" + languages: + - "go" + severity: "WARNING" + paths: + include: + - "nomad/*_endpoint.go" diff --git a/acl/acl.go b/acl/acl.go index c4c9a2370074..8ed59b66d47a 100644 --- a/acl/acl.go +++ b/acl/acl.go @@ -78,9 +78,10 @@ type ACL struct { // The attributes below detail a virtual policy that we never expose // directly to the end user. - client string - server string - isLeader bool + client string + server string + isLeader bool + aclsDisabled bool } // maxPrivilege returns the policy which grants the most privilege @@ -98,6 +99,7 @@ func maxPrivilege(a, b string) string { default: return "" } + } // NewACL compiles a set of policies into an ACL object @@ -305,6 +307,7 @@ func NewACL(management bool, policies []*Policy) (*ACL, error) { acl.client = PolicyDeny acl.server = PolicyDeny acl.isLeader = false + acl.aclsDisabled = false return acl, nil } @@ -324,13 +327,12 @@ func (a *ACL) AllowNsOpFunc(ops ...string) func(string) bool { // AllowNamespaceOperation checks if a given operation is allowed for a namespace. func (a *ACL) AllowNamespaceOperation(ns string, op string) bool { - // Hot path if ACL is not enabled. if a == nil { - return true + return false } - // Hot path management tokens - if a.management { + // Hot path management tokens or when ACLs are disabled + if a.aclsDisabled || a.management { return true } @@ -357,13 +359,12 @@ func (a *ACL) AllowNamespaceOperation(ns string, op string) bool { // AllowNamespace checks if any operations are allowed for a namespace func (a *ACL) AllowNamespace(ns string) bool { - // Hot path if ACL is not enabled. if a == nil { - return true + return false } - // Hot path management tokens - if a.management { + // Hot path management tokens or when ACLs are disabled + if a.aclsDisabled || a.management { return true } @@ -390,8 +391,12 @@ func (a *ACL) AllowNamespace(ns string) bool { // AllowNodePoolOperation returns true if the given operation is allowed in the // node pool specified. func (a *ACL) AllowNodePoolOperation(pool string, op string) bool { - // Hot path if ACL is not enabled or if it's a management token. - if a == nil || a.management { + if a == nil { + return false + } + + // Hot path management tokens or when ACLs are disabled + if a.aclsDisabled || a.management { return true } @@ -407,8 +412,12 @@ func (a *ACL) AllowNodePoolOperation(pool string, op string) bool { // AllowNodePool returns true if any operation is allowed for the node pool. func (a *ACL) AllowNodePool(pool string) bool { - // Hot path if ACL is not enabled or if it's a management token. - if a == nil || a.management { + if a == nil { + return false + } + + // Hot path management tokens or when ACLs are disabled + if a.aclsDisabled || a.management { return true } @@ -431,8 +440,12 @@ func (a *ACL) AllowNodePool(pool string) bool { // This is a very loose check and is expected that callers perform more precise // verification later. func (a *ACL) AllowNodePoolSearch() bool { - // Hot path if ACL is not enabled or token is management. - if a == nil || a.management { + if a == nil { + return false + } + + // Hot path management tokens or when ACLs are disabled + if a.aclsDisabled || a.management { return true } @@ -456,8 +469,12 @@ func (a *ACL) AllowNodePoolSearch() bool { // AllowHostVolumeOperation checks if a given operation is allowed for a host volume func (a *ACL) AllowHostVolumeOperation(hv string, op string) bool { - // Hot path management tokens - if a.management { + if a == nil { + return false + } + + // Hot path management tokens or when ACLs are disabled + if a.aclsDisabled || a.management { return true } @@ -473,8 +490,12 @@ func (a *ACL) AllowHostVolumeOperation(hv string, op string) bool { // AllowHostVolume checks if any operations are allowed for a HostVolume func (a *ACL) AllowHostVolume(ns string) bool { - // Hot path management tokens - if a.management { + if a == nil { + return false + } + + // Hot path management tokens or when ACLs are disabled + if a.aclsDisabled || a.management { return true } @@ -494,11 +515,11 @@ func (a *ACL) AllowHostVolume(ns string) bool { func (a *ACL) AllowVariableOperation(ns, path, op string, claim *ACLClaim) bool { if a == nil { - // ACL is nil only if ACLs are disabled - // TODO(tgross): return false when there are no nil ACLs - return true + return false } - if a.management { + + // Hot path management tokens or when ACLs are disabled + if a.aclsDisabled || a.management { return true } @@ -523,13 +544,14 @@ type ACLClaim struct { // search result will be filtered by specific paths func (a *ACL) AllowVariableSearch(ns string) bool { if a == nil { - // ACL is nil only if ACLs are disabled - // TODO(tgross): return false when there are no nil ACLs - return true + return false } - if a.management { + + // Hot path management tokens or when ACLs are disabled + if a.aclsDisabled || a.management { return true } + if ns == "*" { return a.variables.Len() > 0 || a.wildcardVariables.Len() > 0 } @@ -713,7 +735,9 @@ func findAllMatchingWildcards(radix *iradix.Tree[capabilitySet], name string) [] // AllowAgentRead checks if read operations are allowed for an agent func (a *ACL) AllowAgentRead() bool { switch { - case a.management: + case a == nil: + return false + case a.aclsDisabled, a.management: return true case a.agent == PolicyWrite: return true @@ -727,10 +751,32 @@ func (a *ACL) AllowAgentRead() bool { // AllowAgentWrite checks if write operations are allowed for an agent func (a *ACL) AllowAgentWrite() bool { switch { + case a == nil: + return false + case a.aclsDisabled, a.management: + return true + case a.agent == PolicyWrite: + return true + default: + return false + } +} + +// AllowAgentDebug checks if debug operations are allowed for an agent. This is +// a special case of AllowAgentRead because we don't allow debug if ACLs are +// disabled unless the debug flag is set in the agent config. +func (a *ACL) AllowAgentDebug(isDebugEnabled bool) bool { + switch { + case a == nil: + return false case a.management: return true case a.agent == PolicyWrite: return true + case a.agent == PolicyRead: + return true + case a.aclsDisabled: + return isDebugEnabled default: return false } @@ -739,10 +785,9 @@ func (a *ACL) AllowAgentWrite() bool { // AllowNodeRead checks if read operations are allowed for a node func (a *ACL) AllowNodeRead() bool { switch { - // a is nil if ACLs are disabled. case a == nil: - return true - case a.management: + return false + case a.aclsDisabled, a.management: return true case a.node == PolicyWrite: return true @@ -764,7 +809,9 @@ func (a *ACL) AllowNodeRead() bool { // AllowNodeWrite checks if write operations are allowed for a node func (a *ACL) AllowNodeWrite() bool { switch { - case a.management: + case a == nil: + return false + case a.aclsDisabled, a.management: return true case a.node == PolicyWrite: return true @@ -776,7 +823,9 @@ func (a *ACL) AllowNodeWrite() bool { // AllowOperatorRead checks if read operations are allowed for a operator func (a *ACL) AllowOperatorRead() bool { switch { - case a.management: + case a == nil: + return false + case a.aclsDisabled, a.management: return true case a.operator == PolicyWrite: return true @@ -790,7 +839,9 @@ func (a *ACL) AllowOperatorRead() bool { // AllowOperatorWrite checks if write operations are allowed for a operator func (a *ACL) AllowOperatorWrite() bool { switch { - case a.management: + case a == nil: + return false + case a.aclsDisabled, a.management: return true case a.operator == PolicyWrite: return true @@ -802,7 +853,9 @@ func (a *ACL) AllowOperatorWrite() bool { // AllowQuotaRead checks if read operations are allowed for all quotas func (a *ACL) AllowQuotaRead() bool { switch { - case a.management: + case a == nil: + return false + case a.aclsDisabled, a.management: return true case a.quota == PolicyWrite: return true @@ -816,7 +869,9 @@ func (a *ACL) AllowQuotaRead() bool { // AllowQuotaWrite checks if write operations are allowed for quotas func (a *ACL) AllowQuotaWrite() bool { switch { - case a.management: + case a == nil: + return false + case a.aclsDisabled, a.management: return true case a.quota == PolicyWrite: return true @@ -828,10 +883,9 @@ func (a *ACL) AllowQuotaWrite() bool { // AllowPluginRead checks if read operations are allowed for all plugins func (a *ACL) AllowPluginRead() bool { switch { - // ACL is nil only if ACLs are disabled case a == nil: - return true - case a.management: + return false + case a.aclsDisabled, a.management: return true case a.client == PolicyRead, a.client == PolicyWrite: @@ -846,10 +900,9 @@ func (a *ACL) AllowPluginRead() bool { // AllowPluginList checks if list operations are allowed for all plugins func (a *ACL) AllowPluginList() bool { switch { - // ACL is nil only if ACLs are disabled case a == nil: - return true - case a.management: + return false + case a.aclsDisabled, a.management: return true case a.client == PolicyRead, a.client == PolicyWrite: @@ -864,9 +917,10 @@ func (a *ACL) AllowPluginList() bool { } func (a *ACL) AllowServiceRegistrationReadList(ns string, isWorkload bool) bool { - if a == nil { - // ACL is nil only if ACLs are disabled - // TODO(tgross): return false when there are no nil ACLs + switch { + case a == nil: + return false + case a.aclsDisabled, a.management: return true } return isWorkload || a.AllowNsOp(ns, NamespaceCapabilityReadJob) @@ -875,25 +929,24 @@ func (a *ACL) AllowServiceRegistrationReadList(ns string, isWorkload bool) bool // AllowServerOp checks if server-only operations are allowed func (a *ACL) AllowServerOp() bool { if a == nil { - // ACL is nil only if ACLs are disabled - // TODO(tgross): return false when there are no nil ACLs - return true + return false } return a.server != PolicyDeny || a.isLeader } func (a *ACL) AllowClientOp() bool { if a == nil { - // ACL is nil only if ACLs are disabled - // TODO(tgross): return false when there are no nil ACLs - return true + return false } return a.client != PolicyDeny } // IsManagement checks if this represents a management token func (a *ACL) IsManagement() bool { - return a.management + if a == nil { + return false + } + return a.management || a.aclsDisabled } // NamespaceValidator returns a func that wraps ACL.AllowNamespaceOperation in @@ -901,10 +954,14 @@ func (a *ACL) IsManagement() bool { // *any* capabilities match. func NamespaceValidator(ops ...string) func(*ACL, string) bool { return func(a *ACL, ns string) bool { - // Always allow if ACLs are disabled. if a == nil { + return false + } + // Hot path for management tokens or when ACLs are disabled + if a.aclsDisabled || a.management { return true } + // Clients need to be able to read namespaced objects if a.client != PolicyDeny { return true diff --git a/acl/acl_test.go b/acl/acl_test.go index a0332f6559c2..cf0c4bda3f45 100644 --- a/acl/acl_test.go +++ b/acl/acl_test.go @@ -1030,3 +1030,68 @@ func TestACL_matchingCapabilitySet_difference(t *testing.T) { } } + +func TestAgentDebug(t *testing.T) { + ci.Parallel(t) + + testCases := []struct { + name string + policy string + aclsDisabled bool + isDebugEnabled bool + expect bool + }{ + { + name: "policy read debug not enabled", + policy: `agent { policy = "read" }`, + isDebugEnabled: false, + expect: true, + }, + { + name: "policy read debug enabled", + policy: `agent { policy = "read" }`, + isDebugEnabled: true, + expect: true, + }, + { + name: "policy no read debug enabled", + policy: `node { policy = "read" }`, + isDebugEnabled: true, + expect: false, + }, + { + name: "policy no read debug not enabled", + policy: `node { policy = "read" }`, + isDebugEnabled: false, + expect: false, + }, + { + name: "no acls debug enabled", + aclsDisabled: true, + isDebugEnabled: true, + expect: true, + }, + { + name: "no acls debug not enabled", + aclsDisabled: true, + isDebugEnabled: false, + expect: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + + acl := ACLsDisabledACL + if !tc.aclsDisabled { + policy, err := Parse(tc.policy) + must.NoError(t, err) + + acl, err = NewACL(false, []*Policy{policy}) + must.NoError(t, err) + } + + must.Eq(t, tc.expect, acl.AllowAgentDebug(tc.isDebugEnabled)) + }) + } +} diff --git a/acl/virtual.go b/acl/virtual.go index fa136a3cf799..73b0e84164e9 100644 --- a/acl/virtual.go +++ b/acl/virtual.go @@ -5,6 +5,7 @@ package acl var ClientACL = initClientACL() var ServerACL = initServerACL() +var ACLsDisabledACL = initACLsDisabledACL() func initClientACL() *ACL { aclObj, err := NewACL(false, []*Policy{}) @@ -26,3 +27,12 @@ func initServerACL() *ACL { aclObj.server = PolicyWrite return aclObj } + +func initACLsDisabledACL() *ACL { + aclObj, err := NewACL(false, []*Policy{}) + if err != nil { + panic(err) + } + aclObj.aclsDisabled = true + return aclObj +} diff --git a/client/acl.go b/client/acl.go index 72b1283bb844..b4a5e0f5ec5c 100644 --- a/client/acl.go +++ b/client/acl.go @@ -67,7 +67,7 @@ func (c *Client) ResolveToken(bearerToken string) (*acl.ACL, error) { func (c *Client) resolveTokenAndACL(bearerToken string) (*acl.ACL, *structs.AuthenticatedIdentity, error) { // Fast-path if ACLs are disabled if !c.GetConfig().ACLEnabled { - return nil, nil, nil + return acl.ACLsDisabledACL, nil, nil } defer metrics.MeasureSince([]string{"client", "acl", "resolve_token"}, time.Now()) diff --git a/client/acl_test.go b/client/acl_test.go index 26d8a8986d2b..41b96fd53502 100644 --- a/client/acl_test.go +++ b/client/acl_test.go @@ -190,10 +190,11 @@ func TestClient_ACL_ResolveToken_Disabled(t *testing.T) { }) defer cleanup() - // Should always get nil when disabled + // Should never get nil, even when disabled aclObj, err := c1.ResolveToken("blah") must.NoError(t, err) - must.Nil(t, aclObj) + must.NotNil(t, aclObj) + must.Eq(t, acl.ACLsDisabledACL, aclObj) } func TestClient_ACL_ResolveToken(t *testing.T) { diff --git a/client/agent_endpoint.go b/client/agent_endpoint.go index 527805ea918d..28b61051dfb9 100644 --- a/client/agent_endpoint.go +++ b/client/agent_endpoint.go @@ -40,12 +40,11 @@ func (a *Agent) Profile(args *structs.AgentPprofRequest, reply *structs.AgentPpr aclObj, err := a.c.ResolveToken(args.AuthToken) if err != nil { return err - } else if aclObj != nil && !aclObj.AllowAgentWrite() { + } else if !aclObj.AllowAgentWrite() { return structs.ErrPermissionDenied } - // If ACLs are disabled, EnableDebug must be enabled - if aclObj == nil && !a.c.GetConfig().EnableDebug { + if !aclObj.AllowAgentDebug(a.c.GetConfig().EnableDebug) { return structs.ErrPermissionDenied } @@ -100,7 +99,7 @@ func (a *Agent) monitor(conn io.ReadWriteCloser) { if aclObj, err := a.c.ResolveToken(args.AuthToken); err != nil { handleStreamResultError(err, pointer.Of(int64(403)), encoder) return - } else if aclObj != nil && !aclObj.AllowAgentRead() { + } else if !aclObj.AllowAgentRead() { handleStreamResultError(structs.ErrPermissionDenied, pointer.Of(int64(403)), encoder) return } @@ -220,8 +219,7 @@ func (a *Agent) Host(args *structs.HostDataRequest, reply *structs.HostDataRespo if err != nil { return err } - if (aclObj != nil && !aclObj.AllowAgentRead()) || - (aclObj == nil && !a.c.GetConfig().EnableDebug) { + if !aclObj.AllowAgentRead() && !a.c.GetConfig().EnableDebug { return structs.ErrPermissionDenied } diff --git a/client/alloc_endpoint.go b/client/alloc_endpoint.go index 386bf6b0837c..8eae2da78a32 100644 --- a/client/alloc_endpoint.go +++ b/client/alloc_endpoint.go @@ -39,7 +39,7 @@ func (a *Allocations) GarbageCollectAll(args *nstructs.NodeSpecificRequest, repl // Check node write permissions if aclObj, err := a.c.ResolveToken(args.AuthToken); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNodeWrite() { + } else if !aclObj.AllowNodeWrite() { return nstructs.ErrPermissionDenied } @@ -59,7 +59,7 @@ func (a *Allocations) GarbageCollect(args *nstructs.AllocSpecificRequest, reply // Check namespace submit job permission. if aclObj, err := a.c.ResolveToken(args.AuthToken); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilitySubmitJob) { + } else if !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilitySubmitJob) { return nstructs.ErrPermissionDenied } @@ -82,7 +82,7 @@ func (a *Allocations) Signal(args *nstructs.AllocSignalRequest, reply *nstructs. // Check namespace alloc-lifecycle permission. if aclObj, err := a.c.ResolveToken(args.AuthToken); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityAllocLifecycle) { + } else if !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityAllocLifecycle) { return nstructs.ErrPermissionDenied } @@ -101,7 +101,7 @@ func (a *Allocations) Restart(args *nstructs.AllocRestartRequest, reply *nstruct // Check namespace alloc-lifecycle permission. if aclObj, err := a.c.ResolveToken(args.AuthToken); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityAllocLifecycle) { + } else if !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityAllocLifecycle) { return nstructs.ErrPermissionDenied } @@ -120,7 +120,7 @@ func (a *Allocations) Stats(args *cstructs.AllocStatsRequest, reply *cstructs.Al // Check read-job permission. if aclObj, err := a.c.ResolveToken(args.AuthToken); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityReadJob) { + } else if !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityReadJob) { return nstructs.ErrPermissionDenied } @@ -152,7 +152,7 @@ func (a *Allocations) Checks(args *cstructs.AllocChecksRequest, reply *cstructs. // Check read-job permission if aclObj, aclErr := a.c.ResolveToken(args.AuthToken); aclErr != nil { return aclErr - } else if aclObj != nil && !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityReadJob) { + } else if !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityReadJob) { return nstructs.ErrPermissionDenied } @@ -239,7 +239,7 @@ func (a *Allocations) execImpl(encoder *codec.Encoder, decoder *codec.Decoder, e // Check alloc-exec permission. if err != nil { return nil, err - } else if aclObj != nil && !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityAllocExec) { + } else if !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityAllocExec) { return nil, nstructs.ErrPermissionDenied } @@ -262,7 +262,7 @@ func (a *Allocations) execImpl(encoder *codec.Encoder, decoder *codec.Decoder, e } // check node access - if aclObj != nil && capabilities.FSIsolation == drivers.FSIsolationNone { + if capabilities.FSIsolation == drivers.FSIsolationNone { exec := aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityAllocNodeExec) if !exec { return nil, nstructs.ErrPermissionDenied diff --git a/client/alloc_watcher_e2e_test.go b/client/alloc_watcher_e2e_test.go index 5ef35f384d1c..af9a1e3ab564 100644 --- a/client/alloc_watcher_e2e_test.go +++ b/client/alloc_watcher_e2e_test.go @@ -59,6 +59,7 @@ func TestPrevAlloc_StreamAllocDir_TLS(t *testing.T) { KeyFile: clientKeyFn, } c.Client.Enabled = true + c.Server.Enabled = false c.Client.Servers = []string{server.GetConfig().RPCAddr.String()} } client1 := agent.NewTestAgent(t, "client1", agentConfFunc) diff --git a/client/client_stats_endpoint.go b/client/client_stats_endpoint.go index 26ec2cb9a830..ac5ddc81e9c4 100644 --- a/client/client_stats_endpoint.go +++ b/client/client_stats_endpoint.go @@ -23,7 +23,7 @@ func (s *ClientStats) Stats(args *nstructs.NodeSpecificRequest, reply *structs.C // Check node read permissions if aclObj, err := s.c.ResolveToken(args.AuthToken); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNodeRead() { + } else if !aclObj.AllowNodeRead() { return nstructs.ErrPermissionDenied } diff --git a/client/fs_endpoint.go b/client/fs_endpoint.go index 821967e19a87..240a73c4bb50 100644 --- a/client/fs_endpoint.go +++ b/client/fs_endpoint.go @@ -111,7 +111,7 @@ func (f *FileSystem) List(args *cstructs.FsListRequest, reply *cstructs.FsListRe // Check namespace read-fs permission. if aclObj, err := f.c.ResolveToken(args.QueryOptions.AuthToken); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityReadFS) { + } else if !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityReadFS) { return structs.ErrPermissionDenied } @@ -140,7 +140,7 @@ func (f *FileSystem) Stat(args *cstructs.FsStatRequest, reply *cstructs.FsStatRe // Check namespace read-fs permission. if aclObj, err := f.c.ResolveToken(args.QueryOptions.AuthToken); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityReadFS) { + } else if !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityReadFS) { return structs.ErrPermissionDenied } @@ -197,7 +197,7 @@ func (f *FileSystem) stream(conn io.ReadWriteCloser) { if aclObj, err := f.c.ResolveToken(req.QueryOptions.AuthToken); err != nil { handleStreamResultError(err, pointer.Of(int64(http.StatusForbidden)), encoder) return - } else if aclObj != nil && !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityReadFS) { + } else if !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityReadFS) { handleStreamResultError(structs.ErrPermissionDenied, pointer.Of(int64(http.StatusForbidden)), encoder) return } @@ -379,16 +379,17 @@ func (f *FileSystem) logs(conn io.ReadWriteCloser) { alloc := ar.Alloc() // Check read permissions - if aclObj, err := f.c.ResolveToken(req.QueryOptions.AuthToken); err != nil { + aclObj, err := f.c.ResolveToken(req.QueryOptions.AuthToken) + if err != nil { handleStreamResultError(err, nil, encoder) return - } else if aclObj != nil { - readfs := aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityReadFS) - logs := aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityReadLogs) - if !readfs && !logs { - handleStreamResultError(structs.ErrPermissionDenied, nil, encoder) - return - } + } + + readfs := aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityReadFS) + logs := aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityReadLogs) + if !readfs && !logs { + handleStreamResultError(structs.ErrPermissionDenied, nil, encoder) + return } // Validate the arguments diff --git a/command/agent/agent_endpoint.go b/command/agent/agent_endpoint.go index da91860b0417..54213223e969 100644 --- a/command/agent/agent_endpoint.go +++ b/command/agent/agent_endpoint.go @@ -67,9 +67,7 @@ func (s *HTTPServer) AgentSelfRequest(resp http.ResponseWriter, req *http.Reques if err != nil { return nil, err } - - // Check agent read permissions - if aclObj != nil && !aclObj.AllowAgentRead() { + if !aclObj.AllowAgentRead() { return nil, structs.ErrPermissionDenied } @@ -322,7 +320,7 @@ func (s *HTTPServer) AgentForceLeaveRequest(resp http.ResponseWriter, req *http. // Check agent write permissions if aclObj, err := s.agent.Server().ResolveToken(secret); err != nil { return nil, err - } else if aclObj != nil && !aclObj.AllowAgentWrite() { + } else if !aclObj.AllowAgentWrite() { return nil, structs.ErrPermissionDenied } @@ -458,7 +456,7 @@ func (s *HTTPServer) listServers(resp http.ResponseWriter, req *http.Request) (i // Check agent read permissions if aclObj, err := s.agent.Client().ResolveToken(secret); err != nil { return nil, err - } else if aclObj != nil && !aclObj.AllowAgentRead() { + } else if !aclObj.AllowAgentRead() { return nil, structs.ErrPermissionDenied } @@ -485,7 +483,7 @@ func (s *HTTPServer) updateServers(resp http.ResponseWriter, req *http.Request) // Check agent write permissions if aclObj, err := s.agent.Client().ResolveToken(secret); err != nil { return nil, err - } else if aclObj != nil && !aclObj.AllowAgentWrite() { + } else if !aclObj.AllowAgentWrite() { return nil, structs.ErrPermissionDenied } @@ -512,7 +510,7 @@ func (s *HTTPServer) KeyringOperationRequest(resp http.ResponseWriter, req *http // Check agent write permissions if aclObj, err := srv.ResolveToken(secret); err != nil { return nil, err - } else if aclObj != nil && !aclObj.AllowAgentWrite() { + } else if !aclObj.AllowAgentWrite() { return nil, structs.ErrPermissionDenied } @@ -690,8 +688,7 @@ func (s *HTTPServer) AgentHostRequest(resp http.ResponseWriter, req *http.Reques enableDebug = s.agent.Client().GetConfig().EnableDebug } - if (aclObj != nil && !aclObj.AllowAgentRead()) || - (aclObj == nil && !enableDebug) { + if !aclObj.AllowAgentDebug(enableDebug) { return nil, structs.ErrPermissionDenied } @@ -763,7 +760,7 @@ func (s *HTTPServer) AgentSchedulerWorkerInfoRequest(resp http.ResponseWriter, r // Check agent read permissions if aclObj, err := s.agent.Server().ResolveToken(secret); err != nil { return nil, CodedError(http.StatusInternalServerError, err.Error()) - } else if aclObj != nil && !aclObj.AllowAgentRead() { + } else if !aclObj.AllowAgentRead() { return nil, CodedError(http.StatusForbidden, structs.ErrPermissionDenied.Error()) } @@ -817,7 +814,7 @@ func (s *HTTPServer) getScheduleWorkersConfig(resp http.ResponseWriter, req *htt // Check agent read permissions if aclObj, err := s.agent.Server().ResolveToken(secret); err != nil { return nil, CodedError(http.StatusInternalServerError, err.Error()) - } else if aclObj != nil && !aclObj.AllowAgentRead() { + } else if !aclObj.AllowAgentRead() { return nil, CodedError(http.StatusForbidden, structs.ErrPermissionDenied.Error()) } @@ -843,7 +840,7 @@ func (s *HTTPServer) updateScheduleWorkersConfig(resp http.ResponseWriter, req * // Check agent write permissions if aclObj, err := srv.ResolveToken(secret); err != nil { return nil, CodedError(http.StatusInternalServerError, err.Error()) - } else if aclObj != nil && !aclObj.AllowAgentWrite() { + } else if !aclObj.AllowAgentWrite() { return nil, CodedError(http.StatusForbidden, structs.ErrPermissionDenied.Error()) } diff --git a/command/agent/http_test.go b/command/agent/http_test.go index 1008ba3a9c6e..03dd6b3a1083 100644 --- a/command/agent/http_test.go +++ b/command/agent/http_test.go @@ -1453,8 +1453,8 @@ func TestHTTPServer_ResolveToken(t *testing.T) { t.Run("acl disabled", func(t *testing.T) { req := &http.Request{Body: http.NoBody} got, err := noACLServer.Server.ResolveToken(req) - require.NoError(t, err) - require.Nil(t, got) + must.NoError(t, err) + must.Eq(t, got, acl.ACLsDisabledACL) }) t.Run("token not found", func(t *testing.T) { diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index 1524523fbb14..2821020dc9af 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -763,14 +763,12 @@ func (s *HTTPServer) JobsParseRequest(resp http.ResponseWriter, req *http.Reques } // Check job parse permissions - if aclObj != nil { - hasParseJob := aclObj.AllowNsOp(namespace, acl.NamespaceCapabilityParseJob) - hasSubmitJob := aclObj.AllowNsOp(namespace, acl.NamespaceCapabilitySubmitJob) + hasParseJob := aclObj.AllowNsOp(namespace, acl.NamespaceCapabilityParseJob) + hasSubmitJob := aclObj.AllowNsOp(namespace, acl.NamespaceCapabilitySubmitJob) - allowed := hasParseJob || hasSubmitJob - if !allowed { - return nil, structs.ErrPermissionDenied - } + allowed := hasParseJob || hasSubmitJob + if !allowed { + return nil, structs.ErrPermissionDenied } args := &api.JobsParseRequest{} diff --git a/contributing/checklist-rpc-endpoint.md b/contributing/checklist-rpc-endpoint.md index 7f64c2b28e2f..9a5d991900c4 100644 --- a/contributing/checklist-rpc-endpoint.md +++ b/contributing/checklist-rpc-endpoint.md @@ -15,14 +15,19 @@ Prefer adding a new message to changing any existing RPC messages. * [ ] State method for modifying objects in a `Txn` in the `state` package, located in `nomad/state/`. Every new resource should have its own file and test file, named using the convention `nomad/state/state_store_[resource].go` and `nomad/state/state_store_[resource]_test.go` - * [ ] Handler for the request in `nomad/foo_endpoint.go` * RPCs are resolved by matching the method name for bound structs [net/rpc](https://golang.org/pkg/net/rpc/) - * Check ACLs for security, list endpoints filter by ACL - * Register new RPC struct in `nomad/server.go` - * Check ACLs to enforce security + * Register any new RPC structs in `nomad/server.go` + * Authentication: + * For RPCs that support HTTP APIs, call `Authenticate` before forwarding. Return any error after frowarding, and call `ResolveACL` to get an ACL to check. + * For RPCs that support client-to-server RPCs _only_, use `AuthenticateClientOnly` before forwarding. Check the `AllowClientOp` ACL after forwarding. + * For RPCs that support server-to-server RPCs _only_, use `AuthenticateServerOnly` before forwarding. Check the `AllowServerOp` ACL _before_ forwarding. + * Authorization: + * Use `ResolveACL` to turn the authenticated request into an ACL to check. + * For Update/Get/Delete RPCs, check ACLs before hitting the state store. + * For List RPCs, use ACLs as a filter on the query. * [ ] Wrapper for the HTTP request in `command/agent/foo_endpoint.go` * Backwards compatibility requires a new endpoint, an upgraded diff --git a/nomad/acl.go b/nomad/acl.go index 215df5d981be..cc8a978a3b6a 100644 --- a/nomad/acl.go +++ b/nomad/acl.go @@ -39,7 +39,3 @@ func (srv *Server) ResolveToken(secretID string) (*acl.ACL, error) { func (srv *Server) ResolvePoliciesForClaims(claims *structs.IdentityClaims) ([]*structs.ACLPolicy, error) { return srv.auth.ResolvePoliciesForClaims(claims) } - -func (srv *Server) ResolveSecretToken(secretID string) (*structs.ACLToken, error) { - return srv.auth.ResolveSecretToken(secretID) -} diff --git a/nomad/acl_test.go b/nomad/acl_test.go index d4886643b3c9..e76da9d7e4ec 100644 --- a/nomad/acl_test.go +++ b/nomad/acl_test.go @@ -10,9 +10,7 @@ import ( "time" msgpackrpc "github.com/hashicorp/net-rpc-msgpackrpc" - "github.com/hashicorp/nomad/acl" "github.com/hashicorp/nomad/ci" - "github.com/hashicorp/nomad/helper/pointer" "github.com/hashicorp/nomad/helper/uuid" "github.com/hashicorp/nomad/nomad/mock" "github.com/hashicorp/nomad/nomad/structs" @@ -20,7 +18,6 @@ import ( "github.com/hashicorp/nomad/testutil" "github.com/shoenig/test" "github.com/shoenig/test/must" - "github.com/stretchr/testify/require" ) func TestAuthenticate_mTLS(t *testing.T) { @@ -320,369 +317,3 @@ func TestAuthenticate_mTLS(t *testing.T) { }) } } - -func TestResolveACLToken(t *testing.T) { - ci.Parallel(t) - - testCases := []struct { - name string - testFn func() - }{ - { - name: "leader token", - testFn: func() { - - testServer, _, testServerCleanup := TestACLServer(t, nil) - defer testServerCleanup() - testutil.WaitForLeader(t, testServer.RPC) - - // Check the leader ACL token is correctly set. - leaderACL := testServer.getLeaderAcl() - require.NotEmpty(t, leaderACL) - - // Resolve the token and ensure it's a management token. - aclResp, err := testServer.ResolveToken(leaderACL) - require.NoError(t, err) - require.NotNil(t, aclResp) - require.True(t, aclResp.IsManagement()) - }, - }, - { - name: "anonymous token", - testFn: func() { - - testServer, _, testServerCleanup := TestACLServer(t, nil) - defer testServerCleanup() - testutil.WaitForLeader(t, testServer.RPC) - - // Call the function with an empty input secret ID which is - // classed as representing anonymous access in clusters with - // ACLs enabled. - aclResp, err := testServer.ResolveToken("") - require.NoError(t, err) - require.NotNil(t, aclResp) - require.False(t, aclResp.IsManagement()) - }, - }, - { - name: "token not found", - testFn: func() { - - testServer, _, testServerCleanup := TestACLServer(t, nil) - defer testServerCleanup() - testutil.WaitForLeader(t, testServer.RPC) - - // Call the function with randomly generated secret ID which - // does not exist within state. - aclResp, err := testServer.ResolveToken(uuid.Generate()) - require.Equal(t, structs.ErrTokenNotFound, err) - require.Nil(t, aclResp) - }, - }, - { - name: "token expired", - testFn: func() { - - testServer, _, testServerCleanup := TestACLServer(t, nil) - defer testServerCleanup() - testutil.WaitForLeader(t, testServer.RPC) - - // Create a mock token with an expiration time long in the - // past, and upsert. - token := mock.ACLToken() - token.ExpirationTime = pointer.Of(time.Date( - 1970, time.January, 1, 0, 0, 0, 0, time.UTC)) - - err := testServer.State().UpsertACLTokens( - structs.MsgTypeTestSetup, 10, []*structs.ACLToken{token}) - require.NoError(t, err) - - // Perform the function call which should result in finding the - // token has expired. - aclResp, err := testServer.ResolveToken(uuid.Generate()) - require.Equal(t, structs.ErrTokenNotFound, err) - require.Nil(t, aclResp) - }, - }, - { - name: "management token", - testFn: func() { - - testServer, _, testServerCleanup := TestACLServer(t, nil) - defer testServerCleanup() - testutil.WaitForLeader(t, testServer.RPC) - - // Generate a management token and upsert this. - managementToken := mock.ACLToken() - managementToken.Type = structs.ACLManagementToken - managementToken.Policies = nil - - err := testServer.State().UpsertACLTokens( - structs.MsgTypeTestSetup, 10, []*structs.ACLToken{managementToken}) - require.NoError(t, err) - - // Resolve the token and check that we received a management - // ACL. - aclResp, err := testServer.ResolveToken(managementToken.SecretID) - require.Nil(t, err) - require.NotNil(t, aclResp) - require.True(t, aclResp.IsManagement()) - require.Equal(t, acl.ManagementACL, aclResp) - }, - }, - { - name: "client token with policies only", - testFn: func() { - - testServer, _, testServerCleanup := TestACLServer(t, nil) - defer testServerCleanup() - testutil.WaitForLeader(t, testServer.RPC) - - // Generate a client token with associated policies and upsert - // these. - policy1 := mock.ACLPolicy() - policy2 := mock.ACLPolicy() - err := testServer.State().UpsertACLPolicies( - structs.MsgTypeTestSetup, 10, []*structs.ACLPolicy{policy1, policy2}) - - clientToken := mock.ACLToken() - clientToken.Policies = []string{policy1.Name, policy2.Name} - err = testServer.State().UpsertACLTokens( - structs.MsgTypeTestSetup, 20, []*structs.ACLToken{clientToken}) - require.NoError(t, err) - - // Resolve the token and check that we received a client - // ACL with appropriate permissions. - aclResp, err := testServer.ResolveToken(clientToken.SecretID) - require.Nil(t, err) - require.NotNil(t, aclResp) - require.False(t, aclResp.IsManagement()) - - allowed := aclResp.AllowNamespaceOperation("default", acl.NamespaceCapabilityListJobs) - require.True(t, allowed) - allowed = aclResp.AllowNamespaceOperation("other", acl.NamespaceCapabilityListJobs) - require.False(t, allowed) - - // Resolve the same token again and ensure we get the same - // result. - aclResp2, err := testServer.ResolveToken(clientToken.SecretID) - require.Nil(t, err) - require.NotNil(t, aclResp2) - require.Equal(t, aclResp, aclResp2) - - // Bust the cache by upserting the policy - err = testServer.State().UpsertACLPolicies( - structs.MsgTypeTestSetup, 30, []*structs.ACLPolicy{policy1}) - require.Nil(t, err) - - // Resolve the same token again, should get different value - aclResp3, err := testServer.ResolveToken(clientToken.SecretID) - require.Nil(t, err) - require.NotNil(t, aclResp3) - require.NotEqual(t, aclResp2, aclResp3) - }, - }, - { - name: "client token with roles only", - testFn: func() { - - testServer, _, testServerCleanup := TestACLServer(t, nil) - defer testServerCleanup() - testutil.WaitForLeader(t, testServer.RPC) - - // Create a client token that only has a link to a role. - policy1 := mock.ACLPolicy() - policy2 := mock.ACLPolicy() - err := testServer.State().UpsertACLPolicies( - structs.MsgTypeTestSetup, 10, []*structs.ACLPolicy{policy1, policy2}) - - aclRole := mock.ACLRole() - aclRole.Policies = []*structs.ACLRolePolicyLink{ - {Name: policy1.Name}, - {Name: policy2.Name}, - } - err = testServer.State().UpsertACLRoles( - structs.MsgTypeTestSetup, 30, []*structs.ACLRole{aclRole}, false) - require.NoError(t, err) - - clientToken := mock.ACLToken() - clientToken.Policies = []string{} - clientToken.Roles = []*structs.ACLTokenRoleLink{{ID: aclRole.ID}} - err = testServer.State().UpsertACLTokens( - structs.MsgTypeTestSetup, 30, []*structs.ACLToken{clientToken}) - require.NoError(t, err) - - // Resolve the token and check that we received a client - // ACL with appropriate permissions. - aclResp, err := testServer.ResolveToken(clientToken.SecretID) - require.Nil(t, err) - require.NotNil(t, aclResp) - require.False(t, aclResp.IsManagement()) - - allowed := aclResp.AllowNamespaceOperation("default", acl.NamespaceCapabilityListJobs) - require.True(t, allowed) - allowed = aclResp.AllowNamespaceOperation("other", acl.NamespaceCapabilityListJobs) - require.False(t, allowed) - - // Remove the policies from the ACL role and ensure the resolution - // permissions are updated. - aclRole.Policies = []*structs.ACLRolePolicyLink{} - err = testServer.State().UpsertACLRoles( - structs.MsgTypeTestSetup, 40, []*structs.ACLRole{aclRole}, false) - require.NoError(t, err) - - aclResp, err = testServer.ResolveToken(clientToken.SecretID) - require.Nil(t, err) - require.NotNil(t, aclResp) - require.False(t, aclResp.IsManagement()) - require.False(t, aclResp.AllowNamespaceOperation("default", acl.NamespaceCapabilityListJobs)) - }, - }, - { - name: "client with roles and policies", - testFn: func() { - - testServer, _, testServerCleanup := TestACLServer(t, nil) - defer testServerCleanup() - testutil.WaitForLeader(t, testServer.RPC) - - // Generate two policies, each with a different namespace - // permission set. - policy1 := &structs.ACLPolicy{ - Name: "policy-" + uuid.Generate(), - Rules: `namespace "platform" { policy = "write"}`, - CreateIndex: 10, - ModifyIndex: 10, - } - policy1.SetHash() - policy2 := &structs.ACLPolicy{ - Name: "policy-" + uuid.Generate(), - Rules: `namespace "web" { policy = "write"}`, - CreateIndex: 10, - ModifyIndex: 10, - } - policy2.SetHash() - - err := testServer.State().UpsertACLPolicies( - structs.MsgTypeTestSetup, 10, []*structs.ACLPolicy{policy1, policy2}) - require.NoError(t, err) - - // Create a role which references the policy that has access to - // the web namespace. - aclRole := mock.ACLRole() - aclRole.Policies = []*structs.ACLRolePolicyLink{{Name: policy2.Name}} - err = testServer.State().UpsertACLRoles( - structs.MsgTypeTestSetup, 20, []*structs.ACLRole{aclRole}, false) - require.NoError(t, err) - - // Create a token which references the policy and role. - clientToken := mock.ACLToken() - clientToken.Policies = []string{policy1.Name} - clientToken.Roles = []*structs.ACLTokenRoleLink{{ID: aclRole.ID}} - err = testServer.State().UpsertACLTokens( - structs.MsgTypeTestSetup, 30, []*structs.ACLToken{clientToken}) - require.NoError(t, err) - - // Resolve the token and check that we received a client - // ACL with appropriate permissions. - aclResp, err := testServer.ResolveToken(clientToken.SecretID) - require.Nil(t, err) - require.NotNil(t, aclResp) - require.False(t, aclResp.IsManagement()) - - allowed := aclResp.AllowNamespaceOperation("platform", acl.NamespaceCapabilityListJobs) - require.True(t, allowed) - allowed = aclResp.AllowNamespaceOperation("web", acl.NamespaceCapabilityListJobs) - require.True(t, allowed) - }, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - tc.testFn() - }) - } -} - -func TestResolveSecretToken(t *testing.T) { - ci.Parallel(t) - - testServer, _, testServerCleanup := TestACLServer(t, nil) - defer testServerCleanup() - testutil.WaitForLeader(t, testServer.RPC) - - testCases := []struct { - name string - testFn func(testServer *Server) - }{ - { - name: "valid token", - testFn: func(testServer *Server) { - - // Generate and upsert a token. - token := mock.ACLToken() - err := testServer.State().UpsertACLTokens( - structs.MsgTypeTestSetup, 10, []*structs.ACLToken{token}) - require.NoError(t, err) - - // Attempt to look up the token and perform checks. - tokenResp, err := testServer.ResolveSecretToken(token.SecretID) - require.NoError(t, err) - require.NotNil(t, tokenResp) - require.Equal(t, token, tokenResp) - }, - }, - { - name: "anonymous token", - testFn: func(testServer *Server) { - - // Call the function with an empty input secret ID which is - // classed as representing anonymous access in clusters with - // ACLs enabled. - tokenResp, err := testServer.ResolveSecretToken("") - require.NoError(t, err) - require.NotNil(t, tokenResp) - require.Equal(t, structs.AnonymousACLToken, tokenResp) - }, - }, - { - name: "token not found", - testFn: func(testServer *Server) { - - // Call the function with randomly generated secret ID which - // does not exist within state. - tokenResp, err := testServer.ResolveSecretToken(uuid.Generate()) - require.Equal(t, structs.ErrTokenNotFound, err) - require.Nil(t, tokenResp) - }, - }, - { - name: "token expired", - testFn: func(testServer *Server) { - - // Create a mock token with an expiration time long in the - // past, and upsert. - token := mock.ACLToken() - token.ExpirationTime = pointer.Of(time.Date( - 1970, time.January, 1, 0, 0, 0, 0, time.UTC)) - - err := testServer.State().UpsertACLTokens( - structs.MsgTypeTestSetup, 10, []*structs.ACLToken{token}) - require.NoError(t, err) - - // Perform the function call which should result in finding the - // token has expired. - tokenResp, err := testServer.ResolveSecretToken(uuid.Generate()) - require.Equal(t, structs.ErrTokenNotFound, err) - require.Nil(t, tokenResp) - }, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - tc.testFn(testServer) - }) - } -} diff --git a/nomad/alloc_endpoint.go b/nomad/alloc_endpoint.go index 8a83229fd9dd..4dd75662545a 100644 --- a/nomad/alloc_endpoint.go +++ b/nomad/alloc_endpoint.go @@ -354,7 +354,7 @@ func (a *Alloc) UpdateDesiredTransition(args *structs.AllocUpdateDesiredTransiti // Check that it is a management token. if aclObj, err := a.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.IsManagement() { + } else if !aclObj.IsManagement() { return structs.ErrPermissionDenied } @@ -392,15 +392,13 @@ func (a *Alloc) GetServiceRegistrations( defer metrics.MeasureSince([]string{"nomad", "alloc", "get_service_registrations"}, time.Now()) - // If ACLs are enabled, ensure the caller has the read-job namespace - // capability. + // Ensure the caller has the read-job namespace capability. aclObj, err := a.srv.ResolveACL(args) if err != nil { return err - } else if aclObj != nil { - if !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob) { - return structs.ErrPermissionDenied - } + } + if !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob) { + return structs.ErrPermissionDenied } // Set up the blocking query. diff --git a/nomad/auth/auth.go b/nomad/auth/auth.go index c7d5fd4eb88a..3d05553bc1e7 100644 --- a/nomad/auth/auth.go +++ b/nomad/auth/auth.go @@ -104,15 +104,21 @@ func (s *Authenticator) Authenticate(ctx RPCContext, args structs.RequestWithIde // get the user ACLToken or anonymous token secretID := args.GetAuthToken() - aclToken, err := s.ResolveSecretToken(secretID) + aclToken, err := s.resolveSecretToken(secretID) switch { + case err == nil && (aclToken == structs.AnonymousACLToken || + aclToken == structs.ACLsDisabledToken): + // When ACLs are disabled or if we have an anonymous token, we want to + // continue on to check mTLS certs, if available, so set the token but + // don't return yet + args.SetIdentity(&structs.AuthenticatedIdentity{ACLToken: aclToken}) + case err == nil: - // If ACLs are disabled or we have a non-anonymous token, return that. - if aclToken == nil || aclToken != structs.AnonymousACLToken { - args.SetIdentity(&structs.AuthenticatedIdentity{ACLToken: aclToken}) - return nil - } + // ACLs are enabled and we have a non-anonymous token, so set that as + // our identity and return + args.SetIdentity(&structs.AuthenticatedIdentity{ACLToken: aclToken}) + return nil case errors.Is(err, structs.ErrTokenExpired): return err @@ -168,7 +174,7 @@ func (s *Authenticator) Authenticate(ctx RPCContext, args structs.RequestWithIde // If there's no context we're in a "static" handler which only happens for // cases where the leader is making RPCs internally (volumewatcher and // deploymentwatcher) - if ctx == nil { + if ctx.IsStatic() { args.SetIdentity(&structs.AuthenticatedIdentity{ACLToken: aclToken}) return nil } @@ -206,7 +212,7 @@ func (s *Authenticator) ResolveACL(args structs.RequestWithIdentity) (*acl.ACL, } if !s.aclsEnabled { - return nil, nil + return acl.ACLsDisabledACL, nil } if identity.ClientID != "" { @@ -414,10 +420,14 @@ func (s *Authenticator) resolveACLForToken(aclToken *structs.ACLToken) (*acl.ACL // ResolveToken is used to translate an ACL Token Secret ID into // an ACL object, nil if ACLs are disabled, or an error. +// +// TODO(tgross): this is used in lots of places we probably should be calling +// Authenticate + ResolveACL on, because they support HTTP APIs that may be used +// by the Task API func (s *Authenticator) ResolveToken(secretID string) (*acl.ACL, error) { // Fast-path if ACLs are disabled if !s.aclsEnabled { - return nil, nil + return acl.ACLsDisabledACL, nil } defer metrics.MeasureSince([]string{"nomad", "acl", "resolveToken"}, time.Now()) @@ -437,8 +447,9 @@ func (s *Authenticator) ResolveToken(secretID string) (*acl.ACL, error) { return resolveTokenFromSnapshotCache(snap, s.aclCache, secretID) } -// VerifyClaim asserts that the token is valid and that the resulting -// allocation ID belongs to a non-terminal allocation +// VerifyClaim asserts that the token is valid and that the resulting allocation +// ID belongs to a non-terminal allocation. This should usually not be called by +// RPC handlers, and exists only to support the ACL.WhoAmI endpoint. func (s *Authenticator) VerifyClaim(token string) (*structs.IdentityClaims, error) { claims, err := s.encrypter.VerifyClaim(token) @@ -458,7 +469,7 @@ func (s *Authenticator) VerifyClaim(token string) (*structs.IdentityClaims, erro } // the claims for terminal allocs are always treated as expired - if alloc.TerminalStatus() { + if alloc.ClientTerminalStatus() { return nil, fmt.Errorf("allocation is terminal") } @@ -579,17 +590,15 @@ func resolveACLFromToken(snap *state.StateSnapshot, cache *structs.ACLCache[*acl return aclObj, nil } -// ResolveSecretToken is used to translate an ACL Token Secret ID into -// an ACLToken object, nil if ACLs are disabled, or an error. -func (s *Authenticator) ResolveSecretToken(secretID string) (*structs.ACLToken, error) { - // TODO(Drew) Look into using ACLObject cache or create a separate cache +// resolveSecretToken is used to translate an ACL Token Secret ID into a +// Accessor ID, the anonymous accessor, or an error. +func (s *Authenticator) resolveSecretToken(secretID string) (*structs.ACLToken, error) { + + defer metrics.MeasureSince([]string{"nomad", "acl", "accessorForSecretToken"}, time.Now()) - // Fast-path if ACLs are disabled if !s.aclsEnabled { - return nil, nil + return structs.ACLsDisabledToken, nil } - defer metrics.MeasureSince([]string{"nomad", "acl", "resolveSecretToken"}, time.Now()) - if secretID == "" { return structs.AnonymousACLToken, nil } diff --git a/nomad/auth/auth_test.go b/nomad/auth/auth_test.go index 6bd73073b22c..cfa0d3c63047 100644 --- a/nomad/auth/auth_test.go +++ b/nomad/auth/auth_test.go @@ -61,7 +61,7 @@ func TestAuthenticateDefault(t *testing.T) { err := auth.Authenticate(ctx, args) must.NoError(t, err) - must.Eq(t, "token:anonymous", args.GetIdentity().String()) + must.Eq(t, ":192.168.1.1", args.GetIdentity().String()) aclObj, err := auth.ResolveACL(args) must.NoError(t, err) @@ -80,11 +80,13 @@ func TestAuthenticateDefault(t *testing.T) { err := auth.Authenticate(ctx, args) must.NoError(t, err) - must.NotNil(t, args.GetIdentity()) + must.Eq(t, "token:acls-disabled", args.GetIdentity().String()) aclObj, err := auth.ResolveACL(args) must.NoError(t, err) - must.Nil(t, aclObj) + must.NotNil(t, aclObj) + must.Eq(t, acl.ACLsDisabledACL, aclObj) + must.True(t, aclObj.AllowAgentRead()) }, }, { @@ -98,7 +100,7 @@ func TestAuthenticateDefault(t *testing.T) { err := auth.Authenticate(ctx, args) must.NoError(t, err) - must.Eq(t, "token:anonymous", args.GetIdentity().String()) + must.Eq(t, "cli.global.nomad:192.168.1.1", args.GetIdentity().String()) aclObj, err := auth.ResolveACL(args) must.NoError(t, err) @@ -140,6 +142,11 @@ func TestAuthenticateDefault(t *testing.T) { err := auth.Authenticate(ctx, args) must.ErrorIs(t, err, structs.ErrPermissionDenied) must.Eq(t, ":192.168.1.1", args.GetIdentity().String()) + + aclObj, err := auth.ResolveACL(args) + must.ErrorIs(t, err, structs.ErrPermissionDenied) + must.Nil(t, aclObj) + must.False(t, aclObj.AllowAgentRead()) }, }, { @@ -154,6 +161,11 @@ func TestAuthenticateDefault(t *testing.T) { err := auth.Authenticate(ctx, args) must.ErrorIs(t, err, structs.ErrPermissionDenied) must.Eq(t, ":192.168.1.1", args.GetIdentity().String()) + + aclObj, err := auth.ResolveACL(args) + must.ErrorIs(t, err, structs.ErrPermissionDenied) + must.Nil(t, aclObj) + must.False(t, aclObj.AllowAgentRead()) }, }, { @@ -200,6 +212,11 @@ func TestAuthenticateDefault(t *testing.T) { err := auth.Authenticate(ctx, args) must.ErrorIs(t, err, structs.ErrTokenExpired) must.Eq(t, "unauthenticated", args.GetIdentity().String()) + + aclObj, err := auth.ResolveACL(args) + must.ErrorIs(t, err, structs.ErrPermissionDenied) + must.Nil(t, aclObj) + must.False(t, aclObj.AllowAgentRead()) }, }, { @@ -219,11 +236,13 @@ func TestAuthenticateDefault(t *testing.T) { err := auth.Authenticate(ctx, args) must.NoError(t, err) - must.Nil(t, args.GetIdentity().ACLToken) + must.Eq(t, "token:acls-disabled", args.GetIdentity().String()) aclObj, err := auth.ResolveACL(args) must.NoError(t, err) - must.Nil(t, aclObj) + must.NotNil(t, aclObj) + must.Eq(t, acl.ACLsDisabledACL, aclObj) + must.True(t, aclObj.AllowAgentRead()) }, }, { @@ -231,12 +250,11 @@ func TestAuthenticateDefault(t *testing.T) { testFn: func(t *testing.T, store *state.StateStore) { alloc := mock.Alloc() alloc.ClientStatus = structs.AllocClientStatusRunning - - identity := alloc.LookupTask("web").Identity - wih := alloc.LookupTask("web").IdentityHandle(identity) - - claims := structs.NewIdentityClaims( - alloc.Job, alloc, wih, identity, time.Now()) + task := alloc.LookupTask("web") + identity := task.Identity + wih := task.IdentityHandle(identity) + alloc.ClientStatus = structs.AllocClientStatusRunning + claims := structs.NewIdentityClaims(alloc.Job, alloc, wih, identity, time.Now()) auth := testAuthenticator(t, store, true, true) token, err := auth.encrypter.(*testEncrypter).signClaim(claims) @@ -262,8 +280,8 @@ func TestAuthenticateDefault(t *testing.T) { must.NoError(t, err) must.NotNil(t, aclObj) must.False(t, aclObj.AllowAgentRead()) - - must.NotNil(t, args.GetIdentity().GetClaims()) + must.True(t, + aclObj.AllowServiceRegistrationReadList(alloc.Job.Namespace, true)) must.Eq(t, "alloc:"+alloc.ID, args.GetIdentity().String()) // alloc becomes terminal @@ -278,16 +296,20 @@ func TestAuthenticateDefault(t *testing.T) { must.Eq(t, "unauthenticated", args.GetIdentity().String()) aclObj, err = auth.ResolveACL(args) + must.ErrorIs(t, err, structs.ErrPermissionDenied) must.Nil(t, aclObj) - must.Nil(t, args.GetIdentity().GetClaims()) + must.False(t, + aclObj.AllowServiceRegistrationReadList(alloc.Job.Namespace, true)) + }, }, { name: "mTLS and ACLs with invalid WI token", testFn: func(t *testing.T, store *state.StateStore) { alloc := mock.Alloc() - identity := alloc.LookupTask("web").Identity - wih := alloc.LookupTask("web").IdentityHandle(identity) + task := alloc.LookupTask("web") + identity := task.Identity + wih := task.IdentityHandle(identity) alloc.ClientStatus = structs.AllocClientStatusRunning claims := structs.NewIdentityClaims(alloc.Job, alloc, wih, identity, time.Now()) @@ -303,6 +325,12 @@ func TestAuthenticateDefault(t *testing.T) { err = auth.Authenticate(ctx, args) must.ErrorContains(t, err, "invalid signature") + + aclObj, err := auth.ResolveACL(args) + must.ErrorIs(t, err, structs.ErrPermissionDenied) + must.Nil(t, aclObj) + must.False(t, + aclObj.AllowServiceRegistrationReadList(alloc.Job.Namespace, true)) }, }, { @@ -333,7 +361,6 @@ func TestAuthenticateDefault(t *testing.T) { tc.testFn(t, store) }) } - } func TestAuthenticateServerOnly(t *testing.T) { @@ -387,6 +414,7 @@ func TestAuthenticateServerOnly(t *testing.T) { "invalid certificate: client.global.nomad not in expected server.global.nomad") must.Eq(t, "client.global.nomad:192.168.1.1", args.GetIdentity().String()) must.Nil(t, aclObj) + must.False(t, aclObj.AllowServerOp()) }, }, { @@ -412,7 +440,6 @@ func TestAuthenticateServerOnly(t *testing.T) { tc.testFn(t) }) } - } func TestAuthenticateClientOnly(t *testing.T) { @@ -450,6 +477,7 @@ func TestAuthenticateClientOnly(t *testing.T) { must.ErrorIs(t, err, structs.ErrPermissionDenied) must.Eq(t, ":192.168.1.1", args.GetIdentity().String()) must.Nil(t, aclObj) + must.False(t, aclObj.AllowClientOp()) }, }, { @@ -465,6 +493,7 @@ func TestAuthenticateClientOnly(t *testing.T) { must.NoError(t, err) must.NotNil(t, aclObj) must.Eq(t, "client:"+node.ID, args.GetIdentity().String()) + must.True(t, aclObj.AllowClientOp()) }, }, { @@ -496,6 +525,7 @@ func TestAuthenticateClientOnly(t *testing.T) { must.ErrorIs(t, err, structs.ErrPermissionDenied) must.Eq(t, ":192.168.1.1", args.GetIdentity().String()) must.Nil(t, aclObj) + must.False(t, aclObj.AllowClientOp()) }, }, { @@ -511,6 +541,7 @@ func TestAuthenticateClientOnly(t *testing.T) { "invalid certificate: cli.global.nomad not in expected client.global.nomad, server.global.nomad") must.Eq(t, "cli.global.nomad:192.168.1.1", args.GetIdentity().String()) must.Nil(t, aclObj) + must.False(t, aclObj.AllowClientOp()) }, }, { @@ -526,6 +557,7 @@ func TestAuthenticateClientOnly(t *testing.T) { must.ErrorIs(t, err, structs.ErrPermissionDenied) must.Eq(t, "server.global.nomad:192.168.1.1", args.GetIdentity().String()) must.Nil(t, aclObj) + must.False(t, aclObj.AllowClientOp()) }, }, { @@ -571,7 +603,6 @@ func TestAuthenticateClientOnly(t *testing.T) { tc.testFn(t, store, node) }) } - } func TestResolveACLToken(t *testing.T) { @@ -615,7 +646,9 @@ func TestResolveACLToken(t *testing.T) { aclResp, err := auth.ResolveToken("") must.NoError(t, err) - must.Nil(t, aclResp) + must.NotNil(t, aclResp) + must.Eq(t, acl.ACLsDisabledACL, aclResp) + must.True(t, aclResp.IsManagement()) }, }, { @@ -909,7 +942,7 @@ func TestResolveSecretToken(t *testing.T) { must.NoError(t, err) // Attempt to look up the token and perform checks. - tokenResp, err := auth.ResolveSecretToken(token.SecretID) + tokenResp, err := auth.resolveSecretToken(token.SecretID) must.NoError(t, err) must.NotNil(t, tokenResp) must.Eq(t, token, tokenResp) @@ -922,7 +955,7 @@ func TestResolveSecretToken(t *testing.T) { // Call the function with an empty input secret ID which is // classed as representing anonymous access in clusters with // ACLs enabled. - tokenResp, err := auth.ResolveSecretToken("") + tokenResp, err := auth.resolveSecretToken("") must.NoError(t, err) must.NotNil(t, tokenResp) must.Eq(t, structs.AnonymousACLToken, tokenResp) @@ -934,7 +967,7 @@ func TestResolveSecretToken(t *testing.T) { // Call the function with randomly generated secret ID which // does not exist within state. - tokenResp, err := auth.ResolveSecretToken(uuid.Generate()) + tokenResp, err := auth.resolveSecretToken(uuid.Generate()) must.ErrorIs(t, err, structs.ErrTokenNotFound) must.Nil(t, tokenResp) }, @@ -955,7 +988,7 @@ func TestResolveSecretToken(t *testing.T) { // Perform the function call which should result in finding the // token has expired. - tokenResp, err := auth.ResolveSecretToken(uuid.Generate()) + tokenResp, err := auth.resolveSecretToken(uuid.Generate()) must.ErrorIs(t, err, structs.ErrTokenNotFound) must.Nil(t, tokenResp) }, diff --git a/nomad/client_agent_endpoint.go b/nomad/client_agent_endpoint.go index 4f96087f0a78..02674e30f361 100644 --- a/nomad/client_agent_endpoint.go +++ b/nomad/client_agent_endpoint.go @@ -48,7 +48,10 @@ func (a *Agent) Profile(args *structs.AgentPprofRequest, reply *structs.AgentPpr aclObj, err := a.srv.ResolveACL(args) if err != nil { return err - } else if aclObj != nil && !aclObj.AllowAgentWrite() { + } else if !aclObj.AllowAgentWrite() { + // we're not checking AllowAgentDebug here because the target might not + // be this server, and the server doesn't know if enable_debug has been + // set on the target return structs.ErrPermissionDenied } @@ -83,8 +86,8 @@ func (a *Agent) Profile(args *structs.AgentPprofRequest, reply *structs.AgentPpr } } - // If ACLs are disabled, EnableDebug must be enabled - if aclObj == nil && !a.srv.config.EnableDebug { + // This server is the target, so now we can check for AllowAgentDebug + if !aclObj.AllowAgentDebug(a.srv.config.EnableDebug) { return structs.ErrPermissionDenied } @@ -148,7 +151,7 @@ func (a *Agent) monitor(conn io.ReadWriteCloser) { if aclObj, err := a.srv.ResolveACL(&args); err != nil { handleStreamResultError(err, nil, encoder) return - } else if aclObj != nil && !aclObj.AllowAgentRead() { + } else if !aclObj.AllowAgentRead() { handleStreamResultError(structs.ErrPermissionDenied, pointer.Of(int64(403)), encoder) return } @@ -422,8 +425,7 @@ func (a *Agent) Host(args *structs.HostDataRequest, reply *structs.HostDataRespo if err != nil { return err } - if (aclObj != nil && !aclObj.AllowAgentRead()) || - (aclObj == nil && !a.srv.config.EnableDebug) { + if !aclObj.AllowAgentDebug(a.srv.GetConfig().EnableDebug) { return structs.ErrPermissionDenied } diff --git a/nomad/client_agent_endpoint_test.go b/nomad/client_agent_endpoint_test.go index af1a3f2d80ad..ae0451a4f1d0 100644 --- a/nomad/client_agent_endpoint_test.go +++ b/nomad/client_agent_endpoint_test.go @@ -26,6 +26,7 @@ import ( "github.com/hashicorp/nomad/nomad/mock" "github.com/hashicorp/nomad/nomad/structs" "github.com/hashicorp/nomad/testutil" + "github.com/shoenig/test/must" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -1013,5 +1014,5 @@ func TestAgentHost_ACLDebugRequired(t *testing.T) { var resp structs.HostDataResponse err := s.RPC("Agent.Host", &req, &resp) - require.Equal(t, "Permission denied", err.Error()) + must.EqError(t, err, structs.ErrPermissionDenied.Error()) } diff --git a/nomad/client_alloc_endpoint.go b/nomad/client_alloc_endpoint.go index f0ec65d21d01..2ff62cb33026 100644 --- a/nomad/client_alloc_endpoint.go +++ b/nomad/client_alloc_endpoint.go @@ -57,7 +57,7 @@ func (a *ClientAllocations) GarbageCollectAll(args *structs.NodeSpecificRequest, // Check node read permissions if aclObj, err := a.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNodeWrite() { + } else if !aclObj.AllowNodeWrite() { return structs.ErrPermissionDenied } @@ -125,7 +125,7 @@ func (a *ClientAllocations) Signal(args *structs.AllocSignalRequest, reply *stru // Check namespace alloc-lifecycle permission. if aclObj, err := a.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityAllocLifecycle) { + } else if !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityAllocLifecycle) { return structs.ErrPermissionDenied } @@ -183,7 +183,7 @@ func (a *ClientAllocations) GarbageCollect(args *structs.AllocSpecificRequest, r // Check namespace submit-job permission. if aclObj, err := a.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilitySubmitJob) { + } else if !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilitySubmitJob) { return structs.ErrPermissionDenied } @@ -236,7 +236,7 @@ func (a *ClientAllocations) Restart(args *structs.AllocRestartRequest, reply *st // Check for namespace alloc-lifecycle permissions. if aclObj, err := a.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityAllocLifecycle) { + } else if !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityAllocLifecycle) { return structs.ErrPermissionDenied } @@ -289,7 +289,7 @@ func (a *ClientAllocations) Stats(args *cstructs.AllocStatsRequest, reply *cstru // Check for namespace read-job permissions. if aclObj, err := a.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityReadJob) { + } else if !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityReadJob) { return structs.ErrPermissionDenied } @@ -349,7 +349,7 @@ func (a *ClientAllocations) Checks(args *cstructs.AllocChecksRequest, reply *cst // Check for namespace read-job permissions. if aclObj, err := a.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityReadJob) { + } else if !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityReadJob) { return structs.ErrPermissionDenied } @@ -424,7 +424,7 @@ func (a *ClientAllocations) exec(conn io.ReadWriteCloser) { if aclObj, err := a.srv.ResolveACL(&args); err != nil { handleStreamResultError(err, nil, encoder) return - } else if aclObj != nil && !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityAllocExec) { + } else if !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityAllocExec) { // client ultimately checks if AllocNodeExec is required handleStreamResultError(structs.ErrPermissionDenied, nil, encoder) return diff --git a/nomad/client_fs_endpoint.go b/nomad/client_fs_endpoint.go index 07e410505f25..9869e43cc70b 100644 --- a/nomad/client_fs_endpoint.go +++ b/nomad/client_fs_endpoint.go @@ -200,7 +200,7 @@ func (f *FileSystem) Stat(args *cstructs.FsStatRequest, reply *cstructs.FsStatRe // Check filesystem read permissions if aclObj, err := f.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityReadFS) { + } else if !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityReadFS) { return structs.ErrPermissionDenied } @@ -277,7 +277,7 @@ func (f *FileSystem) stream(conn io.ReadWriteCloser) { if aclObj, err := f.srv.ResolveACL(&args); err != nil { handleStreamResultError(err, nil, encoder) return - } else if aclObj != nil && !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityReadFS) { + } else if !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityReadFS) { handleStreamResultError(structs.ErrPermissionDenied, nil, encoder) return } diff --git a/nomad/client_meta_endpoint.go b/nomad/client_meta_endpoint.go index fd606185af3e..dbf577b6d38b 100644 --- a/nomad/client_meta_endpoint.go +++ b/nomad/client_meta_endpoint.go @@ -45,7 +45,7 @@ func (n *NodeMeta) Apply(args *structs.NodeMetaApplyRequest, reply *structs.Node // Check node write permissions if aclObj, err := n.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNodeWrite() { + } else if !aclObj.AllowNodeWrite() { return nstructs.ErrPermissionDenied } @@ -72,7 +72,7 @@ func (n *NodeMeta) Read(args *structs.NodeSpecificRequest, reply *structs.NodeMe // Check node read permissions if aclObj, err := n.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNodeRead() { + } else if !aclObj.AllowNodeRead() { return nstructs.ErrPermissionDenied } diff --git a/nomad/client_stats_endpoint.go b/nomad/client_stats_endpoint.go index 835c354f7544..d74bcdedbdcd 100644 --- a/nomad/client_stats_endpoint.go +++ b/nomad/client_stats_endpoint.go @@ -45,7 +45,7 @@ func (s *ClientStats) Stats(args *nstructs.NodeSpecificRequest, reply *structs.C // Check node read permissions if aclObj, err := s.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNodeRead() { + } else if !aclObj.AllowNodeRead() { return nstructs.ErrPermissionDenied } diff --git a/nomad/csi_endpoint.go b/nomad/csi_endpoint.go index 8d32efd155a7..ba14fe51042b 100644 --- a/nomad/csi_endpoint.go +++ b/nomad/csi_endpoint.go @@ -1859,7 +1859,7 @@ func (v *CSIPlugin) Delete(args *structs.CSIPluginDeleteRequest, reply *structs. // Check that it is a management token. if aclObj, err := v.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.IsManagement() { + } else if !aclObj.IsManagement() { return structs.ErrPermissionDenied } diff --git a/nomad/deployment_endpoint.go b/nomad/deployment_endpoint.go index d8fd64971659..e775f07a7c0f 100644 --- a/nomad/deployment_endpoint.go +++ b/nomad/deployment_endpoint.go @@ -131,7 +131,7 @@ func (d *Deployment) Fail(args *structs.DeploymentFailRequest, reply *structs.De // Check namespace submit-job permissions if aclObj, err := d.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(deploy.Namespace, acl.NamespaceCapabilitySubmitJob) { + } else if !aclObj.AllowNsOp(deploy.Namespace, acl.NamespaceCapabilitySubmitJob) { return structs.ErrPermissionDenied } @@ -178,7 +178,7 @@ func (d *Deployment) Pause(args *structs.DeploymentPauseRequest, reply *structs. // Check namespace submit-job permissions if aclObj, err := d.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(deploy.Namespace, acl.NamespaceCapabilitySubmitJob) { + } else if !aclObj.AllowNsOp(deploy.Namespace, acl.NamespaceCapabilitySubmitJob) { return structs.ErrPermissionDenied } @@ -229,7 +229,7 @@ func (d *Deployment) Promote(args *structs.DeploymentPromoteRequest, reply *stru // Check namespace submit-job permissions if aclObj, err := d.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(deploy.Namespace, acl.NamespaceCapabilitySubmitJob) { + } else if !aclObj.AllowNsOp(deploy.Namespace, acl.NamespaceCapabilitySubmitJob) { return structs.ErrPermissionDenied } @@ -276,7 +276,7 @@ func (d *Deployment) Run(args *structs.DeploymentRunRequest, reply *structs.Depl // Check namespace submit-job permissions if aclObj, err := d.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(deploy.Namespace, acl.NamespaceCapabilitySubmitJob) { + } else if !aclObj.AllowNsOp(deploy.Namespace, acl.NamespaceCapabilitySubmitJob) { return structs.ErrPermissionDenied } @@ -323,7 +323,7 @@ func (d *Deployment) Unblock(args *structs.DeploymentUnblockRequest, reply *stru // Check namespace submit-job permissions if aclObj, err := d.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(deploy.Namespace, acl.NamespaceCapabilitySubmitJob) { + } else if !aclObj.AllowNsOp(deploy.Namespace, acl.NamespaceCapabilitySubmitJob) { return structs.ErrPermissionDenied } @@ -370,7 +370,7 @@ func (d *Deployment) Cancel(args *structs.DeploymentCancelRequest, reply *struct // Check namespace submit-job permissions if aclObj, err := d.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(deploy.Namespace, acl.NamespaceCapabilitySubmitJob) { + } else if !aclObj.AllowNsOp(deploy.Namespace, acl.NamespaceCapabilitySubmitJob) { return structs.ErrPermissionDenied } @@ -422,7 +422,7 @@ func (d *Deployment) SetAllocHealth(args *structs.DeploymentAllocHealthRequest, // Check namespace submit-job permissions if aclObj, err := d.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(deploy.Namespace, acl.NamespaceCapabilitySubmitJob) { + } else if !aclObj.AllowNsOp(deploy.Namespace, acl.NamespaceCapabilitySubmitJob) { return structs.ErrPermissionDenied } @@ -454,8 +454,7 @@ func (d *Deployment) List(args *structs.DeploymentListRequest, reply *structs.De if err != nil { return err } - - if aclObj != nil && !aclObj.AllowNsOp(namespace, acl.NamespaceCapabilityReadJob) { + if !aclObj.AllowNsOp(namespace, acl.NamespaceCapabilityReadJob) { return structs.ErrPermissionDenied } diff --git a/nomad/eval_endpoint.go b/nomad/eval_endpoint.go index 588f11cd324c..018115355f15 100644 --- a/nomad/eval_endpoint.go +++ b/nomad/eval_endpoint.go @@ -461,7 +461,7 @@ func (e *Eval) Delete( // meaning only those with management tokens can call it. if aclObj, err := e.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.IsManagement() { + } else if !aclObj.IsManagement() { return structs.ErrPermissionDenied } diff --git a/nomad/job_endpoint.go b/nomad/job_endpoint.go index b7d907c1dc48..95f45254d7fa 100644 --- a/nomad/job_endpoint.go +++ b/nomad/job_endpoint.go @@ -131,65 +131,63 @@ func (j *Job) Register(args *structs.JobRegisterRequest, reply *structs.JobRegis // Run the submission controller warnings = append(warnings, j.submissionController(args)) - // Attach the Nomad token's accessor ID so that deploymentwatcher - // can reference the token later - nomadACLToken, err := j.srv.ResolveSecretToken(args.AuthToken) - if err != nil { - return err - } - if nomadACLToken != nil { - args.Job.NomadTokenID = nomadACLToken.AccessorID + // Attach the user token's accessor ID so that deploymentwatcher can + // reference the token later in multiregion deployments. We can't auth once + // and then use the leader ACL because the leader ACLs aren't shared across + // regions. Note this implies WIs can't be used to register multi-region + // jobs b/c their identities are only valid in a single region. + if args.GetIdentity().ACLToken != nil && + args.GetIdentity().ACLToken.AccessorID != "acls-disabled" { + args.Job.NomadTokenID = args.GetIdentity().ACLToken.AccessorID } // Set the warning message reply.Warnings = helper.MergeMultierrorWarnings(warnings...) // Check job submission permissions - if aclObj != nil { - if !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilitySubmitJob) { - return structs.ErrPermissionDenied - } + if !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilitySubmitJob) { + return structs.ErrPermissionDenied + } - // Validate Volume Permissions - for _, tg := range args.Job.TaskGroups { - for _, vol := range tg.Volumes { - switch vol.Type { - case structs.VolumeTypeCSI: - if !allowCSIMount(aclObj, args.RequestNamespace()) { + // Validate Volume Permissions + for _, tg := range args.Job.TaskGroups { + for _, vol := range tg.Volumes { + switch vol.Type { + case structs.VolumeTypeCSI: + if !allowCSIMount(aclObj, args.RequestNamespace()) { + return structs.ErrPermissionDenied + } + case structs.VolumeTypeHost: + // If a volume is readonly, then we allow access if the user has + // ReadOnly or ReadWrite access to the volume. Otherwise we only + // allow access if they have ReadWrite access. + if vol.ReadOnly { + if !aclObj.AllowHostVolumeOperation(vol.Source, acl.HostVolumeCapabilityMountReadOnly) && + !aclObj.AllowHostVolumeOperation(vol.Source, acl.HostVolumeCapabilityMountReadWrite) { return structs.ErrPermissionDenied } - case structs.VolumeTypeHost: - // If a volume is readonly, then we allow access if the user has ReadOnly - // or ReadWrite access to the volume. Otherwise we only allow access if - // they have ReadWrite access. - if vol.ReadOnly { - if !aclObj.AllowHostVolumeOperation(vol.Source, acl.HostVolumeCapabilityMountReadOnly) && - !aclObj.AllowHostVolumeOperation(vol.Source, acl.HostVolumeCapabilityMountReadWrite) { - return structs.ErrPermissionDenied - } - } else { - if !aclObj.AllowHostVolumeOperation(vol.Source, acl.HostVolumeCapabilityMountReadWrite) { - return structs.ErrPermissionDenied - } + } else { + if !aclObj.AllowHostVolumeOperation(vol.Source, acl.HostVolumeCapabilityMountReadWrite) { + return structs.ErrPermissionDenied } - default: - return structs.ErrPermissionDenied } + default: + return structs.ErrPermissionDenied } + } - for _, t := range tg.Tasks { - for _, vm := range t.VolumeMounts { - vol := tg.Volumes[vm.Volume] - if vm.PropagationMode == structs.VolumeMountPropagationBidirectional && - !aclObj.AllowHostVolumeOperation(vol.Source, acl.HostVolumeCapabilityMountReadWrite) { - return structs.ErrPermissionDenied - } + for _, t := range tg.Tasks { + for _, vm := range t.VolumeMounts { + vol := tg.Volumes[vm.Volume] + if vm.PropagationMode == structs.VolumeMountPropagationBidirectional && + !aclObj.AllowHostVolumeOperation(vol.Source, acl.HostVolumeCapabilityMountReadWrite) { + return structs.ErrPermissionDenied } + } - if t.CSIPluginConfig != nil { - if !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityCSIRegisterPlugin) { - return structs.ErrPermissionDenied - } + if t.CSIPluginConfig != nil { + if !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityCSIRegisterPlugin) { + return structs.ErrPermissionDenied } } } @@ -299,7 +297,8 @@ func (j *Job) Register(args *structs.JobRegisterRequest, reply *structs.JobRegis return err } - policyWarnings, err := j.enforceSubmitJob(args.PolicyOverride, args.Job.Copy(), existingJob, nomadACLToken, ns) + policyWarnings, err := j.enforceSubmitJob(args.PolicyOverride, args.Job.Copy(), + existingJob, args.GetIdentity().GetACLToken(), ns) if err != nil { return err } @@ -508,7 +507,7 @@ func (j *Job) Summary(args *structs.JobSummaryRequest, reply *structs.JobSummary // Check for read-job permissions if aclObj, err := j.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob) { + } else if !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob) { return structs.ErrPermissionDenied } @@ -572,7 +571,7 @@ func (j *Job) Validate(args *structs.JobValidateRequest, reply *structs.JobValid // Check for read-job permissions if aclObj, err := j.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob) { + } else if !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob) { return structs.ErrPermissionDenied } @@ -613,7 +612,7 @@ func (j *Job) Revert(args *structs.JobRevertRequest, reply *structs.JobRegisterR // Check for submit-job permissions if aclObj, err := j.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilitySubmitJob) { + } else if !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilitySubmitJob) { return structs.ErrPermissionDenied } @@ -686,7 +685,7 @@ func (j *Job) Stable(args *structs.JobStabilityRequest, reply *structs.JobStabil // Check for read-job permissions if aclObj, err := j.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilitySubmitJob) { + } else if !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilitySubmitJob) { return structs.ErrPermissionDenied } @@ -737,7 +736,7 @@ func (j *Job) Evaluate(args *structs.JobEvaluateRequest, reply *structs.JobRegis // Check for submit-job permissions if aclObj, err := j.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilitySubmitJob) { + } else if !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilitySubmitJob) { return structs.ErrPermissionDenied } @@ -838,7 +837,7 @@ func (j *Job) Deregister(args *structs.JobDeregisterRequest, reply *structs.JobD // Check for submit-job permissions if aclObj, err := j.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilitySubmitJob) { + } else if !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilitySubmitJob) { return structs.ErrPermissionDenied } @@ -932,7 +931,6 @@ func (j *Job) BatchDeregister(args *structs.JobBatchDeregisterRequest, reply *st } defer metrics.MeasureSince([]string{"nomad", "job", "batch_deregister"}, time.Now()) - // Resolve the ACL token aclObj, err := j.srv.ResolveACL(args) if err != nil { return err @@ -949,7 +947,7 @@ func (j *Job) BatchDeregister(args *structs.JobBatchDeregisterRequest, reply *st // Loop through checking for permissions for jobNS := range args.Jobs { // Check for submit-job permissions - if aclObj != nil && !aclObj.AllowNsOp(jobNS.Namespace, acl.NamespaceCapabilitySubmitJob) { + if !aclObj.AllowNsOp(jobNS.Namespace, acl.NamespaceCapabilitySubmitJob) { return structs.ErrPermissionDenied } } @@ -1024,18 +1022,15 @@ func (j *Job) Scale(args *structs.JobScaleRequest, reply *structs.JobRegisterRes namespace := args.RequestNamespace() - // Authorize request aclObj, err := j.srv.ResolveACL(args) if err != nil { return err } - if aclObj != nil { - hasScaleJob := aclObj.AllowNsOp(namespace, acl.NamespaceCapabilityScaleJob) - hasSubmitJob := aclObj.AllowNsOp(namespace, acl.NamespaceCapabilitySubmitJob) - if !(hasScaleJob || hasSubmitJob) { - return structs.ErrPermissionDenied - } + hasScaleJob := aclObj.AllowNsOp(namespace, acl.NamespaceCapabilityScaleJob) + hasSubmitJob := aclObj.AllowNsOp(namespace, acl.NamespaceCapabilitySubmitJob) + if !(hasScaleJob || hasSubmitJob) { + return structs.ErrPermissionDenied } if ok, err := registrationsAreAllowed(aclObj, j.srv.State()); !ok || err != nil { @@ -1216,7 +1211,7 @@ func (j *Job) GetJobSubmission(args *structs.JobSubmissionRequest, reply *struct // Check for read-job permissions if aclObj, err := j.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob) { + } else if !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob) { return structs.ErrPermissionDenied } @@ -1264,7 +1259,7 @@ func (j *Job) GetJob(args *structs.JobSpecificRequest, // Check for read-job permissions if aclObj, err := j.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob) { + } else if !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob) { return structs.ErrPermissionDenied } @@ -1315,7 +1310,7 @@ func (j *Job) GetJobVersions(args *structs.JobVersionsRequest, // Check for read-job permissions if aclObj, err := j.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob) { + } else if !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob) { return structs.ErrPermissionDenied } @@ -1401,7 +1396,7 @@ func registrationsAreAllowed(aclObj *acl.ACL, state *state.StateStore) (bool, er if cfg != nil && !cfg.RejectJobRegistration { return true, nil } - if aclObj != nil && aclObj.IsManagement() { + if aclObj.IsManagement() { return true, nil } return false, nil @@ -1533,7 +1528,7 @@ func (j *Job) Allocations(args *structs.JobSpecificRequest, // Check for read-job permissions if aclObj, err := j.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob) { + } else if !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob) { return structs.ErrPermissionDenied } @@ -1593,7 +1588,7 @@ func (j *Job) Evaluations(args *structs.JobSpecificRequest, // Check for read-job permissions if aclObj, err := j.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob) { + } else if !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob) { return structs.ErrPermissionDenied } @@ -1640,7 +1635,7 @@ func (j *Job) Deployments(args *structs.JobSpecificRequest, // Check for read-job permissions if aclObj, err := j.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob) { + } else if !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob) { return structs.ErrPermissionDenied } @@ -1687,7 +1682,7 @@ func (j *Job) LatestDeployment(args *structs.JobSpecificRequest, // Check for read-job permissions if aclObj, err := j.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob) { + } else if !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob) { return structs.ErrPermissionDenied } @@ -1754,7 +1749,7 @@ func (j *Job) Plan(args *structs.JobPlanRequest, reply *structs.JobPlanResponse) // Check job submission permissions, which we assume is the same for plan if aclObj, err := j.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil { + } else { if !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilitySubmitJob) { return structs.ErrPermissionDenied } @@ -1961,7 +1956,7 @@ func (j *Job) Dispatch(args *structs.JobDispatchRequest, reply *structs.JobDispa aclObj, err := j.srv.ResolveACL(args) if err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityDispatchJob) { + } else if !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityDispatchJob) { return structs.ErrPermissionDenied } @@ -2196,7 +2191,7 @@ func (j *Job) ScaleStatus(args *structs.JobScaleStatusRequest, // Check for autoscaler permissions if aclObj, err := j.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil { + } else { hasReadJob := aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob) hasReadJobScaling := aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJobScaling) if !(hasReadJob || hasReadJobScaling) { @@ -2318,15 +2313,13 @@ func (j *Job) GetServiceRegistrations( } defer metrics.MeasureSince([]string{"nomad", "job", "get_service_registrations"}, time.Now()) - // If ACLs are enabled, ensure the caller has the read-job namespace - // capability. + // Check for read-job namespace capability. aclObj, err := j.srv.ResolveACL(args) if err != nil { return err - } else if aclObj != nil { - if !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob) { - return structs.ErrPermissionDenied - } + } + if !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob) { + return structs.ErrPermissionDenied } // Set up the blocking query. diff --git a/nomad/keyring_endpoint.go b/nomad/keyring_endpoint.go index 6cf91410e053..35f75c37d9c7 100644 --- a/nomad/keyring_endpoint.go +++ b/nomad/keyring_endpoint.go @@ -43,7 +43,7 @@ func (k *Keyring) Rotate(args *structs.KeyringRotateRootKeyRequest, reply *struc if aclObj, err := k.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.IsManagement() { + } else if !aclObj.IsManagement() { return structs.ErrPermissionDenied } @@ -114,7 +114,7 @@ func (k *Keyring) List(args *structs.KeyringListRootKeyMetaRequest, reply *struc if aclObj, err := k.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.IsManagement() { + } else if !aclObj.IsManagement() { return structs.ErrPermissionDenied } @@ -167,7 +167,7 @@ func (k *Keyring) Update(args *structs.KeyringUpdateRootKeyRequest, reply *struc if aclObj, err := k.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.IsManagement() { + } else if !aclObj.IsManagement() { return structs.ErrPermissionDenied } @@ -232,8 +232,6 @@ func (k *Keyring) validateUpdate(args *structs.KeyringUpdateRootKeyRequest) erro // Get retrieves an existing key from the keyring, including both the // key material and metadata. It is used only for replication. func (k *Keyring) Get(args *structs.KeyringGetRootKeyRequest, reply *structs.KeyringGetRootKeyResponse) error { - - // TODO(tgross): this should use the replication token, not cert check aclObj, err := k.srv.AuthenticateServerOnly(k.ctx, args) k.srv.MeasureRPCRate("keyring", structs.RateMetricRead, args) @@ -314,7 +312,7 @@ func (k *Keyring) Delete(args *structs.KeyringDeleteRootKeyRequest, reply *struc if aclObj, err := k.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.IsManagement() { + } else if !aclObj.IsManagement() { return structs.ErrPermissionDenied } diff --git a/nomad/namespace_endpoint.go b/nomad/namespace_endpoint.go index 5e9b5073e4de..ae557cc6ee70 100644 --- a/nomad/namespace_endpoint.go +++ b/nomad/namespace_endpoint.go @@ -44,7 +44,7 @@ func (n *Namespace) UpsertNamespaces(args *structs.NamespaceUpsertRequest, // Check management permissions if aclObj, err := n.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.IsManagement() { + } else if !aclObj.IsManagement() { return structs.ErrPermissionDenied } @@ -90,7 +90,7 @@ func (n *Namespace) DeleteNamespaces(args *structs.NamespaceDeleteRequest, reply // Check management permissions if aclObj, err := n.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.IsManagement() { + } else if !aclObj.IsManagement() { return structs.ErrPermissionDenied } @@ -306,7 +306,7 @@ func (n *Namespace) GetNamespace(args *structs.NamespaceSpecificRequest, reply * // Check capabilities for the given namespace permissions if aclObj, err := n.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNamespace(args.Name) { + } else if !aclObj.AllowNamespace(args.Name) { return structs.ErrPermissionDenied } @@ -360,7 +360,7 @@ func (n *Namespace) GetNamespaces(args *structs.NamespaceSetRequest, reply *stru // Check management permissions if aclObj, err := n.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.IsManagement() { + } else if !aclObj.IsManagement() { return structs.ErrPermissionDenied } diff --git a/nomad/node_endpoint.go b/nomad/node_endpoint.go index 485be68c237a..d9199ba3d312 100644 --- a/nomad/node_endpoint.go +++ b/nomad/node_endpoint.go @@ -383,6 +383,12 @@ func (n *Node) Deregister(args *structs.NodeDeregisterRequest, reply *structs.No } defer metrics.MeasureSince([]string{"nomad", "client", "deregister"}, time.Now()) + if aclObj, err := n.srv.ResolveACL(args); err != nil { + return structs.ErrPermissionDenied + } else if !aclObj.AllowNodeWrite() { + return structs.ErrPermissionDenied + } + if args.NodeID == "" { return fmt.Errorf("missing node ID for client deregistration") } @@ -410,6 +416,12 @@ func (n *Node) BatchDeregister(args *structs.NodeBatchDeregisterRequest, reply * } defer metrics.MeasureSince([]string{"nomad", "client", "batch_deregister"}, time.Now()) + if aclObj, err := n.srv.ResolveACL(args); err != nil { + return structs.ErrPermissionDenied + } else if !aclObj.AllowNodeWrite() { + return structs.ErrPermissionDenied + } + if len(args.NodeIDs) == 0 { return fmt.Errorf("missing node IDs for client deregistration") } @@ -419,18 +431,12 @@ func (n *Node) BatchDeregister(args *structs.NodeBatchDeregisterRequest, reply * }) } -// deregister takes a raftMessage closure, to support both Deregister and BatchDeregister +// deregister takes a raftMessage closure, to support both Deregister and +// BatchDeregister. The caller should have already authorized the request. func (n *Node) deregister(args *structs.NodeBatchDeregisterRequest, reply *structs.NodeUpdateResponse, raftApplyFn func() (interface{}, uint64, error), ) error { - // Check request permissions - if aclObj, err := n.srv.ResolveACL(args); err != nil { - return err - } else if aclObj != nil && !aclObj.AllowNodeWrite() { - return structs.ErrPermissionDenied - } - // Look for the node snap, err := n.srv.fsm.State().Snapshot() if err != nil { @@ -759,7 +765,7 @@ func (n *Node) UpdateDrain(args *structs.NodeUpdateDrainRequest, // Check node write permissions if aclObj, err := n.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNodeWrite() { + } else if !aclObj.AllowNodeWrite() { return structs.ErrPermissionDenied } @@ -859,7 +865,7 @@ func (n *Node) UpdateEligibility(args *structs.NodeUpdateEligibilityRequest, // Check node write permissions if aclObj, err := n.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNodeWrite() { + } else if !aclObj.AllowNodeWrite() { return structs.ErrPermissionDenied } @@ -962,7 +968,7 @@ func (n *Node) Evaluate(args *structs.NodeEvaluateRequest, reply *structs.NodeUp // Check node write permissions if aclObj, err := n.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNodeWrite() { + } else if !aclObj.AllowNodeWrite() { return structs.ErrPermissionDenied } @@ -1025,7 +1031,7 @@ func (n *Node) GetNode(args *structs.NodeSpecificRequest, if err != nil { return err } - if aclObj != nil && !aclObj.AllowNodeRead() { + if !aclObj.AllowNodeRead() { return structs.ErrPermissionDenied } @@ -1086,7 +1092,7 @@ func (n *Node) GetAllocs(args *structs.NodeSpecificRequest, if err != nil { return err } - if aclObj != nil && !aclObj.AllowNodeRead() { + if !aclObj.AllowNodeRead() { return structs.ErrPermissionDenied } @@ -1095,11 +1101,6 @@ func (n *Node) GetAllocs(args *structs.NodeSpecificRequest, // readNS is a caching namespace read-job helper readNS := func(ns string) bool { - if aclObj == nil { - // ACLs are disabled; everything is readable - return true - } - if readable, ok := readableNamespaces[ns]; ok { // cache hit return readable @@ -1598,7 +1599,7 @@ func (n *Node) List(args *structs.NodeListRequest, // Check node read permissions if aclObj, err := n.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNodeRead() { + } else if !aclObj.AllowNodeRead() { return structs.ErrPermissionDenied } diff --git a/nomad/operator_endpoint.go b/nomad/operator_endpoint.go index dd20c084636b..982730b97320 100644 --- a/nomad/operator_endpoint.go +++ b/nomad/operator_endpoint.go @@ -52,7 +52,7 @@ func (op *Operator) RaftGetConfiguration(args *structs.GenericRequest, reply *st // Check management permissions if aclObj, err := op.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.IsManagement() { + } else if !aclObj.IsManagement() { return structs.ErrPermissionDenied } @@ -119,7 +119,7 @@ func (op *Operator) RaftRemovePeerByAddress(args *structs.RaftPeerByAddressReque // Check management permissions if aclObj, err := op.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.IsManagement() { + } else if !aclObj.IsManagement() { return structs.ErrPermissionDenied } @@ -177,7 +177,7 @@ func (op *Operator) RaftRemovePeerByID(args *structs.RaftPeerByIDRequest, reply // Check management permissions if aclObj, err := op.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.IsManagement() { + } else if !aclObj.IsManagement() { return structs.ErrPermissionDenied } @@ -254,7 +254,7 @@ func (op *Operator) TransferLeadershipToPeer(req *structs.RaftPeerRequest, reply // Check ACL permissions if aclObj, err := op.srv.ResolveACL(req); err != nil { return err - } else if aclObj != nil && !aclObj.IsManagement() { + } else if !aclObj.IsManagement() { reply.Err = structs.ErrPermissionDenied return structs.ErrPermissionDenied } @@ -639,7 +639,7 @@ func (op *Operator) snapshotSave(conn io.ReadWriteCloser) { } handleFailure(code, err) return - } else if aclObj != nil && !aclObj.IsManagement() { + } else if !aclObj.IsManagement() { handleFailure(403, structs.ErrPermissionDenied) return } @@ -726,7 +726,7 @@ func (op *Operator) snapshotRestore(conn io.ReadWriteCloser) { } handleFailure(code, err) return - } else if aclObj != nil && !aclObj.IsManagement() { + } else if !aclObj.IsManagement() { handleFailure(403, structs.ErrPermissionDenied) return } diff --git a/nomad/periodic_endpoint.go b/nomad/periodic_endpoint.go index 49b9ff0fea17..1baf9ddfa9b9 100644 --- a/nomad/periodic_endpoint.go +++ b/nomad/periodic_endpoint.go @@ -42,7 +42,7 @@ func (p *Periodic) Force(args *structs.PeriodicForceRequest, reply *structs.Peri // Check for write-job permissions if aclObj, err := p.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityDispatchJob) && !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilitySubmitJob) { + } else if !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityDispatchJob) && !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilitySubmitJob) { return structs.ErrPermissionDenied } diff --git a/nomad/scaling_endpoint.go b/nomad/scaling_endpoint.go index 1a9f2544df77..d7b3766fd004 100644 --- a/nomad/scaling_endpoint.go +++ b/nomad/scaling_endpoint.go @@ -45,7 +45,7 @@ func (p *Scaling) ListPolicies(args *structs.ScalingPolicyListRequest, reply *st if aclObj, err := p.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil { + } else { hasListScalingPolicies := aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityListScalingPolicies) hasListAndReadJobs := aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityListJobs) && aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob) @@ -114,7 +114,7 @@ func (p *Scaling) GetPolicy(args *structs.ScalingPolicySpecificRequest, // Check for list-job permissions if aclObj, err := p.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil { + } else { hasReadScalingPolicy := aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadScalingPolicy) hasListAndReadJobs := aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityListJobs) && aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob) diff --git a/nomad/status_endpoint.go b/nomad/status_endpoint.go index 1a21f99e716c..340269abc76d 100644 --- a/nomad/status_endpoint.go +++ b/nomad/status_endpoint.go @@ -92,7 +92,7 @@ func (s *Status) Members(args *structs.GenericRequest, reply *structs.ServerMemb // Check node read permissions if aclObj, err := s.srv.ResolveACL(args); err != nil { return err - } else if aclObj != nil && !aclObj.AllowNodeRead() { + } else if !aclObj.AllowNodeRead() { return structs.ErrPermissionDenied } diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index ce082665518a..332b442f442a 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -493,15 +493,24 @@ func (w WriteRequest) GetIdentity() *AuthenticatedIdentity { // ACLToken makes the original of the credential clear to RPC handlers, who may // have different behavior for internal vs external origins. type AuthenticatedIdentity struct { - // ACLToken authenticated. Claims will be nil if this is set. + // ACLToken authenticated. Claims and ClientID will be unset if this is set. ACLToken *ACLToken - // Claims authenticated by workload identity. ACLToken will be nil if this is - // set. + // Claims authenticated by workload identity. ACLToken and ClientID will be + // unset if this is set. Claims *IdentityClaims + // ClientID is the Nomad client node ID. ACLToken and Claims will be nil if + // this is set. ClientID string - TLSName string + + // TLSName is the name of the TLS certificate, if any. Outside of the + // AuthenticateServerOnly and AuthenticateClientOnly methods, this should be + // used only to identify the request for metrics, not authorization + TLSName string + + // RemoteIP is the name of the connection's IP address; this should be used + // only to identify the request for metrics, not authorization RemoteIP net.IP } @@ -523,7 +532,7 @@ func (ai *AuthenticatedIdentity) String() string { if ai == nil { return "unauthenticated" } - if ai.ACLToken != nil { + if ai.ACLToken != nil && ai.ACLToken != AnonymousACLToken { return "token:" + ai.ACLToken.AccessorID } if ai.Claims != nil { @@ -13079,8 +13088,8 @@ func (a *ACLToken) Copy() *ACLToken { } var ( - // AnonymousACLToken is used no SecretID is provided, and the - // request is made anonymously. + // AnonymousACLToken is used when no SecretID is provided, and the request + // is made anonymously. AnonymousACLToken = &ACLToken{ AccessorID: "anonymous", Name: "Anonymous Token", @@ -13096,6 +13105,14 @@ var ( Name: "Leader Token", Type: ACLManagementToken, } + + // ACLsDisabledToken is used when ACLs are disabled. + ACLsDisabledToken = &ACLToken{ + AccessorID: "acls-disabled", + Name: "ACLs disabled token", + Type: ACLClientToken, + Global: false, + } ) type ACLTokenListStub struct { diff --git a/nomad/system_endpoint.go b/nomad/system_endpoint.go index accd1e4536d0..1fa0a91be822 100644 --- a/nomad/system_endpoint.go +++ b/nomad/system_endpoint.go @@ -36,9 +36,9 @@ func (s *System) GarbageCollect(args *structs.GenericRequest, reply *structs.Gen } // Check management level permissions - if acl, err := s.srv.ResolveACL(args); err != nil { + if aclObj, err := s.srv.ResolveACL(args); err != nil { return err - } else if acl != nil && !acl.IsManagement() { + } else if !aclObj.IsManagement() { return structs.ErrPermissionDenied } @@ -66,9 +66,9 @@ func (s *System) ReconcileJobSummaries(args *structs.GenericRequest, reply *stru } // Check management level permissions - if acl, err := s.srv.ResolveACL(args); err != nil { + if aclObj, err := s.srv.ResolveACL(args); err != nil { return err - } else if acl != nil && !acl.IsManagement() { + } else if !aclObj.IsManagement() { return structs.ErrPermissionDenied } diff --git a/nomad/variables_endpoint.go b/nomad/variables_endpoint.go index 158c88714b5b..1494ee9dae39 100644 --- a/nomad/variables_endpoint.go +++ b/nomad/variables_endpoint.go @@ -96,13 +96,9 @@ func (sv *Variables) Apply(args *structs.VariablesApplyRequest, reply *structs.V if err != nil { return err } - - // IF ACL is being used, - if aclObj != nil { - err := hasOperationPermissions(aclObj, args.Var.Namespace, args.Var.Path, args.Op) - if err != nil { - return err - } + err = hasOperationPermissions(aclObj, args.Var.Namespace, args.Var.Path, args.Op) + if err != nil { + return err } err = canonicalizeAndValidate(args) @@ -270,12 +266,8 @@ func (sv *Variables) makeVariablesApplyResponse( // The read permission modify the way the response is populated. If ACL is not // used, read permission is granted by default and every call is treated as management. - var canRead bool = true - var isManagement = true - if aclObj != nil { - canRead = hasReadPermission(aclObj, req.Var.Namespace, req.Var.Path) - isManagement = aclObj.IsManagement() - } + canRead := hasReadPermission(aclObj, req.Var.Namespace, req.Var.Path) + isManagement := aclObj.IsManagement() if eResp.IsOk() { if eResp.WrittenSVMeta != nil { @@ -633,13 +625,9 @@ func (sv *Variables) RenewLock(args *structs.VariablesRenewLockRequest, reply *s if err != nil { return err } - - // ACLs are enabled, check for the correct permissions - if aclObj != nil { - if !aclObj.AllowVariableOperation(args.WriteRequest.Namespace, args.Path, - acl.VariablesCapabilityWrite, nil) { - return structs.ErrPermissionDenied - } + if !aclObj.AllowVariableOperation(args.WriteRequest.Namespace, args.Path, + acl.VariablesCapabilityWrite, nil) { + return structs.ErrPermissionDenied } if err := args.Validate(); err != nil {