Repository navigation
feat(cache): authenticate remote bytecode-cache entries - #4206
antoniosarosi wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe bytecode cache now supports optional HMAC-SHA256 authentication for remote entries. Signing and verification keys are read from environment variables, authenticated envelopes are handled during remote reads and writes, and tests cover valid, tampered, required-signature, and unsigned compatibility cases. ChangesRemote cache authentication
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BytecodeCache
participant RemoteCache
participant RemoteStore
BytecodeCache->>RemoteCache: request shared entry
RemoteCache->>RemoteStore: fetch remote payload
RemoteStore-->>RemoteCache: signed or unsigned bytes
RemoteCache->>RemoteCache: verify and unwrap when configured
RemoteCache-->>BytecodeCache: cache entry bytes
BytecodeCache->>BytecodeCache: validate and deserialize
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
baml_language/crates/bex_cache/src/lib.rs (1)
1592-1655: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the untested
(None, true)branch.
authenticate_download's doc comment explicitly calls out tolerating signed uploads when no verification key is configured, but none of the four new tests exercise a signed envelope read by a reader withverification_key: None. The three covered branches (Some,true/Some,false/None,false) leave this documented compatibility path unverified.🧪 Suggested additional test
#[test] fn signed_remote_entry_is_tolerated_without_verification_key() { const AUTH_KEY: [u8; 32] = [0xd8; 32]; let (url, _) = spawn_http_store(); let program = Program::default(); let files = vec![("mixed.baml".to_string(), "fn mixed {}")]; let key = compute_key(&KeyInputs { compiler_fingerprint: [15u8; 32], opt_level: 2, emit_test_cases: false, manifest: None, files: &files, }); let writer_dir = tempfile::tempdir().expect("writer tempdir"); let mut writer = BytecodeCache::open(writer_dir.path().to_path_buf()); writer.remote = Some(remote_for_with_keys(&url, Some(AUTH_KEY), None)); writer.store_shared(&key, &program).expect("signed store"); let reader_dir = tempfile::tempdir().expect("reader tempdir"); let mut reader = BytecodeCache::open(reader_dir.path().to_path_buf()); reader.remote = Some(remote_for(&url)); assert!( reader.load_shared(&key).is_some(), "signed envelope is tolerated when no verification key is configured" ); }🤖 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 `@baml_language/crates/bex_cache/src/lib.rs` around lines 1592 - 1655, Add a test alongside the existing remote cache tests covering a signed upload read without a verification key. Use remote_for_with_keys with an auth key for the writer, remote_for for the reader, store a shared entry, and assert load_shared succeeds to verify the documented (None, true) compatibility branch.
🤖 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.
Nitpick comments:
In `@baml_language/crates/bex_cache/src/lib.rs`:
- Around line 1592-1655: Add a test alongside the existing remote cache tests
covering a signed upload read without a verification key. Use
remote_for_with_keys with an auth key for the writer, remote_for for the reader,
store a shared entry, and assert load_shared succeeds to verify the documented
(None, true) compatibility branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a87474b0-10f4-452d-a55d-e1045b85c0b3
⛔ Files ignored due to path filters (1)
baml_language/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
baml_language/crates/bex_cache/Cargo.tomlbaml_language/crates/bex_cache/src/lib.rs
Binary size checks passed✅ 7 passed
Generated by |
|
Closing — this was an orchestration experiment run; not landing. |
Summary
Root cause
The existing payload SHA-256 was stored alongside attacker-controlled bytes, so a malicious remote could replace both the executable bytecode and its checksum. A keyed MAC known only to writers and readers prevents the blob-serving origin from forging a replacement.
Validation
cargo fmt --allcargo test -p bex_cachecargo clippy -p bex_cache --all-targets -- -D warningsFixes B-747
Summary by CodeRabbit
New Features
Bug Fixes