[Catalog Federation] Add Connection Credential Vendors for Other Auth Types #2782

XJDKC · 2025-10-09T01:07:04Z

Add Connection Credential Vendors for Other Auth Types

This change is a prerequisite for enabling connection credential caching.
By making PolarisCredentialManager the central entry point for obtaining connection credentials, we can introduce caching cleanly and manage all credential flows in a consistent way.

PolarisCredentialManager::getConnectionCredentials will delegate the call to the appropriate credential vendor based on the authentication type:

SigV4 -> SigV4ConnectionCredentialVendor
- ServiceIdentityProvider: Uses ServiceIdentityProvider to retrieve the IAM user’s AWS credential that represents Polaris’s service identity.
- StsClient: Then use StsClient to assume the user-provided iam role and obtain temporary AWS credentials.
OAuth -> OAuthConnectionCredentialVendor
- UserSecretsManager: Uses UserSecretsManager to retrieve the OAuth2 credential from the secret manager.
Bearer -> BearerConnectionCredentialVendor
- UserSecretsManager: Uses UserSecretsManager to retrieve the bearer token from the secret manager.
Implicit -> ImplicitConnectionCredentialVendor
- A no-op vendor used when no authentication credentials are required.

With this structure, we no longer need to use UserSecretsManager directly inside IcebergCatalogPropertiesProvider::asIcebergCatalogProperties.

This unifies credential retrieval across all authentication types and simplifies the overall credential management flow.

…ebergCatalogProperties

dimas-b

LGTM overall 👍 Some comments below.

...core/src/main/java/org/apache/polaris/core/credentials/connection/CatalogAccessProperty.java

dimas-b · 2025-10-10T00:04:27Z

.../java/org/apache/polaris/service/credentials/connection/OAuthConnectionCredentialVendor.java

+    // OAuth credentials don't expire - set expiration to Instant.MAX to indicate infinite validity
+    // If the credential expires, users need to update the catalog entity to rotate the credential.
+    return ConnectionCredentials.builder()
+        .putCredential(CatalogAccessProperty.OAUTH2_CREDENTIAL.getPropertyName(), credential)


There is nothing in this class that actually relates to the OAuth2 spec as far as I can tell. It only relates to the Iceberg OAuth2Properties.CREDENTIAL constant, which again has very little to do with OAuth2 RFCs as far as connection configuration is concerned... It's just a property name plus format spec 🤷

