AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs #14178
Conversation
|
FYI @nastra |
| sessionCache = newSessionCache(name, properties); | ||
| } | ||
|
|
||
| String oauth2ServerUri = properties.get(OAuth2Properties.OAUTH2_SERVER_URI); |
There was a problem hiding this 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.
There was a problem hiding this comment.
isn't that a separate issue from the signer one?
There was a problem hiding this 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.
There was a problem hiding this comment.
is this something we could reproduce in a small test?
| sessionCache = newSessionCache(name, properties); | ||
| } | ||
|
|
||
| String oauth2ServerUri = properties.get(OAuth2Properties.OAUTH2_SERVER_URI); |
There was a problem hiding this comment.
this should be falling back to ResourcePaths.tokens() because this property is not mandatory
| void standaloneTableSessionEmptyProperties() { | ||
| Map<String, String> properties = Map.of(); | ||
| Map<String, String> properties = | ||
| Map.of(OAuth2Properties.OAUTH2_SERVER_URI, "https://auth-server.com/v1/token"); |
There was a problem hiding this 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
| Map<String, String> tableProperties = | ||
| Map.of( | ||
| OAuth2Properties.OAUTH2_SERVER_URI, | ||
| "https://auth-server.com/v1/token", |
There was a problem hiding this comment.
I don't think we should change this here and further below
Fixes #14100.