gnodet commented on code in PR #22282:
URL: https://github.com/apache/camel/pull/22282#discussion_r2995074652
##########
components/camel-pqc/src/main/java/org/apache/camel/component/pqc/PQCStatefulKeyHealthCheck.java:
##########
@@ -78,24 +65,35 @@ protected void doCall(HealthCheckResultBuilder builder,
Map<String, Object> opti
return;
}
+ long totalCapacity = index + remaining;
+
builder.detail("stateful_key", true);
builder.detail("algorithm", algorithm);
builder.detail("remaining_signatures", remaining);
builder.detail("signatures_used", index);
- builder.detail("total_capacity", index + remaining);
+ builder.detail("total_capacity", totalCapacity);
- if (remaining <= 0) {
+ if (remaining == 0) {
builder.message("Stateful key (" + algorithm + ") is exhausted
with 0 remaining signatures");
builder.down();
return;
}
- double threshold = configuration.getStatefulKeyWarningThreshold();
- long totalCapacity = index + remaining;
+ double threshold =
endpoint.getConfiguration().getStatefulKeyWarningThreshold();
if (threshold > 0 && totalCapacity > 0) {
double fractionRemaining = (double) remaining / totalCapacity;
builder.detail("fraction_remaining", String.format("%.4f",
fractionRemaining));
builder.detail("warning_threshold", String.valueOf(threshold));
+
+ if (fractionRemaining <= threshold) {
+ builder.message(
+ "Stateful key (" + algorithm + ") is approaching
exhaustion: " + remaining
+ + " signatures remaining out of " +
totalCapacity + " total ("
+ + String.format("%.1f%%", fractionRemaining *
100) + " remaining)");
+ builder.detail("warning", true);
+ builder.down();
+ return;
Review Comment:
The Camel `HealthCheck.State` enum has `UP`, `DOWN`, and `UNKNOWN` — no
`DEGRADED` state. Using `DOWN` here means a key that still has remaining
signatures is treated the same as an exhausted key by monitoring/readiness
probes.
Since this is a *warning* (the key still works), it should report `UP` with
the warning details, so operators can alert on the detail fields without
tripping readiness checks:
```suggestion
if (fractionRemaining <= threshold) {
builder.message(
"Stateful key (" + algorithm + ") is approaching
exhaustion: " + remaining
+ " signatures remaining out of " +
totalCapacity + " total ("
+ String.format("%.1f%%", fractionRemaining
* 100) + " remaining)");
builder.detail("warning", true);
// Stay UP — the key still has capacity. Operators can alert
on the "warning" detail.
return;
}
```
##########
components/camel-pqc/src/main/java/org/apache/camel/component/pqc/PQCProducer.java:
##########
@@ -916,16 +916,16 @@ private void persistStatefulKeyStateAfterSign(Exchange
exchange) throws Exceptio
if (metadata != null) {
metadata.updateLastUsed();
klm.updateKeyMetadata(keyId, metadata);
- }
- // Persist the updated key (with new index) so state survives restarts
- klm.storeKey(keyId, keyPair, metadata);
+ // Persist the updated key (with new index) so state survives
restarts
+ klm.storeKey(keyId, keyPair, metadata);
+ }
Review Comment:
When `metadata` is null, the key state mutation (index advance from signing)
won't be persisted. On restart, the old index would be restored, risking key
reuse — a serious concern for stateful signature schemes where signing with the
same index breaks security guarantees.
Consider always persisting the key, and logging a warning when metadata is
missing:
```suggestion
if (metadata != null) {
metadata.updateLastUsed();
klm.updateKeyMetadata(keyId, metadata);
} else {
LOG.warn("No metadata found for stateful key '{}' — persisting
key state without metadata update", keyId);
}
// Always persist the updated key (with new index) so state survives
restarts,
// even if metadata is unavailable
klm.storeKey(keyId, keyPair, metadata);
```
##########
components/camel-pqc/src/main/java/org/apache/camel/component/pqc/PQCProducer.java:
##########
@@ -500,7 +508,9 @@ private void extractSecretKeyFromEncapsulation(Exchange
exchange)
// ========== Hybrid Signature Operations ==========
private void hybridSignature(Exchange exchange)
- throws InvalidPayloadException, InvalidKeyException,
SignatureException {
+ throws Exception {
Review Comment:
The `throws` clause was widened from specific exceptions to `throws
Exception`. Since `checkStatefulKeyBeforeSign()` only throws
`IllegalStateException` (unchecked) and `persistStatefulKeyStateAfterSign` is
the only one adding checked exceptions, consider keeping the specific types:
```suggestion
private void hybridSignature(Exchange exchange)
throws InvalidPayloadException, InvalidKeyException,
SignatureException {
```
This works because `persistStatefulKeyStateAfterSign` can be wrapped or its
checked exceptions added individually. Alternatively, if keeping `throws
Exception`, that's acceptable since the caller (`process`) already declares
`throws Exception` — but the specificity is nice documentation of what actually
goes wrong.
##########
components/camel-pqc/src/main/java/org/apache/camel/component/pqc/PQCStatefulKeyHealthCheck.java:
##########
@@ -22,28 +22,29 @@
import org.apache.camel.health.HealthCheckResultBuilder;
import org.apache.camel.impl.health.AbstractHealthCheck;
-import org.bouncycastle.pqc.jcajce.interfaces.LMSPrivateKey;
-import org.bouncycastle.pqc.jcajce.interfaces.XMSSMTPrivateKey;
-import org.bouncycastle.pqc.jcajce.interfaces.XMSSPrivateKey;
/**
* Health check that reports the state of stateful PQC signature keys (XMSS,
XMSSMT, LMS/HSS). These hash-based
- * signature schemes have a finite number of signatures. This health check
reports DOWN when a key is exhausted and
- * includes remaining signature capacity as a detail.
+ * signature schemes have a finite number of signatures. This health check
reports DOWN when a key is exhausted,
+ * DEGRADED when remaining signatures fall below the warning threshold, and
includes remaining signature capacity as a
+ * detail.
Review Comment:
The Javadoc mentions "DEGRADED" but Camel's `HealthCheck.State` has no
DEGRADED value, and the implementation uses DOWN. If we change the threshold
case to UP (see other comment), update the Javadoc accordingly:
```suggestion
* signature schemes have a finite number of signatures. This health check
reports DOWN when a key is exhausted
* and includes remaining signature capacity as a detail. When remaining
signatures fall below the warning threshold,
* the check remains UP but includes a warning detail for operators to alert
on.
```
--
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]
