Skip to content

feat(stack-encrypt): add owned scalar ciphertext API - #1404

Open
coderdan wants to merge 4 commits into
nextfrom
feat/rust-scalar-ciphertext
Open

coderdan wants to merge 4 commits into
nextfrom
feat/rust-scalar-ciphertext

Conversation

@coderdan

Copy link
Copy Markdown
Contributor

The direct Rust API previously required an AAD argument and returned a structural node even for one integer. This implements the first scalar slice of the API redesign in #1365:

let encrypted = keyset.encrypt(42i32).await?;
let value: i32 = keyset.decrypt(encrypted).await?;

let encrypted: Ciphertext = 42i32
    .encrypt_into_with_context(&keyset, ("users", "age"))
    .await?;
let value = i32::decrypt_from_with_context(
    encrypted, &keyset, ("users", "age"),
).await?;

EncryptTarget and DecryptSource declare operations; the owned EncryptInto and destination-directed DecryptFrom helpers execute them through the existing batched engine. RequiredContext, OptionalContext, and NoCallerContext control method availability explicitly: Default never grants permission to omit context. Encryption guards the owned source with Zeroizing, plans synchronously through a borrow, and wipes the source before returning the pending request. This imposes no blanket Clone bound and also covers discarded requests, planning errors and context-conversion unwinding. Returned plaintext belongs to the caller; this is not a claim of complete compiler-copy erasure or constant-time serialization.

Ciphertext exposes canonical byte encoding/decoding and provider-tag access over the unchanged SealedValue v1 envelope. Cross-opening tests exercise both old-writer/new-reader and new-writer/old-reader paths. No Go/dynamic grammar, stored encoding, SQL, or cryptographic derivation changes.

Migration and scope

  • Direct structural calls become encrypt_node / decrypt_node. Existing plans and derives retain their declaration engine; their borrowed conversion helpers move to target::legacy. Existing consumers and tests are updated explicitly.
  • Built-in scalar support covers bool, fixed-width signed integers, u16–u128, f32/f64 and owned String. Collections and borrowed plaintext are rejected. u8 is deferred because the current Vitamin C Rust visitors conflate byte buffers with their element specialization; this PR does not add implicit Vec blob behavior.
  • Blob adapters, collection targets, derive migration, broader context roles, typed text encodings, shared definitions, prepared JSON reads and database adapters remain subsequent slices. Custom declarations can already project a scalar from a non-Clone owned source and declare required or fixed context.
  • Release-plz versions the Rust change from the breaking conventional commit. The empty Changeset records that no npm release is required.

Validation

  • Stack Encrypt all-feature nextest suite: 1,008 passed, one existing skip; final scalar suite: 10 passed.
  • Compile-fail tests cover required context despite Default, forbidden context arguments, collections/borrows and ownership transfer. Existing snapshots refreshed for legacy import paths and rechecked without overwrite.
  • EQL text_eq/v4_types: 23 passed, seven existing PostgreSQL tests skipped; includes frozen Go rows opening in Rust.
  • 52 doctests; rustdoc with warnings denied; runnable credential-free scalar example.
  • All-target/all-feature Clippy with warnings denied and cargo fmt.
  • Go encryption guest, including EQL, checks for wasm32-wasip1 with its locked dependencies.

No live ZeroKMS or PostgreSQL tests were run. Based on latest next at ffeed7a.

@changeset-bot

changeset-bot Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: b684a21

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 0 packages

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@coderabbitai

coderabbitai Bot commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI; Enforced overrides: Organization
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 9573e8d8-f1d7-4ecc-ae95-2997b23c4e58

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • Trigger review

@github-actions

github-actions Bot commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Mutation testing (cargo-mutants, --in-diff, stack-auth + stack-encrypt)

caught missed unviable timeout
10 0 31 0

Every mutant in the changed lines was caught by a test.

@coderdan

Copy link
Copy Markdown
Contributor Author

Two-axis review (standards and spec)

This reviews commit 9a44074b6 against origin/next. The spec is the plan doc docs/plans/2026-10-24-stack-encrypt-api-plan.md on docs/stack-encrypt-api-review, since #1365 only links to it.

