sunchao commented on code in PR #6023:
URL: https://github.com/apache/datafusion-comet/pull/6023#discussion_r4066755718
##########
spark/src/main/java/org/apache/comet/cloud/s3/CometS3CredentialDispatcher.java:
##########
@@ -74,19 +79,43 @@ public static long ensureInitialized(
catalogProperties == null
? Collections.emptyMap()
: Collections.unmodifiableMap(new HashMap<>(catalogProperties));
+ // Key on a digest of the properties, not the values themselves: the
KEY_TO_HANDLE map is
+ // static and lives for the JVM lifetime, and the property bag may carry
secrets (vended
+ // credentials, static keys). The full map is still handed to initialize()
below; only the
+ // long-lived cache key is reduced to a digest. A distinct config still
yields a distinct key.
InstanceKey key =
- new InstanceKey(providerClassName, dispatchKey == null ? "" :
dispatchKey, snapshot);
+ new InstanceKey(
+ providerClassName, dispatchKey == null ? "" : dispatchKey,
digestOf(snapshot));
return KEY_TO_HANDLE.computeIfAbsent(
key,
k -> {
- CometS3CredentialProvider provider =
instantiate(k.providerClassName);
- provider.initialize(k.catalogProperties);
+ CometS3CredentialProvider provider = instantiate(providerClassName);
+ provider.initialize(snapshot);
long handle = HANDLE_SEQ.getAndIncrement();
INSTANCES.put(handle, new RegisteredProvider(provider, k));
return handle;
});
}
+ /** Stable SHA-256 digest of the property bag, so secret values are not
retained in the key. */
+ private static String digestOf(Map<String, String> props) {
+ MessageDigest md;
+ try {
+ md = MessageDigest.getInstance("SHA-256");
+ } catch (NoSuchAlgorithmException e) {
+ throw new IllegalStateException("SHA-256 not available", e);
+ }
+ // Sort by key for order-independence; NUL separators avoid key/value
boundary ambiguity.
+ for (Map.Entry<String, String> e : new TreeMap<>(props).entrySet()) {
+ md.update(e.getKey().getBytes(StandardCharsets.UTF_8));
+ md.update((byte) 0);
+ String v = e.getValue();
+ md.update(v == null ? new byte[] {1} :
v.getBytes(StandardCharsets.UTF_8));
+ md.update((byte) 0);
Review Comment:
### Correctness
[P2] Preserve property boundaries before hashing the cache key
Could the digest input use length-prefixed fields (and an explicit null
tag), with a regression test for embedded U+0000? If an arbitrary vendor
property value contains NUL, the current separators are ambiguous:
`{"fs.s3a.a": V}` with `V = "x" + NUL + "fs.s3a.identity" + NUL + "TENANT-B"`
and `{"fs.s3a.a":"x","fs.s3a.identity":"TENANT-B"}` produce identical input
bytes. With the same provider class and dispatch key, I reproduced both maps
receiving the same handle and the second request returning the first map's
synthetic identity. Reversing their order reverses which identity wins. The
previous map-based key keeps them separate. The unfiltered Iceberg FileIO bag
reaches this method through protobuf strings and JNI's modified UTF-8
conversion without rejecting U+0000. This is conditional on such vendor values,
and comes from the serialization before SHA-256. Ordinary key changes still
separate correctly.
--
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]
