dennii/Termix
1
0
Fork 0
Code Issues Pull Requests 2 Actions Packages Projects Releases 22 Wiki Activity

Fix: OIDC users losing credentials after re-login due to non-persistent encryption keys #363

Merged
thorved merged 2 commits from fix/oidc-credential-persistence into dev-1.7.2 2025-10-06 14:19:22 +00:00
Conversation 5 Commits 2 Files Changed 1 +37 -9
1 changed files with 37 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit d73d2cf3a2 - Show all commits

46
src/backend/utils/user-crypto.ts
View File

@@ -70,7 +70,34 @@ class UserCrypto {
}
gemini-code-assist[bot] commented 2025-10-06 06:42:56 +00:00 (Migrated from github.com)
Review
Copy Link

critical

This new implementation introduces a critical race condition and contains redundant logic.

1. Critical Race Condition (Data Loss):
If two concurrent login requests for the same new OIDC user arrive, both can enter the else block to create a new DEK. The storeEncryptedDEK function performs a non-atomic select-then-update/insert, which can cause one request's DEK to overwrite the other's in the database. If a user's session uses a DEK that is different from the one stored, any data they save will be unrecoverable on their next login. This is a critical data loss scenario.

2. Redundant kekSalt Logic:
The kekSalt is generated, stored, and checked, but it's not used for OIDC key derivation (deriveOIDCSystemKey uses the userId as the salt). This adds unnecessary complexity and database writes.

