server: enforce per-key ACL on the bulk key-listing route - #128
Open
herdiyana256 wants to merge 1 commit into
Open
server: enforce per-key ACL on the bulk key-listing route#128herdiyana256 wants to merge 1 commit into
herdiyana256 wants to merge 1 commit into
Conversation
getKeysHandler's no-query-string path (GET /v0/keys/) returned every key ID in the instance via KeyManager.GetAllKeyIDs(), with no ACL check at all -- unlike every single-key route, which calls authorizeRequest before returning anything. Any principal that could authenticate at all, regardless of any grant on any key, got the full list. GetAllKeyIDs and GetUpdatedKeyIDs now take the calling principal and only return IDs the principal has at least Read access to. ACL data is already stored unencrypted in DBKey, so this doesn't require decrypting key material to filter.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
GET /v0/keys/ with no query string (getKeysHandler's list-all path) returns every key ID in the instance through KeyManager.GetAllKeyIDs(), with no ACL check anywhere in the call path. Every single-key route (getKeyHandler, deleteKeyHandler, putAccessHandler, ...) calls authorizeRequest before returning anything; this one doesn't, so any principal that can authenticate at all -- a machine with an mTLS cert, a service, any user -- gets the full list of key IDs regardless of whether it has a grant on any of them.
Key IDs aren't meant to be public: they're chosen by whoever creates the key and routinely encode what the secret is for, which is exactly the kind of thing an ACL is supposed to keep scoped.
GetAllKeyIDs and GetUpdatedKeyIDs now take the calling principal and filter to keys the principal has at least Read access to. ACL is stored unencrypted in DBKey already, so this doesn't need to decrypt key data to filter -- it's just checking principal.CanAccess(k.ACL, knox.Read) against the same ACL that authorizeRequest already checks for single-key routes. Added a test covering: an unrelated principal with no grants sees nothing, the owner still sees both of their keys, and a principal granted Read on only one of two keys sees exactly that one.