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..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; @@ -52,7 +53,10 @@ 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; +import java.util.function.Supplier; public class GoogleSheetsStoragePlugin extends AbstractStoragePlugin { private final static Logger logger = LoggerFactory.getLogger(GoogleSheetsStoragePlugin.class); @@ -60,11 +64,25 @@ 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<>(); + + /** + * 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) { @@ -95,22 +113,109 @@ 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; + } + + /** + * 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"; } - public DataStore getDataStore(String username) { - if (this.dataStore == null) { - this.dataStore = new DrillDataStoreFactory(tokenProvider, getName()).createDataStore(username); + private DataStore buildDataStore(String queryUser) { + return new DrillDataStoreFactory(tokenProvider, getName(), registryIdentity(queryUser)) + .createDataStore(googleStoreId(queryUser)); + } + + public DataStore getDataStore(String 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(); } - return dataStore; + + 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); } @@ -120,8 +225,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 +267,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 +296,7 @@ public StoragePluginConfig getConfig() { */ @VisibleForTesting public void initializeTokenTableForTesting() { - OAuthTokenProvider tokenProvider = context.getOauthTokenProvider(); - tokenRegistry = tokenProvider.getOauthTokenRegistry(null); + getTokenRegistry(null); } @Override @@ -201,26 +314,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 cached(sheetsServices, queryUser, () -> 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 +335,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 cached(driveServices, queryUser, () -> 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..e53b826f4ff --- /dev/null +++ b/docs/dev/OAuthTokenStorage.md @@ -0,0 +1,248 @@ +# 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. + +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 +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. + +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 +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. + +### 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 +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: 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 +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..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,44 +21,84 @@ 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; + /** * 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; + + // 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)) + ); } - 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; - } + /** + * 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 + "/" + encodePathSegment(username) + : STORAGE_REGISTRY_PATH; + } - if (oauthTokenRegistry == null) { - oauthTokenRegistry = new PersistentTokenRegistry(context, finalpath); - } + /** + * 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(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..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 @@ -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..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 @@ -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); } /** @@ -417,14 +426,14 @@ public Response deletePluginViaGet(@PathParam("name") String 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 + * 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..3deb98a0b6c --- /dev/null +++ b/exec/java-exec/src/test/java/org/apache/drill/exec/oauth/OAuthTokenProviderTest.java @@ -0,0 +1,199 @@ +/* + * 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"; + private static final String PATH_PLUGIN = "path_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); + } + + /** + * 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 + * 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")); + } +}