diff --git a/apps/cli/src/auth/commands/login.command.ts b/apps/cli/src/auth/commands/login.command.ts index 3ceac859c4..d25e9a70d8 100644 --- a/apps/cli/src/auth/commands/login.command.ts +++ b/apps/cli/src/auth/commands/login.command.ts @@ -428,7 +428,8 @@ export class LoginCommand { ); const request = new PasswordRequest(); - request.masterPasswordHash = await this.keyService.hashMasterKey(currentPassword, null); + const masterKey = await this.keyService.getOrDeriveMasterKey(currentPassword, userId); + request.masterPasswordHash = await this.keyService.hashMasterKey(currentPassword, masterKey); request.masterPasswordHint = hint; request.newMasterPasswordHash = newPasswordHash; request.key = newUserKey[1].encryptedString; diff --git a/apps/web/src/app/auth/settings/account/change-email.component.spec.ts b/apps/web/src/app/auth/settings/account/change-email.component.spec.ts index f5c0733e5b..bd0d9df9f0 100644 --- a/apps/web/src/app/auth/settings/account/change-email.component.spec.ts +++ b/apps/web/src/app/auth/settings/account/change-email.component.spec.ts @@ -89,7 +89,7 @@ describe("ChangeEmailComponent", () => { }); keyService.getOrDeriveMasterKey - .calledWith("password", "UserId") + .calledWith("password", "UserId" as UserId) .mockResolvedValue("getOrDeriveMasterKey" as any); keyService.hashMasterKey .calledWith("password", "getOrDeriveMasterKey" as any) diff --git a/apps/web/src/app/auth/settings/security/change-kdf/change-kdf-confirmation.component.ts b/apps/web/src/app/auth/settings/security/change-kdf/change-kdf-confirmation.component.ts index 0bfc46eea9..5aa8eeb907 100644 --- a/apps/web/src/app/auth/settings/security/change-kdf/change-kdf-confirmation.component.ts +++ b/apps/web/src/app/auth/settings/security/change-kdf/change-kdf-confirmation.component.ts @@ -2,14 +2,13 @@ // @ts-strict-ignore import { Component, Inject } from "@angular/core"; import { FormGroup, FormControl, Validators } from "@angular/forms"; -import { firstValueFrom, map } from "rxjs"; +import { firstValueFrom } from "rxjs"; import { ApiService } from "@bitwarden/common/abstractions/api.service"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { KdfRequest } from "@bitwarden/common/models/request/kdf.request"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; -import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { DIALOG_DATA, ToastService } from "@bitwarden/components"; import { KdfConfig, KdfType, KeyService } from "@bitwarden/key-management"; @@ -31,7 +30,6 @@ export class ChangeKdfConfirmationComponent { constructor( private apiService: ApiService, private i18nService: I18nService, - private platformUtilsService: PlatformUtilsService, private keyService: KeyService, private messagingService: MessagingService, @Inject(DIALOG_DATA) params: { kdf: KdfType; kdfConfig: KdfConfig }, @@ -58,6 +56,10 @@ export class ChangeKdfConfirmationComponent { }; private async makeKeyAndSaveAsync() { + const activeAccount = await firstValueFrom(this.accountService.activeAccount$); + if (activeAccount == null) { + throw new Error("No active account found."); + } const masterPassword = this.form.value.masterPassword; // Ensure the KDF config is valid. @@ -70,13 +72,14 @@ export class ChangeKdfConfirmationComponent { request.kdfMemory = this.kdfConfig.memory; request.kdfParallelism = this.kdfConfig.parallelism; } - const masterKey = await this.keyService.getOrDeriveMasterKey(masterPassword); + const masterKey = await this.keyService.getOrDeriveMasterKey(masterPassword, activeAccount.id); request.masterPasswordHash = await this.keyService.hashMasterKey(masterPassword, masterKey); - const email = await firstValueFrom( - this.accountService.activeAccount$.pipe(map((a) => a?.email)), - ); - const newMasterKey = await this.keyService.makeMasterKey(masterPassword, email, this.kdfConfig); + const newMasterKey = await this.keyService.makeMasterKey( + masterPassword, + activeAccount.email, + this.kdfConfig, + ); request.newMasterPasswordHash = await this.keyService.hashMasterKey( masterPassword, newMasterKey, diff --git a/libs/key-management/src/abstractions/key.service.ts b/libs/key-management/src/abstractions/key.service.ts index 3c0d6c8a13..3e2fbf6c63 100644 --- a/libs/key-management/src/abstractions/key.service.ts +++ b/libs/key-management/src/abstractions/key.service.ts @@ -163,11 +163,14 @@ export abstract class KeyService { */ abstract clearStoredUserKey(keySuffix: KeySuffixOptions, userId: string): Promise; /** - * @throws Error when userId is null and no active user + * Retrieves the user's master key if it is in state, or derives it from the provided password * @param password The user's master password that will be used to derive a master key if one isn't found * @param userId The desired user + * @throws Error when userId is null/undefined. + * @throws Error when email or Kdf configuration cannot be found for the user. + * @returns The user's master key if it exists, or a newly derived master key. */ - abstract getOrDeriveMasterKey(password: string, userId?: string): Promise; + abstract getOrDeriveMasterKey(password: string, userId: UserId): Promise; /** * Generates a master key from the provided password * @param password The user's master password @@ -175,7 +178,7 @@ export abstract class KeyService { * @param KdfConfig The user's key derivation function configuration * @returns A master key derived from the provided password */ - abstract makeMasterKey(password: string, email: string, KdfConfig: KdfConfig): Promise; + abstract makeMasterKey(password: string, email: string, kdfConfig: KdfConfig): Promise; /** * Encrypts the existing (or provided) user key with the * provided master key @@ -191,24 +194,25 @@ export abstract class KeyService { * Creates a master password hash from the user's master password. Can * be used for local authentication or for server authentication depending * on the hashPurpose provided. - * @throws Error when password is null or key is null and no active user or active user have no master key * @param password The user's master password * @param key The user's master key or active's user master key. - * @param hashPurpose The iterations to use for the hash + * @param hashPurpose The iterations to use for the hash. Defaults to {@link HashPurpose.ServerAuthorization}. + * @throws Error when password is null/undefined or key is null/undefined. * @returns The user's master password hash */ abstract hashMasterKey( password: string, - key: MasterKey | null, + key: MasterKey, hashPurpose?: HashPurpose, ): Promise; /** * Compares the provided master password to the stored password hash. * @param masterPassword The user's master password - * @param key The user's master key + * @param masterKey The user's master key * @param userId The id of the user to do the operation for. - * @returns True if the provided master password matches either the stored - * key hash or the server key hash + * @throws Error when master key is null/undefined. + * @returns True if the derived master password hash matches the stored + * key hash, false otherwise. */ abstract compareKeyHash( masterPassword: string, diff --git a/libs/key-management/src/key.service.spec.ts b/libs/key-management/src/key.service.spec.ts index 7a033792c7..27d838c6fb 100644 --- a/libs/key-management/src/key.service.spec.ts +++ b/libs/key-management/src/key.service.spec.ts @@ -18,7 +18,7 @@ import { KeyGenerationService } from "@bitwarden/common/platform/abstractions/ke import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { StateService } from "@bitwarden/common/platform/abstractions/state.service"; -import { KeySuffixOptions } from "@bitwarden/common/platform/enums"; +import { HashPurpose, KeySuffixOptions } from "@bitwarden/common/platform/enums"; import { Encrypted } from "@bitwarden/common/platform/interfaces/encrypted"; import { Utils } from "@bitwarden/common/platform/misc/utils"; import { SymmetricCryptoKey } from "@bitwarden/common/platform/models/domain/symmetric-crypto-key"; @@ -47,6 +47,7 @@ import { UserKey, MasterKey } from "@bitwarden/common/types/key"; import { KdfConfigService } from "./abstractions/kdf-config.service"; import { UserPrivateKeyDecryptionFailedError } from "./abstractions/key.service"; import { DefaultKeyService } from "./key.service"; +import { KdfConfig } from "./models/kdf-config"; describe("keyService", () => { let keyService: DefaultKeyService; @@ -817,55 +818,160 @@ describe("keyService", () => { }); describe("getOrDeriveMasterKey", () => { + beforeEach(() => { + masterPasswordService.masterKeySubject.next(null); + }); + + test.each([null as unknown as UserId, undefined as unknown as UserId])( + "throws when the provided userId is %s", + async (userId) => { + await expect(keyService.getOrDeriveMasterKey("password", userId)).rejects.toThrow( + "User ID is required.", + ); + }, + ); + it("returns the master key if it is already available", async () => { - const getMasterKey = jest - .spyOn(masterPasswordService, "masterKey$") - .mockReturnValue(of("masterKey" as any)); + const masterKey = makeSymmetricCryptoKey(32) as MasterKey; + masterPasswordService.masterKeySubject.next(masterKey); const result = await keyService.getOrDeriveMasterKey("password", mockUserId); - expect(getMasterKey).toHaveBeenCalledWith(mockUserId); - expect(result).toEqual("masterKey"); + expect(kdfConfigService.getKdfConfig$).not.toHaveBeenCalledWith(mockUserId); + expect(result).toEqual(masterKey); }); - it("derives the master key if it is not available", async () => { - const getMasterKey = jest - .spyOn(masterPasswordService, "masterKey$") - .mockReturnValue(of(null as any)); + it("throws an error if user's email is not available", async () => { + accountService.accounts$ = of({}); - const deriveKeyFromPassword = jest - .spyOn(keyGenerationService, "deriveKeyFromPassword") - .mockResolvedValue("mockMasterKey" as any); - - kdfConfigService.getKdfConfig$.mockReturnValue(of("mockKdfConfig" as any)); - - const result = await keyService.getOrDeriveMasterKey("password", mockUserId); - - expect(getMasterKey).toHaveBeenCalledWith(mockUserId); - expect(deriveKeyFromPassword).toHaveBeenCalledWith("password", "email", "mockKdfConfig"); - expect(result).toEqual("mockMasterKey"); - }); - - it("throws an error if no user is found", async () => { - accountService.activeAccountSubject.next(null); - - await expect(keyService.getOrDeriveMasterKey("password")).rejects.toThrow("No user found"); + await expect(keyService.getOrDeriveMasterKey("password", mockUserId)).rejects.toThrow( + "No email found for user " + mockUserId, + ); + expect(kdfConfigService.getKdfConfig$).not.toHaveBeenCalled(); }); it("throws an error if no kdf config is found", async () => { - jest.spyOn(masterPasswordService, "masterKey$").mockReturnValue(of(null as any)); kdfConfigService.getKdfConfig$.mockReturnValue(of(null)); await expect(keyService.getOrDeriveMasterKey("password", mockUserId)).rejects.toThrow( "No kdf found for user", ); }); + + it("derives the master key if it is not available", async () => { + keyGenerationService.deriveKeyFromPassword.mockReturnValue("mockMasterKey" as any); + kdfConfigService.getKdfConfig$.mockReturnValue(of("mockKdfConfig" as any)); + + const result = await keyService.getOrDeriveMasterKey("password", mockUserId); + + expect(kdfConfigService.getKdfConfig$).toHaveBeenCalledWith(mockUserId); + expect(keyGenerationService.deriveKeyFromPassword).toHaveBeenCalledWith( + "password", + "email", + "mockKdfConfig", + ); + expect(result).toEqual("mockMasterKey"); + }); + }); + + describe("makeMasterKey", () => { + const password = "testPassword"; + let email = "test@example.com"; + const masterKey = makeSymmetricCryptoKey(32) as MasterKey; + const kdfConfig = mock(); + + it("derives a master key from password and email", async () => { + keyGenerationService.deriveKeyFromPassword.mockResolvedValue(masterKey); + + const result = await keyService.makeMasterKey(password, email, kdfConfig); + + expect(result).toEqual(masterKey); + }); + + it("trims and lowercases the email for key generation call", async () => { + keyGenerationService.deriveKeyFromPassword.mockResolvedValue(masterKey); + email = "TEST@EXAMPLE.COM"; + + await keyService.makeMasterKey(password, email, kdfConfig); + + expect(keyGenerationService.deriveKeyFromPassword).toHaveBeenCalledWith( + password, + email.trim().toLowerCase(), + kdfConfig, + ); + }); + + it("should log the time taken to derive the master key", async () => { + keyGenerationService.deriveKeyFromPassword.mockResolvedValue(masterKey); + jest.spyOn(Date.prototype, "getTime").mockReturnValueOnce(1000).mockReturnValueOnce(1500); + + await keyService.makeMasterKey(password, email, kdfConfig); + + expect(logService.info).toHaveBeenCalledWith("[KeyService] Deriving master key took 500ms"); + }); + }); + + describe("hashMasterKey", () => { + const password = "testPassword"; + const masterKey = makeSymmetricCryptoKey(32) as MasterKey; + + test.each([null as unknown as string, undefined as unknown as string])( + "throws when the provided password is %s", + async (password) => { + await expect(keyService.hashMasterKey(password, masterKey)).rejects.toThrow( + "password is required.", + ); + }, + ); + + test.each([null as unknown as MasterKey, undefined as unknown as MasterKey])( + "throws when the provided key is %s", + async (key) => { + await expect(keyService.hashMasterKey("password", key)).rejects.toThrow("key is required."); + }, + ); + + it("hashes master key with default iterations when no hashPurpose is provided", async () => { + const mockReturnedHashB64 = "bXlfaGFzaA=="; + cryptoFunctionService.pbkdf2.mockResolvedValue(Utils.fromB64ToArray(mockReturnedHashB64)); + + const result = await keyService.hashMasterKey(password, masterKey); + + expect(cryptoFunctionService.pbkdf2).toHaveBeenCalledWith( + masterKey.inner().encryptionKey, + password, + "sha256", + 1, + ); + expect(result).toBe(mockReturnedHashB64); + }); + + test.each([ + [2, HashPurpose.LocalAuthorization], + [1, HashPurpose.ServerAuthorization], + ])( + "hashes master key with %s iterations when hashPurpose is %s", + async (expectedIterations, hashPurpose) => { + const mockReturnedHashB64 = "bXlfaGFzaA=="; + cryptoFunctionService.pbkdf2.mockResolvedValue(Utils.fromB64ToArray(mockReturnedHashB64)); + + const result = await keyService.hashMasterKey(password, masterKey, hashPurpose); + + expect(cryptoFunctionService.pbkdf2).toHaveBeenCalledWith( + masterKey.inner().encryptionKey, + password, + "sha256", + expectedIterations, + ); + expect(result).toBe(mockReturnedHashB64); + }, + ); }); describe("compareKeyHash", () => { type TestCase = { masterKey: MasterKey; - masterPassword: string | null; + masterPassword: string; storedMasterKeyHash: string | null; mockReturnedHash: string; expectedToMatch: boolean; @@ -873,26 +979,33 @@ describe("keyService", () => { const data: TestCase[] = [ { - masterKey: makeSymmetricCryptoKey(64), + masterKey: makeSymmetricCryptoKey(32), masterPassword: "my_master_password", storedMasterKeyHash: "bXlfaGFzaA==", mockReturnedHash: "bXlfaGFzaA==", expectedToMatch: true, }, { - masterKey: makeSymmetricCryptoKey(64), - masterPassword: null, + masterKey: makeSymmetricCryptoKey(32), + masterPassword: null as unknown as string, storedMasterKeyHash: "bXlfaGFzaA==", mockReturnedHash: "bXlfaGFzaA==", expectedToMatch: false, }, { - masterKey: makeSymmetricCryptoKey(64), - masterPassword: null, + masterKey: makeSymmetricCryptoKey(32), + masterPassword: null as unknown as string, storedMasterKeyHash: null, mockReturnedHash: "bXlfaGFzaA==", expectedToMatch: false, }, + { + masterKey: makeSymmetricCryptoKey(32), + masterPassword: "my_master_password", + storedMasterKeyHash: "bXlfaGFzaA==", + mockReturnedHash: "zxccbXlfaGFzaA==", + expectedToMatch: false, + }, ]; it.each(data)( @@ -907,7 +1020,7 @@ describe("keyService", () => { masterPasswordService.masterKeyHashSubject.next(storedMasterKeyHash); cryptoFunctionService.pbkdf2 - .calledWith(masterKey.inner().encryptionKey, masterPassword as string, "sha256", 2) + .calledWith(masterKey.inner().encryptionKey, masterPassword, "sha256", 2) .mockResolvedValue(Utils.fromB64ToArray(mockReturnedHash)); const actualDidMatch = await keyService.compareKeyHash( @@ -919,6 +1032,38 @@ describe("keyService", () => { expect(actualDidMatch).toBe(expectedToMatch); }, ); + + test.each([null as unknown as MasterKey, undefined as unknown as MasterKey])( + "throws an error if masterKey is %s", + async (masterKey) => { + await expect( + keyService.compareKeyHash("my_master_password", masterKey, mockUserId), + ).rejects.toThrow("'masterKey' is required to be non-null."); + }, + ); + + test.each([null as unknown as string, undefined as unknown as string])( + "returns false when masterPassword is %s", + async (masterPassword) => { + const result = await keyService.compareKeyHash( + masterPassword, + makeSymmetricCryptoKey(32), + mockUserId, + ); + expect(result).toBe(false); + }, + ); + + it("returns false when storedMasterKeyHash is null", async () => { + masterPasswordService.masterKeyHashSubject.next(null); + + const result = await keyService.compareKeyHash( + "my_master_password", + makeSymmetricCryptoKey(32), + mockUserId, + ); + expect(result).toBe(false); + }); }); describe("userPrivateKey$", () => { diff --git a/libs/key-management/src/key.service.ts b/libs/key-management/src/key.service.ts index 0f4b101d9b..7cdc104c36 100644 --- a/libs/key-management/src/key.service.ts +++ b/libs/key-management/src/key.service.ts @@ -259,28 +259,28 @@ export class DefaultKeyService implements KeyServiceAbstraction { } } - // TODO: Move to MasterPasswordService - async getOrDeriveMasterKey(password: string, userId?: UserId) { - const [resolvedUserId, email] = await firstValueFrom( - combineLatest([this.accountService.activeAccount$, this.accountService.accounts$]).pipe( - map(([activeAccount, accounts]) => { - userId ??= activeAccount?.id; - if (userId == null || accounts[userId] == null) { - throw new Error("No user found"); - } - return [userId, accounts[userId].email]; - }), - ), - ); - const masterKey = await firstValueFrom(this.masterPasswordService.masterKey$(resolvedUserId)); + async getOrDeriveMasterKey(password: string, userId: UserId): Promise { + if (userId == null) { + throw new Error("User ID is required."); + } + + const masterKey = await firstValueFrom(this.masterPasswordService.masterKey$(userId)); if (masterKey != null) { return masterKey; } - const kdf = await firstValueFrom(this.kdfConfigService.getKdfConfig$(resolvedUserId)); - if (kdf == null) { - throw new Error("No kdf found for user"); + const email = await firstValueFrom( + this.accountService.accounts$.pipe(map((accounts) => accounts[userId]?.email)), + ); + if (email == null) { + throw new Error("No email found for user " + userId); } + + const kdf = await firstValueFrom(this.kdfConfigService.getKdfConfig$(userId)); + if (kdf == null) { + throw new Error("No kdf found for user " + userId); + } + return await this.makeMasterKey(password, email, kdf); } @@ -289,14 +289,14 @@ export class DefaultKeyService implements KeyServiceAbstraction { * * @remarks * Does not validate the kdf config to ensure it satisfies the minimum requirements for the given kdf type. - * TODO: Move to MasterPasswordService */ - async makeMasterKey(password: string, email: string, KdfConfig: KdfConfig): Promise { + async makeMasterKey(password: string, email: string, kdfConfig: KdfConfig): Promise { const start = new Date().getTime(); + email = email.trim().toLowerCase(); const masterKey = (await this.keyGenerationService.deriveKeyFromPassword( password, email, - KdfConfig, + kdfConfig, )) as MasterKey; const end = new Date().getTime(); this.logService.info(`[KeyService] Deriving master key took ${end - start}ms`); @@ -312,23 +312,16 @@ export class DefaultKeyService implements KeyServiceAbstraction { return await this.buildProtectedSymmetricKey(masterKey, userKey); } - // TODO: move to MasterPasswordService async hashMasterKey( password: string, - key: MasterKey | null, + key: MasterKey, hashPurpose?: HashPurpose, ): Promise { - if (key == null) { - const userId = await firstValueFrom(this.stateProvider.activeUserId$); - if (userId == null) { - throw new Error("No active user found."); - } - - key = await firstValueFrom(this.masterPasswordService.masterKey$(userId)); + if (password == null) { + throw new Error("password is required."); } - - if (password == null || key == null) { - throw new Error("Invalid parameters."); + if (key == null) { + throw new Error("key is required."); } const iterations = hashPurpose === HashPurpose.LocalAuthorization ? 2 : 1; @@ -341,9 +334,8 @@ export class DefaultKeyService implements KeyServiceAbstraction { return Utils.fromBufferToB64(hash); } - // TODO: move to MasterPasswordService async compareKeyHash( - masterPassword: string | null, + masterPassword: string, masterKey: MasterKey, userId: UserId, ): Promise {