Repository navigation
docs(stack-encrypt): key vendor backend design (CIP-5203) - #1645
freshtonic wants to merge 1 commit into
Conversation
ADR-0013 (derived data keys for key vendor backends) and ADR-0014 (hand-written vendor clients signed in the guest), the domain terms, and the decision log.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
cipherstash-agent
left a comment
cipherstash-agent
left a comment
There was a problem hiding this comment.
Review attestation
Reviewed 34f6ef2 · posted from Claude Code (scheduled routine) · claude-opus-5-5 · effort medium
Sources: code-review · claude-opus-5-5; rust-security · claude-opus-5-5
Skills: code-review, rust-security, review-cross-check
Findings (10): 4 major, 6 minor — 10 inline, 0 below.
Docs-only PR: findings are places where the ADRs, glossary and decision log contradict each other or the tree. Every finding was verified at 34f6ef2 and cross-checked against existing feedback (none carried findings). rust-security was applied to the recorded crypto design, not to code. One finding was refuted and dropped: the claim that ADR-0015's one-crate-per-vendor reason is false for every SDK.
Generated by Claude Code
| - **Open:** Vault Transit has the same rule. `KeyDerivation::Detect` refuses | ||
| a Transit key that is not an AEAD, so it refuses an RSA Transit key. The | ||
| explicit `KeyDerivation` settings read no key, so they do not refuse one. |
There was a problem hiding this comment.
major An RSA Transit key configured KeyDerivation::NotDerived or Derived is never type-checked, so anyone with its public key (GET transit/keys/<name>) can RSA-OAEP-encrypt a bound key holding a root key they chose and write it into a keyset record — the same forgery that removed Azure Key Vault. This is recorded only here: ADR-0014's bound-key consequence acts on the public-key rule for Azure alone, the "Summary for review" Open list omits it, and the CIP-5310 note at :448-452 still calls RSA-OAEP a sound bound-key wrap. Refuse non-AEAD Transit key types under every KeyDerivation setting, or at minimum record the hole in ADR-0014 and the summary.
Source: code-review · claude-opus-5-5
Generated by Claude Code
| an unauthenticated label in the record. Under a key router a record's name | ||
| is looked up first, so selecting a keyset by name is only as trustworthy as | ||
| the records the process loads. Select by id where that matters. |
There was a problem hiding this comment.
major A record name wins over a ZeroKMS keyset of the same name under KeyRouter, and the name is not bound to the wrapped key, so whoever can edit the record store can rename a valid vendor record to tenant-a and silently redirect new Keyset::name("tenant-a") seals from ZeroKMS to that vendor keyset. ADR-0002 (:45, :56, :109-112) still says ZeroKMS is the authority on what a name means, and ADR-0014 does not record this rule or its risk. Amend ADR-0002 or add an ADR-0014 consequence — and decide whether the router should refuse a record name that collides with a ZeroKMS keyset rather than letting it win.
Source: code-review · claude-opus-5-5
Generated by Claude Code
| - A vendor with no associated data (Azure Managed HSM, a Vault Transit | ||
| key that is not derived) gets a bound key: the key with a digest of the | ||
| associated data beside it, which the client checks after the unwrap. |
There was a problem hiding this comment.
major This says only "a Vault Transit key that is not derived" gets a bound key, but CIP-5310 decided the Vault client always wraps a 65-byte bound key and also sends context to derived keys (log :462-471), rejecting exactly this split. An implementation built from this ADR (which :39-41 says must match byte for byte) sends a raw 32-byte root key to a derived key and cannot open Rust-written records, nor Rust its. Say the Vault client always wraps a bound key, and sends the associated data as context to a derived key.
Source: code-review · claude-opus-5-5
Generated by Claude Code
| - **CIP-5308 (Revisit, security):** the GCP service account login uses the | ||
| `rsa` crate (RUSTSEC-2023-0071); a security reviewer must sign it off | ||
| before the crate is armed; revisit when `rsa` 0.10 is stable. |
There was a problem hiding this comment.
major Stale security item: since 2026-11-13 native RS256 uses aws-lc-rs and no native or guest build ships rsa (:709-715, :813-815), so the only remaining RUSTSEC-2023-0071 exposure is a wasm32 build with service-account on, which this bullet does not say. The Azure client-certificate deferral (:950-951, :1019, :1592) also still cites a missing constant-time RSA signer that aws-lc-rs now provides natively. Rewrite this bullet to the wasm32 + service-account case and restate the Azure reason.
Source: code-review · claude-opus-5-5
Generated by Claude Code
| A name is a *lookup*, not an identity, so a binding is trusted only within | ||
| a window, a keyset holds at most one at a time, and no binding outlives the | ||
| id it names. Two places answer the lookup. ZeroKMS answers it for its own | ||
| keysets, and allows renames. For a keyset in a key vendor backend, the | ||
| loaded keyset records answer it, locally and with no window: the name is | ||
| an unauthenticated label in the record. Under a key router a record's name |
There was a problem hiding this comment.
minor "With no window" contradicts the first sentence: stack-encrypt is unchanged, so the cipher caches a vendor name binding for DEFAULT_NAME_TTL like any other; it is the record set that cannot change within a process. Pending (:348-352, "ZeroKMS key requests") and Freshness window ("asks ZeroKMS again") were not widened and still contradict Settle and Key backend. Say the cipher's window applies to every binding, and use "the key backend" in Pending and Freshness window.
Source: code-review · claude-opus-5-5
Generated by Claude Code
| We own the request signing code for four vendors, and its tests. Recorded | ||
| HTTP fixtures cover every vendor. LocalStack and `vault server -dev` run on | ||
| every PR. Live Google Cloud and Azure tests run in an opt-in, credentialed | ||
| workflow. The Azure one has not run yet: we have no live Managed HSM, so it | ||
| stays off until an instance is available. |
There was a problem hiding this comment.
minor Overstates test coverage: only Vault's fixtures were recorded — AWS responses, and all GCP and Azure fixtures, were written by hand (log :354-358, :735-736, :958-959); the LocalStack and Vault jobs live in tests-crates.yml, which runs only on its listed paths, not every PR; and the Google Cloud live job has not run either (log :777-779), not just Azure's. State what the log records.
Source: code-review · claude-opus-5-5
Generated by Claude Code
| - **Decision:** the work is GitHub stack 1633 on `next`, bottom up: #1624 | ||
| (design: ADRs, `CONTEXT.md` and this log), #1632 (CIP-5304, | ||
| `stack-kms`), #1625 (CIP-5306 and CIP-5313, `stack-kms-vendor`), #1626 | ||
| (CIP-5307, AWS), #1627 (CIP-5310, Vault), #1628 (CIP-5308, GCP), #1629 | ||
| (CIP-5309, Azure) and #1630 (CIP-5311, Go). #1631 (CIP-5312, `stash | ||
| keyset`) is a separate PR on #1629, outside the stack. **Reason:** the | ||
| code layers cite the ADRs and the glossary terms, so the design is the |
There was a problem hiding this comment.
| - **Decision:** the deterministic test build keeps the vendor clients, so it | ||
| now imports `transport_send` and `vendor_credential_get` (not | ||
| `token_get`). Its config is the bare 32-byte seed, as before, or an object | ||
| `{"seed", key_vendor, keyset_records, ...}`. The mise import gates of all | ||
| four builds now require `vendor_credential_get`. |
There was a problem hiding this comment.
minor Never marked superseded: the deterministic build still "imports transport_send and vendor_credential_get" (both gone, :1643). Same for azure-key-vault as a stable vendor name (:115), AzureKeyVault (:1155), AzureKeyVaultError (:1131), DefaultAzureKeyVault::from_env (:1305) and the Azure Key Vault RSA wrap at :128 and :591-592. Add inline superseded notes, as the CIP-5309 section does.
Source: code-review · claude-opus-5-5
Generated by Claude Code
| | deterministic test build | n/a | 1,657,639 B | | | ||
| | deterministic test build, EQL | n/a | 1,726,501 B | | |
There was a problem hiding this comment.
minor These two rows follow a blank line and a paragraph, so GFM ends the table and renders them as literal pipe text. Move them up after :1032 and put the parenthetical after the table.
Source: code-review · claude-opus-5-5
Generated by Claude Code
| - **Decision:** the static key backend (`StaticKeyVendor`) wraps with | ||
| AES-256-GCM under a random 96-bit nonce: `version(1)=1 ‖ nonce(12) ‖ | ||
| ciphertext ‖ tag`. It holds several keys by reference, for rotation. It | ||
| uses `aes-gcm` 0.10.3, already in the lockfile. |
There was a problem hiding this comment.
minor The static-key wrap uses RustCrypto aes-gcm with no recorded reason, though vitaminc-encrypt 0.9.0 (AES-256-GCM, aws-lc-rs native / RustCrypto on wasm32) is already a workspace dependency and already in the guest's Cargo.lock; the HKDF entry at :157-166 likewise gives no reason for hkdf/sha2 (vitaminc has no HKDF and aws-lc-rs does not build for wasm32 — say so). Record why vitaminc was passed over, or wrap with vitaminc-encrypt.
Source: rust-security · claude-opus-5-5
Generated by Claude Code
Records the design for CIP-5203: Stack Encrypt with AWS KMS, Azure Managed HSM, HashiCorp Vault Transit, Google Cloud KMS and a static key, as alternatives to ZeroKMS.
This PR changes docs only. It changes no code.
docs/plans/2026-11-05-key-vendor-backends-decisions.md.Implementation is tracked in the sub-issues of CIP-5203 (CIP-5304 to CIP-5313). The follow-ups are CIP-5314 (TypeScript) and CIP-5315 (migration from a vendor to ZeroKMS).
Replaces #1624, which was closed. The review on #1624 is addressed in this version.