Standards

Hard violations

  1. No CHANGELOG entry for a breaking change. The commit is feat(encrypt)!, but packages/stack-encrypt/CHANGELOG.md has no Unreleased → Breaking bullet. Three things break:
    • KeysetCipher::encrypt / decrypt now take a value and return Ciphertext. The old forms were renamed.
    • The root EncryptInto / DecryptFrom are now different traits. The old ones are only in target::legacy.
    • encrypt_native / open_native no longer require NonEmpty.
  2. Wrong error for a shape mismatch. Ciphertext::from_node maps any node that isn't a single leaf to Error::Aead (src/scalar.rs:78). ADR-0010 and AGENTS.md "Known non-findings" say a shape refusal is Error::OpenedType, never Aead. No current caller can reach it.

Documented-standard concerns (judgement calls)

  1. ADR-0001/0003 say the target-directed path enforces a non-empty context by type. OptionalContext<ScalarContext>, plus dropping NonEmpty from encrypt_native / open_native, moves that guarantee to each call site. No ADR records the change.
  2. CONTEXT.md vocabulary: "node" (encrypt_node, from_node) isn't a glossary term.
  3. Roadmap prose in published rustdoc (lib.rs: "will be added in subsequent API slices") will go stale.

Smells (judgement calls)

  • Speculative Generality: library code never uses RequiredContext or NoCallerContext, only tests do. This is mild, since the plan asks for both.
  • Mysterious Name: two traits each called EncryptInto and DecryptFrom, kept apart only by a doc comment.
  • Middle Man: the four KeysetCipher scalar methods are one-line forwards to encrypt_into / decrypt_from.
  • Duplicated Code: each method and its _with_context twin repeat the same body in target/conversion.rs.

Spec

Matches: the keyset method names and shapes, EncryptTarget / DecryptSource, the three context types (with compile-fail tests), Default never granting omission, wiping the source with Zeroizing and no Clone bound, a safe Debug, and unchanged frozen vectors.

Missing or partial

  1. No Serde on Ciphertext. The plan says "human-readable Serde as Base64 of those bytes, binary Serde as bytes". The PR's deferred list doesn't cover it.
  2. The cross-opening tests open bytes written by encrypt_node in the same build. No frozen bytes go through Ciphertext::from_bytes, so "opens old-writer bytes" is untested.
  3. The plan says to remove the competing ciphertext method and the redundant public encrypt_as. Both are still there, which may be fine if the deferred derive migration covers them.

Implemented, but looks wrong

  1. The accessor is tag() (src/scalar.rs:72). The plan asks for "an accessor explicitly named key_tag", so it can't be confused with the AEAD tag. The crate is published, so the name gets locked in.
  2. Scalar is sealed (src/scalar.rs:24). The accepted decision names this capability OpaqueEncode / OpaqueDecode, and says "A custom type can explicitly implement the single-value encoding capability". Sealing rules that out.
  3. The same Error::Aead mapping as Standards finding 2.

Beyond the spec

  • Removing NonEmpty. The plan says search targets "require a nonempty effective context". That now rests on the callers' validated() alone, and no test shows a searchable target still rejects an empty context.

CI

The mutants gate missed three mutants:

  • Debug for Ciphertext → Ok(())
  • OptionalContext::into_inner → None
  • From<NoCallerContext> for DeclaredContext → Default::default()

BREAKING CHANGE: keyset encrypt/decrypt use opaque Ciphertext and separate
context methods. Structural calls move to encrypt_node/decrypt_node;
borrowed conversion helpers move to target::legacy.

Declare owned conversions with EncryptTarget/DecryptSource and explicit
context capabilities. Guard plaintext during planning and preserve the
existing sealed-leaf bytes and Go/dynamic execution paths.
- Rename the structural entry points encrypt_node / decrypt_node to
  encrypt_tree / decrypt_tree.
- Rename the sealed Scalar trait to OpaqueEncode / OpaqueDecode, the
  names the API plan accepted. Both stay sealed.
- Rename Ciphertext::tag to key_tag, so it cannot be read as the AEAD tag.
- Give Ciphertext Serde: padded Base64 in human-readable formats, raw
  bytes in binary ones.
