gnodet-bot commented on code in PR #29098:
URL: https://github.com/apache/camel/pull/29098#discussion_r4478204146
##########
components/camel-pqc/src/main/java/org/apache/camel/component/pqc/lifecycle/FileBasedKeyLifecycleManager.java:
##########
@@ -485,6 +492,101 @@ 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 {
+ // computeIfAbsent so the whole open-lock-store sequence happens once
per key: a plain containsKey check
+ // followed by a separate put lets two concurrent first-time signers
both open a channel, and the loser then
+ // takes the OverlappingFileLockException path below for no reason.
+ try {
+ statefulClaims.computeIfAbsent(keyId, this::openAndLock);
+ } catch (UncheckedIOException e) {
+ throw e.getCause();
+ }
+ return true;
+ }
+
+ /**
+ * Opens the lock file of a key and takes an exclusive lock on it,
returning the channel that holds it.
+ * <p/>
+ * A channel is only ever closed here when this process holds no lock on
the file. Closing one that may hold the
+ * lock is not safe: on this platform closing any descriptor for the file
releases the lock this process holds
+ * through every other descriptor, while {@code FileLock.isValid()} keeps
reporting {@code true}, so the claim would
+ * be silently gone with nothing able to detect it.
+ */
+ private FileChannel openAndLock(String keyId) {
+ Path lockFile = resolveKeyFile(keyId, LOCK_SUFFIX);
+ FileChannel channel;
+ try {
+ channel = FileChannel.open(lockFile, StandardOpenOption.CREATE,
StandardOpenOption.WRITE);
+ restrictToOwner(lockFile);
+ } catch (IOException e) {
+ throw new UncheckedIOException(e);
+ }
+
+ FileLock lock;
+ try {
+ lock = channel.tryLock();
+ } catch (OverlappingFileLockException e) {
+ // Another KeyLifecycleManager instance in this JVM holds the
lock. The channel is deliberately left
+ // open: closing it would drop that instance's lock, see
openAndLock's javadoc.
+ throw new IllegalStateException(
Review Comment:
⚠️ **GC can silently drop the POSIX lock via Cleaner:**
The comment is right that closing the channel here would release the first
instance's lock. But this channel is **not stored anywhere** —
`computeIfAbsent` throws before the mapping function returns, so
`statefulClaims` never holds a reference to it. Once this method returns and
`e` goes out of scope, the channel is GC-eligible. When the JVM collects it,
`FileChannelImpl.Closer` closes the fd, which on POSIX silently releases every
lock this process holds on the file (while `FileLock.isValid()` stays `true`).
Each refused exchange also leaks one file descriptor until GC runs.
Fix: check a JVM-wide `ConcurrentHashMap<Path, FileChannel>` before
`FileChannel.open`. If the canonical path is already registered, return early
(OFE behaviour, without opening). If not, open, lock, and register. This avoids
opening a second channel at all — no leak, no Cleaner risk.
##########
components/camel-pqc/src/main/java/org/apache/camel/component/pqc/PQCProducer.java:
##########
@@ -1244,6 +1279,116 @@ private void statefulDeleteKeyState(Exchange exchange)
throws Exception {
getRequiredKeyLifecycleManager().deleteKey(keyId);
}
+ /**
+ * 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;
+ }
+
+ // The claim is taken once per key. computeIfAbsent rather than a
contains/claim/add sequence, so two
+ // concurrent first-time signers cannot both reach claimStatefulKey
for the same key.
+ statefulKeyClaims.computeIfAbsent(keyId, id -> {
+ doClaimStatefulKey(klm, id);
+ adoptStoredStatefulKeyIfAhead(klm, id);
+ claimedStatefulKeys.add(id);
+ return Boolean.TRUE;
+ });
+ }
+
+ private void doClaimStatefulKey(KeyLifecycleManager klm, String keyId) {
+ boolean claimed;
+ try {
+ claimed = klm.claimStatefulKey(keyId);
+ } catch (Exception e) {
+ throw new IllegalStateException(
+ "Cannot claim the stateful key '" + keyId + "': " +
e.getMessage(), e);
+ }
+
+ if (!claimed) {
+ if (getConfiguration().isAllowSharedStatefulKey()) {
+ if (sharedStatefulKeyWarned.add(keyId)) {
+ LOG.warn("Signing with the stateful key '{}', which {}
cannot claim for this process alone,"
+ + " because allowSharedStatefulKey is enabled. If
a second process signs with the same"
+ + " key, both hand out the same one-time index
and the key is broken.",
+ keyId, klm.getClass().getSimpleName());
+ }
+ } else {
+ throw new IllegalStateException(
+ "The stateful key '" + keyId + "' cannot be claimed
for this process: "
+ +
klm.getClass().getSimpleName()
+ + " cannot guarantee that no
other process holds it. "
+ +
keyPair.getPrivate().getAlgorithm()
+ + " is a one-time-signature
scheme, and two processes signing from the"
+ + " same index break the key.
Use a KeyLifecycleManager that can claim"
+ + " a key, such as
FileBasedKeyLifecycleManager, or set"
+ + "
allowSharedStatefulKey=true if a single signer is guaranteed"
+ + " elsewhere.");
+ }
+ }
+ }
+
+ /**
+ * Re-reads the stored key once the claim is granted, and adopts it when
its index has moved past the one this
+ * producer loaded.
+ * <p/>
+ * Without this the claim does not stop what it is meant to stop. {@code
keyPair} is read once in
+ * {@link #doStart()}, so in a rolling update the arriving instance loads
the key at some index, waits for the claim
+ * while the leaving instance keeps signing, and then starts from its own
stale index - handing out every index the
+ * other instance already used. The claim only serialises the signers; it
is this read that makes the second one
+ * resume where the first stopped.
+ */
+ private void adoptStoredStatefulKeyIfAhead(KeyLifecycleManager klm, String
keyId) {
+ KeyPair stored;
+ try {
+ stored = klm.getKey(keyId);
Review Comment:
⚠️ **Re-read hits the cache — adoption is a no-op for
`FileBasedKeyLifecycleManager`:**
`klm.getKey(keyId)` is meant to pick up the most-advanced index from the
store. For `FileBasedKeyLifecycleManager` it calls into `keyCache.get(keyId)`,
which returns the same `KeyPair` object this producer already holds —
`openAndLock` does not evict `keyCache` or `metadataCache` when a new lock is
acquired. The rolling-update scenario the Javadoc describes is therefore not
solved: the arriving instance still starts from its own startup index.
Fix: evict `keyCache.remove(keyId)` and `metadataCache.remove(keyId)` inside
`openAndLock` after acquiring the lock, so the next `getKey` call reads the key
fresh from disk. A test with two `FileBasedKeyLifecycleManager` instances on
the same directory (not in-memory as `PQCStatefulKeyAdoptionTest` uses) would
reproduce this.
##########
components/camel-pqc/src/main/java/org/apache/camel/component/pqc/PQCProducer.java:
##########
@@ -1244,6 +1279,116 @@ private void statefulDeleteKeyState(Exchange exchange)
throws Exception {
getRequiredKeyLifecycleManager().deleteKey(keyId);
}
+ /**
+ * 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;
+ }
+
+ // The claim is taken once per key. computeIfAbsent rather than a
contains/claim/add sequence, so two
+ // concurrent first-time signers cannot both reach claimStatefulKey
for the same key.
+ statefulKeyClaims.computeIfAbsent(keyId, id -> {
+ doClaimStatefulKey(klm, id);
+ adoptStoredStatefulKeyIfAhead(klm, id);
+ claimedStatefulKeys.add(id);
+ return Boolean.TRUE;
+ });
+ }
+
+ private void doClaimStatefulKey(KeyLifecycleManager klm, String keyId) {
+ boolean claimed;
+ try {
+ claimed = klm.claimStatefulKey(keyId);
+ } catch (Exception e) {
+ throw new IllegalStateException(
+ "Cannot claim the stateful key '" + keyId + "': " +
e.getMessage(), e);
+ }
+
+ if (!claimed) {
+ if (getConfiguration().isAllowSharedStatefulKey()) {
+ if (sharedStatefulKeyWarned.add(keyId)) {
+ LOG.warn("Signing with the stateful key '{}', which {}
cannot claim for this process alone,"
+ + " because allowSharedStatefulKey is enabled. If
a second process signs with the same"
+ + " key, both hand out the same one-time index
and the key is broken.",
+ keyId, klm.getClass().getSimpleName());
+ }
+ } else {
+ throw new IllegalStateException(
+ "The stateful key '" + keyId + "' cannot be claimed
for this process: "
+ +
klm.getClass().getSimpleName()
+ + " cannot guarantee that no
other process holds it. "
+ +
keyPair.getPrivate().getAlgorithm()
+ + " is a one-time-signature
scheme, and two processes signing from the"
+ + " same index break the key.
Use a KeyLifecycleManager that can claim"
+ + " a key, such as
FileBasedKeyLifecycleManager, or set"
+ + "
allowSharedStatefulKey=true if a single signer is guaranteed"
+ + " elsewhere.");
+ }
+ }
+ }
+
+ /**
+ * Re-reads the stored key once the claim is granted, and adopts it when
its index has moved past the one this
+ * producer loaded.
+ * <p/>
+ * Without this the claim does not stop what it is meant to stop. {@code
keyPair} is read once in
+ * {@link #doStart()}, so in a rolling update the arriving instance loads
the key at some index, waits for the claim
+ * while the leaving instance keeps signing, and then starts from its own
stale index - handing out every index the
+ * other instance already used. The claim only serialises the signers; it
is this read that makes the second one
+ * resume where the first stopped.
+ */
+ private void adoptStoredStatefulKeyIfAhead(KeyLifecycleManager klm, String
keyId) {
+ KeyPair stored;
+ try {
+ stored = klm.getKey(keyId);
+ } catch (Exception e) {
+ LOG.warn("Claimed the stateful key '{}' but could not re-read it
from the store, so this producer keeps"
+ + " the key it loaded at startup. If another process
advanced that key meanwhile, signing now"
+ + " reuses a one-time index.",
+ keyId, e);
+ return;
+ }
+
+ if (stored == null || stored.getPrivate() == null) {
+ return;
+ }
+
+ long storedRemaining = getStatefulKeyRemaining(stored.getPrivate());
+ long loadedRemaining = getStatefulKeyRemaining(keyPair.getPrivate());
+ if (storedRemaining < 0) {
+ return;
+ }
+
+ // Fewer remaining signatures means the stored key has been used more
than the one held here
+ if (storedRemaining < loadedRemaining) {
+ LOG.warn("The stored stateful key '{}' has {} signatures left
while the one loaded at startup has {}."
+ + " Another process advanced it. Adopting the stored key
so this producer resumes from its"
+ + " index instead of reusing indices that have already
been handed out.",
+ keyId, storedRemaining, loadedRemaining);
+ keyPair = stored;
Review Comment:
💡 **No public-key check before adoption:**
The guard only checks `storedRemaining < loadedRemaining`. A key stored
under the same `keyId` but with a different algorithm or key material (key
rotation, misconfigured directory) would be silently adopted as the signing key.
Suggestion:
```suggestion
if (!stored.getPublic().equals(keyPair.getPublic())) {
LOG.warn("Claimed the stateful key '{}' but the stored public
key does not match the one loaded at "
+ "startup; not adopting the stored key to avoid
signing with the wrong key.", keyId);
return;
}
if (storedRemaining < loadedRemaining) {
```
--
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]
