[
https://issues.apache.org/jira/browse/DRILL-8558?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18130548#comment-18130548
]
ASF GitHub Bot commented on DRILL-8558:
---------------------------------------
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)
> Fix Credential Race Condition and Credential Leak
> -------------------------------------------------
>
> Key: DRILL-8558
> URL: https://issues.apache.org/jira/browse/DRILL-8558
> Project: Apache Drill
> Issue Type: Bug
> Components: Storage - GoogleSheets, Storage - HTTP
> Affects Versions: 1.22.0
> Reporter: Charles Givre
> Assignee: Charles Givre
> Priority: Major
> Fix For: 1.23.0
>
>
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