- Report a non-single tree as Error::OpenedType, not Error::Aead
  (ADR-0010).
- Restore NonEmpty on the shared encrypt/open core. The scalar path
  reaches the core through its own entry that allows empty context, and
  ADR-0003 records the exception.
- Open bytes written by the pre-scalar build through Ciphertext.
- Kill the three mutants CI missed. The NoCallerContext conversion is
  equivalent and is excluded.
- Add the CHANGELOG Breaking entries and drop roadmap prose from rustdoc.
@coderdan
coderdan force-pushed the feat/rust-scalar-ciphertext branch from 9a44074 to 5b9f4bd Compare October 31, 2026 08:24
@coderdan

Copy link
Copy Markdown
Contributor Author

Pushed 5b9f4bd, rebased onto the latest next. It addresses the review above:

  • encrypt_node / decrypt_node are now encrypt_tree / decrypt_tree.
  • The sealed Scalar trait is now OpaqueEncode / OpaqueDecode, the names the plan accepted. Both stay sealed; letting outside types implement them can be added later without breaking anything.
  • tag() is now key_tag().
  • Ciphertext has Serde: Base64 text in formats like JSON, raw bytes in binary formats.
  • A tree that isn't a single value now fails with Error::OpenedType instead of Aead.
  • NonEmpty is back on the shared encrypt/open core. The scalar path uses its own entry that allows empty context, and ADR-0003 records that exception.
  • A new test pins bytes written by the build from before this PR and opens them through Ciphertext::from_bytes.
  • New tests kill the Debug and OptionalContext::into_inner mutants. The NoCallerContext mutant can't be told apart from the real code, so it is excluded in .cargo/mutants.toml.
  • The CHANGELOG has Breaking entries now, and the roadmap prose is gone from the rustdoc.

Not changed: encrypt_as and the target::legacy traits are still public, waiting on the derive migration.

@coderdan
coderdan marked this pull request as ready for review October 31, 2026 08:53
@coderdan
coderdan requested a review from a team as a code owner October 31, 2026 08:53
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 31, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-31T08:54:52.011172Z 302aaf9 Draft marked ready
🔒 Security Review ✅ Completed 2026-10-31T08:55:56.440276Z 302aaf9 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to 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" or "@codex security review".

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 cipherstash-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review attestation

Source: Claude Code /code-review medium with cross-check of findings against existing PR feedback, run on head 302aaf9.

Critical or blocking issues: none.

Findings: 1 minor (inline on scalar.rs:126: uncapped Vec::with_capacity from the deserializer's size_hint in Ciphertext's visit_seq; the crate's existing reserved() cap shows the convention). It was verified at head and is not raised in the existing feedback.

Existing feedback: the earlier two-axis review's findings were addressed in 5b9f4bdf6. I found nothing further on those points.

Not covered: I did not run the test suite or CI. Codex review was still running when this review was posted.


Generated by Claude Code

Comment thread packages/stack-encrypt/src/scalar.rs Outdated
self,
mut seq: A,
) -> Result<Ciphertext, A::Error> {
let mut bytes = Vec::with_capacity(seq.size_hint().unwrap_or(0));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Vec::with_capacity(seq.size_hint().unwrap_or(0)) trusts the deserializer's length hint. For untrusted input, a few bytes can declare billions of elements, so this can abort on allocation before from_bytes ever validates the envelope.

The crate already guards against this elsewhere: packages/stack-encrypt/src/dynamic/context.rs:187 has reserved(hint), which caps the reservation at 64 and lets the Vec grow as elements are read. Cap the reservation here the same way, for example .min(…) the hint, or use a small fixed bound.

Source: Claude Code /code-review medium, verified at 302aaf9.


Generated by Claude Code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in b684a21. The reservation is now capped at 256 bytes (.min(256)), and a longer sequence grows as it's read. I inlined the cap because reserved() is in the feature-gated dynamic module. A new test uses a sequence that holds three bytes but claims usize::MAX. Without the cap it panics with "capacity overflow"; with the cap it returns an envelope error.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants