DRILL-8558: Fix credential leak and cross-user OAuth token reuse by cgivre · Pull Request #3094 · apache/drill

cgivre · 2026-10-22T20:05:07Z

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:

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: 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

pjfanning · 2026-10-23T18:31:13Z

-          .message("Error connecting to Googlesheets Service: " + e.getMessage())
-          .build(logger);
-      }
+    return sheetsServices.computeIfAbsent(cacheKey(queryUser), k -> buildSheetsService(queryUser));


These per-identity caches pin whatever tokens existed when the entry was first built. GoogleSheetsUtils.loadCredential copies the access/refresh tokens into the Credential once, and DrillDataStore only reads the token table in its constructor. So if a user queries before authorizing, an empty credential is cached in sheetsServices / driveServices / dataStores until restart, and re-authorizing (e.g. after revoking access) has no effect.

The old single-field cache had the same flaw, but under USER_TRANSLATION the usual flow for a new user is "query, get an auth error, authorize, query again", so most users will now hit it. Could we avoid caching when the stored credential has no tokens, and/or evict a user's entries when OAuthRequests writes their tokens? The Google Sheets suites are all @Ignored, so nothing in-tree would catch this.

Good catch, fixed in e7ff67c. The per-identity data store and the Sheets/Drive services are now tagged with the access token they were built from: nothing is cached while that identity has no access token, and an entry is dropped as soon as the stored token differs from its tag. So "query, auth error, authorize, query again" works without a restart, and so does re-authorizing after a revoke. A token refresh rewrites the stored token too, which costs one extra rebuild.

I went with "do not cache without tokens" rather than having OAuthRequests evict, since that would mean a callback from java-exec into the contrib plugins. Written up in docs/dev/OAuthTokenStorage.md, since you are right that nothing in-tree would catch it.

pjfanning · 2026-10-23T18:31:13Z

+      // The name must be the one queries run under, or tokens would be written
+      // to a table the query path never reads. With authentication disabled that
+      // is the anonymous user, which is also what the query session is given.
+      return principal.getName();


With auth disabled this writes user-translation tokens to oauth_tokens/anonymous. Only web UI / REST queries run as anonymous, though. JDBC/ODBC/sqlline clients with auth off run as whatever user name the client sends (usually the OS user), so they will no longer find these tokens. Previously they all used the shared registry.

The upgrade note in the PR description only covers re-authorizing. It should also say that non-web clients on an auth-disabled cluster lose OAuth access under user translation. Alternatively, keep the shared registry when auth is disabled; there's no real isolation in that mode anyway, since every caller is admin and the user name is client-supplied.

Documented rather than changed, because USER_TRANSLATION with authentication off is not a supported combination in the first place: translating users means giving each one their own credentials, which needs user names that are unique and trusted, and with auth off the name is anonymous on web/REST and whatever the client sends everywhere else, with every caller an admin. Keeping the shared registry in that mode would also mean teaching the query path in all three plugins to read the auth flag, to prop up a configuration that cannot work.

So docs/dev/OAuthTokenStorage.md now has a "User translation needs authentication" section saying exactly that, and pointing at SHARED_USER or turning authentication on.

pjfanning · 2026-10-23T18:31:13Z

   * @return If USER_TRANSLATION is enabled, returns the query user.  If not, returns null.
   */
