cgivre opened a new pull request, #3094: URL: https://github.com/apache/drill/pull/3094
# [DRILL-8558](https://issues.apache.org/jira/browse/DRILL-8558): Fix credential leak and cross-user OAuth token reuse ## Description Two related problems in how Drill stores and serves the credentials a storage plugin uses to authenticate at its data source. ### 1. `/credentials.json` disclosed every user's credentials `CredentialResources` is `@RolesAllowed(AUTHENTICATED_ROLE)`, deliberately: supplying your own data source credentials is not an admin action. But `GET /credentials.json` serialized whole `StoragePluginConfig` objects via `PluginConfigWrapper`, and a config publishes its `credentialsProvider`. For `PlainCredentialsProvider` that is the entire `userCredentials` map — **every** user's stored username and password in cleartext — plus shared secrets such as an OAuth `clientSecret` and `tokenURI`. The equivalent admin-only `/storage/{name}.json` refused the same caller, so a non-admin could read through the credentials endpoint what the storage endpoint denied them. Only `USER_TRANSLATION` plugins were exposed, since that is the endpoint's filter. The sibling `/credentials{group}-plugins.json` looks like it would serve every plugin via `group=all`, but its path regex captures the leading slash, so `pluginGroup` is `"/all"`, `trim()` does not strip it, and the switch always falls through to an empty list. **Fix:** return a new `CredentialPluginInfo` carrying only the fields a caller needs to manage their own credentials — plugin name, enabled state, OAuth flag, the pre-built authorize URL, and whether *this caller* has credentials stored, plus their own stored username. Never a password, never another user's anything, never the config. The server-rendered `/credentials` page keeps the full configs. They never leave the server, and `StoragePluginModel` already scopes `getUserName()`/`getPassword()` to the active user, so a user sees only their own. ### 2. One user's queries could run with another user's OAuth tokens `OAuthTokenProvider` cached a single `TokenRegistry` and honoured its `username` argument only on the first call: ```java public TokenRegistry getOauthTokenRegistry(String username) { if (oauthTokenRegistry == null) { initRemoteRegistries(username); // username used only here } return oauthTokenRegistry; // every later caller gets this one } ``` Under `USER_TRANSLATION` every identity therefore shared one registry, which defeats user translation entirely: tokens written for one user were read by another, including on the query path (`registerSchemas` → `initializeOauthTokenTable(schemaConfig.getUserName())`). The storage plugins compounded it by caching the registry in a mutable field, re-pointed by whichever user planned a query most recently. `GoogleSheetsStoragePlugin` was worse than a race — it cached the authenticated `Sheets` and `Drive` services and the Google `DataStore` and returned the first caller's to every later caller regardless of who asked. **Fix:** resolve registries per identity in `OAuthTokenProvider`, and resolve the token table, data store and Google services per identity in the plugins, from the user the call site already holds (`queryUserName` in the HTTP schema factories, `schemaConfig.getUserName()` in `FileSystemSchemaFactory`). ### Also included - Non-admin writes to a plugin's **shared** OAuth tokens are refused with HTTP 403. Overwriting those changes the identity *all* users' queries run under. Because the `/storage/*` OAuth endpoints are deprecated in favour of the `/credentials/*` ones, admins and non-admins reach the same code, so the check lives in `OAuthRequests` rather than in a resource class. `updateAuthToken` now resolves the target *before* exchanging the authorization code, so a refused caller cannot make Drill spend the client secret or consume a single-use code. - Identity is resolved through `DrillUserPrincipal` rather than `SecurityContext`. With authentication disabled `WebServer` installs no Jetty security handler, so the security context has no principal at all, while queries still run as `anonymous`. Using the principal keeps a user translation plugin's REST writes pointed at the registry the query path actually reads. - The shared registry identity is now spelled consistently. GoogleSheets passed its Google-side placeholder name (`anonymous`) where a Drill registry identity was expected, sending a shared plugin's lookups to `oauth_tokens/anonymous` — a registry the REST endpoints never write to. `DrillDataStoreFactory` now carries the registry identity explicitly, separate from the Google store id. - `PersistentTokenTable.setRefreshToken` and `setExpiresIn` compared the new value against the **access** token. `setRefreshToken` also threw NPE when a refresh token was stored without an access token. - `TestUserTranslationInHttpPlugin#testQueryWithOAuth` stored tokens for `TEST_USER_2` and then queried via the default cluster client. It passed only because every identity shared one registry — it was asserting the bug. It now queries as the user whose tokens it stored. ## Documentation New [`docs/dev/OAuthTokenStorage.md`](docs/dev/OAuthTokenStorage.md): where each kind of credential lives, the registry layout, who may write what, the auth-disabled rules, and the orphaned-token-table limitation. ## Testing | Suite | Result | |---|---| | `CredentialPluginInfoTest` (new, 8 tests) | pass | | `OAuthTokenProviderTest` (new, 7 tests) | pass | | `TestUserTranslationInHttpPlugin` | 5/5 | | `TestOAuthProcess` | 3/3 | | `TestOAuthTokenUpdate` | 3/3 | | `mvn checkstyle:check -pl exec/java-exec` | 0 violations | | Full reactor build | success | `CredentialPluginInfoTest` asserts the response carries no `config`, no `credentialsProvider`, no `userCredentials`, no password, no other user's username, and no shared secret. `OAuthTokenProviderTest` asserts registries are distinct per identity, that a shared write does not clobber a user's tokens, and that `anonymous` is a per-user registry rather than the shared one. ## Reviewer notes **GoogleSheets changes are not verified by tests.** `GoogleSheetsStoragePlugin` is the largest file in this diff and all three of its suites are `@Ignore("Requires live connection to Google Sheets")`. The changes mirror the HTTP plugin's, which is covered by the 11 passing tests above, but someone with Google credentials should run `TestGoogleSheetsQueries`, `TestGoogleSheetsWriter` and `TestGoogleSheetsLimitPushdown` before release. The dfs plugin's OAuth path likewise has no test coverage in-tree. **Upgrade note for release.** An auth-disabled deployment with a `USER_TRANSLATION` OAuth plugin previously wrote tokens to the shared registry and will now read `oauth_tokens/anonymous`. Those users authorize once more; no data is lost. **Known limitation, deliberately not addressed.** Deleting a plugin cannot enumerate every user who held tokens for it, because the user is part of the persistent store *path* rather than a key inside one store (`PersistentAliasRegistry` does the latter and can enumerate). Per-user token tables for users inactive since the last restart are left behind. `OAuthTokenStorage.md` records the three options for a complete fix; all of them are a store format change and none can retroactively find tables already orphaned. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
