Role-restricted API keys#7807
Conversation
cnathe
left a comment
There was a problem hiding this comment.
Here is what Claude code review told me:
Finding #1 — [Severity: Critical] — Auth / Broken access control — core/src/org/labkey/core/login/DbLoginAuthenticationProvider.java:126 (and api/.../SecurityManager.java getSessionUser/attemptAuthentication)
▎ Issue: The role restriction is only ever applied to a session user, never to the user returned for the API-key request itself. Trace of a normal stateless API-key request (apikey header, no session cookie — the standard
▎ usage for curl, Rlabkey, the Python labkey package):
▎ 1. SecurityManager.attemptAuthentication calls getSessionUser(request) → no session → null.
▎ 2. It then calls authenticateBasic(...) → AuthenticationManager.authenticate → handleAuthentication, which calls setAuthenticatedUser and stores RESTRICTION_ROLE_KEY on a newly created session, but returns the unrestricted
▎ primaryAuthUser (response.getUser()).
▎ 3. attemptAuthentication returns new Pair<>(u, request) at SecurityManager.java:642 with that unrestricted user, and AuthFilter.java:178 runs the request as that user.
▎
▎ The new RoleRestrictedUser(...) wrapping happens only in getSessionUser (SecurityManager.java:527), i.e. on a subsequent request that carries the JSESSIONID cookie minted in step 2. getRestrictionRole() is consumed in
▎ exactly one place — storing it on the session (SecurityManager.java:826) — and nowhere applies it to the returned user.
▎ Why it matters: A "read-only" (or Author/Editor-restricted) API key grants the user's full, unrestricted permissions on every stateless request — which is essentially all real API-key traffic, since those clients don't
▎ retain the response cookie. The one flagship use case named in the class Javadoc ("read-only API keys, editor-only API keys") does not actually restrict anything. This is a silent privilege-escalation/authorization bypass on
▎ a security feature whose entire purpose is to limit access.
▎ Suggestion: Apply the restriction to the user that is actually returned from the API-key auth path, not just to the session. The cleanest spot is where the restriction role is already known — DbLoginAuthenticationProvider
▎ line 123/126: wrap the user in the response, e.g. AuthenticationResponse.success(configuration, new RoleRestrictedUser(auth.getUser(), auth.restrictionRole())) when auth.restrictionRole() != null. Since
▎ finalizePrimaryAuthentication uses response.getUser() as primaryAuthUser, the restricted user then flows back through authenticate() → attemptAuthentication → AuthFilter.
|
@labkey-adam what about the app user profile page usage of this APIKeysPanel component to generate API keys. Do we want to bump the components package version there as well (i.e. in limsModules) to pick up this change? |
Thanks, good point... we will want to update the component there as well. I'll create a PR. |
…ion into ApiKeyManager to guarantee consistency. Make PermissionsContexts responsible for stashing session attributes. Stash permissions instead of a role for simplicity.
…fb_role_restricted_api_keys
Rationale
Add support for restricting an API key to a specific role such as Reader or Editor. https://github.com/LabKey/kanban/issues/1846
Related Pull Requests