AWS: Isolate S3V4RestSignerClient signed request cache per principal … - #17914
AWS: Isolate S3V4RestSignerClient signed request cache per principal …#17914SHIVANSH-ux-ys wants to merge 4 commits into
Conversation
singhpk234
left a comment
singhpk234
left a comment
There was a problem hiding this comment.
thanks for the change @SHIVANSH-ux-ys, can you please add test for it
| .credential(signerClient.credential()) | ||
| .token(signerClient.token().get()) |
There was a problem hiding this comment.
it should ideally either credential or token right not both because of case when credentials is exchanged for token ?
There was a problem hiding this comment.
Thanks for reviewing @singhpk234!
Makes sense - updated Key.from to prioritize token if set and fallback to credential. Also added a unit test (testSignedComponentCacheKeyIsolation) in TestS3V4RestSignerClient.java to verify cache key isolation across different credentials.
Pushed the updates!
There was a problem hiding this comment.
I agree with using either the token or the credential rather than both. My remaining concern is whether either value alone is sufficient to define the principal boundary for a process-wide cache.
The auth session is derived from more than these two values (AuthManager, OAuth server URI, scope/resource/audience, and potentially non-OAuth auth types). For example, two signer clients can use the same credential against different OAuth servers/resources, while non-OAuth principals may have both token and credential unset. Those clients would still produce the same cache key here.
Would it be simpler and safer to scope the signed-component cache to the S3V4RestSignerClient instance instead? That makes the isolation structural rather than trying to encode every authentication dimension into Key, and it also avoids retaining bearer tokens/client secrets in a process-wide cache key.
FWIW, #16524 has already been updated to make this cache per-instance, so it may be worth coordinating the two fixes.
…n/credential precedence
| S3V4RestSignerClient.Key key1 = S3V4RestSignerClient.Key.from(request, client1); | ||
| S3V4RestSignerClient.Key key2 = S3V4RestSignerClient.Key.from(request, client2); | ||
|
|
||
| assertThat(key1).isNotEqualTo(key2); |
There was a problem hiding this comment.
Could we make this a regression test for the actual cache behavior rather than testing Key.equals directly?
For #17801, I'd expect the test to populate a cacheable signed response through signer/client A, then make the same method/region/URI request through signer/client B and verify that B still calls its signer and receives B's signed component. That directly protects the security invariant and doesn't couple the test to the current cache-key implementation.
| } | ||
|
|
||
| @Test | ||
| void testSignedComponentCacheKeyIsolation() throws Exception { |
There was a problem hiding this comment.
Nit: Iceberg's current convention for newly added tests avoids the test prefix, so this could be signedComponentCacheKeyIsolation() or, if converted to a behavior level regression test, something like doesNotReuseSignedComponentAcrossSignerClients().
|
Thanks @yangshangqing95! Refactored Also updated the test name to |
3 participants
…(#17801)