Authenticate method improvements (#15734)

This changeset covers a sidebar discussion that @schmichael and I had around the
design for pre-forwarding auth. This includes some changes extracted out of
#15513 to make it easier to review both and leave a clean history.

* Remove fast path for NodeID. Previously-connected clients will have a NodeID
  set on the context, and because this is a large portion of the RPCs sent we
  fast-pathed it at the top of the `Authenticate` method. But the context is
  shared for all yamux streams over the same yamux session (and TCP
  connection). This lets an authenticated HTTP request to a client use the
  NodeID for authentication, which is a privilege escalation. Remove the fast
  path and annotate it so that we don't break it again.

* Add context to decisions around AuthenticatedIdentity. The `Authenticate`
  method taken on its own looks like it wants to return an `acl.ACL` that folds
  over all the various identity types (creating an ephemeral ACL on the fly if
  neccessary). But keeping these fields idependent allows RPC handlers to
  differentiate between internal and external origins so we most likely want to
  avoid this. Leave some docstrings as a warning as to why this is built the way
  it is.

* Mutate the request rather than returning. When reviewing #15513 we decided
  that forcing the request handler to call `SetIdentity` was repetitive and
  error prone. Instead, the `Authenticate` method mutates the request by setting
  its `AuthenticatedIdentity`.

.semgrep/rpc_endpoint.yml

@@ -65,9 +65,11 @@ rules:
       # Pattern used by Authenticate method.
       # TODO: add authorization steps as well.
       - pattern-not-inside: |
+          authErr := $A.$B.Authenticate($A.ctx, args)
           ...
-          ... := $A.$B.Authenticate($A.ctx, args.AuthToken)
-          ...
+          if authErr != nil {
+            return authErr
+          }
       - metavariable-pattern:
           metavariable: $METHOD
           patterns:

nomad/acl.go

@@ -15,32 +15,31 @@ import (
 )
 
 // Authenticate extracts an AuthenticatedIdentity from the request context or
-// provided token. The caller can extract an acl.ACL, WorkloadIdentity, or other
-// identifying token to use for authorization.
+// provided token and sets the identity on the request. The caller can extract
+// an acl.ACL, WorkloadIdentity, or other identifying tokens to use for
+// authorization. Keeping these fields independent rather than merging them into
+// an ephemeral ACLToken makes the original of the credential clear to RPC
+// handlers, who may have different behavior for internal vs external origins.
 //
 // Note: when called on the follower we'll be making stale queries, so it's
 // possible if the follower is behind that the leader will get a different value
 // if an ACL token or allocation's WI has just been created.
-func (s *Server) Authenticate(ctx *RPCContext, secretID string) (*structs.AuthenticatedIdentity, error) {
-
-	// Previously-connected clients will have a NodeID set and will be a large
-	// number of the RPCs sent, so we can fast path this case
-	if ctx != nil && ctx.NodeID != "" {
-		return &structs.AuthenticatedIdentity{ClientID: ctx.NodeID}, nil
-	}
+func (s *Server) Authenticate(ctx *RPCContext, args structs.RequestWithIdentity) error {
 
 	// get the user ACLToken or anonymous token
+	secretID := args.GetAuthToken()
 	aclToken, err := s.ResolveSecretToken(secretID)
 
 	switch {
 	case err == nil:
 		// If ACLs are disabled or we have a non-anonymous token, return that.
 		if aclToken == nil || aclToken != structs.AnonymousACLToken {
-			return &structs.AuthenticatedIdentity{ACLToken: aclToken}, nil
+			args.SetIdentity(&structs.AuthenticatedIdentity{ACLToken: aclToken})
+			return nil
 		}
 
 	case errors.Is(err, structs.ErrTokenExpired):
-		return nil, err
+		return err
 
 	case errors.Is(err, structs.ErrTokenInvalid):
 		// if it's not a UUID it might be an identity claim
@@ -49,10 +48,11 @@ func (s *Server) Authenticate(ctx *RPCContext, secretID string) (*structs.Authen
 			// we already know the token wasn't valid for an ACL in the state
 			// store, so if we get an error at this point we have an invalid
 			// token and there are no other options but to bail out
-			return nil, err
+			return err
 		}
 
-		return &structs.AuthenticatedIdentity{Claims: claims}, nil
+		args.SetIdentity(&structs.AuthenticatedIdentity{Claims: claims})
+		return nil
 
 	case errors.Is(err, structs.ErrTokenNotFound):
 		// Check if the secret ID is the leader's secret ID, in which case treat
@@ -66,15 +66,16 @@ func (s *Server) Authenticate(ctx *RPCContext, secretID string) (*structs.Authen
 			node, err := s.State().NodeBySecretID(nil, secretID)
 			if err != nil {
 				// this is a go-memdb error; shouldn't happen
-				return nil, fmt.Errorf("could not resolve node secret: %w", err)
+				return fmt.Errorf("could not resolve node secret: %w", err)
 			}
 			if node != nil {
-				return &structs.AuthenticatedIdentity{ClientID: node.ID}, nil
+				args.SetIdentity(&structs.AuthenticatedIdentity{ClientID: node.ID})
+				return nil
 			}
 		}
 
 	default: // any other error
-		return nil, fmt.Errorf("could not resolve user: %w", err)
+		return fmt.Errorf("could not resolve user: %w", err)
 
 	}
 
@@ -82,10 +83,22 @@ func (s *Server) Authenticate(ctx *RPCContext, secretID string) (*structs.Authen
 	// cases where the leader is making RPCs internally (volumewatcher and
 	// deploymentwatcher)
 	if ctx == nil {
-		return &structs.AuthenticatedIdentity{ACLToken: aclToken}, nil
+		args.SetIdentity(&structs.AuthenticatedIdentity{ACLToken: aclToken})
+		return nil
 	}
 
 	// At this point we either have an anonymous token or an invalid one.
+
+	// Previously-connected clients will have a NodeID set on the context, which
+	// is available for all yamux streams over the same yamux session (and TCP
+	// connection). This will be a large portion of the RPCs sent, but we can't
+	// fast-path this at the top of the method, because authenticated HTTP
+	// requests to the clients will come in over to the same session.
+	if ctx.NodeID != "" {
+		args.SetIdentity(&structs.AuthenticatedIdentity{ClientID: ctx.NodeID})
+		return nil
+	}
+
 	// Unlike clients that provide their Node ID on first connection, server
 	// RPCs don't include an ID for the server so we identify servers by cert
 	// and IP address.
@@ -99,22 +112,23 @@ func (s *Server) Authenticate(ctx *RPCContext, secretID string) (*structs.Authen
 	if ctx.Session != nil {
 		remoteAddr, ok = ctx.Session.RemoteAddr().(*net.TCPAddr)
 		if !ok {
-			return nil, errors.New("session address was not a TCP address")
+			return errors.New("session address was not a TCP address")
 		}
 	}
 	if remoteAddr == nil && ctx.Conn != nil {
 		remoteAddr, ok = ctx.Conn.RemoteAddr().(*net.TCPAddr)
 		if !ok {
-			return nil, errors.New("session address was not a TCP address")
+			return errors.New("session address was not a TCP address")
 		}
 	}
 	if remoteAddr != nil {
 		identity.RemoteIP = remoteAddr.IP
-		return identity, nil
+		args.SetIdentity(identity)
+		return nil
 	}
 
 	s.logger.Error("could not authenticate RPC request or determine remote address")
-	return nil, structs.ErrPermissionDenied
+	return structs.ErrPermissionDenied
 }
 
 func (s *Server) ResolveACL(aclToken *structs.ACLToken) (*acl.ACL, error) {

nomad/acl_endpoint.go

@@ -1997,11 +1997,10 @@ func (a *ACL) GetAuthMethods(
 // once other Workload Identity work is solidified
 func (a *ACL) WhoAmI(args *structs.GenericRequest, reply *structs.ACLWhoAmIResponse) error {
 
-	identity, err := a.srv.Authenticate(a.ctx, args.AuthToken)
-	if err != nil {
-		return err
+	authErr := a.srv.Authenticate(a.ctx, args)
+	if authErr != nil {
+		return authErr
 	}
-	args.SetIdentity(identity)
 
 	if done, err := a.srv.forward("ACL.WhoAmI", args, args, reply); done {
 		return err

nomad/acl_test.go

@@ -219,7 +219,7 @@ func TestAuthenticate_mTLS(t *testing.T) {
 			name:      "from failed workload", // ex. Variables.List
 			tlsCfg:    clientTLSCfg,
 			testToken: claims1Token,
-			expectErr: "rpc error: allocation is terminal",
+			expectErr: "allocation is terminal",
 		},
 		{
 			name:          "from running workload", // ex. Variables.List
@@ -237,7 +237,7 @@ func TestAuthenticate_mTLS(t *testing.T) {
 			name:      "expired user token",
 			tlsCfg:    clientTLSCfg,
 			testToken: token2.SecretID,
-			expectErr: "rpc error: ACL token expired",
+			expectErr: "ACL token expired",
 		},
 	}
 

nomad/namespace_endpoint.go

@@ -26,11 +26,10 @@ func NewNamespaceEndpoint(srv *Server, ctx *RPCContext) *Namespace {
 func (n *Namespace) UpsertNamespaces(args *structs.NamespaceUpsertRequest,
 	reply *structs.GenericResponse) error {
 
-	identity, err := n.srv.Authenticate(n.ctx, args.AuthToken)
-	if err != nil {
-		return err
+	authErr := n.srv.Authenticate(n.ctx, args)
+	if authErr != nil {
+		return authErr
 	}
-	args.SetIdentity(identity)
 
 	args.Region = n.srv.config.AuthoritativeRegion
 	if done, err := n.srv.forward("Namespace.UpsertNamespaces", args, args, reply); done {

nomad/structs/structs.go

@@ -343,6 +343,10 @@ func (q QueryOptions) AllowStaleRead() bool {
 	return q.AllowStale
 }
 
+func (q *QueryOptions) GetAuthToken() string {
+	return q.AuthToken
+}
+
 func (q *QueryOptions) SetIdentity(identity *AuthenticatedIdentity) {
 	q.identity = identity
 }
@@ -449,6 +453,10 @@ func (w WriteRequest) AllowStaleRead() bool {
 	return false
 }
 
+func (w *WriteRequest) GetAuthToken() string {
+	return w.AuthToken
+}
+
 func (w *WriteRequest) SetIdentity(identity *AuthenticatedIdentity) {
 	w.identity = identity
 }
@@ -461,6 +469,10 @@ func (w WriteRequest) GetIdentity() *AuthenticatedIdentity {
 // return a wrapper around the various elements that can be resolved as an
 // identity. RPC handlers will use the relevant fields for performing
 // authorization.
+//
+// Keeping these fields independent rather than merging them into an ephemeral
+// 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 *ACLToken
 	Claims   *IdentityClaims
@@ -484,6 +496,12 @@ func (ai *AuthenticatedIdentity) GetClaims() *IdentityClaims {
 	return ai.Claims
 }
 
+type RequestWithIdentity interface {
+	GetAuthToken() string
+	SetIdentity(identity *AuthenticatedIdentity)
+	GetIdentity() *AuthenticatedIdentity
+}
+
 // QueryMeta allows a query response to include potentially
 // useful metadata about a query
 type QueryMeta struct {