mirror of
https://github.com/bitwarden/browser
synced 2025-12-19 17:53:39 +00:00
[PM-18026] Implement forced, automatic KDF upgrades (#15937)
* Implement automatic kdf upgrades * Fix kdf config not being updated * Update legacy kdf state on master password unlock sync * Fix cli build * Fix * Deduplicate prompts * Fix dismiss time * Fix default kdf setting * Fix build * Undo changes * Fix test * Fix prettier * Fix test * Update libs/angular/src/key-management/encrypted-migration/encrypted-migrations-scheduler.service.ts Co-authored-by: Maciej Zieniuk <167752252+mzieniukbw@users.noreply.github.com> * Update libs/common/src/key-management/master-password/abstractions/master-password.service.abstraction.ts Co-authored-by: Maciej Zieniuk <167752252+mzieniukbw@users.noreply.github.com> * Update libs/angular/src/key-management/encrypted-migration/encrypted-migrations-scheduler.service.ts Co-authored-by: Maciej Zieniuk <167752252+mzieniukbw@users.noreply.github.com> * Only sync when there is at least one migration * Relative imports * Add tech debt comment * Resolve inconsistent prefix * Clean up * Update docs * Use default PBKDF2 iteratinos instead of custom threshold * Undo type check * Fix build * Add comment * Cleanup * Cleanup * Address component feedback * Use isnullorwhitespace * Fix tests * Allow migration only on vault * Fix tests * Run prettier * Fix tests * Prevent await race condition * Fix min and default values in kdf migration * Run sync only when a migration was run * Update libs/common/src/key-management/encrypted-migrator/default-encrypted-migrator.ts Co-authored-by: Maciej Zieniuk <167752252+mzieniukbw@users.noreply.github.com> * Fix link not being blue * Fix later button on browser --------- Co-authored-by: Maciej Zieniuk <167752252+mzieniukbw@users.noreply.github.com>
This commit is contained in:
@@ -18,6 +18,8 @@ export class AuthResult {
|
||||
email: string;
|
||||
requiresEncryptionKeyMigration: boolean;
|
||||
requiresDeviceVerification: boolean;
|
||||
// The master-password used in the authentication process
|
||||
masterPassword: string | null;
|
||||
|
||||
get requiresTwoFactor() {
|
||||
return this.twoFactorProviders != null;
|
||||
|
||||
@@ -0,0 +1,194 @@
|
||||
import { mock } from "jest-mock-extended";
|
||||
|
||||
// eslint-disable-next-line no-restricted-imports
|
||||
import { KdfConfigService } from "@bitwarden/key-management";
|
||||
import { LogService } from "@bitwarden/logging";
|
||||
|
||||
import { ConfigService } from "../../platform/abstractions/config/config.service";
|
||||
import { SyncService } from "../../platform/sync";
|
||||
import { UserId } from "../../types/guid";
|
||||
import { ChangeKdfService } from "../kdf/change-kdf.service.abstraction";
|
||||
import { MasterPasswordServiceAbstraction } from "../master-password/abstractions/master-password.service.abstraction";
|
||||
|
||||
import { DefaultEncryptedMigrator } from "./default-encrypted-migrator";
|
||||
import { EncryptedMigration } from "./migrations/encrypted-migration";
|
||||
import { MinimumKdfMigration } from "./migrations/minimum-kdf-migration";
|
||||
|
||||
jest.mock("./migrations/minimum-kdf-migration");
|
||||
|
||||
describe("EncryptedMigrator", () => {
|
||||
const mockKdfConfigService = mock<KdfConfigService>();
|
||||
const mockChangeKdfService = mock<ChangeKdfService>();
|
||||
const mockLogService = mock<LogService>();
|
||||
const configService = mock<ConfigService>();
|
||||
const masterPasswordService = mock<MasterPasswordServiceAbstraction>();
|
||||
const syncService = mock<SyncService>();
|
||||
|
||||
let sut: DefaultEncryptedMigrator;
|
||||
const mockMigration = mock<MinimumKdfMigration>();
|
||||
|
||||
const mockUserId = "00000000-0000-0000-0000-000000000000" as UserId;
|
||||
const mockMasterPassword = "masterPassword123";
|
||||
|
||||
beforeEach(() => {
|
||||
jest.clearAllMocks();
|
||||
|
||||
// Mock the MinimumKdfMigration constructor to return our mock
|
||||
(MinimumKdfMigration as jest.MockedClass<typeof MinimumKdfMigration>).mockImplementation(
|
||||
() => mockMigration,
|
||||
);
|
||||
|
||||
sut = new DefaultEncryptedMigrator(
|
||||
mockKdfConfigService,
|
||||
mockChangeKdfService,
|
||||
mockLogService,
|
||||
configService,
|
||||
masterPasswordService,
|
||||
syncService,
|
||||
);
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
jest.resetAllMocks();
|
||||
});
|
||||
|
||||
describe("runMigrations", () => {
|
||||
it("should throw error when userId is null", async () => {
|
||||
await expect(sut.runMigrations(null as any, null)).rejects.toThrow("userId");
|
||||
});
|
||||
|
||||
it("should throw error when userId is undefined", async () => {
|
||||
await expect(sut.runMigrations(undefined as any, null)).rejects.toThrow("userId");
|
||||
});
|
||||
|
||||
it("should not run migration when needsMigration returns 'noMigrationNeeded'", async () => {
|
||||
mockMigration.needsMigration.mockResolvedValue("noMigrationNeeded");
|
||||
|
||||
await sut.runMigrations(mockUserId, null);
|
||||
|
||||
expect(mockMigration.needsMigration).toHaveBeenCalledWith(mockUserId);
|
||||
expect(mockMigration.runMigrations).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("should run migration when needsMigration returns 'needsMigration'", async () => {
|
||||
mockMigration.needsMigration.mockResolvedValue("needsMigration");
|
||||
|
||||
await sut.runMigrations(mockUserId, mockMasterPassword);
|
||||
|
||||
expect(mockMigration.needsMigration).toHaveBeenCalledWith(mockUserId);
|
||||
expect(mockMigration.runMigrations).toHaveBeenCalledWith(mockUserId, mockMasterPassword);
|
||||
});
|
||||
|
||||
it("should run migration when needsMigration returns 'needsMigrationWithMasterPassword'", async () => {
|
||||
mockMigration.needsMigration.mockResolvedValue("needsMigrationWithMasterPassword");
|
||||
|
||||
await sut.runMigrations(mockUserId, mockMasterPassword);
|
||||
|
||||
expect(mockMigration.needsMigration).toHaveBeenCalledWith(mockUserId);
|
||||
expect(mockMigration.runMigrations).toHaveBeenCalledWith(mockUserId, mockMasterPassword);
|
||||
});
|
||||
|
||||
it("should throw error when migration needs master password but null is provided", async () => {
|
||||
mockMigration.needsMigration.mockResolvedValue("needsMigrationWithMasterPassword");
|
||||
|
||||
await sut.runMigrations(mockUserId, null);
|
||||
expect(mockMigration.needsMigration).toHaveBeenCalledWith(mockUserId);
|
||||
expect(mockMigration.runMigrations).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("should run multiple migrations", async () => {
|
||||
const mockSecondMigration = mock<EncryptedMigration>();
|
||||
mockSecondMigration.needsMigration.mockResolvedValue("needsMigration");
|
||||
|
||||
(sut as any).migrations.push({
|
||||
name: "Test Second Migration",
|
||||
migration: mockSecondMigration,
|
||||
});
|
||||
|
||||
mockMigration.needsMigration.mockResolvedValue("needsMigration");
|
||||
|
||||
await sut.runMigrations(mockUserId, mockMasterPassword);
|
||||
|
||||
expect(mockMigration.needsMigration).toHaveBeenCalledWith(mockUserId);
|
||||
expect(mockSecondMigration.needsMigration).toHaveBeenCalledWith(mockUserId);
|
||||
expect(mockMigration.runMigrations).toHaveBeenCalledWith(mockUserId, mockMasterPassword);
|
||||
expect(mockSecondMigration.runMigrations).toHaveBeenCalledWith(
|
||||
mockUserId,
|
||||
mockMasterPassword,
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe("needsMigrations", () => {
|
||||
it("should return 'noMigrationNeeded' when no migrations are needed", async () => {
|
||||
mockMigration.needsMigration.mockResolvedValue("noMigrationNeeded");
|
||||
|
||||
const result = await sut.needsMigrations(mockUserId);
|
||||
|
||||
expect(result).toBe("noMigrationNeeded");
|
||||
expect(mockMigration.needsMigration).toHaveBeenCalledWith(mockUserId);
|
||||
});
|
||||
|
||||
it("should return 'needsMigration' when at least one migration needs to run", async () => {
|
||||
mockMigration.needsMigration.mockResolvedValue("needsMigration");
|
||||
|
||||
const result = await sut.needsMigrations(mockUserId);
|
||||
|
||||
expect(result).toBe("needsMigration");
|
||||
expect(mockMigration.needsMigration).toHaveBeenCalledWith(mockUserId);
|
||||
});
|
||||
|
||||
it("should return 'needsMigrationWithMasterPassword' when at least one migration needs master password", async () => {
|
||||
mockMigration.needsMigration.mockResolvedValue("needsMigrationWithMasterPassword");
|
||||
|
||||
const result = await sut.needsMigrations(mockUserId);
|
||||
|
||||
expect(result).toBe("needsMigrationWithMasterPassword");
|
||||
expect(mockMigration.needsMigration).toHaveBeenCalledWith(mockUserId);
|
||||
});
|
||||
|
||||
it("should prioritize 'needsMigrationWithMasterPassword' over 'needsMigration'", async () => {
|
||||
const mockSecondMigration = mock<EncryptedMigration>();
|
||||
mockSecondMigration.needsMigration.mockResolvedValue("needsMigration");
|
||||
|
||||
(sut as any).migrations.push({
|
||||
name: "Test Second Migration",
|
||||
migration: mockSecondMigration,
|
||||
});
|
||||
|
||||
mockMigration.needsMigration.mockResolvedValue("needsMigrationWithMasterPassword");
|
||||
|
||||
const result = await sut.needsMigrations(mockUserId);
|
||||
|
||||
expect(result).toBe("needsMigrationWithMasterPassword");
|
||||
expect(mockMigration.needsMigration).toHaveBeenCalledWith(mockUserId);
|
||||
expect(mockSecondMigration.needsMigration).toHaveBeenCalledWith(mockUserId);
|
||||
});
|
||||
|
||||
it("should return 'needsMigration' when some migrations need running but none need master password", async () => {
|
||||
const mockSecondMigration = mock<EncryptedMigration>();
|
||||
mockSecondMigration.needsMigration.mockResolvedValue("noMigrationNeeded");
|
||||
|
||||
(sut as any).migrations.push({
|
||||
name: "Test Second Migration",
|
||||
migration: mockSecondMigration,
|
||||
});
|
||||
|
||||
mockMigration.needsMigration.mockResolvedValue("needsMigration");
|
||||
|
||||
const result = await sut.needsMigrations(mockUserId);
|
||||
|
||||
expect(result).toBe("needsMigration");
|
||||
expect(mockMigration.needsMigration).toHaveBeenCalledWith(mockUserId);
|
||||
expect(mockSecondMigration.needsMigration).toHaveBeenCalledWith(mockUserId);
|
||||
});
|
||||
|
||||
it("should throw error when userId is null", async () => {
|
||||
await expect(sut.needsMigrations(null as any)).rejects.toThrow("userId");
|
||||
});
|
||||
|
||||
it("should throw error when userId is undefined", async () => {
|
||||
await expect(sut.needsMigrations(undefined as any)).rejects.toThrow("userId");
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,113 @@
|
||||
// eslint-disable-next-line no-restricted-imports
|
||||
import { KdfConfigService } from "@bitwarden/key-management";
|
||||
import { LogService } from "@bitwarden/logging";
|
||||
|
||||
import { assertNonNullish } from "../../auth/utils";
|
||||
import { ConfigService } from "../../platform/abstractions/config/config.service";
|
||||
import { SyncService } from "../../platform/sync";
|
||||
import { UserId } from "../../types/guid";
|
||||
import { ChangeKdfService } from "../kdf/change-kdf.service.abstraction";
|
||||
import { MasterPasswordServiceAbstraction } from "../master-password/abstractions/master-password.service.abstraction";
|
||||
|
||||
import { EncryptedMigrator } from "./encrypted-migrator.abstraction";
|
||||
import { EncryptedMigration, MigrationRequirement } from "./migrations/encrypted-migration";
|
||||
import { MinimumKdfMigration } from "./migrations/minimum-kdf-migration";
|
||||
|
||||
export class DefaultEncryptedMigrator implements EncryptedMigrator {
|
||||
private migrations: { name: string; migration: EncryptedMigration }[] = [];
|
||||
private isRunningMigration = false;
|
||||
|
||||
constructor(
|
||||
readonly kdfConfigService: KdfConfigService,
|
||||
readonly changeKdfService: ChangeKdfService,
|
||||
private readonly logService: LogService,
|
||||
readonly configService: ConfigService,
|
||||
readonly masterPasswordService: MasterPasswordServiceAbstraction,
|
||||
readonly syncService: SyncService,
|
||||
) {
|
||||
// Register migrations here
|
||||
this.migrations.push({
|
||||
name: "Minimum PBKDF2 Iteration Count Migration",
|
||||
migration: new MinimumKdfMigration(
|
||||
kdfConfigService,
|
||||
changeKdfService,
|
||||
logService,
|
||||
configService,
|
||||
masterPasswordService,
|
||||
),
|
||||
});
|
||||
}
|
||||
|
||||
async runMigrations(userId: UserId, masterPassword: string | null): Promise<void> {
|
||||
assertNonNullish(userId, "userId");
|
||||
|
||||
// Ensure that the requirements for running all migrations are met
|
||||
const needsMigration = await this.needsMigrations(userId);
|
||||
if (needsMigration === "noMigrationNeeded") {
|
||||
return;
|
||||
} else if (needsMigration === "needsMigrationWithMasterPassword" && masterPassword == null) {
|
||||
// If a migration needs a password, but none is provided, the migrations are skipped. If a manual caller
|
||||
// during a login / unlock flow calls without a master password in a login / unlock strategy that has no
|
||||
// password, such as biometric unlock, the migrations are skipped.
|
||||
//
|
||||
// The fallback to this, the encrypted migrations scheduler, will first check if a migration needs a password
|
||||
// and then prompt the user. If the user enters their password, runMigrations is called again with the password.
|
||||
return;
|
||||
}
|
||||
|
||||
try {
|
||||
// No concurrent migrations allowed, so acquire a service-wide lock
|
||||
if (this.isRunningMigration) {
|
||||
return;
|
||||
}
|
||||
this.isRunningMigration = true;
|
||||
|
||||
// Run all migrations sequentially in the order they were registered
|
||||
this.logService.mark("[Encrypted Migrator] Start");
|
||||
this.logService.info(`[Encrypted Migrator] Starting migrations for user: ${userId}`);
|
||||
let ranMigration = false;
|
||||
for (const { name, migration } of this.migrations) {
|
||||
if ((await migration.needsMigration(userId)) !== "noMigrationNeeded") {
|
||||
this.logService.info(`[Encrypted Migrator] Running migration: ${name}`);
|
||||
const start = performance.now();
|
||||
await migration.runMigrations(userId, masterPassword);
|
||||
this.logService.measure(start, "[Encrypted Migrator]", name, "ExecutionTime");
|
||||
ranMigration = true;
|
||||
}
|
||||
}
|
||||
this.logService.mark("[Encrypted Migrator] Finish");
|
||||
this.logService.info(`[Encrypted Migrator] Completed migrations for user: ${userId}`);
|
||||
if (ranMigration) {
|
||||
await this.syncService.fullSync(true);
|
||||
}
|
||||
} catch (error) {
|
||||
this.logService.error(
|
||||
`[Encrypted Migrator] Error running migrations for user: ${userId}`,
|
||||
error,
|
||||
);
|
||||
throw error; // Re-throw the error to be handled by the caller
|
||||
} finally {
|
||||
this.isRunningMigration = false;
|
||||
}
|
||||
}
|
||||
|
||||
async needsMigrations(userId: UserId): Promise<MigrationRequirement> {
|
||||
assertNonNullish(userId, "userId");
|
||||
|
||||
const migrationRequirements = await Promise.all(
|
||||
this.migrations.map(async ({ migration }) => migration.needsMigration(userId)),
|
||||
);
|
||||
|
||||
if (migrationRequirements.includes("needsMigrationWithMasterPassword")) {
|
||||
return "needsMigrationWithMasterPassword";
|
||||
} else if (migrationRequirements.includes("needsMigration")) {
|
||||
return "needsMigration";
|
||||
} else {
|
||||
return "noMigrationNeeded";
|
||||
}
|
||||
}
|
||||
|
||||
isRunningMigrations(): boolean {
|
||||
return this.isRunningMigration;
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,32 @@
|
||||
import { UserId } from "../../types/guid";
|
||||
|
||||
import { MigrationRequirement } from "./migrations/encrypted-migration";
|
||||
|
||||
export abstract class EncryptedMigrator {
|
||||
/**
|
||||
* Runs migrations on a decrypted user, with the cryptographic state initialized.
|
||||
* This only runs the migrations that are needed for the user.
|
||||
* This needs to be run after the decrypted user key has been set to state.
|
||||
*
|
||||
* If the master password is required but not provided, the migrations will not run, and the function will return early.
|
||||
* If migrations are already running, the migrations will not run again, and the function will return early.
|
||||
*
|
||||
* @param userId The ID of the user to run migrations for.
|
||||
* @param masterPassword The user's current master password.
|
||||
* @throws If the user does not exist
|
||||
* @throws If the user is locked or logged out
|
||||
* @throws If a migration fails
|
||||
*/
|
||||
abstract runMigrations(userId: UserId, masterPassword: string | null): Promise<void>;
|
||||
/**
|
||||
* Checks if the user needs to run any migrations.
|
||||
* This is used to determine if the user should be prompted to run migrations.
|
||||
* @param userId The ID of the user to check migrations for.
|
||||
*/
|
||||
abstract needsMigrations(userId: UserId): Promise<MigrationRequirement>;
|
||||
|
||||
/**
|
||||
* Indicates whether migrations are currently running.
|
||||
*/
|
||||
abstract isRunningMigrations(): boolean;
|
||||
}
|
||||
@@ -0,0 +1,36 @@
|
||||
import { UserId } from "../../../types/guid";
|
||||
|
||||
/**
|
||||
* @internal
|
||||
* IMPORTANT: Please read this when implementing new migrations.
|
||||
*
|
||||
* An encrypted migration defines an online migration that mutates the persistent state of the user on the server, or locally.
|
||||
* It should only be run once per user (or for local migrations, once per device). Migrations get scheduled automatically,
|
||||
* during actions such as login and unlock, or during sync.
|
||||
*
|
||||
* Migrations can require the master-password, which is provided by the user if required.
|
||||
* Migrations are run as soon as possible non-lazily, and MAY block unlock / login, if they have to run.
|
||||
*
|
||||
* Most importantly, implementing a migration should be done such that concurrent migrations may fail, but must never
|
||||
* leave the user in a broken state. Locally, these are scheduled with an application-global lock. However, no such guarantees
|
||||
* are made for the server, and other devices may run the migration concurrently.
|
||||
*
|
||||
* When adding a migration, it *MUST* be feature-flagged for the initial roll-out.
|
||||
*/
|
||||
export interface EncryptedMigration {
|
||||
/**
|
||||
* Runs the migration.
|
||||
* @throws If the migration fails, such as when no network is available.
|
||||
* @throws If the requirements for migration are not met (e.g. the user is locked)
|
||||
*/
|
||||
runMigrations(userId: UserId, masterPassword: string | null): Promise<void>;
|
||||
/**
|
||||
* Returns whether the migration needs to be run for the user, and if it does, whether the master password is required.
|
||||
*/
|
||||
needsMigration(userId: UserId): Promise<MigrationRequirement>;
|
||||
}
|
||||
|
||||
export type MigrationRequirement =
|
||||
| "needsMigration"
|
||||
| "needsMigrationWithMasterPassword"
|
||||
| "noMigrationNeeded";
|
||||
@@ -0,0 +1,184 @@
|
||||
import { mock } from "jest-mock-extended";
|
||||
|
||||
// eslint-disable-next-line no-restricted-imports
|
||||
import {
|
||||
Argon2KdfConfig,
|
||||
KdfConfigService,
|
||||
KdfType,
|
||||
PBKDF2KdfConfig,
|
||||
} from "@bitwarden/key-management";
|
||||
import { LogService } from "@bitwarden/logging";
|
||||
|
||||
import { FeatureFlag } from "../../../enums/feature-flag.enum";
|
||||
import { ConfigService } from "../../../platform/abstractions/config/config.service";
|
||||
import { UserId } from "../../../types/guid";
|
||||
import { ChangeKdfService } from "../../kdf/change-kdf.service.abstraction";
|
||||
import { MasterPasswordServiceAbstraction } from "../../master-password/abstractions/master-password.service.abstraction";
|
||||
|
||||
import { MinimumKdfMigration } from "./minimum-kdf-migration";
|
||||
|
||||
describe("MinimumKdfMigration", () => {
|
||||
const mockKdfConfigService = mock<KdfConfigService>();
|
||||
const mockChangeKdfService = mock<ChangeKdfService>();
|
||||
const mockLogService = mock<LogService>();
|
||||
const mockConfigService = mock<ConfigService>();
|
||||
const mockMasterPasswordService = mock<MasterPasswordServiceAbstraction>();
|
||||
|
||||
let sut: MinimumKdfMigration;
|
||||
|
||||
const mockUserId = "00000000-0000-0000-0000-000000000000" as UserId;
|
||||
const mockMasterPassword = "masterPassword";
|
||||
|
||||
beforeEach(() => {
|
||||
jest.clearAllMocks();
|
||||
|
||||
sut = new MinimumKdfMigration(
|
||||
mockKdfConfigService,
|
||||
mockChangeKdfService,
|
||||
mockLogService,
|
||||
mockConfigService,
|
||||
mockMasterPasswordService,
|
||||
);
|
||||
});
|
||||
|
||||
describe("needsMigration", () => {
|
||||
it("should return 'noMigrationNeeded' when user does not have a master password`", async () => {
|
||||
mockMasterPasswordService.userHasMasterPassword.mockResolvedValue(false);
|
||||
const result = await sut.needsMigration(mockUserId);
|
||||
expect(result).toBe("noMigrationNeeded");
|
||||
});
|
||||
|
||||
it("should return 'noMigrationNeeded' when user uses argon2id`", async () => {
|
||||
mockMasterPasswordService.userHasMasterPassword.mockResolvedValue(true);
|
||||
mockKdfConfigService.getKdfConfig.mockResolvedValue(new Argon2KdfConfig(3, 64, 4));
|
||||
const result = await sut.needsMigration(mockUserId);
|
||||
expect(result).toBe("noMigrationNeeded");
|
||||
});
|
||||
|
||||
it("should return 'noMigrationNeeded' when PBKDF2 iterations are already above minimum", async () => {
|
||||
const mockKdfConfig = {
|
||||
kdfType: KdfType.PBKDF2_SHA256,
|
||||
iterations: PBKDF2KdfConfig.ITERATIONS.min + 1000,
|
||||
};
|
||||
mockKdfConfigService.getKdfConfig.mockResolvedValue(mockKdfConfig as any);
|
||||
|
||||
const result = await sut.needsMigration(mockUserId);
|
||||
|
||||
expect(result).toBe("noMigrationNeeded");
|
||||
expect(mockKdfConfigService.getKdfConfig).toHaveBeenCalledWith(mockUserId);
|
||||
});
|
||||
|
||||
it("should return 'noMigrationNeeded' when PBKDF2 iterations equal minimum", async () => {
|
||||
const mockKdfConfig = {
|
||||
kdfType: KdfType.PBKDF2_SHA256,
|
||||
iterations: PBKDF2KdfConfig.ITERATIONS.min,
|
||||
};
|
||||
mockKdfConfigService.getKdfConfig.mockResolvedValue(mockKdfConfig as any);
|
||||
mockConfigService.getFeatureFlag.mockResolvedValue(true);
|
||||
|
||||
const result = await sut.needsMigration(mockUserId);
|
||||
|
||||
expect(result).toBe("noMigrationNeeded");
|
||||
expect(mockKdfConfigService.getKdfConfig).toHaveBeenCalledWith(mockUserId);
|
||||
});
|
||||
|
||||
it("should return 'noMigrationNeeded' when feature flag is disabled", async () => {
|
||||
const mockKdfConfig = {
|
||||
kdfType: KdfType.PBKDF2_SHA256,
|
||||
iterations: PBKDF2KdfConfig.ITERATIONS.min - 1000,
|
||||
};
|
||||
mockKdfConfigService.getKdfConfig.mockResolvedValue(mockKdfConfig as any);
|
||||
mockConfigService.getFeatureFlag.mockResolvedValue(false);
|
||||
|
||||
const result = await sut.needsMigration(mockUserId);
|
||||
|
||||
expect(result).toBe("noMigrationNeeded");
|
||||
expect(mockKdfConfigService.getKdfConfig).toHaveBeenCalledWith(mockUserId);
|
||||
expect(mockConfigService.getFeatureFlag).toHaveBeenCalledWith(
|
||||
FeatureFlag.ForceUpdateKDFSettings,
|
||||
);
|
||||
});
|
||||
|
||||
it("should return 'needsMigrationWithMasterPassword' when PBKDF2 iterations are below minimum and feature flag is enabled", async () => {
|
||||
const mockKdfConfig = {
|
||||
kdfType: KdfType.PBKDF2_SHA256,
|
||||
iterations: PBKDF2KdfConfig.ITERATIONS.min - 1000,
|
||||
};
|
||||
mockKdfConfigService.getKdfConfig.mockResolvedValue(mockKdfConfig as any);
|
||||
mockConfigService.getFeatureFlag.mockResolvedValue(true);
|
||||
|
||||
const result = await sut.needsMigration(mockUserId);
|
||||
|
||||
expect(result).toBe("needsMigrationWithMasterPassword");
|
||||
expect(mockKdfConfigService.getKdfConfig).toHaveBeenCalledWith(mockUserId);
|
||||
expect(mockConfigService.getFeatureFlag).toHaveBeenCalledWith(
|
||||
FeatureFlag.ForceUpdateKDFSettings,
|
||||
);
|
||||
});
|
||||
|
||||
it("should throw error when userId is null", async () => {
|
||||
await expect(sut.needsMigration(null as any)).rejects.toThrow("userId");
|
||||
});
|
||||
|
||||
it("should throw error when userId is undefined", async () => {
|
||||
await expect(sut.needsMigration(undefined as any)).rejects.toThrow("userId");
|
||||
});
|
||||
});
|
||||
|
||||
describe("runMigrations", () => {
|
||||
it("should update KDF parameters with minimum PBKDF2 iterations", async () => {
|
||||
await sut.runMigrations(mockUserId, mockMasterPassword);
|
||||
|
||||
expect(mockLogService.info).toHaveBeenCalledWith(
|
||||
`[MinimumKdfMigration] Updating user ${mockUserId} to minimum PBKDF2 iteration count ${PBKDF2KdfConfig.ITERATIONS.min}`,
|
||||
);
|
||||
expect(mockChangeKdfService.updateUserKdfParams).toHaveBeenCalledWith(
|
||||
mockMasterPassword,
|
||||
expect.any(PBKDF2KdfConfig),
|
||||
mockUserId,
|
||||
);
|
||||
|
||||
// Verify the PBKDF2KdfConfig has the correct iteration count
|
||||
const kdfConfigArg = (mockChangeKdfService.updateUserKdfParams as jest.Mock).mock.calls[0][1];
|
||||
expect(kdfConfigArg.iterations).toBe(PBKDF2KdfConfig.ITERATIONS.defaultValue);
|
||||
});
|
||||
|
||||
it("should throw error when userId is null", async () => {
|
||||
await expect(sut.runMigrations(null as any, mockMasterPassword)).rejects.toThrow("userId");
|
||||
});
|
||||
|
||||
it("should throw error when userId is undefined", async () => {
|
||||
await expect(sut.runMigrations(undefined as any, mockMasterPassword)).rejects.toThrow(
|
||||
"userId",
|
||||
);
|
||||
});
|
||||
|
||||
it("should throw error when masterPassword is null", async () => {
|
||||
await expect(sut.runMigrations(mockUserId, null as any)).rejects.toThrow("masterPassword");
|
||||
});
|
||||
|
||||
it("should throw error when masterPassword is undefined", async () => {
|
||||
await expect(sut.runMigrations(mockUserId, undefined as any)).rejects.toThrow(
|
||||
"masterPassword",
|
||||
);
|
||||
});
|
||||
|
||||
it("should handle errors from changeKdfService", async () => {
|
||||
const mockError = new Error("KDF update failed");
|
||||
mockChangeKdfService.updateUserKdfParams.mockRejectedValue(mockError);
|
||||
|
||||
await expect(sut.runMigrations(mockUserId, mockMasterPassword)).rejects.toThrow(
|
||||
"KDF update failed",
|
||||
);
|
||||
|
||||
expect(mockLogService.info).toHaveBeenCalledWith(
|
||||
`[MinimumKdfMigration] Updating user ${mockUserId} to minimum PBKDF2 iteration count ${PBKDF2KdfConfig.ITERATIONS.min}`,
|
||||
);
|
||||
expect(mockChangeKdfService.updateUserKdfParams).toHaveBeenCalledWith(
|
||||
mockMasterPassword,
|
||||
expect.any(PBKDF2KdfConfig),
|
||||
mockUserId,
|
||||
);
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,68 @@
|
||||
import { UserId } from "@bitwarden/common/types/guid";
|
||||
// eslint-disable-next-line no-restricted-imports
|
||||
import { KdfConfigService, KdfType, PBKDF2KdfConfig } from "@bitwarden/key-management";
|
||||
import { LogService } from "@bitwarden/logging";
|
||||
|
||||
import { assertNonNullish } from "../../../auth/utils";
|
||||
import { FeatureFlag } from "../../../enums/feature-flag.enum";
|
||||
import { ConfigService } from "../../../platform/abstractions/config/config.service";
|
||||
import { ChangeKdfService } from "../../kdf/change-kdf.service.abstraction";
|
||||
import { MasterPasswordServiceAbstraction } from "../../master-password/abstractions/master-password.service.abstraction";
|
||||
|
||||
import { EncryptedMigration, MigrationRequirement } from "./encrypted-migration";
|
||||
|
||||
/**
|
||||
* @internal
|
||||
* This migrator ensures the user's account has a minimum PBKDF2 iteration count.
|
||||
* It will update the entire account, logging out old clients if necessary.
|
||||
*/
|
||||
export class MinimumKdfMigration implements EncryptedMigration {
|
||||
constructor(
|
||||
private readonly kdfConfigService: KdfConfigService,
|
||||
private readonly changeKdfService: ChangeKdfService,
|
||||
private readonly logService: LogService,
|
||||
private readonly configService: ConfigService,
|
||||
private readonly masterPasswordService: MasterPasswordServiceAbstraction,
|
||||
) {}
|
||||
|
||||
async runMigrations(userId: UserId, masterPassword: string | null): Promise<void> {
|
||||
assertNonNullish(userId, "userId");
|
||||
assertNonNullish(masterPassword, "masterPassword");
|
||||
|
||||
this.logService.info(
|
||||
`[MinimumKdfMigration] Updating user ${userId} to minimum PBKDF2 iteration count ${PBKDF2KdfConfig.ITERATIONS.defaultValue}`,
|
||||
);
|
||||
await this.changeKdfService.updateUserKdfParams(
|
||||
masterPassword!,
|
||||
new PBKDF2KdfConfig(PBKDF2KdfConfig.ITERATIONS.defaultValue),
|
||||
userId,
|
||||
);
|
||||
await this.kdfConfigService.setKdfConfig(
|
||||
userId,
|
||||
new PBKDF2KdfConfig(PBKDF2KdfConfig.ITERATIONS.defaultValue),
|
||||
);
|
||||
}
|
||||
|
||||
async needsMigration(userId: UserId): Promise<MigrationRequirement> {
|
||||
assertNonNullish(userId, "userId");
|
||||
|
||||
if (!(await this.masterPasswordService.userHasMasterPassword(userId))) {
|
||||
return "noMigrationNeeded";
|
||||
}
|
||||
|
||||
// Only PBKDF2 users below the minimum iteration count need migration
|
||||
const kdfConfig = await this.kdfConfigService.getKdfConfig(userId);
|
||||
if (
|
||||
kdfConfig.kdfType !== KdfType.PBKDF2_SHA256 ||
|
||||
kdfConfig.iterations >= PBKDF2KdfConfig.ITERATIONS.min
|
||||
) {
|
||||
return "noMigrationNeeded";
|
||||
}
|
||||
|
||||
if (!(await this.configService.getFeatureFlag(FeatureFlag.ForceUpdateKDFSettings))) {
|
||||
return "noMigrationNeeded";
|
||||
}
|
||||
|
||||
return "needsMigrationWithMasterPassword";
|
||||
}
|
||||
}
|
||||
@@ -17,7 +17,7 @@ import {
|
||||
} from "../master-password/types/master-password.types";
|
||||
|
||||
import { ChangeKdfApiService } from "./change-kdf-api.service.abstraction";
|
||||
import { DefaultChangeKdfService } from "./change-kdf-service";
|
||||
import { DefaultChangeKdfService } from "./change-kdf.service";
|
||||
|
||||
describe("ChangeKdfService", () => {
|
||||
const changeKdfApiService = mock<ChangeKdfApiService>();
|
||||
@@ -14,7 +14,7 @@ import {
|
||||
} from "../master-password/types/master-password.types";
|
||||
|
||||
import { ChangeKdfApiService } from "./change-kdf-api.service.abstraction";
|
||||
import { ChangeKdfService } from "./change-kdf-service.abstraction";
|
||||
import { ChangeKdfService } from "./change-kdf.service.abstraction";
|
||||
|
||||
export class DefaultChangeKdfService implements ChangeKdfService {
|
||||
constructor(
|
||||
@@ -106,6 +106,13 @@ export abstract class MasterPasswordServiceAbstraction {
|
||||
password: string,
|
||||
masterPasswordUnlockData: MasterPasswordUnlockData,
|
||||
) => Promise<UserKey>;
|
||||
|
||||
/**
|
||||
* Returns whether the user has a master password set.
|
||||
* @param userId The user ID.
|
||||
* @throws If the user ID is missing.
|
||||
*/
|
||||
abstract userHasMasterPassword(userId: UserId): Promise<boolean>;
|
||||
}
|
||||
|
||||
export abstract class InternalMasterPasswordServiceAbstraction extends MasterPasswordServiceAbstraction {
|
||||
|
||||
@@ -33,6 +33,10 @@ export class FakeMasterPasswordService implements InternalMasterPasswordServiceA
|
||||
this.masterKeyHashSubject.next(initialMasterKeyHash);
|
||||
}
|
||||
|
||||
userHasMasterPassword(userId: UserId): Promise<boolean> {
|
||||
return this.mock.userHasMasterPassword(userId);
|
||||
}
|
||||
|
||||
emailToSalt(email: string): MasterPasswordSalt {
|
||||
return this.mock.emailToSalt(email);
|
||||
}
|
||||
|
||||
@@ -25,6 +25,7 @@ import { MasterKey, UserKey } from "../../../types/key";
|
||||
import { KeyGenerationService } from "../../crypto";
|
||||
import { CryptoFunctionService } from "../../crypto/abstractions/crypto-function.service";
|
||||
import { EncryptedString, EncString } from "../../crypto/models/enc-string";
|
||||
import { USES_KEY_CONNECTOR } from "../../key-connector/services/key-connector.service";
|
||||
import { InternalMasterPasswordServiceAbstraction } from "../abstractions/master-password.service.abstraction";
|
||||
import {
|
||||
MasterKeyWrappedUserKey,
|
||||
@@ -85,6 +86,19 @@ export class MasterPasswordService implements InternalMasterPasswordServiceAbstr
|
||||
private accountService: AccountService,
|
||||
) {}
|
||||
|
||||
async userHasMasterPassword(userId: UserId): Promise<boolean> {
|
||||
assertNonNullish(userId, "userId");
|
||||
// A user has a master-password if they have a master-key encrypted user key *but* are not a key connector user
|
||||
// Note: We can't use the key connector service as an abstraction here because it causes a run-time dependency injection cycle between KC service and MP service.
|
||||
const usesKeyConnector = await firstValueFrom(
|
||||
this.stateProvider.getUser(userId, USES_KEY_CONNECTOR).state$,
|
||||
);
|
||||
const usesMasterKey = await firstValueFrom(
|
||||
this.stateProvider.getUser(userId, MASTER_KEY_ENCRYPTED_USER_KEY).state$,
|
||||
);
|
||||
return usesMasterKey && !usesKeyConnector;
|
||||
}
|
||||
|
||||
saltForUser$(userId: UserId): Observable<MasterPasswordSalt> {
|
||||
assertNonNullish(userId, "userId");
|
||||
return this.accountService.accounts$.pipe(
|
||||
@@ -307,6 +321,7 @@ export class MasterPasswordService implements InternalMasterPasswordServiceAbstr
|
||||
masterPasswordUnlockData.kdf.toSdkConfig(),
|
||||
),
|
||||
);
|
||||
|
||||
return userKey as UserKey;
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user