AWS, S3 Signing: Fix leaked credentials when contacting multiple catalogs

[adutra]

Fixes #14100.

[adutra]

FYI @nastra

[adutra]

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.

[nastra]

isn't that a separate issue from the signer one?

[adutra]

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.

[nastra]

is this something we could reproduce in a small test?

[nastra]

this should be falling back to ResourcePaths.tokens() because this property is not mandatory

[nastra]

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

[adutra]

@nastra while adapting the Dremio AuthManager to this, I realized that there might be a better cache key:
the signer endpoint URI itself.

    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.

[nastra]

I guess the only issue with that is that we're leaking something s3-specific into this

[adutra]

True. I'll leave the decision up to you 🙂

[danielcweeks]

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).

[adutra]

The signer endpoint could be just the first part of the key. The full cache key could be <uri>:<token> or <uri>:<credential>.

[nastra]

Suggested change

	refreshClient = sharedClient.withAuthSession(session);
	this.refreshClient = sharedClient.withAuthSession(session);

[nastra]

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;
  }

[adutra]

I was trying to save one volatile read.

[nastra]

also all places need to call sessionCache()

[adutra]

This synchronization is not needed when calling catalogSession().

[adutra]

I did some simplification, PTAL again.

[nastra]

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

[nastra]

nit: seems like an unrelated change

[adutra]

Was just trying to avoid 2 volatile reads while 1 is enough. I will revert.

[danielcweeks]

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?

[adutra]

This method is public, so I didn't want to change its semantics.

[danielcweeks]

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)

[adutra]

@danielcweeks PTAL again 🙏

[danielcweeks]

Thanks @adutra !

[huaxingao]

@adutra Could you please back-port this fix to 1.10.x? Thanks!

[adutra]

@huaxingao here it is: #14586