auth: remove "mixed auth" special casing for Variables endpoint

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 third in a series to eliminate
the use of `nil` ACLs as a sentinel value for when ACLs are disabled.

This patch involves leveraging the refactored `auth` package to remove the weird
"mixed auth" helper functions that only support the Variables read/list RPC
handlers. Instead, pass the ACL object and claim together into the
`AllowVariableOperations` method in the usual `acl` package.

Ref: hashicorp/nomad-enterprise#1218
Ref: #18703
Ref: #18715
Ref: #16799
Ref: #18730

Fixes: #15875

acl/acl.go

@@ -493,6 +493,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
+	}
 	if a.management {
 		return true
 	}
@@ -517,6 +522,11 @@ type ACLClaim struct {
 // a variables path for the namespace, with an expectation that the actual
 // 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
+	}
 	if a.management {
 		return true
 	}

nomad/acl.go

@@ -40,14 +40,6 @@ func (srv *Server) ResolvePoliciesForClaims(claims *structs.IdentityClaims) ([]*
 	return srv.auth.ResolvePoliciesForClaims(claims)
 }
 
-func (srv *Server) ResolveACLForToken(aclToken *structs.ACLToken) (*acl.ACL, error) {
-	return srv.auth.ResolveACLForToken(aclToken)
-}
-
 func (srv *Server) ResolveSecretToken(secretID string) (*structs.ACLToken, error) {
 	return srv.auth.ResolveSecretToken(secretID)
 }
-
-func (srv *Server) ResolveClaims(claims *structs.IdentityClaims) (*acl.ACL, error) {
-	return srv.auth.ResolveClaims(claims)
-}

nomad/acl_test.go

@@ -686,120 +686,3 @@ func TestResolveSecretToken(t *testing.T) {
 		})
 	}
 }
