This is an automated email from the ASF dual-hosted git repository.
roryqi pushed a commit to branch branch-1.3
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/branch-1.3 by this push:
new bfe2876bd5 [Cherry-pick to branch-1.3] [#14447] fix(iceberg-rest):
authorize /v1/config and report non-Iceberg prefixes as client errors (#14449)
bfe2876bd5 is described below
commit bfe2876bd5802eea3073d63d568e1ee50c619c31
Author: Mark Hoerth <[email protected]>
AuthorDate: Sun Nov 1 19:44:30 2026 -0800
[Cherry-pick to branch-1.3] [#14447] fix(iceberg-rest): authorize
/v1/config and report non-Iceberg prefixes as client errors (#14449)
Backport to branch-1.3 of the fix for #14447 (same change as the PR to
main; it applies cleanly).
### What changes were proposed in this pull request?
- Intercept `IcebergConfigOperations` and authorize `getConfig` with
`LOAD_CATALOG_AUTHORIZATION_EXPRESSION` (the same expression as
`listNamespaces`), against the catalog named by the `warehouse`
parameter. The interceptor accepts a plain catalog name as well as a
path prefix. An empty warehouse now resolves through
`IcebergRESTUtils.getCatalogName`, so it uses the same default catalog
as an empty prefix, which is the catalog it is authorized against.
-
`IcebergMetadataAuthorizationMethodInterceptor.shouldSkipAuthorization`:
treat an `IllegalArgumentException` from the catalog lookup (a catalog
that is not an Iceberg catalog) like a missing catalog. The request is
then authorized normally: a caller without access gets 403 as for any
other name, and an authorized caller gets the operation's 400.
### Why are the changes needed?
Users without access to a catalog could read its configuration and probe
catalog names, and a client mistake was reported as an internal server
error.
Fixes #14447
### Does this PR introduce _any_ user-facing change?
Yes. With authorization enabled, `/v1/config` requires `USE_CATALOG` or
ownership on the catalog, so an engine's catalog initialization fails
with 403 for users who could not use the catalog anyway. A prefix naming
a non-Iceberg catalog returns 403 or 400 instead of 500.
### How was this patch tested?
`TestIcebergMetadataAuthorizationMethodInterceptor`:
- `testConfigEndpointIsAuthorizedOnTheWarehouseCatalog`: the config
resource is intercepted, carries the expression, and resolves a named
and an empty warehouse to the right catalog.
- `testNonIcebergCatalogDoesNotSkipAuthorization`.
-
`testInvokeShouldSkipAuthorizationWrapperLookupFailureReturnsAuthInternalError`
now uses an `IllegalStateException`, so it still checks that an
unexpected lookup failure is a 500.
`./gradlew :iceberg:iceberg-rest-server:test -PskipITs`
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01Eu6fb9pLfmrSJzGQDVLi1g
Co-authored-by: Claude Opus 5.5 <[email protected]>
---
.../service/rest/IcebergConfigOperations.java | 19 +++++--
...bergMetadataAuthorizationMethodInterceptor.java | 12 ++++-
.../filter/IcebergRESTAuthInterceptionService.java | 2 +
...bergMetadataAuthorizationMethodInterceptor.java | 60 +++++++++++++++++++++-
4 files changed, 87 insertions(+), 6 deletions(-)
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergConfigOperations.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergConfigOperations.java
index 719d75ce93..a96ee2b843 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergConfigOperations.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergConfigOperations.java
@@ -38,11 +38,15 @@ import javax.ws.rs.core.Context;
import javax.ws.rs.core.MediaType;
import javax.ws.rs.core.Response;
import org.apache.commons.lang3.StringUtils;
-import org.apache.gravitino.catalog.lakehouse.iceberg.IcebergConstants;
+import org.apache.gravitino.Entity;
+import org.apache.gravitino.MetadataObject;
import org.apache.gravitino.iceberg.service.CatalogWrapperForREST;
import org.apache.gravitino.iceberg.service.IcebergCatalogWrapperManager;
import org.apache.gravitino.iceberg.service.IcebergRESTUtils;
import org.apache.gravitino.metrics.MetricNames;
+import
org.apache.gravitino.server.authorization.annotations.AuthorizationExpression;
+import
org.apache.gravitino.server.authorization.annotations.AuthorizationMetadata;
+import
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants;
import org.apache.iceberg.rest.Endpoint;
import org.apache.iceberg.rest.responses.ConfigResponse;
@@ -98,7 +102,14 @@ public class IcebergConfigOperations {
@Produces(MediaType.APPLICATION_JSON)
@Timed(name = "config." + MetricNames.HTTP_PROCESS_DURATION, absolute = true)
@ResponseMetered(name = "config", absolute = true)
- public Response getConfig(@DefaultValue("") @QueryParam("warehouse") String
warehouse) {
+ @AuthorizationExpression(
+ expression =
AuthorizationExpressionConstants.LOAD_CATALOG_AUTHORIZATION_EXPRESSION,
+ accessMetadataType = MetadataObject.Type.CATALOG)
+ public Response getConfig(
+ @AuthorizationMetadata(type = Entity.EntityType.CATALOG)
+ @DefaultValue("")
+ @QueryParam("warehouse")
+ String warehouse) {
String catalogName = getCatalogName(warehouse);
boolean supportsView = supportsViewOperations(catalogName);
ConfigResponse.Builder builder = ConfigResponse.builder();
@@ -126,7 +137,9 @@ public class IcebergConfigOperations {
private String getCatalogName(String warehouse) {
if (StringUtils.isBlank(warehouse)) {
- return IcebergConstants.ICEBERG_REST_DEFAULT_CATALOG;
+ // The same default catalog an empty prefix resolves to, and that the
request is authorized
+ // against.
+ return IcebergRESTUtils.getCatalogName(warehouse);
} else {
return warehouse;
}
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/IcebergMetadataAuthorizationMethodInterceptor.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/IcebergMetadataAuthorizationMethodInterceptor.java
index f7667247a5..baeb09901d 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/IcebergMetadataAuthorizationMethodInterceptor.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/IcebergMetadataAuthorizationMethodInterceptor.java
@@ -26,6 +26,7 @@ import java.util.Map;
import java.util.Optional;
import org.aopalliance.intercept.MethodInterceptor;
import org.aopalliance.intercept.MethodInvocation;
+import org.apache.commons.lang3.StringUtils;
import org.apache.gravitino.Entity;
import org.apache.gravitino.Entity.EntityType;
import org.apache.gravitino.NameIdentifier;
@@ -93,7 +94,11 @@ public class IcebergMetadataAuthorizationMethodInterceptor
String value = String.valueOf(args[i]);
switch (type) {
case CATALOG:
- catalog = IcebergRESTUtils.getCatalogName(value);
+ // A path prefix ends with '/'; the config endpoint's warehouse is
a plain catalog name.
+ catalog =
+ StringUtils.isBlank(value) || value.endsWith("/")
+ ? IcebergRESTUtils.getCatalogName(value)
+ : value;
nameIdentifierMap.put(
Entity.EntityType.CATALOG,
NameIdentifierUtil.ofCatalog(metalakeName, catalog));
break;
@@ -206,7 +211,10 @@ public class IcebergMetadataAuthorizationMethodInterceptor
IcebergCatalogWrapper catalogWrapper;
try {
catalogWrapper = wrapperManager.getCatalogWrapper(catalogId.name());
- } catch (NoSuchCatalogException e) {
+ } catch (NoSuchCatalogException | IllegalArgumentException e) {
+ // A missing catalog, or a catalog that is not an Iceberg catalog, is
not a REST backend:
+ // authorize the request normally. A caller without access gets 403
whatever the name
+ // refers to, and an authorized caller gets the operation's own error.
return false;
}
// When IRC2 is another Gravitino server, IRC1 acts as a proxy and does
not perform
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/IcebergRESTAuthInterceptionService.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/IcebergRESTAuthInterceptionService.java
index b7ea32d820..9c2e14de79 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/IcebergRESTAuthInterceptionService.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/IcebergRESTAuthInterceptionService.java
@@ -23,6 +23,7 @@ import com.google.common.collect.ImmutableSet;
import java.lang.reflect.Method;
import java.util.List;
import org.aopalliance.intercept.MethodInterceptor;
+import org.apache.gravitino.iceberg.service.rest.IcebergConfigOperations;
import org.apache.gravitino.iceberg.service.rest.IcebergNamespaceOperations;
import org.apache.gravitino.iceberg.service.rest.IcebergTableOperations;
import org.apache.gravitino.iceberg.service.rest.IcebergTableRenameOperations;
@@ -41,6 +42,7 @@ public class IcebergRESTAuthInterceptionService extends
BaseInterceptionService
public Filter getDescriptorFilter() {
return new ClassListFilter(
ImmutableSet.of(
+ IcebergConfigOperations.class.getName(),
IcebergTableOperations.class.getName(),
IcebergTableRenameOperations.class.getName(),
IcebergNamespaceOperations.class.getName(),
diff --git
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/server/web/filter/TestIcebergMetadataAuthorizationMethodInterceptor.java
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/server/web/filter/TestIcebergMetadataAuthorizationMethodInterceptor.java
index abad7e0dd3..63ade84bd6 100644
---
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/server/web/filter/TestIcebergMetadataAuthorizationMethodInterceptor.java
+++
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/server/web/filter/TestIcebergMetadataAuthorizationMethodInterceptor.java
@@ -43,9 +43,11 @@ import
org.apache.gravitino.iceberg.service.IcebergCatalogWrapperManager;
import org.apache.gravitino.iceberg.service.IcebergRESTUtils;
import
org.apache.gravitino.iceberg.service.authorization.IcebergRESTServerContext;
import org.apache.gravitino.iceberg.service.provider.IcebergConfigProvider;
+import org.apache.gravitino.iceberg.service.rest.IcebergConfigOperations;
import org.apache.gravitino.iceberg.service.rest.IcebergTableOperations;
import
org.apache.gravitino.server.authorization.annotations.AuthorizationExpression;
import
org.apache.gravitino.server.authorization.annotations.AuthorizationMetadata;
+import
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants;
import org.apache.gravitino.utils.NameIdentifierUtil;
import org.apache.iceberg.catalog.Catalog;
import org.apache.iceberg.catalog.Namespace;
@@ -54,6 +56,8 @@ import org.apache.iceberg.rest.RESTCatalog;
import org.apache.iceberg.rest.RESTUtil;
import org.apache.iceberg.rest.requests.ReportMetricsRequest;
import org.apache.iceberg.rest.responses.ErrorResponse;
+import org.glassfish.hk2.api.ActiveDescriptor;
+import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestInstance;
@@ -297,6 +301,58 @@ public class
TestIcebergMetadataAuthorizationMethodInterceptor {
Mockito.verify(invocation, Mockito.times(1)).proceed();
}
+ @Test
+ public void testNonIcebergCatalogDoesNotSkipAuthorization() {
+ // A prefix naming a catalog of another kind is authorized normally
instead of failing with
+ // an internal error inside the authorization step.
+ IcebergCatalogWrapperManager wrapperManager =
Mockito.mock(IcebergCatalogWrapperManager.class);
+ Mockito.when(wrapperManager.getCatalogWrapper("glue_catalog"))
+ .thenThrow(new IllegalArgumentException("Catalog glue_catalog is not
an Iceberg catalog"));
+ resetContext(wrapperManager, true);
+
+ IcebergMetadataAuthorizationMethodInterceptor interceptor =
+ new IcebergMetadataAuthorizationMethodInterceptor();
+ assertFalse(
+ interceptor.shouldSkipAuthorization(
+ Map.of(Entity.EntityType.CATALOG, NameIdentifier.of(TEST_METALAKE,
"glue_catalog"))));
+ }
+
+ @Test
+ public void testConfigEndpointIsAuthorizedOnTheWarehouseCatalog() throws
Exception {
+ Assertions.assertTrue(
+ new IcebergRESTAuthInterceptionService()
+ .getDescriptorFilter()
+ .matches(descriptorFor(IcebergConfigOperations.class)));
+
+ Method getConfig = IcebergConfigOperations.class.getMethod("getConfig",
String.class);
+ AuthorizationExpression expression =
getConfig.getAnnotation(AuthorizationExpression.class);
+ assertNotNull(expression);
+ assertEquals(
+ AuthorizationExpressionConstants.LOAD_CATALOG_AUTHORIZATION_EXPRESSION,
+ expression.expression());
+
+ IcebergMetadataAuthorizationMethodInterceptor interceptor =
+ new IcebergMetadataAuthorizationMethodInterceptor();
+ // The warehouse names the catalog; an empty warehouse means the default
catalog.
+ assertEquals(
+ NameIdentifierUtil.ofCatalog(TEST_METALAKE, "other_catalog"),
+ interceptor
+ .extractNameIdentifierFromParameters(
+ getConfig.getParameters(), new Object[] {"other_catalog"})
+ .get(Entity.EntityType.CATALOG));
+ assertEquals(
+ NameIdentifierUtil.ofCatalog(TEST_METALAKE, TEST_CATALOG),
+ interceptor
+ .extractNameIdentifierFromParameters(getConfig.getParameters(),
new Object[] {""})
+ .get(Entity.EntityType.CATALOG));
+ }
+
+ private static ActiveDescriptor<?> descriptorFor(Class<?> clazz) {
+ ActiveDescriptor<?> descriptor = Mockito.mock(ActiveDescriptor.class);
+ Mockito.when(descriptor.getImplementation()).thenReturn(clazz.getName());
+ return descriptor;
+ }
+
@Test
public void testMetricsReportDoesNotSkipAuthorizationForRestCatalog() throws
Exception {
IcebergCatalogWrapperManager wrapperManager =
Mockito.mock(IcebergCatalogWrapperManager.class);
@@ -472,8 +528,10 @@ public class
TestIcebergMetadataAuthorizationMethodInterceptor {
public void
testInvokeShouldSkipAuthorizationWrapperLookupFailureReturnsAuthInternalError()
throws Throwable {
IcebergCatalogWrapperManager wrapperManager =
Mockito.mock(IcebergCatalogWrapperManager.class);
+ // An unexpected lookup failure is a server error. (An
IllegalArgumentException, such as a
+ // prefix naming a catalog that is not an Iceberg catalog, is authorized
normally instead.)
Mockito.when(wrapperManager.getCatalogWrapper(TEST_CATALOG))
- .thenThrow(new IllegalArgumentException("wrapper lookup failed"));
+ .thenThrow(new IllegalStateException("wrapper lookup failed"));
resetContext(wrapperManager, true);
Method method =
