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: [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]
