feat(agent-runtime): verify signed procedural evaluation handoff - #594
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
seonghobae
left a comment
There was a problem hiding this comment.
Current-head review on cc93730c581fa4a41ce218c437939434129d34ee: the signed evaluator handoff remains inside Agent Runtime's verification/admission boundary and preserves activationAuthorized:false; private-key custody, Keyverse identity selection, provider routing, durable State / Checkpoint, Policy / Approval, and publication authority remain external. The post-verification reuse defect is repaired by re-checking signed expiry at consumption, and focused tests now exercise malformed schema/signature/trust, signature-verification failure, reversed/overlong/future/expired intervals, structural-copy rejection, and post-admission expiry. Root CHANGELOG doctoring is present and unrelated historical text/newline drift has been repaired. No additional valid source/DDD/authority finding is identified in this four-file exact diff. This is COMMENT only, not self-approval; merge remains conditioned on the four terminal current-head gates and fresh thread/governance checks.
seonghobae
left a comment
There was a problem hiding this comment.
Current-head review on d4c0ff4753c0281a03fb4ef1968c2708e17af252: the application-CI failure on predecessor cc93730... is now attributable from the job log to a test fixture that accidentally hit the verifier's earlier expiry classification (expires=now) instead of the intended reversed-interval branch. This exact changes only that fixture to issued=now+10, expires=now+5, remaining inside the allowed +30s clock-skew window while making expires <= issued true. Production classification, signature validation, expiry-at-consumption repair, authority boundaries, and gates are unchanged. No additional valid finding is identified in this exact delta. COMMENT only; merge still requires terminal current-head application CI, reviewer-ci, central Security Scan, patch-validator-image, fresh unresolved-thread check, and unchanged live base.
seonghobae
left a comment
There was a problem hiding this comment.
Current-head security review found one remaining identity-malleability defect. The base64url round-trip repair removes textual aliases for identical 64-byte signatures, but ECDSA itself is still malleable: for P-256, (r, s) and (r, n-s) verify the same signed message under the same public key. handoffDigest currently hashes the raw signature string, so an untrusted holder can derive a second valid canonical signature without the private key and obtain a different handoff identity for the same signer/envelope/time claim. This matters before durable retention/CAS because replay/dedup/audit identity must not depend on a randomized or malleable signature instance. Repair should preserve signature verification but derive handoffDigest from the canonical signed claim semantics rather than the raw signature instance (or otherwise normalize ECDSA S). Add a reality-RED regression using the P-256 order and the (r,n-s) counterpart. COMMENT only; do not merge this exact.
seonghobae
left a comment
There was a problem hiding this comment.
Current-head review on 2dd0a32320db5de5e97f8477a4b1d5f13115640b: both handoff-identity malleability findings are repaired without weakening authentication. Transport now rejects noncanonical base64url aliases, ECDSA verification remains mandatory, and handoffDigest is derived from the exact signed claim tuple rather than the randomized/malleable signature instance, so (r,s) and (r,n-s) cannot multiply durable/audit identity for one signer/envelope/time claim. Expiry-at-consumption, structural-copy rejection, Keyverse/key-custody boundary, and activationAuthorized:false remain intact. Four current-head workflows are terminal SUCCESS and there are zero inline review threads. No additional valid source/test/DDD/authority finding is identified in the four-file exact patch. COMMENT only, not self-approval.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Purpose
Continue #584 from protected
main@4bb78a0365a81bc332eb301c96bd98794bf5f87awithout creating evaluator/provider/credential ownership inside Agent Runtime. This lane verifies an evaluator handoff against a separately trusted verification key/key-id supplied by the composition root; Keyverse remains key/identity authority.Reality RED and repairs
2ca196a5520d369d7e8450488db921f56d9fa6ecproduced hosted application-CI RED in run34466414558, job102835939148: the focused suite could not import the deliberately absent signed-handoff boundary.6900cb27c5c0dddbdebd311951312ece5d2f9953added the minimal verifier anda9b5cd0df98adf2daf3c6ccb269eda3e88d373faclosed its error taxonomy.expiresAtEpochSeconds, so downstream consumers could reuse an expired evaluator assertion. Test-first3afeef2c1e9f9752555a2b381f10787401e59b62added the post-admission expiry regression;ac670ace0e01964a67358495f67b4c7d9a7c8316makesassertAuthenticatedProceduralEvaluationEvidence()re-check signed expiry at consumption.c91f707735f491d1891eb2af5ad1e0d4c66b4096closes malformed signature/trust/time/verification edge coverage required by the repository-wide 100% gate.3f608285ef0f19393da7a614e921d4d47041e9fcrecords the behavior under rootCHANGELOG.md## Unreleased;878cdc48877529fbced40fc5cb1bd6e090d95606restores an unrelated historical changelog line exactly after the replacement write;cc93730c581fa4a41ce218c437939434129d34eerestores the pre-existing newline-at-EOF contract inprocedural-input.ts.cc93730c581fa4a41ce218c437939434129d34eeproduced hosted application-CI RED in run34470591310, job102849581864,release tests: one test expectedevaluation_handoff_time_invalidfor an interval whose expiry equaled the current second, but production correctly classified it first asevaluation_handoff_expired.d4c0ff4753c0281a03fb4ef1968c2708e17af252corrected only that fixture so the reversed interval stays future-but-within-skew while satisfyingexpires <= issued.d4c0ff4753c0281a03fb4ef1968c2708e17af252found textual-signature malleability: a 64-byte P-256 signature has 86 base64url characters, but permissive decoders ignore the four unused low bits of the last character. Up to 16 textual aliases can decode to identical signature bytes, pass ECDSA verification, yet produce differenthandoffDigestvalues. Test-firstf2e47d726c7a89357eb6c05b84f1d8f6fced0cfdproduced hosted application-CI RED in run34471769795, job102853080166, atrelease tests;d774476bb52c449f72677ea5fe433ee368f8b00fround-trips decoded bytes to canonical unpadded base64url and rejects aliases before verification/digest construction.(r,s)and(r,n-s)can both verify the same signed claim. Becaused774476...still hashed the raw signature instance, an untrusted holder could derive a second valid canonical signature without the private key and receive a different handoff identity. Test-firste64292c83de7cfe1b7b3eef1bca1d43b367f7336constructed the(r,n-s)counterpart, proved it verifies under the same public key/message, and produced hosted application-CI RED in run34472236312, job102854583498, atrelease tests.2dd0a32320db5de5e97f8477a4b1d5f13115640bis the minimal causal repair: signature verification remains mandatory, buthandoffDigestis now derived from the canonical signed claim tuple (schema, envelope digest, signer key id, issued-at, expires-at) rather than a randomized/malleable signature instance. Canonical base64url is still required for transport.Authority boundary
The verifier owns only exact envelope/signer/time/signature verification, canonical signature transport representation, stable signed-claim identity, and process-local admission. It stores no private key, performs no Keyverse discovery, does not route providers/models, does not create a second State / Checkpoint or Workflow / Task truth, grants no Policy / Approval, publishes no cross-service contract, and never changes
activationAuthorized:false. A signature authenticates only this bounded handoff under the public key/key-id selected by the composition root.assertAuthenticatedProceduralEvaluationEvidence()also fails closed after signed expiry.Readiness
Current exact is
2dd0a32320db5de5e97f8477a4b1d5f13115640b, based on protected4bb78a0365a81bc332eb301c96bd98794bf5f87a. Keep Draft until the complete current-head patch has no valid unresolved review finding/thread and application CI, reviewer-ci, central Security Scan, and patch-validator-image are terminal GREEN on this same exact head. Predecessor GREEN is not transferable.