[
https://issues.apache.org/jira/browse/DRILL-8558?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18131087#comment-18131087
]
ASF GitHub Bot commented on DRILL-8558:
---------------------------------------
pjfanning commented on code in PR #3094:
URL: https://github.com/apache/drill/pull/3094#discussion_r4362985948
##########
contrib/storage-googlesheets/src/main/java/org/apache/drill/exec/store/googlesheets/GoogleSheetsStoragePlugin.java:
##########
@@ -201,26 +254,17 @@ public boolean supportsInsert() {
* @return An authenticated {@link Sheets} Google Sheets service.
*/
public Sheets getSheetsService(String queryUser) {
- if (service != null && dataStore != null) {
- return service;
- } else {
- // Check if datastore is null and initialize if so.
- if (dataStore == null) {
- this.dataStore = getDataStore(queryUser);
- }
-
- try {
- if (config.getAuthMode() == AuthMode.USER_TRANSLATION) {
- service = GoogleSheetsUtils.getSheetsService(config, dataStore,
queryUser);
- } else {
- service = GoogleSheetsUtils.getSheetsService(config, dataStore,
SHARED_USERNAME);
- }
- return service;
- } catch (IOException | GeneralSecurityException e) {
- throw UserException.connectionError(e)
- .message("Error connecting to Googlesheets Service: " +
e.getMessage())
- .build(logger);
- }
+ return sheetsServices.computeIfAbsent(cacheKey(queryUser), k ->
buildSheetsService(queryUser));
Review Comment:
These per-identity caches pin whatever tokens existed when the entry was
first built. `GoogleSheetsUtils.loadCredential` copies the access/refresh
tokens into the `Credential` once, and `DrillDataStore` only reads the token
table in its constructor. So if a user queries before authorizing, an empty
credential is cached in `sheetsServices` / `driveServices` / `dataStores` until
restart, and re-authorizing (e.g. after revoking access) has no effect.
The old single-field cache had the same flaw, but under `USER_TRANSLATION`
the usual flow for a new user is "query, get an auth error, authorize, query
again", so most users will now hit it. Could we avoid caching when the stored
credential has no tokens, and/or evict a user's entries when `OAuthRequests`
writes their tokens? The Google Sheets suites are all `@Ignore`d, so nothing
in-tree would catch this.
##########
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/CredentialResources.java:
##########
@@ -146,7 +183,14 @@ public List<PluginConfigWrapper>
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();
Review Comment:
Nit: indented 8 spaces instead of 4. The same line in `createOrUpdatePlugin`
(L239) has the same issue.
##########
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/StorageResources.java:
##########
@@ -422,9 +431,15 @@ public Response deletePluginViaGet(@PathParam("name")
String name) {
* @param config {@link StoragePluginConfig} The current plugin configuration
* @return If USER_TRANSLATION is enabled, returns the query user. If not,
returns null.
*/
+ /**
+ * The owner of the token table belonging to this plugin: the caller for a
user
Review Comment:
Nit: the old javadoc block just above wasn't removed, so `getQueryUser` now
has two javadoc comments stacked.
##########
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/OAuthRequests.java:
##########
@@ -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.
+ *
+ * <p>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 <em>all</em> 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.
+ *
+ * <p>{@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();
Review Comment:
With auth disabled this writes user-translation tokens to
`oauth_tokens/anonymous`. Only web UI / REST queries run as `anonymous`,
though. JDBC/ODBC/sqlline clients with auth off run as whatever user name the
client sends (usually the OS user), so they will no longer find these tokens.
Previously they all used the shared registry.
The upgrade note in the PR description only covers re-authorizing. It should
also say that non-web clients on an auth-disabled cluster lose OAuth access
under user translation. Alternatively, keep the shared registry when auth is
disabled; there's no real isolation in that mode anyway, since every caller is
admin and the user name is client-supplied.
##########
exec/java-exec/src/main/java/org/apache/drill/exec/oauth/OAuthTokenProvider.java:
##########
@@ -21,44 +21,70 @@
import org.apache.drill.common.AutoCloseables;
import org.apache.drill.exec.server.DrillbitContext;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+
/**
* Class for managing oauth tokens. Storage plugins will have to manage
obtaining the tokens, but
* these classes handle the storage of access and refresh tokens.
+ *
+ * <p>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<String, PersistentTokenRegistry> oauthTokenRegistries =
new ConcurrentHashMap<>();
Review Comment:
This map, and the three per-identity maps in `GoogleSheetsStoragePlugin`,
only ever grow: one entry per distinct user for the life of the drillbit.
That's fine with a handful of users. It's worth a sentence in
`OAuthTokenStorage.md` next to the orphaned-token-table limitation, or a
bounded/expiring cache if many users are expected.
##########
exec/java-exec/src/main/java/org/apache/drill/exec/oauth/OAuthTokenProvider.java:
##########
@@ -21,44 +21,70 @@
import org.apache.drill.common.AutoCloseables;
import org.apache.drill.exec.server.DrillbitContext;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+
/**
* Class for managing oauth tokens. Storage plugins will have to manage
obtaining the tokens, but
* these classes handle the storage of access and refresh tokens.
+ *
+ * <p>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<String, PersistentTokenRegistry> 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.
+ *
+ * <p>The registry is resolved per call: returning a registry belonging to
+ * another user would let one user's queries run with another user's tokens,
+ * which is precisely what user translation exists to prevent.
+ */
public TokenRegistry getOauthTokenRegistry(String username) {
- if (oauthTokenRegistry == null) {
- initRemoteRegistries(username);
- }
- return oauthTokenRegistry;
- }
+ String key = StringUtils.isNotEmpty(username) ? username :
SHARED_REGISTRY_KEY;
- private synchronized void initRemoteRegistries(String username) {
- // Add the username to the path if present
- String finalpath;
- if (StringUtils.isNotEmpty(username)) {
- finalpath = STORAGE_REGISTRY_PATH + "/" + username;
- } else {
- finalpath = STORAGE_REGISTRY_PATH;
- }
+ // computeIfAbsent so that concurrent callers for the same identity share
one
+ // registry, and so a failure to build one is not cached.
+ return oauthTokenRegistries.computeIfAbsent(
+ key,
+ k -> new PersistentTokenRegistry(context, registryPath(username))
+ );
+ }
- if (oauthTokenRegistry == null) {
- oauthTokenRegistry = new PersistentTokenRegistry(context, finalpath);
- }
+ /**
+ * The persistent store path for a user's tokens, or the root path for the
+ * shared registry.
+ */
+ private static String registryPath(String username) {
+ return StringUtils.isNotEmpty(username)
+ ? STORAGE_REGISTRY_PATH + "/" + username
Review Comment:
The user name goes straight into the persistent store path. That was already
true before, but per-user registries are now actually used. A name containing
`/` (e.g. a full Kerberos principal like `user/host@REALM`) or `..` would map
to an unexpected path, possibly one belonging to another identity. Consider
escaping/encoding the name, or rejecting names that contain path separators.
> Fix Credential Race Condition and Credential Leak
> -------------------------------------------------
>
> Key: DRILL-8558
> URL: https://issues.apache.org/jira/browse/DRILL-8558
> Project: Apache Drill
> Issue Type: Bug
> Components: Storage - GoogleSheets, Storage - HTTP
> Affects Versions: 1.22.0
> Reporter: Charles Givre
> Assignee: Charles Givre
> Priority: Major
> Fix For: 1.23.0
>
>
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
