Repository navigation
Conversation
|
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
 |
|
Mutation testing (cargo-mutants,
|
| caught | missed | unviable | timeout |
|---|---|---|---|
| 3 | 0 | 21 | 0 |
Every mutant in the changed lines was caught by a test.
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
Review of this PR's own diff (one commit, c338c6212). No blockers. Four should-fix findings (two doc accuracy, two test gaps) and six nits. Details are in the inline comments; the numbers below match them.
Soundness and security: no finding
I checked whether CipherRef keeps the guarantee the sealed trait gave: that the keyset ID in a scope is one the cipher loaded from ZeroKMS.
- The fields are private. The new compile-fail case pins that (struct literal, E0451). Struct update syntax hits the same error.
- There is no
Default, no serde, and no public constructor other than the threeFromimpls.CopyandClonecopy a valid value whole. They cannot change the keyset. keyset_id()is read-only.cipher()ispub(crate).- The longer lifetime from
&KeysetCipher<'c, K>is sound.KeysetCipheris&'c StackCipherplusArc<KeysetState>. APendingholds&'a StackCipher, anOption<Uuid>, requests and boxed closures. Nothing built through aCipherRefcan borrow the handle, because aCipherRefdoes not carry it.scoped_totakes aUuid. Thejson_*_key_nowpath takes aKeysetCipherdirectly and does not go throughCipherRef. From<KeysetCipher>by value drops theArc<KeysetState>, and nothing needs it. The olddecrypt_withalready read onlykeyset.cipher()andkeyset.keyset_id()fromScope::Keyset.- Verified by running code: an owned foreign
KeysetCipherpassed toi32::decrypt_fromis refused withForeignKeyset, with 0 retrieve calls.
Behaviour preservation: no finding
decrypt_with resolves to the same (cipher, Option<keyset>) pair as before. open_in, bind_pending, collect_releasing, decrypt_through and the Pending constructors read the same two values that the trait methods returned. Every function that took impl CipherScope already accepted both &StackCipher and &KeysetCipher, so no keyset-only entry point now accepts the client. The only widening is that an owned KeysetCipher is now accepted by the typed API, and that is safe (above). Each function converts its argument once, but some convert and then forward to another impl Into function (finding 7).
Removal of dynamic::Scope and CipherScope: no finding
git grep finds no remaining use in Rust, Go, the fuzz crates or the benches. The Go test TestReadTreePreservesCipherScope is about the record context, not this type. No #NNNN was added to a code comment.
Findings
- should fix: the rustdoc on
CipherRefsays every entry point that runs through a cipher takes it. That is not true. - should fix: the CHANGELOG entry leaves out functions that changed, and the new things a caller can now do.
- should fix: no test passes an owned
KeysetCipherto the typed API. No test pins the longer lifetime. - should fix: the guest's
scope_forhas no test for each selector. - nit:
keyset()duplicateskeyset_id(). - nit:
Debugprintskeyset, but the accessor andKeysetCipher'sDebugsaykeyset_id. - nit: some functions convert, then forward to another
impl Intofunction. - nit: a compile-fail case for a wrong argument type is worth adding. The message is good.
- nit: a tracking line in the plan is stale.
- nit: the
CONTEXT.mdentry has one long unwrapped line, and leaves out the ownedKeysetCipher.
Verified by running code: the scratch tests for 3 and the compiler output for 8. Everything else here is from reading the code.
72e8860 to
2f03c31
Compare
63424a2 to
4b30382
Compare
|
Rebased onto the reviewed head of the pull request below it in the stack. The commits are the same changes with new hashes. Nothing else changed. |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
cipherstash-agent
left a comment
cipherstash-agent
left a comment
There was a problem hiding this comment.
Review attestation
Reviewed 4b303829 · posted from Claude Code · claude-opus-5-5 · effort medium
Sources: code-review · claude-opus-5-5
Skills: code-review, review-cross-check, review-post
Findings (1): 1 minor, 1 inline.
The review raised five other findings. All five were refuted at head, so none is posted:
- Confinement in
record::decrypt_withholds structurally, becausecollect_releasingrunscheck_scopeover every merged item. - The three copies of the confine rule and the hand-split in
json_query::openwere already there at base and behave the same. - The
Goguest instantiatesdecrypt_withonly once. - The redundant conversions were fixed in this PR's second commit.
Generated by Claude Code
| impl<K> Sealed for &crate::KeysetCipher<'_, K> {} | ||
| impl<'c, K> CipherRef<'c, K> { | ||
| /// The connection the pending settles through. | ||
| pub(crate) fn cipher(&self) -> &'c StackCipher<K> { |
There was a problem hiding this comment.
minor Base exposed the connection publicly, through CipherScope::cipher() and the public Scope::Client/Scope::Keyset variants. Here cipher() is pub(crate), so an outside function migrated as the CHANGELOG says (impl CipherScope<'a, K> → impl Into<CipherRef<'a, K>>) that calls scope.cipher() no longer compiles, and the CHANGELOG gives no replacement. Make cipher() pub, which leaves the keyset invariant intact because the fields stay private and the three From impls are still the only constructors, or say in the CHANGELOG that the connection is withheld.
Source: code-review · claude-opus-5-5
Generated by Claude Code
2f03c31 to
4cd9647
Compare
b24c0c7 to
18cff59
Compare
914417d to
edc4107
Compare
18cff59 to
32519a5
Compare
… one CipherRef The typed API took a cipher through the sealed trait CipherScope. The dynamic interface took one through the enum dynamic::Scope. Both reduce to a connection and an optional keyset ID. The naming review replaced both with one borrowed value. CipherRef holds the connection and the keyset. It is built from a reference to a StackCipher or a KeysetCipher, or from an owned KeysetCipher, and its fields are private, so the keyset it names is one the cipher loaded. Every entry point that took a scope takes impl Into<CipherRef>, so a call that passes &cipher or &keyset is unchanged. The Go guest builds one from its keyset selector. The rules do not change: a client reference opens a leaf from any keyset, and a keyset reference refuses a foreign leaf before any key is retrieved. Part of #1488
- The rustdoc on CipherRef named every entry point that runs through a cipher. It now names the ones that take it: the decrypt entry points and the Pending constructors. It says that an encrypt entry point takes a KeysetCipher, and that a CipherRef lives as long as the connection. - The CHANGELOG entry lists every changed function. It says that an owned KeysetCipher is now accepted, that a pending through a temporary keyset handle now compiles, and what to write in place of a CipherScope bound. - CipherRef has one accessor for the keyset, keyset_id(). Its Debug output names the field keyset_id, as KeysetCipher's does. - Functions that only forward the argument no longer convert it first. The private collect_releasing and decrypt_through take a CipherRef. - New tests: an owned KeysetCipher refuses a foreign leaf before any key is retrieved, and opens its own; a pending outlives a temporary keyset handle; the guest's scope_for gives the right keyset for each selector; and a compile-fail case pins the "consider borrowing" hint for a StackCipher passed by value. - CONTEXT.md is rewrapped and names the owned KeysetCipher. The plan's tracking line says that #1120 is closed and #1504 is merged.
The CipherRef docs now have a section for each question a caller has: how to pass one, what each kind of reference can do, and how long one is valid. The docs say that a reference limits an operation to a keyset, where they said confined.
32519a5 to
9916276
Compare
Summary
Stack Encrypt now has one type for "which cipher does this call run through":
CipherRef. It replaces two types that did the same job, one in the typed API and one in the dynamic interface.This is one part of #1488, not all of it. #1488 removes the old API in several pull requests. #1597 was the first. This is the second. The plan chain, the plan builder,
EncryptFrom,DecryptInto,encrypt_as,target::legacyandLabelare still there, and #1488 stays open.Stack Encrypt (
packages/stack-encrypt) is the Rust encryption engine under every CipherStash SDK. A program holds aStackCipher, which is the connection to ZeroKMS (our key service), and aKeysetCipherfor each keyset it uses. A keyset is a group of keys, usually one per tenant. A call that decrypts runs through one or the other:StackCipher, it opens a value from any keyset the client can use;KeysetCipher, it opens only that keyset's values, and refuses the rest before any key is fetched.The typed API expressed that choice as a sealed trait,
target::CipherScope. The dynamic interface, which the Go SDK uses, expressed it as an enum,dynamic::Scope. Both came down to a connection and an optional keyset ID.This PR is stacked on #1609. The base branch is the branch of #1609, so the diff shows only this change.
Changes
Stack Encrypt
CipherRef<'c, K>holds a borrowed connection and an optional keyset ID. It is built byFrom<&StackCipher>,From<&KeysetCipher>andFrom<KeysetCipher>, and in no other way: its fields are private. So the keyset it names is one the cipher loaded from ZeroKMS.target::CipherScopeis removed. Every entry point that tookimpl CipherScopetakesimpl Into<CipherRef>:DecryptFrom::decrypt_fromanddecrypt_from_with_context,Pending::ready,failed,requestandall, and the legacydecrypt_into. A call that passes&cipheror&keysetcompiles unchanged.dynamic::Scopeis removed.dynamic::record::decryptanddecrypt_withtakeimpl Into<CipherRef>. A caller writes&cipherin place ofScope::Client(&cipher), and&keysetin place ofScope::Keyset(keyset).CipherRef::keyset_id()returns the keyset the reference is confined to.CipherRefis re-exported from the crate root.KeysetCipher. A pending built through a temporary keyset handle, such asi32::decrypt_from(ct, &cipher.default_keyset()), can be stored and awaited later.Go guest
scope_forinlanguages/golang/encrypt/guest/src/options.rsreturns aCipherRefbuilt from the keyset selector.decrypt_recordtakesimpl Into<CipherRef>.Tests
src/target/pending.rschecks the three ways to build aCipherRef, and that a pending keeps its keyset.tests/scalar.rschecks that an ownedKeysetCipherrefuses a foreign leaf before any key is retrieved and opens its own, and that a pending outlives a temporary keyset handle.each_opening_selector_names_its_keysetin the Go guest'stests/native_ops.rschecksscope_forfor each selector.tests/ui/cipher_ref_by_value.rspins the compiler's "consider borrowing here" hint for aStackCipherpassed by value.tests/ui/foreign_cipher_ref.rsreplacesforeign_cipher_scope.rs. The old case showed that an outside crate could not implement the sealed trait. The new case shows that an outside crate cannot build aCipherRefwith a keyset ID of its choice.Scope::Client(..)orScope::Keyset(..)passes the cipher or keyset directly. That is 13 files inpackages/stack-encrypt/tests, two EQL encryption tests, and two Go guest tests. No expectation changed.Docs
CONTEXT.mdupdates the term Scope.Verification
I ran these locally, on Rust 1.94.1:
cargo nextest run -p stack-encrypt -p stack-encrypt-derive --all-featurescargo test -p stack-encrypt --all-features --doccargo clippy -p stack-encrypt --all-features --all-targets -- -D warningscargo doc -p stack-encrypt --no-deps, warnings as errors, with all features and with nonecargo test -p eql-bindings -p eql-encryption-tests --all-features, and clippy on bothcargo testwith and without--features eql, and clippy for bothcargo check --features eql --target wasm32-wasip1I did not run the Go module's tests, the Postgres suites, the live tests, or mutation testing locally. CI runs them.
Related
Part of #1488. It does not close it. Stacked on #1609.
Review notes
CipherRefinpackages/stack-encrypt/src/target/pending.rs. Then readdecrypt_withinsrc/dynamic/record.rs. Most of the rest of the diff removes a wrapper at a call site.From<&KeysetCipher<'c, K>>gives aCipherRef<'c, K>: the lifetime of the connection, not of the borrow of the keyset handle. The old trait gave the shorter one. The longer one is sound, because aCipherRefcopies the keyset ID and keeps only the connection. It is what lets the Go guest return aCipherReffrom a keyset it resolved inside a function.TargetResolver::decryptstill takes&StackCipher. The engine confines the result to the keyset afterwards, as before. A later pull request for stack-encrypt refactor: remove transitional traits and redundant execution APIs #1488 changes that trait's signature whenLabelis removed, so this one leaves it.