Copilot commented on code in PR #11674:
URL: https://github.com/apache/gravitino/pull/11674#discussion_r3419266310
##########
catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogPropertiesMetadata.java:
##########
@@ -122,6 +126,56 @@ public class FilesetCatalogPropertiesMetadata extends
BaseCatalogPropertiesMetad
false /* hidden */))
.build();
+ /**
+ * The cloud storage credential properties (e.g. S3/OSS access keys, GCS
service account file,
+ * Azure storage account key) that may be configured on a fileset catalog,
schema or fileset.
+ *
+ * <p>These properties are marked as {@code hidden} so that {@code
BaseCatalog.properties()} and
+ * the schema/fileset properties APIs filter them out, preventing sensitive
credentials from being
+ * exposed to clients. This is consistent with how the JDBC catalog hides
{@code jdbc-user} and
+ * {@code jdbc-password}.
+ */
+ public static final Map<String, PropertyEntry<?>>
STORAGE_CREDENTIAL_PROPERTY_ENTRIES =
+ new ImmutableMap.Builder<String, PropertyEntry<?>>()
+ .put(
+ S3Properties.GRAVITINO_S3_ACCESS_KEY_ID,
+ hiddenCredentialEntry(
+ S3Properties.GRAVITINO_S3_ACCESS_KEY_ID, "The access key id
of the S3"))
+ .put(
+ S3Properties.GRAVITINO_S3_SECRET_ACCESS_KEY,
+ hiddenCredentialEntry(
+ S3Properties.GRAVITINO_S3_SECRET_ACCESS_KEY, "The secret
access key of the S3"))
Review Comment:
The user-facing property descriptions have awkward/unclear phrasing (e.g.,
“access key id of the S3”) and inconsistent capitalization (“id” vs “ID”).
Consider rewriting these descriptions to be clearer and more consistent (e.g.,
“S3 access key ID”, “S3 secret access key”), since these strings may surface in
docs/help output even if the properties are hidden.
##########
catalogs/catalog-fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogPropertiesMetadata.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.catalog.fileset;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import org.apache.gravitino.connector.PropertiesMetadata;
+import org.apache.gravitino.storage.AzureProperties;
+import org.apache.gravitino.storage.GCSProperties;
+import org.apache.gravitino.storage.OSSProperties;
+import org.apache.gravitino.storage.S3Properties;
+import org.junit.jupiter.api.Test;
+
+class TestFilesetCatalogPropertiesMetadata {
+
+ private final FilesetCatalogPropertiesMetadata catalogMetadata =
+ new FilesetCatalogPropertiesMetadata();
+ private final FilesetSchemaPropertiesMetadata schemaMetadata =
+ new FilesetSchemaPropertiesMetadata();
+ private final FilesetPropertiesMetadata filesetMetadata = new
FilesetPropertiesMetadata();
+
+ @Test
+ void testStorageCredentialPropertiesAreHidden() {
+ assertCredentialPropertiesHidden(catalogMetadata);
+ assertCredentialPropertiesHidden(schemaMetadata);
+ assertCredentialPropertiesHidden(filesetMetadata);
+ }
+
+ @Test
+ void testNonCredentialStoragePropertiesAreNotHidden() {
+ // Connection/identity information that is not sensitive should remain
visible.
+
assertFalse(catalogMetadata.isHiddenProperty(S3Properties.GRAVITINO_S3_ENDPOINT));
+
assertFalse(catalogMetadata.isHiddenProperty(S3Properties.GRAVITINO_S3_REGION));
+
assertFalse(catalogMetadata.isHiddenProperty(OSSProperties.GRAVITINO_OSS_ENDPOINT));
+ }
Review Comment:
This test only checks non-credential visibility for `catalogMetadata`, but
the PR’s behavior applies to schema/fileset metadata as well. To better lock in
the intended contract, also assert the same non-credential keys are not hidden
for `schemaMetadata` and `filesetMetadata`.
##########
catalogs/catalog-fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogCredential.java:
##########
@@ -74,4 +76,40 @@ void testLocationPrefixNotUserConfigurable() {
.propertiesWithCredentialProviders()
.get(CredentialConstants.S3_CREDENTIAL_LIST_LOCATION_PREFIX));
}
+
+ @Test
+ void testStaticCredentialsHiddenFromCatalogProperties() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(S3Properties.GRAVITINO_S3_ACCESS_KEY_ID, "ak");
+ properties.put(S3Properties.GRAVITINO_S3_SECRET_ACCESS_KEY, "sk");
+ properties.put(S3Properties.GRAVITINO_S3_ENDPOINT,
"https://s3.example.com";);
+ FilesetCatalogImpl catalog = newCatalog(properties);
+
+ Map<String, String> exposed = catalog.properties();
+ // Sensitive credentials are filtered out from the outward-facing
properties.
+
Assertions.assertFalse(exposed.containsKey(S3Properties.GRAVITINO_S3_ACCESS_KEY_ID));
+
Assertions.assertFalse(exposed.containsKey(S3Properties.GRAVITINO_S3_SECRET_ACCESS_KEY));
+ // Non-sensitive connection info remains visible.
+ Assertions.assertEquals(
+ "https://s3.example.com";,
exposed.get(S3Properties.GRAVITINO_S3_ENDPOINT));
+ }
+
+ @Test
+ void testStaticCredentialsStillAvailableForCredentialVending() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(S3Properties.GRAVITINO_S3_ACCESS_KEY_ID, "ak");
+ properties.put(S3Properties.GRAVITINO_S3_SECRET_ACCESS_KEY, "sk");
+ FilesetCatalogImpl catalog = newCatalog(properties);
+
+ // Hiding the credentials does not break credential vending: the raw
credentials are still
+ // available to the server-side credential manager, and the matching
credential provider is
+ // auto-injected so clients (e.g. GVFS) can obtain vended credentials.
+ Map<String, String> credProps =
catalog.propertiesWithCredentialProviders();
+ Assertions.assertEquals("ak",
credProps.get(S3Properties.GRAVITINO_S3_ACCESS_KEY_ID));
+ Assertions.assertEquals("sk",
credProps.get(S3Properties.GRAVITINO_S3_SECRET_ACCESS_KEY));
+ Assertions.assertTrue(
+ credProps
+ .get(CredentialConstants.CREDENTIAL_PROVIDERS)
+ .contains(S3SecretKeyCredential.S3_SECRET_KEY_CREDENTIAL_TYPE));
Review Comment:
Using `String.contains(...)` on the serialized credential provider list can
yield false positives (substring matches) and can be brittle if formatting
changes. Prefer asserting membership by parsing/splitting the provider list
using the expected delimiter (or a shared helper used elsewhere) and checking
exact token equality.
##########
catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogPropertiesMetadata.java:
##########
@@ -122,6 +126,56 @@ public class FilesetCatalogPropertiesMetadata extends
BaseCatalogPropertiesMetad
false /* hidden */))
.build();
+ /**
+ * The cloud storage credential properties (e.g. S3/OSS access keys, GCS
service account file,
+ * Azure storage account key) that may be configured on a fileset catalog,
schema or fileset.
+ *
+ * <p>These properties are marked as {@code hidden} so that {@code
BaseCatalog.properties()} and
+ * the schema/fileset properties APIs filter them out, preventing sensitive
credentials from being
+ * exposed to clients. This is consistent with how the JDBC catalog hides
{@code jdbc-user} and
+ * {@code jdbc-password}.
+ */
+ public static final Map<String, PropertyEntry<?>>
STORAGE_CREDENTIAL_PROPERTY_ENTRIES =
+ new ImmutableMap.Builder<String, PropertyEntry<?>>()
+ .put(
+ S3Properties.GRAVITINO_S3_ACCESS_KEY_ID,
+ hiddenCredentialEntry(
+ S3Properties.GRAVITINO_S3_ACCESS_KEY_ID, "The access key id
of the S3"))
Review Comment:
Each entry repeats the same key constant twice (once as the map key and
again as the `name` argument). This creates a maintainability footgun where a
future edit could accidentally mismatch the two. Consider introducing a small
helper (e.g., a `putHiddenCredentialEntry(builder, key, description)` method)
so the key is only specified once.
--
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]
