feat(repo): add quoter-signer kms signing and custody attestation - #179
Conversation
|
@codex review |
|
@prd-carapulse[bot] can you review and approve if all good |
|
PR #179 review complete. ❌ Changes requested — not approved.
|
![prd-carapulse[bot]](https://avatars.githubusercontent.com/in/2960675?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@codex review |
|
@prd-carapulse[bot] can you review and approve if all good |
|
Review result: Changes requested. PR not approved. Blocking finding: KMS custody attestation is lazy: Required fix: Add the startup/dedicated attestation and freshness/context gates, with tests covering pre-attestation serving and stale/drift refresh—or narrow the PR contract and docs so this is not presented as the custody gate. CI: Lint, Typecheck, Dead-Code, and Notify pass. Test remains pending. Formal |
There was a problem hiding this comment.
Changes requested: the KMS custody check is lazy, not a startup/before-serving attestation. The TIB requires each active signing deployment to attest at cold start and before serving, then keep readiness red / signing closed when the attestation is stale or missing. Here createHandler() only calls createKmsMakerSigner() after a valid in-policy invocation (index.ts:145), and a successful promise is cached without a freshness/refresh mechanism. A malformed or out-of-policy request can therefore reach a serving container before custody is proven, and a later key/image/config drift is not detected. Add the required invocation/context-gated startup or dedicated attestation path (or narrow the PR contract and docs so this is not presented as the custody gate), with tests for pre-attestation serving and stale/drift refresh. CI Test is also still pending, so approval is withheld.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Round-two findings addressed in bd36b8b:
|
|
@codex review |
|
@prd-carapulse[bot] can you review and approve if all good |
|
Changes requested — not approved. • Blocking: • Checks: ✅ Lint, Typecheck, Dead-Code. ⏳ Test still running. • Review: #179 (review) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Code Review SummaryVerdict: Changes Requested 🔴 (1 blocking issue, 0 suggestions) Blocking
Checks
Review: #179 (review) Reviewed by Hermes Agent |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@codex review |
|
@prd-carapulse[bot] can you review and approve if all good |
|
Reviewed PR #179 at
Approval withheld until Test completes successfully. Review posted: #179 (review) Local test execution was blocked by Node engine mismatch: required |
There was a problem hiding this comment.
Reviewed current head 5ed3c3e.
No blocking code findings: the KMS request-id validation, ARN pinning, single-attempt client, cold-start attestation, cache refresh, signer freshness gate, strict SPKI/DER validation, low-s normalization, recovery checks, and regression coverage are present. git diff --check passes.
Approval withheld: required GitHub Test check is still in_progress; Lint, Typecheck, Dead-Code, and Notify pass. Approve after Test completes successfully.
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@prd-carapulse[bot] the Test check has completed successfully since your last review of 5ed3c3e (all checks green) — can you review and approve if all good |
|
Approved PR #179 at All checks pass: Lint, Test, Typecheck, Dead-Code, Notify. No blocking correctness or security findings. Merge not performed. |
There was a problem hiding this comment.
Reviewed current head 5ed3c3e. All required checks pass: Lint, Test, Typecheck, Dead-Code, and Notify. No blocking correctness or security findings.
f00b0bb to
81ff252
Compare
|
@codex review |
|
@prd-carapulse[bot] can you review and approve if all good |
|
Re-reviewed PR #179 at current head • No new blocking correctness/security findings Approval already exists for the implementation commit. Did not add another approval while Test remains pending. Merge not performed. Comment posted and verified: #179 (comment) |
|
Re-reviewed current head |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@prd-carapulse[bot] the Test check has completed successfully on a5fd117 (all checks green, codex approved this head) — can you review and approve if all good |
|
Review: Approved PR #179 at head ✅ Lint, Test, Typecheck, Dead-Code pass. |
There was a problem hiding this comment.
Reviewed head a5fd117. All required checks pass: Lint, Test, Typecheck, Dead-Code. No blocking correctness or security findings. Local verification was blocked by Node v26.3.0 vs required ^24.14.1; CI is green.
Implement the KMS signing layer of TIB-2026-08-12 in the quoter-signer middleware: strict maker-key custody attestation (GetPublicKey, exact canonical-DER SPKI template parse with on-curve validation, derived address pinned to the policy maker) wired into the fail-closed evaluation pipeline, plus the digest-signing primitive the upcoming encode stages will call (Sign with MessageType DIGEST, strict canonical DER parse, low-s normalization, recovery check across both parities, mandatory KMS request id capture for CloudTrail reconciliation). Every intent is still denied: kms:Sign is never called and the encode-and-sign surfaces are later increments. New deployment parameters QUOTER_SIGNER_KMS_KEY_ID / QUOTER_SIGNER_KMS_REGION are strictly parsed; KMS failures map to typed KmsNotConfiguredError, retryable KmsUnavailableError, and KmsAttestationFailedError denials with the TIB's middleware.kms_error log event. The attested signer is memoized per execution environment with eviction on failure. @aws-sdk/client-kms and @noble/curves move into the pnpm catalog so the bot's direct-KMS path and the middleware stay on the same audited versions. Co-Authored-By: Claude Fable 5 <[email protected]>
- Reject blank or whitespace-only KMS request ids as missing-request-id (carapulse blocking): a blank join key is as unreconcilable as an absent one; valid ids pass through untrimmed. - Pin Sign calls to the resolved key ARN captured at attestation (codex P2): a repointed alias can no longer route signing to an unattested key; attestation now requires the resolved ARN (new key-arn reason). - Single-attempt KMS client, no SDK retries (codex P2): every CloudTrail Sign event must reconcile with exactly one middleware signing record; retry decisions belong to the middleware's callers. Co-Authored-By: Claude Fable 5 <[email protected]>
- Cold-start attestation warm-up (carapulse): when the policy document and KMS variables are configured, custody proving starts at container init, before the first invocation; serving never depends on it and a misconfigured deployment stays a typed denial, never an init crash. - Attestation freshness bound (carapulse): the cached attested signer expires after KMS_ATTESTATION_FRESHNESS_MS (5 min), so key or deployment drift on a warm container fails closed at the next window; eviction is race-guarded so a stale rejection cannot drop a fresher attestation. README narrows the claim: the TIB's registry-backed startup/readiness attestation remains a later increment, and this build's guarantee is that the digest-signing primitive is reachable only behind a fresh attestation. - Denial-contract docs (codex): a denial no longer claims zero KMS activity — kms:Sign is never called, but the attested path performs the read-only GetPublicKey attestation. - Tests: cold-start warm-up, pre-attestation wire-contract serving, stale re-attestation with drift fail-closed (mutation-checked). Co-Authored-By: Claude Fable 5 <[email protected]>
Apply carapulse round-three blocking finding: signDigest() now records its attestation time and refuses to sign once it ages past KMS_ATTESTATION_FRESHNESS_MS (typed retryable KmsAttestationStaleError, no KMS call), so the freshness bound holds for a held signer object and never depends on the handler cache's resolution discipline alone. The constant moves into kms-signer.utils.ts, which owns attestation; expiry boundary test added (mutation-checked). Co-Authored-By: Claude Fable 5 <[email protected]>
a5fd117 to
e6f12e0
Compare
|
@codex review |
|
@prd-carapulse[bot] can you review and approve if all good |
|
PR #179 re-reviewed at head • No blocking correctness or security findings. |
|
Re-reviewed current head
Merge not performed. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6f12e0413
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Apply codex post-rebuild P2: a failed Sign call may have produced a signature server-side with the response lost, so it now maps to the new non-retryable KmsSignOutcomeUnknownError instead of the retryable KmsUnavailableError — advertising invocation-level retry there could mint a second signature and CloudTrail Sign event for one artifact, which maxAttempts: 1 (SDK-internal only) cannot prevent. Re-admission after an ambiguous outcome belongs to the reservation ledger's compensation and CloudTrail reconciliation (later TIB increments). KmsUnavailableError narrows to the read-only attestation call. Co-Authored-By: Claude Fable 5 <[email protected]>
|
@codex review |
|
@prd-carapulse[bot] can you review and approve if all good |
|
Review completed on PR #179. • Code: No new blocking correctness or security findings on head • Latest fix: Correctly maps ambiguous KMS • CI: • Local tests: Blocked by Node mismatch: environment has • GitHub action: Posted a formal |
There was a problem hiding this comment.
Reviewed current head 719cb09. No new blocking correctness or security findings. The latest change correctly maps an ambiguous KMS Sign failure to non-retryable KmsSignOutcomeUnknownError and keeps retryable KmsUnavailableError scoped to read-only attestation. Approval withheld until the required Test check completes successfully; current status: Lint, Typecheck, Dead-Code, Notify pass; Test in_progress. Local package test could not run because this environment has Node v26.3.0 but the repository requires ^24.14.1.
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@prd-carapulse[bot] the Test check has completed successfully on 719cb09 (all checks green, codex approved this head) — can you review and approve if all good |
|
Approved PR #179 at head ✅ No blocking correctness or security findings Reply posted: #179 (comment) |
There was a problem hiding this comment.
Reviewed current head 719cb09. No blocking correctness or security findings. The latest ambiguous KMS Sign outcome handling is non-retryable; the required custody, ARN-pinning, freshness, strict SPKI/DER, and regression coverage remain present. All required checks pass: Lint, Test, Typecheck, Dead-Code, and Notify. Local package verification was blocked by Node v26.3.0 versus the repository requirement ^24.14.1; CI is green.
|
Approved current head 719cb09. No blocking correctness or security findings; Lint, Test, Typecheck, Dead-Code, and Notify all pass. Local package verification was blocked by Node v26.3.0 versus the repository requirement ^24.14.1; CI is green. |
1 participant
Implements the KMS signing layer of TIB-2026-08-12 in the quoter-signer middleware — the third increment. Originally stacked on #178 (deterministic policy checks); after #178 merged to main (2026-08-27) the branch was rebuilt as these four signed commits cherry-picked onto current
main, dropping the interim merge commit and the unsigned pre-rewrite #178-lineage duplicates it had pulled into the commit list — the diff is exactly this increment.Why
The middleware is the future sole
kms:Signprincipal on the maker key, but the skeleton so far never touched KMS: intents passing every deterministic check were denied without proving anything about custody. This increment gives the image its custody root:kms:GetPublicKeyon the deployment-pinned key, exactECC_SECG_P256K1/SIGN_VERIFY/ECDSA_SHA_256shape, the resolved key ARN, strict canonical-SPKI parse, on-curve validation, and a derived address that must equal the policy-pinned maker. The cached attestation carries a five-minute freshness bound (KMS_ATTESTATION_FRESHNESS_MS), so drift on a warm container is re-checked at the next window; drift fails closed (KmsAttestationFailedError), a failed call is a typed retryable denial (KmsUnavailableError), missing/invalid KMS addressing refuses to serve (KmsNotConfiguredError), and the TIB'smiddleware.kms_errorlog event lands.kms:Sign(MessageType: 'DIGEST',ECDSA_SHA_256) issued against the resolved key ARN captured at attestation — never the configured alias, which could be repointed to an unattested key — on a single-attempt client (no SDK retries, so every CloudTrailSignevent reconciles with exactly one middleware signing record) → strict canonical ECDSA-DER parse → low-s normalization → recovery check across both parities against the attested maker → mandatory KMS request id capture (the CloudTrail reconciliation join key; a response without one, or with a blank one, is rejected rather than becoming an unreconcilable signature).Every intent is still denied —
kms:Signis never called by any reachable path, and intents passing every stage end inSigningNotImplementedErroruntil the encode stages and the reservation ledger land. The execution role needskms:GetPublicKeyonly.Design choices
SubjectPublicKeyInfohas exactly one 88-byte encoding — validated by exact prefix template plus@noble/curveson-curve check, unlike the bot's asn1js path. The DER signature parser mirrors the bot's proven parser rather than sharing a module, per the middleware's established mirror-not-share auditability rule. Both recorded in the TIB's Dependencies section.KmsTransport): the AWS adapter maps response fields verbatim; every custody decision lives in unit-tested code. Handler tests inject fakes via the newcreateHandlerfactory; the default transport itself is covered against a mocked SDK asserting the exact pinned command shapes (including the single-attempt client config and ARN-pinnedSign).@aws-sdk/client-kmsand@noble/curvesmove into the pnpm catalog so the bot's direct-KMS path and the middleware stay on the same audited versions.Validation
pnpm --filter @morpho-org/quoter-signer run typecheck,pnpm --filter @morpho-org/quoter-bot run typecheck— cleanpnpm lint— 0 warnings,pnpm format— appliedpnpm test: only the pre-existingRPC_URL_8453fork suites fail locally (environmental)Signtarget from the attested ARN fails 2 testsrevieweragent pass: no blocking findings; all 4 suggestions appliedsignDigest()refuses with a retryableKmsAttestationStaleErroronce its attestation ages past the window (carapulse blocking finding), landed in 5ed3c3e with a boundary test (mutation-checked)🤖 Generated with Claude Code