Add encryption compress-then-encrypt path - #1110
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 ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughChangesStage 9A 暗号化圧縮
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant EncryptedStore
participant Pebble
participant Snappy
Client->>EncryptedStore: 値を書き込む
EncryptedStore->>Snappy: 平文を圧縮する
Snappy-->>EncryptedStore: 圧縮済みペイロードとフラグ
EncryptedStore->>Pebble: フラグをAADに含めて暗号化値を保存する
Client->>EncryptedStore: 値を読み取る
EncryptedStore->>Pebble: 暗号化値を取得する
EncryptedStore->>Snappy: 認証後のペイロードを復元する
Snappy-->>Client: 元の平文を返す
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Stage 9A verification completed on the current signed head.
@codex review |
|
✅ Action performedReview finished.
|
![gemini-code-assist[bot]](https://cdn.github.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements Stage 9A data-at-rest compression (compress-then-encrypt) using Snappy before AES-256-GCM encryption. Pebble block compression is disabled for encryption-wired stores to avoid redundant CPU usage on high-entropy ciphertext. Feedback on these changes suggests checking the decoded length of Snappy payloads against maxSnapshotValueSize before decompression to prevent potential out-of-memory (OOM) panics, and adding a fast-path threshold check to bypass compression for very small plaintexts to optimize write-path performance.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if env.Flag&encryption.FlagCompressed != 0 { | ||
| plain, err = snappy.Decode(nil, plain) | ||
| if err != nil { | ||
| return nil, errors.Wrap( | ||
| errors.WithSecondaryError(ErrEncryptedReadCompression, err), | ||
| "store: decompress encrypted value") | ||
| } | ||
| } |
There was a problem hiding this comment.
When decompressing the decrypted payload, snappy.Decode reads the uncompressed length prefix from the untrusted/corrupted payload and allocates a slice of that size. To prevent potential out-of-memory (OOM) panics from corrupted or malformed payloads, check snappy.DecodedLen against maxSnapshotValueSize before calling snappy.Decode.
if env.Flag&encryption.FlagCompressed != 0 {
dLen, err := snappy.DecodedLen(plain)
if err != nil {
return nil, errors.Wrap(
errors.WithSecondaryError(ErrEncryptedReadCompression, err),
"store: decompress encrypted value")
}
if dLen > maxSnapshotValueSize {
return nil, errors.Wrap(
errors.WithSecondaryError(ErrEncryptedReadCompression, errors.New("decoded length exceeds maximum value size")),
"store: decompress encrypted value")
}
plain, err = snappy.Decode(nil, plain)
if err != nil {
return nil, errors.Wrap(
errors.WithSecondaryError(ErrEncryptedReadCompression, err),
"store: decompress encrypted value")
}
}References
- When deserializing data with a size header, pre-allocating a buffer is acceptable if a strictly enforced maximum size limit is checked before allocation and the entire data must be materialized in memory. Avoid adding complexity with chunked reads if it doesn't meaningfully change the peak memory bound.
| func compressForEncryption(plaintext []byte) ([]byte, byte) { | ||
| compressed := snappy.Encode(nil, plaintext) | ||
| if len(compressed) >= len(plaintext) { | ||
| return plaintext, 0 | ||
| } | ||
| return compressed, encryption.FlagCompressed | ||
| } |
There was a problem hiding this comment.
For very small plaintexts, Snappy compression is highly unlikely to yield a strictly smaller size due to the format's framing and varint overhead. Adding a fast-path threshold check avoids unnecessary snappy.Encode allocations and CPU cycles on the write path for small values.
| func compressForEncryption(plaintext []byte) ([]byte, byte) { | |
| compressed := snappy.Encode(nil, plaintext) | |
| if len(compressed) >= len(plaintext) { | |
| return plaintext, 0 | |
| } | |
| return compressed, encryption.FlagCompressed | |
| } | |
| func compressForEncryption(plaintext []byte) ([]byte, byte) { | |
| if len(plaintext) < 32 { | |
| return plaintext, 0 | |
| } | |
| compressed := snappy.Encode(nil, plaintext) | |
| if len(compressed) >= len(plaintext) { | |
| return plaintext, 0 | |
| } | |
| return compressed, encryption.FlagCompressed | |
| } |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/encryption/envelope.go (1)
93-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win新しい境界エラーを
github.com/cockroachdb/errorsに統一してください。3地点とも
github.com/pkg/errors.Wrapfを追加しており、リポジトリの境界エラー規則と一致しません。
internal/encryption/envelope.go#L93-L96: Encode のフラグエラーを指定ライブラリでラップする。internal/encryption/envelope.go#L123-L126: Decode のフラグエラーも同じ実装へ統一する。internal/encryption/raft_envelope.go#L108-L111: Raft のフラグ拒否も同じエラーライブラリへ統一する。As per coding guidelines, 「Wrap errors with
github.com/cockroachdb/errorsat boundaries in Go code」に従ってください。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/encryption/envelope.go` around lines 93 - 96, Replace the boundary-error wrapping in the flag validation paths with github.com/cockroachdb/errors, covering the Encode validation in internal/encryption/envelope.go:93-96, the Decode validation in internal/encryption/envelope.go:123-126, and the Raft flag rejection in internal/encryption/raft_envelope.go:108-111; preserve the existing error values and messages.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@store/lsm_store_sync_mode_benchmark_test.go`:
- Line 113: Remove the //nolint directive from the startTS calculation in the
benchmark and add an explicit negative-boundary check for the loop index,
following the pattern used by BenchmarkApplyMutationsRaft_SyncMode to satisfy
gosec without suppressing the warning.
---
Nitpick comments:
In `@internal/encryption/envelope.go`:
- Around line 93-96: Replace the boundary-error wrapping in the flag validation
paths with github.com/cockroachdb/errors, covering the Encode validation in
internal/encryption/envelope.go:93-96, the Decode validation in
internal/encryption/envelope.go:123-126, and the Raft flag rejection in
internal/encryption/raft_envelope.go:108-111; preserve the existing error values
and messages.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6a9de206-f939-4bfa-b57a-43031028e852
📒 Files selected for processing (14)
docs/design/2026_04_29_partial_data_at_rest_encryption.mddocs/design/2026_07_18_implemented_9a_encryption_compression.mdgo.modinternal/encryption/envelope.gointernal/encryption/envelope_test.gointernal/encryption/errors.gointernal/encryption/raft_envelope.gointernal/encryption/raft_envelope_test.gostore/encryption_compression_test.gostore/encryption_glue.gostore/lsm_store.gostore/lsm_store_env_test.gostore/lsm_store_sync_mode_benchmark_test.gostore/lsm_store_test.go
| b.ResetTimer() | ||
| for i := 0; i < b.N; i++ { | ||
| key := []byte(fmt.Sprintf("enc-bench-%010d", i)) | ||
| startTS := uint64(i) * 2 //nolint:gosec // benchmark loop index is non-negative. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
//nolintディレクティブを削除し、コードをリファクタリングしてください。
コーディングガイドラインに従い、Goのコードでは//nolintを使用せず、リファクタリングで対処する必要があります。同一ファイル内の BenchmarkApplyMutationsRaft_SyncMode で行われているように、負の境界チェックを明示的に行うことで gosec の警告を回避できます。
As per coding guidelines: **/*.go: Avoid using //nolint directives; refactor instead in Go code.
♻️ 提案する修正
- startTS := uint64(i) * 2 //nolint:gosec // benchmark loop index is non-negative.
+ if i < 0 {
+ b.Fatalf("unexpected negative iteration counter: %d", i)
+ }
+ startTS := uint64(i) * 2📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| startTS := uint64(i) * 2 //nolint:gosec // benchmark loop index is non-negative. | |
| if i < 0 { | |
| b.Fatalf("unexpected negative iteration counter: %d", i) | |
| } | |
| startTS := uint64(i) * 2 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@store/lsm_store_sync_mode_benchmark_test.go` at line 113, Remove the //nolint
directive from the startTS calculation in the benchmark and add an explicit
negative-boundary check for the loop index, following the pattern used by
BenchmarkApplyMutationsRaft_SyncMode to satisfy gosec without suppressing the
warning.
Source: Coding guidelines
|
Addressed the current-head compression findings in signed head
@codex review |
|
✅ Action performedReview finished.
|
|
Addressed the current-head benchmark finding in signed head Evidence: benchmark executes both cleartext/encrypted subcases; @codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Risk
The storage envelope gains a second valid flag value. Existing flag-zero envelopes remain readable. Unknown flag bits and nonzero Raft flags fail closed. Compression runs only after the storage-envelope cutover gate selects encrypted writes.
Caller audit
encryptForKey: all callers (PutAt,ExpireAt, direct and RaftApplyMutations) propagate errors and share the same compression policy.decryptForKey:GetAt, forward/reverse scans,ExistsAt, and delete-prefix visibility checks propagate integrity/decompression errors.DecodeEnvelope: storage propagates decode errors; Raft now additionally rejects every nonzero flag.Verification
go test ./... -count=1 -timeout=20mgo test -race ./store -run 'TestEncryption_|TestCompressionRoundTripProperty|TestPebbleCompressionPolicy' -count=1 -timeout=15m\n-go test . -run 'Test.*Encryption|Test.*Sidecar|Test.*Snapshot' -count=1 -timeout=10m\n-go test ./adapter -run TestEncryptionAdmin -count=1 -timeout=10m\n-golangci-lint --allow-parallel-runners --config=.golangci.yaml run ./internal/encryption ./store --timeout=5m\n- paired 1 KiB benchmark, fixed 1000 iterations x 10: cleartext median ~119.7 us/op; encrypted median ~113.9 us/op\n-git diff --check\n- signed commit verified forbootjp <contact@bootjp.me>\n\n## Author\n\nbootjpSummary by CodeRabbit
新機能
バグ修正
ドキュメント