yuqi1129 commented on code in PR #11674:
URL: https://github.com/apache/gravitino/pull/11674#discussion_r3419792755
##########
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:
Resolved by refactoring: the hand-written entries were removed in favor of
reusing the shared definitions from the `core` module
(`S3PropertiesMetadata.PROPERTY_ENTRIES`, etc., introduced in #11264), which
already use clear and consistent descriptions like "S3 access key ID". This
also keeps the wording consistent with the Hive/Iceberg/Glue catalogs.
##########
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:
Resolved by refactoring: the hand-written map (where each key was repeated)
was removed; the metadata now `putAll` the shared
`*PropertiesMetadata.PROPERTY_ENTRIES` maps from `core`, so the key is defined
in exactly one place, eliminating the mismatch footgun.
##########
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:
Good catch. Extended the non-credential visibility check to all three
metadata via a shared `assertNonCredentialPropertiesVisible(metadata)` helper,
now asserted for catalog, schema and fileset metadata.
##########
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:
Fixed: now splitting the provider list on `,` and checking exact token
membership (`Arrays.asList(value.split(",")).contains(...)`) instead of
`String.contains`, avoiding substring false positives.
--
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]
