lasdf1234 commented on code in PR #13539:
URL: https://github.com/apache/gravitino/pull/13539#discussion_r4120618027
##########
core/src/main/java/org/apache/gravitino/secret/SecretPropertyUtils.java:
##########
@@ -89,8 +89,10 @@ public static boolean isSensitivePropertyKey(@Nullable
String key) {
* <li>{@code metadata == null}: do <strong>not</strong> recover
(URN-only). Used when the
* catalog does not expose properties metadata for the entity type.
* <li>otherwise: recover only undeclared keys or declared {@code hidden}
keys. Declared
- * non-hidden configuration (for example {@code credential-providers},
{@code
- * s3-access-key-id}) stays in {@code properties()} and is excluded
here.
+ * non-hidden configuration (for example {@code credential-providers})
stays in {@code
+ * properties()} and is excluded here. Declared hidden static access
key IDs (for example
+ * {@code s3-access-key-id}) are included in {@code getSecrets()} when
present as inline
+ * plaintext.
Review Comment:
Good catch. Fixed in later commits on this PR.
buildSecrets now recovers declared-hidden keys without the sensitive-keyword
gate, so shortening gravitino.secret.sensitiveKeyKeywords (e.g. dropping
access) no longer leaves masked properties with no recovery path. Undeclared
keys still use the keyword matcher.
Separately, catalog property keys that also appear in
Credential.credentialInfo() (CredentialPropertyKeys) are no longer returned by
getSecrets; clients recover them via getCredentials (and we ensure
catalog-specific providers stay listed when credential-providers is set
explicitly).
Added coverage for a shortened keyword list without access / password /
secret in TestSecretPropertyUtils.
##########
catalogs/catalog-common/src/main/java/org/apache/gravitino/storage/CloudStorageCredentialPropertyKeys.java:
##########
@@ -28,22 +28,23 @@
/**
* Gravitino property keys for cloud static <em>secret</em> credentials.
*
- * <p>GVFS must not consume these keys from REST catalog/schema/fileset {@code
properties()}
- * responses (which may be masked as {@code ******}). Secret plaintext is
recovered via {@code
- * getSecrets()}. Non-secret identifiers such as {@code s3-access-key-id} /
{@code
- * gcs-service-account-file} remain in {@code properties()} and are merged
normally. Clients may
- * also supply credentials via local Hadoop {@code Configuration} or {@code
getCredentials()} when
- * credential vending is enabled.
+ * <p>GVFS must not consume static <em>secret</em> keys from REST
catalog/schema/fileset {@code
+ * properties()} responses (which may be masked as {@code ******}). Plaintext
for hidden static
+ * credentials, including {@code s3-access-key-id}, is recovered via {@code
getSecrets()}. Masked
+ * placeholders are dropped by {@link #omitStaticCredentialProperties} so
clients must merge {@code
+ * getSecrets()} (or local Hadoop {@code Configuration}, or {@code
getCredentials()} when credential
+ * vending is enabled). Non-credential configuration such as {@code
gcs-service-account-file} (path,
+ * not a secret half) may still appear in {@code properties()}.
*/
public final class CloudStorageCredentialPropertyKeys {
/** Placeholder returned for masked hidden properties in REST responses. */
public static final String MASKED_PROPERTY_VALUE = "******";
/**
- * Secret-bearing static credential keys only. Access key IDs and GCS
service-account file paths
- * are intentionally excluded: they are declared non-hidden and must stay
available from {@code
- * properties()}.
+ * Secret-bearing static credential keys only. Access key IDs are declared
{@code hidden} and are
+ * not listed here; they are omitted when masked as {@code ******} and
recovered via {@code
+ * getSecrets()}. GCS service-account file paths are non-secret
configuration and pass through.
Review Comment:
Deliberate. Unlike S3/OSS/COS access key IDs, the Azure storage account name
is already disclosed in ADLS URIs
(abfss://[email protected]), so masking it on load/list
does not add confidentiality. Only azure-storage-account-key is treated as the
secret half.
Documented this in CloudStorageCredentialPropertyKeys and with a short
comment on the AzurePropertiesMetadata entry.
--
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]
