From fbbb53b0aad2f82ce8b37e923d12546825fff215 Mon Sep 17 00:00:00 2001 From: Charles Givre Date: Thu, 22 Oct 2026 16:04:13 -0400 Subject: [PATCH 1/2] DRILL-8558: Fix credential leak and cross-user OAuth token reuse 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. --- .../store/googlesheets/DrillDataStore.java | 7 +- .../googlesheets/DrillDataStoreFactory.java | 17 +- .../GoogleSheetsStoragePlugin.java | 159 +++++++----- .../store/http/HttpAPIConnectionSchema.java | 2 +- .../exec/store/http/HttpSchemaFactory.java | 2 +- .../exec/store/http/HttpStoragePlugin.java | 60 +++-- .../exec/store/http/util/SimpleHttp.java | 4 + .../http/TestUserTranslationInHttpPlugin.java | 6 +- docs/dev/DevDocs.md | 5 + docs/dev/OAuthTokenStorage.md | 207 ++++++++++++++++ .../drill/exec/oauth/OAuthTokenProvider.java | 62 +++-- .../exec/oauth/PersistentTokenTable.java | 12 +- .../server/rest/CredentialPluginInfo.java | 127 ++++++++++ .../exec/server/rest/CredentialResources.java | 85 +++++-- .../drill/exec/server/rest/OAuthRequests.java | 105 ++++++-- .../exec/server/rest/StorageResources.java | 27 ++- .../exec/store/dfs/FileSystemPlugin.java | 49 ++-- .../store/dfs/FileSystemSchemaFactory.java | 2 +- .../exec/oauth/OAuthTokenProviderTest.java | 165 +++++++++++++ .../server/rest/CredentialPluginInfoTest.java | 227 ++++++++++++++++++ 20 files changed, 1154 insertions(+), 176 deletions(-) create mode 100644 docs/dev/OAuthTokenStorage.md create mode 100644 exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/CredentialPluginInfo.java create mode 100644 exec/java-exec/src/test/java/org/apache/drill/exec/oauth/OAuthTokenProviderTest.java create mode 100644 exec/java-exec/src/test/java/org/apache/drill/exec/server/rest/CredentialPluginInfoTest.java diff --git a/contrib/storage-googlesheets/src/main/java/org/apache/drill/exec/store/googlesheets/DrillDataStore.java b/contrib/storage-googlesheets/src/main/java/org/apache/drill/exec/store/googlesheets/DrillDataStore.java index 8dc184cf4c5..e49379bd339 100644 --- a/contrib/storage-googlesheets/src/main/java/org/apache/drill/exec/store/googlesheets/DrillDataStore.java +++ b/contrib/storage-googlesheets/src/main/java/org/apache/drill/exec/store/googlesheets/DrillDataStore.java @@ -41,7 +41,12 @@ public class DrillDataStore extends AbstractMemoryDataSt DrillDataStore(OAuthTokenProvider tokenProvider, String pluginName, String userID, DrillDataStoreFactory dataStoreFactory) { super(dataStoreFactory, userID); this.drillDataStoreFactory = dataStoreFactory; - TokenRegistry tokenRegistry = tokenProvider.getOauthTokenRegistry(userID); + // The registry identity comes from the factory, not from userID: userID is the + // Google-side data store id, which for a shared plugin is a placeholder name + // rather than a Drill user. Using it here would look up a per-user registry + // for a plugin whose tokens live in the shared one. + TokenRegistry tokenRegistry = + tokenProvider.getOauthTokenRegistry(dataStoreFactory.getRegistryIdentity()); this.tokenTable = tokenRegistry.getTokenTable(pluginName); if (hasValidTokens(tokenTable)) { keyValueMap.put(tokenTable.ACCESS_TOKEN_KEY, tokenTable.getAccessToken().getBytes(StandardCharsets.UTF_8)); diff --git a/contrib/storage-googlesheets/src/main/java/org/apache/drill/exec/store/googlesheets/DrillDataStoreFactory.java b/contrib/storage-googlesheets/src/main/java/org/apache/drill/exec/store/googlesheets/DrillDataStoreFactory.java index 92ff534db63..27d8f841d0d 100644 --- a/contrib/storage-googlesheets/src/main/java/org/apache/drill/exec/store/googlesheets/DrillDataStoreFactory.java +++ b/contrib/storage-googlesheets/src/main/java/org/apache/drill/exec/store/googlesheets/DrillDataStoreFactory.java @@ -28,10 +28,24 @@ public class DrillDataStoreFactory extends AbstractDataStoreFactory { private final OAuthTokenProvider tokenProvider; private final String pluginName; + private final String registryIdentity; - public DrillDataStoreFactory(OAuthTokenProvider tokenProvider, String pluginName) { + /** + * @param registryIdentity the identity whose Drill token registry backs these + * data stores: the query user for a {@code USER_TRANSLATION} plugin, or + * null for the shared registry. This is deliberately separate from the + * Google-side data store id passed to {@link #createDataStore(String)}, + * which must be a non-null string and is not a Drill identity. + */ + public DrillDataStoreFactory(OAuthTokenProvider tokenProvider, String pluginName, + String registryIdentity) { this.tokenProvider = tokenProvider; this.pluginName = pluginName; + this.registryIdentity = registryIdentity; + } + + public String getRegistryIdentity() { + return registryIdentity; } @Override @@ -44,6 +58,7 @@ public String toString() { return new PlanStringBuilder(this) .field("tokenProvider", tokenProvider) .field("pluginName", pluginName) + .field("registryIdentity", registryIdentity) .toString(); } } diff --git a/contrib/storage-googlesheets/src/main/java/org/apache/drill/exec/store/googlesheets/GoogleSheetsStoragePlugin.java b/contrib/storage-googlesheets/src/main/java/org/apache/drill/exec/store/googlesheets/GoogleSheetsStoragePlugin.java index 8e531917b3d..98b931648e3 100644 --- a/contrib/storage-googlesheets/src/main/java/org/apache/drill/exec/store/googlesheets/GoogleSheetsStoragePlugin.java +++ b/contrib/storage-googlesheets/src/main/java/org/apache/drill/exec/store/googlesheets/GoogleSheetsStoragePlugin.java @@ -52,7 +52,9 @@ import java.io.IOException; import java.security.GeneralSecurityException; import java.util.List; +import java.util.Map; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; public class GoogleSheetsStoragePlugin extends AbstractStoragePlugin { private final static Logger logger = LoggerFactory.getLogger(GoogleSheetsStoragePlugin.class); @@ -60,11 +62,19 @@ public class GoogleSheetsStoragePlugin extends AbstractStoragePlugin { private final GoogleSheetsStoragePluginConfig config; private final GoogleSheetsSchemaFactory schemaFactory; private final OAuthTokenProvider tokenProvider; - private DataStore dataStore; - private Sheets service; - private Drive driveService; - private TokenRegistry tokenRegistry; - private String username; + /** + * Google credential stores and services, cached per identity. + * + *

These were previously single fields on the plugin. A plugin instance is + * shared by every query, so the first caller's authenticated service was handed + * to every later caller regardless of which user asked for it, which under + * {@code USER_TRANSLATION} meant one user's queries reaching Google as another + * user. They are keyed by {@link #cacheKey(String)} so each identity gets, and + * keeps, only its own. + */ + private final Map> dataStores = new ConcurrentHashMap<>(); + private final Map sheetsServices = new ConcurrentHashMap<>(); + private final Map driveServices = new ConcurrentHashMap<>(); public GoogleSheetsStoragePlugin(GoogleSheetsStoragePluginConfig configuration, DrillbitContext context, String name) { @@ -95,22 +105,57 @@ public void initializeOauthTokenTable(SchemaConfig schemaConfig) { // the plugin is loaded, we read the tokens from the persistent store into a GS dataStore. // This happens when the plugin is registered. - if (config.getAuthMode() == AuthMode.USER_TRANSLATION) { - this.username = schemaConfig.getUserName(); - tokenRegistry = tokenProvider.getOauthTokenRegistry(this.username); - } else { - this.username = SHARED_USERNAME; - tokenRegistry = tokenProvider.getOauthTokenRegistry(null); - } + getTokenRegistry(schemaConfig == null ? null : schemaConfig.getUserName()); + } + + /** + * The token registry holding {@code queryUser}'s tokens, with this plugin's + * token table created in it if absent. Resolved per call; see + * {@link #dataStores} for why nothing here is cached on the plugin. + */ + public TokenRegistry getTokenRegistry(String queryUser) { + TokenRegistry tokenRegistry = tokenProvider.getOauthTokenRegistry(registryIdentity(queryUser)); tokenRegistry.createTokenTable(getName()); - this.dataStore = new DrillDataStoreFactory(tokenProvider, getName()).createDataStore(this.username); + return tokenRegistry; } - public DataStore getDataStore(String username) { - if (this.dataStore == null) { - this.dataStore = new DrillDataStoreFactory(tokenProvider, getName()).createDataStore(username); - } - return dataStore; + /** + * The identity whose Drill token registry holds this plugin's tokens: the query + * user under user translation, or null for the single shared registry. Note that + * {@link #SHARED_USERNAME} is only a Google-side placeholder and must not be used + * as a registry identity, or a shared plugin's tokens would be looked up in a + * per-user registry that the REST credential endpoints never write to. + */ + private String registryIdentity(String queryUser) { + return config.getAuthMode() == AuthMode.USER_TRANSLATION ? queryUser : null; + } + + /** + * The Google-side data store id, which must be a non-null string and is not a + * Drill identity. Under user translation it is the query user; otherwise a + * fixed placeholder. + */ + private String googleStoreId(String queryUser) { + return config.getAuthMode() == AuthMode.USER_TRANSLATION ? queryUser : SHARED_USERNAME; + } + + /** + * Cache key for the per-identity maps. Prefixed rather than using the user name + * directly, so that a user literally named {@link #SHARED_USERNAME} — which is + * what every web user is called when authentication is disabled — cannot share + * a cache entry with a shared-credential plugin. + */ + private String cacheKey(String queryUser) { + return config.getAuthMode() == AuthMode.USER_TRANSLATION ? "user:" + queryUser : "shared"; + } + + private DataStore buildDataStore(String queryUser) { + return new DrillDataStoreFactory(tokenProvider, getName(), registryIdentity(queryUser)) + .createDataStore(googleStoreId(queryUser)); + } + + public DataStore getDataStore(String queryUser) { + return dataStores.computeIfAbsent(cacheKey(queryUser), k -> buildDataStore(queryUser)); } @@ -120,8 +165,17 @@ public void registerSchemas(SchemaConfig schemaConfig, SchemaPlus parent) { schemaFactory.registerSchemas(schemaConfig, parent); } + /** + * This plugin's token table for the shared identity. Prefer + * {@link #getTokenTable(String)} wherever the query user is known. + */ public PersistentTokenTable getTokenTable() { - return tokenRegistry.getTokenTable(getName()); + return getTokenTable(null); + } + + /** This plugin's token table holding {@code queryUser}'s tokens. */ + public PersistentTokenTable getTokenTable(String queryUser) { + return getTokenRegistry(queryUser).getTokenTable(getName()); } @Override @@ -153,7 +207,7 @@ public AbstractGroupScan getPhysicalScan(String userName, JSONOptions selection) public AbstractGroupScan getPhysicalScan(String userName, JSONOptions selection, List columns, SessionOptionManager options, MetadataProviderManager metadataProviderManager) throws IOException { GoogleSheetsScanSpec scanSpec = selection.getListWith(context.getLpPersistence().getMapper(), new TypeReference() {}); - return new GoogleSheetsGroupScan(this.username, scanSpec, this, metadataProviderManager); + return new GoogleSheetsGroupScan(userName, scanSpec, this, metadataProviderManager); } @Override @@ -182,8 +236,7 @@ public StoragePluginConfig getConfig() { */ @VisibleForTesting public void initializeTokenTableForTesting() { - OAuthTokenProvider tokenProvider = context.getOauthTokenProvider(); - tokenRegistry = tokenProvider.getOauthTokenRegistry(null); + getTokenRegistry(null); } @Override @@ -201,26 +254,17 @@ public boolean supportsInsert() { * @return An authenticated {@link Sheets} Google Sheets service. */ public Sheets getSheetsService(String queryUser) { - if (service != null && dataStore != null) { - return service; - } else { - // Check if datastore is null and initialize if so. - if (dataStore == null) { - this.dataStore = getDataStore(queryUser); - } - - try { - if (config.getAuthMode() == AuthMode.USER_TRANSLATION) { - service = GoogleSheetsUtils.getSheetsService(config, dataStore, queryUser); - } else { - service = GoogleSheetsUtils.getSheetsService(config, dataStore, SHARED_USERNAME); - } - return service; - } catch (IOException | GeneralSecurityException e) { - throw UserException.connectionError(e) - .message("Error connecting to Googlesheets Service: " + e.getMessage()) - .build(logger); - } + return sheetsServices.computeIfAbsent(cacheKey(queryUser), k -> buildSheetsService(queryUser)); + } + + private Sheets buildSheetsService(String queryUser) { + try { + return GoogleSheetsUtils.getSheetsService( + config, getDataStore(queryUser), googleStoreId(queryUser)); + } catch (IOException | GeneralSecurityException e) { + throw UserException.connectionError(e) + .message("Error connecting to Googlesheets Service: " + e.getMessage()) + .build(logger); } } @@ -231,26 +275,17 @@ public Sheets getSheetsService(String queryUser) { * @return A validated and authenticated {@link Drive} instance. */ public Drive getDriveService(String queryUser) { - if (driveService != null && dataStore != null) { - return driveService; - } else { - // Check if datastore is null and initialize if so. - if (dataStore == null) { - this.dataStore = getDataStore(queryUser); - } - - try { - if (config.getAuthMode() == AuthMode.USER_TRANSLATION) { - driveService = GoogleSheetsUtils.getDriveService(config, dataStore, queryUser); - } else { - driveService = GoogleSheetsUtils.getDriveService(config, dataStore, SHARED_USERNAME); - } - return driveService; - } catch (IOException | GeneralSecurityException e) { - throw UserException.connectionError(e) - .message("Error connecting to Google Drive Service: " + e.getMessage()) - .build(logger); - } + return driveServices.computeIfAbsent(cacheKey(queryUser), k -> buildDriveService(queryUser)); + } + + private Drive buildDriveService(String queryUser) { + try { + return GoogleSheetsUtils.getDriveService( + config, getDataStore(queryUser), googleStoreId(queryUser)); + } catch (IOException | GeneralSecurityException e) { + throw UserException.connectionError(e) + .message("Error connecting to Google Drive Service: " + e.getMessage()) + .build(logger); } } } diff --git a/contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/HttpAPIConnectionSchema.java b/contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/HttpAPIConnectionSchema.java index 9263c0df3df..62e2bd9738b 100644 --- a/contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/HttpAPIConnectionSchema.java +++ b/contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/HttpAPIConnectionSchema.java @@ -68,7 +68,7 @@ public Table getTable(String tableName) { // Register a new table return registerTable(tableName, new DynamicDrillTable(plugin, plugin.getName(), new HttpScanSpec(plugin.getName(), name, tableName, - plugin.getConfig().copyForPlan(name), plugin.getTokenTable(), queryUserName, plugin.getRegistry()))); + plugin.getConfig().copyForPlan(name), plugin.getTokenTable(queryUserName), queryUserName, plugin.getRegistry()))); } } diff --git a/contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/HttpSchemaFactory.java b/contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/HttpSchemaFactory.java index ef6368d63cc..8289007c96e 100644 --- a/contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/HttpSchemaFactory.java +++ b/contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/HttpSchemaFactory.java @@ -106,7 +106,7 @@ public Table getTable(String name) { // Register a new table return registerTable(name, new DynamicDrillTable(plugin, plugin.getName(), new HttpScanSpec(plugin.getName(), name, null, - plugin.getConfig().copyForPlan(name), plugin.getTokenTable(), queryUserName, plugin.getRegistry()))); + plugin.getConfig().copyForPlan(name), plugin.getTokenTable(queryUserName), queryUserName, plugin.getRegistry()))); } else { return null; // Unknown table } diff --git a/contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/HttpStoragePlugin.java b/contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/HttpStoragePlugin.java index 7b65310d76c..e290f7b579b 100644 --- a/contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/HttpStoragePlugin.java +++ b/contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/HttpStoragePlugin.java @@ -35,7 +35,6 @@ import org.apache.drill.exec.store.SchemaConfig; import org.apache.drill.exec.store.StoragePluginRegistry; import org.apache.drill.exec.store.base.filter.FilterPushDownUtils; -import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableSet; import com.fasterxml.jackson.core.type.TypeReference; @@ -48,7 +47,6 @@ public class HttpStoragePlugin extends AbstractStoragePlugin { private final HttpStoragePluginConfig config; private final HttpSchemaFactory schemaFactory; private final StoragePluginRegistry registry; - private TokenRegistry tokenRegistry; public HttpStoragePlugin(HttpStoragePluginConfig configuration, DrillbitContext context, String name) { super(context, name); @@ -57,29 +55,16 @@ public HttpStoragePlugin(HttpStoragePluginConfig configuration, DrillbitContext this.schemaFactory = new HttpSchemaFactory(this); if (config.getAuthMode() != AuthMode.USER_TRANSLATION) { - initializeOauthTokenTable(null); + // Pre-create the shared token table so it exists before the first query. + getTokenRegistry(null); } } @Override public void registerSchemas(SchemaConfig schemaConfig, SchemaPlus parent) { - - // For user translation mode, this is moved here because we don't have the - // active username in the constructor. Removing it from the constructor makes - // it difficult to test, so we do the check and leave it in both places. - if (config.getAuthMode() == AuthMode.USER_TRANSLATION) { - initializeOauthTokenTable(schemaConfig.getUserName()); - } schemaFactory.registerSchemas(schemaConfig, parent); } - @VisibleForTesting - public void initializeOauthTokenTable(String username) { - OAuthTokenProvider tokenProvider = context.getOauthTokenProvider(); - tokenRegistry = tokenProvider.getOauthTokenRegistry(username); - tokenRegistry.createTokenTable(getName()); - } - @Override public HttpStoragePluginConfig getConfig() { return config; @@ -89,23 +74,48 @@ public StoragePluginRegistry getRegistry() { return registry; } + /** + * The token registry for the shared service identity. Only meaningful for a + * plugin that does not translate users; see {@link #getTokenRegistry(String)}. + */ public TokenRegistry getTokenRegistry() { - return tokenRegistry; + return getTokenRegistry(null); } /** - * This method returns the {@link TokenRegistry} for a given user. It is only used for testing user translation - * with OAuth 2.0. - * @param username A {@link String} of the current active user. - * @return A {@link TokenRegistry} for the given user. + * Returns the {@link TokenRegistry} holding {@code username}'s tokens, creating + * this plugin's token table in it if absent. + * + *

Resolved on every call rather than cached on the plugin. A plugin instance + * is shared by all queries, so a cached registry field would be re-pointed by + * whichever user planned a query most recently, and a concurrent query by + * another user could then read it and authenticate at the data source as the + * wrong user. + * + * @param username the user whose tokens are wanted; ignored unless this plugin + * uses {@code USER_TRANSLATION}, since every other auth mode + * shares one token table */ - @VisibleForTesting public TokenRegistry getTokenRegistry(String username) { - initializeOauthTokenTable(username); + String identity = config.getAuthMode() == AuthMode.USER_TRANSLATION ? username : null; + OAuthTokenProvider tokenProvider = context.getOauthTokenProvider(); + TokenRegistry tokenRegistry = tokenProvider.getOauthTokenRegistry(identity); + tokenRegistry.createTokenTable(getName()); return tokenRegistry; } - public PersistentTokenTable getTokenTable() { return tokenRegistry.getTokenTable(getName()); } + /** + * This plugin's token table for the shared service identity. Prefer + * {@link #getTokenTable(String)} on any path that knows the query user. + */ + public PersistentTokenTable getTokenTable() { + return getTokenTable(null); + } + + /** This plugin's token table holding {@code username}'s tokens. */ + public PersistentTokenTable getTokenTable(String username) { + return getTokenRegistry(username).getTokenTable(getName()); + } @Override public boolean supportsRead() { diff --git a/contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/util/SimpleHttp.java b/contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/util/SimpleHttp.java index 3171e1c55f3..9ce8a09e082 100644 --- a/contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/util/SimpleHttp.java +++ b/contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/util/SimpleHttp.java @@ -992,6 +992,10 @@ public static SimpleHttp apiCall( .tempDir(new File(context.getConfig().getString(ExecConstants.DRILL_TMP_DIR))) .url(HttpUrl.parse(finalUrl)) .proxyConfig(proxyConfig) + // The http_get() UDF is not given the query user, so it can only use the + // shared token table. A USER_TRANSLATION plugin's per-user tokens are + // therefore not reachable from this UDF; it fails rather than falling back + // to some other user's tokens. .tokenTable(plugin.getTokenTable()) .build(); } diff --git a/contrib/storage-http/src/test/java/org/apache/drill/exec/store/http/TestUserTranslationInHttpPlugin.java b/contrib/storage-http/src/test/java/org/apache/drill/exec/store/http/TestUserTranslationInHttpPlugin.java index 717302e81e4..8ee76916beb 100644 --- a/contrib/storage-http/src/test/java/org/apache/drill/exec/store/http/TestUserTranslationInHttpPlugin.java +++ b/contrib/storage-http/src/test/java/org/apache/drill/exec/store/http/TestUserTranslationInHttpPlugin.java @@ -235,8 +235,12 @@ public void testQueryWithOAuth() throws Exception { .setResponseCode(200) .setBody(TEST_JSON_RESPONSE_WITH_DATATYPES)); + // The query must run as TEST_USER_2, the user whose tokens were stored above. + // Using the default cluster client here would run as the anonymous user and + // pick up whatever tokens that identity holds, which is what user translation + // exists to prevent. String sql = "SELECT * FROM oauth.sharedEndpoint"; - RowSet results = queryBuilder().sql(sql).rowSet(); + RowSet results = client.queryBuilder().sql(sql).rowSet(); TupleMetadata expectedSchema = new SchemaBuilder() .add("col_1", MinorType.FLOAT8, DataMode.OPTIONAL) diff --git a/docs/dev/DevDocs.md b/docs/dev/DevDocs.md index eef8105b4d6..ae2d395bae9 100644 --- a/docs/dev/DevDocs.md +++ b/docs/dev/DevDocs.md @@ -1,5 +1,10 @@ # Drill Developer Docs +## Credentials and OAuth + +- [OAuthTokenStorage.md](OAuthTokenStorage.md) — **where plugin credentials live and who may change them**: shared vs `USER_TRANSLATION` auth modes, the per-identity OAuth token registry layout, the admin gate on shared tokens, how identity resolves when user authentication is disabled, and the orphaned-token-table limitation. +- [PluginCredentialsProvider.md](PluginCredentialsProvider.md) — credential provider implementations (plain, env, Vault, Hadoop). + ## Environment For information about configuring your development enviornment see [Environment.md](Environment.md). diff --git a/docs/dev/OAuthTokenStorage.md b/docs/dev/OAuthTokenStorage.md new file mode 100644 index 00000000000..7feea9c8d1a --- /dev/null +++ b/docs/dev/OAuthTokenStorage.md @@ -0,0 +1,207 @@ +# OAuth token storage and per-user credentials + +How Drill stores the credentials a storage plugin uses to authenticate at its +data source, who is allowed to change them over REST, and how both behave when +Drill's own user authentication is switched off. + +Companion doc: [`PluginCredentialsProvider.md`](PluginCredentialsProvider.md) +for the credential provider implementations. + +## Two kinds of credential, two places they live + +A `StoragePluginConfig` has an `AuthMode`: + +| Auth mode | Whose credentials | Where they live | +|---|---|---| +| `SHARED_USER` (default) | one service identity for the whole cluster | the plugin config's `credentialsProvider` | +| `USER_TRANSLATION` | each end user's own | per-user, outside the shared config | + +Under `USER_TRANSLATION` the storage differs again by credential type: + +- **Username/password** — in the config's credentials provider, under a per-user + map. `PlainCredentialsProvider` keeps `userCredentials: {user -> {username, + password}}`; `VaultCredentialsProvider` resolves a path containing `$user`. +- **OAuth tokens** — *not* in the plugin config. Access and refresh tokens live + in a `TokenRegistry` backed by the persistent store, one registry per + identity. + +That split matters: the shared `credentials` map on an OAuth plugin still holds +the admin-owned `clientID`, `clientSecret` and `tokenURI`, while the per-user +tokens obtained with them are held separately. + +## Token registry layout + +`OAuthTokenProvider.getOauthTokenRegistry(username)` returns the registry for +one identity, creating it on first use: + +``` +oauth_tokens <- shared registry (username null or empty) +oauth_tokens/ <- one registry per user translation user +``` + +Each registry is a separate persistent store, keyed inside by plugin name. + +**A registry must be resolved per call and never cached across identities.** +Returning a registry belonging to another user would let that user's queries run +with someone else's tokens at the data source, which is exactly what user +translation exists to prevent. `OAuthTokenProvider` therefore keeps a map of +registries keyed by identity rather than a single instance. + +The same rule applies on the plugin side. `HttpStoragePlugin`, +`FileSystemPlugin` and `GoogleSheetsStoragePlugin` each previously cached the +registry in a mutable field, re-pointed by whichever user planned a query most +recently. Because a plugin instance is shared by every query, that let a +concurrent query by another user read it and authenticate at the data source as +the wrong user. They now resolve per call: + +```java +plugin.getTokenTable(queryUserName) // not plugin.getTokenTable() +``` + +Call sites pass the user they already hold — `HttpSchemaFactory` and +`HttpAPIConnectionSchema` their `queryUserName`, `FileSystemSchemaFactory` its +`schemaConfig.getUserName()`. + +GoogleSheets additionally cached the authenticated `Sheets` and `Drive` services +and the Google `DataStore` in plugin fields, which was worse than a race: once +set, the first caller's service was returned to every later caller whoever asked. +Those are now keyed per identity. Its cache key is prefixed (`user:` or +`shared`) rather than being the bare user name, so that a user actually called +`anonymous` — which is every web user when authentication is disabled — cannot +share an entry with a shared-credential plugin. + +### Why not the aliases layout + +[`Aliases.md`](Aliases.md)' `PersistentAliasRegistry` solves the same per-user +problem the other way round: **one** store, keyed by user name inside it, with a +reserved `$public_aliases` key for the shared case. That makes +`store.getAll()` an enumeration of every user. + +OAuth tokens put the user in the store *path* instead, so there is no way to ask +"which users have tokens?" — see [Known limitation](#known-limitation-orphaned-token-tables) +below. The aliases layout is the better design; changing to it is a persistent +store format migration and has not been done. + +## Who may change what, over REST + +The token and credential endpoints live on two resource classes: + +| Resource | Role required | Purpose | +|---|---|---| +| `StorageResources` (`/storage/*`) | `ADMIN_ROLE` | full plugin configuration | +| `CredentialResources` (`/credentials/*`) | `AUTHENTICATED_ROLE` | a user managing their own credentials | + +`CredentialResources` is open to any authenticated user on purpose: supplying +your own data source credentials is not an admin action. Two consequences: + +1. **It must not serialize plugin configs.** A config carries every user's + stored credentials plus shared secrets, so `/credentials.json` returns + `CredentialPluginInfo` — name, enabled, OAuth flag, authorize URL, and the + caller's own credential state — never the config. +2. **Shared tokens need an in-method admin check.** The `/storage/*` OAuth + endpoints are deprecated in favour of the `/credentials/*` ones, so admins and + non-admins reach the same code. `OAuthRequests.getQueryUser` decides the + target table and refuses the write if the caller may not make it: + +| Plugin auth mode | Caller | Target | +|---|---|---| +| `USER_TRANSLATION` | anyone authenticated | that caller's own registry | +| anything else | admin | the shared registry | +| anything else | non-admin | **refused**, HTTP 403 | + +Overwriting shared tokens changes the identity *all* users' queries run under at +the data source, which is why it is admin-only. + +`updateAuthToken` resolves the target before exchanging the authorization code, +so a refused caller cannot make Drill spend the client secret on an outbound +request or consume a single-use code. + +## With user authentication disabled + +This is easy to get wrong, because the usual way of asking "who is calling?" +stops working. + +`WebServer` installs the Jetty security handler only when authentication is +enabled. With it off there is no security handler, so `SecurityContext` +carries **no principal at all**: `getUserPrincipal()` returns null and +`isUserInRole()` returns false. `@RolesAllowed` is not enforced either, because +`RolesAllowedDynamicFeature` is only registered when auth is on. + +Meanwhile queries still run as *somebody*: `DrillRestServer` builds the +anonymous session's user credentials from `AnonDrillUserPrincipal`, whose name is +`anonymous` and which reports `isAdminUser() == true` ("in anonymous mode all +users are admins"). + +So **`anonymous` is the identity, not "no identity"**. Resolve it by injecting +`DrillUserPrincipal`, which Drill binds to `AnonDrillUserPrincipalProvider` when +auth is off and `DrillUserPrincipalProvider` when it is on: + +```java +@Inject +DrillUserPrincipal principal; // never read identity from SecurityContext here +``` + +`UserResources.getCurrentUser` is the reference use. `CredentialResources`, +`StorageResources` and `OAuthRequests` all use it, which is what keeps a user +translation plugin's REST writes pointed at `oauth_tokens/anonymous` — the same +registry the query path reads — rather than at the shared registry. + +Getting this wrong is silent: before registries were resolved per identity, a +single cached registry served every lookup, so a write to the "wrong" registry +still read back correctly and the mismatch was invisible until the caching was +fixed. + +### Upgrade note + +An auth-off deployment with a `USER_TRANSLATION` OAuth plugin previously wrote +tokens to the shared `oauth_tokens` registry. Those tokens are now read from +`oauth_tokens/anonymous`, so the old ones are ignored and the user authorizes +once more. No data is lost; the stale entries can be removed by hand. + +### The shared identity must be spelled the same way everywhere + +`null` means "the shared registry". GoogleSheets also has a Google-side data +store id, which must be a non-null string and is a placeholder name +(`anonymous`) for a shared plugin. Passing that placeholder where a registry +identity is expected sends a shared plugin's lookups to +`oauth_tokens/anonymous`, which the REST credential endpoints never write to — +so tokens appear to vanish. `DrillDataStoreFactory` therefore carries an explicit +`registryIdentity` alongside the Google store id, and the two are not +interchangeable. + +This was invisible while one cached registry served every lookup, and only +surfaced once registries were resolved per identity. + +## Known limitation: orphaned token tables + +Deleting a storage plugin (`DELETE /storage/{name}.json`) deletes its token +table from the registries Drill can reach. Because the user is part of the store +*path*, there is no way to enumerate every user who ever held tokens for a +plugin, so per-user token tables belonging to users that this Drillbit has not +seen since it started cannot be discovered, and are left behind in the +persistent store. + +What that means in practice: + +- Tokens for users who have queried or authorized since the last restart *are* + removed, because those registries are live in `OAuthTokenProvider`. +- Tokens written before a restart, by a user who has not been active since, are + not reachable and remain until removed by hand. +- Nothing retroactively finds tables already orphaned by earlier versions. + +A complete fix needs one of: + +1. **Adopt the aliases layout** — one store keyed by user, making enumeration a + `store.getAll()`. Correct, and a store format migration. Note that existing + per-user stores still cannot be enumerated, so a migration cannot find them + either; users would re-authorize. +2. **Keep an index** of users holding token registries, maintained by + `OAuthTokenProvider`. Complete for anything written after the change, adds + persistent state. +3. **Add listing to `PersistentStoreProvider`** so a path's child stores can be + enumerated per backend (local filesystem, ZooKeeper). The most general, and it + touches an SPI shared by every Drill store. + +Until one is chosen, treat per-user OAuth token tables as data that outlives the +plugin, and remove them from the persistent store directly when +decommissioning a plugin that used user translation. diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/oauth/OAuthTokenProvider.java b/exec/java-exec/src/main/java/org/apache/drill/exec/oauth/OAuthTokenProvider.java index 544400f0021..e00e2e876d0 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/oauth/OAuthTokenProvider.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/oauth/OAuthTokenProvider.java @@ -21,44 +21,70 @@ import org.apache.drill.common.AutoCloseables; import org.apache.drill.exec.server.DrillbitContext; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; + /** * Class for managing oauth tokens. Storage plugins will have to manage obtaining the tokens, but * these classes handle the storage of access and refresh tokens. + * + *

Tokens are held in one registry per identity: a registry for each user of a + * {@code USER_TRANSLATION} plugin, plus one shared registry for plugins that + * authenticate as a single service account. Each maps to its own path in the + * persistent store, so a registry must be looked up by the identity whose tokens + * are wanted and never cached across identities. */ public class OAuthTokenProvider implements AutoCloseable { private static final String STORAGE_REGISTRY_PATH = "oauth_tokens"; + /** + * Map key standing in for the shared registry, which has no user. The empty + * string cannot collide with a user name because a blank name is treated as + * absent. + */ + private static final String SHARED_REGISTRY_KEY = ""; + private final DrillbitContext context; - private PersistentTokenRegistry oauthTokenRegistry; + private final Map oauthTokenRegistries = new ConcurrentHashMap<>(); public OAuthTokenProvider(DrillbitContext context) { this.context = context; } + /** + * Returns the token registry holding {@code username}'s tokens, creating it on + * first use. A null or empty user name selects the shared registry used by + * plugins that do not translate users. + * + *

The registry is resolved per call: returning a registry belonging to + * another user would let one user's queries run with another user's tokens, + * which is precisely what user translation exists to prevent. + */ public TokenRegistry getOauthTokenRegistry(String username) { - if (oauthTokenRegistry == null) { - initRemoteRegistries(username); - } - return oauthTokenRegistry; - } + String key = StringUtils.isNotEmpty(username) ? username : SHARED_REGISTRY_KEY; - private synchronized void initRemoteRegistries(String username) { - // Add the username to the path if present - String finalpath; - if (StringUtils.isNotEmpty(username)) { - finalpath = STORAGE_REGISTRY_PATH + "/" + username; - } else { - finalpath = STORAGE_REGISTRY_PATH; - } + // computeIfAbsent so that concurrent callers for the same identity share one + // registry, and so a failure to build one is not cached. + return oauthTokenRegistries.computeIfAbsent( + key, + k -> new PersistentTokenRegistry(context, registryPath(username)) + ); + } - if (oauthTokenRegistry == null) { - oauthTokenRegistry = new PersistentTokenRegistry(context, finalpath); - } + /** + * The persistent store path for a user's tokens, or the root path for the + * shared registry. + */ + private static String registryPath(String username) { + return StringUtils.isNotEmpty(username) + ? STORAGE_REGISTRY_PATH + "/" + username + : STORAGE_REGISTRY_PATH; } @Override public void close() throws Exception { - AutoCloseables.closeSilently(oauthTokenRegistry); + AutoCloseables.closeSilently(oauthTokenRegistries.values().toArray(new AutoCloseable[0])); + oauthTokenRegistries.clear(); } } diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/oauth/PersistentTokenTable.java b/exec/java-exec/src/main/java/org/apache/drill/exec/oauth/PersistentTokenTable.java index 7350fe26b7a..4d0fc18d98d 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/oauth/PersistentTokenTable.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/oauth/PersistentTokenTable.java @@ -96,7 +96,9 @@ public String getExpiresIn() { @Override @JsonIgnore public void setExpiresIn(String expiresIn) { - if (!tokens.containsKey(EXPIRES_IN_KEY) || !expiresIn.equals(getAccessToken())) { + // Only write if the value actually changed. Compare against the stored + // expiry, not the access token. + if (!tokens.containsKey(EXPIRES_IN_KEY) || !expiresIn.equals(getExpiresIn())) { put(EXPIRES_IN_KEY, expiresIn, true); } } @@ -113,9 +115,11 @@ public void setAccessToken(String token) { @Override @JsonIgnore public void setRefreshToken(String token) { - // Only update the access token if it is not the same as the previous token - if (!tokens.containsKey(REFRESH_TOKEN_KEY) || !getAccessToken().equals(token)) { - put(REFRESH_TOKEN_KEY, token,true); + // Only update the refresh token if it is not the same as the previous token. + // Compare against the stored refresh token, not the access token: the latter + // also threw NPE whenever a refresh token was stored without an access token. + if (!tokens.containsKey(REFRESH_TOKEN_KEY) || !token.equals(getRefreshToken())) { + put(REFRESH_TOKEN_KEY, token, true); } } diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/CredentialPluginInfo.java b/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/CredentialPluginInfo.java new file mode 100644 index 00000000000..12d2745dd98 --- /dev/null +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/CredentialPluginInfo.java @@ -0,0 +1,127 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.drill.exec.server.rest; + +import com.fasterxml.jackson.annotation.JsonProperty; +import jakarta.xml.bind.annotation.XmlRootElement; + +import org.apache.commons.lang3.StringUtils; +import org.apache.drill.common.logical.StoragePluginConfig; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Read-only, per-user view of a {@code USER_TRANSLATION} storage plugin, as + * returned by {@code GET /credentials.json}. + * + *

This endpoint is reachable by any authenticated user, not just admins, + * because supplying one's own data source credentials is a non-admin action. + * It therefore must not expose the plugin configuration: a + * {@link StoragePluginConfig} serializes its {@code credentialsProvider}, and + * for a {@code PlainCredentialsProvider} that includes every user's + * stored username and password plus any shared secrets such as an OAuth + * client secret. Only the fields the caller needs to act on their own + * credentials are carried here. + * + *

Admin-facing plugin configuration is served separately by + * {@link StorageResources}, which is gated on {@code ADMIN_ROLE} and uses + * {@link PluginConfigWrapper}. + */ +@XmlRootElement +public class CredentialPluginInfo { + private static final Logger logger = LoggerFactory.getLogger(CredentialPluginInfo.class); + + private final String name; + private final boolean enabled; + private final boolean oauth; + private final String authorizationUrl; + private final boolean hasCredentials; + private final String userName; + + public CredentialPluginInfo(String name, boolean enabled, boolean oauth, + String authorizationUrl, boolean hasCredentials, + String userName) { + this.name = name; + this.enabled = enabled; + this.oauth = oauth; + this.authorizationUrl = authorizationUrl; + this.hasCredentials = hasCredentials; + this.userName = userName; + } + + /** + * Builds the view of {@code config} that {@code queryUser} is allowed to see. + * + * @param name the storage config name + * @param config the stored plugin config, which is not serialized + * @param queryUser the authenticated caller, or null if not resolvable + * @return a config-free summary of the plugin for this caller + */ + public static CredentialPluginInfo from(String name, StoragePluginConfig config, String queryUser) { + PluginConfigWrapper wrapper = new PluginConfigWrapper(name, config); + boolean isOauth = wrapper.isOauth(); + + // Only built for OAuth plugins; the wrapper logs a warning otherwise. The URL + // carries the client ID and callback but no secret, and exists precisely so the + // browser can start the OAuth flow without reading the plugin config. + String authorizationUrl = isOauth ? wrapper.getAuthorizationURIWithParams() : ""; + + // For OAuth plugins the caller's per-user state is a token in the token + // registry rather than a username, so there is nothing to report here. + String ownUserName = null; + boolean credentialsPresent = false; + if (!isOauth && queryUser != null) { + try { + ownUserName = wrapper.getUserName(queryUser); + credentialsPresent = StringUtils.isNotEmpty(ownUserName); + } catch (RuntimeException e) { + // A credentials provider need not support per-user credentials, in which + // case it throws. That is not fatal: the plugin is still listed, just + // without an indication of whether this caller has credentials stored. + logger.warn("Could not read per-user credentials of {} for {}: {}", + name, queryUser, e.getMessage()); + } + } + + return new CredentialPluginInfo(name, config.isEnabled(), isOauth, + authorizationUrl, credentialsPresent, ownUserName); + } + + public String getName() { return name; } + + /** Whether the plugin is currently enabled in the storage registry. */ + public boolean isEnabled() { return enabled; } + + @JsonProperty("isOauth") + public boolean isOauth() { return oauth; } + + /** + * The provider authorize URL, pre-built with client ID, callback and scope, + * or an empty string for a non-OAuth plugin. + */ + public String getAuthorizationUrl() { return authorizationUrl; } + + /** Whether the calling user already has credentials stored for this plugin. */ + public boolean isHasCredentials() { return hasCredentials; } + + /** + * The calling user's own stored username for this plugin, or null. Never + * another user's, and never a password. + */ + public String getUserName() { return userName; } +} diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/CredentialResources.java b/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/CredentialResources.java index 4074e5e662f..2e46e766d2a 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/CredentialResources.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/CredentialResources.java @@ -25,6 +25,7 @@ import org.apache.drill.common.logical.security.CredentialsProvider; import org.apache.drill.exec.server.rest.DrillRestServer.UserAuthEnabled; import org.apache.drill.exec.server.rest.StorageResources.StoragePluginModel; +import org.apache.drill.exec.server.rest.auth.DrillUserPrincipal; import org.apache.drill.exec.store.StoragePluginRegistry; import org.apache.drill.exec.store.StoragePluginRegistry.PluginException; import org.apache.drill.exec.store.StoragePluginRegistry.PluginFilter; @@ -62,7 +63,9 @@ @RolesAllowed(AUTHENTICATED_ROLE) public class CredentialResources { private static final Logger logger = LoggerFactory.getLogger(CredentialResources.class); - private static final Comparator PLUGIN_COMPARATOR = + private static final Comparator PLUGIN_COMPARATOR = + Comparator.comparing(CredentialPluginInfo::getName); + private static final Comparator CONFIG_COMPARATOR = Comparator.comparing(PluginConfigWrapper::getName); private static final String ALL_PLUGINS = "all"; private static final String ENABLED_PLUGINS = "enabled"; @@ -78,6 +81,14 @@ public class CredentialResources { @Inject SecurityContext sc; + /** + * The caller. Injected rather than read from the {@code SecurityContext} so it + * also resolves with authentication disabled, where Drill binds the anonymous + * principal but the security context has none. + */ + @Inject + DrillUserPrincipal principal; + @Inject HttpServletRequest request; @@ -86,7 +97,7 @@ public class CredentialResources { @Produces(MediaType.TEXT_HTML) @Operation(externalDocs = @ExternalDocumentation(description = "Apache Drill REST API documentation:", url = "https://drill.apache.org/docs/rest-api-introduction/")) public Viewable getPlugins() { - List model = getPluginsJSON().stream() + List model = getUserTranslationConfigs().stream() .map(plugin -> new StoragePluginModel(plugin, request, sc)) .collect(Collectors.toList()); // Creating an empty model with CSRF token, if there are no storage plugins @@ -99,15 +110,31 @@ public Viewable getPlugins() { @GET @Path("/credentials.json") @Produces(MediaType.APPLICATION_JSON) - public List getPluginsJSON() { + public List getPluginsJSON() { return getConfigsFor(TRANSLATES_USERS); } + /** + * The stored configs of every user translation plugin, for server-side + * rendering only. These must never be serialized to a client: a config carries + * every user's stored credentials along with shared secrets such as an OAuth + * client secret, and this resource is open to any authenticated user. + */ + private List getUserTranslationConfigs() { + return StreamSupport.stream( + Spliterators.spliteratorUnknownSize( + storage.storedConfigs(PluginFilter.TRANSLATES_USERS).entrySet().iterator(), + Spliterator.ORDERED), false) + .map(entry -> new PluginConfigWrapper(entry.getKey(), entry.getValue())) + .sorted(CONFIG_COMPARATOR) + .collect(Collectors.toList()); + } + @GET @Path("/credentials{group: (/[^/]+?)*}-plugins.json") @Produces(MediaType.APPLICATION_JSON) @Operation(externalDocs = @ExternalDocumentation(description = "Apache Drill REST API documentation:", url = "https://drill.apache.org/docs/rest-api-introduction/")) - public List getConfigsFor(@PathParam("group") String pluginGroup) { + public List getConfigsFor(@PathParam("group") String pluginGroup) { PluginFilter filter; switch (pluginGroup.trim()) { case ALL_PLUGINS: @@ -125,17 +152,27 @@ public List getConfigsFor(@PathParam("group") String plugin default: return Collections.emptyList(); } - List results = StreamSupport.stream( + // Any authenticated user may reach this resource, so the plugin configs + // themselves must not be serialized: they carry every user's stored + // credentials and any shared secrets. Map each one down to the fields this + // caller needs to manage their own credentials. Admins read full configs + // from StorageResources instead, which is gated on ADMIN_ROLE. + String queryUser = queryUser(); + return StreamSupport.stream( Spliterators.spliteratorUnknownSize(storage.storedConfigs(filter).entrySet().iterator(), Spliterator.ORDERED), false) - .map(entry -> new PluginConfigWrapper(entry.getKey(), entry.getValue())) + .map(entry -> CredentialPluginInfo.from(entry.getKey(), entry.getValue(), queryUser)) .sorted(PLUGIN_COMPARATOR) .collect(Collectors.toList()); + } - if (results.isEmpty()) { - return Collections.emptyList(); - } else { - return results; - } + /** + * The caller, used to scope credential reads and writes to their own entry. + * With authentication disabled this is the anonymous user, which is also the + * user queries run as, so per-user credentials stay readable by the query path. + * Null only with authentication on and no authenticated principal. + */ + private String queryUser() { + return principal == null ? null : principal.getName(); } @POST @@ -146,7 +183,14 @@ public List getConfigsFor(@PathParam("group") String plugin public Response createOrUpdateCredentials(@FormParam("plugin") String pluginName, @FormParam("username") String username, @FormParam("password") String password) { - String queryUser = sc.getUserPrincipal().getName(); + String queryUser = queryUser(); + if (queryUser == null) { + // Credentials are always stored against a user, so refuse rather than + // write an entry under a null key that nobody could ever read back. + return Response.status(Status.UNAUTHORIZED) + .entity(message("Could not determine the current user.")) + .build(); + } pluginName = pluginName.trim(); if (pluginName.isEmpty()) { return Response.status(Response.Status.BAD_REQUEST) @@ -192,7 +236,14 @@ public Response createOrUpdateCredentials(@FormParam("plugin") String pluginName @Produces(MediaType.APPLICATION_JSON) @Operation(externalDocs = @ExternalDocumentation(description = "Apache Drill REST API documentation:", url = "https://drill.apache.org/docs/rest-api-introduction/")) public Response createOrUpdatePlugin(@PathParam("pluginName") String pluginName, UsernamePasswordContainer credentials) { - String queryUser = sc.getUserPrincipal().getName(); + String queryUser = queryUser(); + if (queryUser == null) { + // Credentials are always stored against a user, so refuse rather than + // write an entry under a null key that nobody could ever read back. + return Response.status(Status.UNAUTHORIZED) + .entity(message("Could not determine the current user.")) + .build(); + } String cleanPluginName; if (pluginName.isEmpty()) { return Response.status(Response.Status.BAD_REQUEST) @@ -238,7 +289,7 @@ public Response createOrUpdatePlugin(@PathParam("pluginName") String pluginName, @Produces(MediaType.APPLICATION_JSON) @Operation(externalDocs = @ExternalDocumentation(description = "Apache Drill REST API documentation:", url = "https://drill.apache.org/docs/rest-api-introduction/")) public Response updateRefreshToken(@PathParam("name") String name, OAuthTokenContainer tokens) { - return OAuthRequests.updateRefreshToken(name, tokens, storage, authEnabled, sc); + return OAuthRequests.updateRefreshToken(name, tokens, storage, principal); } @POST @@ -247,7 +298,7 @@ public Response updateRefreshToken(@PathParam("name") String name, OAuthTokenCon @Produces(MediaType.APPLICATION_JSON) @Operation(externalDocs = @ExternalDocumentation(description = "Apache Drill REST API documentation:", url = "https://drill.apache.org/docs/rest-api-introduction/")) public Response updateAccessToken(@PathParam("name") String name, OAuthTokenContainer tokens) { - return OAuthRequests.updateAccessToken(name, tokens, storage, authEnabled, sc); + return OAuthRequests.updateAccessToken(name, tokens, storage, principal); } @POST @@ -257,7 +308,7 @@ public Response updateAccessToken(@PathParam("name") String name, OAuthTokenCont @Operation(externalDocs = @ExternalDocumentation(description = "Apache Drill REST API documentation:", url = "https://drill.apache.org/docs/rest-api-introduction/")) public Response updateOAuthTokens(@PathParam("name") String name, OAuthTokenContainer tokenContainer) { - return OAuthRequests.updateOAuthTokens(name, tokenContainer, storage, authEnabled, sc); + return OAuthRequests.updateOAuthTokens(name, tokenContainer, storage, principal); } @GET @@ -265,7 +316,7 @@ public Response updateOAuthTokens(@PathParam("name") String name, @Produces(MediaType.TEXT_HTML) @Operation(externalDocs = @ExternalDocumentation(description = "Apache Drill REST API documentation:", url = "https://drill.apache.org/docs/rest-api-introduction/")) public Response updateAuthToken(@PathParam("name") String name, @QueryParam("code") String code) { - return OAuthRequests.updateAuthToken(name, code, request, storage, authEnabled, sc); + return OAuthRequests.updateAuthToken(name, code, request, storage, principal); } private JsonResult message(String message, Object... args) { diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/OAuthRequests.java b/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/OAuthRequests.java index 7de30469480..75178fc9791 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/OAuthRequests.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/OAuthRequests.java @@ -28,8 +28,8 @@ import org.apache.drill.exec.oauth.PersistentTokenTable; import org.apache.drill.exec.oauth.TokenRegistry; import org.apache.drill.exec.server.DrillbitContext; -import org.apache.drill.exec.server.rest.DrillRestServer.UserAuthEnabled; import org.apache.drill.exec.server.rest.StorageResources.JsonResult; +import org.apache.drill.exec.server.rest.auth.DrillUserPrincipal; import org.apache.drill.exec.store.AbstractStoragePlugin; import org.apache.drill.exec.store.StoragePluginRegistry; import org.apache.drill.exec.store.StoragePluginRegistry.PluginException; @@ -41,7 +41,6 @@ import jakarta.servlet.http.HttpServletRequest; import jakarta.ws.rs.core.Response; import jakarta.ws.rs.core.Response.Status; -import jakarta.ws.rs.core.SecurityContext; import java.io.BufferedReader; import java.io.IOException; import java.io.InputStream; @@ -58,12 +57,11 @@ public class OAuthRequests { public static Response updateAccessToken(String name, OAuthTokenContainer tokens, StoragePluginRegistry storage, - UserAuthEnabled authEnabled, - SecurityContext sc) { + DrillUserPrincipal principal) { try { DrillbitContext context = ((AbstractStoragePlugin) storage.getPlugin(name)).getContext(); OAuthTokenProvider tokenProvider = context.getOauthTokenProvider(); - PersistentTokenTable tokenTable = tokenProvider.getOauthTokenRegistry(getQueryUser(storage.getPlugin(name).getConfig(), authEnabled, sc)).getTokenTable(name); + PersistentTokenTable tokenTable = tokenProvider.getOauthTokenRegistry(getQueryUser(storage.getPlugin(name).getConfig(), principal)).getTokenTable(name); // Set the access token tokenTable.setAccessToken(tokens.getAccessToken()); @@ -71,6 +69,8 @@ public static Response updateAccessToken(String name, return Response.status(Status.OK) .entity("Access tokens have been updated.") .build(); + } catch (TokenAccessDeniedException e) { + return forbidden(name, e); } catch (PluginException e) { logger.error("Error when adding tokens to {}", name); return Response.status(Status.INTERNAL_SERVER_ERROR) @@ -80,13 +80,13 @@ public static Response updateAccessToken(String name, } public static Response updateRefreshToken(String name, OAuthTokenContainer tokens, - StoragePluginRegistry storage, UserAuthEnabled authEnabled, - SecurityContext sc) { + StoragePluginRegistry storage, + DrillUserPrincipal principal) { try { DrillbitContext context = ((AbstractStoragePlugin) storage.getPlugin(name)).getContext(); OAuthTokenProvider tokenProvider = context.getOauthTokenProvider(); PersistentTokenTable tokenTable = tokenProvider.getOauthTokenRegistry( - getQueryUser(storage.getPlugin(name).getConfig(), authEnabled, sc)).getTokenTable(name); + getQueryUser(storage.getPlugin(name).getConfig(), principal)).getTokenTable(name); // Set the access token tokenTable.setRefreshToken(tokens.getRefreshToken()); @@ -94,6 +94,8 @@ public static Response updateRefreshToken(String name, OAuthTokenContainer token return Response.status(Status.OK) .entity("Refresh token have been updated.") .build(); + } catch (TokenAccessDeniedException e) { + return forbidden(name, e); } catch (PluginException e) { logger.error("Error when adding tokens to {}", name); return Response.status(Status.INTERNAL_SERVER_ERROR) @@ -102,13 +104,14 @@ public static Response updateRefreshToken(String name, OAuthTokenContainer token } } - public static Response updateOAuthTokens(String name, OAuthTokenContainer tokenContainer, StoragePluginRegistry storage, - UserAuthEnabled authEnabled, SecurityContext sc) { + public static Response updateOAuthTokens(String name, OAuthTokenContainer tokenContainer, + StoragePluginRegistry storage, + DrillUserPrincipal principal) { try { DrillbitContext context = ((AbstractStoragePlugin) storage.getPlugin(name)).getContext(); OAuthTokenProvider tokenProvider = context.getOauthTokenProvider(); PersistentTokenTable tokenTable = tokenProvider - .getOauthTokenRegistry(getQueryUser(storage.getPlugin(name).getConfig(), authEnabled, sc)) + .getOauthTokenRegistry(getQueryUser(storage.getPlugin(name).getConfig(), principal)) .getTokenTable(name); // Set the access and refresh token @@ -119,6 +122,8 @@ public static Response updateOAuthTokens(String name, OAuthTokenContainer tokenC return Response.status(Status.OK) .entity("Access tokens have been updated.") .build(); + } catch (TokenAccessDeniedException e) { + return forbidden(name, e); } catch (PluginException e) { logger.error("Error when adding tokens to {}", name); return Response.status(Status.INTERNAL_SERVER_ERROR) @@ -128,9 +133,14 @@ public static Response updateOAuthTokens(String name, OAuthTokenContainer tokenC } public static Response updateAuthToken(String name, String code, HttpServletRequest request, - StoragePluginRegistry storage, UserAuthEnabled authEnabled, - SecurityContext sc) { + StoragePluginRegistry storage, + DrillUserPrincipal principal) { try { + // Establish who may store these tokens before exchanging the code. Doing + // this first means a refused caller cannot make Drill spend the client + // secret on an outbound request, nor consume a single-use auth code. + String queryUser = getQueryUser(storage.getPlugin(name).getConfig(), principal); + CredentialsProvider credentialsProvider = storage.getPlugin(name).getConfig().getCredentialsProvider(); String callbackURL = request.getRequestURL().toString(); @@ -146,7 +156,7 @@ public static Response updateAuthToken(String name, String code, HttpServletRequ TokenRegistry tokenRegistry = ((AbstractStoragePlugin) storage.getPlugin(name)) .getContext() .getOauthTokenProvider() - .getOauthTokenRegistry(getQueryUser(storage.getPlugin(name).getConfig(), authEnabled, sc)); + .getOauthTokenRegistry(queryUser); // Add a token registry table if none exists tokenRegistry.createTokenTable(name); @@ -176,6 +186,8 @@ public static Response updateAuthToken(String name, String code, HttpServletRequ } return Response.status(Status.OK).entity(successPage).build(); + } catch (TokenAccessDeniedException e) { + return forbidden(name, e); } catch (PluginException e) { logger.error("Error when adding auth token to {}", name); return Response.status(Status.INTERNAL_SERVER_ERROR) @@ -189,18 +201,63 @@ private static JsonResult message(String message, Object... args) { } /** - * This function checks to see if a given storage plugin is using USER_TRANSLATION mode and if user - * authentication is enabled. If so, it will return the active user name. If not it will return null. + * Resolves which token table the caller may write to, and refuses the write if + * they may not. + * + *

A {@code USER_TRANSLATION} plugin gives each user their own token table, + * so the caller gets their own name and can only ever touch their own tokens. + * Every other auth mode shares one token table across the whole cluster, and + * those tokens are admin-owned configuration: overwriting them changes the + * identity that all users' queries run under at the data source. The + * credential endpoints that reach this code are open to any authenticated + * user, so the shared table has to be gated on the admin role here rather than + * left to the resource class. + * + *

{@link DrillUserPrincipal} is used rather than the {@code SecurityContext} + * because it is bound to the anonymous admin principal when authentication is + * disabled, where the security context carries no principal at all. + * * @param config {@link StoragePluginConfig} The current plugin configuration - * @return If USER_TRANSLATION is enabled, returns the active user. If not, returns null. + * @param principal the caller, never null in practice + * @return the caller's user name for a user translation plugin, or null to + * select the shared token table + * @throws TokenAccessDeniedException if the caller may not write these tokens */ - private static String getQueryUser(StoragePluginConfig config, - UserAuthEnabled authEnabled, - SecurityContext sc) { - if (config.getAuthMode() == AuthMode.USER_TRANSLATION && authEnabled.get()) { - return sc.getUserPrincipal().getName(); - } else { - return null; + private static String getQueryUser(StoragePluginConfig config, DrillUserPrincipal principal) { + if (principal == null) { + // Only reachable with authentication on but no authenticated principal; + // with it off the anonymous principal is always supplied. + throw new TokenAccessDeniedException("Could not determine the current user."); + } + + if (config.getAuthMode() == AuthMode.USER_TRANSLATION) { + // 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(); } + + if (!principal.isAdminUser()) { + throw new TokenAccessDeniedException( + "Only an administrator may change the shared OAuth tokens of a storage plugin."); + } + return null; + } + + /** + * Thrown when the caller is not permitted to write the token table they asked + * for. Carries the message sent back to the client. + */ + private static class TokenAccessDeniedException extends RuntimeException { + TokenAccessDeniedException(String message) { + super(message); + } + } + + private static Response forbidden(String name, TokenAccessDeniedException e) { + logger.warn("Refused an OAuth token update for {}: {}", name, e.getMessage()); + return Response.status(Status.FORBIDDEN) + .entity(message("%s", e.getMessage())) + .build(); } } diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/StorageResources.java b/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/StorageResources.java index 6a5642c698d..48652a43306 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/StorageResources.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/StorageResources.java @@ -50,6 +50,7 @@ import org.apache.drill.common.logical.StoragePluginConfig.AuthMode; import org.apache.drill.exec.oauth.TokenRegistry; import org.apache.drill.exec.server.rest.DrillRestServer.UserAuthEnabled; +import org.apache.drill.exec.server.rest.auth.DrillUserPrincipal; import org.apache.drill.exec.store.StoragePluginRegistry; import org.apache.drill.exec.store.StoragePluginRegistry.PluginEncodingException; import org.apache.drill.exec.store.StoragePluginRegistry.PluginException; @@ -82,6 +83,14 @@ public class StorageResources { @Inject SecurityContext sc; + /** + * The caller. Injected rather than read from the {@code SecurityContext} so it + * also resolves with authentication disabled, where Drill binds the anonymous + * principal but the security context has none. + */ + @Inject + DrillUserPrincipal principal; + @Inject HttpServletRequest request; @@ -201,7 +210,7 @@ public Response enablePlugin(@PathParam("name") String name, @PathParam("val") B public Response updateRefreshToken(@PathParam("name") String name, OAuthTokenContainer tokens) { // This endpoint is deprecated. Use the same path in credentials resources instead. logger.warn("Deprecated endpoint call: {}", "/storage/" + name + "/update_refresh_token."); - return OAuthRequests.updateRefreshToken(name, tokens, storage, authEnabled, sc); + return OAuthRequests.updateRefreshToken(name, tokens, storage, principal); } @POST @@ -213,7 +222,7 @@ public Response updateAccessToken(@PathParam("name") String name, OAuthTokenCont // This endpoint is deprecated. Use the same path in credentials resources instead. logger.warn("Deprecated endpoint call: {}", "/storage/" + name + "/update_access_token."); - return OAuthRequests.updateAccessToken(name, tokens, storage, authEnabled, sc); + return OAuthRequests.updateAccessToken(name, tokens, storage, principal); } @POST @@ -225,7 +234,7 @@ public Response updateOAuthTokens(@PathParam("name") String name, OAuthTokenContainer tokenContainer) { // This endpoint is deprecated. Use the same path in credentials resources instead. logger.warn("Deprecated endpoint call: {}", "/storage/" + name + "/update_oauth_tokens."); - return OAuthRequests.updateOAuthTokens(name, tokenContainer, storage, authEnabled, sc); + return OAuthRequests.updateOAuthTokens(name, tokenContainer, storage, principal); } @GET @@ -235,7 +244,7 @@ public Response updateOAuthTokens(@PathParam("name") String name, public Response updateAuthToken(@PathParam("name") String name, @QueryParam("code") String code) { // This endpoint is deprecated. Use the same path in credentials resources instead. logger.warn("Deprecated endpoint call: {}", "/storage/" + name + "/update_oauth2_authtoken."); - return OAuthRequests.updateAuthToken(name, code, request, storage, authEnabled, sc); + return OAuthRequests.updateAuthToken(name, code, request, storage, principal); } /** @@ -422,9 +431,15 @@ public Response deletePluginViaGet(@PathParam("name") String name) { * @param config {@link StoragePluginConfig} The current plugin configuration * @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 + * translation plugin, or null for the single shared table. The name must match + * the one queries run under, which with authentication disabled is the + * anonymous user rather than no user at all. + */ private String getQueryUser(StoragePluginConfig config) { - if (config.getAuthMode() == AuthMode.USER_TRANSLATION && authEnabled.get()) { - return sc.getUserPrincipal().getName(); + if (config.getAuthMode() == AuthMode.USER_TRANSLATION && principal != null) { + return principal.getName(); } else { return null; } diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSystemPlugin.java b/exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSystemPlugin.java index 5665ae72a4a..bcf1bfb7c4f 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSystemPlugin.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSystemPlugin.java @@ -92,7 +92,6 @@ public class FileSystemPlugin extends AbstractStoragePlugin { private final Map formatPluginsByConfig; private final FileSystemConfig config; private final Configuration fsConf; - private TokenRegistry tokenRegistry; private final boolean mountCommandsEnabled; public FileSystemPlugin(FileSystemConfig config, DrillbitContext context, String name) throws ExecutionSetupException { @@ -232,28 +231,51 @@ private void handleSFTPCredentials(CredentialsProvider credentialsProvider, Stri @VisibleForTesting public void initializeOauthTokenTable(String username) { - OAuthTokenProvider tokenProvider = context.getOauthTokenProvider(); - tokenRegistry = tokenProvider.getOauthTokenRegistry(username); - tokenRegistry.createTokenTable(getName()); + getTokenRegistry(username); } + /** + * The token registry for the shared service identity. Only meaningful for a + * plugin that does not translate users; see {@link #getTokenRegistry(String)}. + */ public TokenRegistry getTokenRegistry() { - return tokenRegistry; + return getTokenRegistry(null); } /** - * This method returns the {@link TokenRegistry} for a given user. It is only used for testing user translation - * with OAuth 2.0. - * @param username A {@link String} of the current active user. - * @return A {@link TokenRegistry} for the given user. + * Returns the {@link TokenRegistry} holding {@code username}'s tokens, creating + * this plugin's token table in it if absent. + * + *

Resolved on every call rather than cached on the plugin. A plugin instance + * is shared by all queries, so a cached registry field would be re-pointed by + * whichever user planned a query most recently, and a concurrent query by + * another user could then read it and authenticate at the data source as the + * wrong user. + * + * @param username the user whose tokens are wanted; ignored unless this plugin + * uses {@code USER_TRANSLATION}, since every other auth mode + * shares one token table */ - @VisibleForTesting public TokenRegistry getTokenRegistry(String username) { - initializeOauthTokenTable(username); + String identity = config.getAuthMode() == AuthMode.USER_TRANSLATION ? username : null; + OAuthTokenProvider tokenProvider = context.getOauthTokenProvider(); + TokenRegistry tokenRegistry = tokenProvider.getOauthTokenRegistry(identity); + tokenRegistry.createTokenTable(getName()); return tokenRegistry; } - public PersistentTokenTable getTokenTable() { return tokenRegistry.getTokenTable(getName()); } + /** + * This plugin's token table for the shared service identity. Prefer + * {@link #getTokenTable(String)} on any path that knows the query user. + */ + public PersistentTokenTable getTokenTable() { + return getTokenTable(null); + } + + /** This plugin's token table holding {@code username}'s tokens. */ + public PersistentTokenTable getTokenTable(String username) { + return getTokenRegistry(username).getTokenTable(getName()); + } /** * Creates a new FormatCreator instance. @@ -372,8 +394,7 @@ public Configuration getFsConf() { */ @VisibleForTesting public void initializeTokenTableForTesting() { - OAuthTokenProvider tokenProvider = context.getOauthTokenProvider(); - tokenRegistry = tokenProvider.getOauthTokenRegistry(null); + getTokenRegistry(null); } /** diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSystemSchemaFactory.java b/exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSystemSchemaFactory.java index f7b35aa3b51..5844474ff9e 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSystemSchemaFactory.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSystemSchemaFactory.java @@ -88,7 +88,7 @@ public FileSystemSchema(String name, SchemaConfig schemaConfig) throws IOExcepti if (oAuthConfig != null) { OAuthEnabledFileSystem underlyingFileSystem = (OAuthEnabledFileSystem) fs.getUnderlyingFs(); underlyingFileSystem.setPluginConfig(plugin.getConfig()); - underlyingFileSystem.setTokenTable(plugin.getTokenTable()); + underlyingFileSystem.setTokenTable(plugin.getTokenTable(schemaConfig.getUserName())); underlyingFileSystem.setoAuthConfig(plugin.getConfig().oAuthConfig()); } diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/oauth/OAuthTokenProviderTest.java b/exec/java-exec/src/test/java/org/apache/drill/exec/oauth/OAuthTokenProviderTest.java new file mode 100644 index 00000000000..9a934b86edc --- /dev/null +++ b/exec/java-exec/src/test/java/org/apache/drill/exec/oauth/OAuthTokenProviderTest.java @@ -0,0 +1,165 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.drill.exec.oauth; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotSame; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertSame; + +import org.apache.drill.categories.SecurityTest; +import org.apache.drill.exec.server.rest.auth.DrillUserPrincipal; +import org.apache.drill.test.ClusterFixture; +import org.apache.drill.test.ClusterFixtureBuilder; +import org.apache.drill.test.ClusterTest; +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +/** + * Under {@code USER_TRANSLATION} each user's OAuth tokens live in their own + * registry, keyed by user name in the persistent store. These tests pin down that + * the provider resolves a registry by the identity asked for, because handing back + * a registry belonging to a different user would make that user's queries run with + * someone else's tokens at the data source. + */ +@Category(SecurityTest.class) +public class OAuthTokenProviderTest extends ClusterTest { + + /** + * Token tables are held in the cluster's persistent store and outlive each test + * method, and test order is not fixed. Each test therefore uses its own plugin + * name so that one test's writes cannot be observed by another. + */ + private static final String LEAK_PLUGIN = "leak_test_plugin"; + private static final String OVERWRITE_PLUGIN = "overwrite_test_plugin"; + + @BeforeClass + public static void setUp() throws Exception { + ClusterFixtureBuilder builder = ClusterFixture.builder(dirTestWatcher); + startCluster(builder); + } + + private static OAuthTokenProvider provider() { + return cluster.drillbit().getContext().getOauthTokenProvider(); + } + + @Test + public void testEachUserGetsTheirOwnRegistry() { + OAuthTokenProvider provider = provider(); + + TokenRegistry alice = provider.getOauthTokenRegistry("alice"); + TokenRegistry bob = provider.getOauthTokenRegistry("bob"); + + assertNotSame("each user must get their own token registry", alice, bob); + } + + @Test + public void testUserRegistryIsNotTheSharedRegistry() { + OAuthTokenProvider provider = provider(); + + TokenRegistry shared = provider.getOauthTokenRegistry(null); + TokenRegistry carol = provider.getOauthTokenRegistry("carol"); + + assertNotSame("a user's registry must not be the shared registry", shared, carol); + } + + @Test + public void testRegistryIsStableForTheSameUser() { + OAuthTokenProvider provider = provider(); + + assertSame(provider.getOauthTokenRegistry("dave"), provider.getOauthTokenRegistry("dave")); + assertSame(provider.getOauthTokenRegistry(null), provider.getOauthTokenRegistry(null)); + } + + /** A blank user name means "no user", i.e. the shared registry. */ + @Test + public void testBlankUserSelectsTheSharedRegistry() { + OAuthTokenProvider provider = provider(); + + assertSame(provider.getOauthTokenRegistry(null), provider.getOauthTokenRegistry("")); + } + + /** + * The behaviour that matters: a token stored for one user must not be readable + * as another user, nor through the shared registry. + */ + @Test + public void testTokensDoNotLeakBetweenUsers() { + OAuthTokenProvider provider = provider(); + + TokenRegistry erinRegistry = provider.getOauthTokenRegistry("erin"); + erinRegistry.createTokenTable(LEAK_PLUGIN); + erinRegistry.getTokenTable(LEAK_PLUGIN).setAccessToken("erin-access-token"); + + TokenRegistry frankRegistry = provider.getOauthTokenRegistry("frank"); + frankRegistry.createTokenTable(LEAK_PLUGIN); + + assertEquals("erin-access-token", erinRegistry.getTokenTable(LEAK_PLUGIN).getAccessToken()); + assertNull("another user must not read these tokens", + frankRegistry.getTokenTable(LEAK_PLUGIN).getAccessToken()); + + TokenRegistry sharedRegistry = provider.getOauthTokenRegistry(null); + sharedRegistry.createTokenTable(LEAK_PLUGIN); + assertNull("the shared registry must not expose a user's tokens", + sharedRegistry.getTokenTable(LEAK_PLUGIN).getAccessToken()); + } + + /** + * With authentication disabled, queries run as + * {@link org.apache.drill.exec.server.rest.auth.DrillUserPrincipal#ANONYMOUS_USER} + * (see {@code DrillRestServer}, which builds the anonymous session's user + * credentials from that principal). A user translation plugin's REST writes must + * therefore target the anonymous user's registry and not the shared one, or the + * tokens would be written where the query path never reads them. + */ + @Test + public void testAnonymousUserIsNotTheSharedRegistry() { + OAuthTokenProvider provider = provider(); + + TokenRegistry anonymous = provider.getOauthTokenRegistry(DrillUserPrincipal.ANONYMOUS_USER); + TokenRegistry shared = provider.getOauthTokenRegistry(null); + + assertNotSame( + "the anonymous user's registry is a per-user registry, not the shared one", + shared, anonymous); + } + + /** + * Writing the shared token table must not disturb a user's own tokens. Before + * registries were resolved per user, whichever identity was looked up first won + * and every later lookup returned that same registry. + */ + @Test + public void testSharedTokensDoNotOverwriteUserTokens() { + OAuthTokenProvider provider = provider(); + + TokenRegistry graceRegistry = provider.getOauthTokenRegistry("grace"); + graceRegistry.createTokenTable(OVERWRITE_PLUGIN); + graceRegistry.getTokenTable(OVERWRITE_PLUGIN).setAccessToken("grace-access-token"); + + TokenRegistry sharedRegistry = provider.getOauthTokenRegistry(null); + sharedRegistry.createTokenTable(OVERWRITE_PLUGIN); + sharedRegistry.getTokenTable(OVERWRITE_PLUGIN).setAccessToken("shared-access-token"); + + assertEquals("a shared token write must not clobber a user's tokens", + "grace-access-token", graceRegistry.getTokenTable(OVERWRITE_PLUGIN).getAccessToken()); + assertEquals("the shared table must hold its own value", + "shared-access-token", sharedRegistry.getTokenTable(OVERWRITE_PLUGIN).getAccessToken()); + } +} diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/server/rest/CredentialPluginInfoTest.java b/exec/java-exec/src/test/java/org/apache/drill/exec/server/rest/CredentialPluginInfoTest.java new file mode 100644 index 00000000000..bf5bde746b7 --- /dev/null +++ b/exec/java-exec/src/test/java/org/apache/drill/exec/server/rest/CredentialPluginInfoTest.java @@ -0,0 +1,227 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.drill.exec.server.rest; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; + +import java.util.HashMap; +import java.util.Map; + +import com.fasterxml.jackson.databind.ObjectMapper; +import org.apache.drill.common.logical.OAuthConfig; +import org.apache.drill.common.logical.StoragePluginConfig; +import org.apache.drill.common.logical.security.CredentialsProvider; +import org.apache.drill.common.logical.security.PlainCredentialsProvider; +import org.apache.drill.categories.SecurityTest; +import org.apache.drill.test.BaseTest; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +/** + * {@code GET /credentials.json} is reachable by any authenticated user, because + * supplying one's own data source credentials is a non-admin action. These tests + * pin down that the response carries only what the caller needs for that, and in + * particular never another user's credentials nor the plugin config they live in. + */ +@Category(SecurityTest.class) +public class CredentialPluginInfoTest extends BaseTest { + + private static final String ALICE = "alice"; + private static final String BOB = "bob"; + + private static final ObjectMapper MAPPER = new ObjectMapper(); + + /** + * Minimal concrete config. Mirrors a real user translation plugin: credentials + * live in the provider rather than on the config, so {@code directCredentials} + * is false and the provider would be serialized if the config ever were. + */ + private static class TestPluginConfig extends StoragePluginConfig { + TestPluginConfig(CredentialsProvider credentialsProvider, AuthMode authMode, OAuthConfig oAuthConfig) { + super(credentialsProvider, false, authMode, oAuthConfig); + } + + @Override + public String getValue(String key) { + return null; + } + + @Override + public boolean equals(Object o) { + return this == o; + } + + @Override + public int hashCode() { + return System.identityHashCode(this); + } + } + + /** + * A username/password user translation plugin with two users' credentials + * stored, plus a shared fallback secret in the provider's shared map. + * + *

Note the shared map deliberately holds no {@code clientID}: Drill defines + * an OAuth plugin as one whose credentials carry a client ID, so adding one + * here would make this an OAuth plugin instead. + */ + private static StoragePluginConfig twoUserConfig() { + Map shared = new HashMap<>(); + shared.put("password", "shared-fallback-password"); + + PlainCredentialsProvider provider = new PlainCredentialsProvider(shared, new HashMap<>()); + provider.setUserCredentials("alice_at_source", "alice_secret", ALICE); + provider.setUserCredentials("bob_at_source", "bob_secret", BOB); + + StoragePluginConfig config = new TestPluginConfig(provider, StoragePluginConfig.AuthMode.USER_TRANSLATION, null); + config.setEnabled(true); + return config; + } + + private static String serialize(String name, StoragePluginConfig config, String queryUser) throws Exception { + return MAPPER.writeValueAsString(CredentialPluginInfo.from(name, config, queryUser)); + } + + @Test + public void testResponseOmitsPluginConfigEntirely() throws Exception { + String json = serialize("h2", twoUserConfig(), ALICE); + + assertFalse("the plugin config must not be serialized", json.contains("\"config\"")); + assertFalse(json.contains("credentialsProvider")); + assertFalse(json.contains("userCredentials")); + } + + @Test + public void testResponseLeaksNoPasswordsOrOtherUsers() throws Exception { + String json = serialize("h2", twoUserConfig(), ALICE); + + // The caller's own password is as sensitive as anyone else's: they supplied + // it, so there is no reason to hand it back. + assertFalse(json.contains("alice_secret")); + assertFalse("another user's password must never be disclosed", json.contains("bob_secret")); + assertFalse("another user's source username must never be disclosed", json.contains("bob_at_source")); + assertFalse(json.contains(BOB)); + } + + /** + * A user translation plugin's provider still has a shared credentials map + * alongside the per-user one. It is admin-owned configuration and must not + * reach a non-admin either. + */ + @Test + public void testResponseLeaksNoSharedSecrets() throws Exception { + String json = serialize("h2", twoUserConfig(), ALICE); + + assertFalse("a shared secret must never be disclosed to a non-admin", + json.contains("shared-fallback-password")); + } + + @Test + public void testReportsTheCallersOwnCredentialStateOnly() { + StoragePluginConfig config = twoUserConfig(); + + CredentialPluginInfo forAlice = CredentialPluginInfo.from("h2", config, ALICE); + assertFalse("fixture must be a username/password plugin, not OAuth", forAlice.isOauth()); + assertTrue(forAlice.isHasCredentials()); + assertEquals("alice_at_source", forAlice.getUserName()); + + CredentialPluginInfo forBob = CredentialPluginInfo.from("h2", config, BOB); + assertTrue(forBob.isHasCredentials()); + assertEquals("bob_at_source", forBob.getUserName()); + } + + @Test + public void testUserWithNoStoredCredentials() { + CredentialPluginInfo info = CredentialPluginInfo.from("h2", twoUserConfig(), "carol"); + + assertFalse(info.isHasCredentials()); + assertNull(info.getUserName()); + assertEquals("h2", info.getName()); + assertTrue(info.isEnabled()); + } + + @Test + public void testUnresolvedUserReportsNoCredentialState() { + CredentialPluginInfo info = CredentialPluginInfo.from("h2", twoUserConfig(), null); + + assertFalse(info.isHasCredentials()); + assertNull(info.getUserName()); + } + + /** + * A credentials provider is not obliged to support per-user credentials; the + * interface default throws. Listing the plugin must still succeed, or one such + * plugin would blank the whole page. + */ + @Test + public void testProviderWithoutPerUserSupportIsNotFatal() { + CredentialsProvider noPerUser = new CredentialsProvider() { + @Override + public Map getCredentials() { + return new HashMap<>(); + } + }; + + StoragePluginConfig config = + new TestPluginConfig(noPerUser, StoragePluginConfig.AuthMode.USER_TRANSLATION, null); + config.setEnabled(true); + + CredentialPluginInfo info = CredentialPluginInfo.from("env_backed", config, ALICE); + + assertEquals("env_backed", info.getName()); + assertFalse(info.isHasCredentials()); + assertNull(info.getUserName()); + } + + /** + * OAuth plugins keep per-user state as tokens in the token registry rather than + * as a username, so no credential state is reported. The authorize URL is still + * built, since the browser needs it to start the flow, and it carries the client + * ID and callback but no secret. + */ + @Test + public void testOauthPluginExposesAuthorizeUrlButNoSecret() throws Exception { + OAuthConfig oAuthConfig = OAuthConfig.builder() + .authorizationURL("https://example.com/authorize") + .callbackURL("https://drill.example.com/callback") + .scope("read") + .build(); + + Map shared = new HashMap<>(); + shared.put("clientID", "shared-client-id"); + shared.put("clientSecret", "shared-client-secret"); + + StoragePluginConfig config = new TestPluginConfig( + new PlainCredentialsProvider(shared, new HashMap<>()), + StoragePluginConfig.AuthMode.USER_TRANSLATION, + oAuthConfig); + config.setEnabled(true); + + CredentialPluginInfo info = CredentialPluginInfo.from("oauth_plugin", config, ALICE); + + assertTrue(info.isOauth()); + assertTrue(info.getAuthorizationUrl().startsWith("https://example.com/authorize?client_id=shared-client-id")); + assertFalse(info.isHasCredentials()); + assertNull(info.getUserName()); + + assertFalse(MAPPER.writeValueAsString(info).contains("shared-client-secret")); + } +} From e7ff67cf3b5edc3436a5eaa9f29ee96762cabb26 Mon Sep 17 00:00:00 2001 From: cgivre Date: Sun, 25 Oct 2026 14:02:39 -0400 Subject: [PATCH 2/2] Address review feedback on OAuth token isolation 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. --- .../GoogleSheetsStoragePlugin.java | 66 ++++++++++++++++++- docs/dev/OAuthTokenStorage.md | 41 ++++++++++++ .../drill/exec/oauth/OAuthTokenProvider.java | 16 ++++- .../exec/server/rest/CredentialResources.java | 4 +- .../exec/server/rest/StorageResources.java | 6 -- .../exec/oauth/OAuthTokenProviderTest.java | 34 ++++++++++ 6 files changed, 155 insertions(+), 12 deletions(-) diff --git a/contrib/storage-googlesheets/src/main/java/org/apache/drill/exec/store/googlesheets/GoogleSheetsStoragePlugin.java b/contrib/storage-googlesheets/src/main/java/org/apache/drill/exec/store/googlesheets/GoogleSheetsStoragePlugin.java index 98b931648e3..500c6ba2a09 100644 --- a/contrib/storage-googlesheets/src/main/java/org/apache/drill/exec/store/googlesheets/GoogleSheetsStoragePlugin.java +++ b/contrib/storage-googlesheets/src/main/java/org/apache/drill/exec/store/googlesheets/GoogleSheetsStoragePlugin.java @@ -44,6 +44,7 @@ import org.apache.drill.exec.store.base.filter.FilterPushDownUtils; import org.apache.drill.exec.store.googlesheets.schema.GoogleSheetsSchemaFactory; import org.apache.drill.exec.store.googlesheets.utils.GoogleSheetsUtils; +import org.apache.commons.lang3.StringUtils; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableSet; import org.slf4j.Logger; @@ -55,6 +56,7 @@ import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; +import java.util.function.Supplier; public class GoogleSheetsStoragePlugin extends AbstractStoragePlugin { private final static Logger logger = LoggerFactory.getLogger(GoogleSheetsStoragePlugin.class); @@ -76,6 +78,12 @@ public class GoogleSheetsStoragePlugin extends AbstractStoragePlugin { private final Map sheetsServices = new ConcurrentHashMap<>(); private final Map driveServices = new ConcurrentHashMap<>(); + /** + * The access token each identity's cached entries were built from; see + * {@link #cached(Map, String, Supplier)}. + */ + private final Map cachedAccessTokens = new ConcurrentHashMap<>(); + public GoogleSheetsStoragePlugin(GoogleSheetsStoragePluginConfig configuration, DrillbitContext context, String name) { super(context, name); @@ -155,7 +163,59 @@ private DataStore buildDataStore(String queryUser) { } public DataStore getDataStore(String queryUser) { - return dataStores.computeIfAbsent(cacheKey(queryUser), k -> buildDataStore(queryUser)); + return cached(dataStores, queryUser, () -> buildDataStore(queryUser)); + } + + /** + * This identity's cached value, built on first use, but only kept for as long + * as the tokens it was built from are the tokens in the token table. + * + *

Google's {@code Credential} copies the access and refresh tokens once, and + * {@link DrillDataStore} reads the token table only in its constructor. So an + * entry built before the user authorized would serve an empty credential until + * the drillbit restarted, and under user translation that is the normal flow + * for a new user: query, get an auth error, authorize, query again. Nothing is + * cached while the identity has no access token, and an entry is dropped once + * the stored token differs from the one it was built from, so authorizing or + * re-authorizing takes effect on the next query. + * + *

A token refresh rewrites the stored token as well, which costs one extra + * rebuild; racing callers may likewise build one entry more than they use. + * Both are cheap next to serving the wrong identity's credential. + */ + private T cached(Map cache, String queryUser, Supplier builder) { + String key = cacheKey(queryUser); + String accessToken = getTokenTable(queryUser).getAccessToken(); + + if (StringUtils.isEmpty(accessToken)) { + invalidate(key); + return builder.get(); + } + + if (!accessToken.equals(cachedAccessTokens.get(key))) { + invalidate(key); + cachedAccessTokens.put(key, accessToken); + } + + T cachedValue = cache.get(key); + if (cachedValue != null) { + return cachedValue; + } + + // Built outside computeIfAbsent: building a service asks for this identity's + // data store, which comes back through here, and a mapping function must not + // touch the map it is updating. + T value = builder.get(); + cache.put(key, value); + return value; + } + + /** Forgets every service cached for one identity. */ + private void invalidate(String key) { + dataStores.remove(key); + sheetsServices.remove(key); + driveServices.remove(key); + cachedAccessTokens.remove(key); } @@ -254,7 +314,7 @@ public boolean supportsInsert() { * @return An authenticated {@link Sheets} Google Sheets service. */ public Sheets getSheetsService(String queryUser) { - return sheetsServices.computeIfAbsent(cacheKey(queryUser), k -> buildSheetsService(queryUser)); + return cached(sheetsServices, queryUser, () -> buildSheetsService(queryUser)); } private Sheets buildSheetsService(String queryUser) { @@ -275,7 +335,7 @@ private Sheets buildSheetsService(String queryUser) { * @return A validated and authenticated {@link Drive} instance. */ public Drive getDriveService(String queryUser) { - return driveServices.computeIfAbsent(cacheKey(queryUser), k -> buildDriveService(queryUser)); + return cached(driveServices, queryUser, () -> buildDriveService(queryUser)); } private Drive buildDriveService(String queryUser) { diff --git a/docs/dev/OAuthTokenStorage.md b/docs/dev/OAuthTokenStorage.md index 7feea9c8d1a..e53b826f4ff 100644 --- a/docs/dev/OAuthTokenStorage.md +++ b/docs/dev/OAuthTokenStorage.md @@ -41,6 +41,12 @@ oauth_tokens/ <- one registry per user translation user Each registry is a separate persistent store, keyed inside by plugin name. +The user name is percent-encoded before it becomes a path segment, because a user +name is not restricted to what a store path accepts: a full Kerberos principal +(`user/host@REALM`) carries a separator, and a name of `..` would walk up the +tree, so either could otherwise land one identity's tokens in a path belonging to +another. `.` is encoded as well, which `URLEncoder` leaves alone. + **A registry must be resolved per call and never cached across identities.** Returning a registry belonging to another user would let that user's queries run with someone else's tokens at the data source, which is exactly what user @@ -70,6 +76,16 @@ Those are now keyed per identity. Its cache key is prefixed (`user:` or `anonymous` — which is every web user when authentication is disabled — cannot share an entry with a shared-credential plugin. +Those entries are also tagged with the access token they were built from, and +nothing is cached while an identity has no access token at all. Google's +`Credential` copies the tokens once and `DrillDataStore` reads the token table +only in its constructor, so an entry built before the user authorized would +otherwise serve an empty credential until the drillbit restarted — and under user +translation that is the normal flow for a new user: query, get an auth error, +authorize, query again. A stored token that no longer matches the tag drops the +entry, so authorizing, or re-authorizing after a revoke, takes effect on the next +query. A token refresh rewrites the stored token too, which costs one rebuild. + ### Why not the aliases layout [`Aliases.md`](Aliases.md)' `PersistentAliasRegistry` solves the same per-user @@ -158,6 +174,19 @@ tokens to the shared `oauth_tokens` registry. Those tokens are now read from `oauth_tokens/anonymous`, so the old ones are ignored and the user authorizes once more. No data is lost; the stale entries can be removed by hand. +### User translation needs authentication + +`USER_TRANSLATION` with Drill's user authentication switched off is not a +supported combination, and the per-identity registries make that plainer rather +than changing it. Translating users means handing each one their own credentials, +which requires user names that are unique and trusted. With auth off neither +holds: web UI and REST callers are all `anonymous`, a JDBC/ODBC/sqlline client +sends whatever name it likes (usually the OS user), and every caller is an admin. +Tokens authorized through the web UI therefore sit in `oauth_tokens/anonymous`, +which is not where a JDBC client's query looks. + +Use `SHARED_USER` on an auth-off cluster, or turn user authentication on. + ### The shared identity must be spelled the same way everywhere `null` means "the shared registry". GoogleSheets also has a Google-side data @@ -172,6 +201,18 @@ interchangeable. This was invisible while one cached registry served every lookup, and only surfaced once registries were resolved per identity. +## Known limitation: per-identity caches are unbounded + +`OAuthTokenProvider` holds one `PersistentTokenRegistry` per identity, and +`GoogleSheetsStoragePlugin` one data store and one of each Google service per +identity. Both are populated on first use and never evicted, so they grow to one +entry per distinct user for the life of the drillbit. The entries are small — +a registry wraps a persistent store handle, a Google service an HTTP transport — +and a cluster with a handful of user translation users will not notice. A +deployment with many thousands of distinct users wants a bounded or expiring +cache here; nothing in the current design depends on an entry living forever, +since both are rebuilt on demand. + ## Known limitation: orphaned token tables Deleting a storage plugin (`DELETE /storage/{name}.json`) deletes its token diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/oauth/OAuthTokenProvider.java b/exec/java-exec/src/main/java/org/apache/drill/exec/oauth/OAuthTokenProvider.java index e00e2e876d0..ddb38840050 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/oauth/OAuthTokenProvider.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/oauth/OAuthTokenProvider.java @@ -21,6 +21,8 @@ import org.apache.drill.common.AutoCloseables; import org.apache.drill.exec.server.DrillbitContext; +import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; @@ -78,10 +80,22 @@ public TokenRegistry getOauthTokenRegistry(String username) { */ private static String registryPath(String username) { return StringUtils.isNotEmpty(username) - ? STORAGE_REGISTRY_PATH + "/" + username + ? STORAGE_REGISTRY_PATH + "/" + encodePathSegment(username) : STORAGE_REGISTRY_PATH; } + /** + * Percent-encodes a user name so that it forms a single path segment. A user + * name is not restricted to what a store path accepts: a full Kerberos + * principal ({@code user/host@REALM}) carries a separator, and a name of + * {@code ..} would walk up the tree, so either could place one identity's + * tokens in a path belonging to another. {@code .} is encoded as well, which + * {@link URLEncoder} leaves untouched. + */ + private static String encodePathSegment(String username) { + return URLEncoder.encode(username, StandardCharsets.UTF_8).replace(".", "%2E"); + } + @Override public void close() throws Exception { AutoCloseables.closeSilently(oauthTokenRegistries.values().toArray(new AutoCloseable[0])); diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/CredentialResources.java b/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/CredentialResources.java index 2e46e766d2a..4a216857750 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/CredentialResources.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/CredentialResources.java @@ -183,7 +183,7 @@ private String queryUser() { public Response createOrUpdateCredentials(@FormParam("plugin") String pluginName, @FormParam("username") String username, @FormParam("password") String password) { - String queryUser = queryUser(); + String queryUser = queryUser(); if (queryUser == null) { // Credentials are always stored against a user, so refuse rather than // write an entry under a null key that nobody could ever read back. @@ -236,7 +236,7 @@ public Response createOrUpdateCredentials(@FormParam("plugin") String pluginName @Produces(MediaType.APPLICATION_JSON) @Operation(externalDocs = @ExternalDocumentation(description = "Apache Drill REST API documentation:", url = "https://drill.apache.org/docs/rest-api-introduction/")) public Response createOrUpdatePlugin(@PathParam("pluginName") String pluginName, UsernamePasswordContainer credentials) { - String queryUser = queryUser(); + String queryUser = queryUser(); if (queryUser == null) { // Credentials are always stored against a user, so refuse rather than // write an entry under a null key that nobody could ever read back. diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/StorageResources.java b/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/StorageResources.java index 48652a43306..3075e562c39 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/StorageResources.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/StorageResources.java @@ -425,12 +425,6 @@ public Response deletePluginViaGet(@PathParam("name") String name) { return deletePlugin(name); } - /** - * This function checks to see if a given storage plugin is using USER_TRANSLATION mode. - * If so, it will return the query user name. If not it will return null. - * @param config {@link StoragePluginConfig} The current plugin configuration - * @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 * translation plugin, or null for the single shared table. The name must match diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/oauth/OAuthTokenProviderTest.java b/exec/java-exec/src/test/java/org/apache/drill/exec/oauth/OAuthTokenProviderTest.java index 9a934b86edc..3deb98a0b6c 100644 --- a/exec/java-exec/src/test/java/org/apache/drill/exec/oauth/OAuthTokenProviderTest.java +++ b/exec/java-exec/src/test/java/org/apache/drill/exec/oauth/OAuthTokenProviderTest.java @@ -48,6 +48,7 @@ public class OAuthTokenProviderTest extends ClusterTest { */ private static final String LEAK_PLUGIN = "leak_test_plugin"; private static final String OVERWRITE_PLUGIN = "overwrite_test_plugin"; + private static final String PATH_PLUGIN = "path_test_plugin"; @BeforeClass public static void setUp() throws Exception { @@ -140,6 +141,39 @@ public void testAnonymousUserIsNotTheSharedRegistry() { shared, anonymous); } + /** + * A user name becomes part of the store path, so a name carrying a path + * separator or a parent reference must not be able to reach another identity's + * path. A name ending in {@code /..} is the dangerous one: unencoded it walks + * back up to the shared registry's own path. + */ + @Test + public void testUserNamesCannotEscapeTheirOwnPath() { + OAuthTokenProvider provider = provider(); + + TokenRegistry traversal = provider.getOauthTokenRegistry("ivan/.."); + traversal.createTokenTable(PATH_PLUGIN); + traversal.getTokenTable(PATH_PLUGIN).setAccessToken("traversal-access-token"); + + TokenRegistry shared = provider.getOauthTokenRegistry(null); + shared.createTokenTable(PATH_PLUGIN); + assertNull("a user name must not be able to walk up into the shared registry", + shared.getTokenTable(PATH_PLUGIN).getAccessToken()); + + // A full Kerberos principal contains a separator and must still work, and + // must not share a path with the user name it starts with. + TokenRegistry principal = provider.getOauthTokenRegistry("heidi/host@EXAMPLE.COM"); + principal.createTokenTable(PATH_PLUGIN); + principal.getTokenTable(PATH_PLUGIN).setAccessToken("heidi-access-token"); + + TokenRegistry heidi = provider.getOauthTokenRegistry("heidi"); + heidi.createTokenTable(PATH_PLUGIN); + assertNull("a principal's tokens must not be readable as the bare user name", + heidi.getTokenTable(PATH_PLUGIN).getAccessToken()); + assertEquals("heidi-access-token", + principal.getTokenTable(PATH_PLUGIN).getAccessToken()); + } + /** * Writing the shared token table must not disturb a user's own tokens. Before * registries were resolved per user, whichever identity was looked up first won