walterddr commented on code in PR #8670:
URL: https://github.com/apache/pinot/pull/8670#discussion_r870470887
##########
pinot-spi/src/main/java/org/apache/pinot/spi/ingestion/batch/spec/SegmentGenerationJobSpec.java:
##########
@@ -124,6 +124,12 @@ public class SegmentGenerationJobSpec implements
Serializable {
/**
* Controller auth token
+ *
+ * <br/>NOTE: jobs MUST NOT allow references to external tokens via URL or
path to prevent:
+ * (a) file system crawling
+ * (b) unauthorized injection of system tokens from the server's local file
system.
+ *
+ * Instead, resolve tokens right when the job command is run. This allows
injection of client-local credentials.
*/
private String _authToken;
Review Comment:
why don't we also make this _authProvider similar to
InstanceDataManagerConfig change?
##########
pinot-common/src/main/java/org/apache/pinot/common/auth/StaticTokenAuthProvider.java:
##########
@@ -0,0 +1,68 @@
+/**
+ * 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.pinot.common.auth;
+
+import java.util.Collections;
+import java.util.Map;
+import javax.ws.rs.core.HttpHeaders;
+import org.apache.pinot.spi.auth.AuthProvider;
+
+
+/**
+ * Auth provider for static client tokens, typically used for job specs or
when mimicking legacy behavior.
+ */
+public class StaticTokenAuthProvider implements AuthProvider {
+ public static final String HEADER = "header";
+ public static final String PREFIX = "prefix";
+ public static final String TOKEN = "token";
+
+ protected final String _header;
+ protected final String _prefix;
+ protected final String _token;
+
+ public StaticTokenAuthProvider(String token) {
+ _header = HttpHeaders.AUTHORIZATION;
+ _prefix = "";
+ _token = token;
+ }
+
+ public StaticTokenAuthProvider(AuthConfig authConfig) {
+ _header = AuthProviderUtils.getOrDefault(authConfig, HEADER,
HttpHeaders.AUTHORIZATION);
+ _prefix = AuthProviderUtils.getOrDefault(authConfig, PREFIX, "Basic");
+ _token = authConfig.getProperties().get(TOKEN).toString();
+ }
+
+ @Override
+ public Map<String, Object> getRequestHeaders() {
+ return Collections.singletonMap(_header, makeToken());
Review Comment:
both getRequestHeaders and getTaskTokens return values are constant, let's
make them final member variable as well in constructor?
##########
pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/AbstractBaseAdminCommand.java:
##########
@@ -125,34 +129,44 @@ Map<String, Object> readConfigFromFile(String
configFileName)
}
/**
- * Generate an (optional) HTTP Authorization header given an auth token
- * @see HttpClient#makeAuthHeader(String)
+ * Generate an (optional) HTTP Authorization header given an auth config
*
- * @param authToken auth token
- * @return list of 0 or 1 "Authorization" headers
+ * @param authProvider auth provider
+ * @return list of headers
*/
- static List<Header> makeAuthHeader(String authToken) {
- return HttpClient.makeAuthHeader(authToken);
+ static List<Header> makeAuthHeaders(AuthProvider authProvider) {
+ return AuthProviderUtils.toRequestHeaders(authProvider);
}
/**
* Generate auth token from pass-thru token or generate basic auth from
user/password pair
*
+ * @param provider optional provider
+ * @param tokenUrl optional token url
* @param authToken optional pass-thru token
* @param user optional username
* @param password optional password
* @return auth token, or null if neither pass-thru token nor user info
available
Review Comment:
```suggestion
* @return auth provider, or NullauthProvider if neither pass-thru token
nor user info available
```
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/BaseMultipleSegmentsConversionExecutor.java:
##########
@@ -291,8 +292,8 @@ public String getUploadURL() {
return _uploadURL;
}
- public String getAuthToken() {
- return _authToken;
+ public AuthProvider getAuthProvider() {
+ return _authProvider;
}
Review Comment:
this and other replacement for getAuthToken(). what's the basic rule for
removing API entirely vs. changing it to return provider API?
in general. should _authProvider be a singleton across the JVM? or should we
instantiate one at each class object via
`AuthProviderUtils.extractAuthProvider` from config?
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManager.java:
##########
@@ -103,7 +105,7 @@ public synchronized void init(PinotConfiguration config,
HelixManager helixManag
_instanceId = _instanceDataManagerConfig.getInstanceId();
_helixManager = helixManager;
_serverMetrics = serverMetrics;
- _authToken =
config.getProperty(CommonConstants.Server.CONFIG_OF_AUTH_TOKEN);
+ _authProvider = AuthProviderUtils.extractAuthProvider(config,
CommonConstants.Server.CONFIG_OF_AUTH);
Review Comment:
this authprovider seems to be only used by TableDataManager. can't we
extract this from TableDataManager init method instead? (for backward
compatibility as not changing the init method)
##########
pinot-common/src/main/java/org/apache/pinot/common/auth/AuthProviderUtils.java:
##########
@@ -0,0 +1,168 @@
+/**
+ * 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.pinot.common.auth;
+
+import java.lang.reflect.Constructor;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.stream.Collectors;
+import javax.annotation.Nullable;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.http.Header;
+import org.apache.http.message.BasicHeader;
+import org.apache.pinot.spi.auth.AuthProvider;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * Utility class to wrap inference of optimal auth provider from component
configs.
+ */
+public final class AuthProviderUtils {
+ private AuthProviderUtils() {
+ // left blank
+ }
+
+ /**
+ * Extract an AuthConfig from a pinot configuration subset namespace.
+ *
+ * @param pinotConfig pinot configuration
+ * @param namespace subset namespace
+ * @return auth config
+ */
+ public static AuthConfig extractAuthConfig(PinotConfiguration pinotConfig,
String namespace) {
+ return new AuthConfig(pinotConfig.subset(namespace).toMap());
+ }
+
+ /**
+ * Create an AuthProvider after extracting a config from a pinot
configuration subset namespace
+ * @see AuthProviderUtils#extractAuthConfig(PinotConfiguration, String)
+ *
+ * @param pinotConfig pinot configuration
+ * @param namespace subset namespace
+ * @return auth provider
+ */
+ public static AuthProvider extractAuthProvider(PinotConfiguration
pinotConfig, String namespace) {
+ return makeDynamicProvider(extractAuthConfig(pinotConfig, namespace));
+ }
+
+ /**
+ * Create auth provider based on the availability of a static token only, if
any. This typically applies to task specs
+ *
+ * @param authToken static auth token
+ * @return auth provider
+ */
+ public static AuthProvider makeStaticProvider(String authToken) {
+ if (StringUtils.isBlank(authToken)) {
+ return new NullAuthProvider();
+ }
+ return new StaticTokenAuthProvider(authToken);
+ }
+
+ /**
+ * Create auth provider based on an auth config. Mimics legacy behavior for
static tokens if provided, or dynamic auth
+ * providers if additional configs are given.
+ *
+ * @param authConfig auth config
+ * @return auth provider
+ */
+ public static AuthProvider makeDynamicProvider(AuthConfig authConfig) {
Review Comment:
i think both methods should be named `makeAuthProvider` with the different
argument types. as it is not guarantee you will create an auth provider that
returns dynamic requestheader / tasktokens
##########
pinot-common/src/main/java/org/apache/pinot/common/auth/AuthProviderUtils.java:
##########
@@ -0,0 +1,168 @@
+/**
+ * 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.pinot.common.auth;
+
+import java.lang.reflect.Constructor;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.stream.Collectors;
+import javax.annotation.Nullable;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.http.Header;
+import org.apache.http.message.BasicHeader;
+import org.apache.pinot.spi.auth.AuthProvider;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * Utility class to wrap inference of optimal auth provider from component
configs.
+ */
+public final class AuthProviderUtils {
+ private AuthProviderUtils() {
+ // left blank
+ }
+
+ /**
+ * Extract an AuthConfig from a pinot configuration subset namespace.
+ *
+ * @param pinotConfig pinot configuration
+ * @param namespace subset namespace
+ * @return auth config
+ */
+ public static AuthConfig extractAuthConfig(PinotConfiguration pinotConfig,
String namespace) {
+ return new AuthConfig(pinotConfig.subset(namespace).toMap());
+ }
+
+ /**
+ * Create an AuthProvider after extracting a config from a pinot
configuration subset namespace
+ * @see AuthProviderUtils#extractAuthConfig(PinotConfiguration, String)
+ *
+ * @param pinotConfig pinot configuration
+ * @param namespace subset namespace
+ * @return auth provider
+ */
+ public static AuthProvider extractAuthProvider(PinotConfiguration
pinotConfig, String namespace) {
+ return makeDynamicProvider(extractAuthConfig(pinotConfig, namespace));
+ }
+
+ /**
+ * Create auth provider based on the availability of a static token only, if
any. This typically applies to task specs
+ *
+ * @param authToken static auth token
+ * @return auth provider
+ */
+ public static AuthProvider makeStaticProvider(String authToken) {
+ if (StringUtils.isBlank(authToken)) {
+ return new NullAuthProvider();
+ }
+ return new StaticTokenAuthProvider(authToken);
+ }
+
+ /**
+ * Create auth provider based on an auth config. Mimics legacy behavior for
static tokens if provided, or dynamic auth
+ * providers if additional configs are given.
+ *
+ * @param authConfig auth config
+ * @return auth provider
+ */
+ public static AuthProvider makeDynamicProvider(AuthConfig authConfig) {
+ if (authConfig == null) {
+ return new NullAuthProvider();
+ }
+
+ Object providerClassValue =
authConfig.getProperties().get(AuthConfig.PROVIDER_CLASS);
+ if (providerClassValue != null) {
+ try {
+ Class<?> providerClass = Class.forName(providerClassValue.toString());
+ Constructor<?> constructor =
providerClass.getConstructor(AuthConfig.class);
+ return (AuthProvider) constructor.newInstance(authConfig);
+ } catch (Exception e) {
+ throw new IllegalStateException("Could not create AuthProvider " +
providerClassValue, e);
+ }
+ }
+
+ // mimic legacy behavior for "auth.token" property
+ if (authConfig.getProperties().containsKey(StaticTokenAuthProvider.TOKEN))
{
+ return new StaticTokenAuthProvider(authConfig);
+ }
+
+ if (!authConfig.getProperties().isEmpty()) {
+ throw new IllegalArgumentException("Some auth properties defined, but no
provider created. Aborting.");
+ }
+
+ return new NullAuthProvider();
+ }
+
+ /**
+ * Convenience helper to convert Map to list of Http Headers
+ * @param headers header map
+ * @return list of http headers
+ */
+ public static List<Header> toRequestHeaders(@Nullable Map<String, Object>
headers) {
+ if (headers == null) {
+ return Collections.emptyList();
+ }
+ return headers.entrySet().stream().filter(entry ->
Objects.nonNull(entry.getValue()))
+ .map(entry -> new BasicHeader(entry.getKey(),
entry.getValue().toString())).collect(Collectors.toList());
Review Comment:
checking null values, stream it, filter and reduce to a list on a per
request basis?
this is for backward compatibility yes? because previously we don't know
what's the content of the headers.
If we only allow the API below (e.g. header extracted from AuthProvider and
requires the AuthProvider plugin to return non null header objects), can we
avoid this check?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
