Skip to content

feat: request-time data encryption - #349

Merged
infiniteregrets merged 132 commits into
mainfrom
m/encryption
Apr 8, 2026
Merged

feat: request-time data encryption#349
infiniteregrets merged 132 commits into
mainfrom
m/encryption

Conversation

@infiniteregrets

Copy link
Copy Markdown
Member

No description provided.

@infiniteregrets
infiniteregrets marked this pull request as draft March 23, 2026 14:46
@infiniteregrets infiniteregrets changed the title feat: request-time data encryption [WIP] feat: request-time data encryption Mar 23, 2026
@greptile-apps

greptile-apps Bot commented Mar 23, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds request-time data encryption to the S2 stream store, supporting AEGIS-256 and AES-256-GCM. Records are encrypted on append (via the s2-encryption HTTP header) and decrypted on read, with the stream ID used as AEAD additional data to bind ciphertext to its stream. The SDK injects the encryption header per append/read request only — not as a shared default header — which resolves the concern from the prior review thread. Command records are never encrypted by design. The cryptographic implementation, nonce handling, and key zeroization are solid.

Confidence Score: 5/5

Safe to merge; all remaining findings are P2 style suggestions that do not affect correctness or security.

The cryptographic implementation is solid: random nonces, stream-scoped AAD, correct zeroization on error paths, and per-request (not global) header injection. The two flagged items are both P2 — a cosmetic over-marking of the 'plain' header as sensitive, and the TUI silently skipping encryption — neither of which affects the encryption correctness or security of the core append/read paths.

cli/src/tui/app.rs (encryption not surfaced to users of encrypted streams) and common/src/encryption.rs (plain header sensitivity flag)

Important Files Changed

Filename Overview
common/src/record/encryption.rs New file: per-record AEAD with random nonces, stream_id AAD binding, and a post-decryption metered_size integrity check; both AEGIS-256 and AES-256-GCM paths are well-tested
common/src/encryption.rs Header parsing and key handling with correct zeroization; plain header value is marked sensitive unnecessarily (P2)
lite/src/handlers/v1/records.rs Encryption injected correctly per append/read request; stream_id used as AAD; all three protocol variants (unary, SSE, S2s) handled
api/src/v1/stream/extract.rs Missing encryption header defaults to EncryptionConfig::Plain via unwrap_or_default(); encryption propagated to all read and append variants
sdk/src/api.rs Encryption header injected per-request via set_encryption_header on append/read paths only — not in default_headers — resolving the previously noted concern
cli/src/tui/app.rs TUI hardcodes None for encryption across all three read call sites; users of encrypted streams will see opaque decryption errors in the TUI
cli/src/main.rs resolve_encryption correctly handles both --encryption flag and --encryption-file; encryption passed as Option<&EncryptionConfig> to all stream ops
cli/src/cli.rs EncryptionArgs flattened into AppendArgs, ReadArgs, TailArgs; --encryption and --encryption-file are mutually exclusive via conflicts_with

Sequence Diagram

sequenceDiagram
    participant Client
    participant SDK
    participant Server
    participant KV

    Note over Client,KV: Append (encrypted)
    Client->>SDK: append(records, EncryptionConfig::Aegis256(key))
    SDK->>Server: POST /records [s2-encryption: aegis-256; <key>]
    Server->>Server: parse s2-encryption header → EncryptionConfig
    Server->>Server: encrypt_record(record, config, aad=stream_id)
    Note right of Server: [suite_id‖nonce‖ciphertext‖tag]
    Server->>KV: put(stream_record_data_key, StoredRecord::Encrypted)
    Server-->>SDK: AppendAck
    SDK-->>Client: AppendAck

    Note over Client,KV: Read (decrypt on the fly)
    Client->>SDK: read(start, end, EncryptionConfig::Aegis256(key))
    SDK->>Server: GET /records [s2-encryption: aegis-256; <key>]
    Server->>KV: scan(start_key..end_key)
    KV-->>Server: StoredRecord::Encrypted{metered_size, record}
    Server->>Server: decrypt_payload(record, config, aad=stream_id)
    Server->>Server: verify metered_size matches plaintext
    Server-->>SDK: ReadBatch (plaintext records)
    SDK-->>Client: ReadBatch

    Note over Client,KV: Missing header → plaintext
    Client->>Server: GET /records (no s2-encryption header)
    Server->>Server: unwrap_or_default() → EncryptionConfig::Plain
    Server->>KV: scan(...)
    KV-->>Server: StoredRecord::Plaintext(record)
    Server-->>Client: ReadBatch (plaintext, no decryption)
Loading
Prompt To Fix All With AI
This is a comment left during a code review.
Path: common/src/encryption.rs
Line: 86-98

Comment:
**`plain` header value unnecessarily marked sensitive**

`value.set_sensitive(true)` is called unconditionally, including for the `Plain` variant whose header value is the constant string `"plain"`. This causes any HTTP-logging middleware or debugging tool that respects `HeaderValue::is_sensitive()` to redact a non-secret constant, making it harder to diagnose plaintext-stream issues without any security benefit.

```suggestion
    pub fn to_header_value(&self) -> HeaderValue {
        let mut value = match self {
            Self::Plain => return HeaderValue::from_static("plain"),
            Self::Aegis256(key) => {
                header_value_for_key(EncryptionAlgorithm::Aegis256, key.secret())
            }
            Self::Aes256Gcm(key) => {
                header_value_for_key(EncryptionAlgorithm::Aes256Gcm, key.secret())
            }
        };
        value.set_sensitive(true);
        value
    }
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: cli/src/tui/app.rs
Line: 4611

Comment:
**TUI silently ignores encryption for all read paths**

All three TUI `read` call sites set `encryption: Default::default()` in `ReadArgs` but then pass `None` directly to `ops::read`. A user who has encryption configured for their stream will see `AlgorithmMismatch` or `AuthenticationFailed` errors in the TUI with no indication that an encryption key is required. If full TUI support is deferred, consider surfacing a user-visible hint when a decryption error is encountered (e.g. "stream appears encrypted; use the CLI with `--encryption` to read it").

How can I resolve this? If you propose a fix, please make it concise.

Reviews (4): Last reviewed commit: "red" | Re-trigger Greptile

Comment thread sdk/src/api.rs Outdated
Comment thread lite/src/backend/core.rs Outdated
Comment thread common/src/encryption.rs Outdated
Comment thread common/src/encryption.rs Outdated
infiniteregrets and others added 12 commits March 23, 2026 21:43
…r layer

Drop seq_num from AAD per team decision. AAD is now stream_id (BLAKE3
hash of basin+stream), matching StreamId::new in lite. This allows
encryption to happen in the HTTP handler before the backend, removing
all encryption plumbing from the streamer pipeline.

- Replace effective_aad_v1(base, seq_num) with stream_id_aad(basin, stream)
- Replace encrypt_sequenced_records with encrypt_append_input (pre-sequencing)
- Remove AppendEncryption struct from streamer/backend
- Encrypt in handler before backend.append, decrypt after backend.read

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
These tests manually encrypted before backend.append() and decrypted
after backend.read(), but encryption now happens in the handler layer.
The roundtrip logic is already covered by unit tests in common/src/encryption.rs.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Compute stream_id AAD lazily (only when encryption header present)
- Remove unused secrecy dev-dependency from lite

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@shikhar

shikhar commented Apr 7, 2026

Copy link
Copy Markdown
Member

@greptileai rereview

@infiniteregrets
infiniteregrets merged commit 1f15730 into main Apr 8, 2026
30 of 31 checks passed
@infiniteregrets
infiniteregrets deleted the m/encryption branch April 8, 2026 15:23
@release-pleaze release-pleaze Bot mentioned this pull request Apr 8, 2026
shikhar pushed a commit that referenced this pull request Jul 6, 2026
Removes `Record::sequenced`, a convenience method for wrapping a
`Record`
into a `SequencedRecord`, along with its sole test coverage. All
production
call sites use `Metered<T>::sequenced` instead; `Record::sequenced` had
no
production callers.

## History
- Last materially changed April 2026 (#349 @infiniteregrets)

---
<sub>_Dead Code PRs can be [configured
here](https://app.detail.dev/org_89d327b3-b883-4365-b6a3-46b6701342a9/settings/repos/repo_c4bd6a47-9b7d-4b62-9c18-8cf0ac18a8f9/dead-code)._</sub>

Co-authored-by: detail-app[bot] <180357370+detail-app[bot]@users.noreply.github.com>
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