oscerd commented on code in PR #29098:
URL: https://github.com/apache/camel/pull/29098#discussion_r4466912415
##########
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:
Fixed in 963ccbd, taking the `computeIfAbsent` option rather than the
re-check.
Both levels are now atomic: `PQCProducer` wraps claim-and-record in
`statefulKeyClaims.computeIfAbsent`, and `FileBasedKeyLifecycleManager` does
open-lock-store in `statefulClaims.computeIfAbsent`. That removes the
false-positive path for callers of one manager rather than catching it after
the fact, which also means the OFE handler now only ever runs for a genuinely
different manager instance — the case its message describes.
New `concurrentFirstClaimsOnOneManagerAllSucceed` covers it: eight threads
racing the first claim. Against the previous commit it fails with exactly the
misleading error you predicted.
_Claude Code on behalf of @oscerd_
##########
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:
Confirmed and fixed in 963ccbd. I reproduced it on Linux before changing
anything — one JVM holding the lock, opening a second channel on the same file,
catching the OFE and closing that channel:
```
holder: first lock = true
holder: got OverlappingFileLockException as expected
holder: closed second channel; first lock isValid=true <- still reports
valid
--- separate process ---
prober: acquired lock = true <- the lock was gone
```
So on this JDK it is not using OFD locks, and `isValid()` keeps answering
`true` while the lock is gone — the code cannot even detect the loss. The
channel is no longer closed on the OFE path. It is still closed where no lock
was taken on it (`tryLock` returning null, or an `IOException`), which is safe,
and `openAndLock` records why in its javadoc.
_Claude Code on behalf of @oscerd_
##########
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:
Fixed in 963ccbd — `label` removed from `@UriParam`, left on `@Metadata` as
the surrounding options do. Confirmed no generated metadata changed, so it was
purely redundant.
_Claude Code on behalf of @oscerd_
##########
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:
Kept, with the reasoning written down in `releaseStatefulKey` rather than
deleting the file.
Deleting it is not safe: another process can already hold the same file open
and locked, and unlinking it lets a third process create a fresh file at that
path and lock *that* — two owners, each holding a valid lock on a different
inode. An empty `.lock` beside each stateful key is the price of avoiding that.
The javadoc now says so, so the next reader does not have to rediscover it.
_Claude Code on behalf of @oscerd_
--
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]
