You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "adutra (via GitHub)" <gi...@apache.org> on 2025/09/24 17:01:26 UTC
[PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
adutra opened a new pull request, #14178:
URL: https://github.com/apache/iceberg/pull/14178
Fixes #14100.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#discussion_r2399133843
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
@@ -259,16 +260,41 @@ protected OAuth2Util.AuthSession newSessionFromTokenExchange(
protected OAuth2Util.AuthSession newSessionFromTokenResponse(
AuthConfig config, OAuth2Util.AuthSession parent) {
+ RESTClient client = refreshClient;
OAuthTokenResponse response =
OAuth2Util.fetchToken(
- refreshClient,
+ client,
Map.of(),
config.credential(),
config.scope(),
config.oauth2ServerUri(),
config.optionalOAuthParams());
return OAuth2Util.AuthSession.fromTokenResponse(
- refreshClient, refreshExecutor(), response, System.currentTimeMillis(), parent);
+ client, refreshExecutor(), response, System.currentTimeMillis(), parent);
+ }
+
+ private void maybeInitRefreshClient(RESTClient sharedClient, AuthSession session) {
+ if (refreshClient == null) {
+ synchronized (this) {
+ if (refreshClient == null) {
+ refreshClient = sharedClient.withAuthSession(session);
+ }
+ }
+ }
+ }
+
+ private AuthSessionCache getOrCreateSessionCache(Map<String, String> properties) {
+ AuthSessionCache cache = this.sessionCache;
Review Comment:
I was trying to save one volatile read.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#discussion_r2399165073
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
@@ -259,16 +260,41 @@ protected OAuth2Util.AuthSession newSessionFromTokenExchange(
protected OAuth2Util.AuthSession newSessionFromTokenResponse(
AuthConfig config, OAuth2Util.AuthSession parent) {
+ RESTClient client = refreshClient;
OAuthTokenResponse response =
OAuth2Util.fetchToken(
- refreshClient,
+ client,
Map.of(),
config.credential(),
config.scope(),
config.oauth2ServerUri(),
config.optionalOAuthParams());
return OAuth2Util.AuthSession.fromTokenResponse(
- refreshClient, refreshExecutor(), response, System.currentTimeMillis(), parent);
+ client, refreshExecutor(), response, System.currentTimeMillis(), parent);
+ }
+
+ private void maybeInitRefreshClient(RESTClient sharedClient, AuthSession session) {
Review Comment:
That won't be possible unfortunately, you need the parent session and the shared client to create the refresh client.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#discussion_r2405172267
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
@@ -173,14 +173,17 @@ public AuthSession tableSession(RESTClient sharedClient, Map<String, String> pro
sessionCache = newSessionCache(name, properties);
}
+ String oauth2ServerUri =
+ properties.getOrDefault(OAuth2Properties.OAUTH2_SERVER_URI, ResourcePaths.tokens());
Review Comment:
The signer endpoint would be just the first part of the key. The full cache key would be `<uri>:<token>` or `<uri>:<credential>`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#discussion_r2378320499
##########
core/src/test/java/org/apache/iceberg/rest/auth/TestOAuth2Manager.java:
##########
@@ -530,7 +531,12 @@ void standaloneTableSessionEmptyProperties() {
@Test
void standaloneTableSessionTokenProvided() {
- Map<String, String> tableProperties = Map.of(OAuth2Properties.TOKEN, "table-token");
+ Map<String, String> tableProperties =
+ Map.of(
+ OAuth2Properties.OAUTH2_SERVER_URI,
+ "https://auth-server.com/v1/token",
Review Comment:
I don't think we should change this here and further below
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#discussion_r2399187196
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
@@ -168,22 +168,23 @@ public AuthSession tableSession(RESTClient sharedClient, Map<String, String> pro
// Important: this method is invoked from standalone components; we must not assume that
// the refresh client and session cache have been initialized, because catalogSession()
// won't be called.
- if (refreshClient == null) {
- refreshClient = sharedClient.withAuthSession(parent);
- }
- if (sessionCache == null) {
- sessionCache = newSessionCache(name, properties);
- }
+ String oauth2ServerUri =
+ properties.getOrDefault(OAuth2Properties.OAUTH2_SERVER_URI, ResourcePaths.tokens());
if (config.token() != null) {
+ refreshClient(sharedClient, parent);
Review Comment:
sorry I'm just realizing now that the refresh client and the cache is used differently than I was expecting, which actually makes just calling `refreshClient(..)` here weird (since I was assuming we would update all places to just call `refreshClient(..)` and that method would return the initialized client.
Let me think a bit about this
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#discussion_r2402238988
##########
aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java:
##########
@@ -101,6 +102,11 @@ public String endpoint() {
return properties().getOrDefault(S3_SIGNER_ENDPOINT, S3_SIGNER_DEFAULT_ENDPOINT);
}
+ @Value.Lazy
+ public String endpointUri() {
Review Comment:
This method is public, so I didn't want to change its semantics.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#discussion_r2377921566
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
@@ -173,14 +174,17 @@ public AuthSession tableSession(RESTClient sharedClient, Map<String, String> pro
sessionCache = newSessionCache(name, properties);
}
+ String oauth2ServerUri = properties.get(OAuth2Properties.OAUTH2_SERVER_URI);
Review Comment:
this should be falling back to `ResourcePaths.tokens()` because this property is not mandatory
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#discussion_r2399146349
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
@@ -259,16 +260,41 @@ protected OAuth2Util.AuthSession newSessionFromTokenExchange(
protected OAuth2Util.AuthSession newSessionFromTokenResponse(
AuthConfig config, OAuth2Util.AuthSession parent) {
+ RESTClient client = refreshClient;
OAuthTokenResponse response =
OAuth2Util.fetchToken(
- refreshClient,
+ client,
Map.of(),
config.credential(),
config.scope(),
config.oauth2ServerUri(),
config.optionalOAuthParams());
return OAuth2Util.AuthSession.fromTokenResponse(
- refreshClient, refreshExecutor(), response, System.currentTimeMillis(), parent);
+ client, refreshExecutor(), response, System.currentTimeMillis(), parent);
+ }
+
+ private void maybeInitRefreshClient(RESTClient sharedClient, AuthSession session) {
+ if (refreshClient == null) {
+ synchronized (this) {
+ if (refreshClient == null) {
+ refreshClient = sharedClient.withAuthSession(session);
+ }
+ }
+ }
+ }
+
+ private AuthSessionCache getOrCreateSessionCache(Map<String, String> properties) {
+ AuthSessionCache cache = this.sessionCache;
Review Comment:
also all places need to call `sessionCache()`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#discussion_r2402690097
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
@@ -173,14 +173,17 @@ public AuthSession tableSession(RESTClient sharedClient, Map<String, String> pro
sessionCache = newSessionCache(name, properties);
}
+ String oauth2ServerUri =
+ properties.getOrDefault(OAuth2Properties.OAUTH2_SERVER_URI, ResourcePaths.tokens());
Review Comment:
I'm not sure that signer endpoint works in isolation. You can have two different tokens/sessions from two table loads and they're going to have the same sigher uri (the uri can be the same for all signing requests).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#issuecomment-3421143950
@danielcweeks PTAL again 🙏
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#discussion_r2378317089
##########
core/src/test/java/org/apache/iceberg/rest/auth/TestOAuth2Manager.java:
##########
@@ -509,7 +509,8 @@ void tableSessionDisallowedTableProperties() {
@Test
void standaloneTableSessionEmptyProperties() {
- Map<String, String> properties = Map.of();
+ Map<String, String> properties =
+ Map.of(OAuth2Properties.OAUTH2_SERVER_URI, "https://auth-server.com/v1/token");
Review Comment:
I guess this can be reverted, since the test says "empty properties" and we should be falling back to the tokens endpoint if the property isn't specified
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#discussion_r2399287155
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
@@ -259,16 +274,17 @@ protected OAuth2Util.AuthSession newSessionFromTokenExchange(
protected OAuth2Util.AuthSession newSessionFromTokenResponse(
AuthConfig config, OAuth2Util.AuthSession parent) {
+ RESTClient client = refreshClient;
Review Comment:
Was just trying to avoid 2 volatile reads while 1 is enough. I will revert.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks merged PR #14178:
URL: https://github.com/apache/iceberg/pull/14178
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#discussion_r2405172267
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
@@ -173,14 +173,17 @@ public AuthSession tableSession(RESTClient sharedClient, Map<String, String> pro
sessionCache = newSessionCache(name, properties);
}
+ String oauth2ServerUri =
+ properties.getOrDefault(OAuth2Properties.OAUTH2_SERVER_URI, ResourcePaths.tokens());
Review Comment:
The signer endpoint could be just the first part of the key. The full cache key could be `<uri>:<token>` or `<uri>:<credential>`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#discussion_r2398682597
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
Review Comment:
Another issue here is that I think we need to synchronize on the instantiation of this cache. Looking at `S3V4RestSignerClient` I think this method could be called by more than one thread in parallel. Wdyt?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#discussion_r2399139594
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
@@ -259,16 +260,41 @@ protected OAuth2Util.AuthSession newSessionFromTokenExchange(
protected OAuth2Util.AuthSession newSessionFromTokenResponse(
AuthConfig config, OAuth2Util.AuthSession parent) {
+ RESTClient client = refreshClient;
OAuthTokenResponse response =
OAuth2Util.fetchToken(
- refreshClient,
+ client,
Map.of(),
config.credential(),
config.scope(),
config.oauth2ServerUri(),
config.optionalOAuthParams());
return OAuth2Util.AuthSession.fromTokenResponse(
- refreshClient, refreshExecutor(), response, System.currentTimeMillis(), parent);
+ client, refreshExecutor(), response, System.currentTimeMillis(), parent);
+ }
+
+ private void maybeInitRefreshClient(RESTClient sharedClient, AuthSession session) {
Review Comment:
and also I think all access should always go through calling `refreshClient()` everywhere instead of using `refreshClient` directly
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#discussion_r2399229781
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
@@ -259,16 +260,41 @@ protected OAuth2Util.AuthSession newSessionFromTokenExchange(
protected OAuth2Util.AuthSession newSessionFromTokenResponse(
AuthConfig config, OAuth2Util.AuthSession parent) {
+ RESTClient client = refreshClient;
OAuthTokenResponse response =
OAuth2Util.fetchToken(
- refreshClient,
+ client,
Map.of(),
config.credential(),
config.scope(),
config.oauth2ServerUri(),
config.optionalOAuthParams());
return OAuth2Util.AuthSession.fromTokenResponse(
- refreshClient, refreshExecutor(), response, System.currentTimeMillis(), parent);
+ client, refreshExecutor(), response, System.currentTimeMillis(), parent);
+ }
+
+ private void maybeInitRefreshClient(RESTClient sharedClient, AuthSession session) {
Review Comment:
you're right, I've also just realized that :(
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#issuecomment-3530701778
@adutra Could you please back-port this fix to 1.10.x? Thanks!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#discussion_r2378124710
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
@@ -173,14 +174,17 @@ public AuthSession tableSession(RESTClient sharedClient, Map<String, String> pro
sessionCache = newSessionCache(name, properties);
}
+ String oauth2ServerUri = properties.get(OAuth2Properties.OAUTH2_SERVER_URI);
Review Comment:
It's connected imo: in the original issue reported by @c-thiel, if both catalogs use the same credential we would still get a 401 from the second one without this change.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#discussion_r2378323326
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
@@ -173,14 +174,17 @@ public AuthSession tableSession(RESTClient sharedClient, Map<String, String> pro
sessionCache = newSessionCache(name, properties);
}
+ String oauth2ServerUri = properties.get(OAuth2Properties.OAUTH2_SERVER_URI);
Review Comment:
is this something we could reproduce in a small test?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#issuecomment-3532570083
@huaxingao here it is: #14586
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#discussion_r2398484479
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
@@ -173,14 +173,17 @@ public AuthSession tableSession(RESTClient sharedClient, Map<String, String> pro
sessionCache = newSessionCache(name, properties);
}
+ String oauth2ServerUri =
+ properties.getOrDefault(OAuth2Properties.OAUTH2_SERVER_URI, ResourcePaths.tokens());
Review Comment:
True. I'll leave the decision up to you 🙂
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#discussion_r2402474578
##########
aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java:
##########
@@ -101,6 +102,11 @@ public String endpoint() {
return properties().getOrDefault(S3_SIGNER_ENDPOINT, S3_SIGNER_DEFAULT_ENDPOINT);
}
+ @Value.Lazy
+ public String endpointUri() {
Review Comment:
Is this actually changing the behavior though? We're just returning the endpoint constructed at request time rather than at creation time? I think the expectation is that whatever comes from the `endpoint()` call should reflect the url to be called.
If we think it's a significant behavior change, then we should deprecate the other so that we don't have two different paths for getting the endpoint (that's awkward to carry forward)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#discussion_r2402690097
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
@@ -173,14 +173,17 @@ public AuthSession tableSession(RESTClient sharedClient, Map<String, String> pro
sessionCache = newSessionCache(name, properties);
}
+ String oauth2ServerUri =
+ properties.getOrDefault(OAuth2Properties.OAUTH2_SERVER_URI, ResourcePaths.tokens());
Review Comment:
I'm not sure that signer endpoint works in isolation. You can have two different tokens/sessions from two table loads and they're going to have the same sigher uri (the uri can be teh same for all signing requests).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#issuecomment-3329880736
FYI @nastra
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#issuecomment-3505297466
Thanks @adutra !
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#discussion_r2394474961
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
@@ -173,14 +173,17 @@ public AuthSession tableSession(RESTClient sharedClient, Map<String, String> pro
sessionCache = newSessionCache(name, properties);
}
+ String oauth2ServerUri =
+ properties.getOrDefault(OAuth2Properties.OAUTH2_SERVER_URI, ResourcePaths.tokens());
Review Comment:
@nastra while adapting the Dremio AuthManager to this, I realized that there might be a better cache key:
the signer endpoint URI itself.
```java
String cacheKey = properties.getOrDefault("s3.signer.uri", properties.get(CatalogProperties.URI));
if (config.token() != null) {
return sessionCache.cachedSession(
cacheKey, k -> newSessionFromAccessToken(config.token(), properties, parent));
}
if (config.credential() != null && !config.credential().isEmpty()) {
return sessionCache.cachedSession(cacheKey, k -> newSessionFromTokenResponse(config, parent));
}
```
WDYT? Caching by signer endpoint seems more natural to me, since that's what we want conceptually: one session for each signing catalog.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#discussion_r2399270430
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
@@ -259,16 +274,17 @@ protected OAuth2Util.AuthSession newSessionFromTokenExchange(
protected OAuth2Util.AuthSession newSessionFromTokenResponse(
AuthConfig config, OAuth2Util.AuthSession parent) {
+ RESTClient client = refreshClient;
Review Comment:
nit: seems like an unrelated change
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#discussion_r2399187196
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
@@ -168,22 +168,23 @@ public AuthSession tableSession(RESTClient sharedClient, Map<String, String> pro
// Important: this method is invoked from standalone components; we must not assume that
// the refresh client and session cache have been initialized, because catalogSession()
// won't be called.
- if (refreshClient == null) {
- refreshClient = sharedClient.withAuthSession(parent);
- }
- if (sessionCache == null) {
- sessionCache = newSessionCache(name, properties);
- }
+ String oauth2ServerUri =
+ properties.getOrDefault(OAuth2Properties.OAUTH2_SERVER_URI, ResourcePaths.tokens());
if (config.token() != null) {
+ refreshClient(sharedClient, parent);
Review Comment:
sorry I'm just realizing now that the refresh client is used differently than I was expecting, which actually makes just calling `refreshClient(..)` here weird (since I was assuming we would update all places to just call `refreshClient(..)` and that method would return the initialized client.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#discussion_r2399166204
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
@@ -259,16 +260,41 @@ protected OAuth2Util.AuthSession newSessionFromTokenExchange(
protected OAuth2Util.AuthSession newSessionFromTokenResponse(
AuthConfig config, OAuth2Util.AuthSession parent) {
+ RESTClient client = refreshClient;
OAuthTokenResponse response =
OAuth2Util.fetchToken(
- refreshClient,
+ client,
Map.of(),
config.credential(),
config.scope(),
config.oauth2ServerUri(),
config.optionalOAuthParams());
return OAuth2Util.AuthSession.fromTokenResponse(
- refreshClient, refreshExecutor(), response, System.currentTimeMillis(), parent);
+ client, refreshExecutor(), response, System.currentTimeMillis(), parent);
+ }
+
+ private void maybeInitRefreshClient(RESTClient sharedClient, AuthSession session) {
+ if (refreshClient == null) {
+ synchronized (this) {
+ if (refreshClient == null) {
+ refreshClient = sharedClient.withAuthSession(session);
+ }
+ }
+ }
+ }
+
+ private AuthSessionCache getOrCreateSessionCache(Map<String, String> properties) {
+ AuthSessionCache cache = this.sessionCache;
Review Comment:
I did some simplification, PTAL again.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#discussion_r2399131581
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
@@ -259,16 +260,41 @@ protected OAuth2Util.AuthSession newSessionFromTokenExchange(
protected OAuth2Util.AuthSession newSessionFromTokenResponse(
AuthConfig config, OAuth2Util.AuthSession parent) {
+ RESTClient client = refreshClient;
OAuthTokenResponse response =
OAuth2Util.fetchToken(
- refreshClient,
+ client,
Map.of(),
config.credential(),
config.scope(),
config.oauth2ServerUri(),
config.optionalOAuthParams());
return OAuth2Util.AuthSession.fromTokenResponse(
- refreshClient, refreshExecutor(), response, System.currentTimeMillis(), parent);
+ client, refreshExecutor(), response, System.currentTimeMillis(), parent);
+ }
+
+ private void maybeInitRefreshClient(RESTClient sharedClient, AuthSession session) {
+ if (refreshClient == null) {
+ synchronized (this) {
+ if (refreshClient == null) {
+ refreshClient = sharedClient.withAuthSession(session);
+ }
+ }
+ }
+ }
+
+ private AuthSessionCache getOrCreateSessionCache(Map<String, String> properties) {
+ AuthSessionCache cache = this.sessionCache;
Review Comment:
I think this can be simplified to
```
private AuthSessionCache sessionCache(Map<String, String> properties) {
if (sessionCache == null) {
synchronized (this) {
if (sessionCache == null) {
this.sessionCache = newSessionCache(name, properties);
}
}
}
return sessionCache;
}
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#discussion_r2398865729
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
Review Comment:
yes I agree, we should move initialization of `sessionCache` and `refreshClient` into their own methods and synchronize access (similar to how it's done for `authManager()` in the signer)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#discussion_r2399113441
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
@@ -259,16 +260,41 @@ protected OAuth2Util.AuthSession newSessionFromTokenExchange(
protected OAuth2Util.AuthSession newSessionFromTokenResponse(
AuthConfig config, OAuth2Util.AuthSession parent) {
+ RESTClient client = refreshClient;
OAuthTokenResponse response =
OAuth2Util.fetchToken(
- refreshClient,
+ client,
Map.of(),
config.credential(),
config.scope(),
config.oauth2ServerUri(),
config.optionalOAuthParams());
return OAuth2Util.AuthSession.fromTokenResponse(
- refreshClient, refreshExecutor(), response, System.currentTimeMillis(), parent);
+ client, refreshExecutor(), response, System.currentTimeMillis(), parent);
+ }
+
+ private void maybeInitRefreshClient(RESTClient sharedClient, AuthSession session) {
Review Comment:
nit: why not just call this `refreshClient()`?
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
@@ -259,16 +260,41 @@ protected OAuth2Util.AuthSession newSessionFromTokenExchange(
protected OAuth2Util.AuthSession newSessionFromTokenResponse(
AuthConfig config, OAuth2Util.AuthSession parent) {
+ RESTClient client = refreshClient;
OAuthTokenResponse response =
OAuth2Util.fetchToken(
- refreshClient,
+ client,
Map.of(),
config.credential(),
config.scope(),
config.oauth2ServerUri(),
config.optionalOAuthParams());
return OAuth2Util.AuthSession.fromTokenResponse(
- refreshClient, refreshExecutor(), response, System.currentTimeMillis(), parent);
+ client, refreshExecutor(), response, System.currentTimeMillis(), parent);
+ }
+
+ private void maybeInitRefreshClient(RESTClient sharedClient, AuthSession session) {
+ if (refreshClient == null) {
+ synchronized (this) {
+ if (refreshClient == null) {
+ refreshClient = sharedClient.withAuthSession(session);
Review Comment:
```suggestion
this.refreshClient = sharedClient.withAuthSession(session);
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#discussion_r2399161274
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
@@ -259,16 +260,41 @@ protected OAuth2Util.AuthSession newSessionFromTokenExchange(
protected OAuth2Util.AuthSession newSessionFromTokenResponse(
AuthConfig config, OAuth2Util.AuthSession parent) {
+ RESTClient client = refreshClient;
OAuthTokenResponse response =
OAuth2Util.fetchToken(
- refreshClient,
+ client,
Map.of(),
config.credential(),
config.scope(),
config.oauth2ServerUri(),
config.optionalOAuthParams());
return OAuth2Util.AuthSession.fromTokenResponse(
- refreshClient, refreshExecutor(), response, System.currentTimeMillis(), parent);
+ client, refreshExecutor(), response, System.currentTimeMillis(), parent);
+ }
+
+ private void maybeInitRefreshClient(RESTClient sharedClient, AuthSession session) {
+ if (refreshClient == null) {
+ synchronized (this) {
+ if (refreshClient == null) {
+ refreshClient = sharedClient.withAuthSession(session);
+ }
+ }
+ }
+ }
+
+ private AuthSessionCache getOrCreateSessionCache(Map<String, String> properties) {
+ AuthSessionCache cache = this.sessionCache;
Review Comment:
This synchronization is not needed when calling `catalogSession()`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#discussion_r2397963835
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
@@ -173,14 +173,17 @@ public AuthSession tableSession(RESTClient sharedClient, Map<String, String> pro
sessionCache = newSessionCache(name, properties);
}
+ String oauth2ServerUri =
+ properties.getOrDefault(OAuth2Properties.OAUTH2_SERVER_URI, ResourcePaths.tokens());
Review Comment:
I guess the only issue with that is that we're leaking something s3-specific into this
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#discussion_r2399847635
##########
aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java:
##########
@@ -101,6 +102,11 @@ public String endpoint() {
return properties().getOrDefault(S3_SIGNER_ENDPOINT, S3_SIGNER_DEFAULT_ENDPOINT);
}
+ @Value.Lazy
+ public String endpointUri() {
Review Comment:
Why aren't we just changing the `endpoint()` behavior? It looks like it's now only being used by `endpointUri()`, so can't we just update how we return the endpoint?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#discussion_r2377916688
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
@@ -173,14 +174,17 @@ public AuthSession tableSession(RESTClient sharedClient, Map<String, String> pro
sessionCache = newSessionCache(name, properties);
}
+ String oauth2ServerUri = properties.get(OAuth2Properties.OAUTH2_SERVER_URI);
Review Comment:
isn't that a separate issue from the signer one?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #14178:
URL: https://github.com/apache/iceberg/pull/14178#discussion_r2376450995
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
@@ -173,14 +174,17 @@ public AuthSession tableSession(RESTClient sharedClient, Map<String, String> pro
sessionCache = newSessionCache(name, properties);
}
+ String oauth2ServerUri = properties.get(OAuth2Properties.OAUTH2_SERVER_URI);
Review Comment:
My rationale here is: two auth sessions for 2 different catalogs, but with the same credential, would be wrongly conflated since they would share the same cache key.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org