This is an automated email from the ASF dual-hosted git repository. github-actions[bot] pushed a commit to branch cherry-pick-10ba5761-to-branch-1.3 in repository https://gitbox.apache.org/repos/asf/gravitino.git
commit 17167e165d8cab12e6878a3a4797a0e1015bcd05 Author: Yuhui <[email protected]> AuthorDate: Fri Oct 23 19:43:18 2026 +0800 [#14069] fix(core): Reject token credential providers on catalogs without path-based credentials (#14080) ### What changes were proposed in this pull request? Reject path-based token credential providers (`s3-token`, `oss-token`, `gcs-token`, `adls-token`, `cos-token`) when creating or altering a catalog that cannot provide path-based credential contexts. Only Fileset and Iceberg catalogs support them. Also correct the supported catalogs table in the credential vending docs. ### Why are the changes needed? Token providers return no credentials for a catalog-level context. A Hive catalog configured with `gcs-token` is accepted, but every engine query then fails with a storage authorization error. The docs also list Hive as vending GCS. Fix: #14069 ### Does this PR introduce _any_ user-facing change? Yes. Creating or altering a catalog other than Fileset and Iceberg with a token provider now fails with an error naming the provider and the catalog type. Existing catalogs still load. ### How was this patch tested? Added unit tests in `TestBaseCatalogCredentialProviders`, `TestCatalogManager`, `TestFilesetCatalogCredential` and `TestIcebergCatalog`. --------- Co-authored-by: Jerry Shao <[email protected]> # Conflicts: # catalogs/catalog-fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogCredential.java # core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java # core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java # docs/security/credential-vending.md --- .../fileset/TestFilesetCatalogCredential.java | 66 +++ .../apache/gravitino/catalog/glue/GlueCatalog.java | 5 + .../apache/gravitino/catalog/hive/HiveCatalog.java | 5 + .../gravitino/catalog/hive/TestHiveCatalog.java | 25 ++ .../apache/gravitino/catalog/jdbc/JdbcCatalog.java | 5 + .../gravitino/catalog/kafka/KafkaCatalog.java | 5 + .../catalog/lakehouse/generic/GenericCatalog.java | 5 + .../catalog/lakehouse/hudi/HudiCatalog.java | 5 + .../lakehouse/iceberg/TestIcebergCatalog.java | 11 + .../catalog/lakehouse/paimon/PaimonCatalog.java | 5 + .../gravitino/catalog/model/ModelCatalogImpl.java | 5 + .../apache/gravitino/catalog/CatalogManager.java | 67 +++ .../apache/gravitino/connector/BaseCatalog.java | 51 ++- .../gravitino/credential/CredentialUtils.java | 14 + .../java/org/apache/gravitino/TestCatalog.java | 5 + .../gravitino/catalog/TestCatalogManager.java | 459 +++++++++++++++++++++ .../TestBaseCatalogCredentialProviders.java | 94 ++++- docs/security/credential-vending.md | 151 +++++++ 18 files changed, 981 insertions(+), 2 deletions(-) diff --git a/catalogs/catalog-fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogCredential.java b/catalogs/catalog-fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogCredential.java index 11d8978947..cbb2d7658e 100644 --- a/catalogs/catalog-fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogCredential.java +++ b/catalogs/catalog-fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogCredential.java @@ -74,4 +74,70 @@ public class TestFilesetCatalogCredential { .propertiesWithCredentialProviders() .get(CredentialConstants.S3_CREDENTIAL_LIST_LOCATION_PREFIX)); } +<<<<<<< HEAD +======= + + @Test + void testCatalogPropertiesMaskCloudCredentials() { + Map<String, String> properties = + ImmutableMap.<String, String>builder() + .put(S3Properties.GRAVITINO_S3_ACCESS_KEY_ID, "s3-ak") + .put(S3Properties.GRAVITINO_S3_SECRET_ACCESS_KEY, "s3-sk") + .put(OSSProperties.GRAVITINO_OSS_ACCESS_KEY_ID, "oss-ak") + .put(OSSProperties.GRAVITINO_OSS_ACCESS_KEY_SECRET, "oss-sk") + .put(AzureProperties.GRAVITINO_AZURE_STORAGE_ACCOUNT_NAME, "abs-account") + .put(AzureProperties.GRAVITINO_AZURE_STORAGE_ACCOUNT_KEY, "abs-key") + .put(COSProperties.GRAVITINO_COS_ACCESS_KEY_ID, "cos-ak") + .put(COSProperties.GRAVITINO_COS_ACCESS_KEY_SECRET, "cos-sk") + .build(); + FilesetCatalogImpl catalog = newCatalog(properties); + + Map<String, String> masked = catalog.properties(); + Assertions.assertEquals( + HiddenPropertyMaskUtils.MASKED_VALUE, masked.get(S3Properties.GRAVITINO_S3_ACCESS_KEY_ID)); + Assertions.assertEquals( + HiddenPropertyMaskUtils.MASKED_VALUE, + masked.get(S3Properties.GRAVITINO_S3_SECRET_ACCESS_KEY)); + Assertions.assertEquals( + HiddenPropertyMaskUtils.MASKED_VALUE, + masked.get(OSSProperties.GRAVITINO_OSS_ACCESS_KEY_ID)); + Assertions.assertEquals( + HiddenPropertyMaskUtils.MASKED_VALUE, + masked.get(OSSProperties.GRAVITINO_OSS_ACCESS_KEY_SECRET)); + Assertions.assertEquals( + "abs-account", masked.get(AzureProperties.GRAVITINO_AZURE_STORAGE_ACCOUNT_NAME)); + Assertions.assertEquals( + HiddenPropertyMaskUtils.MASKED_VALUE, + masked.get(AzureProperties.GRAVITINO_AZURE_STORAGE_ACCOUNT_KEY)); + Assertions.assertEquals( + HiddenPropertyMaskUtils.MASKED_VALUE, + masked.get(COSProperties.GRAVITINO_COS_ACCESS_KEY_ID)); + Assertions.assertEquals( + HiddenPropertyMaskUtils.MASKED_VALUE, + masked.get(COSProperties.GRAVITINO_COS_ACCESS_KEY_SECRET)); + } + + @Test + void testCosCredentialProviderAutoDetected() { + Map<String, String> properties = Maps.newHashMap(); + properties.put(COSProperties.GRAVITINO_COS_ACCESS_KEY_ID, "cos-ak"); + properties.put(COSProperties.GRAVITINO_COS_ACCESS_KEY_SECRET, "cos-sk"); + FilesetCatalogImpl catalog = newCatalog(properties); + Assertions.assertTrue( + catalog + .propertiesWithCredentialProviders() + .get(CredentialConstants.CREDENTIAL_PROVIDERS) + .contains(COSSecretKeyCredential.COS_SECRET_KEY_CREDENTIAL_TYPE)); + } + + @Test + void testPathBasedCredentialProvidersAllowed() { + FilesetCatalogImpl catalog = newCatalog(Maps.newHashMap()); + Assertions.assertTrue(catalog.supportsPathBasedCredentials()); + Assertions.assertDoesNotThrow( + () -> + catalog.validateCredentialProviders( + ImmutableMap.of(CredentialConstants.CREDENTIAL_PROVIDERS, "s3-token,gcs-token"))); + } +>>>>>>> 10ba57615 ([#14069] fix(core): Reject token credential providers on catalogs without path-based credentials (#14080)) } diff --git a/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalog.java b/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalog.java index 8222c0d71b..70b25be499 100644 --- a/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalog.java +++ b/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalog.java @@ -54,6 +54,11 @@ public class GlueCatalog extends BaseCatalog<GlueCatalog> { return "glue"; } + @Override + public boolean supportsPathBasedCredentials() { + return false; + } + /** * Creates a new instance of {@link GlueCatalogOperations} with the provided configuration. * diff --git a/catalogs/catalog-hive/src/main/java/org/apache/gravitino/catalog/hive/HiveCatalog.java b/catalogs/catalog-hive/src/main/java/org/apache/gravitino/catalog/hive/HiveCatalog.java index 6fcccdf210..5df89aae53 100644 --- a/catalogs/catalog-hive/src/main/java/org/apache/gravitino/catalog/hive/HiveCatalog.java +++ b/catalogs/catalog-hive/src/main/java/org/apache/gravitino/catalog/hive/HiveCatalog.java @@ -51,6 +51,11 @@ public class HiveCatalog extends BaseCatalog<HiveCatalog> { return "hive"; } + @Override + public boolean supportsPathBasedCredentials() { + return false; + } + /** * Creates a new instance of {@link HiveCatalogOperations} with the provided configuration. * diff --git a/catalogs/catalog-hive/src/test/java/org/apache/gravitino/catalog/hive/TestHiveCatalog.java b/catalogs/catalog-hive/src/test/java/org/apache/gravitino/catalog/hive/TestHiveCatalog.java index 8fdb8dabae..45ddc5e433 100644 --- a/catalogs/catalog-hive/src/test/java/org/apache/gravitino/catalog/hive/TestHiveCatalog.java +++ b/catalogs/catalog-hive/src/test/java/org/apache/gravitino/catalog/hive/TestHiveCatalog.java @@ -38,6 +38,7 @@ import org.apache.gravitino.connector.HasPropertyMetadata; import org.apache.gravitino.connector.PropertiesMetadata; import org.apache.gravitino.connector.capability.Capability; import org.apache.gravitino.connector.capability.CapabilityResult; +import org.apache.gravitino.credential.CredentialConstants; import org.apache.gravitino.hive.client.HiveClientClassLoader.HiveVersion; import org.apache.gravitino.hive.hms.MiniHiveMetastoreService; import org.apache.gravitino.meta.AuditInfo; @@ -228,4 +229,28 @@ public class TestHiveCatalog extends MiniHiveMetastoreService { "Properties or property prefixes are required and must be set: [%s]", METASTORE_URIS))); } + + @Test + void testPathBasedCredentialProvidersRejected() { + CatalogEntity entity = + CatalogEntity.builder() + .withId(1L) + .withName("catalog") + .withNamespace(Namespace.of("metalake")) + .withType(HiveCatalog.Type.RELATIONAL) + .withProvider("hive") + .withAuditInfo( + AuditInfo.builder().withCreator("creator").withCreateTime(Instant.now()).build()) + .build(); + HiveCatalog catalog = new HiveCatalog().withCatalogEntity(entity); + + Assertions.assertFalse(catalog.supportsPathBasedCredentials()); + IllegalArgumentException exception = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + catalog.validateCredentialProviders( + ImmutableMap.of(CredentialConstants.CREDENTIAL_PROVIDERS, "s3-token"))); + Assertions.assertTrue(exception.getMessage().contains("s3-token")); + } } diff --git a/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalog.java b/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalog.java index 2d8877f8c8..7f5c2f831c 100644 --- a/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalog.java +++ b/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalog.java @@ -81,6 +81,11 @@ public abstract class JdbcCatalog extends BaseCatalog<JdbcCatalog> { return new JdbcCatalogCapability(); } + @Override + public boolean supportsPathBasedCredentials() { + return false; + } + /** * Returns the effective {@link JdbcConfig#VERIFY_ON_CREATE verify-on-create} setting for this * catalog. A relational JDBC catalog verifies its backend at create time by default; an diff --git a/catalogs/catalog-kafka/src/main/java/org/apache/gravitino/catalog/kafka/KafkaCatalog.java b/catalogs/catalog-kafka/src/main/java/org/apache/gravitino/catalog/kafka/KafkaCatalog.java index a0de7228d6..aa667c7ce3 100644 --- a/catalogs/catalog-kafka/src/main/java/org/apache/gravitino/catalog/kafka/KafkaCatalog.java +++ b/catalogs/catalog-kafka/src/main/java/org/apache/gravitino/catalog/kafka/KafkaCatalog.java @@ -42,6 +42,11 @@ public class KafkaCatalog extends BaseCatalog<KafkaCatalog> { return "kafka"; } + @Override + public boolean supportsPathBasedCredentials() { + return false; + } + @Override protected CatalogOperations newOps(Map<String, String> config) { KafkaCatalogOperations ops = new KafkaCatalogOperations(); diff --git a/catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/generic/GenericCatalog.java b/catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/generic/GenericCatalog.java index c8000989a4..690392d1a9 100644 --- a/catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/generic/GenericCatalog.java +++ b/catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/generic/GenericCatalog.java @@ -46,6 +46,11 @@ public class GenericCatalog extends BaseCatalog<GenericCatalog> { return "lakehouse-generic"; } + @Override + public boolean supportsPathBasedCredentials() { + return false; + } + /** * Creates a new instance of {@link GenericCatalogOperations} with the provided configuration. * diff --git a/catalogs/catalog-lakehouse-hudi/src/main/java/org/apache/gravitino/catalog/lakehouse/hudi/HudiCatalog.java b/catalogs/catalog-lakehouse-hudi/src/main/java/org/apache/gravitino/catalog/lakehouse/hudi/HudiCatalog.java index d5356cd56d..8dcf787ae2 100644 --- a/catalogs/catalog-lakehouse-hudi/src/main/java/org/apache/gravitino/catalog/lakehouse/hudi/HudiCatalog.java +++ b/catalogs/catalog-lakehouse-hudi/src/main/java/org/apache/gravitino/catalog/lakehouse/hudi/HudiCatalog.java @@ -43,6 +43,11 @@ public class HudiCatalog extends BaseCatalog<HudiCatalog> { return "lakehouse-hudi"; } + @Override + public boolean supportsPathBasedCredentials() { + return false; + } + /** * Creates a new instance of {@link HudiCatalogOperations} with the provided configuration. * diff --git a/catalogs/catalog-lakehouse-iceberg/src/test/java/org/apache/gravitino/catalog/lakehouse/iceberg/TestIcebergCatalog.java b/catalogs/catalog-lakehouse-iceberg/src/test/java/org/apache/gravitino/catalog/lakehouse/iceberg/TestIcebergCatalog.java index 51e184526f..c99bfbdb6b 100644 --- a/catalogs/catalog-lakehouse-iceberg/src/test/java/org/apache/gravitino/catalog/lakehouse/iceberg/TestIcebergCatalog.java +++ b/catalogs/catalog-lakehouse-iceberg/src/test/java/org/apache/gravitino/catalog/lakehouse/iceberg/TestIcebergCatalog.java @@ -22,6 +22,7 @@ import static org.apache.gravitino.catalog.lakehouse.iceberg.IcebergCatalog.CATA import static org.apache.gravitino.catalog.lakehouse.iceberg.IcebergCatalog.SCHEMA_PROPERTIES_META; import static org.apache.gravitino.catalog.lakehouse.iceberg.IcebergCatalog.TABLE_PROPERTIES_META; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.Maps; import java.time.Instant; import java.util.Map; @@ -490,4 +491,14 @@ public class TestIcebergCatalog { Assertions.assertTrue( credentialProviders.contains(AzureAccountKeyCredential.AZURE_ACCOUNT_KEY_CREDENTIAL_TYPE)); } + + @Test + void testPathBasedCredentialProvidersAllowed() { + IcebergCatalog catalog = new IcebergCatalog(); + Assertions.assertTrue(catalog.supportsPathBasedCredentials()); + Assertions.assertDoesNotThrow( + () -> + catalog.validateCredentialProviders( + ImmutableMap.of(CredentialConstants.CREDENTIAL_PROVIDERS, "s3-token,gcs-token"))); + } } diff --git a/catalogs/catalog-lakehouse-paimon/src/main/java/org/apache/gravitino/catalog/lakehouse/paimon/PaimonCatalog.java b/catalogs/catalog-lakehouse-paimon/src/main/java/org/apache/gravitino/catalog/lakehouse/paimon/PaimonCatalog.java index 5c7afa7a63..9950514472 100644 --- a/catalogs/catalog-lakehouse-paimon/src/main/java/org/apache/gravitino/catalog/lakehouse/paimon/PaimonCatalog.java +++ b/catalogs/catalog-lakehouse-paimon/src/main/java/org/apache/gravitino/catalog/lakehouse/paimon/PaimonCatalog.java @@ -51,6 +51,11 @@ public class PaimonCatalog extends BaseCatalog<PaimonCatalog> { return "lakehouse-paimon"; } + @Override + public boolean supportsPathBasedCredentials() { + return false; + } + /** * Creates a new instance of {@link PaimonCatalogOperations} with the provided configuration. * diff --git a/catalogs/catalog-model/src/main/java/org/apache/gravitino/catalog/model/ModelCatalogImpl.java b/catalogs/catalog-model/src/main/java/org/apache/gravitino/catalog/model/ModelCatalogImpl.java index ba3bfa4984..27b8724c84 100644 --- a/catalogs/catalog-model/src/main/java/org/apache/gravitino/catalog/model/ModelCatalogImpl.java +++ b/catalogs/catalog-model/src/main/java/org/apache/gravitino/catalog/model/ModelCatalogImpl.java @@ -45,6 +45,11 @@ public class ModelCatalogImpl extends BaseCatalog<ModelCatalogImpl> { return "model"; } + @Override + public boolean supportsPathBasedCredentials() { + return false; + } + @Override protected CatalogOperations newOps(Map<String, String> config) { EntityStore store = GravitinoEnv.getInstance().entityStore(); diff --git a/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java b/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java index 153e8b7002..0644386df4 100644 --- a/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java +++ b/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java @@ -89,6 +89,7 @@ import org.apache.gravitino.connector.HasPropertyMetadata; import org.apache.gravitino.connector.SupportsSchemas; import org.apache.gravitino.connector.authorization.BaseAuthorization; import org.apache.gravitino.connector.capability.Capability; +import org.apache.gravitino.credential.CredentialConstants; import org.apache.gravitino.exceptions.CatalogAlreadyExistsException; import org.apache.gravitino.exceptions.CatalogInUseException; import org.apache.gravitino.exceptions.CatalogNotInUseException; @@ -800,6 +801,12 @@ public class CatalogManager implements CatalogDispatcher, Closeable { Map<String, String> properties) throws NoSuchMetalakeException, CatalogAlreadyExistsException { NameIdentifier metalakeIdent = NameIdentifier.of(ident.namespace().levels()); + if (secretBindings != null) { + checkCredentialProvidersNotSecret(secretBindings.keySet()); + } + if (secretReferences != null) { + checkCredentialProvidersNotSecret(secretReferences.keySet()); + } Map<String, String> mergedConfig = buildCatalogConf(provider, properties); long uid = idGenerator.nextId(); @@ -1022,6 +1029,7 @@ public class CatalogManager implements CatalogDispatcher, Closeable { metadata.catalogPropertiesMetadata(), alterProperty.getLeft(), alterProperty.getRight()); + storedCatalog.validateCredentialProviders(alterProperty.getLeft()); return null; }); @@ -1180,6 +1188,7 @@ public class CatalogManager implements CatalogDispatcher, Closeable { f.catalogPropertiesMetadata(), alterProperty.getLeft(), alterProperty.getRight()); + catalog.validateCredentialProviders(alterProperty.getLeft()); return null; }); } catch (IllegalArgumentException e1) { @@ -1623,6 +1632,20 @@ public class CatalogManager implements CatalogDispatcher, Closeable { if (catalogChange instanceof SetProperty) { SetProperty setProperty = (SetProperty) catalogChange; upserts.put(setProperty.getProperty(), setProperty.getValue()); +<<<<<<< HEAD +======= + } else if (catalogChange instanceof CatalogChange.SetSecretBinding) { + CatalogChange.SetSecretBinding setSecretBinding = + (CatalogChange.SetSecretBinding) catalogChange; + checkCredentialProvidersNotSecret(Set.of(setSecretBinding.getProperty())); + upserts.put( + setSecretBinding.getProperty(), setSecretBinding.getBinding().plaintext()); + } else if (catalogChange instanceof CatalogChange.SetSecretReference) { + CatalogChange.SetSecretReference setSecretReference = + (CatalogChange.SetSecretReference) catalogChange; + checkCredentialProvidersNotSecret(Set.of(setSecretReference.getProperty())); + upserts.put(setSecretReference.getProperty(), setSecretReference.getProperty()); +>>>>>>> 10ba57615 ([#14069] fix(core): Reject token credential providers on catalogs without path-based credentials (#14080)) } else if (catalogChange instanceof RemoveProperty) { RemoveProperty removeProperty = (RemoveProperty) catalogChange; deletes.put(removeProperty.getProperty(), removeProperty.getProperty()); @@ -1632,6 +1655,14 @@ public class CatalogManager implements CatalogDispatcher, Closeable { return Pair.of(upserts, deletes); } + private static void checkCredentialProvidersNotSecret(Set<String> secretKeys) { + // Credential providers must stay plaintext so they can be validated against the catalog. + Preconditions.checkArgument( + !secretKeys.contains(CredentialConstants.CREDENTIAL_PROVIDERS), + "Property %s cannot be set as a secret", + CredentialConstants.CREDENTIAL_PROVIDERS); + } + private CatalogWrapper loadCatalogInternal(NameIdentifier ident) throws NoSuchCatalogException { try { CatalogEntity entity = store.get(ident, EntityType.CATALOG, CatalogEntity.class); @@ -1688,6 +1719,7 @@ public class CatalogManager implements CatalogDispatcher, Closeable { IsolatedClassLoader classLoader = createClassLoader(provider, conf); BaseCatalog<?> catalog = createBaseCatalog(classLoader, entity); +<<<<<<< HEAD CatalogWrapper wrapper = new CatalogWrapper(catalog, classLoader); // Validate catalog properties and initialize the config classLoader.withClassLoader( @@ -1715,6 +1747,41 @@ public class CatalogManager implements CatalogDispatcher, Closeable { }, IllegalArgumentException.class, ConnectionFailedException.class); +======= + /** + * Creates the catalog instance, validates properties, and preloads property/capability values + * into the given wrapper. + */ + private CatalogWrapper initCatalogWrapper( + CatalogWrapper wrapper, + CatalogEntity entity, + @Nullable Map<String, String> propsToValidate, + boolean validateBackend) { + BaseCatalog<?> catalog = createBaseCatalog(wrapper.classLoader, entity); + wrapper.catalog = catalog; + try { + wrapper.classLoader.withClassLoader( + cl -> { + validatePropertyForCreate(catalog.catalogPropertiesMetadata(), propsToValidate); + catalog.validateCredentialProviders(propsToValidate); + if (propsToValidate == null) { + // Loading stays lenient for catalogs persisted with unsupported providers, so only + // warn about them. Providers persisted as a secret are checked by their plaintext. + try { + catalog.validateCredentialProviders( + secretManager.toPlaintextProperties(entity.getProperties())); + } catch (IllegalArgumentException e) { + LOG.warn( + "Catalog {} has credential providers that cannot vend credentials: {}", + entity.nameIdentifier(), + e.getMessage()); + } + } + // Preload properties() and capability() inside the IsolatedClassLoader so that + // AppClassLoader can read them later without needing the isolated context. + catalog.properties(); + catalog.capability(); +>>>>>>> 10ba57615 ([#14069] fix(core): Reject token credential providers on catalogs without path-based credentials (#14080)) return wrapper; } diff --git a/core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java b/core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java index 777a2cce54..6d23332d80 100644 --- a/core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java +++ b/core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java @@ -26,8 +26,10 @@ import java.io.Closeable; import java.io.IOException; import java.util.ArrayList; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.Optional; +import java.util.stream.Collectors; import javax.annotation.Nullable; import org.apache.commons.lang3.StringUtils; import org.apache.gravitino.Audit; @@ -40,9 +42,11 @@ import org.apache.gravitino.annotation.Evolving; import org.apache.gravitino.connector.authorization.AuthorizationPlugin; import org.apache.gravitino.connector.authorization.BaseAuthorization; import org.apache.gravitino.connector.capability.Capability; +import org.apache.gravitino.connector.credential.SupportsPathBasedCredentials; import org.apache.gravitino.credential.AzureAccountKeyCredential; import org.apache.gravitino.credential.CatalogCredentialManager; import org.apache.gravitino.credential.CredentialConstants; +import org.apache.gravitino.credential.CredentialUtils; import org.apache.gravitino.credential.GCSTokenCredential; import org.apache.gravitino.credential.OSSSecretKeyCredential; import org.apache.gravitino.credential.S3SecretKeyCredential; @@ -237,6 +241,50 @@ public abstract class BaseCatalog<T extends BaseCatalog> ops(); } + /** + * Whether this catalog provides path-based credential contexts, which token credential providers + * such as {@code s3-token} require to vend credentials. + * + * <p>A catalog that cannot provide path-based contexts, neither through operations implementing + * {@link SupportsPathBasedCredentials} nor through another server such as the Iceberg REST + * server, should override this method to return {@code false}, so that creating or altering it + * with a token provider fails. + * + * @return {@code true} (default) if path-based credentials are supported; {@code false} otherwise + */ + public boolean supportsPathBasedCredentials() { + return true; + } + + /** + * Validates that the credential providers set in {@code properties} can vend credentials for this + * catalog. Token providers listed in {@link CredentialUtils#PATH_BASED_CREDENTIAL_TYPES} are + * rejected when {@link #supportsPathBasedCredentials()} returns {@code false}. + * + * @param properties the catalog properties to validate, may be null + * @throws IllegalArgumentException if a credential provider cannot be vended by this catalog + */ + public void validateCredentialProviders(@Nullable Map<String, String> properties) { + if (properties == null || supportsPathBasedCredentials()) { + return; + } + List<String> unsupportedProviders = + CredentialUtils.getCredentialProvidersByOrder(() -> properties).stream() + // Provider lookup ignores case, so match the type names case-insensitively too. + .filter( + p -> + CredentialUtils.PATH_BASED_CREDENTIAL_TYPES.contains( + p.toLowerCase(Locale.ROOT))) + .sorted() + .collect(Collectors.toList()); + Preconditions.checkArgument( + unsupportedProviders.isEmpty(), + "Credential providers %s are not supported by catalog provider %s, it cannot vend" + + " path-based credentials", + unsupportedProviders, + provider()); + } + public void checkMetalakeAndCatalogInUse() { Map<String, String> catalogProperties = entity.getProperties(); if (catalogProperties == null) { @@ -587,7 +635,8 @@ public abstract class BaseCatalog<T extends BaseCatalog> } String gcsServiceAccountFile = properties.get(GCSProperties.GRAVITINO_GCS_SERVICE_ACCOUNT_FILE); - if (StringUtils.isNotBlank(gcsServiceAccountFile)) { + // gcs-token only vends credentials for path-based contexts. + if (StringUtils.isNotBlank(gcsServiceAccountFile) && supportsPathBasedCredentials()) { credentialProviders.add(GCSTokenCredential.GCS_TOKEN_CREDENTIAL_TYPE); } } diff --git a/core/src/main/java/org/apache/gravitino/credential/CredentialUtils.java b/core/src/main/java/org/apache/gravitino/credential/CredentialUtils.java index 9d2ea43e66..7e378b6fb2 100644 --- a/core/src/main/java/org/apache/gravitino/credential/CredentialUtils.java +++ b/core/src/main/java/org/apache/gravitino/credential/CredentialUtils.java @@ -19,6 +19,7 @@ package org.apache.gravitino.credential; +import com.google.common.collect.ImmutableSet; import java.util.Collections; import java.util.List; import java.util.Map; @@ -29,6 +30,19 @@ import org.apache.gravitino.credential.config.CredentialConfig; public class CredentialUtils { + /** + * Credential types that only vend credentials for a path-based credential context. They return + * nothing for a catalog-level context, so they only work on catalogs that provide path contexts. + */ + public static final Set<String> PATH_BASED_CREDENTIAL_TYPES = + ImmutableSet.of( + S3TokenCredential.S3_TOKEN_CREDENTIAL_TYPE, + AwsIrsaCredential.AWS_IRSA_CREDENTIAL_TYPE, + OSSTokenCredential.OSS_TOKEN_CREDENTIAL_TYPE, + GCSTokenCredential.GCS_TOKEN_CREDENTIAL_TYPE, + ADLSTokenCredential.ADLS_TOKEN_CREDENTIAL_TYPE, + COSTokenCredential.COS_TOKEN_CREDENTIAL_TYPE); + public static Map<String, CredentialProvider> loadCredentialProviders( Map<String, String> catalogProperties) { CredentialConfig credentialConfig = new CredentialConfig(catalogProperties); diff --git a/core/src/test/java/org/apache/gravitino/TestCatalog.java b/core/src/test/java/org/apache/gravitino/TestCatalog.java index d3fb322182..6d5a27bee0 100644 --- a/core/src/test/java/org/apache/gravitino/TestCatalog.java +++ b/core/src/test/java/org/apache/gravitino/TestCatalog.java @@ -55,6 +55,11 @@ public class TestCatalog extends BaseCatalog<TestCatalog> { return "test"; } + @Override + public boolean supportsPathBasedCredentials() { + return false; + } + @Override protected CatalogOperations newOps(Map<String, String> config) { return new TestCatalogOperations(config); diff --git a/core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java b/core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java index d64aff46de..49bc4ced9c 100644 --- a/core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java +++ b/core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java @@ -39,6 +39,8 @@ import java.nio.file.Files; import java.nio.file.Path; import java.time.Duration; import java.time.Instant; +import java.util.ArrayList; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Set; @@ -67,6 +69,7 @@ import org.apache.gravitino.connector.SupportsSchemas; import org.apache.gravitino.connector.TestCatalogOperations; import org.apache.gravitino.connector.capability.Capability; import org.apache.gravitino.connector.capability.CapabilityResult; +import org.apache.gravitino.credential.CredentialConstants; import org.apache.gravitino.exceptions.CatalogAlreadyExistsException; import org.apache.gravitino.exceptions.CatalogNotInUseException; import org.apache.gravitino.exceptions.ConnectionFailedException; @@ -83,6 +86,20 @@ import org.apache.gravitino.meta.BaseMetalake; import org.apache.gravitino.meta.CatalogEntity; import org.apache.gravitino.meta.SchemaEntity; import org.apache.gravitino.meta.SchemaVersion; +<<<<<<< HEAD +======= +import org.apache.gravitino.secret.SecretBinding; +import org.apache.gravitino.secret.SecretConstants; +import org.apache.gravitino.secret.SecretManager; +import org.apache.gravitino.secret.SecretMaterial; +import org.apache.gravitino.secret.SecretPropertyUtils; +import org.apache.gravitino.secret.SecretProvider; +import org.apache.gravitino.secret.SecretProviderRegistry; +import org.apache.gravitino.secret.SecretReference; +import org.apache.gravitino.secret.SecretUrn; +import org.apache.gravitino.secret.memory.InMemorySecretsProvider; +import org.apache.gravitino.storage.IdGenerator; +>>>>>>> 10ba57615 ([#14069] fix(core): Reject token credential providers on catalogs without path-based credentials (#14080)) import org.apache.gravitino.storage.RandomIdGenerator; import org.apache.gravitino.storage.memory.TestMemoryEntityStore; import org.apache.gravitino.storage.memory.TestMemoryEntityStore.InMemoryEntityStore; @@ -92,6 +109,13 @@ import org.apache.gravitino.storage.relational.po.cache.EntityChangeRecord; import org.apache.gravitino.storage.relational.po.cache.OperateType; import org.apache.gravitino.utils.PrincipalUtils; import org.apache.gravitino.utils.ThrowableFunction; +import org.apache.logging.log4j.Level; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.core.LogEvent; +import org.apache.logging.log4j.core.LoggerContext; +import org.apache.logging.log4j.core.appender.AbstractAppender; +import org.apache.logging.log4j.core.config.Configuration; +import org.apache.logging.log4j.core.config.LoggerConfig; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Assertions; @@ -277,6 +301,127 @@ public class TestCatalogManager { } @Test +<<<<<<< HEAD +======= + void testAlterCatalogRejectsMaskedHiddenProperty() throws IOException { + NameIdentifier ident = NameIdentifier.of("metalake", "masked_hidden"); + Map<String, String> props = + ImmutableMap.<String, String>builder() + .put(PROPERTY_KEY1, "value1") + .put(PROPERTY_KEY2, "value2") + .put(PROPERTY_KEY3, "3") + .put(PROPERTY_KEY4, "value4") + .put(PROPERTY_KEY5_PREFIX + "1", "value1") + .put(PROPERTY_KEY6_PREFIX + "1", "value1") + .put(PROPERTY_HIDDEN_KEY, "secret") + .put("mock", "mock") + .build(); + catalogManager.createCatalog(ident, Catalog.Type.RELATIONAL, provider, "comment", props); + + IllegalArgumentException exception = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + catalogManager.alterCatalog( + ident, + CatalogChange.setProperty( + PROPERTY_HIDDEN_KEY, HiddenPropertyMaskUtils.MASKED_VALUE))); + + Assertions.assertTrue(exception.getMessage().contains(PROPERTY_HIDDEN_KEY)); + CatalogEntity entity = entityStore.get(ident, EntityType.CATALOG, CatalogEntity.class); + Assertions.assertEquals("secret", entity.getProperties().get(PROPERTY_HIDDEN_KEY)); + reset(); + } + + @Test + void testCreateCatalogRejectsMaskedHiddenProperty() throws IOException { + NameIdentifier ident = NameIdentifier.of("metalake", "masked_create"); + Map<String, String> props = + ImmutableMap.<String, String>builder() + .put(PROPERTY_KEY1, "value1") + .put(PROPERTY_KEY2, "value2") + .put(PROPERTY_KEY3, "3") + .put(PROPERTY_KEY4, "value4") + .put(PROPERTY_KEY5_PREFIX + "1", "value1") + .put(PROPERTY_KEY6_PREFIX + "1", "value1") + .put(PROPERTY_HIDDEN_KEY, HiddenPropertyMaskUtils.MASKED_VALUE) + .put("mock", "mock") + .build(); + + IllegalArgumentException exception = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + catalogManager.createCatalog( + ident, Catalog.Type.RELATIONAL, provider, "comment", props)); + Assertions.assertTrue(exception.getMessage().contains(PROPERTY_HIDDEN_KEY)); + reset(); + } + + @Test + void testPathBasedCredentialProviderValidation() throws IOException { + NameIdentifier ident = NameIdentifier.of("metalake", "token_provider"); + Map<String, String> props = + ImmutableMap.<String, String>builder() + .put(PROPERTY_KEY1, "value1") + .put(PROPERTY_KEY2, "value2") + .put(PROPERTY_KEY3, "3") + .put(PROPERTY_KEY4, "value4") + .put(PROPERTY_KEY5_PREFIX + "1", "value1") + .put(PROPERTY_KEY6_PREFIX + "1", "value1") + .put("mock", "mock") + .build(); + Map<String, String> tokenProps = + ImmutableMap.<String, String>builder() + .putAll(props) + .put(CredentialConstants.CREDENTIAL_PROVIDERS, "s3-secret-key,gcs-token") + .build(); + + IllegalArgumentException exception = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + catalogManager.createCatalog( + ident, Catalog.Type.RELATIONAL, provider, "comment", tokenProps)); + Assertions.assertTrue(exception.getMessage().contains("gcs-token")); + Assertions.assertTrue(exception.getMessage().contains(provider)); + Assertions.assertFalse(catalogManager.catalogExists(ident)); + + catalogManager.createCatalog(ident, Catalog.Type.RELATIONAL, provider, "comment", props); + exception = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + catalogManager.alterCatalog( + ident, + CatalogChange.setProperty( + CredentialConstants.CREDENTIAL_PROVIDERS, "adls-token"))); + Assertions.assertTrue(exception.getMessage().contains("adls-token")); + + exception = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + catalogManager.testConnection( + ident, + CatalogChange.setProperty( + CredentialConstants.CREDENTIAL_PROVIDERS, "s3-token"))); + Assertions.assertTrue(exception.getMessage().contains("s3-token")); + + catalogManager.alterCatalog( + ident, + CatalogChange.setProperty(CredentialConstants.CREDENTIAL_PROVIDERS, "s3-secret-key")); + Assertions.assertEquals( + "s3-secret-key", + catalogManager + .loadCatalog(ident) + .properties() + .get(CredentialConstants.CREDENTIAL_PROVIDERS)); + reset(); + } + + @Test +>>>>>>> 10ba57615 ([#14069] fix(core): Reject token credential providers on catalogs without path-based credentials (#14080)) void testPropertyValidationInCreate() throws IOException { // key1 is required and immutable and do not have default value, is not hidden and not reserved // key2 is required and mutable and do not have default value, is not hidden and not reserved @@ -1687,6 +1832,320 @@ public class TestCatalogManager { Assertions.assertEquals(v, testProps.get(k)); }); +<<<<<<< HEAD Assertions.assertFalse(testProps.containsKey(ID_KEY), "`gravitino.identifier` is missing"); +======= + Assertions.assertFalse( + testProps.containsKey(ID_KEY), + "`gravitino.identifier` is reserved+hidden and should be omitted from responses"); + } + + @Test + void testSecrets() throws Exception { + try (SecretManager secrets = memorySecretManager(); + CatalogManager manager = + new CatalogManager(config, entityStore, new RandomIdGenerator(), secrets)) { + NameIdentifier ident = NameIdentifier.of("metalake", "secret_ok"); + Catalog catalog = + manager.createCatalog( + ident, + Catalog.Type.RELATIONAL, + provider, + "comment", + catalogProps(), + Map.of(PROPERTY_KEY4, new SecretBinding("memory", "s3cr3t")), + Map.of()); + Assertions.assertEquals( + HiddenPropertyMaskUtils.MASKED_VALUE, catalog.properties().get(PROPERTY_KEY4)); + String urn = + entityStore + .get(ident, EntityType.CATALOG, CatalogEntity.class) + .getProperties() + .get(PROPERTY_KEY4); + Assertions.assertTrue(SecretPropertyUtils.isSecretProperty(PROPERTY_KEY4, urn)); + Assertions.assertEquals("s3cr3t", secrets.readSecret(SecretUrn.parse(urn))); + + manager.alterCatalog(ident, CatalogChange.removeProperty(PROPERTY_KEY4)); + Assertions.assertFalse( + entityStore + .get(ident, EntityType.CATALOG, CatalogEntity.class) + .getProperties() + .containsKey(PROPERTY_KEY4)); + Assertions.assertThrows( + IllegalArgumentException.class, () -> secrets.readSecret(SecretUrn.parse(urn))); + + Assertions.assertTrue(manager.dropCatalog(ident, true)); + Assertions.assertThrows( + IllegalArgumentException.class, () -> secrets.readSecret(SecretUrn.parse(urn))); + } + } + + @Test + void testCredentialProvidersCannotBeSecret() throws Exception { + try (SecretManager secrets = memorySecretManager(); + CatalogManager manager = + new CatalogManager(config, entityStore, new RandomIdGenerator(), secrets)) { + NameIdentifier ident = NameIdentifier.of("metalake", "secret_credential_providers"); + SecretReference reference = new SecretReference("reference", Map.of("path", "providers")); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + manager.createCatalog( + ident, + Catalog.Type.RELATIONAL, + provider, + "comment", + catalogProps(), + Map.of( + CredentialConstants.CREDENTIAL_PROVIDERS, + new SecretBinding("memory", "gcs-token")), + Map.of())); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + manager.createCatalog( + ident, + Catalog.Type.RELATIONAL, + provider, + "comment", + catalogProps(), + Map.of(), + Map.of(CredentialConstants.CREDENTIAL_PROVIDERS, reference))); + Assertions.assertFalse(manager.catalogExists(ident)); + + manager.createCatalog(ident, Catalog.Type.RELATIONAL, provider, "comment", catalogProps()); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + manager.alterCatalog( + ident, + CatalogChange.setSecretBinding( + CredentialConstants.CREDENTIAL_PROVIDERS, + new SecretBinding("memory", "gcs-token")))); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + manager.alterCatalog( + ident, + CatalogChange.setSecretReference( + CredentialConstants.CREDENTIAL_PROVIDERS, reference))); + Assertions.assertTrue(manager.dropCatalog(ident, true)); + } + } + + @Test + void testLoadWarnsForSecretCredentialProviders() throws Exception { + LoggerContext context = + (LoggerContext) LogManager.getContext(CatalogManager.class.getClassLoader(), false); + Configuration logConfig = context.getConfiguration(); + List<LogEvent> events = new ArrayList<>(); + AbstractAppender appender = + new AbstractAppender("credentialProvidersCapture", null, null, true, null) { + @Override + public void append(LogEvent event) { + events.add(event.toImmutable()); + } + }; + appender.start(); + LoggerConfig logger = new LoggerConfig(CatalogManager.class.getName(), Level.WARN, false); + logger.addAppender(appender, Level.WARN, null); + logConfig.addLogger(CatalogManager.class.getName(), logger); + context.updateLoggers(); + + try (SecretManager secrets = memorySecretManager(); + CatalogManager manager = + new CatalogManager(config, entityStore, new RandomIdGenerator(), secrets)) { + // A catalog persisted before credential providers were rejected as secrets. + NameIdentifier ident = NameIdentifier.of("metalake", "legacy_secret_providers"); + long id = new RandomIdGenerator().nextId(); + SecretUrn urn = writeThroughUrn("catalog", id, CredentialConstants.CREDENTIAL_PROVIDERS); + secrets.writeSecrets(List.of(new SecretMaterial(urn, "gcs-token"))); + Map<String, String> props = new HashMap<>(catalogProps()); + props.put(CredentialConstants.CREDENTIAL_PROVIDERS, urn.toString()); + entityStore.put( + CatalogEntity.builder() + .withId(id) + .withName(ident.name()) + .withNamespace(ident.namespace()) + .withType(Catalog.Type.RELATIONAL) + .withProvider(provider) + .withComment("comment") + .withProperties(props) + .withAuditInfo( + AuditInfo.builder().withCreator("test").withCreateTime(Instant.now()).build()) + .build()); + + Assertions.assertDoesNotThrow(() -> manager.loadCatalog(ident)); + Assertions.assertTrue( + events.stream() + .anyMatch(e -> e.getMessage().getFormattedMessage().contains("gcs-token"))); + Assertions.assertTrue(manager.dropCatalog(ident, true)); + } finally { + logConfig.removeLogger(CatalogManager.class.getName()); + context.updateLoggers(); + appender.stop(); + } + } + + @Test + void testConnectionChangesDoNotMutateSecrets() throws Exception { + try (SecretManager secrets = memorySecretManager(); + CatalogManager manager = + Mockito.spy( + new CatalogManager(config, entityStore, new RandomIdGenerator(), secrets))) { + NameIdentifier ident = NameIdentifier.of("metalake", "secret_connection_test"); + manager.createCatalog( + ident, + Catalog.Type.RELATIONAL, + provider, + "comment", + catalogProps(), + Map.of(PROPERTY_KEY4, new SecretBinding("memory", "stored-secret")), + Map.of()); + CatalogEntity stored = entityStore.get(ident, EntityType.CATALOG, CatalogEntity.class); + String storedUrn = stored.getProperties().get(PROPERTY_KEY4); + Assertions.assertEquals("stored-secret", secrets.readSecret(SecretUrn.parse(storedUrn))); + SecretUrn proposedUrn = writeThroughUrn("catalog", stored.id(), PROPERTY_KEY2); + + CatalogManager.CatalogWrapper temporaryWrapper = + Mockito.mock(CatalogManager.CatalogWrapper.class); + AtomicReference<CatalogEntity> effectiveEntity = new AtomicReference<>(); + Mockito.doAnswer( + invocation -> { + effectiveEntity.set(invocation.getArgument(0)); + return temporaryWrapper; + }) + .when(manager) + .createCatalogWrapper(any(CatalogEntity.class), eq(null)); + Mockito.doReturn(null).when(temporaryWrapper).doWithCatalogOps(any()); + + manager.testConnection( + ident, + CatalogChange.setSecretBinding( + PROPERTY_KEY4, new SecretBinding("memory", "temporary-secret")), + CatalogChange.setSecretBinding( + PROPERTY_KEY2, new SecretBinding("memory", "temporary-new-secret"))); + Assertions.assertEquals( + "temporary-secret", effectiveEntity.get().getProperties().get(PROPERTY_KEY4)); + Assertions.assertEquals( + "temporary-new-secret", effectiveEntity.get().getProperties().get(PROPERTY_KEY2)); + Assertions.assertEquals("stored-secret", secrets.readSecret(SecretUrn.parse(storedUrn))); + Assertions.assertThrows( + IllegalArgumentException.class, () -> secrets.readSecret(proposedUrn)); + + manager.testConnection(ident, CatalogChange.removeProperty(PROPERTY_KEY4)); + Assertions.assertFalse(effectiveEntity.get().getProperties().containsKey(PROPERTY_KEY4)); + Assertions.assertEquals("stored-secret", secrets.readSecret(SecretUrn.parse(storedUrn))); + + manager.testConnection( + ident, + CatalogChange.setSecretReference( + PROPERTY_KEY4, new SecretReference("reference", Map.of("path", "external-secret")))); + String referenceUrn = effectiveEntity.get().getProperties().get(PROPERTY_KEY4); + Assertions.assertTrue(SecretPropertyUtils.isSecretProperty(PROPERTY_KEY4, referenceUrn)); + Assertions.assertEquals( + "resolved-external-secret", + secrets.toPlaintextProperties(effectiveEntity.get().getProperties()).get(PROPERTY_KEY4)); + Assertions.assertEquals("stored-secret", secrets.readSecret(SecretUrn.parse(storedUrn))); + + Assertions.assertThrows( + IllegalArgumentException.class, + () -> manager.testConnection(ident, CatalogChange.setProperty(PROPERTY_KEY4, storedUrn))); + CatalogEntity storedAfter = entityStore.get(ident, EntityType.CATALOG, CatalogEntity.class); + Assertions.assertEquals(stored.getProperties(), storedAfter.getProperties()); + Assertions.assertEquals("stored-secret", secrets.readSecret(SecretUrn.parse(storedUrn))); + Mockito.verify(manager, Mockito.times(4)).acquireCatalogLease(ident); + Mockito.verify(temporaryWrapper, Mockito.times(3)).close(); + } + } + + @Test + void testSecretRollback() throws Exception { + try (SecretManager secrets = memorySecretManager()) { + IdGenerator ids = new AtomicLong(4242L)::getAndIncrement; + CatalogManager manager = Mockito.spy(new CatalogManager(config, entityStore, ids, secrets)); + NameIdentifier ident = NameIdentifier.of("metalake", "secret_fail"); + Mockito.doThrow(new RuntimeException("init failed")) + .when(manager) + .createCatalogWrapper(any(CatalogEntity.class), any()); + SecretUrn urn = writeThroughUrn("catalog", 4242L, PROPERTY_KEY4); + Assertions.assertThrows( + RuntimeException.class, + () -> + manager.createCatalog( + ident, + Catalog.Type.RELATIONAL, + provider, + "comment", + catalogProps(), + Map.of(PROPERTY_KEY4, new SecretBinding("memory", "x")), + Map.of())); + Assertions.assertFalse(entityStore.exists(ident, EntityType.CATALOG)); + Assertions.assertThrows(IllegalArgumentException.class, () -> secrets.readSecret(urn)); + manager.close(); + } + } + + @Test + void testSecretNoMetalake() throws Exception { + try (SecretManager secrets = memorySecretManager(); + CatalogManager manager = + new CatalogManager( + config, entityStore, new AtomicLong(4343L)::getAndIncrement, secrets)) { + NameIdentifier ident = NameIdentifier.of("missing_metalake", "secret_catalog"); + SecretUrn urn = writeThroughUrn("catalog", 4343L, PROPERTY_KEY4); + Assertions.assertThrows( + NoSuchMetalakeException.class, + () -> + manager.createCatalog( + ident, + Catalog.Type.RELATIONAL, + provider, + "comment", + catalogProps(), + Map.of(PROPERTY_KEY4, new SecretBinding("memory", "x")), + Map.of())); + Assertions.assertThrows(IllegalArgumentException.class, () -> secrets.readSecret(urn)); + } + } + + private static Map<String, String> catalogProps() { + return ImmutableMap.of( + "provider", + "test", + PROPERTY_KEY1, + "value1", + PROPERTY_KEY2, + "value2", + PROPERTY_KEY5_PREFIX + "1", + "value3"); + } + + private static SecretUrn writeThroughUrn(String entityType, long entityId, String key) { + return SecretUrn.buildWriteThrough( + "memory", + Map.of( + SecretConstants.ATTR_ENTITY_TYPE, entityType, + SecretConstants.ATTR_ENTITY_ID, String.valueOf(entityId), + SecretConstants.ATTR_PROPERTY_KEY, key)); + } + + private static SecretManager memorySecretManager() { + Config c = new Config(false) {}; + Properties p = new Properties(); + p.setProperty(SecretProviderRegistry.GRAVITINO_SECRET_PROVIDERS, "memory,reference"); + p.setProperty( + SecretProviderRegistry.GRAVITINO_SECRET_PROVIDER_PREFIX + + "memory." + + SecretProviderRegistry.CLASS_NAME, + InMemorySecretsProvider.class.getName()); + p.setProperty( + SecretProviderRegistry.GRAVITINO_SECRET_PROVIDER_PREFIX + + "reference." + + SecretProviderRegistry.CLASS_NAME, + TestReferenceSecretsProvider.class.getName()); + c.loadFromProperties(p); + return new SecretManager(c); +>>>>>>> 10ba57615 ([#14069] fix(core): Reject token credential providers on catalogs without path-based credentials (#14080)) } } diff --git a/core/src/test/java/org/apache/gravitino/connector/TestBaseCatalogCredentialProviders.java b/core/src/test/java/org/apache/gravitino/connector/TestBaseCatalogCredentialProviders.java index 8b9ff46acd..d8beeb039e 100644 --- a/core/src/test/java/org/apache/gravitino/connector/TestBaseCatalogCredentialProviders.java +++ b/core/src/test/java/org/apache/gravitino/connector/TestBaseCatalogCredentialProviders.java @@ -18,6 +18,8 @@ */ package org.apache.gravitino.connector; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import java.time.Instant; import java.util.HashMap; import java.util.Map; @@ -25,10 +27,18 @@ import org.apache.commons.lang3.reflect.FieldUtils; import org.apache.gravitino.Catalog; import org.apache.gravitino.Namespace; import org.apache.gravitino.connector.capability.Capability; +import org.apache.gravitino.credential.ADLSTokenCredential; +import org.apache.gravitino.credential.AwsIrsaCredential; +import org.apache.gravitino.credential.COSTokenCredential; import org.apache.gravitino.credential.CredentialConstants; +import org.apache.gravitino.credential.CredentialUtils; +import org.apache.gravitino.credential.GCSTokenCredential; +import org.apache.gravitino.credential.OSSTokenCredential; import org.apache.gravitino.credential.S3SecretKeyCredential; +import org.apache.gravitino.credential.S3TokenCredential; import org.apache.gravitino.meta.AuditInfo; import org.apache.gravitino.meta.CatalogEntity; +import org.apache.gravitino.storage.GCSProperties; import org.apache.gravitino.storage.S3Properties; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; @@ -68,7 +78,82 @@ public class TestBaseCatalogCredentialProviders { Assertions.assertFalse(result.containsKey(CredentialConstants.CREDENTIAL_PROVIDERS)); } + @Test + void testPathBasedCredentialTypes() { + Assertions.assertEquals( + ImmutableSet.of( + S3TokenCredential.S3_TOKEN_CREDENTIAL_TYPE, + AwsIrsaCredential.AWS_IRSA_CREDENTIAL_TYPE, + OSSTokenCredential.OSS_TOKEN_CREDENTIAL_TYPE, + GCSTokenCredential.GCS_TOKEN_CREDENTIAL_TYPE, + ADLSTokenCredential.ADLS_TOKEN_CREDENTIAL_TYPE, + COSTokenCredential.COS_TOKEN_CREDENTIAL_TYPE), + CredentialUtils.PATH_BASED_CREDENTIAL_TYPES); + } + + @Test + void testValidateCredentialProviders() throws IllegalAccessException { + TestCatalogImpl catalog = newCatalog(new NoPathBasedCredentialsCatalogImpl(), new HashMap<>()); + Assertions.assertDoesNotThrow(() -> catalog.validateCredentialProviders(null)); + Assertions.assertDoesNotThrow( + () -> + catalog.validateCredentialProviders( + ImmutableMap.of( + CredentialConstants.CREDENTIAL_PROVIDERS, + "s3-secret-key,oss-secret-key,azure-account-key"))); + + for (String type : CredentialUtils.PATH_BASED_CREDENTIAL_TYPES) { + IllegalArgumentException exception = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + catalog.validateCredentialProviders( + ImmutableMap.of( + CredentialConstants.CREDENTIAL_PROVIDERS, "s3-secret-key," + type))); + Assertions.assertTrue(exception.getMessage().contains(type)); + Assertions.assertTrue(exception.getMessage().contains("test")); + } + + // Provider lookup ignores case, so validation does too. + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + catalog.validateCredentialProviders( + ImmutableMap.of(CredentialConstants.CREDENTIAL_PROVIDERS, "GCS-Token"))); + } + + @Test + void testGcsTokenInferredOnlyWithPathBasedCredentials() throws IllegalAccessException { + Map<String, String> props = new HashMap<>(); + props.put(GCSProperties.GRAVITINO_GCS_SERVICE_ACCOUNT_FILE, "/tmp/gcs.json"); + + Map<String, String> result = + newCatalog(new NoPathBasedCredentialsCatalogImpl(), props).properties(); + Assertions.assertFalse(result.containsKey(CredentialConstants.CREDENTIAL_PROVIDERS)); + + Assertions.assertEquals( + GCSTokenCredential.GCS_TOKEN_CREDENTIAL_TYPE, + newCatalog(props).properties().get(CredentialConstants.CREDENTIAL_PROVIDERS)); + } + + @Test + void testPathBasedCredentialsSupportedByDefault() throws IllegalAccessException { + // A catalog that does not override supportsPathBasedCredentials() keeps accepting token + // providers. + TestCatalogImpl catalog = newCatalog(new HashMap<>()); + Assertions.assertTrue(catalog.supportsPathBasedCredentials()); + Assertions.assertDoesNotThrow( + () -> + catalog.validateCredentialProviders( + ImmutableMap.of(CredentialConstants.CREDENTIAL_PROVIDERS, "s3-token,gcs-token"))); + } + private TestCatalogImpl newCatalog(Map<String, String> props) throws IllegalAccessException { + return newCatalog(new TestCatalogImpl(), props); + } + + private TestCatalogImpl newCatalog(TestCatalogImpl catalog, Map<String, String> props) + throws IllegalAccessException { CatalogEntity entity = CatalogEntity.builder() .withId(1L) @@ -81,7 +166,6 @@ public class TestBaseCatalogCredentialProviders { .withAuditInfo( AuditInfo.builder().withCreator("tester").withCreateTime(Instant.now()).build()) .build(); - TestCatalogImpl catalog = new TestCatalogImpl(); FieldUtils.writeField(catalog, "entity", entity, true); return catalog; } @@ -108,4 +192,12 @@ public class TestBaseCatalogCredentialProviders { return BaseCatalogPropertiesMetadata.BASIC_CATALOG_PROPERTIES_METADATA; } } + + private static class NoPathBasedCredentialsCatalogImpl extends TestCatalogImpl { + + @Override + public boolean supportsPathBasedCredentials() { + return false; + } + } } diff --git a/docs/security/credential-vending.md b/docs/security/credential-vending.md index 7604f51548..5e32d05fba 100644 --- a/docs/security/credential-vending.md +++ b/docs/security/credential-vending.md @@ -11,6 +11,7 @@ Gravitino credential vending is used to generate temporary or static credentials ### Capabilities +<<<<<<< HEAD - Supports Gravitino Iceberg REST server. - Supports Gravitino server with the following catalog types: @@ -28,6 +29,91 @@ Gravitino credential vending is used to generate temporary or static credentials - ADLS: `ADLSTokenCredential`, `AzureAccountKeyCredential` - OSS: `OSSTokenCredential`, `OSSSecretKeyCredential` - Spark, Flink, and Trino connectors automatically consume vended credentials for Hive, Iceberg, Glue, JDBC, and Paimon catalogs. +======= +| Catalog type | Vends | +|--------------|---------------------------------| +| Fileset | S3, OSS, GCS, ADLS, COS | +| Hive | S3, OSS, ADLS | +| Iceberg | S3, OSS, GCS, ADLS | +| Glue | S3 | +| JDBC | JDBC user and password | +| Paimon | S3, OSS, JDBC user and password | + +S3 is Amazon S3, OSS is Alibaba Cloud OSS, GCS is Google Cloud Storage, ADLS is Azure Data Lake Storage, and COS is Tencent Cloud COS. The Gravitino Spark, Flink, and Trino connectors consume vended credentials automatically for these catalogs, except that the Trino Hive connector does not consume OSS credentials. For Iceberg catalogs without a `REST` backend that vends credentials, the Flink connector receives only static credentials, see [Flink Iceberg credential vending](../flink-conn [...] + +Token providers (`s3-token`, `oss-token`, `gcs-token`, `adls-token`, and `cos-token`) and `aws-irsa` scope credentials to a path, so only Fileset and Iceberg catalogs support them. Other catalogs vend static keys only, such as `s3-secret-key`, `oss-secret-key`, and `azure-account-key`. Creating or altering such a catalog with one of these providers fails. + +## Quick Start + +Vend scoped S3 credentials to Spark through the IRC. Create the catalog through the Gravitino REST catalog API: + +```shell +curl -X POST http://localhost:8090/api/metalakes/{metalake}/catalogs \ +-H "Content-Type: application/json" \ +-d '{ + "name": "iceberg_catalog", + "type": "RELATIONAL", + "provider": "lakehouse-iceberg", + "properties": { + "catalog-backend": "jdbc", + "uri": "jdbc:postgresql://{postgres_host}:5432/{database}", + "jdbc-driver": "org.postgresql.Driver", + "jdbc-user": "{jdbc_user}", + "jdbc-password": "{jdbc_password}", + "jdbc-initialize": "true", + "warehouse": "s3://{bucket_name}/{warehouse_path}", + "io-impl": "org.apache.iceberg.aws.s3.S3FileIO", + "credential-providers": "s3-token", + "s3-access-key-id": "{access_key_id}", + "s3-secret-access-key": "{secret_access_key}", + "s3-region": "{region_name}", + "s3-role-arn": "{role_arn}" + } +}' +``` + +Point Spark at the IRC and request vended credentials with the delegation header: + +```shell +./bin/spark-sql -v \ +--packages org.apache.iceberg:iceberg-spark-runtime-3.5_2.12:1.10.0,org.apache.iceberg:iceberg-aws-bundle:1.10.0 \ +--conf spark.sql.extensions=org.apache.iceberg.spark.extensions.IcebergSparkSessionExtensions \ +--conf spark.sql.catalog.rest=org.apache.iceberg.spark.SparkCatalog \ +--conf spark.sql.catalog.rest.type=rest \ +--conf spark.sql.catalog.rest.uri=http://127.0.0.1:9001/iceberg/ \ +--conf spark.sql.catalog.rest.prefix=iceberg_catalog \ +--conf spark.sql.catalog.rest.header.X-Iceberg-Access-Delegation=vended-credentials +``` + +The role in `s3-role-arn` needs a trust policy and S3 permissions before this works. See [`s3-token`](#s3-token). + +For Trino instead of Spark: + +```properties +connector.name=iceberg +iceberg.catalog.type=rest +iceberg.rest-catalog.uri=http://127.0.0.1:9001/iceberg/ +iceberg.rest-catalog.prefix=iceberg_catalog +iceberg.rest-catalog.vended-credentials-enabled=true +fs.native-s3.enabled=true +s3.region={region_name} +``` + +For the full Trino setup, see [Connect Trino to the IRC](../iceberg-rest-engine/trino.md). + +## Setting Properties + +Credential vending properties go in the catalog's `properties` map when you create it through the Gravitino REST catalog API, alongside the warehouse location and the other catalog settings. + +The Gravitino Iceberg REST Catalog (IRC) is the exception. It can also read a catalog straight from `gravitino.conf`, using the same property names prefixed with `gravitino.iceberg-rest.`: + +```properties +s3-role-arn # as a catalog property +gravitino.iceberg-rest.s3-role-arn # in gravitino.conf +``` + +Catalogs defined in `gravitino.conf` are not registered in a metalake, so Gravitino access control does not apply to them. Privileges are granted on catalogs in a metalake, and there is nothing to grant them on. +>>>>>>> 10ba57615 ([#14069] fix(core): Reject token credential providers on catalogs without path-based credentials (#14080)) ## General Configurations @@ -38,13 +124,74 @@ Gravitino credential vending is used to generate temporary or static credentials | `credential-cache-expire-ratio` | `gravitino.iceberg-rest.credential-cache-expire-ratio` | Ratio of the credential's expiration time when Gravitino remove credential from the cache. | 0.15 | No | | `credential-cache-max-size` | `gravitino.iceberg-rest.cache-max-size` | Max size for the credential cache. | 10000 | No | +<<<<<<< HEAD If a catalog does not set `credential-providers`, Gravitino infers providers from the credential properties present. The inferred provider list is returned as the catalog's `credential-providers` property when the catalog is loaded, so clients such as the Spark connector see the same list the server vends credentials with. Only the provider names are returned, never the credential values. +======= +### Values for `credential-providers` + +| Value | Storage | Vends | +|----------------------|---------|------------------------------------------------------------| +| `s3-token` | S3 | Temporary STS credentials, scoped to the table path | +| `aws-irsa` | S3 | Credentials from an IAM role for service accounts, for EKS | +| `s3-secret-key` | S3 | The configured static access key and secret | +| `oss-token` | OSS | Temporary STS credentials, scoped to the table path | +| `oss-secret-key` | OSS | The configured static access key and secret | +| `adls-token` | ADLS | A user delegation SAS token | +| `azure-account-key` | ADLS | The configured static storage account key | +| `gcs-token` | GCS | A downscoped access token | +| `cos-token` | COS | A temporary STS token | +| `cos-secret-key` | COS | The configured static access key and secret | +| `jdbc-user-password` | JDBC | The configured JDBC username and password | +| `aws-secret-key` | Glue | The configured AWS access key and secret for Glue API auth | +| `dlf-secret-key` | Paimon | The configured DLF access key and secret (optional token) | + +Each value has its own properties, listed in the sections below. To vend for more than one storage type on a catalog, separate values with a comma. Custom providers can be added by implementing `CredentialProvider`, described under [Custom Credentials](#custom-credentials). + +### When `credential-providers` Is Omitted + +If a catalog does not set `credential-providers`, Gravitino infers providers from the credential properties present: + +| Properties present | Provider enabled | +|--------------------------------------------------------------|---------------------| +| `s3-access-key-id` and `s3-secret-access-key` | `s3-secret-key` | +| `oss-access-key-id` and `oss-secret-access-key` | `oss-secret-key` | +| `azure-storage-account-name` and `azure-storage-account-key` | `azure-account-key` | +| `cos-access-key-id` and `cos-secret-access-key` | `cos-secret-key` | +| `gcs-service-account-file` (Fileset and Iceberg only) | `gcs-token` | +| `aws-access-key-id` and `aws-secret-access-key` | `aws-secret-key` | +| `dlf-access-key-id` and `dlf-access-key-secret` | `dlf-secret-key` | + +JDBC catalogs additionally infer `jdbc-user-password` from `jdbc-user` and `jdbc-password`. + +The inferred provider list is returned as the catalog's `credential-providers` property when the catalog is loaded, so clients such as the Spark connector see the same list the server vends credentials with. Only the provider names are returned, never the credential values. + +Four providers have no inference rule and must always be set explicitly: `s3-token`, `oss-token`, `adls-token`, and `aws-irsa`. In particular, setting `s3-role-arn` without `credential-providers` does not enable `s3-token`. The catalog falls back to `s3-secret-key` and vends the static access key instead, which is long-lived and not scoped to the table path. Set `credential-providers` explicitly whenever you want token-based vending. + +### Static providers and `getCredentials` privilege risk + +Static providers such as `s3-secret-key`, `oss-secret-key`, `cos-secret-key`, `azure-account-key`, +`jdbc-user-password`, `aws-secret-key`, and `dlf-secret-key` return the configured long-lived +plaintext keys from `getCredentials` / `GET .../credentials`. That endpoint does **not** require a +dedicated privilege beyond being able to load the metadata object (unlike `getSecrets`, which +requires `USE_SECRETS`, with cloud access-key pairs gated by `INCLUDE_CREDENTIAL_SECRETS`). +>>>>>>> 10ba57615 ([#14069] fix(core): Reject token credential providers on catalogs without path-based credentials (#14080)) ## Build-in Credentials Configurations ### S3 Credentials +<<<<<<< HEAD #### S3 IRSA Credential +======= +- Prefer short-lived token providers (`s3-token`, `oss-token`, `adls-token`, `gcs-token`, + `cos-token`, `aws-irsa`) over static `*-secret-key` / `azure-account-key` / `jdbc-user-password` + whenever possible. These providers are only accepted on + Fileset and Iceberg catalogs. +- When static providers are unavoidable, grant load / use privileges only to trusted principals + (connectors and operators that must recover credentials). +- Do not treat `getCredentials` as a secrets-protected API for static keys; use `USE_SECRETS` / + `INCLUDE_CREDENTIAL_SECRETS` and `getSecrets` when you need privilege-gated plaintext access. +>>>>>>> 10ba57615 ([#14069] fix(core): Reject token credential providers on catalogs without path-based credentials (#14080)) A credential using AWS IAM Roles for Service Accounts (IRSA) to access S3 with temporary credentials, typically used in EKS environments. This provider supports both basic IRSA credentials and fine-grained path-based access control with dynamically generated IAM policies. @@ -225,7 +372,11 @@ Clients retrieve vended credentials from: GET /api/metalakes/{metalake}/objects/catalog/{catalog}/credentials ``` +<<<<<<< HEAD The server returns short-lived or static credentials that the client can use to access the underlying storage directly. +======= +The Gravitino Spark and Flink connectors call this for you and inject the returned credentials, so no client configuration is needed. These credentials are catalog-level and are not refreshed. Path-based providers such as `s3-token` and `aws-irsa` vend nothing at catalog level. +>>>>>>> 10ba57615 ([#14069] fix(core): Reject token credential providers on catalogs without path-based credentials (#14080)) ### Spark and Flink Connector Integration
