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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
