diqiu50 commented on code in PR #13599: URL: https://github.com/apache/gravitino/pull/13599#discussion_r4140791020
########## core/src/main/java/org/apache/gravitino/credential/AwsSecretKeyCredentialProvider.java: ########## @@ -0,0 +1,75 @@ +/* + * 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.Map; +import javax.annotation.Nullable; +import org.apache.commons.lang3.StringUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** Generates static AWS access-key credentials for Glue API authentication. */ +public class AwsSecretKeyCredentialProvider implements CredentialProvider { + + private static final Logger LOG = LoggerFactory.getLogger(AwsSecretKeyCredentialProvider.class); + + private String accessKeyId; + private String secretAccessKey; + + @Override + public void initialize(Map<String, String> properties) { + if (properties == null) { + return; + } + this.accessKeyId = properties.get(AwsSecretKeyCredential.GRAVITINO_AWS_ACCESS_KEY_ID); + this.secretAccessKey = properties.get(AwsSecretKeyCredential.GRAVITINO_AWS_SECRET_ACCESS_KEY); + if (StringUtils.isNotBlank(accessKeyId) ^ StringUtils.isNotBlank(secretAccessKey)) { + LOG.warn( Review Comment: Auto-detection only adds this provider when both keys are present, so this branch is reached only when a user explicitly lists `aws-secret-key` with an incomplete pair. In that case we log once at init and then silently return nothing on every `getCredentials`, and the connector falls back to the default chain with an unrelated auth error. `S3SecretKeyProvider` fails loudly in the same situation. Consider `Preconditions.checkArgument` for the incomplete-pair case (keep returning null when both are absent). Same for `DlfSecretKeyCredentialProvider`. ########## catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/TestGlueCatalogCredentials.java: ########## @@ -0,0 +1,106 @@ +/* + * 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.glue; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.time.Instant; +import java.util.Map; +import org.apache.gravitino.Catalog; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.credential.AwsSecretKeyCredential; +import org.apache.gravitino.credential.CredentialConstants; +import org.apache.gravitino.credential.S3SecretKeyCredential; +import org.apache.gravitino.meta.AuditInfo; +import org.apache.gravitino.meta.CatalogEntity; +import org.apache.gravitino.storage.S3Properties; +import org.junit.jupiter.api.Test; + +public class TestGlueCatalogCredentials { + + @Test + void testAwsKeysAutoDetectAwsAndS3Providers() { + Map<String, String> props = + Map.of( + GlueConstants.AWS_ACCESS_KEY_ID, + "AKIA", + GlueConstants.AWS_SECRET_ACCESS_KEY, + "secret", + GlueConstants.AWS_REGION, + "us-east-1"); + + CatalogEntity entity = + CatalogEntity.builder() + .withId(1L) + .withName("glue") + .withNamespace(Namespace.of("metalake")) + .withType(Catalog.Type.RELATIONAL) + .withProvider("glue") + .withProperties(props) + .withAuditInfo( + AuditInfo.builder().withCreator("test").withCreateTime(Instant.now()).build()) + .build(); + + GlueCatalog catalog = new GlueCatalog().withCatalogEntity(entity).withCatalogConf(props); + Map<String, String> withProviders = catalog.propertiesWithCredentialProviders(); + + String providers = withProviders.get(CredentialConstants.CREDENTIAL_PROVIDERS); + assertTrue(providers.contains(AwsSecretKeyCredential.AWS_SECRET_KEY_CREDENTIAL_TYPE)); + assertTrue(providers.contains(S3SecretKeyCredential.S3_SECRET_KEY_CREDENTIAL_TYPE)); + assertEquals("AKIA", withProviders.get(S3Properties.GRAVITINO_S3_ACCESS_KEY_ID)); + assertEquals("secret", withProviders.get(S3Properties.GRAVITINO_S3_SECRET_ACCESS_KEY)); + assertEquals("AKIA", withProviders.get(GlueConstants.AWS_ACCESS_KEY_ID)); + } + + @Test + void testExplicitCredentialProvidersStillGetsAwsAndS3() { Review Comment: This test uses `custom-provider`, so it doesn't catch the `s3-token` case above and actually locks in appending `s3-secret-key`. Could you add a case with `credential-providers=s3-token` and assert the exact provider list (split by `,`) instead of `contains()`? ########## api/src/main/java/org/apache/gravitino/credential/AwsSecretKeyCredential.java: ########## @@ -0,0 +1,117 @@ +/* + * 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 com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableMap; +import java.util.Map; +import org.apache.commons.lang3.StringUtils; + +/** + * Static AWS access-key credential for Glue (and similar) API authentication. + * + * <p>Distinct from {@link S3SecretKeyCredential}: credential-info keys are {@code + * aws-access-key-id} / {@code aws-secret-access-key}, matching Glue catalog properties. + */ +public class AwsSecretKeyCredential implements Credential { + + /** AWS secret-key credential type. */ + public static final String AWS_SECRET_KEY_CREDENTIAL_TYPE = "aws-secret-key"; + /** AWS access key ID. */ + public static final String GRAVITINO_AWS_ACCESS_KEY_ID = "aws-access-key-id"; + /** AWS secret access key. */ + public static final String GRAVITINO_AWS_SECRET_ACCESS_KEY = "aws-secret-access-key"; + + private String accessKeyId; + private String secretAccessKey; + + /** + * Constructs an {@link AwsSecretKeyCredential}. + * + * @param accessKeyId the AWS access key ID + * @param secretAccessKey the AWS secret access key + */ + public AwsSecretKeyCredential(String accessKeyId, String secretAccessKey) { + validate(accessKeyId, secretAccessKey, 0); + this.accessKeyId = accessKeyId; + this.secretAccessKey = secretAccessKey; + } + + /** Used by the credential factory. */ + public AwsSecretKeyCredential() {} + + @Override + public String credentialType() { + return AWS_SECRET_KEY_CREDENTIAL_TYPE; + } + + @Override + public long expireTimeInMs() { + return 0; + } + + @Override + public Map<String, String> credentialInfo() { + return new ImmutableMap.Builder<String, String>() + .put(GRAVITINO_AWS_ACCESS_KEY_ID, accessKeyId) + .put(GRAVITINO_AWS_SECRET_ACCESS_KEY, secretAccessKey) + .build(); + } + + @Override + public void initialize(Map<String, String> credentialInfo, long expireTimeInMs) { + String accessKeyId = credentialInfo.get(GRAVITINO_AWS_ACCESS_KEY_ID); + String secretAccessKey = credentialInfo.get(GRAVITINO_AWS_SECRET_ACCESS_KEY); + validate(accessKeyId, secretAccessKey, expireTimeInMs); + this.accessKeyId = accessKeyId; + this.secretAccessKey = secretAccessKey; + } + + /** + * Returns the AWS access key ID. + * + * @return access key ID + */ + public String accessKeyId() { + return accessKeyId; + } + + /** + * Returns the AWS secret access key. + * + * @return secret access key + */ + public String secretAccessKey() { + return secretAccessKey; + } + + @Override + public String toString() { Review Comment: #13539 hides access key IDs, but `toString()` prints `accessKeyId`, so any `LOG.debug("{}", credential)` would leak it. `S3SecretKeyCredential` / `OSSSecretKeyCredential` don't override `toString`. Suggest dropping or masking it. Same for `DlfSecretKeyCredential.toString()`. ########## api/src/main/java/org/apache/gravitino/credential/DlfSecretKeyCredential.java: ########## @@ -0,0 +1,151 @@ +/* + * 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 com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableMap; +import java.util.Map; +import javax.annotation.Nullable; +import org.apache.commons.lang3.StringUtils; + +/** + * Static Alibaba Cloud DLF (Data Lake Formation) access-key credential for Paimon DLF catalogs. + * + * <p>Credential-info keys match Gravitino Paimon catalog properties: {@code dlf-access-key-id}, + * {@code dlf-access-key-secret}, and optionally {@code dlf-security-token}. Connectors map these to + * Paimon REST keys {@code dlf.access-key-id}, {@code dlf.access-key-secret}, and {@code + * dlf.security-token}. + */ +public class DlfSecretKeyCredential implements Credential { + + /** DLF secret-key credential type. */ + public static final String DLF_SECRET_KEY_CREDENTIAL_TYPE = "dlf-secret-key"; + /** DLF access key ID. */ + public static final String GRAVITINO_DLF_ACCESS_KEY_ID = "dlf-access-key-id"; + /** DLF access key secret. */ + public static final String GRAVITINO_DLF_ACCESS_KEY_SECRET = "dlf-access-key-secret"; + /** Optional DLF security token. */ + public static final String GRAVITINO_DLF_SECURITY_TOKEN = "dlf-security-token"; + + private String accessKeyId; + private String accessKeySecret; + @Nullable private String securityToken; + + /** + * Constructs a {@link DlfSecretKeyCredential} without a security token. + * + * @param accessKeyId the DLF access key ID + * @param accessKeySecret the DLF access key secret + */ + public DlfSecretKeyCredential(String accessKeyId, String accessKeySecret) { + this(accessKeyId, accessKeySecret, null); + } + + /** + * Constructs a {@link DlfSecretKeyCredential}. + * + * @param accessKeyId the DLF access key ID + * @param accessKeySecret the DLF access key secret + * @param securityToken optional security token + */ + public DlfSecretKeyCredential( + String accessKeyId, String accessKeySecret, @Nullable String securityToken) { + validate(accessKeyId, accessKeySecret, 0); + this.accessKeyId = accessKeyId; + this.accessKeySecret = accessKeySecret; + this.securityToken = securityToken; Review Comment: `dlf-security-token` is an STS token that does expire, while `expireTimeInMs()` is always 0. Please document that it is passed through as-is and never refreshed (or point users to `dlf-token-loader`). ########## catalogs/catalog-lakehouse-paimon/src/main/java/org/apache/gravitino/catalog/lakehouse/paimon/PaimonCatalog.java: ########## @@ -88,8 +89,35 @@ public PropertiesMetadata schemaPropertiesMetadata() throws UnsupportedOperation } /** - * Adds a JDBC credential provider when the backend is JDBC and credentials are configured, then - * delegates to the parent for storage (S3/OSS/Azure/GCS) credential provider detection. + * Ensures JDBC / DLF providers stay listed even when {@code credential-providers} was set + * explicitly. {@code super} skips {@link #addCatalogSpecificCredentialProviders} in that case, + * and those keys are no longer recovered via {@code getSecrets}. Review Comment: "those keys are no longer recovered via `getSecrets`" isn't accurate. JDBC secrets are intentionally excluded from `CLOUD_ACCESS_KEY_PAIR_KEYS` and are still returned by `getSecrets`, and DLF keys are returned with `INCLUDE_CREDENTIAL_SECRETS`. The actual reason is that an explicit `credential-providers` skips auto-detection, so `getCredentials` would not vend JDBC/DLF. The JDBC/DLF detection is also duplicated with `addCatalogSpecificCredentialProviders`; could be extracted into a private helper. ########## docs/lakehouse-paimon-catalog.md: ########## @@ -136,7 +136,8 @@ Download the corresponding JDBC driver and place it to the `catalogs/lakehouse-p Refer to [Manage Catalogs and Schemas](./manage-catalogs-and-schemas.md#catalog-operations) for more details. :::note -Sensitive catalog properties such as `jdbc-password` are hidden from the default load catalog response (`jdbc-user` is returned in plaintext). Retrieve secret-manager-backed properties (including `jdbc-password` when stored as a secret URN) via `getSecrets` / `GET .../objects/{type}/{fullName}/secrets`. The [credential vending API](security/credential-vending.md) (`getCredentials` / `JdbcCredential`) remains available for typed credential delivery. +Sensitive catalog properties such as `jdbc-password` and DLF credential keys are hidden from the default load catalog response (`jdbc-user` is returned in plaintext). Recover JDBC / DLF credential fields via the [credential vending API](security/credential-vending.md) (`getCredentials` / `JdbcCredential` / `DlfSecretKeyCredential`). Other non-credential secrets (secret-manager URNs, declared `hidden` properties, undeclared sensitive-named keys) use `getSecrets` / `GET .../objects/{type}/{fullName}/secrets` with `USE_SECRETS` (cloud access-key pairs also need `INCLUDE_CREDENTIAL_SECRETS`). Review Comment: This reads as if JDBC/DLF fields are not available via `getSecrets`, which isn't the case. Suggest stating both paths work, and that `getCredentials` requires no extra privilege. ########## 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 / s3-secret-key stay listed so Glue API keys and + // remapped storage keys remain available via getCredentials. 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: This force-appends `s3-secret-key` even when `credential-providers` is set explicitly, which contradicts the rule in `BaseCatalog.propertiesWithCredentialProviders()` ("Explicit credential-providers wins: do not auto-append ... s3-secret-key beside s3-token"). Scenario: `credential-providers=s3-token` + `aws-*` keys (for Glue API only). The list becomes `s3-token,aws-secret-key,s3-secret-key`, and since the Glue connectors pick the first `S3SecretKeyCredential`, engines silently get the long-lived static keys instead of the scoped STS token. Suggest only ensuring `aws-secret-key` here (its `supportsScheme` returns false, so no conflict) and not appending `s3-secret-key` in the explicit case. ########## catalogs/catalog-lakehouse-paimon/src/test/java/org/apache/gravitino/catalog/lakehouse/paimon/TestPaimonCatalog.java: ########## @@ -390,11 +423,11 @@ void testJdbcBackendWithAzureCredentialProviders() { } @Test - void testExplicitCredentialProvidersNotOverridden() { + void testExplicitCredentialProvidersStillGetsJdbc() { Review Comment: The behavior change (explicit list now also gets `jdbc`) is fine and matches `JdbcCatalog` / `IcebergCatalog`, but switching from `assertEquals` to `contains()` loses coverage of "an explicit list without JDBC/DLF keys stays unchanged". Please keep a test for that, plus a JDBC + DLF combined case, and mention the behavior change in the PR description. -- 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]
