fix(secrets): make workspace key rotation atomic#1097
Conversation
|
Review changes with Changed Files
|
WalkthroughWorkspace encryption and secret updates now use transaction-scoped workspace reloads and locks. Key rotation separates cryptographic processing from database writes, returns updated workspaces, and synchronizes Redis through a shared helper. ChangesWorkspace encryption flow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Handler
participant Database
participant Encryption
participant Redis
Handler->>Database: begin transaction and lock workspace
Database-->>Handler: workspace row
Handler->>Encryption: rotate workspace key and re-encrypt secrets
Encryption->>Database: update secrets and workspace
Database-->>Handler: updated workspace and count
Handler->>Redis: cache updated workspace
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR strengthens workspace encryption key rotation and secret writes by adding explicit database row locking and refactoring workspace Redis-cache updates into a shared utility, aiming to prevent non-atomic key rotation and writes occurring with stale keys.
Changes:
- Refactors workspace Redis cache updates into
service_utils::redis::put_workspace_in_redisand updates callers accordingly. - Makes workspace key rotation more atomic by (a) row-locking the workspace record during rotation, (b) completing crypto before DB mutation, and (c) returning the updated
Workspacefrom the rotation helper for post-commit cache updates. - Ensures secret create/update operations acquire a workspace row lock and reload the workspace encryption key from the database inside the same transaction to avoid encrypting with an old key while rotation is in flight.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| crates/superposition/src/workspace/handlers.rs | Switches workspace cache updates to shared Redis helper and updates workspace key rotation flow to use atomic helper return value. |
| crates/service_utils/src/redis.rs | Introduces shared put_workspace_in_redis helper for best-effort workspace cache refresh. |
| crates/service_utils/src/encryption.rs | Reworks workspace key rotation to lock the workspace row, precompute crypto before DB writes, and return the updated workspace; adds unit tests for re-encryption helper. |
| crates/context_aware_config/src/api/secrets/handlers.rs | Wraps secret create/update in transactions that acquire a workspace row lock and reload the workspace key from DB; updates master-key rotation to refresh Redis cache post-commit. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/service_utils/src/redis.rs`:
- Around line 28-60: Update put_workspace_in_redis so concurrent cache writes
are monotonic: atomically compare the snapshot’s last_modified_at with the
cached version and only replace the entry when it is newer, preventing an older
workspace snapshot from overwriting a newer one. If the Redis helpers cannot
support this conditional write, invalidate the workspace key instead of
performing an unconditional stale overwrite.
🪄 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: 851fd65f-c3d0-4a61-b141-f28ec4771fc5
📒 Files selected for processing (4)
crates/context_aware_config/src/api/secrets/handlers.rscrates/service_utils/src/encryption.rscrates/service_utils/src/redis.rscrates/superposition/src/workspace/handlers.rs
| /// Best-effort update of the canonical workspace cache entry. | ||
| pub async fn put_workspace_in_redis( | ||
| workspace: &Workspace, | ||
| redis_pool: &Option<RedisPool>, | ||
| ) { | ||
| let Some(pool) = redis_pool else { | ||
| log::debug!("Redis not configured, skipping workspace cache update"); | ||
| return; | ||
| }; | ||
|
|
||
| let serialized = match serde_json::to_string(workspace) { | ||
| Ok(serialized) => serialized, | ||
| Err(error) => { | ||
| log::error!("Failed to serialize workspace for Redis: {}", error); | ||
| return; | ||
| } | ||
| }; | ||
| let key_ttl: i64 = get_from_env_or_default("REDIS_KEY_TTL", 604800); | ||
|
|
||
| if let Err(error) = redis_set_data( | ||
| pool, | ||
| workspace.workspace_schema_name.clone(), | ||
| serialized, | ||
| Some(Expiration::EX(key_ttl)), | ||
| ) | ||
| .await | ||
| { | ||
| log::warn!( | ||
| "Failed to update Redis cache for workspace '{}': {}", | ||
| workspace.workspace_schema_name, | ||
| error | ||
| ); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make workspace cache updates monotonic.
Concurrent mutations can commit in order W1, W2 but complete these unconditional Redis writes as W2, W1, leaving the older encryption key cached. Use an atomic version check based on last_modified_at, or invalidate the entry rather than overwriting it with a potentially stale snapshot.
🤖 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 `@crates/service_utils/src/redis.rs` around lines 28 - 60, Update
put_workspace_in_redis so concurrent cache writes are monotonic: atomically
compare the snapshot’s last_modified_at with the cached version and only replace
the entry when it is newer, preventing an older workspace snapshot from
overwriting a newer one. If the Redis helpers cannot support this conditional
write, invalidate the workspace key instead of performing an unconditional stale
overwrite.
Problem
Describe the problem you are trying to solve here
Solution
Provide a brief summary of your solution so that reviewers can understand your code
Environment variable changes
What ENVs need to be added or changed
Pre-deployment activity
Things needed to be done before deploying this change (if any)
Post-deployment activity
Things needed to be done after deploying this change (if any)
API changes
Possible Issues in the future
Describe any possible issues that could occur because of this change
Summary by CodeRabbit