What the client does with this property (whether it's OAuth2 or something else) is beyond the scope of this credential vendor. At this level we do not know how the credential is used.

I'd propose to rename this to "Iceberg Credential Vendor".

Yes, you're right, this class currently just retrieves the OAuth credential from UserSecretsManager and returns it. The PolarisCredentialManager will then cache it in AccessCredsCache (not yet implemented, I plan to add that in a follow-up PR) so that we don't need to fetch the credential from UserSecretsManager every time we make a call, especially if the secret manager doesn't have its own cache.

The main goal of this PR is to make PolarisCredentialManager the single entry point for credential retrieval so we can centralize the caching logic in one place.

Regarding your suggestion, I agree, it's still beneficial to include OAuth in the class name to make the purpose of this class clear at a glance.

Right now, the class only returns the stored OAuth credential, but in the future, we could extend it to perform the client credentials flow to obtain a temporary token directly here, rather than relying on the Iceberg SDK to fetch a new token for every request.

WDYT?

I fully support implementing OAuth2 flows in the future.

However, in its current form this credential vendor provides a simple pair of values, but claims the type of AuthenticationType.OAUTH. I personally find it misleading 🤔

Even if Polaris supported OAuth2 flows to obtain a token for a catalog connection, this class populates Iceberg's credential property, which is not a token. This property cannot hold an OAuth2 token. Instead it instruct the (Iceberg) client to perform some activities internally in order to obtain the access token. From the Polaris POV OAuth is not apparent.

Correct, how about renaming it to OAuthClientCredentialVendor then add a real OAuthConnectionCredentialVendor to perform the oauth2 flow and get a token?

.../org/apache/polaris/service/credentials/connection/BearerConnectionCredentialVendorTest.java

...rg/apache/polaris/service/credentials/connection/ImplicitConnectionCredentialVendorTest.java

...a/org/apache/polaris/service/credentials/connection/OAuthConnectionCredentialVendorTest.java

.../java/org/apache/polaris/service/credentials/connection/SigV4ConnectionCredentialVendor.java

HonahX

Thanks for working on this! It looks great!

HonahX · 2025-10-10T17:19:42Z

...core/src/main/java/org/apache/polaris/core/connection/BearerAuthenticationParametersDpo.java

  @Override
  public @Nonnull Map<String, String> asIcebergCatalogProperties(
-      UserSecretsManager secretsManager, PolarisCredentialManager credentialManager) {
-    String bearerToken = secretsManager.readSecret(getBearerTokenReference());
-    return Map.of(OAuth2Properties.TOKEN, bearerToken);
+      PolarisCredentialManager credentialManager) {
+    // Return only metadata properties - credentials are handled by ConnectionCredentialVendor
+    // Bearer auth has no metadata properties
+    return Map.of();


Could we make this a default implementation in abstract class AuthenticationParametersDpo or the interface?

HonahX · 2025-10-10T21:23:26Z

runtime/service/src/main/java/org/apache/polaris/service/config/ProductionReadinessChecks.java

+                      "Catalog federation is enabled but no ConnectionCredentialVendor found for "
+                          + "authentication type '%s'. External catalog connections using this "
+                          + "authentication type will fail.",
+                      type),


Just curious, would this check be too strong that we only consider it to be production ready if all auth types need the vendor, or this is more like a warning despite it is called "Error"?

Yeah, naming needs improvement here. An error production readiness check will not block startup. Only severe cases break startup.

I'm not sure the check is logically correct. A particular deployment only needs to provide ConnectionCredentialVendor implementations for auth types that are actually used by created catalogs.

I think the vendor availability is implicitly checked on catalog creation already.

On startup, I think we only need to check types for existing catalogs, not for all types listed in the enum. The latter would put an undue burden on custom deployments.

HonahX · 2025-10-10T21:25:16Z

...java/org/apache/polaris/service/credentials/connection/BearerConnectionCredentialVendor.java

+ */
+@RequestScoped
+@AuthType(AuthenticationType.BEARER)
+@Priority(100)


Could we extract the priority to some constants class, like FilterPriorities. It will be easier for custom implementation to use and managed the prioty.

XJDKC added 2 commits October 8, 2025 17:19

Add ConnectionCredentialVendor for other auth types

377fcf5

Remove UserSecretsManager from IcebergCatalogPropertiesProvider::asIc…

d75ebe6

…ebergCatalogProperties

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Oct 9, 2025

github-project-automation bot added this to Basic Kanban Board Oct 9, 2025

Fix ExternalCatalogFactory::createGenericCatalog

119e010

dimas-b reviewed Oct 10, 2025

View reviewed changes

XJDKC added 3 commits October 10, 2025 11:39

Add a production readiness check for Connection Credential Vendors

971cab9

Fix imports in tests

cff683a

Update CatalogAccessProperty

2ca565d

HonahX reviewed Oct 10, 2025

View reviewed changes

[Catalog Federation] Add Connection Credential Vendors for Other Auth Types #2782

Are you sure you want to change the base?

[Catalog Federation] Add Connection Credential Vendors for Other Auth Types #2782

Uh oh!

Conversation

XJDKC commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dimas-b Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

XJDKC Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

XJDKC Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HonahX left a comment

Choose a reason for hiding this comment

Uh oh!

HonahX Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

HonahX Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

dimas-b Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HonahX Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

XJDKC commented Oct 9, 2025 •

edited

Loading

XJDKC Oct 10, 2025 •

edited

Loading

dimas-b Oct 10, 2025 •

edited

Loading

dimas-b Oct 10, 2025 •

edited

Loading

dimas-b Oct 10, 2025 •

edited

Loading