jerryshao commented on code in PR #13539:
URL: https://github.com/apache/gravitino/pull/13539#discussion_r4119658072
##########
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:
[Important] This contract holds only while `access` is in the active
sensitive-key keyword list, and masking and recovery are gated on different
things.
`HiddenPropertyMaskUtils.classifyHiddenProperties` masks on the declared
`hidden` flag alone
(`core/src/main/java/org/apache/gravitino/connector/HiddenPropertyMaskUtils.java:108-116`),
but `shouldRecoverSensitiveNamedSecret` (line 106-112 of this file) returns
`false` unless `isSensitivePropertyKey(key)` matches first.
`gravitino.secret.sensitiveKeyKeywords` *replaces* the default keyword set, and
its own doc invites operators to "set a shorter list to stop masking keys that
only match a default keyword"
(`core/src/main/java/org/apache/gravitino/Configs.java:670-682`).
Failure scenario: an operator sets `gravitino.secret.sensitiveKeyKeywords`
without `access`. Before this PR that changed nothing for access key IDs; after
it, every key flipped here (`s3-access-key-id`, `oss-access-key-id`,
`cos-access-key-id`, `aws-access-key-id`, `dlf-access-key-id`) is still masked
as `******` by the `hidden` flag, but is no longer returned by `getSecrets()`.
GVFS, Spark and Trino then drop the identifier half with no recovery path and
fall back to the default credential chain or fail. `HiddenPropertyMaskUtils`'s
class javadoc at lines 48-51 documents exactly this dead end ("A property that
is merely declared hidden ... remains `******` after merging").
Suggestion: in `buildSecrets`, recover declared-`hidden` keys regardless of
keyword matching (or exempt declared static-credential keys from the keyword
gate), and add a test that configures a keyword list without `access`.
Not blocking under the default configuration, and the secret halves already
sit in the same trap (`s3-secret-access-key` needs `secret` to stay in the
list), but this PR widens the exposure from one half of the pair to both.
Verified by: read `SecretPropertyUtils.java:76-115` and `buildSecrets` at
175-195, `HiddenPropertyMaskUtils.java:48-51,104-116`, `Configs.java:670-682`.
`core/src/test/java/org/apache/gravitino/secret/TestSensitivePropertyKeyMatcher.java:50`
already asserts `isSensitivePropertyKey("aws-access-key-id")` is `false` once
the keyword list is replaced.
##########
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:
[Question] This comment states the policy as "the identifier half of a
static credential pair is hidden", which now holds for S3/OSS/COS/AWS/DLF.
`azure-storage-account-name` is the remaining exception: it is the identifier
half of `azure-storage-account-key` and is still declared `false /* hidden */`
at
`core/src/main/java/org/apache/gravitino/cloud/storage/AzurePropertiesMetadata.java:34-40`,
so it stays cleartext on load/list.
Is that deliberate — e.g. because the account name is already part of the
`abfss://[email protected]` URI and so is not a disclosure
— or was Azure simply missed? If deliberate, it would be worth one clause here
saying so, since the next reader will apply this comment's rule to Azure and
find it inconsistent.
Verified by: read `AzurePropertiesMetadata.java:30-50` on this head; `git
diff origin/main...HEAD` does not touch that file.
`TestFilesetCloudPropertiesMetadata.java:60` (updated in this PR) asserts only
the Azure *key* is hidden, leaving the name unasserted.
##########
core/src/main/java/org/apache/gravitino/cloud/storage/S3PropertiesMetadata.java:
##########
@@ -38,7 +38,7 @@ public class S3PropertiesMetadata {
"S3 access key ID",
false /* immutable */,
null /* defaultValue */,
- false /* hidden */))
+ true /* hidden */))
Review Comment:
[Nit] This flip is a user-facing read-path change for `s3-access-key-id`,
but the fileset storage docs still present the key with no indication that
load/list now returns `******`: `docs/fileset-catalog-with-s3.md:49`,
`docs/fileset-catalog-with-oss.md:45`, `docs/fileset-catalog-with-cos.md:46`.
`docs/aws-glue-catalog.md:58` already carries the note ("Sensitive catalog
properties ... are hidden from the default load catalog response. Retrieve ...
via `getSecrets`"), so copying that admonition into the three fileset pages
would keep them in step. Only `design-docs/gravitino-glue-catalog.md` was
updated in this PR.
Verified by: grepped `docs/*.md` for the five key names and for
`getSecrets`/`******` on this head; the three fileset pages contain neither.
--
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]
