xiangfu0 commented on code in PR #18975:
URL: https://github.com/apache/pinot/pull/18975#discussion_r4244450345
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/Authorize.java:
##########
@@ -37,6 +37,11 @@
// If the target type is the Pinot cluster, then this field is not required
String paramName() default "";
+ /// Optional path or query parameter that narrows a cluster operation to one
table for coarse access control.
+ /// Use only when the entire operation is limited to that table, rather than
filtering part of the response.
+ /// Omitting the parameter retains cluster-wide scope. Fine-grained
authorization still uses targetType.
+ String tableScopeParam() default "";
Review Comment:
The `tableScopeParam` opt-in is a good design — it makes the "scopes the
operation" vs "filters the response" distinction explicit at the annotation
site instead of relying on a heuristic. The Javadoc here captures that well.
Suggest also stating the fail-closed behavior explicitly: if `tableScopeParam`
names a parameter the method binds as neither `@PathParam` nor a declared
`@QueryParam`, resolution yields `null` and the endpoint stays cluster-wide
(denied for table-scoped principals) rather than erroring — so a typo degrades
to deny, not to escalate. That's the safe direction, but it's silent (see the
test-coverage note on `AuthenticationFilterTest`).
##########
pinot-controller/src/test/java/org/apache/pinot/controller/api/access/AuthenticationFilterTest.java:
##########
@@ -179,15 +293,93 @@ public void testReadOnlyEndpointsDeclareReadAccess() {
@Test
public void testSegmentCompletionGetEndpointsDeclareCreateAccess() {
+ MultivaluedMap<String, String> pathParams = new MultivaluedHashMap<>();
+ MultivaluedMap<String, String> queryParams = new MultivaluedHashMap<>();
+ queryParams.putSingle("tableName", "someTable");
for (String methodName : new String[]{"extendBuildTime",
"segmentConsumed", "segmentStoppedConsuming",
"segmentCommitStart", "reduceSegmentSize"}) {
Method method =
Arrays.stream(LLCSegmentCompletionHandlers.class.getDeclaredMethods())
.filter(candidate ->
candidate.getName().equals(methodName)).findFirst().orElseThrow();
assertEquals(_authFilter.extractAccessType(method), AccessType.CREATE);
assertEquals(method.getAnnotation(Authorize.class).action(),
Actions.Cluster.COMMIT_SEGMENT);
+ // These callbacks name no table, so they take the cluster-wide check.
That is why a table-scoped server token
+ // is denied and realtime commit stalls — the consequence documented on
BaseBasicAuthAccessControl.
+ assertNull(AuthenticationFilter.extractTableName(method, pathParams,
queryParams),
+ methodName + " must not resolve a table name");
}
}
+ /// Guards the assumption stated on
[org.apache.pinot.core.auth.FineGrainedAuthUtils#getDeclaredQueryParams]: a
+ /// table target's parameter must be bound as a `@PathParam` or a
method-level `@QueryParam`. An endpoint binding it
+ /// via `@BeanParam` or field injection would resolve no table and hard-fail
with 400 for every caller, admin
+ /// included.
+ @Test
+ public void testTableAuthorizeParamsAreBoundOnTheMethod()
+ throws Exception {
+ List<Class<?>> resourceClasses = controllerResourceClasses();
+ assertTrue(resourceClasses.size() > 20, "expected to discover the
controller resource classes, found "
+ + resourceClasses.size());
+ List<String> violations = new ArrayList<>();
+ for (Class<?> resourceClass : resourceClasses) {
+ for (Method method : resourceClass.getDeclaredMethods()) {
+ Authorize authorize = method.getAnnotation(Authorize.class);
+ if (authorize == null || authorize.targetType() != TargetType.TABLE) {
Review Comment:
Test-coverage gap: `testTableAuthorizeParamsAreBoundOnTheMethod` skips
everything that isn't `TargetType.TABLE`, so it does not validate that a
`TargetType.CLUSTER` endpoint's `tableScopeParam` is actually bound as a
`@PathParam` or declared `@QueryParam`. With the new attribute now on ~12
cluster endpoints, a future typo in `tableScopeParam` (or a rename of the bound
param without updating it) would silently resolve to `null` and make that
endpoint cluster-wide again — the exact escalation this PR closes, reintroduced
with no test catching it. It fails closed (deny), so it's not a vulnerability,
but it is a silent correctness trap. Consider extending this guard to also
assert, for `targetType == CLUSTER && !tableScopeParam().isEmpty()`, that
`bindsParameter(method, authorize.tableScopeParam())` holds.
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthPrincipal.java:
##########
@@ -44,12 +47,21 @@ public BasicAuthPrincipal(String name, String token,
Set<String> tables, Set<Str
Set<String> permissions, Map<String, List<String>> rlsFilters) {
_name = name;
_token = token;
- _tables = tables;
- _excludeTables = excludeTables;
+ // Copied because hasUnrestrictedTableAccess() gates cluster-level
authorization off these sets: a caller that
+ // retained and later cleared its own set would silently widen this
principal to unrestricted scope. Copied
+ // permissively rather than with Set.copyOf, which rejects null elements —
a user record stored in ZooKeeper can
+ // carry a null table name, and the broker and server rebuild every
principal on each request, so one malformed
+ // record would otherwise fail authentication for every user.
+ _tables = copyOfNullable(tables);
+ _excludeTables = copyOfNullable(excludeTables);
_permissions = permissions.stream().map(s ->
s.toLowerCase()).collect(Collectors.toSet());
_rlsFilters = rlsFilters;
}
+ private static Set<String> copyOfNullable(@Nullable Set<String> values) {
Review Comment:
Good defensive copy now that `hasUnrestrictedTableAccess()` authorizes off
these sets. The choice of `new HashSet<>()` over `Set.copyOf()` to tolerate a
null element from a malformed ZK user record is correct and well-commented
(`Set.copyOf` would NPE and break auth for every principal on one bad record,
since principals are rebuilt per request). Worth a targeted unit test
constructing a principal with a null table entry to lock in that this path
doesn't throw — it's the specific failure mode the comment guards against, and
nothing currently exercises it.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AuthenticationFilter.java:
##########
@@ -140,35 +145,55 @@ AccessType extractAccessType(Method endpointMethod) {
return AccessType.READ;
}
+ /// Resolves the table `endpointMethod` acts on, or `null` when the request
addresses the cluster rather than a
+ /// table. `AccessControlUtils.validatePermission` picks the table-scoped or
the cluster-wide check on that answer,
+ /// so it must not be steerable by the caller.
+ ///
+ /// Path parameters are template variables of the endpoint's own `@Path`,
hence part of its declaration. Query
+ /// parameters are caller-supplied and JAX-RS surfaces every one present on
the URI, so only those the endpoint
+ /// declares may name the table: otherwise a caller could append
`?tableName=<a table it is scoped to>` to a cluster
+ /// endpoint and have it authorized as a table-scoped request.
+ ///
+ @Nullable
@VisibleForTesting
static String extractTableName(Method endpointMethod, MultivaluedMap<String,
String> pathParameters,
MultivaluedMap<String, String> queryParameters) {
Authorize authorize = endpointMethod.getAnnotation(Authorize.class);
if (authorize != null && authorize.targetType() == TargetType.TABLE) {
- return FineGrainedAuthUtils.findRawTargetId(authorize, pathParameters,
queryParameters);
+ // Same primitive as FineGrainedAuthUtils.validateFineGrainedAuth: path
first, then a declared query param.
+ return FineGrainedAuthUtils.findRawTargetId(authorize, endpointMethod,
pathParameters, queryParameters);
+ }
+ if (authorize != null && authorize.targetType() == TargetType.CLUSTER) {
+ String parameter = authorize.tableScopeParam();
Review Comment:
Control flow here is correct and worth confirming for other reviewers: the
CLUSTER branch resolves only from `tableScopeParam` (path first, then declared
query param) and `return`s, so the generic `TABLE_NAME_KEYS` query-param loop
below is unreachable for any `@Authorize`-annotated method — it now only
applies to unannotated endpoints. Path-param trust is fine (it's a variable of
the endpoint's own `@Path`). The one thing not enforced anywhere is that
`tableScopeParam` is actually a bound param; see the guard-test note.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotHelixTaskResourceManager.java:
##########
@@ -1294,11 +1294,12 @@ private synchronized TaskDebugInfo
getTaskDebugInfo(WorkflowContext workflowCont
String taskIdForPartition =
jobContext.getTaskIdForPartition(partition);
TaskPartitionState partitionState =
jobContext.getPartitionState(partition);
TaskConfig helixTaskConfig =
jobConfig.getTaskConfig(taskIdForPartition);
- PinotTaskConfig pinotTaskConfig = null;
- if (helixTaskConfig != null) {
- pinotTaskConfig =
PinotTaskConfig.fromHelixTaskConfig(helixTaskConfig);
- if ((tableNameWithType != null) &&
(!tableNameWithType.equals(pinotTaskConfig.getTableName()))) {
- // Filter task configs that match this table name
+ PinotTaskConfig pinotTaskConfig =
+ helixTaskConfig != null ?
PinotTaskConfig.fromHelixTaskConfig(helixTaskConfig) : null;
+ if (tableNameWithType != null) {
+ // A table filter was requested, so keep only subtasks confirmably
belonging to that table. A missing Helix
Review Comment:
This is a real authorization-boundary fix, not just a refactor: a subtask
whose Helix config is missing was previously *kept* under a table filter; now
it's dropped (fail closed). Since this filter is what makes
`getTaskDebugInfo`'s `tableName` a response filter rather than an auth scope,
leaking an unresolvable-table subtask would disclose another table's subtask to
a table-scoped caller. Correct. Please call this out explicitly in the PR
description — it's easy to miss as a ternary refactor.
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthPrincipal.java:
##########
@@ -62,6 +74,21 @@ public boolean hasTable(String tableName) {
return isTableIncluded(tableName) && isTableNotExcluded(tableName);
}
+ /// Returns whether this principal is scoped to every table, i.e. it carries
neither an allow-list nor an
+ /// exclude-list. Access control implementations use this to decide requests
that name no table, so the return value
+ /// must satisfy: `true` implies [#hasTable(String)] holds for every table
name. A subclass that narrows table scope
+ /// by any other means — in particular by overriding [#hasTable(String)] —
must override this method to match, or it
+ /// will report unrestricted scope while denying individual tables.
+ ///
+ /// Table lists are the signal rather than [ZkBasicAuthPrincipal]'s
`RoleType`. The static factory has no role, both
+ /// controller factories share this check, and an ADMIN that also carries an
allow-list is still scoped. Granting
+ /// cluster access by role would let that principal reach cluster state
outside the list. The bootstrapped admin
+ /// is bootstrapped with a null `UserConfig#getTables()`, which normalizes
to an empty set here, so
+ /// `RoleType.ADMIN` without a table list still passes.
+ public boolean hasUnrestrictedTableAccess() {
Review Comment:
Note this revision dropped the `final` that an earlier iteration had on
`hasTable` and `hasUnrestrictedTableAccess`. The Javadoc still states the
contract "a subclass that overrides `hasTable` must override this to match,"
but without `final` nothing prevents a subclass from overriding `hasTable` and
forgetting this method — which would report unrestricted scope while denying
individual tables (a cluster-access escalation). The only current subclass
(`ZkBasicAuthPrincipal`) doesn't override `hasTable`, so it's correct today.
Consider restoring `final` on both (or at least on
`hasUnrestrictedTableAccess`) to enforce the invariant the Javadoc only
documents.
--
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]
