implement pre-forwarding auth on select RPCs #15513
Conversation
tgross
force-pushed
the
pre-forwarding-auth-poc
branch
from
2a2b7b3 to
c1448e6
Compare
tgross
force-pushed
the
pre-forwarding-auth-poc
branch
from
c3be24a to
e970fc2
Compare
nomad/node_endpoint.go
Outdated
| identity, authErr := n.srv.Authenticate(n.ctx, args.AuthToken) | ||
| args.SetIdentity(identity) |
There was a problem hiding this comment.
Instead of having to remember to call args.SetIdentity after every call to Authenticate, what if made Server.Authenticate (or a new wrapper around Authenticate) take an interface that could both retrieve the auth token and set the identity?
This is just a really non-idiomatic pattern to expect devs to get right in every RPC: both understanding when to call SetIdentity (always?) as well as knowing where exactly to check authErr (after forwarding?).
There was a problem hiding this comment.
An alternative might be to actually update our ancient Server.forward method (or make a new method to migrate RPCs piecemeal) to just encapsulate all of this. This would reduce the per-RPC boilerplate as well as only have to write the slightly unusual "check error after a subsequent operation" authErr handling.
There was a problem hiding this comment.
We could definitely call SetIdentity inside Authenticate, as we always want it. Should be easy enough to do.
But although I love the idea of wrapping it into Server.forward in principle, there's a bunch of leader-only RPCs like Eval.Dequeue that don't forward because we always send them to the leader. This is why I was exploring that RPC middleware notion previously (but the type signatures are terrible 😁 )
There was a problem hiding this comment.
Ok, I've moved the SetIdentity into Authenticate, which also cleans up Authenticate's signature so that if we eventually get it to produce ACLs for claims we don't have to change it again everywhere
There was a problem hiding this comment.
I've also updated the semgrep rule so that if we're using Authenticate we expect to return the error at some point. That should minimize the risk of misuse.
|
I've extracted a subset of this PR out into #15734 to pick up some work @schmichael and I discussed in a sidebar (and so that we're keeping the history a little easier to read) |
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`.
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`.
In #15417 we added a new `Authenticate` method to the server that returns an `AuthenticatedIdentity` struct. This changeset implements this method for a small number of RPC endpoints that together represent all the various ways in which RPCs are sent, so that we can validate that we're happy with this approach.
tgross
force-pushed
the
pre-forwarding-auth-poc
branch
from
b21ebf8 to
f15beb3
Compare
|
Rebased and should be ready for final review. |
In #15417 we added a new
Authenticatemethod to the server that returns anAuthenticatedIdentitystruct. This changeset implements this method for a small number of RPC endpoints that together represent all the various ways in which RPCs are sent, so that we can validate that we're happy with this approach.A follow-up PR will do all the dirty work of adding this everywhere else.