From cab35b3b1cf3a6084946487c600417d24c5de8a3 Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Tue, 10 Jan 2023 09:46:38 -0500 Subject: [PATCH] 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 | 6 ++-- nomad/acl.go | 56 +++++++++++++++++++++++-------------- nomad/acl_endpoint.go | 7 ++--- nomad/acl_test.go | 4 +-- nomad/namespace_endpoint.go | 7 ++--- nomad/structs/structs.go | 18 ++++++++++++ 6 files changed, 65 insertions(+), 33 deletions(-) diff --git a/.semgrep/rpc_endpoint.yml b/.semgrep/rpc_endpoint.yml index 95f01881f..622251c55 100644 --- a/.semgrep/rpc_endpoint.yml +++ b/.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: diff --git a/nomad/acl.go b/nomad/acl.go index ab73ea705..a8f03f642 100644 --- a/nomad/acl.go +++ b/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) { diff --git a/nomad/acl_endpoint.go b/nomad/acl_endpoint.go index e1e057142..b28e31b69 100644 --- a/nomad/acl_endpoint.go +++ b/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 diff --git a/nomad/acl_test.go b/nomad/acl_test.go index f25da5b97..994b270f9 100644 --- a/nomad/acl_test.go +++ b/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", }, } diff --git a/nomad/namespace_endpoint.go b/nomad/namespace_endpoint.go index 1b7f29760..5cbaf798b 100644 --- a/nomad/namespace_endpoint.go +++ b/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 { diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index f91a23591..ca53fcb56 100644 --- a/nomad/structs/structs.go +++ b/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 {