gnodet-bot commented on code in PR #29098:
URL: https://github.com/apache/camel/pull/29098#discussion_r4456272728
##########
components/camel-pqc/src/main/java/org/apache/camel/component/pqc/PQCProducer.java:
##########
@@ -572,14 +577,37 @@ protected void doStop() throws Exception {
healthCheckRepository.removeHealthCheck(producerHealthCheck);
producerHealthCheck = null;
}
+ releaseClaimedStatefulKeys();
metrics.close();
metrics = PQCMetrics.NOOP;
super.doStop();
}
+ /**
+ * Gives back the stateful keys this producer claimed, so another process
can take them over once this one is
+ * stopped. A claim that cannot be released is logged rather than failing
the stop: the operating system releases it
+ * with the process anyway.
+ */
+ private void releaseClaimedStatefulKeys() {
+ KeyLifecycleManager klm = getConfiguration().getKeyLifecycleManager();
+ if (klm == null) {
+ claimedStatefulKeys.clear();
+ return;
+ }
+ for (String keyId : claimedStatefulKeys) {
Review Comment:
⚠️ **Shared `KeyLifecycleManager` instance: stopping one producer releases
another's claim**
`claimedStatefulKeys` is per-`PQCProducer` instance, but
`FileBasedKeyLifecycleManager.statefulClaims` is shared across all producers
that use the same KLM instance. The flow:
1. Producer A claims `keyId` → KLM opens a `FileChannel`, stores it in
`statefulClaims`, holds the OS lock.
2. Producer B calls `klm.claimStatefulKey(keyId)` → hits
`statefulClaims.containsKey(keyId)` → returns `true` immediately, adds `keyId`
to its own `claimedStatefulKeys`.
3. Producer A stops → `releaseClaimedStatefulKeys()` calls
`klm.releaseStatefulKey(keyId)` → KLM removes from `statefulClaims`, closes the
channel, **releases the OS lock**.
4. Producer B continues to sign, believing it holds an exclusive claim — but
the OS lock is gone, and another process can now claim the same key.
The KLM's `claimStatefulKey` is not reference-counted. If multiple producers
share one KLM instance, the claim should be reference-counted or the KLM design
should document that sharing is not safe for stateful keys.
##########
components/camel-pqc/src/main/java/org/apache/camel/component/pqc/PQCProducer.java:
##########
@@ -1250,6 +1279,65 @@ private void statefulDeleteKeyState(Exchange exchange)
throws Exception {
*
* @throws IllegalStateException if the key has zero remaining signatures
*/
+ /**
+ * Claims a stateful signature key for this process before it is used.
+ * <p/>
+ * XMSS, XMSSMT, LMS and HSS are one-time-signature schemes, and a store
several processes read from - a shared
+ * directory, a vault - hands each of them the same key at the same index.
The in-process ordering of
+ * {@link #recordKeyUsageAfterSign(Exchange)} does nothing about that, so
the key is claimed from the
+ * {@link KeyLifecycleManager} the first time it is signed with, and held
until this producer stops.
+ */
+ private void claimStatefulKeyBeforeSign(Exchange exchange) {
Review Comment:
💡 **`keyPair` is read before the claim; stale reference possible after a
rolling key rotation**
`claimStatefulKeyBeforeSign` reads `keyPair` at entry (to guard on `keyPair
== null` and `getStatefulKeyRemaining()`), then calls
`klm.claimStatefulKey(keyId)`. The claim may block briefly on a contested lock
file. If the key was rotated (re-loaded via `loadStatefulKey`) by another
thread between the `keyPair` guard and the actual signing call in
`signature()`, the producer will sign with the stale `keyPair` reference it
cached.
In the current single-producer model this is unlikely, but with shared KLM
and multiple producers it becomes possible. Consider re-reading `keyPair` after
a successful claim, or documenting that the method assumes `keyPair` is stable
during the claim window.
##########
components/camel-pqc/src/main/java/org/apache/camel/component/pqc/lifecycle/FileBasedKeyLifecycleManager.java:
##########
@@ -485,6 +491,70 @@ private Path getLegacyKeyFile(String keyId) {
return resolveKeyFile(keyId, ".key");
}
+ /**
+ * Takes an exclusive {@link FileLock} on a lock file beside the key, so
that a second process reading the same key
+ * directory - two containers on one volume, say - cannot sign from the
same one-time index.
+ * <p/>
+ * The lock lives as long as the channel, which this manager keeps open,
so the operating system releases it if the
+ * process dies without releasing the claim.
+ */
+ @Override
+ public boolean claimStatefulKey(String keyId) throws IOException {
+ if (statefulClaims.containsKey(keyId)) {
+ return true;
+ }
+
+ Path lockFile = resolveKeyFile(keyId, LOCK_SUFFIX);
+ FileChannel channel = FileChannel.open(lockFile,
StandardOpenOption.CREATE, StandardOpenOption.WRITE);
+ restrictToOwner(lockFile);
+
+ FileLock lock;
+ try {
+ lock = channel.tryLock();
+ } catch (OverlappingFileLockException e) {
+ closeQuietly(channel);
Review Comment:
⚠️ **POSIX lock-release risk: `channel.close()` in the
`OverlappingFileLockException` handler may release the existing lock**
On macOS/BSD and older Linux JDKs using traditional `fcntl(F_SETLK)`
advisory locks, closing **any** file descriptor pointing to the same inode from
the same process releases **all** advisory locks held by that process on the
file. Here that means: thread A holds the lock via `channel1`; thread B opens
`channel2` for the same `.lock` file, gets `OverlappingFileLockException`,
calls `closeQuietly(channel2)` — which on those platforms silently drops
`channel1`'s lock and leaves the key unclaimed.
Modern Linux JDKs (≥ 11 on kernels ≥ 3.15) use `F_OFD_SETLK`
(open-file-description locks), which are per-description rather than
per-process, so they are safe here. But since the code targets shared
directories (`"two containers on one volume"`) and is likely also run in
development on macOS, the risk is real.
Consider adding a comment documenting the OFD-lock assumption, or switching
the error log path to avoid closing the channel if no lock was acquired (i.e.
only call `closeQuietly` when `lock == null`, not when OFE is thrown).
--
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]
