yangshangqing95 commented on code in PR #17914:
URL: https://github.com/apache/iceberg/pull/17914#discussion_r3907034694
##########
aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java:
##########
@@ -400,11 +400,19 @@ interface Key {
String uri();
- static Key from(RemoteSignRequest request) {
+ @Nullable
+ String credential();
+
+ @Nullable
+ String token();
+
+ static Key from(RemoteSignRequest request, S3V4RestSignerClient
signerClient) {
return ImmutableKey.builder()
.method(request.method())
.region(request.region())
.uri(request.uri().toString())
+ .credential(signerClient.credential())
+ .token(signerClient.token().get())
Review 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.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
