gnodet commented on PR #22264:
URL: https://github.com/apache/camel/pull/22264#issuecomment-4127769708
@oscerd Thanks for this contribution! Since it's already merged, here are
some findings that may warrant a follow-up:
### 1. Redundant condition in `checkStatefulKeyBeforeSign()`
In `PQCProducer.java`, after `if (remaining < 0) { return; }`, the next
check `if (remaining <= 0)` is equivalent to `remaining == 0`. The `<=` is
misleading since negative values were already handled.
### 2. Health check reads key from configuration, not from the producer
`PQCStatefulKeyHealthCheck.doCall()` reads `configuration.getKeyPair()`, but
the producer stores its own `keyPair` field (set during `doStart()`). If these
diverge, the health check would report stale/incorrect state.
### 3. Health check never reports degraded state
When the fraction remaining is below the warning threshold, the health check
still reports `UP`. It computes `fraction_remaining` and `warning_threshold` as
details but never calls `builder.degraded()`. Monitoring systems won't see any
state change until the key is fully exhausted (DOWN).
### 4. No input validation on `statefulKeyWarningThreshold`
Values > 1.0 or < 0.0 are silently accepted. A value > 1.0 would trigger
warnings on every signing operation; negative values silently disable warnings.
### 5. `persistStatefulKeyStateAfterSign` may call `storeKey` with null
metadata
```java
KeyMetadata metadata = klm.getKeyMetadata(keyId);
if (metadata != null) {
metadata.updateLastUsed();
klm.updateKeyMetadata(keyId, metadata);
}
klm.storeKey(keyId, keyPair, metadata); // metadata could be null here
```
### 6. `testKeyExhaustion` doesn't test the Camel component
The test exercises BouncyCastle's XMSS directly, not the PQC producer. It
doesn't verify that the producer's `checkStatefulKeyBeforeSign()` actually
throws `IllegalStateException` when the key is exhausted.
### 7. Duplicated `instanceof` dispatch pattern
The `instanceof XMSSPrivateKey / XMSSMTPrivateKey / LMSPrivateKey` dispatch
is repeated in 4 places (`getStatefulKeyRemaining`, `getStatefulKeyIndex`,
`PQCStatefulKeyHealthCheck.doCall`, `statefulGetKeyState`). The health check
could reuse the producer's helpers or the `StatefulKeyState` model.
### 8. Hybrid signing not covered
`hybridSign` doesn't call `checkStatefulKeyBeforeSign()`, so exhaustion
protection doesn't cover hybrid signatures with stateful keys.
None of these are critical, but items 2, 3, 5, and 6 would improve
reliability if addressed in a follow-up.
_Claude Code on behalf of Guillaume Nodet_
--
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]
