Skip to content

Commit

Permalink
client: send node secret with every client-to-server RPC
Browse files Browse the repository at this point in the history
In Nomad 1.5.3 we fixed a security bug that allowed bypass of ACL checks if the
request came thru a client node first. But this fix broke (knowingly) the
identification of many client-to-server RPCs. These will be now measured as if
they were anonymous. The reason for this is that many client-to-server RPCs do
not send the node secret and instead rely on the protection of mTLS.

This changeset ensures that the node secret is being sent with every
client-to-server RPC request.
  • Loading branch information
tgross committed Apr 5, 2023
1 parent c1a09a0 commit 8d32bf2
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 18 deletions.
24 changes: 17 additions & 7 deletions client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -1878,8 +1878,11 @@ func (c *Client) submitNodeEvents(events []*structs.NodeEvent) error {
nodeID: events,
}
req := structs.EmitNodeEventsRequest{
NodeEvents: nodeEvents,
WriteRequest: structs.WriteRequest{Region: c.Region()},
NodeEvents: nodeEvents,
WriteRequest: structs.WriteRequest{
Region: c.Region(),
AuthToken: c.secretNodeID(),
},
}
var resp structs.EmitNodeEventsResponse
if err := c.RPC("Node.EmitEvents", &req, &resp); err != nil {
Expand Down Expand Up @@ -1997,9 +2000,12 @@ func (c *Client) registerNode() error {
func (c *Client) updateNodeStatus() error {
start := time.Now()
req := structs.NodeUpdateStatusRequest{
NodeID: c.NodeID(),
Status: structs.NodeStatusReady,
WriteRequest: structs.WriteRequest{Region: c.Region()},
NodeID: c.NodeID(),
Status: structs.NodeStatusReady,
WriteRequest: structs.WriteRequest{
Region: c.Region(),
AuthToken: c.secretNodeID(),
},
}
var resp structs.NodeUpdateResponse
if err := c.RPC("Node.UpdateStatus", &req, &resp); err != nil {
Expand Down Expand Up @@ -2145,8 +2151,11 @@ func (c *Client) allocSync() {

// Send to server.
args := structs.AllocUpdateRequest{
Alloc: sync,
WriteRequest: structs.WriteRequest{Region: c.Region()},
Alloc: sync,
WriteRequest: structs.WriteRequest{
Region: c.Region(),
AuthToken: c.secretNodeID(),
},
}

var resp structs.GenericResponse
Expand Down Expand Up @@ -2205,6 +2214,7 @@ func (c *Client) watchAllocations(updates chan *allocUpdates) {
// After the first request, only require monotonically
// increasing state.
AllowStale: false,
AuthToken: c.secretNodeID(),
},
}
var resp structs.NodeClientAllocsResponse
Expand Down
9 changes: 5 additions & 4 deletions client/rpc.go
Original file line number Diff line number Diff line change
Expand Up @@ -434,13 +434,14 @@ func resolveServer(s string) (net.Addr, error) {
return net.ResolveTCPAddr("tcp", net.JoinHostPort(host, port))
}

// Ping never mutates the request, so reuse a singleton to avoid the extra
// malloc
var pingRequest = &structs.GenericRequest{}

// Ping is used to ping a particular server and returns whether it is healthy or
// a potential error.
func (c *Client) Ping(srv net.Addr) error {
pingRequest := &structs.GenericRequest{
QueryOptions: structs.QueryOptions{
AuthToken: c.secretNodeID(),
},
}
var reply struct{}
err := c.connPool.RPC(c.Region(), srv, "Status.Ping", pingRequest, &reply)
return err
Expand Down
7 changes: 0 additions & 7 deletions nomad/acl.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,13 +102,6 @@ func (s *Server) Authenticate(ctx *RPCContext, args structs.RequestWithIdentity)

// At this point we either have an anonymous token or an invalid one.

// TODO(tgross): remove this entirely in 1.6.0 and enforce that all RPCs
// driven by the clients have secret IDs set
if ctx.NodeID != "" && secretID != "" {
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.
Expand Down

0 comments on commit 8d32bf2

Please sign in to comment.