From e56afb76807b66e8f1b6dc8c3e174a97ad72a5f6 Mon Sep 17 00:00:00 2001 From: okumin Date: Mon, 10 Nov 2025 19:28:35 +0900 Subject: [PATCH] Core: Stop fetchToken from sending the Authorization header --- .../apache/iceberg/rest/auth/OAuth2Util.java | 10 +- .../apache/iceberg/rest/TestRESTCatalog.java | 121 ++++++++++++++---- .../iceberg/rest/auth/TestOAuth2Manager.java | 6 +- 3 files changed, 107 insertions(+), 30 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Util.java b/core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Util.java index c2b47e6e944f..3dfcea02ca4e 100644 --- a/core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Util.java +++ b/core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Util.java @@ -34,6 +34,7 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; import java.util.regex.Pattern; +import java.util.stream.Collectors; import org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting; import org.apache.iceberg.relocated.com.google.common.base.Joiner; import org.apache.iceberg.relocated.com.google.common.base.Preconditions; @@ -243,7 +244,14 @@ public static OAuthTokenResponse fetchToken( oauth2ServerUri, request, OAuthTokenResponse.class, - headers, + // RFC 6749 proposes two ways to send a credential: HTTP Basic authentication or + // request-body. Historically, the Iceberg library prefers the latter one for + // compatibility while the RFC recommends the former one. As sending both the + // Authorization header and the request-body parameters might confuse some authorization + // servers, the following line is excluding the Authorization header. + headers.entrySet().stream() + .filter(entry -> !AUTHORIZATION_HEADER.equals(entry.getKey())) + .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)), ErrorHandlers.oauthErrorHandler()); response.validate(); diff --git a/core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java b/core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java index 59e91150eeae..6b80c86ed7cf 100644 --- a/core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java +++ b/core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java @@ -415,15 +415,21 @@ public void testCatalogCredentialNoOauth2ServerUri() { RESTCatalog catalog = new RESTCatalog(SessionCatalog.SessionContext.createEmpty(), (config) -> adapter); + String catalogCredential = "catalog:secret"; catalog.initialize( - "prod", ImmutableMap.of(CatalogProperties.URI, "ignored", "credential", "catalog:secret")); + "prod", ImmutableMap.of(CatalogProperties.URI, "ignored", "credential", catalogCredential)); assertThat(catalog.tableExists(TBL)).isFalse(); // no token or credential for catalog token exchange Mockito.verify(adapter) .execute( - reqMatcher(HTTPMethod.POST, ResourcePaths.tokens(), emptyHeaders), + reqMatcher( + HTTPMethod.POST, + ResourcePaths.tokens(), + emptyHeaders, + ImmutableMap.of(), + createClientCredentialsRequest(catalogCredential)), eq(OAuthTokenResponse.class), any(), any()); @@ -454,13 +460,14 @@ public void testCatalogCredential(String oauth2ServerUri) { RESTCatalog catalog = new RESTCatalog(SessionCatalog.SessionContext.createEmpty(), (config) -> adapter); + String catalogCredential = "catalog:secret"; catalog.initialize( "prod", ImmutableMap.of( CatalogProperties.URI, "ignored", "credential", - "catalog:secret", + catalogCredential, OAuth2Properties.OAUTH2_SERVER_URI, oauth2ServerUri)); @@ -469,7 +476,12 @@ public void testCatalogCredential(String oauth2ServerUri) { // no token or credential for catalog token exchange Mockito.verify(adapter) .execute( - reqMatcher(HTTPMethod.POST, oauth2ServerUri, emptyHeaders), + reqMatcher( + HTTPMethod.POST, + oauth2ServerUri, + emptyHeaders, + ImmutableMap.of(), + createClientCredentialsRequest(catalogCredential)), eq(OAuthTokenResponse.class), any(), any()); @@ -498,11 +510,12 @@ public void testCatalogBearerTokenWithClientCredential(String oauth2ServerUri) { RESTCatalogAdapter adapter = Mockito.spy(new RESTCatalogAdapter(backendCatalog)); + String contextCredential = "user:secret"; SessionCatalog.SessionContext context = new SessionCatalog.SessionContext( UUID.randomUUID().toString(), "user", - ImmutableMap.of("credential", "user:secret"), + ImmutableMap.of("credential", contextCredential), ImmutableMap.of()); RESTCatalog catalog = new RESTCatalog(context, (config) -> adapter); @@ -525,10 +538,15 @@ public void testCatalogBearerTokenWithClientCredential(String oauth2ServerUri) { eq(ConfigResponse.class), any(), any()); - // use the bearer token to fetch the context token + // use the request-body to fetch the context token Mockito.verify(adapter) .execute( - reqMatcher(HTTPMethod.POST, oauth2ServerUri, catalogHeaders), + reqMatcher( + HTTPMethod.POST, + oauth2ServerUri, + ImmutableMap.of(), + ImmutableMap.of(), + createClientCredentialsRequest(contextCredential)), eq(OAuthTokenResponse.class), any(), any()); @@ -552,21 +570,23 @@ public void testCatalogCredentialWithClientCredential(String oauth2ServerUri) { RESTCatalogAdapter adapter = Mockito.spy(new RESTCatalogAdapter(backendCatalog)); + String contextCredential = "user:secret"; SessionCatalog.SessionContext context = new SessionCatalog.SessionContext( UUID.randomUUID().toString(), "user", - ImmutableMap.of("credential", "user:secret"), + ImmutableMap.of("credential", contextCredential), ImmutableMap.of()); RESTCatalog catalog = new RESTCatalog(context, (config) -> adapter); + String catalogCredential = "catalog:secret"; catalog.initialize( "prod", ImmutableMap.of( CatalogProperties.URI, "ignored", "credential", - "catalog:secret", + catalogCredential, OAuth2Properties.OAUTH2_SERVER_URI, oauth2ServerUri)); @@ -575,7 +595,12 @@ public void testCatalogCredentialWithClientCredential(String oauth2ServerUri) { // call client credentials with no initial auth Mockito.verify(adapter) .execute( - reqMatcher(HTTPMethod.POST, oauth2ServerUri, emptyHeaders), + reqMatcher( + HTTPMethod.POST, + oauth2ServerUri, + emptyHeaders, + ImmutableMap.of(), + createClientCredentialsRequest(catalogCredential)), eq(OAuthTokenResponse.class), any(), any()); @@ -589,7 +614,12 @@ public void testCatalogCredentialWithClientCredential(String oauth2ServerUri) { // use the client credential to fetch the context token Mockito.verify(adapter) .execute( - reqMatcher(HTTPMethod.POST, oauth2ServerUri, catalogHeaders), + reqMatcher( + HTTPMethod.POST, + oauth2ServerUri, + emptyHeaders, + ImmutableMap.of(), + createClientCredentialsRequest(contextCredential)), eq(OAuthTokenResponse.class), any(), any()); @@ -613,11 +643,12 @@ public void testCatalogBearerTokenAndCredentialWithClientCredential(String oauth RESTCatalogAdapter adapter = Mockito.spy(new RESTCatalogAdapter(backendCatalog)); + String contextCredential = "user:secret"; SessionCatalog.SessionContext context = new SessionCatalog.SessionContext( UUID.randomUUID().toString(), "user", - ImmutableMap.of("credential", "user:secret"), + ImmutableMap.of("credential", contextCredential), ImmutableMap.of()); RESTCatalog catalog = new RESTCatalog(context, (config) -> adapter); @@ -652,7 +683,12 @@ public void testCatalogBearerTokenAndCredentialWithClientCredential(String oauth // use the client credential to fetch the context token Mockito.verify(adapter) .execute( - reqMatcher(HTTPMethod.POST, oauth2ServerUri, catalogHeaders), + reqMatcher( + HTTPMethod.POST, + oauth2ServerUri, + ImmutableMap.of(), + ImmutableMap.of(), + createClientCredentialsRequest(contextCredential)), eq(OAuthTokenResponse.class), any(), any()); @@ -828,7 +864,19 @@ private void testClientAuth( // token passes a static token. otherwise, validate a client credentials or token exchange // request - if (!credentials.containsKey("token")) { + if (!credentials.containsKey("token") && credentials.containsKey("credential")) { + Mockito.verify(adapter) + .execute( + reqMatcher( + HTTPMethod.POST, + oauth2ServerUri, + ImmutableMap.of(), + ImmutableMap.of(), + createClientCredentialsRequest(credentials.get("credential"))), + eq(OAuthTokenResponse.class), + any(), + any()); + } else if (!credentials.containsKey("token")) { Mockito.verify(adapter) .execute( reqMatcher(HTTPMethod.POST, oauth2ServerUri, catalogHeaders), @@ -1294,13 +1342,14 @@ public void testCatalogTokenRefresh(String oauth2ServerUri) { UUID.randomUUID().toString(), "user", contextCredentials, ImmutableMap.of()); RESTCatalog catalog = new RESTCatalog(context, (config) -> adapter); + String catalogCredential = "catalog:secret"; catalog.initialize( "prod", ImmutableMap.of( CatalogProperties.URI, "ignored", "credential", - "catalog:secret", + catalogCredential, OAuth2Properties.OAUTH2_SERVER_URI, oauth2ServerUri)); @@ -1311,7 +1360,12 @@ public void testCatalogTokenRefresh(String oauth2ServerUri) { // call client credentials with no initial auth Mockito.verify(adapter) .execute( - reqMatcher(HTTPMethod.POST, oauth2ServerUri, emptyHeaders), + reqMatcher( + HTTPMethod.POST, + oauth2ServerUri, + emptyHeaders, + ImmutableMap.of(), + createClientCredentialsRequest(catalogCredential)), eq(OAuthTokenResponse.class), any(), any()); @@ -1400,13 +1454,14 @@ public void testCatalogRefreshedTokenIsUsed(String oauth2ServerUri) { UUID.randomUUID().toString(), "user", contextCredentials, ImmutableMap.of()); RESTCatalog catalog = new RESTCatalog(context, (config) -> adapter); + String catalogCredential = "catalog:secret"; catalog.initialize( "prod", ImmutableMap.of( CatalogProperties.URI, "ignored", "credential", - "catalog:secret", + catalogCredential, OAuth2Properties.OAUTH2_SERVER_URI, oauth2ServerUri)); @@ -1418,12 +1473,6 @@ public void testCatalogRefreshedTokenIsUsed(String oauth2ServerUri) { assertThat(catalog.tableExists(TBL)).isFalse(); // call client credentials with no initial auth - Map clientCredentialsRequest = - ImmutableMap.of( - "grant_type", "client_credentials", - "client_id", "catalog", - "client_secret", "secret", - "scope", "catalog"); Mockito.verify(adapter) .execute( reqMatcher( @@ -1431,7 +1480,7 @@ public void testCatalogRefreshedTokenIsUsed(String oauth2ServerUri) { oauth2ServerUri, emptyHeaders, Map.of(), - clientCredentialsRequest), + createClientCredentialsRequest(catalogCredential)), eq(OAuthTokenResponse.class), any(), any()); @@ -1516,13 +1565,14 @@ public void testCatalogTokenRefreshExchangeDisabled(String oauth2ServerUri) { UUID.randomUUID().toString(), "user", contextCredentials, ImmutableMap.of()); RESTCatalog catalog = new RESTCatalog(context, (config) -> adapter); + String catalogCredential = "catalog:secret"; catalog.initialize( "prod", ImmutableMap.of( CatalogProperties.URI, "ignored", OAuth2Properties.CREDENTIAL, - "catalog:secret", + catalogCredential, OAuth2Properties.TOKEN_EXCHANGE_ENABLED, "false", OAuth2Properties.OAUTH2_SERVER_URI, @@ -1535,7 +1585,12 @@ public void testCatalogTokenRefreshExchangeDisabled(String oauth2ServerUri) { // call client credentials with no initial auth Mockito.verify(adapter) .execute( - reqMatcher(HTTPMethod.POST, oauth2ServerUri, emptyHeaders), + reqMatcher( + HTTPMethod.POST, + oauth2ServerUri, + emptyHeaders, + ImmutableMap.of(), + createClientCredentialsRequest(catalogCredential)), eq(OAuthTokenResponse.class), any(), any()); @@ -3163,4 +3218,18 @@ private static List allRequests(RESTCatalogAdapter adapter) { verify(adapter, atLeastOnce()).execute(captor.capture(), any(), any(), any()); return captor.getAllValues(); } + + private static Map createClientCredentialsRequest(String credential) { + String[] parsed = credential.split(":"); + assertThat(parsed.length).isEqualTo(2); + return ImmutableMap.of( + "grant_type", + "client_credentials", + "client_id", + parsed[0], + "client_secret", + parsed[1], + "scope", + "catalog"); + } } diff --git a/core/src/test/java/org/apache/iceberg/rest/auth/TestOAuth2Manager.java b/core/src/test/java/org/apache/iceberg/rest/auth/TestOAuth2Manager.java index 4b57e990a8fa..cfd5cf7a4a2d 100644 --- a/core/src/test/java/org/apache/iceberg/rest/auth/TestOAuth2Manager.java +++ b/core/src/test/java/org/apache/iceberg/rest/auth/TestOAuth2Manager.java @@ -230,7 +230,7 @@ void contextualSessionCredentialsProvided() { SessionCatalog.SessionContext context = new SessionCatalog.SessionContext( "test", "test", Map.of(OAuth2Properties.CREDENTIAL, "client:secret"), Map.of()); - Map properties = Map.of(); + Map properties = Map.of(OAuth2Properties.CREDENTIAL, "client:secret"); try (OAuth2Manager manager = new OAuth2Manager("test"); OAuth2Util.AuthSession catalogSession = manager.catalogSession(client, properties); OAuth2Util.AuthSession contextualSession = @@ -243,7 +243,7 @@ void contextualSessionCredentialsProvided() { .as("should create session cache for context with credentials") .satisfies(cache -> assertThat(cache.sessionCache().asMap()).hasSize(1)); } - Mockito.verify(client) + Mockito.verify(client, times(2)) .postForm( any(), eq( @@ -255,7 +255,7 @@ void contextualSessionCredentialsProvided() { eq(OAuthTokenResponse.class), eq(Map.of()), any()); - Mockito.verify(client).withAuthSession(any()); + Mockito.verify(client, times(2)).withAuthSession(any()); Mockito.verifyNoMoreInteractions(client); }