Authenticate method improvements #15734
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
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`.
tgross
force-pushed
the
f-authenticate-identity
branch
from
5a9c2ef to
f763920
Compare
Co-authored-by: James Rasell <[email protected]>
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Authenticatemethod. But the context isshared 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. The
Authenticatemethod taken on its own lookslike it wants to return an
acl.ACLthat folds over all the various identitytypes (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 a value. When reviewing implement pre-forwarding auth on select RPCs #15513 we
decided that forcing the request handler to call
SetIdentitywas repetitiveand error prone. Instead, the
Authenticatemethod mutates the request bysetting its
AuthenticatedIdentity.I'm marking folks other than @schmichael for review on this as he's discussed these changes with me already but isn't going to be available for a review this week, so spreading the design decisions around seems like a good idea.