+  /**
+   * The owner of the token table belonging to this plugin: the caller for a user


Nit: the old javadoc block just above wasn't removed, so getQueryUser now has two javadoc comments stacked.

Removed, thanks.

pjfanning · 2026-10-23T18:31:13Z

                                       @FormParam("username") String username,
                                       @FormParam("password") String password) {
-    String queryUser = sc.getUserPrincipal().getName();
+        String queryUser = queryUser();


Nit: indented 8 spaces instead of 4. The same line in createOrUpdatePlugin (L239) has the same issue.

Fixed both, thanks.

pjfanning · 2026-10-23T18:31:13Z

  private final DrillbitContext context;

-  private PersistentTokenRegistry oauthTokenRegistry;
+  private final Map<String, PersistentTokenRegistry> oauthTokenRegistries = new ConcurrentHashMap<>();


This map, and the three per-identity maps in GoogleSheetsStoragePlugin, only ever grow: one entry per distinct user for the life of the drillbit. That's fine with a handful of users. It's worth a sentence in OAuthTokenStorage.md next to the orphaned-token-table limitation, or a bounded/expiring cache if many users are expected.

Added a "Known limitation: per-identity caches are unbounded" section in docs/dev/OAuthTokenStorage.md, next to the orphaned-token-table one. It names both caches, notes the entries are small and that a deployment with very many distinct users wants a bounded or expiring cache, and that nothing in the design depends on an entry living forever since both are rebuilt on demand.

pjfanning · 2026-10-23T18:31:13Z

+   */
+  private static String registryPath(String username) {
+    return StringUtils.isNotEmpty(username)
+      ? STORAGE_REGISTRY_PATH + "/" + username


The user name goes straight into the persistent store path. That was already true before, but per-user registries are now actually used. A name containing / (e.g. a full Kerberos principal like user/host@REALM) or .. would map to an unexpected path, possibly one belonging to another identity. Consider escaping/encoding the name, or rejecting names that contain path separators.

The user name is now percent-encoded before it becomes a path segment (URLEncoder plus "." to %2E, which URLEncoder leaves alone), so a full Kerberos principal or a name containing ".." stays inside its own segment. Encoding rather than rejecting, so a Kerberos deployment is not locked out of a plugin by its own principal names.

Existing per-user paths move as a result, which the upgrade note already covers: those users re-authorize once.

OAuthTokenProviderTest#testUserNamesCannotEscapeTheirOwnPath guards it. Being straight about it: it is a regression guard, not a reproduction, as I could not get the unencoded path to actually collide under the test persistent store.

GET /credentials.json is reachable by any authenticated user, since supplying one's own data source credentials is not an admin action. It serialized whole StoragePluginConfig objects, and a config publishes its credentialsProvider: for PlainCredentialsProvider that is every user's stored username and password in cleartext, plus shared secrets such as an OAuth clientSecret. The equivalent admin-only /storage/{name}.json refused the same caller. Return CredentialPluginInfo instead, carrying only what the 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. The server-rendered /credentials page keeps the full configs; they never leave the server and StoragePluginModel already scopes them to the active user. Separately, OAuthTokenProvider cached a single TokenRegistry and honoured its username argument only on the first call, so every later lookup returned that registry whatever identity was requested. Under USER_TRANSLATION this let one user's queries run with another user's OAuth tokens at the data source. Resolve a registry per identity instead. The storage plugins compounded this by caching the registry in a mutable field, re-pointed by whichever user planned a query most recently. GoogleSheets also cached the authenticated Sheets and Drive services and the Google DataStore, returning the first caller's to every later caller regardless of who asked. Resolve all of these per identity, from the user the call site already holds. Also in this change: - Refuse non-admin writes to a plugin's shared OAuth tokens with HTTP 403, and resolve the target before exchanging the authorization code, so a refused caller cannot make Drill spend the client secret or consume a single-use code. - Resolve the caller through DrillUserPrincipal rather than SecurityContext. With authentication disabled there is no Jetty security handler and so no principal on the security context, while queries still run as the anonymous user. Using the principal keeps a user translation plugin's REST writes pointed at the registry the query path reads. - Keep the shared registry identity spelled consistently. GoogleSheets passed its Google-side placeholder name where a Drill registry identity was expected, which sent a shared plugin's lookups to a per-user registry that the REST endpoints never write to. DrillDataStoreFactory now carries the registry identity explicitly. - Fix PersistentTokenTable.setRefreshToken and setExpiresIn, which compared the new value against the access token. - Fix TestUserTranslationInHttpPlugin#testQueryWithOAuth, which stored tokens for one user and then queried as the default user. It passed only because every identity shared one registry. Document the model in docs/dev/OAuthTokenStorage.md, including the orphaned per-user token table limitation and the options for addressing it.

Stop GoogleSheets pinning a credential that has no tokens: the per-identity data store and services are tagged with the access token they were built from, nothing is cached while an identity has none, and an entry is dropped once the stored token differs, so a user who queries before authorizing no longer gets an empty credential until the drillbit restarts. Percent-encode the user name in the token store path so that a name carrying a path separator, such as a full Kerberos principal, or a parent reference stays within its own path segment. Document that the per-identity registry and service caches are unbounded, and that user translation needs user authentication: with auth off the user name is neither unique nor trusted, so the combination is unsupported and SHARED_USER is the mode to use. Also drop a duplicated javadoc block in StorageResources and fix the indentation of two lines in CredentialResources.

cgivre self-assigned this Oct 22, 2026

cgivre added bug doc-impacting PRs that affect the documentation security labels Oct 23, 2026

cgivre requested a review from pjfanning October 23, 2026 18:22

pjfanning reviewed Oct 23, 2026

View reviewed changes

cgivre force-pushed the DRILL-8558 branch from fdb6e0d to fbbb53b Compare October 25, 2026 17:53

cgivre force-pushed the DRILL-8558 branch from 56dab32 to e7ff67c Compare October 25, 2026 18:04

Navigation Menu

Repository navigation

DRILL-8558: Fix credential leak and cross-user OAuth token reuse - #3094

DRILL-8558: Fix credential leak and cross-user OAuth token reuse#3094
cgivre wants to merge 2 commits into
apache:masterfrom
cgivre:DRILL-8558

cgivre commented Oct 22, 2026

Uh oh!

pjfanning Oct 23, 2026

Uh oh!

cgivre Oct 25, 2026

Uh oh!

pjfanning Oct 23, 2026

Uh oh!

cgivre Oct 25, 2026

Uh oh!

pjfanning Oct 23, 2026

Uh oh!

cgivre Oct 25, 2026

Uh oh!

pjfanning Oct 23, 2026

Uh oh!

cgivre Oct 25, 2026

Uh oh!

pjfanning Oct 23, 2026

Uh oh!

cgivre Oct 25, 2026

Uh oh!

pjfanning Oct 23, 2026

Uh oh!

cgivre Oct 25, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

cgivre commented Oct 22, 2026

DRILL-8558: Fix credential leak and cross-user OAuth token reuse

Description

1. /credentials.json disclosed every user's credentials

2. One user's queries could run with another user's OAuth tokens

Also included

Documentation

Testing

Reviewer notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

1. `/credentials.json` disclosed every user's credentials