Repository navigation
CAMEL-25797: camel-pqc - the usage-count and PENDING_ROTATION key rotation signals now fire - #28094
Conversation
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Two issues, both in PQCProducer.java.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks @oscerd, both fixes are right: usageCount now moves for every signing key, and a PENDING_ROTATION key reaches needsRotation(). All five managers mark the old key DEPRECATED on rotation, so the wider default filter cannot rotate the same key twice. Both gnodet-bot threads are fixed, and I resolved them.
Two things to change:
-
A vault write per signature for the stateless keys. The description says this reduces traffic, but for ML-DSA, SLH-DSA and Falcon it adds writes where there were none. In the AWS Secrets Manager, HashiCorp Vault and Azure Key Vault managers,
updateKeyMetadataisgetKey+storeKey, which rewrites the private key, public key and metadata secrets: three new secret versions per signature. A route that signs at volume with akeyLifecycleManagerandCamelPQCKeyIdnow writes to the vault three times per message, and a vault error now fails an exchange that signed fine before.- Please say this in the upgrade-guide entry.
- For a stateless key, consider logging (once) instead of failing the exchange when recording the usage fails. A missed count only delays a rotation; a lost stateful index is a different matter.
- For the stateful keys,
storeKeyalready writes the metadata in every manager, so theupdateKeyMetadatabefore it is a redundant round (six secret writes per signature today). Calling onlystoreKeythere would halve that.
-
Docs. The "Automated Rotation Scheduling" section of
pqc-key-lifecycle.adocstill says the scheduler keeps "by default onlyACTIVEkeys". Please update it, and its copy incatalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/docs/. Please also say there thatmaxKeyUsagecounts signing operations only, and only when akeyLifecycleManageris configured and the exchange has theCamelPQCKeyIdheader.
Non-blocking:
- The usage is recorded on the key named by the
CamelPQCKeyIdheader, not on the key that signed. InKeyRotationSignalsTestthe producer signs with the default ML-DSA material, and the count goes tosigning-key, which never signed. The stateful path already worked this way. A test that signs with the generated key (set as the endpointkeyPair) would show the intended setup. updateLastUsed()is a plainusageCount++on the metadata object the managers cache, so concurrent exchanges can lose increments. That's fine for a threshold, but a comment would help.
CI is still running on 24365a4.
Claude Code on behalf of Claus Ibsen
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — both findings have been addressed.
✅ Inaccurate KEM Javadoc — fixed in 24365a4. The recordKeyUsageAfterSign Javadoc now explicitly states that KEM operations do not call this method and do not advance the usage count.
✅ Stale error message — fixed in 24365a4. The catch block now says "Failed to record key usage after hybrid signing".
All davsclaus blocking items are also addressed in af51b2f:
- Upgrade guide and the IMPORTANT adoc block both document that non-stateful signing now writes to the backing store on every signature, and advise using
maxKeyAge/nextRotationAton high-volume routes against remote vaults. - Stateful path now calls only
storeKey(eliminating the redundantupdateKeyMetadatacall that would have doubled remote writes). - Non-stateful
updateKeyMetadatafailures are swallowed with a once-per-producerLOG.warnviaAtomicBoolean— the exchange is not failed. - Both adoc files (source and generated) updated identically.
KeyMetadata.updateLastUsed()has a concurrency Javadoc explaining the non-atomicity is accepted.
The slf4j format string, PENDING_ROTATION filter widening, and test key alignment are all correct. No new issues introduced by the latest commits.
LGTM.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
Thanks @davsclaus — all four points addressed in af51b2f. The write cost. You are right and my description had it backwards: for ML-DSA, SLH-DSA and Falcon this adds writes where there were none. Three changes follow.
Docs. Non-blocking, both done. Claude Code on behalf of @oscerd |
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks, af51b2f addresses all three points:
- a stateful key now does one
storeKeyper signature and a non-stateful key oneupdateKeyMetadata, documented in the lifecycle page and the upgrade guide; - a failed usage write for a non-stateful key is logged once and the signature is kept;
- the rotation docs now say the default filter keeps
ACTIVEandPENDING_ROTATIONkeys.
Optional nit: the klm.getKeyMetadata(keyId) read before the branch is still outside that try. The vault managers cache the metadata, but on a cache miss a vault error on that read still fails the exchange for a non-stateful key. Moving the read inside the same guard for the non-stateful case would make it consistent.
Claude Code on behalf of Claus Ibsen
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 8 of 707 tested, 25 compile-only — current: 8 all testedMaveniverse Scalpel detected 8 affected modules (current approach: 8). Skip-tests mode would test 8 modules (3 direct + 7 downstream), skip tests for 25 (generated code, meta-modules) Modules Scalpel would test (8)
Modules with tests skipped (25)
All tested modules (35 modules, 4m 59s total)Total reactor time: 4m 59s
Top 20 slowest modules:
|
|
Rebased onto
#28099 is rebased on top of this commit, so the fix reaches it too. Verification: camel-pqc on JDK 21 ( Claude Code on behalf of oscerd |
af51b2f to
71322b5
Compare
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks @oscerd, 71322b5 is right: a failed getKeyMetadata read for a non-stateful key is now caught like the failed write, the signature is kept, and a stateful key still fails. aFailedMetadataReadKeepsTheSignatureOfAStatelessKey covers it. Turning off strictKeyLifecycle in that test is the right way to reach the read. The lifecycle page and its catalog copy match.
One thing to fix before merge. The rebase put two paragraphs of the 4.24 upgrade guide in the wrong section. Recording the usage also costs a write. ... and The key encapsulation operations do not record usage ... now sit at the end of === camel-pqc - an unknown hybridKdfAlgorithm is rejected (lines 266-273), about 40 lines above this PR's own section === camel-pqc - the usage-count and PENDING_ROTATION key rotation signals now fire. Before the rebase they followed check once the counter starts advancing.. Readers of the hybridKdf note would now see them as part of it.
This also causes the only conflict left in the camel-pqc series. #28089 adds its note right after that hybridKdf section, so #28089 and this PR (and #28099, which carries these commits) conflict in the upgrade guide in either merge order. I checked with git merge-file: once the two paragraphs are moved back below check once the counter starts advancing., this PR merges cleanly with #28089 and with all the other camel-pqc PRs.
Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
…ation signals now fire KeyRotationScheduler documents three rotation signals. Two of them could never trigger. usageCount is advanced only by KeyMetadata.updateLastUsed(), whose single production caller sat inside persistStatefulKeyStateAfterSign() behind an early return for any key that is not XMSS/XMSSMT/LMS/HSS. For ML-DSA, SLH-DSA, Falcon and every KEM key the counter therefore stayed at zero forever, while all five lifecycle managers gate rotation on "maxUsage > 0 && metadata.getUsageCount() >= maxUsage". setMaxKeyUsage and the documented camel.component.pqc.key-rotation-max-usage property were inert. That method is now recordKeyUsageAfterSign(): it advances and persists the usage count for every key, and only re-stores the key pair itself for the stateful algorithms, where the advanced one-time index has to survive a restart. This also removes a redundant remote write per signature for the non-stateful keys, which previously did neither. The scheduler's default key filter accepted only ACTIVE keys, so a key an operator had marked PENDING_ROTATION was skipped at the filter before needsRotation() - which does test for PENDING_ROTATION - was ever consulted. The default now accepts ACTIVE and PENDING_ROTATION; a filter supplied through setKeyFilter is unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
…and a stale message The javadoc of recordKeyUsageAfterSign claimed the KEM keys were covered. They are not: no key encapsulation path calls the method, so a KEM key still does not advance its usage count and maxKeyUsage does not apply to it. Stating otherwise would mislead an operator configuring rotation on a KEM key. The javadoc now says which keys are covered and which are not. The hybrid signing catch block still reported "Failed to persist stateful key state" after the method was generalised from persisting stateful state to recording usage. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
…nd the rotation docs Recording the usage of a non-stateful key writes to the backing store on every signature, where such a key previously wrote nothing. The PR description had this backwards. Three changes follow from it. For a stateful key, storeKey already writes the metadata entry in every manager, so the updateKeyMetadata call before it was a second round trip for the same data: six secret writes per signature where three suffice. Only storeKey is called now. For a non-stateful key, a failure to record the usage no longer fails the exchange. It is logged once per producer and the signature is returned. A lost usage count only delays a rotation, which is not worth discarding a good signature for; a lost stateful index is a different matter and still fails. The upgrade guide now states the per-signature write, that a failure of it does not fail the exchange for a non-stateful key, and that the KEM operations never advance the count. pqc-key-lifecycle.adoc still described the default key filter as ACTIVE only, and said nothing about what maxKeyUsage actually counts. It now covers the widened filter, that the count needs a keyLifecycleManager and a CamelPQCKeyId header, that it is recorded against the key named by that header, that the KEM operations do not advance it, and that it costs a remote write per signature. Catalog copy updated. KeyRotationSignalsTest signed with the autowired default material while counting against a different key id, so it did not show the intended setup. It now signs with the very key pair it counts. updateLastUsed carries a note that the increment is not atomic. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
…on-stateful key keeps the signature The metadata read before the usage write went to the keyLifecycleManager unguarded, so a vault that could not be read failed the exchange of a non-stateful key although only its usage count was at stake. The read is now treated like the write: logged once per producer, the signature is returned. A stateful key still fails, as its index must be persisted. KeyRotationSignalsTest covers the read failure with a manager whose reads can be made to fail. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
71322b5 to
0fd54fe
Compare
|
Rebased onto
#28099, which is stacked on this branch, follows with the same rebase and its own nit fix. Claude Code on behalf of oscerd |
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks Andrea, the open point is addressed. The two paragraphs on the cost of recording usage and on the KEM keys are back in this PR's own section camel-pqc - the usage-count and PENDING_ROTATION key rotation signals now fire, after check once the counter starts advancing., and the end of the hybridKdfAlgorithm note is no longer touched. A range-diff against the previous head (71322b5) shows the code, the test and the pqc-key-lifecycle.adoc changes are unchanged; only the upgrade-guide placement moved. The branch is rebased onto main including CAMEL-25798 (#28093), and merges cleanly. The catalog copy of pqc-key-lifecycle.adoc matches the component doc.
CI: the build jobs for the new head are still queued; LGTM once they are green.
This review covers project rules and conventions only. It does not replace CodeRabbit, Sourcery or SonarCloud.
Claude Code on behalf of Claus Ibsen
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Fixes CAMEL-25797.
KeyRotationSchedulerdocuments three rotation signals — key age, usage count, and aPENDING_ROTATIONstatus. Twoof the three could never trigger.
1.
usageCountwas only advanced for stateful keysKeyMetadata.updateLastUsed()is the only thing that increments the counter, and its single production caller satinside
persistStatefulKeyStateAfterSign()behind:So for ML-DSA, SLH-DSA, Falcon and every KEM key — the overwhelming majority — the counter stayed at
0forever,while all five lifecycle managers gate on
maxUsage > 0 && metadata.getUsageCount() >= maxUsage.setMaxKeyUsageand the documentedcamel.component.pqc.key-rotation-max-usageproperty were inert.The method is now
recordKeyUsageAfterSign(). It advances and persists the usage count for every key, andre-stores the key pair itself only for the stateful algorithms, where the advanced one-time index has to survive a
restart. That also drops a redundant remote write per signature for non-stateful keys — they previously did a
storeKeyonly in the stateful branch, so this strictly reduces traffic relative to what stateful keys already dowhile finally recording usage for the rest.
2.
PENDING_ROTATIONwas filtered out before it was readKeyMetadata.needsRotation()does test forPENDING_ROTATION, but a key marked that way is no longerACTIVE, sothe scheduler
continued past it at the filter beforeneedsRotation()was ever consulted. OnlynextRotationAtonan otherwise-ACTIVE key actually worked.
The default filter now accepts
ACTIVEandPENDING_ROTATION. A filter supplied viasetKeyFilteris unaffected.Javadoc on the class and on
setKeyFilterupdated to match.3. A failed metadata read failed the exchange (review follow-up)
Recording the usage reads the key metadata before it writes it. The write was already guarded for a non-stateful
key - losing a usage count only delays a rotation - but the
getKeyMetadataread was not, so a vault that could not beread failed the exchange although the signature was fine. The read is now treated like the write: logged once per
producer (shared
warnUsageRecordFailure), signature kept. A stateful key still fails the exchange, as its advancedindex must be persisted. The lifecycle page and the upgrade-guide note say "read or write".
Tests
New
KeyRotationSignalsTest:signingAStatelessKeyAdvancesTheUsageCount— three ML-DSA signatures, asserts the count reaches 3 and thatneedsRotation(.., maxUsage=3)then reports true;aPendingRotationKeyIsSelectedByTheDefaultFilter— asserts the default filter accepts the key and thatcheckAndRotate()actually rotates it.aFailedMetadataReadKeepsTheSignatureOfAStatelessKey— an in-memory manager whose reads can be made to fail;the exchange reaches the mock with its signature and the usage count stays at zero. It uses an endpoint with
strictKeyLifecycle=false: the key status check before signing is a deliberate fail-closed read of the samemetadata, so with it off the usage read is the exchange's only vault read.
Against
main:expected: <3> but was: <0>andexpected: <true> but was: <false>. Fullcamel-pqcbuild green:241 tests, 0 failures, no regenerated-file drift.
Scope note
Usage is recorded on the signing paths, which are the ones that already had the hook. A pure-KEM deployment still
will not advance
usageCount; extending the hook to the KEM operations is a larger behavioural change and is betterdone separately if wanted.
Upgrade guide
Added a 4.24 entry. This makes previously-inert configuration take effect, so a deployment that already set
maxKeyUsagemay rotate on the first check once the counter starts moving — worth flagging before upgrade.Reported, analysed and fixed by Claude Code (AI agent) on behalf of @oscerd.
🤖 Generated with Claude Code