-
-func TestResolveClaims(t *testing.T) {
-	ci.Parallel(t)
-
-	srv, _, cleanup := TestACLServer(t, nil)
-	defer cleanup()
-
-	store := srv.fsm.State()
-	index := uint64(100)
-
-	alloc := mock.Alloc()
-
-	claims := &structs.IdentityClaims{
-		Namespace:    alloc.Namespace,
-		JobID:        alloc.Job.ID,
-		AllocationID: alloc.ID,
-		TaskName:     alloc.Job.TaskGroups[0].Tasks[0].Name,
-	}
-
-	// unrelated policy
-	policy0 := mock.ACLPolicy()
-
-	// policy for job
-	policy1 := mock.ACLPolicy()
-	policy1.JobACL = &structs.JobACL{
-		Namespace: claims.Namespace,
-		JobID:     claims.JobID,
-	}
-
-	// policy for job and group
-	policy2 := mock.ACLPolicy()
-	policy2.JobACL = &structs.JobACL{
-		Namespace: claims.Namespace,
-		JobID:     claims.JobID,
-		Group:     alloc.Job.TaskGroups[0].Name,
-	}
-
-	// policy for job and group	and task
-	policy3 := mock.ACLPolicy()
-	policy3.JobACL = &structs.JobACL{
-		Namespace: claims.Namespace,
-		JobID:     claims.JobID,
-		Group:     alloc.Job.TaskGroups[0].Name,
-		Task:      claims.TaskName,
-	}
-
-	// policy for job and group	but different task
-	policy4 := mock.ACLPolicy()
-	policy4.JobACL = &structs.JobACL{
-		Namespace: claims.Namespace,
-		JobID:     claims.JobID,
-		Group:     alloc.Job.TaskGroups[0].Name,
-		Task:      "another",
-	}
-
-	// policy for job but different group
-	policy5 := mock.ACLPolicy()
-	policy5.JobACL = &structs.JobACL{
-		Namespace: claims.Namespace,
-		JobID:     claims.JobID,
-		Group:     "another",
-	}
-
-	// policy for same namespace but different job
-	policy6 := mock.ACLPolicy()
-	policy6.JobACL = &structs.JobACL{
-		Namespace: claims.Namespace,
-		JobID:     "another",
-	}
-
-	// policy for same job in different namespace
-	policy7 := mock.ACLPolicy()
-	policy7.JobACL = &structs.JobACL{
-		Namespace: "another",
-		JobID:     claims.JobID,
-	}
-
-	aclObj, err := srv.ResolveClaims(claims)
-	must.Nil(t, aclObj)
-	must.EqError(t, err, "allocation does not exist")
-
-	// upsert the allocation
-	index++
-	err = store.UpsertAllocs(structs.MsgTypeTestSetup, index, []*structs.Allocation{alloc})
-	must.NoError(t, err)
-
-	// Resolve claims and check we that the ACL object without policies provides no access
-	aclObj, err = srv.ResolveClaims(claims)
-	must.NoError(t, err)
-	must.NotNil(t, aclObj)
-	must.False(t, aclObj.AllowNamespaceOperation("default", acl.NamespaceCapabilityListJobs))
-
-	// Add the policies
-	index++
-	err = store.UpsertACLPolicies(structs.MsgTypeTestSetup, index, []*structs.ACLPolicy{
-		policy0, policy1, policy2, policy3, policy4, policy5, policy6, policy7})
-	must.NoError(t, err)
-
-	// Re-resolve and check that the resulting ACL looks reasonable
-	aclObj, err = srv.ResolveClaims(claims)
-	must.NoError(t, err)
-	must.NotNil(t, aclObj)
-	must.False(t, aclObj.IsManagement())
-	must.True(t, aclObj.AllowNamespaceOperation("default", acl.NamespaceCapabilityListJobs))
-	must.False(t, aclObj.AllowNamespaceOperation("other", acl.NamespaceCapabilityListJobs))
-
-	// Resolve the same claim again, should get cache value
-	aclObj2, err := srv.ResolveClaims(claims)
-	must.NoError(t, err)
-	must.NotNil(t, aclObj)
-	must.Eq(t, aclObj, aclObj2, must.Sprintf("expected cached value"))
-
-	policies, err := srv.ResolvePoliciesForClaims(claims)
-	must.NoError(t, err)
-	must.Len(t, 3, policies)
-	must.SliceContainsAll(t, policies, []*structs.ACLPolicy{policy1, policy2, policy3})
-}

nomad/auth/auth.go

@@ -194,24 +194,37 @@ func (s *Authenticator) Authenticate(ctx RPCContext, args structs.RequestWithIde
 	return nil
 }
 
