Skip to content

Conversation

@AshishKumar4
Copy link
Collaborator

@AshishKumar4 AshishKumar4 commented Nov 21, 2025

Summary

This PR introduces a production-ready user-level secrets management system built on Cloudflare Durable Objects, enabling secure storage of API keys and sensitive credentials with encryption, key rotation support, and comprehensive audit trails.

Changes

Core Infrastructure

  • UserSecretsStore Durable Object (worker/services/secrets/UserSecretsStore.ts) - One DO per user architecture with SQLite backend
  • Hierarchical key derivation (worker/services/secrets/KeyDerivation.ts) - MEK → UMK → DEK chain using PBKDF2
  • XChaCha20-Poly1305 encryption (worker/services/secrets/EncryptionService.ts) - AEAD encryption with authenticated data integrity

Security Features

  • Per-secret encryption with unique DEK and random nonces
  • Key rotation with automatic re-encryption of all active secrets
  • Soft deletion with 90-day retention
  • Access tracking (last accessed timestamp, access count)
  • Expiration support with automatic cleanup via DO alarms

API Endpoints

  • GET /api/user-secrets - List all secrets (metadata only)
  • POST /api/user-secrets - Store new secret
  • GET /api/user-secrets/:id/value - Retrieve decrypted secret value
  • PATCH /api/user-secrets/:id - Update secret
  • DELETE /api/user-secrets/:id - Soft delete secret

Configuration

  • Added UserSecretsStore DO binding to wrangler.jsonc
  • New migration tag v3 for DO SQLite schema
  • Added SECRETS_ENCRYPTION_KEY environment variable requirement
  • Test configuration in wrangler.test.jsonc for isolated testing

Testing

  • 90+ comprehensive tests across encryption, key derivation, and DO operations
  • Full test coverage of key rotation, error handling, and edge cases
  • Test infrastructure setup with dedicated vitest configuration

Documentation

  • Updated CLAUDE.md with User Secrets Store architecture overview
  • Added detailed inline security notes and design rationale

Motivation

VibeSDK needs secure storage for user API keys (OpenAI, Anthropic, Google AI) without exposing them in plaintext or storing them in the main database. This implementation provides:

  1. Cryptographic isolation - Each user's secrets encrypted with unique derived keys
  2. Key rotation capability - Infrastructure supports master key rotation without data loss
  3. Audit trail - Access tracking for security monitoring
  4. Automatic cleanup - Expired secrets removed via DO alarms

Testing

Manual Testing

  1. Deploy with SECRETS_ENCRYPTION_KEY environment variable set to 64-char hex string
  2. Use API endpoints to store/retrieve/update/delete secrets
  3. Verify encryption by inspecting DO SQLite storage (should see binary ciphertext)
  4. Test key rotation by changing SECRETS_ENCRYPTION_KEY and verifying secrets remain accessible

Automated Testing

Run: bun run test

All 90+ tests pass, covering:

  • Encryption/decryption roundtrip
  • Key derivation correctness
  • DO RPC methods
  • Key rotation logic
  • Error handling and edge cases

Breaking Changes

None. This is a new feature with no impact on existing functionality.


This PR description was automatically generated by Claude Code

- Added UserSecretsStore durable object for secure secret storage and management
- Implemented XChaCha20-Poly1305 encryption service for secure secret storage
- Heirarchial key derivation
- Increased PBKDF2 iterations to 600k/100k for stronger key derivation
- Added memory zeroing for plaintext bytes in encryption/decryption operations
- Implemented promise-based locking for key rotation to prevent concurrent modifications
- Added detailed security documentation for deterministic salt design and SQL injection prevention
- Updated UserSecretsStore to wait for any in-progress key rotation before executing operations
- Added comprehensive
@github-actions github-actions bot added documentation Improvements or additions to documentation dependencies Pull requests that update a dependency file ci/cd worker size/XL labels Dec 5, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Dec 5, 2025

Claude finished @AshishKumar4's task —— View job


Code & Security Review (Critical Path)

Recommendation: APPROVE WITH MINOR SUGGESTIONS

I've completed a comprehensive security-focused review of this user secrets management implementation. This is high-quality, production-ready code with strong cryptographic foundations and excellent test coverage.


Security Analysis

Strengths

Encryption Architecture

  • XChaCha20-Poly1305 AEAD provides both confidentiality and integrity
  • Proper use of @noble/ciphers library (well-audited cryptographic implementation)
  • Unique DEK per secret with random salt prevents key reuse
  • 24-byte nonces (XChaCha20 standard) eliminate collision risk
  • Authentication tag verification prevents tampering

Key Derivation

  • Hierarchical design (MEK → UMK → DEK) provides proper isolation
  • PBKDF2 with 600k iterations for UMK (exceeds OWASP 2023 recommendation of 600k)
  • PBKDF2 with 100k iterations for DEK (appropriate for already-strong UMK)
  • Proper key material zeroing after use (dek.fill(0), plaintext.fill(0))
  • Master key validation (64 hex chars = 256 bits)

Durable Object Design

  • One DO per user prevents cross-user data leakage
  • blockConcurrencyWhile ensures initialization completes before any RPC calls
  • Promise-based rotation locking prevents race conditions during key rotation
  • Proper use of transactionSync for atomic multi-row updates
  • SQLite indexes optimize query performance

