[diqiu50]

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.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[jerryshao]

Thanks for the fix. Rejecting path-based token providers on create/alter for catalogs that can't vend them looks right, and keeping load lenient is good for upgrade. A few entry points still let a token provider through, though (these lines are outside the diff, so I'm listing them here):

1. CatalogManager.java:1029: testConnection(ident, changes) skips the check
testConnection(ident, changes) validates proposed properties with validatePropertyForAlter but never calls validateCredentialProviders. alterCatalog does (line 1188), and this method then builds the temporary catalog with createCatalogWrapper(effectiveEntity, null), which skips the check again because propsToValidate is null. A Hive catalog change that sets credential-providers=gcs-token (or s3-token, oss-token, adls-token, cos-token) is reported as a successful connection test and then rejected on alter. Please call storedCatalog.validateCredentialProviders(alterProperty.getLeft()) here and cover it in TestCatalogManager.

2. CatalogManager.java:830 / :1703: secret-backed credential-providers bypasses the check
validateCredentialProviders only sees the string stored under credential-providers. On create, assembleSecretMaterials (line 830) replaces that value with a secret URN before createCatalogWrapper(e, mergedConfig) validates it, and checkSecretKeys rejects putting the key in both properties and secretBindings/secretReferences. Creating a Hive catalog with secretBindings.credential-providers plaintext gcs-token therefore passes, and propertiesWithCredentialProviders() later resolves the URN and vends gcs-token. Alter setSecretBinding is fine because getCatalogAlterProperty records binding.plaintext(). Alter setSecretReference is not: line 1703 puts the property name into the upsert map, then prepareCatalogChanges persists the reference URN. Please validate the plaintext provider list (binding plaintext, and the resolved reference) before the URN is substituted.

3. CatalogManager.java:1720: question about a load-time warning
Load uses createCatalogWrapper(convertedEntity, null), so a Hive catalog already stored with credential-providers=gcs-token still loads and still fails engine queries, with no warning. Keeping load successful is right for upgrade. Could we log a warning when the stored provider list contains a path-based token type and supportsPathBasedCredentials() is false, so operators can see the bad config without a failed startup? Is that in scope for this PR?

[jerryshao]

nit: supportsPathBasedCredentials() defaults to false, while path vending at request time is ops() instanceof SupportsPathBasedCredentials. Fileset sets both; Iceberg correctly overrides only the flag because the Iceberg REST server builds PathBasedCredentialContext itself. Please note on this method that a custom catalog whose operations implement SupportsPathBasedCredentials must override it, otherwise create/alter will reject s3-token even though getCredentials can vend a path. Same docs pass: the "prefer short-lived token providers" section in docs/security/credential-vending.md should say that those providers are only accepted on Fileset and Iceberg.

[yuqi1129]

The validation direction looks reasonable, but I think the new capability flag needs a compatibility path for existing catalog plugins. I also found a smaller gap in the load-time warning. The six targeted test classes passed (82 tests); neither case below is covered by them.