-// ResolveACL is an authentication wrapper which handles resolving both ACL
-// tokens and Workload Identities. If both are provided the ACL token is
-// preferred, but it is best for the RPC caller to only include the credentials
-// for the identity they intend the operation to be performed with.
+// ResolveACL is an authentication wrapper which handles resolving ACL tokens,
+// Workload Identities, or client secrets into acl.ACL objects. Exclusively
+// server-to-server or client-to-server requests should by using
+// AuthenticateServerOnly or AuthenticateClientOnly and never use this method.
 func (s *Authenticator) ResolveACL(args structs.RequestWithIdentity) (*acl.ACL, error) {
 	identity := args.GetIdentity()
-	if !s.aclsEnabled || identity == nil {
+	if identity == nil {
+		// should never happen
+		return nil, structs.ErrPermissionDenied
+	}
+
+	if !s.aclsEnabled {
 		return nil, nil
 	}
-	aclToken := identity.GetACLToken()
-	if aclToken != nil {
-		return s.ResolveACLForToken(aclToken)
+
+	if identity.ClientID != "" {
+		return acl.ClientACL, nil
 	}
 	claims := identity.GetClaims()
 	if claims != nil {
-		return s.ResolveClaims(claims)
+		return s.resolveClaims(claims)
 	}
-	return nil, nil
+
+	// this will include any anonymous token, so this is the last chance to
+	// avoid an error
+	aclToken := identity.GetACLToken()
+	if aclToken != nil {
+		return s.resolveACLForToken(aclToken)
+	}
+
+	return nil, structs.ErrPermissionDenied
 }
 
 // AuthenticateServerOnly returns an ACL object for use *only* with internal
@@ -363,16 +376,35 @@ func validateCertificateForNames(cert *x509.Certificate, expectedNames []string)
 
 }
 
-// ResolveACLForToken resolves an ACL from a token only. It should be used only
+// ToACLClaim returns an ACLClaim suitable for checking permissions
+func IdentityToACLClaim(ai *structs.AuthenticatedIdentity, store *state.StateStore) *acl.ACLClaim {
+	if ai == nil || ai.Claims == nil {
+		return nil
+	}
+
+	var group string
+	alloc, err := store.AllocByID(nil, ai.Claims.AllocationID)
+	if err != nil {
+		// we should never hit this error, but if we did the caller would get a
+		// nil claim and auth will fail
+		return nil
+	}
+	if alloc != nil {
+		group = alloc.TaskGroup
+	}
+
+	return &acl.ACLClaim{
+		Namespace: ai.Claims.Namespace,
+		Job:       ai.Claims.JobID,
+		Group:     group,
+		Task:      ai.Claims.TaskName,
+	}
+}
+
+// resolveACLForToken resolves an ACL from a token only. It should be used only
 // by Variables endpoints, which have additional implicit policies for their
 // claims so we can't wrap them up in ResolveACL.
-//
-// TODO: figure out a way to the Variables endpoint implicit policies baked into
-// their acl.ACL object so that we can avoid using this method.
-func (s *Authenticator) ResolveACLForToken(aclToken *structs.ACLToken) (*acl.ACL, error) {
-	if !s.aclsEnabled {
-		return nil, nil
-	}
+func (s *Authenticator) resolveACLForToken(aclToken *structs.ACLToken) (*acl.ACL, error) {
 	snap, err := s.getState().Snapshot()
 	if err != nil {
 		return nil, err
@@ -433,7 +465,7 @@ func (s *Authenticator) VerifyClaim(token string) (*structs.IdentityClaims, erro
 	return claims, nil
 }
 
-func (s *Authenticator) ResolveClaims(claims *structs.IdentityClaims) (*acl.ACL, error) {
+func (s *Authenticator) resolveClaims(claims *structs.IdentityClaims) (*acl.ACL, error) {
 
 	policies, err := s.ResolvePoliciesForClaims(claims)
 	if err != nil {

nomad/auth/auth_test.go

@@ -124,7 +124,8 @@ func TestAuthenticateDefault(t *testing.T) {
 
 				aclObj, err := auth.ResolveACL(args)
 				must.NoError(t, err)
-				must.Nil(t, aclObj)
+				must.NotNil(t, aclObj)
+				must.True(t, aclObj.AllowClientOp())
 			},
 		},
 		{
@@ -840,6 +841,57 @@ func TestResolveACLToken(t *testing.T) {
 	}
 }
 
+func TestIdentityToACLClaim(t *testing.T) {
+
+	alloc := mock.Alloc()
+	alloc.ClientStatus = structs.AllocClientStatusRunning
+	tg := alloc.Job.LookupTaskGroup(alloc.TaskGroup)
+	task := tg.Tasks[0]
+
+	//	claims := alloc.ToTaskIdentityClaims(alloc.Job, "web")
+
+	defaultWI := &structs.WorkloadIdentity{Name: "default"}
+	claims := structs.NewIdentityClaims(alloc.Job, alloc,
+		task.IdentityHandle(defaultWI), task.Identity, time.Now())
+
+	store := testStateStore(t)
+
+	leaderACL := uuid.Generate()
+
+	auth := NewAuthenticator(&AuthenticatorConfig{
+		StateFn:        func() *state.StateStore { return store },
+		Logger:         testlog.HCLogger(t),
+		GetLeaderACLFn: func() string { return leaderACL },
+		AclsEnabled:    true,
+		TLSEnabled:     true,
+		Region:         "global",
+		Encrypter:      newTestEncrypter(),
+	})
+
+	store.UpsertAllocs(structs.MsgTypeTestSetup, 100,
+		[]*structs.Allocation{alloc})
+
+	token, err := auth.encrypter.(*testEncrypter).signClaim(claims)
+	must.NoError(t, err)
+
+	ctx := newTestContext(t, "client.nomad.global", "192.168.1.1")
+	args := &structs.GenericRequest{}
+	args.AuthToken = token
+
+	err = auth.Authenticate(ctx, args)
+	must.NoError(t, err)
+
+	claim := IdentityToACLClaim(args.GetIdentity(), auth.getState())
+	must.Eq(t, &acl.ACLClaim{
+		Namespace: alloc.Job.Namespace,
+		Job:       alloc.Job.ID,
+		Group:     alloc.TaskGroup,
+		Task:      alloc.Job.TaskGroups[0].Tasks[0].Name,
+	}, claim)
+
+	must.Nil(t, IdentityToACLClaim(nil, auth.getState()))
+}
+
 func TestResolveSecretToken(t *testing.T) {
 	ci.Parallel(t)
 	auth := testDefaultAuthenticator(t)
@@ -1001,7 +1053,7 @@ func TestResolveClaims(t *testing.T) {
 		JobID:     claims.JobID,
 	}
 
-	aclObj, err := auth.ResolveClaims(claims)
+	aclObj, err := auth.resolveClaims(claims)
 	must.Nil(t, aclObj)
 	must.EqError(t, err, "allocation does not exist")
 
@@ -1011,7 +1063,7 @@ func TestResolveClaims(t *testing.T) {
 	must.NoError(t, err)
 
 	// Resolve claims and check we that the ACL object without policies provides no access
-	aclObj, err = auth.ResolveClaims(claims)
+	aclObj, err = auth.resolveClaims(claims)
 	must.NoError(t, err)
 	must.NotNil(t, aclObj)
 	must.False(t, aclObj.AllowNamespaceOperation("default", acl.NamespaceCapabilityListJobs))
@@ -1023,15 +1075,15 @@ func TestResolveClaims(t *testing.T) {
 	must.NoError(t, err)
 
 	// Re-resolve and check that the resulting ACL looks reasonable
-	aclObj, err = auth.ResolveClaims(claims)
+	aclObj, err = auth.resolveClaims(claims)
 	must.NoError(t, err)
 	must.NotNil(t, aclObj)
 	must.False(t, aclObj.IsManagement())
 	must.True(t, aclObj.AllowNamespaceOperation("default", acl.NamespaceCapabilityListJobs))
 	must.False(t, aclObj.AllowNamespaceOperation("other", acl.NamespaceCapabilityListJobs))
 
 	// Resolve the same claim again, should get cache value
-	aclObj2, err := auth.ResolveClaims(claims)
+	aclObj2, err := auth.resolveClaims(claims)
 	must.NoError(t, err)
 	must.NotNil(t, aclObj)
 	must.Eq(t, aclObj, aclObj2, must.Sprintf("expected cached value"))
@@ -1042,7 +1094,7 @@ func TestResolveClaims(t *testing.T) {
 	must.SliceContainsAll(t, policies, []*structs.ACLPolicy{policy1, policy2, policy3})
 
 	// Check the dispatch claims
-	aclObj3, err := auth.ResolveClaims(dispatchClaims)
+	aclObj3, err := auth.resolveClaims(dispatchClaims)
 	must.NoError(t, err)
 	must.NotNil(t, aclObj)
 	must.Eq(t, aclObj, aclObj3, must.Sprintf("expected cached value"))