Suggested Fix:
The following suggestion addresses both issues:

  • It removes the unnecessary kekSalt handling.
  • It mitigates the race condition by re-reading the DEK from the database after storing it. This "write-then-read-and-verify" check ensures the session uses the correct, persisted DEK, guaranteeing data consistency.
  async setupOIDCUserEncryption(userId: string): Promise<void> {
    const existingEncryptedDEK = await this.getEncryptedDEK(userId);

    let DEK: Buffer;

    if (existingEncryptedDEK) {
      // User already has a persisted DEK, retrieve it
      const systemKey = this.deriveOIDCSystemKey(userId);
      DEK = this.decryptDEK(existingEncryptedDEK, systemKey);
      systemKey.fill(0);
    } else {
      // First time setup - create and persist new DEK
      DEK = crypto.randomBytes(UserCrypto.DEK_LENGTH);

      const systemKey = this.deriveOIDCSystemKey(userId);
      try {
        const encryptedDEK = this.encryptDEK(DEK, systemKey);
        await this.storeEncryptedDEK(userId, encryptedDEK);

        // MITIGATION: Read back the stored DEK to ensure we use the one that won the race.
        const storedEncryptedDEK = await this.getEncryptedDEK(userId);
        if (storedEncryptedDEK && storedEncryptedDEK.data !== encryptedDEK.data) {
          // We lost the race. Use the DEK from the database.
          DEK.fill(0); // Discard our generated DEK.
          DEK = this.decryptDEK(storedEncryptedDEK, systemKey);
        } else if (!storedEncryptedDEK) {
          // This is an unexpected state; the store operation should have worked.
          throw new Error("Failed to store and retrieve user encryption key.");
        }
      } finally {
        systemKey.fill(0);
      }
    }
![critical](https://www.gstatic.com/codereviewagent/critical.svg) This new implementation introduces a critical race condition and contains redundant logic. **1. Critical Race Condition (Data Loss):** If two concurrent login requests for the same new OIDC user arrive, both can enter the `else` block to create a new DEK. The `storeEncryptedDEK` function performs a non-atomic `select-then-update/insert`, which can cause one request's DEK to overwrite the other's in the database. If a user's session uses a DEK that is different from the one stored, any data they save will be unrecoverable on their next login. This is a critical data loss scenario. **2. Redundant `kekSalt` Logic:** The `kekSalt` is generated, stored, and checked, but it's not used for OIDC key derivation (`deriveOIDCSystemKey` uses the `userId` as the salt). This adds unnecessary complexity and database writes. **Suggested Fix:** The following suggestion addresses both issues: * It removes the unnecessary `kekSalt` handling. * It mitigates the race condition by re-reading the DEK from the database after storing it. This "write-then-read-and-verify" check ensures the session uses the correct, persisted DEK, guaranteeing data consistency. ```typescript async setupOIDCUserEncryption(userId: string): Promise<void> { const existingEncryptedDEK = await this.getEncryptedDEK(userId); let DEK: Buffer; if (existingEncryptedDEK) { // User already has a persisted DEK, retrieve it const systemKey = this.deriveOIDCSystemKey(userId); DEK = this.decryptDEK(existingEncryptedDEK, systemKey); systemKey.fill(0); } else { // First time setup - create and persist new DEK DEK = crypto.randomBytes(UserCrypto.DEK_LENGTH); const systemKey = this.deriveOIDCSystemKey(userId); try { const encryptedDEK = this.encryptDEK(DEK, systemKey); await this.storeEncryptedDEK(userId, encryptedDEK); // MITIGATION: Read back the stored DEK to ensure we use the one that won the race. const storedEncryptedDEK = await this.getEncryptedDEK(userId); if (storedEncryptedDEK && storedEncryptedDEK.data !== encryptedDEK.data) { // We lost the race. Use the DEK from the database. DEK.fill(0); // Discard our generated DEK. DEK = this.decryptDEK(storedEncryptedDEK, systemKey); } else if (!storedEncryptedDEK) { // This is an unexpected state; the store operation should have worked. throw new Error("Failed to store and retrieve user encryption key."); } } finally { systemKey.fill(0); } } ```
thorved commented 2025-10-06 07:01:22 +00:00 (Migrated from github.com)
Review
Copy Link

✅ Changes Made

1. Race Condition Mitigation (Critical Fix)

Problem Identified

You're right - the original implementation had a classic check-then-act race condition. Two concurrent login requests could both generate different DEKs, and whichever stored last would win, leaving the other session with a mismatched key.

Race Condition Scenario:

Time  | Request A (User Login)         | Request B (User Login - Same User)
------+-------------------------------+------------------------------------
T1    | Check: No DEK exists          |
T2    | Generate Random DEK_A         |
T3    |                               | Check: No DEK exists (still!)
T4    |                               | Generate Random DEK_B (DIFFERENT!)
T5    | Store DEK_A in database       |
T6    |                               | Store DEK_B (OVERWRITES!)
T7    | Session A uses DEK_A          |
T8    |                               | Session B uses DEK_B
T9    | User creates credential       |
T10   | Credential encrypted with DEK_A|
T11   | Database now has DEK_B        |
------+-------------------------------+------------------------------------
Result: Next login uses DEK_B, but credential was encrypted with DEK_A
        ❌ DATA LOSS!

Solution Implemented

Added a "write-then-read-and-verify" pattern in setupOIDCUserEncryption():

async setupOIDCUserEncryption(userId: string): Promise<void> {
  const existingEncryptedDEK = await this.getEncryptedDEK(userId);

  let DEK: Buffer;

  if (existingEncryptedDEK) {
    // User already has a persisted DEK, retrieve it
    const systemKey = this.deriveOIDCSystemKey(userId);
    DEK = this.decryptDEK(existingEncryptedDEK, systemKey);
    systemKey.fill(0);
  } else {
    // First time setup - create and persist new DEK
    DEK = crypto.randomBytes(UserCrypto.DEK_LENGTH);
    const systemKey = this.deriveOIDCSystemKey(userId);

    try {
      const encryptedDEK = this.encryptDEK(DEK, systemKey);
      await this.storeEncryptedDEK(userId, encryptedDEK);

      // ✅ MITIGATION: Read back the stored DEK to ensure we use the one that won the race
      const storedEncryptedDEK = await this.getEncryptedDEK(userId);
      if (
        storedEncryptedDEK &&
        storedEncryptedDEK.data !== encryptedDEK.data
      ) {
        // We lost the race. Use the DEK from the database.
        DEK.fill(0); // Discard our generated DEK
        DEK = this.decryptDEK(storedEncryptedDEK, systemKey);
      } else if (!storedEncryptedDEK) {
        // This is an unexpected state; the store operation should have worked.
        throw new Error(
          "Failed to store and retrieve user encryption key.",
        );
      }
    } finally {
      systemKey.fill(0); // Always clean up sensitive material
    }
  }

  // Load verified DEK into session
  const now = Date.now();
  this.userSessions.set(userId, {
    dataKey: Buffer.from(DEK),
    lastActivity: now,
    expiresAt: now + UserCrypto.SESSION_DURATION,
  });

  DEK.fill(0);
}

How This Prevents Data Loss:

  1. Both requests generate their own DEKs (DEK_A and DEK_B)
  2. Both attempt to store their DEK
  3. Critical Step: Both read back the stored DEK from database
  4. They compare: "Is my DEK the same as what's stored?"
  5. If different (race detected): Discard generated DEK, use the stored one
  6. Result: Both sessions end up using the same DEK (whichever won the race)
  7. ✅ Data encrypted by either session can be decrypted by future logins
## ✅ Changes Made ### 1. **Race Condition Mitigation** (Critical Fix) #### Problem Identified You're right - the original implementation had a classic **check-then-act race condition**. Two concurrent login requests could both generate different DEKs, and whichever stored last would win, leaving the other session with a mismatched key. **Race Condition Scenario:** ``` Time | Request A (User Login) | Request B (User Login - Same User) ------+-------------------------------+------------------------------------ T1 | Check: No DEK exists | T2 | Generate Random DEK_A | T3 | | Check: No DEK exists (still!) T4 | | Generate Random DEK_B (DIFFERENT!) T5 | Store DEK_A in database | T6 | | Store DEK_B (OVERWRITES!) T7 | Session A uses DEK_A | T8 | | Session B uses DEK_B T9 | User creates credential | T10 | Credential encrypted with DEK_A| T11 | Database now has DEK_B | ------+-------------------------------+------------------------------------ Result: Next login uses DEK_B, but credential was encrypted with DEK_A ❌ DATA LOSS! ``` #### Solution Implemented Added a **"write-then-read-and-verify"** pattern in `setupOIDCUserEncryption()`: ```typescript async setupOIDCUserEncryption(userId: string): Promise<void> { const existingEncryptedDEK = await this.getEncryptedDEK(userId); let DEK: Buffer; if (existingEncryptedDEK) { // User already has a persisted DEK, retrieve it const systemKey = this.deriveOIDCSystemKey(userId); DEK = this.decryptDEK(existingEncryptedDEK, systemKey); systemKey.fill(0); } else { // First time setup - create and persist new DEK DEK = crypto.randomBytes(UserCrypto.DEK_LENGTH); const systemKey = this.deriveOIDCSystemKey(userId); try { const encryptedDEK = this.encryptDEK(DEK, systemKey); await this.storeEncryptedDEK(userId, encryptedDEK); // ✅ MITIGATION: Read back the stored DEK to ensure we use the one that won the race const storedEncryptedDEK = await this.getEncryptedDEK(userId); if ( storedEncryptedDEK && storedEncryptedDEK.data !== encryptedDEK.data ) { // We lost the race. Use the DEK from the database. DEK.fill(0); // Discard our generated DEK DEK = this.decryptDEK(storedEncryptedDEK, systemKey); } else if (!storedEncryptedDEK) { // This is an unexpected state; the store operation should have worked. throw new Error( "Failed to store and retrieve user encryption key.", ); } } finally { systemKey.fill(0); // Always clean up sensitive material } } // Load verified DEK into session const now = Date.now(); this.userSessions.set(userId, { dataKey: Buffer.from(DEK), lastActivity: now, expiresAt: now + UserCrypto.SESSION_DURATION, }); DEK.fill(0); } ``` **How This Prevents Data Loss:** 1. Both requests generate their own DEKs (DEK_A and DEK_B) 2. Both attempt to store their DEK 3. **Critical Step**: Both read back the stored DEK from database 4. They compare: "Is my DEK the same as what's stored?" 5. If different (race detected): Discard generated DEK, use the stored one 6. **Result**: Both sessions end up using the **same DEK** (whichever won the race) 7. ✅ Data encrypted by either session can be decrypted by future logins ---
async setupOIDCUserEncryption(userId: string): Promise<void> {
const DEK = crypto.randomBytes(UserCrypto.DEK_LENGTH);
// Check if DEK already exists for this OIDC user
const existingKEKSalt = await this.getKEKSalt(userId);
const existingEncryptedDEK = await this.getEncryptedDEK(userId);
let DEK: Buffer;
if (existingKEKSalt && existingEncryptedDEK) {
// User already has a persisted DEK, retrieve it
const systemKey = this.deriveOIDCSystemKey(userId);
DEK = this.decryptDEK(existingEncryptedDEK, systemKey);
systemKey.fill(0);
} else {
// First time setup - create and persist new DEK
DEK = crypto.randomBytes(UserCrypto.DEK_LENGTH);
// Generate a KEK salt for OIDC user (using a deterministic approach)
const kekSalt = await this.generateKEKSalt();
await this.storeKEKSalt(userId, kekSalt);
// Derive system key for OIDC user
const systemKey = this.deriveOIDCSystemKey(userId);
// Encrypt and store the DEK
const encryptedDEK = this.encryptDEK(DEK, systemKey);
await this.storeEncryptedDEK(userId, encryptedDEK);
systemKey.fill(0);
}
const now = Date.now();
this.userSessions.set(userId, {
@@ -135,34 +162,34 @@ class UserCrypto {
async authenticateOIDCUser(userId: string): Promise<boolean> {
gemini-code-assist[bot] commented 2025-10-06 06:42:56 +00:00 (Migrated from github.com)
Review
Copy Link

medium

The check for kekSalt is redundant for OIDC users. The kekSalt is not used in the OIDC key derivation process, as deriveOIDCSystemKey uses the userId as its salt. Relying solely on encryptedDEK to determine if encryption is set up simplifies the code and removes an unnecessary database read.

      const encryptedDEK = await this.getEncryptedDEK(userId);

      if (!encryptedDEK) {
        // First time login or missing encryption data - set up encryption
        await this.setupOIDCUserEncryption(userId);
        return true;
      }
![medium](https://www.gstatic.com/codereviewagent/medium-priority.svg) The check for `kekSalt` is redundant for OIDC users. The `kekSalt` is not used in the OIDC key derivation process, as `deriveOIDCSystemKey` uses the `userId` as its salt. Relying solely on `encryptedDEK` to determine if encryption is set up simplifies the code and removes an unnecessary database read. ```typescript const encryptedDEK = await this.getEncryptedDEK(userId); if (!encryptedDEK) { // First time login or missing encryption data - set up encryption await this.setupOIDCUserEncryption(userId); return true; } ```
thorved commented 2025-10-06 07:02:20 +00:00 (Migrated from github.com)
Review
Copy Link

2. Removed Redundant kekSalt Logic

Problem Identified

Correct - the kekSalt is generated and stored but never actually used for OIDC users. The deriveOIDCSystemKey() function uses userId directly as the salt, making the stored kekSalt completely redundant.

Why It Was Redundant:

// kekSalt was generated and stored...
const kekSalt = await this.generateKEKSalt();
await this.storeKEKSalt(userId, kekSalt);

// ...but deriveOIDCSystemKey() doesn't use it!
private deriveOIDCSystemKey(userId: string): Buffer {
  const systemSecret = process.env.OIDC_SYSTEM_SECRET || "termix-oidc-system-secret-default";
  const salt = Buffer.from(userId, "utf8");  // ← Uses userId, not kekSalt!
  return crypto.pbkdf2Sync(systemSecret, salt, 100000, UserCrypto.KEK_LENGTH, "sha256");
}

Solution Implemented

In setupOIDCUserEncryption():

  • ❌ Removed: const kekSalt = await this.generateKEKSalt();
  • ❌ Removed: await this.storeKEKSalt(userId, kekSalt);
  • ❌ Removed: const existingKEKSalt = await this.getKEKSalt(userId);

In authenticateOIDCUser():

// Before - Redundant check
const kekSalt = await this.getKEKSalt(userId);
const encryptedDEK = await this.getEncryptedDEK(userId);
if (!kekSalt || !encryptedDEK) {
  // ...
}

// After - Only check what's needed
const encryptedDEK = await this.getEncryptedDEK(userId);
if (!encryptedDEK) {
  // First time login or missing encryption data - set up encryption
  await this.setupOIDCUserEncryption(userId);
  return true;
}

Benefits:

  • ✅ Removed unnecessary database operations
  • ✅ Simplified code logic
  • ✅ Removed potential confusion about kekSalt purpose
  • ✅ Made it clear that OIDC and password-based auth have different key derivation methods
### 2. **Removed Redundant kekSalt Logic** #### Problem Identified Correct - the `kekSalt` is generated and stored but **never actually used** for OIDC users. The `deriveOIDCSystemKey()` function uses `userId` directly as the salt, making the stored `kekSalt` completely redundant. **Why It Was Redundant:** ```typescript // kekSalt was generated and stored... const kekSalt = await this.generateKEKSalt(); await this.storeKEKSalt(userId, kekSalt); // ...but deriveOIDCSystemKey() doesn't use it! private deriveOIDCSystemKey(userId: string): Buffer { const systemSecret = process.env.OIDC_SYSTEM_SECRET || "termix-oidc-system-secret-default"; const salt = Buffer.from(userId, "utf8"); // ← Uses userId, not kekSalt! return crypto.pbkdf2Sync(systemSecret, salt, 100000, UserCrypto.KEK_LENGTH, "sha256"); } ``` #### Solution Implemented **In `setupOIDCUserEncryption()`:** - ❌ Removed: `const kekSalt = await this.generateKEKSalt();` - ❌ Removed: `await this.storeKEKSalt(userId, kekSalt);` - ❌ Removed: `const existingKEKSalt = await this.getKEKSalt(userId);` **In `authenticateOIDCUser()`:** ```typescript // Before - Redundant check const kekSalt = await this.getKEKSalt(userId); const encryptedDEK = await this.getEncryptedDEK(userId); if (!kekSalt || !encryptedDEK) { // ... } // After - Only check what's needed const encryptedDEK = await this.getEncryptedDEK(userId); if (!encryptedDEK) { // First time login or missing encryption data - set up encryption await this.setupOIDCUserEncryption(userId); return true; } ``` **Benefits:** - ✅ Removed unnecessary database operations - ✅ Simplified code logic - ✅ Removed potential confusion about kekSalt purpose - ✅ Made it clear that OIDC and password-based auth have different key derivation methods ---
try {
const kekSalt = await this.getKEKSalt(userId);
if (!kekSalt) {
await this.setupOIDCUserEncryption(userId);
return true;
}
const systemKey = this.deriveOIDCSystemKey(userId);
const encryptedDEK = await this.getEncryptedDEK(userId);
if (!encryptedDEK) {
systemKey.fill(0);
if (!kekSalt || !encryptedDEK) {
// First time login or missing encryption data - set up encryption
await this.setupOIDCUserEncryption(userId);
return true;
}
// Retrieve persisted DEK
const systemKey = this.deriveOIDCSystemKey(userId);
const DEK = this.decryptDEK(encryptedDEK, systemKey);
systemKey.fill(0);
if (!DEK || DEK.length === 0) {
// DEK decryption failed - recreate encryption
await this.setupOIDCUserEncryption(userId);
return true;
}
const now = Date.now();
// Clear any existing session
const oldSession = this.userSessions.get(userId);
if (oldSession) {
oldSession.dataKey.fill(0);
}
// Create new session with the persisted DEK
this.userSessions.set(userId, {
dataKey: Buffer.from(DEK),
lastActivity: now,
@@ -173,6 +200,7 @@ class UserCrypto {
return true;
} catch (error) {
// On error, set up fresh encryption
await this.setupOIDCUserEncryption(userId);
return true;
}