gnodet-bot commented on code in PR #29098:
URL: https://github.com/apache/camel/pull/29098#discussion_r4456024141
##########
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);
+ throw new IllegalStateException(
+ "The stateful key '" + keyId + "' is already claimed in
this JVM through " + lockFile
+ + ". A stateful signature key must
have a single owner: share the"
+ + " KeyLifecycleManager instance
rather than the key directory.",
+ e);
Review Comment:
⚠️ **Concurrency edge case:** `OverlappingFileLockException` from
`tryLock()` means another channel in this JVM already holds the lock — but it
could be a concurrent call on *this same manager instance* that won the race
(both threads saw `containsKey()==false` and both opened a channel). Re-check
`statefulClaims.containsKey(keyId)` here: if found, close the new channel and
return `true`. Without this, two concurrent first-time signers on the same
producer (normal for high-throughput routes) can both pass the outer
`claimedStatefulKeys.contains()` check, then the second one hits this path and
fails with a misleading 'share the KeyLifecycleManager instance' error — even
though they *do* share the same instance.
```suggestion
} catch (OverlappingFileLockException e) {
closeQuietly(channel);
// A concurrent call on this same manager may have just acquired
the lock.
// Re-check before throwing to avoid failing a valid concurrent
signer.
if (statefulClaims.containsKey(keyId)) {
return true;
}
throw new IllegalStateException(
"The stateful key '" + keyId + "' is already claimed in
this JVM through " + lockFile
+ ". A stateful signature key
must have a single owner: share the"
+ " KeyLifecycleManager instance
rather than the key directory.",
e);
```
##########
components/camel-pqc/src/main/java/org/apache/camel/component/pqc/PQCConfiguration.java:
##########
@@ -125,6 +125,15 @@ public class PQCConfiguration implements Cloneable {
@Metadata(label = "advanced")
private double statefulKeyWarningThreshold = 0.1;
+ @UriParam(defaultValue = "false", label = "security,advanced", security =
"insecure:dev",
+ description = "Whether to sign with a stateful key (XMSS,
XMSSMT, LMS/HSS) that the configured "
+ + "KeyLifecycleManager cannot claim for this
process alone. These are one-time-signature "
+ + "schemes: when two processes load the same key
from a shared store, both sign from the "
+ + "same index and the key is broken. Only enable
it when something outside Camel "
+ + "guarantees a single signer.")
Review Comment:
🔧 **Nit — redundant `label` attribute:** The existing convention in this
class puts `label` only in `@Metadata`, not in `@UriParam`. Remove it from
`@UriParam` for consistency.
```suggestion
@UriParam(defaultValue = "false", security = "insecure:dev",
description = "Whether to sign with a stateful key (XMSS,
XMSSMT, LMS/HSS) that the configured "
+ "KeyLifecycleManager cannot claim for this
process alone. These are one-time-signature "
+ "schemes: when two processes load the same key
from a shared store, both sign from the "
+ "same index and the key is broken. Only enable
it when something outside Camel "
+ "guarantees a single signer.")
```
##########
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);
+ throw new IllegalStateException(
+ "The stateful key '" + keyId + "' is already claimed in
this JVM through " + lockFile
+ + ". A stateful signature key must
have a single owner: share the"
+ + " KeyLifecycleManager instance
rather than the key directory.",
+ e);
+ } catch (IOException e) {
+ closeQuietly(channel);
+ throw e;
+ }
+
+ if (lock == null) {
+ closeQuietly(channel);
+ throw new IllegalStateException(
+ "The stateful key '" + keyId + "' is held by another
process through " + lockFile
+ + ". Signing from it here would
hand out a one-time index twice, which"
+ + " breaks the key. Give each
process its own stateful key, or stop the"
+ + " other one.");
+ }
+
+ statefulClaims.put(keyId, channel);
+ LOG.info("Claimed the stateful key '{}' through {}", keyId, lockFile);
+ return true;
+ }
+
+ @Override
+ public void releaseStatefulKey(String keyId) {
+ FileChannel channel = statefulClaims.remove(keyId);
Review Comment:
💡 **Nit — `.lock` files accumulate:** `releaseStatefulKey` closes the
channel (releasing the OS lock) but never deletes the `.lock` file. The next
`claimStatefulKey` recreates it via `StandardOpenOption.CREATE` if absent, so
this is not a correctness bug. Worth a comment explaining the intentional
design, so future maintainers don't wonder why `.lock` files pile up beside the
key files.
--
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]