API Security

  • All endpoints require authentication (AuthConfig.authenticated)
  • User context properly isolated (getUserSecretsStub uses user.id from auth context)
  • Input validation with size limits (50KB per secret, 10KB metadata)
  • Returns null on validation failures (no exception information leakage)
  • Proper HTTP status codes (400 for validation, 404 for not found, 500 for errors)

Operational Security

  • Soft deletion with 90-day retention for audit/recovery
  • Automatic expired secret cleanup via DO alarms
  • Access tracking (last_accessed, access_count) for security monitoring
  • Key rotation support with automatic re-encryption
  • Expiration checking before decryption (prevents timing attacks)

Minor Concerns & Suggestions

1. Deterministic Salt for UMK Derivation

Location: KeyDerivation.ts lines 46-54

Issue: The userId is used as a deterministic salt for deriving the User Master Key. While this is documented as required for the DO architecture, it means:

  • Same userId always produces same UMK (by design)
  • If MEK is compromised, all user secrets are compromised
  • No additional entropy from random salt

Assessment: This is an acceptable architectural trade-off given the DO constraints, but it's worth noting:

  • The security model relies entirely on MEK confidentiality
  • Consider documenting MEK rotation procedures
  • MEK should be stored in Cloudflare Workers Secrets (not in wrangler.jsonc vars)

Recommendation: Add a section to documentation about MEK management best practices:

  • How to securely generate MEK (crypto.randomBytes or similar)
  • Where to store MEK (Workers Secrets, not version control)
  • How to perform MEK rotation (requires coordinated re-encryption)

2. Memory Zeroing Limitation

Location: EncryptionService.ts lines 33-40

Issue: The code correctly notes that JavaScript strings are immutable and cannot be zeroed from memory. The plaintext secret values remain in heap until garbage collected.

Assessment: This is a fundamental JavaScript limitation, not a code flaw. The comment correctly documents this. Mitigation:

  • Workers have short lifetimes, reducing exposure window
  • Key material (DEK, UMK) IS properly zeroed (most critical)
  • WASM implementation could address this but adds complexity

Recommendation: No code changes needed. The inline comment is excellent. Consider adding to documentation that this is a known limitation.

3. SQL Injection Safety (Dynamic Query)

Location: UserSecretsStore.ts lines 343-351

Assessment: The dynamic SQL query construction for updates is SAFE:

UPDATE secrets SET ${updateFields.join(', ')} WHERE id = ?
  • Field names are hardcoded string literals ("name = ?", "metadata = ?")
  • User input only goes into parameterized placeholders
  • Array.join only combines hardcoded assignments

The inline comment (lines 343-348) correctly documents this safety. Well done.

4. Error Message Information Disclosure

Location: UserSecretsStore.ts throughout

Assessment: Good job returning null instead of throwing exceptions on validation failures. This prevents leaking implementation details. However, some exceptions are still thrown:

  • Line 336: throw new Error('No fields to update')
  • Line 621: throw new Error('Key rotation metadata not initialized')

Recommendation: Consider returning null for these cases too, or ensure they're caught and logged without exposing details to the client.

5. Key Rotation Race Condition Protection

Location: UserSecretsStore.ts lines 526-607

Assessment: Excellent implementation:

  • rotationInProgress promise acts as a lock
  • All RPC methods call waitForRotation() first
  • finally block ensures lock is always released
  • DO single-threading + promise await = complete mutual exclusion

Well-designed concurrency control.


Code Quality

Excellent:

  • Comprehensive inline documentation
  • Clear separation of concerns (KeyDerivation, EncryptionService, UserSecretsStore)
  • DRY principle followed (rowToMetadata helper, getEncryptedData helper)
  • Strong TypeScript typing (no any types found)
  • Proper error handling throughout
  • 90+ tests with excellent coverage

Minor Issues:

  • No issues found

Testing

Coverage: Excellent

  • 90+ comprehensive tests across all components
  • Tests cover: encryption roundtrip, key derivation, DO RPC methods, key rotation, error cases
  • Test files: EncryptionService.test.ts, KeyDerivation.test.ts, UserSecretsStore.test.ts
  • Dedicated test configuration (wrangler.test.jsonc, vitest.config.ts)

Recommendation: All tests should pass before merge. Run bun run test to verify.


Architecture Alignment

Durable Object Patterns: Perfect implementation following vibesdk conventions:

  • SQLite backend for persistent state
  • blockConcurrencyWhile for initialization
  • Proper RPC method signatures
  • DO binding registered in wrangler.jsonc with migration tag

API Patterns: Follows existing controller/route patterns:

  • Controller extends BaseController
  • Routes use adaptController helper
  • Auth middleware properly applied
  • Consistent error response format

Documentation: CLAUDE.md properly updated with architecture overview.


Summary

This is a well-designed, secure implementation of user secrets management. The cryptographic choices are sound, the DO architecture is proper, and the code quality is high. The 90+ tests demonstrate thorough validation.

Key Strengths:

  • Strong encryption (XChaCha20-Poly1305 AEAD)
  • Proper key derivation hierarchy
  • Excellent concurrency control
  • Good operational features (soft delete, expiration, rotation)
  • Comprehensive test coverage

Minor Suggestions:

  1. Add MEK management documentation
  2. Ensure MEK stored in Workers Secrets (not vars)
  3. Consider returning null instead of throwing for edge cases

Approval: This code is ready to merge after ensuring SECRETS_ENCRYPTION_KEY is properly configured in production environment.


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

Labels

ci/cd dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation size/XL worker

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant