jerryshao commented on code in PR #13539:
URL: https://github.com/apache/gravitino/pull/13539#discussion_r4120964999
##########
core/src/main/java/org/apache/gravitino/secret/SecretPropertyUtils.java:
##########
@@ -184,9 +174,23 @@ public static Map<String, String> buildSecrets(
if (key == null || value == null) {
continue;
}
- if (isSecretProperty(key, value)) {
+ if (CredentialPropertyKeys.isCredentialPropertyKey(key)) {
+ continue;
+ }
Review Comment:
[Important] This skip is unconditional across entity types, but the
replacement recovery path exists only for catalogs — so schema-level and
fileset-level static cloud credentials become unrecoverable.
The three pieces:
1. `CloudStorageCredentialPropertyKeys.STATIC_CREDENTIAL_KEYS` now also
strips the access key IDs
(`catalogs/catalog-common/src/main/java/org/apache/gravitino/storage/CloudStorageCredentialPropertyKeys.java:56-65`),
and `omitStaticCredentialProperties` runs for **every** level in
`putPropsAndSecrets`
(`clients/filesystem-hadoop3/src/main/java/org/apache/gravitino/filesystem/hadoop/BaseGVFSOperations.java:974-985`).
2. This `continue` makes `schema.getSecrets()` and `fileset.getSecrets()`
drop those keys too — `buildSecrets` has no notion of which entity it is
serving.
3. The replacement, `putCredentialInfo`, is called only for the catalog
(`BaseGVFSOperations.java:978`; the Python client does the same at
`clients/client-python/gravitino/filesystem/gvfs_base_operations.py:521`).
And the server cannot vend a schema/fileset-level value even if the client
asked: every provider is resolved from
`baseCatalog.catalogCredentialManager()`, which is constructed once from
`BaseCatalog.propertiesWithCredentialProviders()` — the **catalog** entity's
properties
(`core/src/main/java/org/apache/gravitino/credential/CredentialOperationDispatcher.java:71-104`,
`core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java:391,531-544`).
The path-based branch at `CredentialOperationDispatcher.java:97-100` picks the
context from the path, but still the catalog's providers.
Failure scenario: a fileset catalog points at bucket A with the catalog's
`s3-access-key-id`/`s3-secret-access-key`, and one fileset overrides both to a
different pair for bucket B. Before this PR, GVFS recovered the fileset pair
through `fileset.getSecrets()`. After it, `fileset.properties()` has both keys
stripped by `omitStaticCredentialProperties`, `fileset.getSecrets()` returns
neither, and only the catalog-level pair is merged — so that fileset silently
reads with the wrong credentials (or fails). Fileset/schema overrides are a
supported configuration:
`catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetPropertiesMetadata.java:78`
and `FilesetSchemaPropertiesMetadata.java:71` both register
`CloudPropertiesMetadata.STORAGE_PROPERTY_ENTRIES`.
Suggestion: either keep `buildSecrets` recovering these keys for non-catalog
entities (the credential path only claims to cover catalogs), or gate the skip
on the entity type and add a GVFS test that sets the pair on the fileset.
Verified by: read `SecretPropertyUtils.buildSecrets` 162-198 on this head;
`CredentialOperationDispatcher.getCredentials`/`getCredentialContexts` 65-104;
`BaseCatalog.propertiesWithCredentialProviders` 531-544 and the
`catalogCredentialManager()` construction at 388-393;
`BaseGVFSOperations.getAllProperties`/`putPropsAndSecrets`/`putCredentialInfo`
966-1019; both fileset properties-metadata classes. No test in this PR sets a
static cloud pair below the catalog.
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/provider/DynamicIcebergConfigProvider.java:
##########
@@ -133,16 +132,20 @@ private static Map<String, String> resolveProps(Catalog
catalog) {
} catch (UnsupportedOperationException ignored) {
// Catalog does not support secret property operations.
}
- if (catalog instanceof SupportsCredentials) {
- Arrays.stream(((SupportsCredentials) catalog).getCredentials())
- .filter(c -> c instanceof JdbcCredential)
- .map(c -> (JdbcCredential) c)
- .findFirst()
- .ifPresent(
- jdbc -> {
- props.put(IcebergConstants.GRAVITINO_JDBC_USER,
jdbc.jdbcUser());
- props.put(IcebergConstants.GRAVITINO_JDBC_PASSWORD,
jdbc.jdbcPassword());
- });
+ try {
+ SupportsCredentials supportsCredentials = catalog.supportsCredentials();
+ if (supportsCredentials != null) {
+ Credential[] credentials = supportsCredentials.getCredentials();
+ if (credentials != null) {
+ for (Credential credential : credentials) {
+ if (credential != null && credential.credentialInfo() != null) {
+ props.putAll(credential.credentialInfo());
Review Comment:
[Important] This loop dropped the `instanceof JdbcCredential` filter that
the previous code had, so expiring credentials are now written into
configuration that is built once and cached.
The code being replaced narrowed the merge to `JdbcCredential` — a
credential whose `expireTimeInMs()` is 0. The new loop merges every credential,
including `S3TokenCredential` / `OSSTokenCredential` / `GCSTokenCredential`,
whose `expireTimeInMs()` is non-zero by construction
(`api/src/main/java/org/apache/gravitino/credential/S3TokenCredential.java:73-75,126-133`).
Failure scenario: an Iceberg catalog with `credential-providers=s3-token`.
`getIcebergCatalogConfig` bakes the vended session keys into the
`IcebergConfig`, and `IcebergCatalogWrapperManager` caches the wrapper built
from it
(`iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergCatalogWrapperManager.java:120-125`).
Once the token expires (default an hour), every request served from that cache
entry fails with an expired-token error until the entry is evicted — the
wrapper has no way to re-vend. Before this PR the static-only filter made that
impossible.
The same pattern, without the filter that used to be here, is now in Trino
(`trino-connector/.../CatalogConnectorManager.java:1098-1106`, resolved once
per `createCatalogConnectorContext` at line 993 for a connector that then lives
as long as the catalog), Spark
(`spark-connector/.../catalog/BaseCatalog.java:737-747`), Flink
(`flink-connector/.../utils/PropertyUtils.java:90-105`, where
`GravitinoCatalogStore` persists the result into the Flink catalog store), and
both GVFS clients.
Suggestion: skip credentials with `expireTimeInMs() != 0` at these merge
sites — they are the ones with a refresh path of their own
(`DefaultGravitinoFileSystemCredentialsProvider` for GVFS) and must not be
frozen into static config. Restricting the merge to keys in
`CredentialPropertyKeys` would also drop `s3-session-token`, which none of
these consumers refresh.
Verified by: read this method 110-150 on this head and `git diff
dc4c2a9...HEAD` for the removed `filter(c -> c instanceof JdbcCredential)`;
read `S3TokenCredential` and `DlfSecretKeyCredential` (`expireTimeInMs()` 0,
validated at line 147) to confirm which credential types expire; read the
wrapper cache in `IcebergCatalogWrapperManager` and the single call site of
`withResolvedSecrets` in `CatalogConnectorManager`.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogConnectorManager.java:
##########
@@ -1058,33 +1059,64 @@ static Map<String, String> visibleProps(Catalog
catalog) {
}
/**
- * Overlays the secrets the Gravitino server vends for this catalog onto its
properties.
+ * Overlays secrets and credential info the Gravitino server vends for this
catalog onto its
+ * properties.
*
* <p>Resolved here, on the node that is about to build the connector,
rather than once at
* registration time: the registered definition travels through a CREATE
CATALOG statement that
* Trino persists as a catalog properties file, and a secret placed in it
would be readable there
- * for as long as the catalog exists.
+ * for as long as the catalog exists. Cloud/JDBC credential fields come from
{@code
+ * getCredentials()}; other secrets come from {@code getSecrets()}.
*/
private GravitinoCatalog withResolvedSecrets(
GravitinoCatalog catalog, GravitinoMetalake metalake) {
- Map<String, String> secrets;
+ Catalog loaded;
try {
- secrets =
metalake.loadCatalog(catalog.getName()).supportsSecrets().getSecrets();
+ loaded = metalake.loadCatalog(catalog.getName());
} catch (Exception e) {
- // Named explicitly: the caller's message only says the connector could
not be created, and
- // this step is the one that needs the Gravitino server reachable from
this node.
throw new TrinoException(
GravitinoErrorCode.GRAVITINO_OPERATION_FAILED,
String.format(
"Failed to resolve the secrets of catalog %s in metalake %s: %s",
catalog.getName(), catalog.getMetalake(), toErrorMessage(e)),
e);
}
- if (secrets.isEmpty()) {
+ Map<String, String> properties = new HashMap<>(catalog.getProperties());
+ try {
+ Map<String, String> secrets = loaded.supportsSecrets().getSecrets();
+ if (secrets != null && !secrets.isEmpty()) {
+ properties.putAll(secrets);
+ }
+ } catch (Exception e) {
+ throw new TrinoException(
+ GravitinoErrorCode.GRAVITINO_OPERATION_FAILED,
+ String.format(
+ "Failed to resolve the secrets of catalog %s in metalake %s: %s",
+ catalog.getName(), catalog.getMetalake(), toErrorMessage(e)),
+ e);
+ }
+ try {
+ Credential[] credentials = loaded.supportsCredentials().getCredentials();
+ if (credentials != null) {
+ for (Credential credential : credentials) {
+ if (credential != null && credential.credentialInfo() != null) {
+ properties.putAll(credential.credentialInfo());
+ }
+ }
+ }
+ } catch (UnsupportedOperationException ignored) {
+ // Catalog does not support credential vending.
+ } catch (Exception e) {
Review Comment:
[Question] Was it intended that a missing or failing `/credentials` is fatal
to connector creation here, when the Flink equivalent tolerates it?
Only `UnsupportedOperationException` is swallowed; everything else becomes a
`TrinoException` and the connector fails to build. The Flink version added in
the same commit catches `UnsupportedOperationException | NotFoundException |
RESTException` and carries on with whatever overlays succeeded
(`flink-connector/flink-common/src/main/java/org/apache/gravitino/flink/connector/utils/PropertyUtils.java:100-104`),
and its javadoc explicitly calls out "older Gravitino servers".
If a Trino node points at a server that does not serve `/credentials`, or
the call fails transiently, catalogs that previously built fine (they only
needed `/secrets`) now fail outright. If `/credentials` is considered old
enough to always be present, that is a reasonable answer — but then the
asymmetry with Flink is worth removing in one direction or the other.
Verified by: read `withResolvedSecrets` 1071-1117 on this head and compared
with `PropertyUtils.propertiesWithSecretsAndCredentials` 78-106; the
`NotFoundException`/`RESTException` handling exists in the Flink path and not
here.
##########
api/src/main/java/org/apache/gravitino/credential/CredentialPropertyKeys.java:
##########
@@ -0,0 +1,87 @@
+/*
+ * 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.credential;
+
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Set;
+import javax.annotation.Nullable;
+
+/**
+ * Catalog entity property keys that also appear in {@link
Credential#credentialInfo()} and are
+ * delivered via {@link SupportsCredentials#getCredentials()}, not {@code
getSecrets()}.
+ *
+ * <p>Omits fields that exist only in vended credential payloads and are never
catalog properties
+ * (for example {@code s3-session-token}, {@code oss-security-token}, {@code
cos-security-token},
+ * {@code adls-sas-token}, GCS {@code token}, and AWS IRSA {@code
access-key-id} / {@code
+ * secret-access-key} / {@code session-token}).
+ */
+public final class CredentialPropertyKeys {
+
+ private static final Set<String> KEYS;
+
+ static {
+ Set<String> keys = new HashSet<>();
+ // S3 static pair (also reused as session AK/SK field names in s3-token
payloads)
+ keys.add(S3SecretKeyCredential.GRAVITINO_S3_STATIC_ACCESS_KEY_ID);
+ keys.add(S3SecretKeyCredential.GRAVITINO_S3_STATIC_SECRET_ACCESS_KEY);
+ // OSS static pair
+ keys.add(OSSSecretKeyCredential.GRAVITINO_OSS_STATIC_ACCESS_KEY_ID);
+ keys.add(OSSSecretKeyCredential.GRAVITINO_OSS_STATIC_SECRET_ACCESS_KEY);
+ // COS static pair
+ keys.add(COSSecretKeyCredential.GRAVITINO_COS_STATIC_ACCESS_KEY_ID);
+ keys.add(COSSecretKeyCredential.GRAVITINO_COS_STATIC_SECRET_ACCESS_KEY);
+ // Azure account key pair
+ keys.add(AzureAccountKeyCredential.GRAVITINO_AZURE_STORAGE_ACCOUNT_NAME);
+ keys.add(AzureAccountKeyCredential.GRAVITINO_AZURE_STORAGE_ACCOUNT_KEY);
+ // JDBC
+ keys.add(JdbcCredential.GRAVITINO_JDBC_USER);
+ keys.add(JdbcCredential.GRAVITINO_JDBC_PASSWORD);
+ // Glue AWS API credentials
+ keys.add(AwsSecretKeyCredential.GRAVITINO_AWS_ACCESS_KEY_ID);
+ keys.add(AwsSecretKeyCredential.GRAVITINO_AWS_SECRET_ACCESS_KEY);
+ // Paimon DLF (dlf-security-token is an optional catalog property, not
vended-only)
+ keys.add(DlfSecretKeyCredential.GRAVITINO_DLF_ACCESS_KEY_ID);
+ keys.add(DlfSecretKeyCredential.GRAVITINO_DLF_ACCESS_KEY_SECRET);
+ keys.add(DlfSecretKeyCredential.GRAVITINO_DLF_SECURITY_TOKEN);
+ KEYS = Collections.unmodifiableSet(keys);
Review Comment:
[Nit] This set has to be kept in sync by hand with every `Credential`
implementation's `credentialInfo()` keys, and the failure mode is silent.
If a new `Credential` type is added later with a catalog-stored key that
nobody adds here, that key keeps flowing through `getSecrets()` — which is the
exact leak this PR exists to close — and nothing fails.
`TestCredentialPropertyKeys` asserts the current membership literal-by-literal,
so it will not catch the omission either.
Suggestion: add a test that walks the registered `Credential` SPI
implementations
(`api/src/main/resources/META-INF/services/org.apache.gravitino.credential.Credential`,
which this PR extends) and asserts that each one's `credentialInfo()` keys are
either listed here or on an explicit vended-only allowlist — the exclusions the
class javadoc already enumerates (`s3-session-token`, `oss-security-token`,
IRSA keys, …). That turns the javadoc's contract into something enforced.
Verified by: read this file in full and
`api/src/test/java/org/apache/gravitino/credential/TestCredentialPropertyKeys.java:27-52`
on this head; the test enumerates keys as string literals with no reflection
over the SPI.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalog.java:
##########
@@ -91,12 +93,17 @@ public Map<String, String>
propertiesWithCredentialProviders() {
// super() skips addCatalogSpecificCredentialProviders() when
credential-providers is already
// set, so the aws-* → s3-* key mapping never runs. Apply it
unconditionally here so that
// S3SecretKeyProvider.initialize() can read s3-access-key-id regardless
of how the catalog
- // was configured.
+ // was configured. Also ensure aws-secret-key is listed so Glue API keys
remain available via
+ // getCredentials after getSecrets stopped returning them.
String accessKeyId = props.get(GlueConstants.AWS_ACCESS_KEY_ID);
String secretAccessKey = props.get(GlueConstants.AWS_SECRET_ACCESS_KEY);
if (StringUtils.isNotBlank(accessKeyId) &&
StringUtils.isNotBlank(secretAccessKey)) {
props.putIfAbsent(S3Properties.GRAVITINO_S3_ACCESS_KEY_ID, accessKeyId);
props.putIfAbsent(S3Properties.GRAVITINO_S3_SECRET_ACCESS_KEY,
secretAccessKey);
+ ensureCredentialProviderListed(props,
AwsSecretKeyCredential.AWS_SECRET_KEY_CREDENTIAL_TYPE);
+ // Remap creates s3-* keys; register s3-secret-key so getCredentials can
vend them even when
+ // credential-providers was already set (super skips auto-detect).
+ ensureCredentialProviderListed(props,
S3SecretKeyCredential.S3_SECRET_KEY_CREDENTIAL_TYPE);
Review Comment:
[Important] Force-appending the static-key providers overrides an explicit
`credential-providers`, and when a token provider is also configured the client
ends up with a mixed, unusable credential set.
`ensureCredentialProviderListed` adds `s3-secret-key` (and `aws-secret-key`)
whenever the Glue keys are present, regardless of what the operator configured.
So a Glue catalog deliberately set to `credential-providers=s3-token` now vends
**both** `s3-token` and `s3-secret-key`.
`getCredentials()` returns both, and every client merge site does
`props.putAll(credential.credentialInfo())` in array order. The order is not
defined: `CredentialUtils.getCredentialProvidersByOrder` collects into a
`Collectors.toSet()`
(`core/src/main/java/org/apache/gravitino/credential/CredentialUtils.java:68-76`),
`getCatalogCredentialContexts` re-collects into a `Collectors.toMap()` HashMap
(`core/src/main/java/org/apache/gravitino/credential/CredentialOperationDispatcher.java:105-112`),
and `getCredentials` iterates that map
(`CredentialOperationDispatcher.java:65-89`).
Failure scenario: both credentials are vended.
`S3TokenCredential.credentialInfo()` writes `s3-access-key-id`,
`s3-secret-access-key` **and** `s3-session-token`
(`api/src/main/java/org/apache/gravitino/credential/S3TokenCredential.java:33-37,78-84`);
`S3SecretKeyCredential` writes only the first two, under the same key names.
If the static credential is merged last, the client is left with the **static**
access key pair plus the **session** `s3-session-token` from the token
credential — AWS rejects that combination (the token does not belong to the
long-lived key). If the token credential is merged last, the static pair is
shadowed instead. Which one happens depends on HashMap iteration order for that
catalog's provider names.
Secondary point, independent of the ordering bug: silently extending an
explicitly-set `credential-providers` means an operator who narrowed vending to
short-lived tokens now also hands out the permanent keys to every caller who
passes `FILTER_USE_SECRET_AUTHORIZATION_EXPRESSION`. That is a policy decision
worth making explicit rather than a side effect of the masking fix.
The same unconditional force-listing is at
`catalogs/catalog-lakehouse-paimon/src/main/java/org/apache/gravitino/catalog/lakehouse/paimon/PaimonCatalog.java:107,113`,
`catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalog.java:163-167`
and
`catalogs/catalog-lakehouse-iceberg/src/main/java/org/apache/gravitino/catalog/lakehouse/iceberg/IcebergCatalog.java:117-124`
(JDBC is less exposed, since there is no token-based JDBC provider to collide
with).
Suggestion: skip the static provider when a token provider for the same
scheme is already listed, or make the client merge pick one credential per
scheme instead of blind `putAll`.
Verified by: read `GlueCatalog.propertiesWithCredentialProviders` 91-109 and
`ensureCredentialProviderListed`
(`core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java:562-582`)
on this head; traced provider ordering through
`CredentialUtils.getCredentialProvidersByOrder` and
`CredentialOperationDispatcher`; compared the key constants in
`S3TokenCredential` and `S3SecretKeyCredential`. No test in this PR configures
`credential-providers` explicitly alongside a static pair.
--
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]
