feat: enhance security of secrets management system

- 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

Author: AshishKumar4

docs/llm.md

@@ -3078,10 +3078,10 @@ async performKeyRotation() {
 
 **Location:** `/test/worker/services/secrets/`
 
-Comprehensive test suite with 90 tests:
-- Unit tests for KeyDerivation (17 tests)
-- Unit tests for EncryptionService (18 tests)
-- E2E tests for UserSecretsStore (55 tests)
+Comprehensive test suite with **90+ tests** (3 test files):
+- **KeyDerivation.test.ts** - 17 unit tests for key derivation
+- **EncryptionService.test.ts** - 18 unit tests for encryption/decryption
+- **UserSecretsStore.test.ts** - 55+ E2E tests for full DO lifecycle
 
 **Run tests:**
 ```bash

worker/services/secrets/EncryptionService.ts

@@ -29,6 +29,15 @@ export class EncryptionService {
     /**
      * Encrypt a secret value using XChaCha20-Poly1305
      * Returns encrypted data with nonce and salt for decryption
+     * 
+     * Security Note: Memory Zeroing Limitation
+     * - DEK (Uint8Array) is properly zeroed after use
+     * - Plaintext string CANNOT be zeroed (JavaScript strings are immutable)
+     * - This is a fundamental JavaScript limitation - no equivalent to C's memset(0)
+     * - Plaintext remains in heap until garbage collection
+     * - Cloudflare Workers have short lifetimes, reducing exposure window
+     * - Key material (DEK, UMK) IS properly zeroed - most critical assets protected
+     * - WASM could solve this but adds significant complexity
      */
     async encrypt(value: string): Promise<EncryptedSecretData> {
         // Generate random salt and derive DEK
@@ -43,8 +52,10 @@ export class EncryptionService {
 
         const keyPreview = this.createKeyPreview(value);
 
-        // Zero out DEK from memory (security best practice)
+        // Zero out DEK and plaintext bytes from memory (security best practice)
+        // Note: Original 'value' string cannot be zeroed (JS limitation documented above)
         dek.fill(0);
+        plaintext.fill(0);
 
         return {
             encryptedValue: ciphertext,
@@ -70,10 +81,14 @@ export class EncryptionService {
             const cipher = xchacha20poly1305(dek, encrypted.nonce);
             const plaintext = cipher.decrypt(encrypted.encryptedValue);
 
-            // Zero out DEK from memory (security best practice)
+            // Decode to string before zeroing bytes
+            const result = new TextDecoder().decode(plaintext);
+            
+            // Zero out DEK and plaintext bytes from memory (security best practice)
             dek.fill(0);
+            plaintext.fill(0);
 
-            return new TextDecoder().decode(plaintext);
+            return result;
         } catch (error) {
             throw new Error('Failed to decrypt secret: invalid data or tampering detected');
         }

worker/services/secrets/KeyDerivation.ts

@@ -35,6 +35,13 @@ export class KeyDerivation {
      * UMK = PBKDF2(MEK, salt=userId, 100k iterations)
      * 
      * This creates a unique key per user while only storing one master key
+     * 
+     * Security Note: Deterministic Salt Design
+     * - userId is used as salt for DETERMINISTIC derivation (same userId → same UMK)
+     * - This is REQUIRED for Durable Object architecture (DO must derive same key on each invocation)
+     * - Random salt would require persistent storage, defeating hierarchical key design
+     * - If MEK is compromised, attacker can decrypt everything regardless (MEK compromise = total failure)
+     * - Context prefix provides domain separation: vibesdk:user:{userId}
      */
     async deriveUserMasterKey(userId: string): Promise<Uint8Array> {
         if (!userId || userId.trim().length === 0) {
@@ -43,7 +50,7 @@ export class KeyDerivation {
 
         const encoder = new TextEncoder();
 
-        // Use userId as salt with context prefix
+        // Use userId as salt with context prefix for deterministic derivation
         const salt = encoder.encode(`${CRYPTO_CONSTANTS.UMK_CONTEXT_PREFIX}${userId}`);
 
         // Import master key as key material

worker/services/secrets/UserSecretsStore.ts

@@ -1,10 +1,16 @@
 /**
- * UserSecretsStore - Rebuilt from scratch using proven MinimalSecretsStore pattern
+ * UserSecretsStore - Durable Object for secure user API key storage
  * 
  * Architecture:
  * - One DO per user (userId as DO ID)
  * - Hierarchical key derivation: MEK → UMK → DEK
- * - XChaCha20-Poly1305 encryption
+ * - XChaCha20-Poly1305 AEAD encryption
+ * 
+ * Key Rotation Locking:
+ * - Promise-based lock prevents concurrent modifications during rotation
+ * - All RPC methods call waitForRotation() first
+ * - rotationInProgress promise cleared in finally block
+ * - DO single-threading + promise await = complete mutual exclusion
  */
 
 import { DurableObject } from 'cloudflare:workers';
@@ -43,13 +49,17 @@ export class UserSecretsStore extends DurableObject<Env> {
     private userId: string;
     private keyDerivation!: KeyDerivation;
     private encryption!: EncryptionService;
+    private rotationInProgress: Promise<void> | null = null;
 
     constructor(ctx: DurableObjectState, env: Env) {
         super(ctx, env);
 
         this.userId = ctx.id.name ?? ctx.id.toString();
 
         // Use blockConcurrencyWhile for initialization
+        // Security Note: This guarantees no RPC methods execute until initialize() completes.
+        // Cloudflare Workers DO platform blocks ALL incoming requests during this call.
+        // No additional await checks needed in methods - this is the official pattern.
         ctx.blockConcurrencyWhile(async () => {
             await this.initialize();
         });
@@ -150,7 +160,19 @@ export class UserSecretsStore extends DurableObject<Env> {
         return !!(this.encryption && this.keyDerivation);
     }
 
+    /**
+     * Wait for any in-progress key rotation to complete
+     * All RPC methods should call this first to ensure data consistency
+     */
+    private async waitForRotation(): Promise<void> {
+        if (this.rotationInProgress) {
+            await this.rotationInProgress;
+        }
+    }
+    
     async listSecrets(): Promise<SecretMetadata[]> {
+        await this.waitForRotation();
+        
         const result = this.ctx.storage.sql.exec(`
             SELECT 
                 id, name, secret_type, provider, key_preview, metadata,
@@ -166,6 +188,8 @@ export class UserSecretsStore extends DurableObject<Env> {
     }
 
     async storeSecret(request: StoreSecretRequest): Promise<SecretMetadata | null> {
+        await this.waitForRotation();
+        
         // Validate request - returns null on validation failure
         const validationError = this.validateSecretRequest(request);
         if (validationError) {
@@ -219,6 +243,8 @@ export class UserSecretsStore extends DurableObject<Env> {
     }
 
     async getSecretValue(secretId: string): Promise<SecretWithValue | null> {
+        await this.waitForRotation();
+        
         const result = this.ctx.storage.sql.exec(`
             SELECT * FROM secrets WHERE id = ? AND is_active = 1
         `, secretId);
@@ -259,6 +285,8 @@ export class UserSecretsStore extends DurableObject<Env> {
     }
 
     async updateSecret(secretId: string, request: UpdateSecretRequest): Promise<SecretMetadata | null> {
+        await this.waitForRotation();
+        
         // Check exists
         const result = this.ctx.storage.sql.exec(`
             SELECT * FROM secrets WHERE id = ? AND is_active = 1
@@ -312,6 +340,12 @@ export class UserSecretsStore extends DurableObject<Env> {
         updateValues.push(now);
         updateValues.push(secretId);
 
+        // Security Note: SQL Injection Safety
+        // This dynamic query construction is SAFE because:
+        // 1. Field names ("name = ?", "metadata = ?") are hardcoded string literals
+        // 2. User input goes into parameterized ? placeholders (updateValues array)
+        // 3. Array.join() only combines our hardcoded field assignments
+        // 4. This is the standard parameterized query pattern for dynamic updates
         this.ctx.storage.sql.exec(`
             UPDATE secrets SET ${updateFields.join(', ')} WHERE id = ?
         `, ...updateValues);
@@ -326,6 +360,8 @@ export class UserSecretsStore extends DurableObject<Env> {
     }
 
     async deleteSecret(secretId: string): Promise<boolean> {
+        await this.waitForRotation();
+        
         // Soft delete
         const result = this.ctx.storage.sql.exec(`
             UPDATE secrets SET is_active = 0, updated_at = ? 
@@ -479,81 +515,103 @@ export class UserSecretsStore extends DurableObject<Env> {
 
     /**
      * Re-encrypt all active secrets with new master key
-     * Pre-computes all encrypted values, then applies SQL updates atomically
+     * 
+     * Locking Strategy:
+     * - Sets rotationInProgress promise at start
+     * - All RPC methods await this promise via waitForRotation()
+     * - Ensures no operations can modify secrets during rotation
+     * - Promise cleared in finally block (even on errors)
+     * - DO single-threading + promise-based locking = complete protection
      */
     private async performKeyRotation(newKeyFingerprint: string): Promise<number> {
-        const result = this.ctx.storage.sql.exec(`
-            SELECT * FROM secrets WHERE is_active = 1
-        `);
-        
-        const secrets = result.toArray();
-        const now = Date.now();
-        
-        // Phase 1: Pre-compute all encrypted values (async operations)
-        const reencryptedSecrets: Array<{
-            id: string;
-            encryptedValue: Uint8Array;
-            nonce: Uint8Array;
-            salt: Uint8Array;
-        }> = [];
-        
-        for (const row of secrets) {
-            const secretRow = row as Record<string, SqlStorageValue>;
-            
+        // Create and store the rotation promise for locking
+        const rotationPromise = (async (): Promise<number> => {
             try {
-                const encrypted = await this.getEncryptedData(secretRow);
-                const plaintext = await this.encryption.decrypt(encrypted);
-                const reencrypted = await this.encryption.encrypt(plaintext);
+                const result = this.ctx.storage.sql.exec(`
+                    SELECT * FROM secrets WHERE is_active = 1
+                `);
+                
+                const secrets = result.toArray();
+                const now = Date.now();
+                
+                // Phase 1: Pre-compute all encrypted values (async crypto operations)
+                const reencryptedSecrets: Array<{
+                    id: string;
+                    encryptedValue: Uint8Array;
+                    nonce: Uint8Array;
+                    salt: Uint8Array;
+                }> = [];
+                
+                for (const row of secrets) {
+                    const secretRow = row as Record<string, SqlStorageValue>;
+                    
+                    try {
+                        const encrypted = await this.getEncryptedData(secretRow);
+                        const plaintext = await this.encryption.decrypt(encrypted);
+                        const reencrypted = await this.encryption.encrypt(plaintext);
+                        
+                        reencryptedSecrets.push({
+                            id: String(secretRow.id),
+                            encryptedValue: reencrypted.encryptedValue,
+                            nonce: reencrypted.nonce,
+                            salt: reencrypted.salt
+                        });
+                    } catch (error) {
+                        console.error(`Failed to rotate secret ${String(secretRow.id)}:`, error);
+                    }
+                }
 
-                reencryptedSecrets.push({
-                    id: String(secretRow.id),
-                    encryptedValue: reencrypted.encryptedValue,
-                    nonce: reencrypted.nonce,
-                    salt: reencrypted.salt
+                // Phase 2: Apply all SQL updates atomically in transaction
+                const rotatedCount = this.ctx.storage.transactionSync(() => {
+                    let count = 0;
+                    
+                    for (const secret of reencryptedSecrets) {
+                        this.ctx.storage.sql.exec(`
+                            UPDATE secrets 
+                            SET encrypted_value = ?, nonce = ?, salt = ?, key_fingerprint = ?, updated_at = ?
+                            WHERE id = ?
+                        `,
+                            secret.encryptedValue,
+                            secret.nonce,
+                            secret.salt,
+                            newKeyFingerprint,
+                            now,
+                            secret.id
+                        );
+                        count++;
+                    }
+                    
+                    this.ctx.storage.sql.exec(`
+                        UPDATE key_rotation_metadata 
+                        SET current_key_fingerprint = ?, 
+                            last_rotation_at = ?, 
+                            rotation_count = rotation_count + 1
+                        WHERE id = 1
+                    `, newKeyFingerprint, now);
+                    
+                    return count;
                 });
-            } catch (error) {
-                console.error(`Failed to rotate secret ${String(secretRow.id)}:`, error);
+                
+                return rotatedCount;
+            } finally {
+                // Always clear the lock, even on errors
+                this.rotationInProgress = null;
             }
-        }
+        })();
 
-        // Phase 2: Apply all SQL updates atomically in transaction
-        const rotatedCount = this.ctx.storage.transactionSync(() => {
-            let count = 0;
-            
-            for (const secret of reencryptedSecrets) {
-                this.ctx.storage.sql.exec(`
-                    UPDATE secrets 
-                    SET encrypted_value = ?, nonce = ?, salt = ?, key_fingerprint = ?, updated_at = ?
-                    WHERE id = ?
-                `,
-                    secret.encryptedValue,
-                    secret.nonce,
-                    secret.salt,
-                    newKeyFingerprint,
-                    now,
-                    secret.id
-                );
-                count++;
-            }
-            
-            this.ctx.storage.sql.exec(`
-                UPDATE key_rotation_metadata 
-                SET current_key_fingerprint = ?, 
-                    last_rotation_at = ?, 
-                    rotation_count = rotation_count + 1
-                WHERE id = 1
-            `, newKeyFingerprint, now);
-            
-            return count;
-        });
+        // Store the promise to block other operations
+        this.rotationInProgress = rotationPromise.then(() => {});
 
-        return rotatedCount;
+        // Await and return the result
+        return await rotationPromise;
     }
 
     /**
      * Get key rotation statistics
      */
     async getKeyRotationInfo(): Promise<KeyRotationInfo> {
+        await this.waitForRotation();
+        
         const metadataResult = this.ctx.storage.sql.exec(`
             SELECT * FROM key_rotation_metadata WHERE id = 1
         `);

worker/services/secrets/constants.ts

@@ -8,8 +8,8 @@
  */
 export const CRYPTO_CONSTANTS = {
     // Key derivation
-    UMK_ITERATIONS: 100000,      // User Master Key derivation (PBKDF2)
-    DEK_ITERATIONS: 10000,       // Data Encryption Key derivation (PBKDF2)
+    UMK_ITERATIONS: 600000,      // User Master Key derivation (PBKDF2-SHA256)
+    DEK_ITERATIONS: 100000,       // Data Encryption Key derivation (PBKDF2-SHA256)
     SALT_SIZE: 16,               // bytes
     NONCE_SIZE: 24,              // bytes (XChaCha20)
     KEY_SIZE: 32,                // bytes (256 bits)