gnodet-bot commented on code in PR #29098:
URL: https://github.com/apache/camel/pull/29098#discussion_r4456227760
##########
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) {
+ if (keyPair == null || keyPair.getPrivate() == null
+ || getStatefulKeyRemaining(keyPair.getPrivate()) < 0) {
+ return;
+ }
+
+ KeyLifecycleManager klm = getConfiguration().getKeyLifecycleManager();
+ String keyId = exchange.getMessage().getHeader(PQCConstants.KEY_ID,
String.class);
+ if (klm == null || ObjectHelper.isEmpty(keyId)) {
+ // There is no store to claim from, and no key to name.
recordKeyUsageAfterSign already warns that the
+ // index of this signature is not being persisted at all, which is
the larger problem of the two.
+ return;
+ }
+
+ if (claimedStatefulKeys.contains(keyId)) {
+ return;
Review Comment:
⚠️ **Concurrency race: `contains()` and `add()` are not atomic**
`claimedStatefulKeys` is a `ConcurrentHashMap`-backed set, but the
check-then-act sequence across lines 377–411 is not atomic:
```
if (claimedStatefulKeys.contains(keyId)) { return; } // ← thread A and B
both pass here
// ... calls klm.claimStatefulKey(keyId) ...
claimedStatefulKeys.add(keyId); // ← only thread A
gets here first
```
Two concurrent threads handling different exchanges for the same `keyId` can
both pass the `contains()` guard before either adds to the set. Thread A
acquires the `FileLock`; thread B then calls `claimStatefulKey()` on the same
`FileChannel` and hits `OverlappingFileLockException`, which
`FileBasedKeyLifecycleManager` re-throws as `IllegalStateException("already
claimed in this JVM ... share the KeyLifecycleManager instance")` — a
misleading error, because the managers *are* the same instance.
**Fix options:**
- Use `computeIfAbsent` in `claimedStatefulKeys` to make the check-then-add
atomic, and call `klm.claimStatefulKey()` inside the compute lambda only when
the entry is new.
- Or synchronize the entire `contains → klm.claim → add` block on
`keyId.intern()` (or a `ConcurrentHashMap<String, Object>` of per-key locks).
##########
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.")
+ @Metadata(label = "security,advanced")
+ private boolean allowSharedStatefulKey;
Review Comment:
🔧 **Nit — redundant `label` in `@UriParam`**
The `label` attribute is set in both `@UriParam` and `@Metadata`. The
existing convention in this class (e.g. `statefulKeyWarningThreshold` just
above) 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 after release**
`releaseStatefulKey` closes the `FileChannel` (which releases the OS lock)
but never deletes the `.lock` file. The next `claimStatefulKey` call for the
same `keyId` recreates it via `StandardOpenOption.CREATE`, so it is idempotent
— but the files accumulate in the key directory across restarts and repeated
claim/release cycles, which is surprising to users inspecting the directory.
Consider adding `Files.deleteIfExists(lockFile)` after
`closeQuietly(channel)`:
```java
closeQuietly(channel);
try {
Files.deleteIfExists(resolveKeyFile(keyId, LOCK_SUFFIX));
} catch (IOException e) {
LOG.debug("Could not delete lock file for key '{}'", keyId, e);
}
```
--
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]
