oscerd commented on code in PR #29098:
URL: https://github.com/apache/camel/pull/29098#discussion_r4478560946
##########
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:
Folded in after all, in 1f647786205a, since both reviews asked for it here:
the claims of one `FileBasedKeyLifecycleManager` are now counted per key (a
`holders` count on the claim), so producers that share the manager do not
release each other's claim and the key is given back by the last holder. The
interface Javadoc of `claimStatefulKey`/`releaseStatefulKey` says so, and the
key-lifecycle page tells a custom manager to count its claims too.
`aClaimIsGrantedAndTakesALockFile` now checks that a key claimed twice is still
held after one release and free after the second. CAMEL-26588 is covered by
this.
_Claude Code on behalf of oscerd_
##########
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:
Applied in 1f647786205a: a JVM-wide `ConcurrentHashMap<Path,
FileBasedKeyLifecycleManager>` keyed by the real path of the lock file,
consulted inside `compute` before `FileChannel.open`. A second manager of the
same directory, also through a symbolic link, is refused there and no channel
is opened, so there is nothing for the `Cleaner` to close and no descriptor
leaks per refused exchange. `OverlappingFileLockException` can now only mean a
lock taken through a channel this class did not open; such a channel is kept
referenced for the life of the JVM rather than closed or left to the collector.
Tests: `aSecondHolderOfTheSameKeyDirectoryIsRefused` (comment updated) and the
new `aSecondHolderThroughAnotherPathToTheSameDirectoryIsRefused`.
_Claude Code on behalf of oscerd_
##########
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:
Applied in 1f647786205a: `openAndLock` evicts `keyCache` and `metadataCache`
for the key once the lock is taken, so the re-read in
`adoptStoredStatefulKeyIfAhead` comes from the store. New test
`aNewClaimReadsTheKeyAndItsMetadataFromTheStore` with two
`FileBasedKeyLifecycleManager` instances on one directory: the arriving one
serves its cached key and metadata until it claims, and the stored key and
usage count after.
_Claude Code on behalf of oscerd_
##########
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:
Applied in 1f647786205a: `adoptStoredStatefulKeyIfAhead` keeps the key
loaded at startup, with a warning, when the stored public key encodes
differently (`MessageDigest.isEqual` on the encodings; keys that give no
encoding cannot be told apart and are not treated as different). New test
`aStoredKeyThatIsAnotherKeyIsNotAdopted`: another LMS key stored under the same
id, further ahead, is not adopted, and the producer signs once from the index
it loaded.
_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]
