You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Vamsi-klu (via GitHub)" <gi...@apache.org> on 2026/07/11 21:39:38 UTC
[PR] Restrict BasicAuth cluster access to unrestricted users [pinot]
Vamsi-klu opened a new pull request, #18975:
URL: https://github.com/apache/pinot/pull/18975
Fixes #14595
## What changed
This PR tightens BasicAuth access checks so table-scoped users do not automatically receive cluster-level access.
`BasicAuthPrincipal` now exposes whether a principal has unrestricted table access. The static and ZooKeeper-backed BasicAuth access-control factories use that information when evaluating non-table and cluster targets:
- Table checks still evaluate the requested table name and action permission.
- Cluster/non-table checks now require unrestricted table scope plus the requested action permission.
- Fine-grained action checks are wired through the same scope rules for both static and ZK BasicAuth factories.
- Existing coarse permissions remain compatible by mapping common fine-grained action names back to coarse `READ`, `CREATE`, `DELETE`, or `UPDATE` permissions when an exact action string is not configured.
## Why it matters
A user who is scoped to one or more specific tables should not be treated as authorized for cluster-wide operations just because they have a valid BasicAuth principal. Cluster endpoints can expose or mutate state outside the user's table scope, so the authorization check must distinguish table-level access from unrestricted/cluster-level access.
This keeps table-scoped BasicAuth useful for table operations while closing the privilege boundary for cluster and other non-table targets.
The coarse-permission compatibility mapping matters because existing BasicAuth configs commonly use broad actions such as `READ` and `UPDATE`. Requiring every deployment to switch immediately to exact fine-grained action names would make the security fix unnecessarily disruptive.
## Impact
Table-scoped BasicAuth users can continue accessing their allowed tables.
Table-scoped users are denied cluster-level and non-table operations unless they also have unrestricted table scope.
Unrestricted principals with the required action permission can still access cluster/non-table targets.
Both static BasicAuth configuration and ZooKeeper-backed BasicAuth configuration follow the same scope and action rules.
## Testing
- `./mvnw -Dmaven.repo.local=/tmp/m2-pinot-14595 -pl pinot-core,pinot-controller -am -Dtest=BasicAuthTest,BasicAuthAccessControlFactoryTest -Dsurefire.failIfNoSpecifiedTests=false test`
- `./mvnw -Dmaven.repo.local=/tmp/m2-pinot-14595 -pl pinot-core,pinot-controller spotless:apply checkstyle:check license:check`
- `git diff --check`
The tests cover static and ZK-backed BasicAuth behavior, table-scoped users on allowed tables, denial of cluster access for table-scoped users, denial for excluded-table principals, fine-grained table actions, and cluster updates for unrestricted admin principals.
---
Drafted-by: Codex (GPT-5) (no human review before posting)
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [controller] Require unrestricted table scope for cluster-level BasicAuth access [pinot]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on code in PR #18975:
URL: https://github.com/apache/pinot/pull/18975#discussion_r3817439217
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/BaseBasicAuthAccessControl.java:
##########
@@ -48,22 +55,36 @@ public final boolean hasAccess(String tableName, AccessType accessType, HttpHead
&& authenticatedPrincipal.hasPermission(Objects.toString(accessType));
}
+ /// Guards endpoints that name no table. Such a request is cluster-wide, so beyond the requested permission it
+ /// requires a principal whose table scope is unrestricted: a principal confined to a subset of tables must not reach
+ /// cluster state that lies outside that subset.
@Override
public final boolean hasAccess(AccessType accessType, HttpHeaders httpHeaders, String endpointUrl) {
Optional<P> principal = getPrincipal(httpHeaders);
if (principal.isEmpty()) {
throw new NotAuthorizedException("Basic");
}
- return principal.get().hasPermission(Objects.toString(accessType));
+ P authenticatedPrincipal = principal.get();
+ return authenticatedPrincipal.hasUnrestrictedTableAccess()
+ && authenticatedPrincipal.hasPermission(Objects.toString(accessType));
Review Comment:
Yeah, that one is a data-path break, not a UI annoyance. I added LLCSegmentCompletionHandlers and the minion callbacks that carry no tableName to the Backward-incompatible list, next to POST /segments. Helm now says the same thing: if [pinot.server.segment.uploader.auth.token](https://github.com/apache/pinot/pull/18975#) (or the controller-facing token) is a table-scoped principal, move it to an unrestricted one before you roll controllers, or commit stalls with 403.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [controller] Require unrestricted table scope for cluster-level BasicAuth access [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #18975:
URL: https://github.com/apache/pinot/pull/18975#discussion_r4244765343
##########
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:
Expanded the PR description to state the behavior change explicitly: table-filtered debug results drop subtasks with missing Helix config or unresolved table ownership instead of retaining them. Task-level debug metadata remains cluster-wide; this response filter does not grant table-scoped access to the task-name debug endpoint.
Addressed in `6b0ece6f8c`. Validation: 31 focused tests passed, plus affected-module formatting, Checkstyle, and license checks.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Restrict BasicAuth cluster access to unrestricted users [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #18975:
URL: https://github.com/apache/pinot/pull/18975#discussion_r3762694038
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControlUtils.java:
##########
@@ -74,6 +75,36 @@ public static void validatePermission(@Nullable String tableName, AccessType acc
Response.Status.FORBIDDEN);
}
+ /// Maps a fine grained action name (see [Actions]) to the coarse [AccessType] bucket that guards the corresponding
+ /// endpoint, so that access control implementations backed by coarse permissions can authorize fine grained actions.
+ /// Action names follow a `<verb><noun>` convention, hence the verb prefix selects the bucket.
+ ///
+ /// Actions whose verb does not match the access type of their endpoint are listed as exact matches, which are
+ /// evaluated before the prefixes. Keep new actions in sync with the `@Authenticate` annotation of the endpoint they
+ /// protect, otherwise a principal configured with only that endpoint's coarse permission is denied.
Review Comment:
This derivation does not match the existing endpoint annotations. Some actions are shared across different coarse types: `REBALANCE_TENANT_TABLES` guards both DELETE (`PinotTenantRestletResource:703-704`) and UPDATE (`:739-740`), and `EXECUTE_TASK` guards CREATE (`PinotTaskRestletResource:870-871`) and UPDATE (`PinotControllerPeriodicTaskRestletResource:113-114`). Other actions are directly misclassified: `CREATE_TASK` protects an UPDATE endpoint (`PinotTaskRestletResource:818-820`), while `FORCE_RELEASE_TASK_GENERATION_LOCK` protects DELETE (`:1003-1008`) but falls through to UPDATE. Consequently, principals with the endpoint's configured coarse permission pass the first auth check and are then incorrectly denied here. Use access-type-specific action constants (or pass the endpoint's `AccessType` through) and cover these cases for both factories.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [controller] Require unrestricted table scope for cluster-level BasicAuth access [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #18975:
URL: https://github.com/apache/pinot/pull/18975#discussion_r4244764417
##########
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:
Documented the fail-closed behavior on tableScopeParam: missing input, or a name absent from the endpoint path parameters and declared query parameters, resolves no table and retains cluster-wide authorization. A table-scoped principal is denied.
Addressed in `6b0ece6f8c`. Validation: 31 focused tests passed, plus affected-module formatting, Checkstyle, and license checks.
##########
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:
Extended the existing resource binding guard to validate non-empty tableScopeParam declarations on CLUSTER endpoints as well as paramName on TABLE endpoints. The renamed testAuthorizeTableParamsAreBoundOnTheMethod scans the controller resources and reports unbound table parameters.
Addressed in `6b0ece6f8c`. Validation: 31 focused tests passed, plus affected-module formatting, Checkstyle, and license checks.
##########
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:
Confirmed the CLUSTER branch remains authoritative and returns before the generic table-name heuristic. The extended resource binding guard now verifies that every explicit cluster tableScopeParam is bound by the method, covering the missing enforcement noted here.
Addressed in `6b0ece6f8c`. Validation: 31 focused tests passed, plus affected-module formatting, Checkstyle, and license checks.
##########
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:
Keeping both methods overridable to preserve compatibility with existing BasicAuthPrincipal subclasses. Making the previously overridable hasTable method final would break those subclasses; making only hasUnrestrictedTableAccess final would not enforce the invariant and would prevent custom restrictions from being reflected in the cluster check. The Javadoc already requires subclasses narrowing hasTable to override hasUnrestrictedTableAccess, and the PR migration notes now make that requirement explicit. The in-tree ZkBasicAuthPrincipal does not override hasTable and follows the same table-list policy.
Addressed in `6b0ece6f8c`. Validation: 31 focused tests passed, plus affected-module formatting, Checkstyle, and license checks.
##########
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:
Added testBasicAuthPrincipalWithNullTableEntry for allow-lists and exclude-lists containing both a valid table and null. It verifies construction succeeds, valid table authorization behaves correctly, and the malformed entry never makes the principal unrestricted.
Addressed in `6b0ece6f8c`. Validation: 31 focused tests passed, plus affected-module formatting, Checkstyle, and license checks.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [controller] Require unrestricted table scope for cluster-level BasicAuth access [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #18975:
URL: https://github.com/apache/pinot/pull/18975#discussion_r4244880235
##########
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:
(Carried over, optional.) `hasTable` and `hasUnrestrictedTableAccess` are still non-`final`, so the invariant the Javadoc states — a subclass overriding `hasTable` must also override this, or it reports unrestricted scope while denying individual tables (a cluster-access escalation) — is documented but not enforced. Correct today since the only subclass (`ZkBasicAuthPrincipal`) doesn't override `hasTable`. Restoring `final` on both would make the compiler enforce it rather than relying on the next subclass author reading the contract. Not blocking.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [controller] Require unrestricted table scope for cluster-level BasicAuth access [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #18975:
URL: https://github.com/apache/pinot/pull/18975#discussion_r3817340731
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/BaseBasicAuthAccessControl.java:
##########
@@ -48,22 +55,36 @@ public final boolean hasAccess(String tableName, AccessType accessType, HttpHead
&& authenticatedPrincipal.hasPermission(Objects.toString(accessType));
}
+ /// Guards endpoints that name no table. Such a request is cluster-wide, so beyond the requested permission it
+ /// requires a principal whose table scope is unrestricted: a principal confined to a subset of tables must not reach
+ /// cluster state that lies outside that subset.
@Override
public final boolean hasAccess(AccessType accessType, HttpHeaders httpHeaders, String endpointUrl) {
Optional<P> principal = getPrincipal(httpHeaders);
if (principal.isEmpty()) {
throw new NotAuthorizedException("Basic");
}
- return principal.get().hasPermission(Objects.toString(accessType));
+ P authenticatedPrincipal = principal.get();
+ return authenticatedPrincipal.hasUnrestrictedTableAccess()
+ && authenticatedPrincipal.hasPermission(Objects.toString(accessType));
Review Comment:
**Back-compat case missing from the description: realtime segment completion.**
The `LLCSegmentCompletionHandlers` endpoints — `/segmentConsumed`, `/segmentCommitStart`, `/segmentCommitEndWithMetadata`, `/segmentUpload`, `/segmentStoppedConsuming` — are all `@Authorize(targetType = TargetType.CLUSTER)` and declare no table-identifying query parameter (their params are `instanceId` / `segmentName` / offsets). So they resolve to a `null` table name and land here.
On master a server principal scoped to the tables that server hosts, holding CREATE, passes these. After this change it gets 403 and realtime ingestion stalls at commit time.
That is a data-path break rather than a UI one, so it belongs in the **Backward-incompatible** list next to `POST /segments` — anyone who set `pinot.server.segment.uploader.auth.token` (or the controller-facing auth token) to a table-scoped principal has to move it to an unrestricted one *before* rolling controllers.
Same reasoning applies to minion callbacks on cluster-targeted task endpoints that carry no `tableName`.
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthPrincipal.java:
##########
@@ -62,6 +62,15 @@ 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.
+ public boolean hasUnrestrictedTableAccess() {
+ return _tables.isEmpty() && _excludeTables.isEmpty();
+ }
+
Review Comment:
The contract and the subclass warning are good.
Question on the choice of signal for the ZK-backed path: `ZkBasicAuthPrincipal` carries an explicit `RoleType` (ADMIN/USER), and the server-side `ZkBasicAuthAccessFactory` already authorizes off it via `hasPermission(RoleType.ADMIN, ComponentType.SERVER)`. Keying the controller's cluster gate purely off table scope means a ZK user with `RoleType.ADMIN` **and** a table allow-list loses `/users` and every other cluster endpoint.
The bootstrapped admin from `initUserACLConfig` has `tables=null`, so the default deployment is fine — but if the intent is "admins run cluster endpoints", `RoleType` looks like the more direct signal for the ZK factory. Worth a sentence in the PR on why table scope was chosen over role, since the two can disagree.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/BaseBasicAuthAccessControl.java:
##########
@@ -36,8 +37,14 @@ public final boolean protectAnnotatedOnly() {
}
@Override
- public final boolean hasAccess(String tableName, AccessType accessType, HttpHeaders httpHeaders,
+ public final boolean hasAccess(@Nullable String tableName, AccessType accessType, HttpHeaders httpHeaders,
String endpointUrl) {
+ // A null table name means the request named no table, which makes it cluster-wide however the caller reached this
+ // overload. Route it to the cluster check so the scope rule cannot be sidestepped by omitting the table: callers
+ // include AccessControl's own default cluster overload and the /auth/verify probe, whose table name is optional.
+ if (tableName == null) {
+ return hasAccess(accessType, httpHeaders, endpointUrl);
+ }
Review Comment:
Routing `null` here is right, and I confirmed it is load-bearing — it is what stops the table overload being used to sidestep the scope rule.
One consequence worth adding to the back-compat list: the deprecated `GET /auth/verify` probe passes its optional `tableName` straight into this overload (`PinotControllerAuthResource#verify`). Called without `tableName` it now runs the cluster check, so a table-scoped principal gets `false` where it previously got `true`. The UI is on `v2` so the login gate is unaffected, but third-party tooling still polling the v1 probe will read that as a credential failure.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AuthenticationFilter.java:
##########
@@ -130,35 +142,70 @@ 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);
+ // The annotation names the parameter, but it is only trustworthy where the endpoint also binds it.
+ MultivaluedMap<String, String> trustedQueryParameters =
+ declaredQueryParams(endpointMethod).contains(authorize.paramName()) ? queryParameters
+ : new MultivaluedHashMap<>();
+ return FineGrainedAuthUtils.findRawTargetId(authorize, pathParameters, trustedQueryParameters);
Review Comment:
The new invariant — *only parameters the endpoint declares may name the table* — is applied here on the coarse path, but `FineGrainedAuthUtils.validateFineGrainedAuth` still resolves the same `@Authorize#paramName` from the **full** query map (`findRawTargetId(auth, uriInfo.getPathParameters(), uriInfo.getQueryParameters())`), and that is the copy the broker uses too.
For BasicAuth this is inert, since `hasAccess(HttpHeaders, TargetType, targetId, action)` only checks authentication. But a custom `FineGrainedAccessControl` now receives a caller-steerable `targetId` that can disagree with the coarse decision the filter just made a few lines earlier — the coarse check treats the request as cluster-wide while the fine-grained check authorizes it against a table the caller picked.
Since this PR establishes the invariant, threading the declared-parameter set into `findRawTargetId` (or filtering before the call, as done here) would keep the two resolutions from diverging. Same primitive, left open on the other side.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AuthenticationFilter.java:
##########
@@ -130,35 +142,70 @@ 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);
+ // The annotation names the parameter, but it is only trustworthy where the endpoint also binds it.
+ MultivaluedMap<String, String> trustedQueryParameters =
+ declaredQueryParams(endpointMethod).contains(authorize.paramName()) ? queryParameters
+ : new MultivaluedHashMap<>();
Review Comment:
Nit: this allocates a `MultivaluedHashMap` per request on what is actually the *common* table-endpoint path — most table endpoints bind the `@Authorize` `paramName` as a `@PathParam`, not a `@QueryParam` (e.g. `GET /tables/{tableName}`), so `declaredQueryParams(...)` does not contain it and the empty map is built every time.
Slightly at odds with memoizing `getParameterAnnotations` two methods down for exactly this "runs on every request before authentication" reason. Inlining the two-step lookup avoids the allocation entirely and reads no worse:
```java
String targetId = pathParameters.getFirst(authorize.paramName());
if (targetId == null && declaredQueryParams(endpointMethod).contains(authorize.paramName())) {
targetId = queryParameters.getFirst(authorize.paramName());
}
return targetId;
```
(Or hoist a shared immutable empty `MultivaluedMap` constant, since `findParam` only reads it.)
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AuthenticationFilter.java:
##########
@@ -130,35 +142,70 @@ 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);
+ // The annotation names the parameter, but it is only trustworthy where the endpoint also binds it.
+ MultivaluedMap<String, String> trustedQueryParameters =
+ declaredQueryParams(endpointMethod).contains(authorize.paramName()) ? queryParameters
+ : new MultivaluedHashMap<>();
+ return FineGrainedAuthUtils.findRawTargetId(authorize, pathParameters, trustedQueryParameters);
}
- return extractTableName(pathParameters, queryParameters);
- }
-
- @VisibleForTesting
- static String extractTableName(MultivaluedMap<String, String> pathParameters,
- MultivaluedMap<String, String> queryParameters) {
String tableName = extractTableName(pathParameters);
if (tableName != null) {
return tableName;
}
- return extractTableName(queryParameters);
+ Set<String> declaredQueryParams = declaredQueryParams(endpointMethod);
+ for (String key : TABLE_NAME_KEYS) {
+ if (queryParameters.containsKey(key) && declaredQueryParams.contains(key)) {
+ return queryParameters.getFirst(key);
+ }
+ }
+ return null;
Review Comment:
This closes the reported bypass — I verified it is non-vacuous by reverting the `declaredQueryParams.contains(key)` conjunct and re-running: `ControllerClusterBasicAuthAuthorizationTest` fails with `expected [403] but found [200]` on `/cluster/configs?tableName=allowedTable`, in both the static and ZK factories.
One caveat on the rule itself, worth a note rather than a change: *declared* is treated as *authoritative for scope*, which assumes every endpoint that binds `tableName` uses it to scope the operation. `GET /tasks/task/{taskName}/debug` declares `@QueryParam("tableName")`, but it only filters **subtask** details — `PinotHelixTaskResourceManager#getTaskDebugInfo` still returns task-level state, start/finish times, `triggeredBy` and subtask counts for the named task whatever table it belongs to. So a principal scoped to table A can still read task-level metadata for a task on table B via `?tableName=A`.
Pre-existing (master trusts the parameter unconditionally, so this is not a regression), but the PR's framing implies declared parameters are safe and this one is not quite. A sentence in the description would set expectations for whoever adds the next cluster endpoint with an optional `tableName`.
##########
helm/pinot/values.yaml:
##########
@@ -82,6 +82,10 @@ pinotAuth:
# - access.control.principals.user.password=secret
# - access.control.principals.user.tables=baseballStats,otherstuff
# - access.control.principals.user.permissions=READ
+ # Setting `tables` (or `excludeTables`) scopes a principal to those tables and, on the controller, withholds every
+ # endpoint that names no table — cluster configs, instances, users, and cluster-wide listings such as GET /tables.
+ # `admin` above is unscoped and keeps that access. Give an operator both: a scoped principal for table work and an
+ # unscoped one for cluster work. Widening `tables` to restore cluster access would also widen table access.
Review Comment:
Good that the migration guidance ships with the change.
Two additions worth making here and in the PR's **Backward-incompatible** section, since both are silent failures rather than UI annoyances:
- servers/minions authenticating to the controller with a **table-scoped** principal lose the `LLCSegmentCompletionHandlers` endpoints (no table parameter → cluster-wide), which stalls realtime segment commit;
- the deprecated `GET /auth/verify` probe answers `false` for a table-scoped principal when called without `tableName`.
Also worth stating explicitly that the unrestricted principal must be granted **before** controllers roll, since during the mixed-version window the same request answers 200 or 403 depending on which controller serves it — that point is in the PR description but not in this chart comment, and the chart is what operators actually read.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AuthenticationFilter.java:
##########
@@ -130,35 +142,70 @@ 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);
+ // The annotation names the parameter, but it is only trustworthy where the endpoint also binds it.
+ MultivaluedMap<String, String> trustedQueryParameters =
+ declaredQueryParams(endpointMethod).contains(authorize.paramName()) ? queryParameters
+ : new MultivaluedHashMap<>();
+ return FineGrainedAuthUtils.findRawTargetId(authorize, pathParameters, trustedQueryParameters);
}
- return extractTableName(pathParameters, queryParameters);
- }
-
- @VisibleForTesting
- static String extractTableName(MultivaluedMap<String, String> pathParameters,
- MultivaluedMap<String, String> queryParameters) {
String tableName = extractTableName(pathParameters);
if (tableName != null) {
return tableName;
}
- return extractTableName(queryParameters);
+ Set<String> declaredQueryParams = declaredQueryParams(endpointMethod);
+ for (String key : TABLE_NAME_KEYS) {
+ if (queryParameters.containsKey(key) && declaredQueryParams.contains(key)) {
+ return queryParameters.getFirst(key);
+ }
+ }
+ return null;
}
- private static String extractTableName(MultivaluedMap<String, String> mmap) {
- if (mmap.containsKey(KEY_TABLE_NAME)) {
- return mmap.getFirst(KEY_TABLE_NAME);
- }
- if (mmap.containsKey(KEY_TABLE_NAME_WITH_TYPE)) {
- return mmap.getFirst(KEY_TABLE_NAME_WITH_TYPE);
+ /// Returns the names `endpointMethod` binds as [QueryParam]s.
+ ///
+ /// Only method-level `@QueryParam` binding is recognized; an endpoint binding parameters through `@BeanParam` or
+ /// resource-class field injection is treated as declaring none. That direction denies table scope rather than
+ /// granting it, so it fails closed — no controller resource uses either form today.
+ ///
+ /// Results are memoized because [Method#getParameterAnnotations()] re-parses the class-file annotation bytes on
+ /// every call, and this runs on every request before authentication. The key set is bounded by the number of
+ /// endpoints.
Review Comment:
I checked this claim and it holds: no controller resource uses `@BeanParam`, and there is no field-injected `@QueryParam` anywhere under `pinot-controller/src/main/java/org/apache/pinot/controller/api/`.
Since the guarantee is "a future endpoint that does will fail closed", a test would pin it — a method with a `@BeanParam` (or no parameter annotations at all) asserting `extractTableName(...)` returns `null` even with `?tableName=` supplied. Cheap, and it keeps the documented direction from silently drifting if someone later widens `findDeclaredQueryParams`.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [controller] Require unrestricted table scope for cluster-level BasicAuth access [pinot]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on code in PR #18975:
URL: https://github.com/apache/pinot/pull/18975#discussion_r3817447600
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AuthenticationFilter.java:
##########
@@ -130,35 +142,70 @@ 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);
+ // The annotation names the parameter, but it is only trustworthy where the endpoint also binds it.
+ MultivaluedMap<String, String> trustedQueryParameters =
+ declaredQueryParams(endpointMethod).contains(authorize.paramName()) ? queryParameters
+ : new MultivaluedHashMap<>();
+ return FineGrainedAuthUtils.findRawTargetId(authorize, pathParameters, trustedQueryParameters);
}
- return extractTableName(pathParameters, queryParameters);
- }
-
- @VisibleForTesting
- static String extractTableName(MultivaluedMap<String, String> pathParameters,
- MultivaluedMap<String, String> queryParameters) {
String tableName = extractTableName(pathParameters);
if (tableName != null) {
return tableName;
}
- return extractTableName(queryParameters);
+ Set<String> declaredQueryParams = declaredQueryParams(endpointMethod);
+ for (String key : TABLE_NAME_KEYS) {
+ if (queryParameters.containsKey(key) && declaredQueryParams.contains(key)) {
+ return queryParameters.getFirst(key);
+ }
+ }
+ return null;
}
- private static String extractTableName(MultivaluedMap<String, String> mmap) {
- if (mmap.containsKey(KEY_TABLE_NAME)) {
- return mmap.getFirst(KEY_TABLE_NAME);
- }
- if (mmap.containsKey(KEY_TABLE_NAME_WITH_TYPE)) {
- return mmap.getFirst(KEY_TABLE_NAME_WITH_TYPE);
+ /// Returns the names `endpointMethod` binds as [QueryParam]s.
+ ///
+ /// Only method-level `@QueryParam` binding is recognized; an endpoint binding parameters through `@BeanParam` or
+ /// resource-class field injection is treated as declaring none. That direction denies table scope rather than
+ /// granting it, so it fails closed — no controller resource uses either form today.
+ ///
+ /// Results are memoized because [Method#getParameterAnnotations()] re-parses the class-file annotation bytes on
+ /// every call, and this runs on every request before authentication. The key set is bounded by the number of
+ /// endpoints.
Review Comment:
Yep, that was the common path. I dropped the empty map. Path param first, then a declared query param, same as you sketched. No allocation on GET /tables/{tableName} and friends.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [controller] Require unrestricted table scope for cluster-level BasicAuth access [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #18975:
URL: https://github.com/apache/pinot/pull/18975
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Restrict BasicAuth cluster access to unrestricted users [pinot]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on code in PR #18975:
URL: https://github.com/apache/pinot/pull/18975#discussion_r3755549903
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/BasicAuthAccessControlFactory.java:
##########
@@ -87,15 +88,58 @@ public boolean hasAccess(String tableName, AccessType accessType, HttpHeaders ht
@Override
public boolean hasAccess(AccessType accessType, HttpHeaders httpHeaders, String endpointUrl) {
- if (getPrincipal(httpHeaders).isEmpty()) {
+ Optional<BasicAuthPrincipal> principal = getPrincipal(httpHeaders);
+ if (principal.isEmpty()) {
throw new NotAuthorizedException("Basic");
}
- return true;
+ return hasClusterAccess(principal.get(), Objects.toString(accessType));
}
@Override
public boolean hasAccess(HttpHeaders httpHeaders, TargetType targetType) {
- return getPrincipal(httpHeaders).isPresent();
+ return getPrincipal(httpHeaders)
+ .filter(p -> targetType == TargetType.TABLE || p.hasUnrestrictedTableAccess())
+ .isPresent();
+ }
+
+ @Override
+ public boolean hasAccess(HttpHeaders httpHeaders, TargetType targetType, String targetId, String action) {
+ Optional<BasicAuthPrincipal> principal = getPrincipal(httpHeaders);
+ if (targetType == TargetType.TABLE) {
+ return principal
+ .filter(p -> p.hasTable(TableNameBuilder.extractRawTableName(targetId)) && hasActionPermission(p, action))
+ .isPresent();
+ }
+ if (targetType == TargetType.CLUSTER) {
+ return principal.filter(p -> hasClusterAccess(p, action)).isPresent();
+ }
+ return false;
+ }
+
+ private static boolean hasClusterAccess(BasicAuthPrincipal principal, String action) {
+ return principal.hasUnrestrictedTableAccess() && hasActionPermission(principal, action);
+ }
+
+ private static boolean hasActionPermission(BasicAuthPrincipal principal, String action) {
+ if (action != null && principal.hasPermission(action)) {
+ return true;
+ }
+ return principal.hasPermission(Objects.toString(getAccessTypeForAction(action)));
+ }
+
+ private static AccessType getAccessTypeForAction(String action) {
+ if (action == null || action.startsWith("Get") || action.startsWith("List") || action.startsWith("Query")
+ || action.startsWith("Debug") || action.startsWith("Estimate") || action.startsWith("Recommend")) {
+ return AccessType.READ;
+ }
+ if (action.startsWith("Create") || action.startsWith("Ingest") || action.startsWith("Commit")
+ || action.startsWith("Upload")) {
+ return AccessType.CREATE;
+ }
+ if (action.startsWith("Delete") || action.startsWith("Cancel")) {
+ return AccessType.DELETE;
+ }
+ return AccessType.UPDATE;
}
Review Comment:
Fixed in `552a472`. Rather than patching the two copies separately I pulled the mapping into a single `AccessControlUtils.getAccessTypeForAction` that both factories call, so they cannot drift apart later.
`Download` maps to READ now, and `CancelRebalance` is an exact match returning UPDATE evaluated before the `Cancel` prefix, which stays DELETE so `CancelQuery` is unaffected.
While auditing the rest of `Actions` against the endpoint annotations I found `Validate` has the same defect you flagged: `ValidateSchema` and `ValidateTableConfigs` are READ endpoints but the prefix fell through to the default UPDATE branch, so that moved to READ as well. Tests cover all of these for both factories, plus null still mapping to READ.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/ZkBasicAuthAccessControlFactory.java:
##########
@@ -90,12 +90,54 @@ public boolean hasAccess(String tableName, AccessType accessType, HttpHeaders ht
@Override
public boolean hasAccess(HttpHeaders httpHeaders, TargetType targetType) {
- return getPrincipal(httpHeaders).isPresent();
+ return getPrincipal(httpHeaders)
+ .filter(p -> targetType == TargetType.TABLE || p.hasUnrestrictedTableAccess())
+ .isPresent();
}
@Override
public boolean hasAccess(AccessType accessType, HttpHeaders httpHeaders, String endpointUrl) {
- return getPrincipal(httpHeaders).isPresent();
+ return getPrincipal(httpHeaders).filter(p -> hasClusterAccess(p, Objects.toString(accessType))).isPresent();
+ }
+
+ @Override
+ public boolean hasAccess(HttpHeaders httpHeaders, TargetType targetType, String targetId, String action) {
+ Optional<ZkBasicAuthPrincipal> principal = getPrincipal(httpHeaders);
+ if (targetType == TargetType.TABLE) {
+ return principal
+ .filter(p -> p.hasTable(TableNameBuilder.extractRawTableName(targetId)) && hasActionPermission(p, action))
+ .isPresent();
+ }
+ if (targetType == TargetType.CLUSTER) {
+ return principal.filter(p -> hasClusterAccess(p, action)).isPresent();
+ }
+ return false;
+ }
+
+ private static boolean hasClusterAccess(ZkBasicAuthPrincipal principal, String action) {
+ return principal.hasUnrestrictedTableAccess() && hasActionPermission(principal, action);
+ }
+
+ private static boolean hasActionPermission(ZkBasicAuthPrincipal principal, String action) {
+ if (action != null && principal.hasPermission(action)) {
+ return true;
+ }
+ return principal.hasPermission(Objects.toString(getAccessTypeForAction(action)));
+ }
+
+ private static AccessType getAccessTypeForAction(String action) {
+ if (action == null || action.startsWith("Get") || action.startsWith("List") || action.startsWith("Query")
+ || action.startsWith("Debug") || action.startsWith("Estimate") || action.startsWith("Recommend")) {
+ return AccessType.READ;
+ }
+ if (action.startsWith("Create") || action.startsWith("Ingest") || action.startsWith("Commit")
+ || action.startsWith("Upload")) {
+ return AccessType.CREATE;
+ }
+ if (action.startsWith("Delete") || action.startsWith("Cancel")) {
+ return AccessType.DELETE;
+ }
+ return AccessType.UPDATE;
}
Review Comment:
Same fix, see the reply on the other thread. Both factories now delegate to one shared `AccessControlUtils.getAccessTypeForAction`, so the two copies cannot drift.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [controller] Require unrestricted table scope for cluster-level BasicAuth access [pinot]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on code in PR #18975:
URL: https://github.com/apache/pinot/pull/18975#discussion_r3817447600
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AuthenticationFilter.java:
##########
@@ -130,35 +142,70 @@ 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);
+ // The annotation names the parameter, but it is only trustworthy where the endpoint also binds it.
+ MultivaluedMap<String, String> trustedQueryParameters =
+ declaredQueryParams(endpointMethod).contains(authorize.paramName()) ? queryParameters
+ : new MultivaluedHashMap<>();
+ return FineGrainedAuthUtils.findRawTargetId(authorize, pathParameters, trustedQueryParameters);
}
- return extractTableName(pathParameters, queryParameters);
- }
-
- @VisibleForTesting
- static String extractTableName(MultivaluedMap<String, String> pathParameters,
- MultivaluedMap<String, String> queryParameters) {
String tableName = extractTableName(pathParameters);
if (tableName != null) {
return tableName;
}
- return extractTableName(queryParameters);
+ Set<String> declaredQueryParams = declaredQueryParams(endpointMethod);
+ for (String key : TABLE_NAME_KEYS) {
+ if (queryParameters.containsKey(key) && declaredQueryParams.contains(key)) {
+ return queryParameters.getFirst(key);
+ }
+ }
+ return null;
}
- private static String extractTableName(MultivaluedMap<String, String> mmap) {
- if (mmap.containsKey(KEY_TABLE_NAME)) {
- return mmap.getFirst(KEY_TABLE_NAME);
- }
- if (mmap.containsKey(KEY_TABLE_NAME_WITH_TYPE)) {
- return mmap.getFirst(KEY_TABLE_NAME_WITH_TYPE);
+ /// Returns the names `endpointMethod` binds as [QueryParam]s.
+ ///
+ /// Only method-level `@QueryParam` binding is recognized; an endpoint binding parameters through `@BeanParam` or
+ /// resource-class field injection is treated as declaring none. That direction denies table scope rather than
+ /// granting it, so it fails closed — no controller resource uses either form today.
+ ///
+ /// Results are memoized because [Method#getParameterAnnotations()] re-parses the class-file annotation bytes on
+ /// every call, and this runs on every request before authentication. The key set is bounded by the number of
+ /// endpoints.
Review Comment:
Yep, that was the common path. I dropped the empty map. Path param first, then a declared query param, same as you sketched. No allocation on GET /tables/{tableName} and friends.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [controller] Require unrestricted table scope for cluster-level BasicAuth access [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #18975:
URL: https://github.com/apache/pinot/pull/18975#discussion_r4244914163
##########
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:
Retaining the existing subclass contract for compatibility, as explained in the previous thread: https://github.com/apache/pinot/pull/18975#discussion_r4244764956
Making both methods final would enforce the table-list policy, but hasTable was already overridable before this PR; existing custom principal subclasses overriding it would no longer compile or link. Making only hasUnrestrictedTableAccess final would leave the mismatch possible while preventing a subclass from reflecting its custom restriction in the cluster gate. The method Javadoc and PR migration notes require subclasses narrowing hasTable to override hasUnrestrictedTableAccess as well. The in-tree ZkBasicAuthPrincipal uses the base policy. Keeping that documented extension contract is the chosen tradeoff for this security patch; no further code change for this optional suggestion.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Restrict BasicAuth cluster access to unrestricted users [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #18975:
URL: https://github.com/apache/pinot/pull/18975#issuecomment-5186865070
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/18975?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 56.95%. Comparing base ([`e0210aa`](https://app.codecov.io/gh/apache/pinot/commit/e0210aaf2530fc123aa48f7560174c59d10aaec5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`0464665`](https://app.codecov.io/gh/apache/pinot/commit/0464665000fcb0c52c484b5cb3995ee8bdcb826e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 140 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (e0210aa) and HEAD (0464665). Click for more details.
>
> <details><summary>HEAD has 16 uploads less than BASE</summary>
>
>| Flag | BASE (e0210aa) | HEAD (0464665) |
>|------|------|------|
>|java-21|5|1|
>|unittests|2|1|
>|temurin|5|1|
>|unittests2|1|0|
>|integration|3|0|
>|integration1|1|0|
>|custom-integration1|1|0|
>|integration2|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #18975 +/- ##
============================================
- Coverage 65.06% 56.95% -8.11%
+ Complexity 1403 1 -1402
============================================
Files 3399 2605 -794
Lines 212797 153834 -58963
Branches 33567 25094 -8473
============================================
- Hits 138457 87622 -50835
+ Misses 63204 58733 -4471
+ Partials 11136 7479 -3657
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/18975/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/18975/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/18975/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/18975/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/18975/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/18975/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.95% <100.00%> (-8.11%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/18975/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.95% <100.00%> (-8.11%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/18975/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.95% <100.00%> (-8.11%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/18975/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.95% <100.00%> (-0.05%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/18975/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/pinot/pull/18975?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [controller] Require unrestricted table scope for cluster-level BasicAuth access [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #18975:
URL: https://github.com/apache/pinot/pull/18975#discussion_r3857774724
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/FineGrainedAuthUtils.java:
##########
@@ -92,18 +144,21 @@ public static void validateFineGrainedAuth(Method endpointMethod, UriInfo uriInf
if (auth.targetType() == TargetType.TABLE) {
// paramName is mandatory for table level authorization
if (StringUtils.isEmpty(auth.paramName())) {
+ // TODO: PinotTableRestletResource#copyTable declares @Authorize(TABLE) with no paramName
+ // and already 500s here. Prefer startup-time validation of the resource model (or 403)
+ // so a misannotation is not an error-rate page.
throw new WebApplicationException(
"paramName not found for table level authorization in API: " + uriInfo.getRequestUri(),
Response.Status.INTERNAL_SERVER_ERROR);
}
- // find the paramName in the path or query params
- targetId = findRawTargetId(auth, uriInfo.getPathParameters(), uriInfo.getQueryParameters());
+ // Path params are part of the endpoint declaration. Query params are caller-supplied, so only a name
+ // the method binds as @QueryParam may identify the table. Otherwise a caller could append
+ // ?tableName=<a table it is scoped to> and have a cluster-wide request authorized as table-scoped.
+ targetId = findRawTargetId(auth, endpointMethod, uriInfo.getPathParameters(), uriInfo.getQueryParameters());
if (StringUtils.isEmpty(targetId)) {
- throw new WebApplicationException(
- "Could not find paramName " + auth.paramName() + " in path or query params of the API: "
- + uriInfo.getRequestUri(), Response.Status.INTERNAL_SERVER_ERROR);
+ throw unboundTableParamException(auth.paramName(), uriInfo.getRequestUri());
Review Comment:
**The two misannotation paths now disagree, and I do not think that is intended.**
Both branches below signal the same class of defect — an `@Authorize(TABLE)` endpoint whose table parameter cannot be resolved — but they now answer differently:
- empty `paramName` (line 147) → **500**, with a `// TODO` saying it should be 403 or startup validation;
- `paramName` declared but unresolved (line 160) → **403**.
`PinotTableRestletResource#copyTable` hits the first, so the one in-tree misannotation still produces the error-rate page the change was meant to avoid. Whichever status is right, these two should match; otherwise the next person debugging a misannotated endpoint gets a different signal depending on which way it was misannotated.
Separately, on the change itself: my earlier comment was explicit that I would **not** hold the PR for this and suggested a `// TODO` or a follow-up issue. Turning it into a behavior change inside a security PR widens the blast radius for no security benefit.
Worth being precise about what it affects, because it is not only misannotations. `StringUtils.isEmpty(targetId)` also fires when a caller **omits a properly declared but optional** `tableName` — `/v2/segments` (both overloads), `/segments/batchUpload`, `/segments/reingested`. Those requests already fail on master with 500, so no working client breaks and the risk is genuinely low; but the observable status for a real client path changes, and that is not in the **Backward-incompatible** list.
My recommendation is to drop this commit and file it separately, where the segment-upload status change can be reviewed on its own merits. If you prefer to keep it, it needs the two branches reconciled and a line in the back-compat list.
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/FineGrainedAuthUtils.java:
##########
@@ -33,38 +40,83 @@
import org.slf4j.LoggerFactory;
-/// Utility methods to share in Broker and Controller request filters related to fine grain authorization.
+/// Shared broker and controller helpers for fine-grained authorization.
+///
+/// The broker request filter calls [#validateFineGrainedAuth], so tightening target resolution
+/// here applies to both roles. Every in-tree broker `@Authorize(targetType = TargetType.TABLE)`
+/// endpoint (`PinotBrokerDebug`, `PinotBrokerRouting`) binds `tableName` as a `@PathParam` inside
+/// its own `@Path` template, so the declared-query-param filter does not change broker resolution.
+///
+/// [#findRawTargetId] is public. It previously took 3 arguments (`Authorize`, path map, query
+/// map) and now takes 4 (`Authorize`, `Method`, path map, query map). There is no overload: the
+/// `Method` is required to apply the declared-parameter filter. A plugin calling the old
+/// signature will fail to link.
public class FineGrainedAuthUtils {
private static final Logger LOGGER = LoggerFactory.getLogger(FineGrainedAuthUtils.class);
+ /// Memoizes [#declaredQueryParams(Method)]; bounded by the number of endpoints.
+ private static final Map<Method, Set<String>> DECLARED_QUERY_PARAMS = new ConcurrentHashMap<>();
+
+ /// Status when `@Authorize(TABLE)` names a parameter the endpoint never binds.
+ ///
+ /// Historically [Response.Status#INTERNAL_SERVER_ERROR], which pages on error-rate alerts and
+ /// reads as a controller bug. [Response.Status#FORBIDDEN] is the authorization outcome.
+ /// Restore `INTERNAL_SERVER_ERROR` here — or pass it to [#unboundTableParamException] — to
+ /// revert to the previous status. Tests pin both directions.
+ static final Response.Status UNBOUND_TABLE_PARAM_STATUS = Response.Status.FORBIDDEN;
Review Comment:
This constant, the two-argument/three-argument `unboundTableParamException` pair below, and `testUnboundTableParamExceptionRevertsToInternalServerError` exist only to make a future revert convenient. I would drop all three.
Nothing in production calls the three-argument overload — `validateFineGrainedAuth` calls the two-argument form, which always resolves to `FORBIDDEN`. So the `Response.Status` parameter is unreachable outside the test that exercises it, and the javadoc instruction "Restore `INTERNAL_SERVER_ERROR` here ... Tests pin both directions" is describing a branch the codebase never takes.
Reverting a one-line status change is `git revert`; it does not need a seam carried in the shipped code. Suggest: inline `Response.Status.FORBIDDEN` at the throw site, delete the constant and both helpers, and delete the revert test. The behavior is unchanged and roughly 25 lines of main + test code go away.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControl.java:
##########
@@ -25,6 +25,15 @@
import org.apache.pinot.spi.annotations.InterfaceStability;
+/// Controller access-control SPI.
+///
+/// Custom implementations should audit two resolution changes from apache/pinot#18975:
+/// 1. A table name appended as an undeclared query parameter no longer reaches
+/// [#hasAccess(String, AccessType, HttpHeaders, String)]; the request arrives with a `null`
+/// table name and must be treated as cluster-wide.
+/// 2. [org.apache.pinot.core.auth.FineGrainedAuthUtils#findRawTargetId] changed from 3 arguments
+/// (`Authorize`, path map, query map) to 4 by adding the resource `Method`. There is no
+/// overload; a plugin calling the old signature will fail to link.
Review Comment:
`AccessControl` is `@InterfaceAudience.Public @InterfaceStability.Stable`, so this is permanent, published API documentation — and what it now says is a point-in-time migration note ("audit two resolution changes from apache/pinot#18975", "changed from 3 arguments ... to 4").
That content is right and worth publishing, but it belongs in the release notes and the PR description rather than in the type's javadoc, where it will read as stale by 1.6 and cannot be removed without another API-doc change.
What does belong here is the durable contract, which the per-method javadoc already states well: a `null` table name means the request named no table and must be treated as cluster-wide. I would keep that and move the two numbered migration items out.
The `findRawTargetId` signature note is also on the wrong type — that method lives on `FineGrainedAuthUtils`, and the class javadoc there (added in the previous commit) already covers it.
##########
pinot-core/src/test/java/org/apache/pinot/core/auth/FineGrainedAuthUtilsTest.java:
##########
@@ -36,22 +38,113 @@
public class FineGrainedAuthUtilsTest {
@Test
- public void testFindRawTargetId() throws Exception {
+ public void testFindRawTargetId()
+ throws Exception {
MultivaluedMap<String, String> pathParams = new MultivaluedHashMap<>();
MultivaluedMap<String, String> queryParams = new MultivaluedHashMap<>();
- Authorize tableAuth = TestResource.class.getDeclaredMethod("getTable").getAnnotation(Authorize.class);
+ Method tableMethod = TestResource.class.getDeclaredMethod("getTable");
+ Method tableQueryMethod = TestResource.class.getDeclaredMethod("getTableByQuery", String.class);
+ Authorize tableAuth = tableMethod.getAnnotation(Authorize.class);
+ Authorize tableQueryAuth = tableQueryMethod.getAnnotation(Authorize.class);
Authorize clusterAuth = getAnnotatedMethod().getAnnotation(Authorize.class);
pathParams.putSingle("tableName", "pathTable");
- assertEquals(FineGrainedAuthUtils.findRawTargetId(tableAuth, pathParams, queryParams), "pathTable");
+ assertEquals(FineGrainedAuthUtils.findRawTargetId(tableAuth, tableMethod, pathParams, queryParams), "pathTable");
pathParams.clear();
queryParams.putSingle("tableName", "queryTable");
- assertEquals(FineGrainedAuthUtils.findRawTargetId(tableAuth, pathParams, queryParams), "queryTable");
- assertNull(FineGrainedAuthUtils.findRawTargetId(clusterAuth, pathParams, queryParams));
+ // The annotation names tableName, but getTable never binds it, so the query value is not trusted.
+ assertNull(FineGrainedAuthUtils.findRawTargetId(tableAuth, tableMethod, pathParams, queryParams));
+ assertEquals(FineGrainedAuthUtils.findRawTargetId(tableQueryAuth, tableQueryMethod, pathParams, queryParams),
+ "queryTable");
+ assertNull(FineGrainedAuthUtils.findRawTargetId(clusterAuth, getAnnotatedMethod(), pathParams, queryParams));
queryParams.clear();
- assertNull(FineGrainedAuthUtils.findRawTargetId(tableAuth, pathParams, queryParams));
+ assertNull(FineGrainedAuthUtils.findRawTargetId(tableAuth, tableMethod, pathParams, queryParams));
+ }
+
+ @Test
+ public void testValidateFineGrainedAuthIgnoresUndeclaredTableQueryParam()
+ throws Exception {
+ FineGrainedAccessControl ac = Mockito.mock(FineGrainedAccessControl.class);
+ Mockito.when(ac.hasAccess(Mockito.any(HttpHeaders.class), Mockito.any(), Mockito.any(), Mockito.any()))
+ .thenReturn(true);
+
+ UriInfo mockUriInfo = Mockito.mock(UriInfo.class);
+ MultivaluedMap<String, String> pathParams = new MultivaluedHashMap<>();
+ MultivaluedMap<String, String> queryParams = new MultivaluedHashMap<>();
+ queryParams.putSingle("tableName", "callerPicked");
+ Mockito.when(mockUriInfo.getPathParameters()).thenReturn(pathParams);
+ Mockito.when(mockUriInfo.getQueryParameters()).thenReturn(queryParams);
+ Mockito.when(mockUriInfo.getRequestUri()).thenReturn(URI.create("http://localhost/tables"));
+ HttpHeaders mockHttpHeaders = Mockito.mock(HttpHeaders.class);
+
+ Method unboundTableMethod = TestResource.class.getDeclaredMethod("getTable");
+ try {
+ FineGrainedAuthUtils.validateFineGrainedAuth(unboundTableMethod, mockUriInfo, mockHttpHeaders, ac);
+ Assert.fail("Expected WebApplicationException");
+ } catch (WebApplicationException e) {
+ Assert.assertTrue(e.getMessage().contains("Could not find paramName"));
+ Assert.assertEquals(e.getResponse().getStatus(),
+ FineGrainedAuthUtils.UNBOUND_TABLE_PARAM_STATUS.getStatusCode());
+ Assert.assertEquals(e.getResponse().getStatus(), Response.Status.FORBIDDEN.getStatusCode());
+ }
+ Mockito.verify(ac, Mockito.never())
+ .hasAccess(Mockito.any(HttpHeaders.class), Mockito.any(), Mockito.any(), Mockito.any());
+ }
+
+ @Test
+ public void testUnboundTableParamExceptionRevertsToInternalServerError() {
+ URI requestUri = URI.create("http://localhost/tables");
+ WebApplicationException current = FineGrainedAuthUtils.unboundTableParamException("tableName", requestUri);
+ Assert.assertEquals(current.getResponse().getStatus(), Response.Status.FORBIDDEN.getStatusCode());
+
+ // Revert path: passing the previous status restores the 500 that this case used to pin.
+ WebApplicationException reverted = FineGrainedAuthUtils.unboundTableParamException("tableName", requestUri,
+ Response.Status.INTERNAL_SERVER_ERROR);
+ Assert.assertTrue(reverted.getMessage().contains("Could not find paramName"));
+ Assert.assertEquals(reverted.getResponse().getStatus(), Response.Status.INTERNAL_SERVER_ERROR.getStatusCode());
+ Assert.assertEquals(reverted.getMessage(), current.getMessage());
+ }
Review Comment:
This test asserts that a helper overload with no production caller returns 500 when you hand it 500. It cannot fail for any reason connected to the endpoint behavior it is named after, and it will keep passing if `validateFineGrainedAuth` stops calling the helper entirely.
If the revert seam goes away as suggested on the constant above, this test goes with it. The coverage that matters — an undeclared `?tableName=` is rejected without reaching `hasAccess` — is already pinned by `testValidateFineGrainedAuthIgnoresUndeclaredTableQueryParam`, including the `Mockito.verify(ac, never())`, which is the right assertion.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [controller] Require unrestricted table scope for cluster-level BasicAuth access [pinot]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on code in PR #18975:
URL: https://github.com/apache/pinot/pull/18975#discussion_r3859966243
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/FineGrainedAuthUtils.java:
##########
@@ -92,18 +144,21 @@ public static void validateFineGrainedAuth(Method endpointMethod, UriInfo uriInf
if (auth.targetType() == TargetType.TABLE) {
// paramName is mandatory for table level authorization
if (StringUtils.isEmpty(auth.paramName())) {
+ // TODO: PinotTableRestletResource#copyTable declares @Authorize(TABLE) with no paramName
+ // and already 500s here. Prefer startup-time validation of the resource model (or 403)
+ // so a misannotation is not an error-rate page.
throw new WebApplicationException(
"paramName not found for table level authorization in API: " + uriInfo.getRequestUri(),
Response.Status.INTERNAL_SERVER_ERROR);
}
- // find the paramName in the path or query params
- targetId = findRawTargetId(auth, uriInfo.getPathParameters(), uriInfo.getQueryParameters());
+ // Path params are part of the endpoint declaration. Query params are caller-supplied, so only a name
+ // the method binds as @QueryParam may identify the table. Otherwise a caller could append
+ // ?tableName=<a table it is scoped to> and have a cluster-wide request authorized as table-scoped.
+ targetId = findRawTargetId(auth, endpointMethod, uriInfo.getPathParameters(), uriInfo.getQueryParameters());
if (StringUtils.isEmpty(targetId)) {
- throw new WebApplicationException(
- "Could not find paramName " + auth.paramName() + " in path or query params of the API: "
- + uriInfo.getRequestUri(), Response.Status.INTERNAL_SERVER_ERROR);
+ throw unboundTableParamException(auth.paramName(), uriInfo.getRequestUri());
Review Comment:
Reverted 33fd79729a. Tree matches 159bac2bfe. 500 stays on both unbound and missing table paramName, so the two misannotation paths agree again. Optional tableName uploads keep 500 as well. 403 is follow-up if we still want it.
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/FineGrainedAuthUtils.java:
##########
@@ -33,38 +40,83 @@
import org.slf4j.LoggerFactory;
-/// Utility methods to share in Broker and Controller request filters related to fine grain authorization.
+/// Shared broker and controller helpers for fine-grained authorization.
+///
+/// The broker request filter calls [#validateFineGrainedAuth], so tightening target resolution
+/// here applies to both roles. Every in-tree broker `@Authorize(targetType = TargetType.TABLE)`
+/// endpoint (`PinotBrokerDebug`, `PinotBrokerRouting`) binds `tableName` as a `@PathParam` inside
+/// its own `@Path` template, so the declared-query-param filter does not change broker resolution.
+///
+/// [#findRawTargetId] is public. It previously took 3 arguments (`Authorize`, path map, query
+/// map) and now takes 4 (`Authorize`, `Method`, path map, query map). There is no overload: the
+/// `Method` is required to apply the declared-parameter filter. A plugin calling the old
+/// signature will fail to link.
public class FineGrainedAuthUtils {
private static final Logger LOGGER = LoggerFactory.getLogger(FineGrainedAuthUtils.class);
+ /// Memoizes [#declaredQueryParams(Method)]; bounded by the number of endpoints.
+ private static final Map<Method, Set<String>> DECLARED_QUERY_PARAMS = new ConcurrentHashMap<>();
+
+ /// Status when `@Authorize(TABLE)` names a parameter the endpoint never binds.
+ ///
+ /// Historically [Response.Status#INTERNAL_SERVER_ERROR], which pages on error-rate alerts and
+ /// reads as a controller bug. [Response.Status#FORBIDDEN] is the authorization outcome.
+ /// Restore `INTERNAL_SERVER_ERROR` here — or pass it to [#unboundTableParamException] — to
+ /// revert to the previous status. Tests pin both directions.
+ static final Response.Status UNBOUND_TABLE_PARAM_STATUS = Response.Status.FORBIDDEN;
Review Comment:
Dropped the constant, both unboundTableParamException helpers, and the revert test with that commit. Nothing left to inline as 403. The throw is 500 again.
##########
pinot-core/src/test/java/org/apache/pinot/core/auth/FineGrainedAuthUtilsTest.java:
##########
@@ -36,22 +38,113 @@
public class FineGrainedAuthUtilsTest {
@Test
- public void testFindRawTargetId() throws Exception {
+ public void testFindRawTargetId()
+ throws Exception {
MultivaluedMap<String, String> pathParams = new MultivaluedHashMap<>();
MultivaluedMap<String, String> queryParams = new MultivaluedHashMap<>();
- Authorize tableAuth = TestResource.class.getDeclaredMethod("getTable").getAnnotation(Authorize.class);
+ Method tableMethod = TestResource.class.getDeclaredMethod("getTable");
+ Method tableQueryMethod = TestResource.class.getDeclaredMethod("getTableByQuery", String.class);
+ Authorize tableAuth = tableMethod.getAnnotation(Authorize.class);
+ Authorize tableQueryAuth = tableQueryMethod.getAnnotation(Authorize.class);
Authorize clusterAuth = getAnnotatedMethod().getAnnotation(Authorize.class);
pathParams.putSingle("tableName", "pathTable");
- assertEquals(FineGrainedAuthUtils.findRawTargetId(tableAuth, pathParams, queryParams), "pathTable");
+ assertEquals(FineGrainedAuthUtils.findRawTargetId(tableAuth, tableMethod, pathParams, queryParams), "pathTable");
pathParams.clear();
queryParams.putSingle("tableName", "queryTable");
- assertEquals(FineGrainedAuthUtils.findRawTargetId(tableAuth, pathParams, queryParams), "queryTable");
- assertNull(FineGrainedAuthUtils.findRawTargetId(clusterAuth, pathParams, queryParams));
+ // The annotation names tableName, but getTable never binds it, so the query value is not trusted.
+ assertNull(FineGrainedAuthUtils.findRawTargetId(tableAuth, tableMethod, pathParams, queryParams));
+ assertEquals(FineGrainedAuthUtils.findRawTargetId(tableQueryAuth, tableQueryMethod, pathParams, queryParams),
+ "queryTable");
+ assertNull(FineGrainedAuthUtils.findRawTargetId(clusterAuth, getAnnotatedMethod(), pathParams, queryParams));
queryParams.clear();
- assertNull(FineGrainedAuthUtils.findRawTargetId(tableAuth, pathParams, queryParams));
+ assertNull(FineGrainedAuthUtils.findRawTargetId(tableAuth, tableMethod, pathParams, queryParams));
+ }
+
+ @Test
+ public void testValidateFineGrainedAuthIgnoresUndeclaredTableQueryParam()
+ throws Exception {
+ FineGrainedAccessControl ac = Mockito.mock(FineGrainedAccessControl.class);
+ Mockito.when(ac.hasAccess(Mockito.any(HttpHeaders.class), Mockito.any(), Mockito.any(), Mockito.any()))
+ .thenReturn(true);
+
+ UriInfo mockUriInfo = Mockito.mock(UriInfo.class);
+ MultivaluedMap<String, String> pathParams = new MultivaluedHashMap<>();
+ MultivaluedMap<String, String> queryParams = new MultivaluedHashMap<>();
+ queryParams.putSingle("tableName", "callerPicked");
+ Mockito.when(mockUriInfo.getPathParameters()).thenReturn(pathParams);
+ Mockito.when(mockUriInfo.getQueryParameters()).thenReturn(queryParams);
+ Mockito.when(mockUriInfo.getRequestUri()).thenReturn(URI.create("http://localhost/tables"));
+ HttpHeaders mockHttpHeaders = Mockito.mock(HttpHeaders.class);
+
+ Method unboundTableMethod = TestResource.class.getDeclaredMethod("getTable");
+ try {
+ FineGrainedAuthUtils.validateFineGrainedAuth(unboundTableMethod, mockUriInfo, mockHttpHeaders, ac);
+ Assert.fail("Expected WebApplicationException");
+ } catch (WebApplicationException e) {
+ Assert.assertTrue(e.getMessage().contains("Could not find paramName"));
+ Assert.assertEquals(e.getResponse().getStatus(),
+ FineGrainedAuthUtils.UNBOUND_TABLE_PARAM_STATUS.getStatusCode());
+ Assert.assertEquals(e.getResponse().getStatus(), Response.Status.FORBIDDEN.getStatusCode());
+ }
+ Mockito.verify(ac, Mockito.never())
+ .hasAccess(Mockito.any(HttpHeaders.class), Mockito.any(), Mockito.any(), Mockito.any());
+ }
+
+ @Test
+ public void testUnboundTableParamExceptionRevertsToInternalServerError() {
+ URI requestUri = URI.create("http://localhost/tables");
+ WebApplicationException current = FineGrainedAuthUtils.unboundTableParamException("tableName", requestUri);
+ Assert.assertEquals(current.getResponse().getStatus(), Response.Status.FORBIDDEN.getStatusCode());
+
+ // Revert path: passing the previous status restores the 500 that this case used to pin.
+ WebApplicationException reverted = FineGrainedAuthUtils.unboundTableParamException("tableName", requestUri,
+ Response.Status.INTERNAL_SERVER_ERROR);
+ Assert.assertTrue(reverted.getMessage().contains("Could not find paramName"));
+ Assert.assertEquals(reverted.getResponse().getStatus(), Response.Status.INTERNAL_SERVER_ERROR.getStatusCode());
+ Assert.assertEquals(reverted.getMessage(), current.getMessage());
+ }
Review Comment:
Deleted. The pin that matters is still testValidateFineGrainedAuthIgnoresUndeclaredTableQueryParam (500, hasAccess never called).
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControl.java:
##########
@@ -25,6 +25,15 @@
import org.apache.pinot.spi.annotations.InterfaceStability;
+/// Controller access-control SPI.
+///
+/// Custom implementations should audit two resolution changes from apache/pinot#18975:
+/// 1. A table name appended as an undeclared query parameter no longer reaches
+/// [#hasAccess(String, AccessType, HttpHeaders, String)]; the request arrives with a `null`
+/// table name and must be treated as cluster-wide.
+/// 2. [org.apache.pinot.core.auth.FineGrainedAuthUtils#findRawTargetId] changed from 3 arguments
+/// (`Authorize`, path map, query map) to 4 by adding the resource `Method`. There is no
+/// overload; a plugin calling the old signature will fail to link.
Review Comment:
Class-level migration note is gone with the revert. Per-method javadoc still has the durable contract (null table name = cluster-wide). findRawTargetId 3 to 4 and the broker-resolution notes are in the PR description now, not on the AccessControl SPI.
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/FineGrainedAuthUtils.java:
##########
@@ -37,34 +43,58 @@
public class FineGrainedAuthUtils {
private static final Logger LOGGER = LoggerFactory.getLogger(FineGrainedAuthUtils.class);
+ /// Memoizes [#declaredQueryParams(Method)]; bounded by the number of endpoints.
+ private static final Map<Method, Set<String>> DECLARED_QUERY_PARAMS = new ConcurrentHashMap<>();
private FineGrainedAuthUtils() {
}
- /// Returns the parameter from the path or query params.
- /// @param paramName to look for
- /// @param pathParams path params
- /// @param queryParams query params
- /// @return the value of the parameter
- private static String findParam(String paramName, MultivaluedMap<String, String> pathParams,
- MultivaluedMap<String, String> queryParams) {
- String name = pathParams.getFirst(paramName);
- if (name == null) {
- name = queryParams.getFirst(paramName);
+ /// Returns the names `endpointMethod` binds as [QueryParam]s.
+ ///
+ /// Only method-level `@QueryParam` binding is recognized; an endpoint binding parameters through `@BeanParam` or
+ /// resource-class field injection is treated as declaring none. That direction denies table scope rather than
+ /// granting it, so it fails closed. No in-tree controller or broker resource uses either form today.
+ ///
+ /// Results are memoized because [Method#getParameterAnnotations()] re-parses the class-file annotation bytes on
+ /// every call, and this runs on every request before authentication. The key set is bounded by the number of
+ /// endpoints.
+ public static Set<String> declaredQueryParams(Method endpointMethod) {
+ return DECLARED_QUERY_PARAMS.computeIfAbsent(endpointMethod, FineGrainedAuthUtils::findDeclaredQueryParams);
+ }
+
+ private static Set<String> findDeclaredQueryParams(Method endpointMethod) {
+ Set<String> declared = new HashSet<>();
+ for (Annotation[] parameterAnnotations : endpointMethod.getParameterAnnotations()) {
+ for (Annotation parameterAnnotation : parameterAnnotations) {
+ if (parameterAnnotation instanceof QueryParam queryParam) {
+ declared.add(queryParam.value());
+ }
+ }
}
- return name;
+ return Set.copyOf(declared);
}
/// Finds the raw target parameter identified by an [Authorize] annotation.
///
+ /// Path parameters are template variables of the endpoint's own `@Path` and are always trusted. Query parameters
+ /// are caller-supplied, so only a name the method binds as `@QueryParam` may identify the table.
+ ///
/// @param auth annotation identifying the authorization target
+ /// @param endpointMethod the resource method, used to decide which query parameters are declared
/// @param pathParams request path parameters
/// @param queryParams request query parameters
/// @return the unnormalized table parameter value, or `null` for a cluster target or missing table parameter
@Nullable
- public static String findRawTargetId(Authorize auth, MultivaluedMap<String, String> pathParams,
- MultivaluedMap<String, String> queryParams) {
- return auth.targetType() == TargetType.TABLE ? findParam(auth.paramName(), pathParams, queryParams) : null;
+ public static String findRawTargetId(Authorize auth, Method endpointMethod,
+ MultivaluedMap<String, String> pathParams, MultivaluedMap<String, String> queryParams) {
+ if (auth.targetType() != TargetType.TABLE) {
+ return null;
+ }
+ String targetId = pathParams.getFirst(auth.paramName());
+ if (targetId == null && declaredQueryParams(endpointMethod).contains(auth.paramName())) {
+ targetId = queryParams.getFirst(auth.paramName());
+ }
+ return targetId;
}
Review Comment:
Scope and Custom AccessControl sections are updated. FineGrainedAuthUtils is shared with the broker filter, so undeclared ?tableName= is ignored there too. The 8 in-tree broker TABLE endpoints use @PathParam, so behavior is unchanged. Broker BasicAuth cluster-scope gap stays follow-up. findRawTargetId went 3 to 4 args (Method added), no overload; old plugins fail to link. Also restored the LLC completion and GET /auth/verify back-compat bullets that helm already had.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Restrict BasicAuth cluster access to unrestricted users [pinot]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on code in PR #18975:
URL: https://github.com/apache/pinot/pull/18975#discussion_r3725705141
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/BasicAuthAccessControlFactory.java:
##########
@@ -87,15 +88,58 @@ public boolean hasAccess(String tableName, AccessType accessType, HttpHeaders ht
@Override
public boolean hasAccess(AccessType accessType, HttpHeaders httpHeaders, String endpointUrl) {
- if (getPrincipal(httpHeaders).isEmpty()) {
+ Optional<BasicAuthPrincipal> principal = getPrincipal(httpHeaders);
+ if (principal.isEmpty()) {
throw new NotAuthorizedException("Basic");
}
- return true;
+ return hasClusterAccess(principal.get(), Objects.toString(accessType));
}
@Override
public boolean hasAccess(HttpHeaders httpHeaders, TargetType targetType) {
- return getPrincipal(httpHeaders).isPresent();
+ return getPrincipal(httpHeaders)
+ .filter(p -> targetType == TargetType.TABLE || p.hasUnrestrictedTableAccess())
+ .isPresent();
+ }
+
+ @Override
+ public boolean hasAccess(HttpHeaders httpHeaders, TargetType targetType, String targetId, String action) {
+ Optional<BasicAuthPrincipal> principal = getPrincipal(httpHeaders);
+ if (targetType == TargetType.TABLE) {
+ return principal
+ .filter(p -> p.hasTable(TableNameBuilder.extractRawTableName(targetId)) && hasActionPermission(p, action))
+ .isPresent();
+ }
+ if (targetType == TargetType.CLUSTER) {
+ return principal.filter(p -> hasClusterAccess(p, action)).isPresent();
+ }
+ return false;
+ }
+
+ private static boolean hasClusterAccess(BasicAuthPrincipal principal, String action) {
+ return principal.hasUnrestrictedTableAccess() && hasActionPermission(principal, action);
+ }
+
+ private static boolean hasActionPermission(BasicAuthPrincipal principal, String action) {
+ if (action != null && principal.hasPermission(action)) {
+ return true;
+ }
+ return principal.hasPermission(Objects.toString(getAccessTypeForAction(action)));
+ }
+
+ private static AccessType getAccessTypeForAction(String action) {
+ if (action == null || action.startsWith("Get") || action.startsWith("List") || action.startsWith("Query")
+ || action.startsWith("Debug") || action.startsWith("Estimate") || action.startsWith("Recommend")) {
+ return AccessType.READ;
+ }
+ if (action.startsWith("Create") || action.startsWith("Ingest") || action.startsWith("Commit")
+ || action.startsWith("Upload")) {
+ return AccessType.CREATE;
+ }
+ if (action.startsWith("Delete") || action.startsWith("Cancel")) {
+ return AccessType.DELETE;
+ }
+ return AccessType.UPDATE;
}
Review Comment:
Good catch, both misclassifications are real. downloadSegment is a GET protected with @Authenticate(AccessType.READ), but DownloadSegment falls through to the default UPDATE branch here, and cancelRebalance is protected with @Authenticate(AccessType.UPDATE) while the Cancel prefix maps it to DELETE. I will fix it in both factories by adding a Download prefix to the READ branch and adding an explicit case mapping CancelRebalance to UPDATE. The generic Cancel prefix will keep mapping to DELETE because the CancelQuery endpoints in PinotRunningQueryResource are annotated with @Authenticate(AccessType.DELETE). I will also add test cases for DownloadSegment and CancelRebalance in BasicAuthAccessControlFactoryTest.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/ZkBasicAuthAccessControlFactory.java:
##########
@@ -90,12 +90,54 @@ public boolean hasAccess(String tableName, AccessType accessType, HttpHeaders ht
@Override
public boolean hasAccess(HttpHeaders httpHeaders, TargetType targetType) {
- return getPrincipal(httpHeaders).isPresent();
+ return getPrincipal(httpHeaders)
+ .filter(p -> targetType == TargetType.TABLE || p.hasUnrestrictedTableAccess())
+ .isPresent();
}
@Override
public boolean hasAccess(AccessType accessType, HttpHeaders httpHeaders, String endpointUrl) {
- return getPrincipal(httpHeaders).isPresent();
+ return getPrincipal(httpHeaders).filter(p -> hasClusterAccess(p, Objects.toString(accessType))).isPresent();
+ }
+
+ @Override
+ public boolean hasAccess(HttpHeaders httpHeaders, TargetType targetType, String targetId, String action) {
+ Optional<ZkBasicAuthPrincipal> principal = getPrincipal(httpHeaders);
+ if (targetType == TargetType.TABLE) {
+ return principal
+ .filter(p -> p.hasTable(TableNameBuilder.extractRawTableName(targetId)) && hasActionPermission(p, action))
+ .isPresent();
+ }
+ if (targetType == TargetType.CLUSTER) {
+ return principal.filter(p -> hasClusterAccess(p, action)).isPresent();
+ }
+ return false;
+ }
+
+ private static boolean hasClusterAccess(ZkBasicAuthPrincipal principal, String action) {
+ return principal.hasUnrestrictedTableAccess() && hasActionPermission(principal, action);
+ }
+
+ private static boolean hasActionPermission(ZkBasicAuthPrincipal principal, String action) {
+ if (action != null && principal.hasPermission(action)) {
+ return true;
+ }
+ return principal.hasPermission(Objects.toString(getAccessTypeForAction(action)));
+ }
+
+ private static AccessType getAccessTypeForAction(String action) {
+ if (action == null || action.startsWith("Get") || action.startsWith("List") || action.startsWith("Query")
+ || action.startsWith("Debug") || action.startsWith("Estimate") || action.startsWith("Recommend")) {
+ return AccessType.READ;
+ }
+ if (action.startsWith("Create") || action.startsWith("Ingest") || action.startsWith("Commit")
+ || action.startsWith("Upload")) {
+ return AccessType.CREATE;
+ }
+ if (action.startsWith("Delete") || action.startsWith("Cancel")) {
+ return AccessType.DELETE;
+ }
+ return AccessType.UPDATE;
}
Review Comment:
Confirmed, this factory has the identical mapping and the identical problem. I will apply the same fix here as described in my reply on the BasicAuthAccessControlFactory comment: add a Download prefix to the READ branch and an explicit CancelRebalance to UPDATE case, keeping the Cancel prefix mapped to DELETE for CancelQuery.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [controller] Require unrestricted table scope for cluster-level BasicAuth access [pinot]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on code in PR #18975:
URL: https://github.com/apache/pinot/pull/18975#discussion_r3817445955
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AuthenticationFilter.java:
##########
@@ -130,35 +142,70 @@ 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);
+ // The annotation names the parameter, but it is only trustworthy where the endpoint also binds it.
+ MultivaluedMap<String, String> trustedQueryParameters =
+ declaredQueryParams(endpointMethod).contains(authorize.paramName()) ? queryParameters
+ : new MultivaluedHashMap<>();
+ return FineGrainedAuthUtils.findRawTargetId(authorize, pathParameters, trustedQueryParameters);
}
- return extractTableName(pathParameters, queryParameters);
- }
-
- @VisibleForTesting
- static String extractTableName(MultivaluedMap<String, String> pathParameters,
- MultivaluedMap<String, String> queryParameters) {
String tableName = extractTableName(pathParameters);
if (tableName != null) {
return tableName;
}
- return extractTableName(queryParameters);
+ Set<String> declaredQueryParams = declaredQueryParams(endpointMethod);
+ for (String key : TABLE_NAME_KEYS) {
+ if (queryParameters.containsKey(key) && declaredQueryParams.contains(key)) {
+ return queryParameters.getFirst(key);
+ }
+ }
+ return null;
Review Comment:
Good catch, and I kept it as a note rather than a behavior change. Declared is treated as “this parameter may name the table,” which is only safe when the endpoint actually scopes the operation with it. GET /tasks/task/{taskName}/debug only filters subtasks. Task-level state still comes back, so ?tableName=A still lets a principal scoped to A read metadata for a task on B. Master already trusted that parameter, so this is not a regression. I added that caveat to extractTableName and to the description so the next cluster endpoint with an optional tableName does not copy the pattern by accident.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [controller] Require unrestricted table scope for cluster-level BasicAuth access [pinot]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on code in PR #18975:
URL: https://github.com/apache/pinot/pull/18975#discussion_r3817442879
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthPrincipal.java:
##########
@@ -62,6 +62,15 @@ 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.
+ public boolean hasUnrestrictedTableAccess() {
+ return _tables.isEmpty() && _excludeTables.isEmpty();
+ }
+
Review Comment:
Table lists are the signal on purpose. The static factory has no RoleType, and both controller factories share BaseBasicAuthAccessControl, so the cluster gate has to be something both understand. An ADMIN that also has an allow-list is still scoped. If we keyed off role, that user would get /users and the rest of the cluster while we were telling ourselves they were confined to a subset. Default bootstrap is fine (initUserACLConfig sets tables=null), so [RoleType.ADMIN](https://github.com/apache/pinot/pull/18975#) with no list still passes. I put that sentence on hasUnrestrictedTableAccess() and in the PR description.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [controller] Require unrestricted table scope for cluster-level BasicAuth access [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [controller] Require unrestricted table scope for cluster-level BasicAuth access [pinot]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on PR #18975:
URL: https://github.com/apache/pinot/pull/18975#issuecomment-5421268222
Merged apache/pinot master into this branch (e9802e3111). The git conflicts were AuthenticationFilter, FineGrainedAuthUtils, and FineGrainedAuthUtilsTest. Resolution keeps the cluster-scope security fix and the revert of 33fd79729a, and takes master's #19231 400 for a missing or invalid table param.
Commit 3 stays reverted. 403 is still follow-up.
spotless, checkstyle, and license passed on pinot-core and pinot-controller. Auth tests passed, including FineGrainedAuthUtilsTest, AuthenticationFilterTest, ControllerClusterBasicAuthAuthorizationTest, and PinotSegmentUploadAuthorizationTest.
All open review threads have a reply and are resolved.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [controller] Require unrestricted table scope for cluster-level BasicAuth access [pinot]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on code in PR #18975:
URL: https://github.com/apache/pinot/pull/18975#discussion_r3817444507
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AuthenticationFilter.java:
##########
@@ -130,35 +142,70 @@ 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);
+ // The annotation names the parameter, but it is only trustworthy where the endpoint also binds it.
+ MultivaluedMap<String, String> trustedQueryParameters =
+ declaredQueryParams(endpointMethod).contains(authorize.paramName()) ? queryParameters
+ : new MultivaluedHashMap<>();
+ return FineGrainedAuthUtils.findRawTargetId(authorize, pathParameters, trustedQueryParameters);
Review Comment:
That was the real leftover. I moved the declared-query-param lookup into FineGrainedAuthUtils and threaded it through findRawTargetId. validateFineGrainedAuth and the coarse filter now use the same primitive, so a custom FineGrainedAccessControl cannot see a targetId the coarse check already treated as cluster-wide. Added a test that an undeclared ?tableName= no longer reaches hasAccess.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [controller] Require unrestricted table scope for cluster-level BasicAuth access [pinot]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on code in PR #18975:
URL: https://github.com/apache/pinot/pull/18975#discussion_r3817440696
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/BaseBasicAuthAccessControl.java:
##########
@@ -36,8 +37,14 @@ public final boolean protectAnnotatedOnly() {
}
@Override
- public final boolean hasAccess(String tableName, AccessType accessType, HttpHeaders httpHeaders,
+ public final boolean hasAccess(@Nullable String tableName, AccessType accessType, HttpHeaders httpHeaders,
String endpointUrl) {
+ // A null table name means the request named no table, which makes it cluster-wide however the caller reached this
+ // overload. Route it to the cluster check so the scope rule cannot be sidestepped by omitting the table: callers
+ // include AccessControl's own default cluster overload and the /auth/verify probe, whose table name is optional.
+ if (tableName == null) {
+ return hasAccess(accessType, httpHeaders, endpointUrl);
+ }
Review Comment:
Agreed, and it belongs in that list. v1 GET /auth/verify with no tableName now hits the cluster check, so a table-scoped principal gets false where it used to get true. UI login stays on v2, so that gate is fine. Third-party probes still on v1 will read it as a credential failure. Called that out in the description and in the helm comment.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [controller] Require unrestricted table scope for cluster-level BasicAuth access [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #18975:
URL: https://github.com/apache/pinot/pull/18975#issuecomment-6119329759
Documentation follow-up merged: https://github.com/pinot-contrib/pinot-docs/pull/1097. Covers unrestricted controller cluster scope, scoped endpoint behavior, and service credential preparation before rolling controllers.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [controller] Require unrestricted table scope for cluster-level BasicAuth access [pinot]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on code in PR #18975:
URL: https://github.com/apache/pinot/pull/18975#discussion_r3860045180
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControlUtils.java:
##########
@@ -74,6 +75,36 @@ public static void validatePermission(@Nullable String tableName, AccessType acc
Response.Status.FORBIDDEN);
}
+ /// Maps a fine grained action name (see [Actions]) to the coarse [AccessType] bucket that guards the corresponding
+ /// endpoint, so that access control implementations backed by coarse permissions can authorize fine grained actions.
+ /// Action names follow a `<verb><noun>` convention, hence the verb prefix selects the bucket.
+ ///
+ /// Actions whose verb does not match the access type of their endpoint are listed as exact matches, which are
+ /// evaluated before the prefixes. Keep new actions in sync with the `@Authenticate` annotation of the endpoint they
+ /// protect, otherwise a principal configured with only that endpoint's coarse permission is denied.
Review Comment:
This thread is on an older revision. AccessControlUtils no longer derives AccessType from action prefixes, so the REBALANCE_TENANT_TABLES / CREATE_TASK / FORCE_RELEASE mismatches are not in this PR. Current work is the cluster-scope check plus declared-query-param resolution. Resolving as obsolete.
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/FineGrainedAuthUtils.java:
##########
@@ -37,34 +43,58 @@
public class FineGrainedAuthUtils {
private static final Logger LOGGER = LoggerFactory.getLogger(FineGrainedAuthUtils.class);
+ /// Memoizes [#declaredQueryParams(Method)]; bounded by the number of endpoints.
+ private static final Map<Method, Set<String>> DECLARED_QUERY_PARAMS = new ConcurrentHashMap<>();
private FineGrainedAuthUtils() {
}
- /// Returns the parameter from the path or query params.
- /// @param paramName to look for
- /// @param pathParams path params
- /// @param queryParams query params
- /// @return the value of the parameter
- private static String findParam(String paramName, MultivaluedMap<String, String> pathParams,
- MultivaluedMap<String, String> queryParams) {
- String name = pathParams.getFirst(paramName);
- if (name == null) {
- name = queryParams.getFirst(paramName);
+ /// Returns the names `endpointMethod` binds as [QueryParam]s.
+ ///
+ /// Only method-level `@QueryParam` binding is recognized; an endpoint binding parameters through `@BeanParam` or
+ /// resource-class field injection is treated as declaring none. That direction denies table scope rather than
+ /// granting it, so it fails closed. No in-tree controller or broker resource uses either form today.
+ ///
+ /// Results are memoized because [Method#getParameterAnnotations()] re-parses the class-file annotation bytes on
+ /// every call, and this runs on every request before authentication. The key set is bounded by the number of
+ /// endpoints.
+ public static Set<String> declaredQueryParams(Method endpointMethod) {
+ return DECLARED_QUERY_PARAMS.computeIfAbsent(endpointMethod, FineGrainedAuthUtils::findDeclaredQueryParams);
+ }
+
+ private static Set<String> findDeclaredQueryParams(Method endpointMethod) {
+ Set<String> declared = new HashSet<>();
+ for (Annotation[] parameterAnnotations : endpointMethod.getParameterAnnotations()) {
+ for (Annotation parameterAnnotation : parameterAnnotations) {
+ if (parameterAnnotation instanceof QueryParam queryParam) {
+ declared.add(queryParam.value());
+ }
+ }
}
- return name;
+ return Set.copyOf(declared);
}
/// Finds the raw target parameter identified by an [Authorize] annotation.
///
+ /// Path parameters are template variables of the endpoint's own `@Path` and are always trusted. Query parameters
+ /// are caller-supplied, so only a name the method binds as `@QueryParam` may identify the table.
+ ///
/// @param auth annotation identifying the authorization target
+ /// @param endpointMethod the resource method, used to decide which query parameters are declared
/// @param pathParams request path parameters
/// @param queryParams request query parameters
/// @return the unnormalized table parameter value, or `null` for a cluster target or missing table parameter
@Nullable
- public static String findRawTargetId(Authorize auth, MultivaluedMap<String, String> pathParams,
- MultivaluedMap<String, String> queryParams) {
- return auth.targetType() == TargetType.TABLE ? findParam(auth.paramName(), pathParams, queryParams) : null;
+ public static String findRawTargetId(Authorize auth, Method endpointMethod,
+ MultivaluedMap<String, String> pathParams, MultivaluedMap<String, String> queryParams) {
+ if (auth.targetType() != TargetType.TABLE) {
+ return null;
+ }
+ String targetId = pathParams.getFirst(auth.paramName());
+ if (targetId == null && declaredQueryParams(endpointMethod).contains(auth.paramName())) {
+ targetId = queryParams.getFirst(auth.paramName());
+ }
+ return targetId;
}
Review Comment:
PR description still has the Scope notes: FineGrainedAuthUtils is shared with the broker, the 8 in-tree broker TABLE endpoints use PathParam, findRawTargetId went from 3 to 4 args with no overload. The master merge did not change that. Resolving.
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/FineGrainedAuthUtils.java:
##########
@@ -97,8 +127,10 @@ public static void validateFineGrainedAuth(Method endpointMethod, UriInfo uriInf
Response.Status.INTERNAL_SERVER_ERROR);
}
- // find the paramName in the path or query params
- targetId = findRawTargetId(auth, uriInfo.getPathParameters(), uriInfo.getQueryParameters());
+ // Path params are part of the endpoint declaration. Query params are caller-supplied, so only a name
+ // the method binds as @QueryParam may identify the table. Otherwise a caller could append
+ // ?tableName=<a table it is scoped to> and have a cluster-wide request authorized as table-scoped.
+ targetId = findRawTargetId(auth, endpointMethod, uriInfo.getPathParameters(), uriInfo.getQueryParameters());
Review Comment:
Agreed this is not a hold. We reverted 33fd79729a so this PR does not ship 403. After merging master, a missing or unbound table param is 400 from #19231, not 500 and not 403. Empty paramName on the annotation is still 500. 403 stays follow-up if we still want it.
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/FineGrainedAuthUtils.java:
##########
@@ -33,38 +40,83 @@
import org.slf4j.LoggerFactory;
-/// Utility methods to share in Broker and Controller request filters related to fine grain authorization.
+/// Shared broker and controller helpers for fine-grained authorization.
+///
+/// The broker request filter calls [#validateFineGrainedAuth], so tightening target resolution
+/// here applies to both roles. Every in-tree broker `@Authorize(targetType = TargetType.TABLE)`
+/// endpoint (`PinotBrokerDebug`, `PinotBrokerRouting`) binds `tableName` as a `@PathParam` inside
+/// its own `@Path` template, so the declared-query-param filter does not change broker resolution.
+///
+/// [#findRawTargetId] is public. It previously took 3 arguments (`Authorize`, path map, query
+/// map) and now takes 4 (`Authorize`, `Method`, path map, query map). There is no overload: the
+/// `Method` is required to apply the declared-parameter filter. A plugin calling the old
+/// signature will fail to link.
public class FineGrainedAuthUtils {
private static final Logger LOGGER = LoggerFactory.getLogger(FineGrainedAuthUtils.class);
+ /// Memoizes [#declaredQueryParams(Method)]; bounded by the number of endpoints.
+ private static final Map<Method, Set<String>> DECLARED_QUERY_PARAMS = new ConcurrentHashMap<>();
+
+ /// Status when `@Authorize(TABLE)` names a parameter the endpoint never binds.
+ ///
+ /// Historically [Response.Status#INTERNAL_SERVER_ERROR], which pages on error-rate alerts and
+ /// reads as a controller bug. [Response.Status#FORBIDDEN] is the authorization outcome.
+ /// Restore `INTERNAL_SERVER_ERROR` here — or pass it to [#unboundTableParamException] — to
+ /// revert to the previous status. Tests pin both directions.
+ static final Response.Status UNBOUND_TABLE_PARAM_STATUS = Response.Status.FORBIDDEN;
Review Comment:
The constant, both unboundTableParamException helpers, and the revert test stayed deleted through the master merge. The throw site is not 403. Resolving.
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/FineGrainedAuthUtils.java:
##########
@@ -92,18 +144,21 @@ public static void validateFineGrainedAuth(Method endpointMethod, UriInfo uriInf
if (auth.targetType() == TargetType.TABLE) {
// paramName is mandatory for table level authorization
if (StringUtils.isEmpty(auth.paramName())) {
+ // TODO: PinotTableRestletResource#copyTable declares @Authorize(TABLE) with no paramName
+ // and already 500s here. Prefer startup-time validation of the resource model (or 403)
+ // so a misannotation is not an error-rate page.
throw new WebApplicationException(
"paramName not found for table level authorization in API: " + uriInfo.getRequestUri(),
Response.Status.INTERNAL_SERVER_ERROR);
}
- // find the paramName in the path or query params
- targetId = findRawTargetId(auth, uriInfo.getPathParameters(), uriInfo.getQueryParameters());
+ // Path params are part of the endpoint declaration. Query params are caller-supplied, so only a name
+ // the method binds as @QueryParam may identify the table. Otherwise a caller could append
+ // ?tableName=<a table it is scoped to> and have a cluster-wide request authorized as table-scoped.
+ targetId = findRawTargetId(auth, endpointMethod, uriInfo.getPathParameters(), uriInfo.getQueryParameters());
if (StringUtils.isEmpty(targetId)) {
- throw new WebApplicationException(
- "Could not find paramName " + auth.paramName() + " in path or query params of the API: "
- + uriInfo.getRequestUri(), Response.Status.INTERNAL_SERVER_ERROR);
+ throw unboundTableParamException(auth.paramName(), uriInfo.getRequestUri());
Review Comment:
33fd79729a stays reverted. The tree still matches the approved security fix, plus master #19231 400 for a missing table param. 403 is follow-up. Resolving.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControl.java:
##########
@@ -25,6 +25,15 @@
import org.apache.pinot.spi.annotations.InterfaceStability;
+/// Controller access-control SPI.
+///
+/// Custom implementations should audit two resolution changes from apache/pinot#18975:
+/// 1. A table name appended as an undeclared query parameter no longer reaches
+/// [#hasAccess(String, AccessType, HttpHeaders, String)]; the request arrives with a `null`
+/// table name and must be treated as cluster-wide.
+/// 2. [org.apache.pinot.core.auth.FineGrainedAuthUtils#findRawTargetId] changed from 3 arguments
+/// (`Authorize`, path map, query map) to 4 by adding the resource `Method`. There is no
+/// overload; a plugin calling the old signature will fail to link.
Review Comment:
Class-level migration note stayed out after the revert and the master merge. Per-method javadoc still has the durable contract: null table name means cluster-wide. Resolving.
##########
pinot-core/src/test/java/org/apache/pinot/core/auth/FineGrainedAuthUtilsTest.java:
##########
@@ -36,22 +38,113 @@
public class FineGrainedAuthUtilsTest {
@Test
- public void testFindRawTargetId() throws Exception {
+ public void testFindRawTargetId()
+ throws Exception {
MultivaluedMap<String, String> pathParams = new MultivaluedHashMap<>();
MultivaluedMap<String, String> queryParams = new MultivaluedHashMap<>();
- Authorize tableAuth = TestResource.class.getDeclaredMethod("getTable").getAnnotation(Authorize.class);
+ Method tableMethod = TestResource.class.getDeclaredMethod("getTable");
+ Method tableQueryMethod = TestResource.class.getDeclaredMethod("getTableByQuery", String.class);
+ Authorize tableAuth = tableMethod.getAnnotation(Authorize.class);
+ Authorize tableQueryAuth = tableQueryMethod.getAnnotation(Authorize.class);
Authorize clusterAuth = getAnnotatedMethod().getAnnotation(Authorize.class);
pathParams.putSingle("tableName", "pathTable");
- assertEquals(FineGrainedAuthUtils.findRawTargetId(tableAuth, pathParams, queryParams), "pathTable");
+ assertEquals(FineGrainedAuthUtils.findRawTargetId(tableAuth, tableMethod, pathParams, queryParams), "pathTable");
pathParams.clear();
queryParams.putSingle("tableName", "queryTable");
- assertEquals(FineGrainedAuthUtils.findRawTargetId(tableAuth, pathParams, queryParams), "queryTable");
- assertNull(FineGrainedAuthUtils.findRawTargetId(clusterAuth, pathParams, queryParams));
+ // The annotation names tableName, but getTable never binds it, so the query value is not trusted.
+ assertNull(FineGrainedAuthUtils.findRawTargetId(tableAuth, tableMethod, pathParams, queryParams));
+ assertEquals(FineGrainedAuthUtils.findRawTargetId(tableQueryAuth, tableQueryMethod, pathParams, queryParams),
+ "queryTable");
+ assertNull(FineGrainedAuthUtils.findRawTargetId(clusterAuth, getAnnotatedMethod(), pathParams, queryParams));
queryParams.clear();
- assertNull(FineGrainedAuthUtils.findRawTargetId(tableAuth, pathParams, queryParams));
+ assertNull(FineGrainedAuthUtils.findRawTargetId(tableAuth, tableMethod, pathParams, queryParams));
+ }
+
+ @Test
+ public void testValidateFineGrainedAuthIgnoresUndeclaredTableQueryParam()
+ throws Exception {
+ FineGrainedAccessControl ac = Mockito.mock(FineGrainedAccessControl.class);
+ Mockito.when(ac.hasAccess(Mockito.any(HttpHeaders.class), Mockito.any(), Mockito.any(), Mockito.any()))
+ .thenReturn(true);
+
+ UriInfo mockUriInfo = Mockito.mock(UriInfo.class);
+ MultivaluedMap<String, String> pathParams = new MultivaluedHashMap<>();
+ MultivaluedMap<String, String> queryParams = new MultivaluedHashMap<>();
+ queryParams.putSingle("tableName", "callerPicked");
+ Mockito.when(mockUriInfo.getPathParameters()).thenReturn(pathParams);
+ Mockito.when(mockUriInfo.getQueryParameters()).thenReturn(queryParams);
+ Mockito.when(mockUriInfo.getRequestUri()).thenReturn(URI.create("http://localhost/tables"));
+ HttpHeaders mockHttpHeaders = Mockito.mock(HttpHeaders.class);
+
+ Method unboundTableMethod = TestResource.class.getDeclaredMethod("getTable");
+ try {
+ FineGrainedAuthUtils.validateFineGrainedAuth(unboundTableMethod, mockUriInfo, mockHttpHeaders, ac);
+ Assert.fail("Expected WebApplicationException");
+ } catch (WebApplicationException e) {
+ Assert.assertTrue(e.getMessage().contains("Could not find paramName"));
+ Assert.assertEquals(e.getResponse().getStatus(),
+ FineGrainedAuthUtils.UNBOUND_TABLE_PARAM_STATUS.getStatusCode());
+ Assert.assertEquals(e.getResponse().getStatus(), Response.Status.FORBIDDEN.getStatusCode());
+ }
+ Mockito.verify(ac, Mockito.never())
+ .hasAccess(Mockito.any(HttpHeaders.class), Mockito.any(), Mockito.any(), Mockito.any());
+ }
+
+ @Test
+ public void testUnboundTableParamExceptionRevertsToInternalServerError() {
+ URI requestUri = URI.create("http://localhost/tables");
+ WebApplicationException current = FineGrainedAuthUtils.unboundTableParamException("tableName", requestUri);
+ Assert.assertEquals(current.getResponse().getStatus(), Response.Status.FORBIDDEN.getStatusCode());
+
+ // Revert path: passing the previous status restores the 500 that this case used to pin.
+ WebApplicationException reverted = FineGrainedAuthUtils.unboundTableParamException("tableName", requestUri,
+ Response.Status.INTERNAL_SERVER_ERROR);
+ Assert.assertTrue(reverted.getMessage().contains("Could not find paramName"));
+ Assert.assertEquals(reverted.getResponse().getStatus(), Response.Status.INTERNAL_SERVER_ERROR.getStatusCode());
+ Assert.assertEquals(reverted.getMessage(), current.getMessage());
+ }
Review Comment:
The revert-seam test stayed deleted. The pin is still testValidateFineGrainedAuthIgnoresUndeclaredTableQueryParam, and hasAccess is never called. Status there is 400 after #19231. Resolving.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [controller] Require unrestricted table scope for cluster-level BasicAuth access [pinot]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on PR #18975:
URL: https://github.com/apache/pinot/pull/18975#issuecomment-5349174298
Thanks for running the suites and for the revert checks. I agree the UI story should be in the release note in plain words: a table-scoped BasicAuth user can sign in and then hit 403 on /tables, /schemas, /instances, and /tenants. Denying those listings is safer than shipping an unfiltered cluster dump. Scope-filtered listings stay follow-up. The back-compat list and helm comment now also cover segment completion and v1 /auth/verify.
I could not push the description edit to apache/pinot from this environment. When you post the replies, please also add those Backward-incompatible bullets and the RoleType / task-debug notes to the PR body.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [controller] Require unrestricted table scope for cluster-level BasicAuth access [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #18975:
URL: https://github.com/apache/pinot/pull/18975#discussion_r3848969485
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/FineGrainedAuthUtils.java:
##########
@@ -97,8 +127,10 @@ public static void validateFineGrainedAuth(Method endpointMethod, UriInfo uriInf
Response.Status.INTERNAL_SERVER_ERROR);
}
- // find the paramName in the path or query params
- targetId = findRawTargetId(auth, uriInfo.getPathParameters(), uriInfo.getQueryParameters());
+ // Path params are part of the endpoint declaration. Query params are caller-supplied, so only a name
+ // the method binds as @QueryParam may identify the table. Otherwise a caller could append
+ // ?tableName=<a table it is scoped to> and have a cluster-wide request authorized as table-scoped.
+ targetId = findRawTargetId(auth, endpointMethod, uriInfo.getPathParameters(), uriInfo.getQueryParameters());
Review Comment:
Nit, non-blocking: the fail-closed outcome for an undeclared table query parameter arrives as **500 INTERNAL_SERVER_ERROR** via the `Could not find paramName` branch below, and the new `testValidateFineGrainedAuthIgnoresUndeclaredTableQueryParam` pins that.
Failing closed is the right call, but 500 is the wrong status for an authorization outcome — it will page on error-rate alerts and read as a controller bug rather than a denied request. It is pre-existing behavior for a missing target and only reachable through a misannotated endpoint, so I would not hold the PR for it. Worth a `// TODO` or a follow-up issue to make the "annotation names a parameter the endpoint never binds" case a 403 (or a startup-time validation over the resource model, which would catch `copyTable` too).
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/FineGrainedAuthUtils.java:
##########
@@ -37,34 +43,58 @@
public class FineGrainedAuthUtils {
private static final Logger LOGGER = LoggerFactory.getLogger(FineGrainedAuthUtils.class);
+ /// Memoizes [#declaredQueryParams(Method)]; bounded by the number of endpoints.
+ private static final Map<Method, Set<String>> DECLARED_QUERY_PARAMS = new ConcurrentHashMap<>();
private FineGrainedAuthUtils() {
}
- /// Returns the parameter from the path or query params.
- /// @param paramName to look for
- /// @param pathParams path params
- /// @param queryParams query params
- /// @return the value of the parameter
- private static String findParam(String paramName, MultivaluedMap<String, String> pathParams,
- MultivaluedMap<String, String> queryParams) {
- String name = pathParams.getFirst(paramName);
- if (name == null) {
- name = queryParams.getFirst(paramName);
+ /// Returns the names `endpointMethod` binds as [QueryParam]s.
+ ///
+ /// Only method-level `@QueryParam` binding is recognized; an endpoint binding parameters through `@BeanParam` or
+ /// resource-class field injection is treated as declaring none. That direction denies table scope rather than
+ /// granting it, so it fails closed. No in-tree controller or broker resource uses either form today.
+ ///
+ /// Results are memoized because [Method#getParameterAnnotations()] re-parses the class-file annotation bytes on
+ /// every call, and this runs on every request before authentication. The key set is bounded by the number of
+ /// endpoints.
+ public static Set<String> declaredQueryParams(Method endpointMethod) {
+ return DECLARED_QUERY_PARAMS.computeIfAbsent(endpointMethod, FineGrainedAuthUtils::findDeclaredQueryParams);
+ }
+
+ private static Set<String> findDeclaredQueryParams(Method endpointMethod) {
+ Set<String> declared = new HashSet<>();
+ for (Annotation[] parameterAnnotations : endpointMethod.getParameterAnnotations()) {
+ for (Annotation parameterAnnotation : parameterAnnotations) {
+ if (parameterAnnotation instanceof QueryParam queryParam) {
+ declared.add(queryParam.value());
+ }
+ }
}
- return name;
+ return Set.copyOf(declared);
}
/// Finds the raw target parameter identified by an [Authorize] annotation.
///
+ /// Path parameters are template variables of the endpoint's own `@Path` and are always trusted. Query parameters
+ /// are caller-supplied, so only a name the method binds as `@QueryParam` may identify the table.
+ ///
/// @param auth annotation identifying the authorization target
+ /// @param endpointMethod the resource method, used to decide which query parameters are declared
/// @param pathParams request path parameters
/// @param queryParams request query parameters
/// @return the unnormalized table parameter value, or `null` for a cluster target or missing table parameter
@Nullable
- public static String findRawTargetId(Authorize auth, MultivaluedMap<String, String> pathParams,
- MultivaluedMap<String, String> queryParams) {
- return auth.targetType() == TargetType.TABLE ? findParam(auth.paramName(), pathParams, queryParams) : null;
+ public static String findRawTargetId(Authorize auth, Method endpointMethod,
+ MultivaluedMap<String, String> pathParams, MultivaluedMap<String, String> queryParams) {
+ if (auth.targetType() != TargetType.TABLE) {
+ return null;
+ }
+ String targetId = pathParams.getFirst(auth.paramName());
+ if (targetId == null && declaredQueryParams(endpointMethod).contains(auth.paramName())) {
+ targetId = queryParams.getFirst(auth.paramName());
+ }
+ return targetId;
}
Review Comment:
This is exactly the consolidation I was after — coarse and fine-grained now resolve through one primitive, so they cannot disagree. Thanks for moving `declaredQueryParams` down here rather than duplicating it.
Two follow-ups on the blast radius, both description-only:
**1. The PR's "Scope: Controller only" line is now stale.** `FineGrainedAuthUtils` is shared with `pinot-broker`'s `AuthenticationFilter`, so the broker's fine-grained target resolution is tightened by this commit too. I checked every broker `@Authorize(targetType = TargetType.TABLE)` endpoint — all 8 across `PinotBrokerDebug` and `PinotBrokerRouting` bind `tableName` as a `@PathParam` inside their own `@Path` template, so none of them regress. But the description should say the broker's resolution changed and was verified, rather than "left for a follow-up" — a reader auditing this later will otherwise assume `pinot-broker` was untouched.
**2. `findRawTargetId` is a public signature change.** It went from 3 args to 4 (`Authorize, MultivaluedMap, MultivaluedMap` -> `Authorize, Method, MultivaluedMap, MultivaluedMap`). Every in-tree caller is updated, and the class carries no `@InterfaceAudience` marker, so I do not think it needs an overload — but it belongs in the **Custom `AccessControl` implementations** paragraph alongside the `null`-table-name note, since a plugin calling it will fail to link.
Verified across controller + broker: 92 of the 93 `@Authorize(TABLE)` endpoints resolve their `paramName` from either a `@Path` template variable or a declared `@QueryParam`, so nothing newly falls into the "could not find paramName" branch. The one exception is `PinotTableRestletResource#copyTable`, which declares `@Authorize(targetType = TargetType.TABLE)` with **no** `paramName` and therefore already 500s on master via the `StringUtils.isEmpty(auth.paramName())` guard — pre-existing and untouched here, worth a separate issue.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Restrict BasicAuth cluster access to unrestricted users [pinot]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on PR #18975:
URL: https://github.com/apache/pinot/pull/18975#issuecomment-4949200056
Review request: @soumitra-st @Jackie-Jiang
GitHub did not allow this account/integration to add requested reviewers through the reviewer section, so I am tagging the relevant reviewers here for visibility.
This PR fixes #14595 by tightening BasicAuth cluster/non-table access checks so table-scoped users do not automatically receive cluster-level access, while keeping table-level access behavior and coarse permission compatibility intact. Review from the BasicAuth/FineGrainedAccessControl side would be especially useful to confirm the intended cluster-level authorization boundary and compatibility with existing role-based access-control deployments.
---
Drafted-by: Codex (GPT-5); human-reviewed by Vamsi-klu before posting
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Restrict BasicAuth cluster access to unrestricted users [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #18975:
URL: https://github.com/apache/pinot/pull/18975#discussion_r3717359334
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/BasicAuthAccessControlFactory.java:
##########
@@ -87,15 +88,58 @@ public boolean hasAccess(String tableName, AccessType accessType, HttpHeaders ht
@Override
public boolean hasAccess(AccessType accessType, HttpHeaders httpHeaders, String endpointUrl) {
- if (getPrincipal(httpHeaders).isEmpty()) {
+ Optional<BasicAuthPrincipal> principal = getPrincipal(httpHeaders);
+ if (principal.isEmpty()) {
throw new NotAuthorizedException("Basic");
}
- return true;
+ return hasClusterAccess(principal.get(), Objects.toString(accessType));
}
@Override
public boolean hasAccess(HttpHeaders httpHeaders, TargetType targetType) {
- return getPrincipal(httpHeaders).isPresent();
+ return getPrincipal(httpHeaders)
+ .filter(p -> targetType == TargetType.TABLE || p.hasUnrestrictedTableAccess())
+ .isPresent();
+ }
+
+ @Override
+ public boolean hasAccess(HttpHeaders httpHeaders, TargetType targetType, String targetId, String action) {
+ Optional<BasicAuthPrincipal> principal = getPrincipal(httpHeaders);
+ if (targetType == TargetType.TABLE) {
+ return principal
+ .filter(p -> p.hasTable(TableNameBuilder.extractRawTableName(targetId)) && hasActionPermission(p, action))
+ .isPresent();
+ }
+ if (targetType == TargetType.CLUSTER) {
+ return principal.filter(p -> hasClusterAccess(p, action)).isPresent();
+ }
+ return false;
+ }
+
+ private static boolean hasClusterAccess(BasicAuthPrincipal principal, String action) {
+ return principal.hasUnrestrictedTableAccess() && hasActionPermission(principal, action);
+ }
+
+ private static boolean hasActionPermission(BasicAuthPrincipal principal, String action) {
+ if (action != null && principal.hasPermission(action)) {
+ return true;
+ }
+ return principal.hasPermission(Objects.toString(getAccessTypeForAction(action)));
+ }
+
+ private static AccessType getAccessTypeForAction(String action) {
+ if (action == null || action.startsWith("Get") || action.startsWith("List") || action.startsWith("Query")
+ || action.startsWith("Debug") || action.startsWith("Estimate") || action.startsWith("Recommend")) {
+ return AccessType.READ;
+ }
+ if (action.startsWith("Create") || action.startsWith("Ingest") || action.startsWith("Commit")
+ || action.startsWith("Upload")) {
+ return AccessType.CREATE;
+ }
+ if (action.startsWith("Delete") || action.startsWith("Cancel")) {
+ return AccessType.DELETE;
+ }
+ return AccessType.UPDATE;
}
Review Comment:
The coarse-permission fallback in getAccessTypeForAction() misclassifies some existing fine-grained actions. For example, Actions.Table.DOWNLOAD_SEGMENT is a GET endpoint (PinotSegmentUploadDownloadRestletResource) but is currently mapped to UPDATE because it starts with "Download"; and Actions.Table.CANCEL_REBALANCE is protected with AccessType.UPDATE (PinotTableRestletResource) but is currently mapped to DELETE due to the "Cancel" prefix. This will cause fine-grained authorization to deny requests that previously worked with coarse READ/UPDATE configs.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/ZkBasicAuthAccessControlFactory.java:
##########
@@ -90,12 +90,54 @@ public boolean hasAccess(String tableName, AccessType accessType, HttpHeaders ht
@Override
public boolean hasAccess(HttpHeaders httpHeaders, TargetType targetType) {
- return getPrincipal(httpHeaders).isPresent();
+ return getPrincipal(httpHeaders)
+ .filter(p -> targetType == TargetType.TABLE || p.hasUnrestrictedTableAccess())
+ .isPresent();
}
@Override
public boolean hasAccess(AccessType accessType, HttpHeaders httpHeaders, String endpointUrl) {
- return getPrincipal(httpHeaders).isPresent();
+ return getPrincipal(httpHeaders).filter(p -> hasClusterAccess(p, Objects.toString(accessType))).isPresent();
+ }
+
+ @Override
+ public boolean hasAccess(HttpHeaders httpHeaders, TargetType targetType, String targetId, String action) {
+ Optional<ZkBasicAuthPrincipal> principal = getPrincipal(httpHeaders);
+ if (targetType == TargetType.TABLE) {
+ return principal
+ .filter(p -> p.hasTable(TableNameBuilder.extractRawTableName(targetId)) && hasActionPermission(p, action))
+ .isPresent();
+ }
+ if (targetType == TargetType.CLUSTER) {
+ return principal.filter(p -> hasClusterAccess(p, action)).isPresent();
+ }
+ return false;
+ }
+
+ private static boolean hasClusterAccess(ZkBasicAuthPrincipal principal, String action) {
+ return principal.hasUnrestrictedTableAccess() && hasActionPermission(principal, action);
+ }
+
+ private static boolean hasActionPermission(ZkBasicAuthPrincipal principal, String action) {
+ if (action != null && principal.hasPermission(action)) {
+ return true;
+ }
+ return principal.hasPermission(Objects.toString(getAccessTypeForAction(action)));
+ }
+
+ private static AccessType getAccessTypeForAction(String action) {
+ if (action == null || action.startsWith("Get") || action.startsWith("List") || action.startsWith("Query")
+ || action.startsWith("Debug") || action.startsWith("Estimate") || action.startsWith("Recommend")) {
+ return AccessType.READ;
+ }
+ if (action.startsWith("Create") || action.startsWith("Ingest") || action.startsWith("Commit")
+ || action.startsWith("Upload")) {
+ return AccessType.CREATE;
+ }
+ if (action.startsWith("Delete") || action.startsWith("Cancel")) {
+ return AccessType.DELETE;
+ }
+ return AccessType.UPDATE;
}
Review Comment:
Same issue as the static BasicAuth factory: the coarse-permission fallback in getAccessTypeForAction() misclassifies some fine-grained actions used by controller endpoints (e.g. DownloadSegment should behave like READ; CancelRebalance is protected with AccessType.UPDATE but is mapped to DELETE via the "Cancel" prefix). This can cause fine-grained auth to block requests even when the coarse `@Authenticate` check would allow them.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org