Copilot commented on code in PR #9785:
URL: https://github.com/apache/gravitino/pull/9785#discussion_r2787329883
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -973,15 +982,16 @@ private CatalogWrapper createCatalogWrapper(
* properties) of the catalog entity.
*
* @param entity The catalog entity.
- * @return The resolved properties.
+ * @return The resolved BaseCatalog instance.
*/
- private Map<String, String> getResolvedProperties(CatalogEntity entity) {
+ private BaseCatalog getBaseCatalog(CatalogEntity entity) {
Map<String, String> conf = entity.getProperties();
String provider = entity.getProvider();
try (IsolatedClassLoader classLoader = createClassLoader(provider, conf)) {
BaseCatalog<?> catalog = createBaseCatalog(classLoader, entity);
- return classLoader.withClassLoader(cl -> catalog.properties(),
RuntimeException.class);
+ classLoader.withClassLoader(cl -> catalog.properties(),
RuntimeException.class);
+ return catalog;
}
Review Comment:
getBaseCatalog returns a BaseCatalog instance whose IsolatedClassLoader has
been closed (try-with-resources). Returning objects whose classes come from a
closed classloader is fragile (and the caller currently invokes methods outside
a withClassLoader block). Prefer returning the resolved properties / metadata
extracted inside withClassLoader, or returning a CatalogWrapper that owns the
classloader and can be closed later.
##########
server/src/main/java/org/apache/gravitino/server/web/rest/CatalogOperations.java:
##########
@@ -73,11 +82,14 @@ public class CatalogOperations {
private final CatalogDispatcher catalogDispatcher;
+ private final boolean filterSensitiveProperties;
+
@Context private HttpServletRequest httpRequest;
@Inject
public CatalogOperations(CatalogDispatcher catalogDispatcher) {
this.catalogDispatcher = catalogDispatcher;
+ this.filterSensitiveProperties =
GravitinoEnv.getInstance().filterSensitiveProperties();
}
Review Comment:
CatalogOperations snapshots filterSensitiveProperties in the constructor via
GravitinoEnv.getInstance().filterSensitiveProperties(). This makes the behavior
depend on resource construction order (tests now have to inject
GravitinoEnv.config early) and prevents runtime config changes from taking
effect. Consider reading the flag from GravitinoEnv/config per request (or at
least null-guarding when env config is not yet initialized).
##########
server/src/main/java/org/apache/gravitino/server/web/rest/CatalogOperations.java:
##########
@@ -268,7 +296,29 @@ public Response loadCatalog(
try {
NameIdentifier ident = NameIdentifierUtil.ofCatalog(metalakeName,
catalogName);
Catalog catalog = catalogDispatcher.loadCatalog(ident);
- Response response = Utils.ok(new
CatalogResponse(DTOConverters.toDTO(catalog)));
+ CatalogDTO catalogDTO;
+ if (filterSensitiveProperties) {
+ // Only allow users with alter permission to access sensitive data in
catalog properties.
+ boolean enableAccessSensitiveData =
+ MetadataAuthzHelper.checkAccess(
+ ident,
+ Entity.EntityType.CATALOG,
+ AuthorizationExpressionConstants.CATALOG_OWNER_EXPRESSION);
Review Comment:
The comment says "alter permission" but the access check uses
CATALOG_OWNER_EXPRESSION (owner-only). Please either update the comment (and
docs) to match owner-only semantics, or change the expression used here if
alter-level access should also see sensitive properties.
##########
server/src/main/java/org/apache/gravitino/server/web/rest/CatalogOperations.java:
##########
@@ -101,16 +113,32 @@ public Response listCatalogs(
// Lock the root and the metalake with WRITE lock to ensure the
consistency of the list.
if (verbose) {
Catalog[] catalogs =
catalogDispatcher.listCatalogsInfo(catalogNS);
- catalogs =
- MetadataAuthzHelper.filterByExpression(
- metalake,
-
AuthorizationExpressionConstants.LOAD_CATALOG_AUTHORIZATION_EXPRESSION,
- Entity.EntityType.CATALOG,
- catalogs,
- (catalogEntity) ->
- NameIdentifierUtil.ofCatalog(metalake,
catalogEntity.name()));
- Response response = Utils.ok(new
CatalogListResponse(DTOConverters.toDTOs(catalogs)));
- LOG.info("List {} catalogs info under metalake: {}",
catalogs.length, metalake);
+ CatalogDTO[] catalogDTOs;
+ if (filterSensitiveProperties) {
+ MetadataAuthzHelper.FilterResult<Catalog, Catalog>
filterResult =
+ MetadataAuthzHelper.partitionByTwoExpressions(
+ metalake,
+
AuthorizationExpressionConstants.CATALOG_OWNER_EXPRESSION,
+
AuthorizationExpressionConstants.USE_CATALOG_EXPRESSION,
+ Entity.EntityType.CATALOG,
+ catalogs,
+ (catalogEntity) ->
+ NameIdentifierUtil.ofCatalog(metalake,
catalogEntity.name()));
+ // First array: catalogs with full access (can see sensitive
properties)
+ CatalogDTO[] fullAccessCatalogs =
DTOConverters.toDTOs(filterResult.getFirst());
+ // Second array: catalogs with use access only (hide sensitive
properties)
+ CatalogDTO[] limitedAccessCatalogs =
+
buildCatalogDTOsWithoutSensitiveProps(filterResult.getSecond());
+ catalogDTOs =
+ Stream.concat(
+ Arrays.stream(fullAccessCatalogs),
Arrays.stream(limitedAccessCatalogs))
+ .toArray(CatalogDTO[]::new);
Review Comment:
The current implementation concatenates "full access" catalogs first and
"limited access" catalogs second, which can reorder results compared to the
underlying catalog list. If clients rely on stable ordering, consider producing
DTOs in the original order and deciding per-catalog whether to strip sensitive
properties based on the user’s privileges.
##########
server/src/test/java/org/apache/gravitino/server/web/rest/TestTableOperations.java:
##########
@@ -119,7 +119,14 @@ public static void setup() throws IllegalAccessException {
Mockito.doReturn(100000L).when(config).get(TREE_LOCK_MAX_NODE_IN_MEMORY);
Mockito.doReturn(1000L).when(config).get(TREE_LOCK_MIN_NODE_IN_MEMORY);
Mockito.doReturn(36000L).when(config).get(TREE_LOCK_CLEAN_INTERVAL);
+ Mockito.doReturn(false)
+ .when(config)
+ .get(org.apache.gravitino.Configs.FILTER_SENSITIVE_PROPERTIES);
+
Mockito.doReturn(false).when(config).get(org.apache.gravitino.Configs.CACHE_ENABLED);
+
Mockito.doReturn(false).when(config).get(org.apache.gravitino.Configs.ENABLE_AUTHORIZATION);
Review Comment:
Avoid using fully-qualified class names in method bodies for Configs; this
file already follows imported usage elsewhere (e.g., TestCatalogOperations
imports Configs). Add an import for org.apache.gravitino.Configs and reference
Configs.FILTER_SENSITIVE_PROPERTIES / CACHE_ENABLED / ENABLE_AUTHORIZATION
directly to match project conventions.
##########
core/src/main/java/org/apache/gravitino/credential/CredentialProviderFactory.java:
##########
@@ -44,6 +46,24 @@ public static CredentialProvider create(
}
}
+ /**
+ * Looks up all sensitive property keys from all registered credential
providers.
+ *
+ * @return A set of all sensitive property keys used by credential providers.
+ */
+ public static Set<String> lookupSensitivePropertyKeys() {
+ ClassLoader classLoader = Thread.currentThread().getContextClassLoader();
+ ServiceLoader<CredentialProvider> serviceLoader =
+ ServiceLoader.load(CredentialProvider.class, classLoader);
+
+ Set<String> allPropertyKeys = new HashSet<>();
+ for (CredentialProvider provider : serviceLoader) {
+ Set<String> providerPropertyKeys = provider.sensitivePropertyKeys();
+ allPropertyKeys.addAll(providerPropertyKeys);
+ }
Review Comment:
lookupSensitivePropertyKeys() assumes
CredentialProvider.sensitivePropertyKeys() is non-null and that providers can
be safely instantiated via ServiceLoader without initialization. To make this
utility robust, consider null-guarding the returned set and handling/isolating
ServiceLoader/provider errors so a single bad provider can’t break catalog
property metadata generation.
##########
docs/gravitino-server-config.md:
##########
@@ -260,6 +260,16 @@ Writer configuration begins with
`gravitino.audit.writer.${name}`, where `${name
Refer to [security](security/security.md) for HTTPS and authentication
configurations.
+#### Catalog security configuration
+
+| Configuration item | Description
| Default value | Required | Since version |
+|-----------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------|----------|---------------|
+| `gravitino.authorization.filterSensitiveProperties` | Whether to filter
sensitive properties (passwords, secret keys, tokens) in catalog responses.
When enabled, sensitive properties are hidden from users without appropriate
permissions (owner or alter permission). | `true` | No | 1.1.0
|
+
+:::info
+When `gravitino.authorization.filterSensitiveProperties` is set to `true`,
sensitive catalog properties such as passwords, JDBC credentials, AWS secret
keys, Azure storage account keys, and authentication tokens will be hidden in
API responses unless the user has owner or alter permissions on the catalog.
Review Comment:
The docs state that users with "owner or alter permission" can see sensitive
catalog properties, but the implementation checks only the owner expression
(ANY(OWNER, METALAKE, CATALOG)). Please align the documentation with the actual
behavior (or update the authorization expression in code if alter-level access
is intended).
```suggestion
| `gravitino.authorization.filterSensitiveProperties` | Whether to filter
sensitive properties (passwords, secret keys, tokens) in catalog responses.
When enabled, sensitive properties are hidden from users without appropriate
owner-level permissions on the catalog. | `true` | No | 1.1.0
|
:::info
When `gravitino.authorization.filterSensitiveProperties` is set to `true`,
sensitive catalog properties such as passwords, JDBC credentials, AWS secret
keys, Azure storage account keys, and authentication tokens will be hidden in
API responses unless the user has owner-level permissions on the catalog.
```
##########
server/src/main/java/org/apache/gravitino/server/web/rest/CatalogOperations.java:
##########
@@ -101,16 +113,32 @@ public Response listCatalogs(
// Lock the root and the metalake with WRITE lock to ensure the
consistency of the list.
if (verbose) {
Catalog[] catalogs =
catalogDispatcher.listCatalogsInfo(catalogNS);
- catalogs =
- MetadataAuthzHelper.filterByExpression(
- metalake,
-
AuthorizationExpressionConstants.LOAD_CATALOG_AUTHORIZATION_EXPRESSION,
- Entity.EntityType.CATALOG,
- catalogs,
- (catalogEntity) ->
- NameIdentifierUtil.ofCatalog(metalake,
catalogEntity.name()));
- Response response = Utils.ok(new
CatalogListResponse(DTOConverters.toDTOs(catalogs)));
- LOG.info("List {} catalogs info under metalake: {}",
catalogs.length, metalake);
+ CatalogDTO[] catalogDTOs;
+ if (filterSensitiveProperties) {
+ MetadataAuthzHelper.FilterResult<Catalog, Catalog>
filterResult =
+ MetadataAuthzHelper.partitionByTwoExpressions(
+ metalake,
+
AuthorizationExpressionConstants.CATALOG_OWNER_EXPRESSION,
+
AuthorizationExpressionConstants.USE_CATALOG_EXPRESSION,
+ Entity.EntityType.CATALOG,
+ catalogs,
+ (catalogEntity) ->
+ NameIdentifierUtil.ofCatalog(metalake,
catalogEntity.name()));
+ // First array: catalogs with full access (can see sensitive
properties)
+ CatalogDTO[] fullAccessCatalogs =
DTOConverters.toDTOs(filterResult.getFirst());
+ // Second array: catalogs with use access only (hide sensitive
properties)
+ CatalogDTO[] limitedAccessCatalogs =
+
buildCatalogDTOsWithoutSensitiveProps(filterResult.getSecond());
+ catalogDTOs =
+ Stream.concat(
+ Arrays.stream(fullAccessCatalogs),
Arrays.stream(limitedAccessCatalogs))
+ .toArray(CatalogDTO[]::new);
+ } else {
+ // If filtering is disabled, return all catalogs with full
properties
+ catalogDTOs = DTOConverters.toDTOs(catalogs);
+ }
Review Comment:
In the verbose listCatalogs path, authorization filtering is skipped
entirely when filterSensitiveProperties is false (it returns DTOs for all
catalogs). Disabling sensitive-property filtering should not bypass access
control; catalogs should still be filtered by the same authorization expression
(e.g., LOAD_CATALOG_AUTHORIZATION_EXPRESSION / owner-or-use) and only toggle
whether sensitive keys are removed.
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -357,7 +360,13 @@ public Catalog[] listCatalogsInfo(Namespace namespace)
throws NoSuchMetalakeExce
// provider is "fileset", still using "hadoop" will lead to catalog
loading issue. So
// after reading the catalog entity, we convert it to the new
fileset catalog entity.
.map(this::convertFilesetCatalogEntity)
- .map(e -> e.toCatalogInfoWithResolvedProps(getResolvedProperties(e)))
+ .map(
+ e -> {
+ BaseCatalog catalog = getBaseCatalog(e);
+ return new CatalogInfoWithPropertyMeta(
+ e.toCatalogInfoWithResolvedProps(catalog.properties()),
+ catalog.catalogPropertiesMetadata());
+ })
Review Comment:
listCatalogsInfo now builds CatalogInfoWithPropertyMeta using a BaseCatalog
created from an IsolatedClassLoader that is closed immediately after
getBaseCatalog returns. The returned BaseCatalog is then accessed outside the
isolated classloader context (properties()/catalogPropertiesMetadata()), which
can break when catalog loading is isolated and provider classes/resources are
needed. Consider extracting the needed properties + PropertiesMetadata within
classLoader.withClassLoader and return plain data (or keep the
wrapper/classloader alive) instead of returning the catalog instance.
--
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]
