yashmayya commented on code in PR #19233:
URL: https://github.com/apache/pinot/pull/19233#discussion_r3786536508
##########
pinot-controller/src/main/java/org/apache/pinot/controller/util/ServerSegmentMetadataReader.java:
##########
@@ -68,15 +71,23 @@ public class ServerSegmentMetadataReader {
private final Executor _executor;
private final HttpClientConnectionManager _connectionManager;
+ @Nullable
+ private final AuthProvider _authProvider;
public ServerSegmentMetadataReader() {
- _executor =
Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors());
- _connectionManager = new PoolingHttpClientConnectionManager();
+
this(Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors()),
+ new PoolingHttpClientConnectionManager(), null);
}
public ServerSegmentMetadataReader(Executor executor,
HttpClientConnectionManager connectionManager) {
+ this(executor, connectionManager, null);
+ }
Review Comment:
This overload leaves `_authProvider` null, and two callers still use it:
`UpsertCompactionTaskGenerator` and `UpsertCompactMergeTaskGenerator`. Both
call `getSegmentToValidDocIdsMetadataFromServer`, which posts to
`/tables/{table}/validDocIdsMetadata` — a privileged route. So upsert
compaction stops being scheduled once a Server access-control factory is
configured.
The README files this under the Minion boundary and points operators at a
proxy or service mesh. But these two generators run on the Controller, not on
Minion, and
`_clusterInfoAccessor.getPinotHelixResourceManager().getServerAdminAuthProvider()`
is available at both call sites. Can we pass it here instead?
##########
pinot-core/src/main/java/org/apache/pinot/server/access/ZkBasicAuthAccessFactory.java:
##########
@@ -81,38 +90,79 @@ public boolean isAuthorizedChannel(ChannelHandlerContext
channelHandlerContext)
return true;
}
+ @Override
+ public AuthorizationResult authorizeAdminAccess(RequesterIdentity
requesterIdentity) {
+ Optional<ZkBasicAuthPrincipal> principal =
getPrincipal(requesterIdentity);
+ if (!principal.isPresent()) {
+ throw new NotAuthorizedException("Basic");
+ }
+ return new BasicAuthorizationResultImpl(
+ principal.get().hasPermission(RoleType.ADMIN, ComponentType.SERVER));
+ }
+
@Override
public boolean hasDataAccess(RequesterIdentity requesterIdentity, String
tableName) {
+ return getPrincipal(requesterIdentity)
+ .map(principal -> StringUtils.isEmpty(tableName) ||
principal.hasTable(
+ TableNameBuilder.extractRawTableName(tableName)))
+ .orElse(false);
+ }
+
+ private Optional<ZkBasicAuthPrincipal> getPrincipal(RequesterIdentity
requesterIdentity) {
+ Collection<String> tokens = getTokens(requesterIdentity);
+ if (tokens.isEmpty()) {
+ return Optional.empty();
+ }
+
+ Map<String, String> name2password = new LinkedHashMap<>();
+ for (String token : tokens) {
+ String decodedToken;
+ try {
+ decodedToken = BasicAuthTokenUtils.decodeBasicAuthToken(token);
+ } catch (IllegalArgumentException e) {
+ continue;
+ }
+ int separatorIndex = decodedToken != null ? decodedToken.indexOf(':')
: -1;
+ if (separatorIndex <= 0 || separatorIndex == decodedToken.length() -
1) {
+ continue;
+ }
+ String username = decodedToken.substring(0, separatorIndex);
+ String password = decodedToken.substring(separatorIndex + 1);
+ name2password.put(username, password);
+ }
+ if (name2password.isEmpty()) {
+ return Optional.empty();
+ }
+
if (_userCache == null) {
initUserCache();
}
- Collection<String> tokens = getTokens(requesterIdentity);
- _name2principal =
+ Map<String, ZkBasicAuthPrincipal> name2principal =
BasicAuthPrincipalUtils.extractBasicAuthPrincipals(_userCache.getAllServerUserConfig()).stream()
.collect(Collectors.toMap(ZkBasicAuthPrincipal::getName, p ->
p));
Review Comment:
Minor: this rebuilds the full principal map on every call, and
`extractBasicAuthPrincipals` base64-encodes a token per user each time. Before
this change it only ran on the two table-data routes; now it runs on every
privileged request, including the Controller's periodic fan-outs. Worth caching
it against the user cache.
##########
pinot-server/src/main/java/org/apache/pinot/server/api/ServerAdminApiAccessControlFilter.java:
##########
@@ -0,0 +1,83 @@
+/**
+ * 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.server.api;
+
+import java.io.IOException;
+import java.lang.reflect.Method;
+import javax.annotation.Priority;
+import javax.inject.Inject;
+import javax.ws.rs.Priorities;
+import javax.ws.rs.WebApplicationException;
+import javax.ws.rs.container.ContainerRequestContext;
+import javax.ws.rs.container.ContainerRequestFilter;
+import javax.ws.rs.container.ResourceInfo;
+import javax.ws.rs.core.Context;
+import javax.ws.rs.core.HttpHeaders;
+import javax.ws.rs.core.MediaType;
+import javax.ws.rs.core.Response;
+import org.apache.pinot.server.access.AccessControlFactory;
+import org.apache.pinot.server.access.HttpRequesterIdentity;
+import org.apache.pinot.server.api.resources.HealthCheckResource;
+import org.apache.pinot.server.api.resources.TablesResource;
+import org.apache.pinot.spi.auth.AuthorizationResult;
+
+
+/// Enforces the authorization boundary for the Server administrative API.
+///
+/// Only the Server health endpoints are public. Methods marked with
[ServerDataAccess] retain their explicit table-data
+/// access checks. Every other Server route, including custom resource routes,
requires administrative authorization
+/// from the configured Server access control.
+@Priority(Priorities.AUTHENTICATION)
+public class ServerAdminApiAccessControlFilter implements
ContainerRequestFilter {
+ private static final String GET = "GET";
+
+ @Inject
+ private AccessControlFactory _accessControlFactory;
+
+ @Context
+ private ResourceInfo _resourceInfo;
+
+ @Context
+ private HttpHeaders _httpHeaders;
+
+ @Override
+ public void filter(ContainerRequestContext requestContext)
+ throws IOException {
+ Method endpointMethod = _resourceInfo.getResourceMethod();
+ Object originalRequestMethod =
requestContext.getProperty(ServerRequestMethodCaptureFilter.REQUEST_METHOD_PROPERTY);
+ String requestMethod = originalRequestMethod instanceof String ? (String)
originalRequestMethod
+ : requestContext.getMethod();
+ if (GET.equals(requestMethod) &&
HealthCheckResource.class.equals(_resourceInfo.getResourceClass())
Review Comment:
HEAD on the three health paths now needs admin credentials.
`AccessControlTest` and the filter test both pin this, so I read it as
deliberate — but is it wanted? HEAD on these methods returns nothing that GET
does not, and some load balancers probe with HEAD.
If HEAD is allowed too, the original method stops mattering here: Jersey has
already remapped HEAD to GET by the time this post-matching filter runs, so the
class and annotation checks are enough on their own and
`ServerRequestMethodCaptureFilter` can go away.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
