gnodet-bot commented on code in PR #28099:
URL: https://github.com/apache/camel/pull/28099#discussion_r4322005899
##########
components/camel-pqc/src/main/java/org/apache/camel/component/pqc/PQCProducer.java:
##########
@@ -1146,47 +1156,102 @@ private void checkStatefulKeyBeforeSign() {
}
/**
- * Persists stateful key state after a signing operation through the
KeyLifecycleManager, if configured. This
- * ensures the key index is tracked across restarts.
+ * Records the use of the signing key through the KeyLifecycleManager, if
one is configured and the exchange names a
+ * key.
+ * <p/>
+ * This advances {@code usageCount}, which is what the {@code maxKeyUsage}
rotation signal of
+ * {@link org.apache.camel.component.pqc.lifecycle.KeyRotationScheduler}
reads. It therefore runs for every signing
+ * key, not only the stateful ones: ML-DSA, SLH-DSA and Falcon keys would
otherwise sit at a usage count of zero
+ * forever and never be rotated.
+ * <p/>
+ * The key encapsulation operations do not call this method, so a KEM key
still does not advance its usage count and
+ * {@code maxKeyUsage} does not apply to it.
+ * <p/>
+ * For a stateful signature key the advanced private key is stored as
well, so the one-time index survives a
+ * restart.
*/
- private void persistStatefulKeyStateAfterSign(Exchange exchange) throws
Exception {
+ private void recordKeyUsageAfterSign(Exchange exchange) throws Exception {
if (keyPair == null || keyPair.getPrivate() == null) {
return;
}
PrivateKey privateKey = keyPair.getPrivate();
- long remaining = getStatefulKeyRemaining(privateKey);
- if (remaining < 0) {
- // Not a stateful key
- return;
- }
+ boolean stateful = getStatefulKeyRemaining(privateKey) >= 0;
KeyLifecycleManager klm = getConfiguration().getKeyLifecycleManager();
if (klm == null) {
+ // Only the stateful risk is worth reporting here. A plain signing
endpoint with no lifecycle management
+ // is an ordinary setup, and with no manager there is no
maxKeyUsage threshold to miss.
+ if (stateful) {
+ warnKeyUsageNotRecorded(true, "no keyLifecycleManager is
configured on the endpoint");
+ }
return;
}
String keyId = exchange.getMessage().getHeader(PQCConstants.KEY_ID,
String.class);
if (ObjectHelper.isEmpty(keyId)) {
+ warnKeyUsageNotRecorded(stateful,
+ "the exchange carries no " + PQCConstants.KEY_ID + "
header identifying the key");
return;
}
- // Update metadata with current usage
KeyMetadata metadata = klm.getKeyMetadata(keyId);
if (metadata == null) {
Review Comment:
⚠️ **Bug: unwrapped `getKeyMetadata` can fail the exchange for a
non-stateful key**
This call declares `throws Exception`, and remote vault implementations
(Hashicorp, Azure) perform a network read here. If it throws, the exception
propagates through `signature()` / `hybridSignature()` and **fails the exchange
after the signature was already computed but before the signature header is
set** — exactly what the try-catch around `updateKeyMetadata` below (line 1215)
was designed to prevent.
The author's comment at that try-catch is explicit: _"Failing the exchange
over it would throw away a perfectly good signature."_ The same reasoning
applies to this read.
Fix: restructure `recordKeyUsageAfterSign` so that for the non-stateful
path, the entire `getKeyMetadata` + null-check + `updateLastUsed` +
`updateKeyMetadata` sequence is inside a single try-catch. The stateful path
(`storeKey`) does not need this protection because losing a stateful index on a
vault write failure is already fatal.
--
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]
