From fe2098f4c2d034dc4f9027d13b99a29dbb978aff Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Tue, 14 Mar 2023 10:35:43 -0400 Subject: [PATCH] client: pass secret in `Status.Members` RPC In #16217 we switched clients using Consul discovery to the `Status.Members` endpoint for getting the list of servers so that we're using the correct address. This endpoint is authenticated, and we failed to add the `AuthToken` with the client secret to the request. This changeset also expands the test coverage of the RPC endpoint and closes up potential holes in the `ResolveACL` method that aren't currently bugs but easily could become bugs if we called the method without ensuring its invariants are upheld. --- .changelog/16490.txt | 3 ++ client/client.go | 3 +- nomad/acl.go | 12 ++++++-- nomad/status_endpoint.go | 12 +++++--- nomad/status_endpoint_test.go | 58 ++++++++++++++++++++--------------- 5 files changed, 57 insertions(+), 31 deletions(-) create mode 100644 .changelog/16490.txt diff --git a/.changelog/16490.txt b/.changelog/16490.txt new file mode 100644 index 000000000000..42584d3f7461 --- /dev/null +++ b/.changelog/16490.txt @@ -0,0 +1,3 @@ +```release-note:bug +client: Fixed a bug where clients using Consul discovery to join the cluster would get permission denied errors +``` diff --git a/client/client.go b/client/client.go index 09b2be0db030..4667f453980a 100644 --- a/client/client.go +++ b/client/client.go @@ -2910,7 +2910,8 @@ func (c *Client) consulDiscoveryImpl() error { region := c.Region() rpcargs := structs.GenericRequest{ QueryOptions: structs.QueryOptions{ - Region: region, + Region: region, + AuthToken: c.secretNodeID(), }, } diff --git a/nomad/acl.go b/nomad/acl.go index f2a7a5058a4e..38947d7f834b 100644 --- a/nomad/acl.go +++ b/nomad/acl.go @@ -161,9 +161,14 @@ func (s *Server) remoteIPFromRPCContext(ctx *RPCContext) (net.IP, error) { // for the identity they intend the operation to be performed with. func (s *Server) ResolveACL(args structs.RequestWithIdentity) (*acl.ACL, error) { identity := args.GetIdentity() - if !s.config.ACLEnabled || identity == nil { + if !s.config.ACLEnabled { return nil, nil } + if identity == nil { + // Server.Authenticate should never return a nil identity unless there's + // an authentication error, but enforce that invariant here + return nil, structs.ErrPermissionDenied + } aclToken := identity.GetACLToken() if aclToken != nil { return s.ResolveACLForToken(aclToken) @@ -172,7 +177,10 @@ func (s *Server) ResolveACL(args structs.RequestWithIdentity) (*acl.ACL, error) if claims != nil { return s.ResolveClaims(claims) } - return nil, nil + + // return an error here so that we enforce the invariant that we check for + // Identity.ClientID before trying to resolve ACLs + return nil, structs.ErrPermissionDenied } // ResolveACLForToken resolves an ACL from a token only. It should be used only diff --git a/nomad/status_endpoint.go b/nomad/status_endpoint.go index f5b53a5c99c1..704b341dcfbe 100644 --- a/nomad/status_endpoint.go +++ b/nomad/status_endpoint.go @@ -86,11 +86,15 @@ func (s *Status) Members(args *structs.GenericRequest, reply *structs.ServerMemb if authErr != nil { return structs.ErrPermissionDenied } + // Check node read permissions - if aclObj, err := s.srv.ResolveACL(args); err != nil { - return err - } else if aclObj != nil && !aclObj.AllowNodeRead() { - return structs.ErrPermissionDenied + if args.GetIdentity().ClientID == "" { + aclObj, err := s.srv.ResolveACL(args) + if err != nil { + return err + } else if aclObj != nil && !aclObj.AllowNodeRead() { + return structs.ErrPermissionDenied + } } serfMembers := s.srv.Members() diff --git a/nomad/status_endpoint_test.go b/nomad/status_endpoint_test.go index e7eb6901c645..05d67a5a3c7b 100644 --- a/nomad/status_endpoint_test.go +++ b/nomad/status_endpoint_test.go @@ -10,6 +10,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" ) @@ -99,12 +100,13 @@ func TestStatusMembers_ACL(t *testing.T) { s1, root, cleanupS1 := TestACLServer(t, nil) defer cleanupS1() codec := rpcClient(t, s1) - assert := assert.New(t) - state := s1.fsm.State() + store := s1.fsm.State() // Create the namespace policy and tokens - validToken := mock.CreatePolicyAndToken(t, state, 1001, "test-valid", mock.NodePolicy(acl.PolicyRead)) - invalidToken := mock.CreatePolicyAndToken(t, state, 1003, "test-invalid", mock.AgentPolicy(acl.PolicyRead)) + validToken := mock.CreatePolicyAndToken(t, store, 1001, "test-valid", mock.NodePolicy(acl.PolicyRead)) + invalidToken := mock.CreatePolicyAndToken(t, store, 1003, "test-invalid", mock.AgentPolicy(acl.PolicyRead)) + node := mock.Node() + must.NoError(t, store.UpsertNode(structs.MsgTypeTestSetup, 1004, node)) arg := &structs.GenericRequest{ QueryOptions: structs.QueryOptions{ @@ -113,38 +115,46 @@ func TestStatusMembers_ACL(t *testing.T) { }, } - // Try without a token and expect failure - { + t.Run("without token should fail", func(t *testing.T) { var out structs.ServerMembersResponse err := msgpackrpc.CallWithCodec(codec, "Status.Members", arg, &out) - assert.NotNil(err) - assert.Equal(err.Error(), structs.ErrPermissionDenied.Error()) - } + must.ErrorContains(t, err, structs.ErrPermissionDenied.Error()) + }) - // Try with an invalid token and expect failure - { + t.Run("unauthorized token should fail", func(t *testing.T) { arg.AuthToken = invalidToken.SecretID var out structs.ServerMembersResponse err := msgpackrpc.CallWithCodec(codec, "Status.Members", arg, &out) - assert.NotNil(err) - assert.Equal(err.Error(), structs.ErrPermissionDenied.Error()) - } + must.ErrorContains(t, err, structs.ErrPermissionDenied.Error()) + }) + + t.Run("invalid token should fail", func(t *testing.T) { + arg.AuthToken = uuid.Generate() + var out structs.ServerMembersResponse + err := msgpackrpc.CallWithCodec(codec, "Status.Members", arg, &out) + must.ErrorContains(t, err, structs.ErrPermissionDenied.Error()) + }) - // Try with a valid token - { + t.Run("valid token should work", func(t *testing.T) { arg.AuthToken = validToken.SecretID var out structs.ServerMembersResponse - assert.Nil(msgpackrpc.CallWithCodec(codec, "Status.Members", arg, &out)) - assert.Len(out.Members, 1) - } + must.NoError(t, msgpackrpc.CallWithCodec(codec, "Status.Members", arg, &out)) + must.Len(t, 1, out.Members) + }) - // Try with a management token - { + t.Run("management token should work", func(t *testing.T) { arg.AuthToken = root.SecretID var out structs.ServerMembersResponse - assert.Nil(msgpackrpc.CallWithCodec(codec, "Status.Members", arg, &out)) - assert.Len(out.Members, 1) - } + must.NoError(t, msgpackrpc.CallWithCodec(codec, "Status.Members", arg, &out)) + must.Len(t, 1, out.Members) + }) + + t.Run("valid node token should work", func(t *testing.T) { + arg.AuthToken = node.SecretID + var out structs.ServerMembersResponse + must.NoError(t, msgpackrpc.CallWithCodec(codec, "Status.Members", arg, &out)) + must.Len(t, 1, out.Members) + }) } func TestStatus_HasClientConn(t *testing.T) {