bitwarden/browser
1
0
Fork 0
mirror of https://github.com/bitwarden/browser synced 2025-12-24 04:04:24 +00:00
Code Issues Releases Wiki Activity

[PM-24128] New Pin service, using PasswordProtectedKeyEnvelope (#15863)

Browse Source 
* fix: broken SDK interface

* Fix all compile errors related to uuids

* Update usages of sdk to type-safe SDK type

* Update sdk version

* Update to "toSdk"

* Move pin service to km ownership

* Run format

* Eslint

* Fix tsconfig

* Fix imports and test

* Clean up imports

* Pin tmp

* Initial version of updated pin service

* Add tests

* Rename function

* Clean up logging

* Fix imports

* Fix cli build

* Fix browser desktop

* Fix tests

* Attempt to fix

* Fix build

* Fix tests

* Fix browser build

* Add missing empty line

* Fix linting

* Remove non-required change

* Missing newline

* Re-add comment

* Undo change to file

* Fix missing empty line

* Cleanup

* Cleanup

* Cleanup

* Cleanup

* Switch to replaysubject

* Add comments

* Fix tests

* Run prettier

* Undo change

* Fix browser

* Fix circular dependency on browser

* Add missing clear ephemeral pin

* Address feedback

* Update docs

* Simplify sdk usage in pin service

* Replace with mock sdk

* Update sdk

* Initialize pin service via unlock instead of listening to keyservice

* Cleanup

* Fix test

* Prevent race condition with userkey not being set

* Filter null userkeys

* [PM-24124] Pin State Service (#16641)

* add pin-state.service

* add remaining tests

* improve description for clearEphemeralPinState

* rename getUserKeyWrappedPin$ to userKeyWrappedPin$

* drop temp variable in setPinState

* add new test and remove copied one

* Fix dep cycle

* Fix tests and remaining build issues

* Fix cli build

* Add comments about functions not being public API

---------

Co-authored-by: Andreas Coroiu <andreas.coroiu@gmail.com>
Co-authored-by: Hinton <hinton@users.noreply.github.com>
Co-authored-by: Jake Fink <jfink@bitwarden.com>
This commit is contained in:
Bernd Schoolmann
2025-10-17 16:30:29 +02:00
committed by GitHub
parent 7015663c38
commit a860f218bd
33 changed files with 1610 additions and 1029 deletions
Download Patch File Download Diff File Expand all files Collapse all files

12
libs/key-management/src/abstractions/key.service.ts
View File

@@ -142,11 +142,10 @@ export abstract class KeyService {
abstract makeUserKeyV1(): Promise<UserKey>;
/**
* Clears the user's stored version of the user key
* @param keySuffix The desired version of the key to clear
* @param userId The desired user
* @throws Error when userId is null or undefined.
*/
abstract clearStoredUserKey(keySuffix: KeySuffixOptions, userId: string): Promise<void>;
abstract clearStoredUserKey(userId: string): Promise<void>;
/**
* 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
@@ -228,6 +227,7 @@ export abstract class KeyService {
* @deprecated Use {@link orgKeys$} with a required {@link UserId} instead.
*/
abstract activeUserOrgKeys$: Observable<Record<OrganizationId, OrgKey>>;
/**
* Returns the organization's symmetric key
* @deprecated Use the observable userOrgKeys$ and `map` to the desired {@link OrgKey} instead
@@ -345,14 +345,6 @@ export abstract class KeyService {
* @throws If the provided key is a null-ish value.
*/
abstract makeKeyPair(key: SymmetricCryptoKey): Promise<[string, EncString]>;
/**
* Clears the user's pin keys from storage
* Note: This will remove the stored pin and as a result,
* disable pin protection for the user
* @param userId The desired user
* @throws Error when provided userId is null or undefined
*/
abstract clearPinKeys(userId: UserId): Promise<void>;
/**
* @param keyMaterial The key material to derive the send key from
* @returns A new send key

88
libs/key-management/src/key.service.spec.ts
View File

@@ -10,7 +10,6 @@ import {
EncryptedString,
} from "@bitwarden/common/key-management/crypto/models/enc-string";
import { FakeMasterPasswordService } from "@bitwarden/common/key-management/master-password/services/fake-master-password.service";
import { PinServiceAbstraction } from "@bitwarden/common/key-management/pin/pin.service.abstraction";
import { UnsignedPublicKey, WrappedSigningKey } from "@bitwarden/common/key-management/types";
import { VaultTimeoutStringType } from "@bitwarden/common/key-management/vault-timeout";
import { VAULT_TIMEOUT } from "@bitwarden/common/key-management/vault-timeout/services/vault-timeout-settings.state";
@@ -57,7 +56,6 @@ import { KdfConfig } from "./models/kdf-config";
describe("keyService", () => {
let keyService: DefaultKeyService;
const pinService = mock<PinServiceAbstraction>();
const keyGenerationService = mock<KeyGenerationService>();
const cryptoFunctionService = mock<CryptoFunctionService>();
const encryptService = mock<EncryptService>();
@@ -77,7 +75,6 @@ describe("keyService", () => {
stateProvider = new FakeStateProvider(accountService);
keyService = new DefaultKeyService(
pinService,
masterPasswordService,
keyGenerationService,
cryptoFunctionService,
@@ -256,54 +253,6 @@ describe("keyService", () => {
"No userId provided.",
);
});
describe("Pin Key refresh", () => {
const mockPinKeyEncryptedUserKey = new EncString(
"2.AAAw2vTUePO+CCyokcIfVw==|DTBNlJ5yVsV2Bsk3UU3H6Q==|YvFBff5gxWqM+UsFB6BKimKxhC32AtjF3IStpU1Ijwg=",
);
const mockUserKeyEncryptedPin = new EncString(
"2.BBBw2vTUePO+CCyokcIfVw==|DTBNlJ5yVsV2Bsk3UU3H6Q==|YvFBff5gxWqM+UsFB6BKimKxhC32AtjF3IStpU1Ijwg=",
);
it("sets a pinKeyEncryptedUserKeyPersistent if a userKeyEncryptedPin and pinKeyEncryptedUserKey is set", async () => {
pinService.createPinKeyEncryptedUserKey.mockResolvedValue(mockPinKeyEncryptedUserKey);
pinService.getUserKeyEncryptedPin.mockResolvedValue(mockUserKeyEncryptedPin);
pinService.getPinKeyEncryptedUserKeyPersistent.mockResolvedValue(
mockPinKeyEncryptedUserKey,
);
await keyService.setUserKey(mockUserKey, mockUserId);
expect(pinService.storePinKeyEncryptedUserKey).toHaveBeenCalledWith(
mockPinKeyEncryptedUserKey,
false,
mockUserId,
);
});
it("sets a pinKeyEncryptedUserKeyEphemeral if a userKeyEncryptedPin is set, but a pinKeyEncryptedUserKey is not set", async () => {
pinService.createPinKeyEncryptedUserKey.mockResolvedValue(mockPinKeyEncryptedUserKey);
pinService.getUserKeyEncryptedPin.mockResolvedValue(mockUserKeyEncryptedPin);
pinService.getPinKeyEncryptedUserKeyPersistent.mockResolvedValue(null);
await keyService.setUserKey(mockUserKey, mockUserId);
expect(pinService.storePinKeyEncryptedUserKey).toHaveBeenCalledWith(
mockPinKeyEncryptedUserKey,
true,
mockUserId,
);
});
it("clears the pinKeyEncryptedUserKeyPersistent and pinKeyEncryptedUserKeyEphemeral if the UserKeyEncryptedPin is not set", async () => {
pinService.getUserKeyEncryptedPin.mockResolvedValue(null);
await keyService.setUserKey(mockUserKey, mockUserId);
expect(pinService.clearPinKeyEncryptedUserKeyPersistent).toHaveBeenCalledWith(mockUserId);
expect(pinService.clearPinKeyEncryptedUserKeyEphemeral).toHaveBeenCalledWith(mockUserId);
});
});
});
describe("setUserKeys", () => {
@@ -388,36 +337,22 @@ describe("keyService", () => {
const invalidUserIdTestCases = [
{ keySuffix: KeySuffixOptions.Auto, userId: null as unknown as UserId },
{ keySuffix: KeySuffixOptions.Auto, userId: undefined as unknown as UserId },
{ keySuffix: KeySuffixOptions.Pin, userId: null as unknown as UserId },
{ keySuffix: KeySuffixOptions.Pin, userId: undefined as unknown as UserId },
];
test.each(invalidUserIdTestCases)(
"throws when keySuffix is $keySuffix and userId is $userId",
async ({ keySuffix, userId }) => {
await expect(keyService.clearStoredUserKey(keySuffix, userId)).rejects.toThrow(
"UserId is required",
);
await expect(keyService.clearStoredUserKey(userId)).rejects.toThrow("UserId is required");
},
);
});
describe("with Auto key suffix", () => {
it("UserKeyAutoUnlock is cleared and pin keys are not cleared", async () => {
await keyService.clearStoredUserKey(KeySuffixOptions.Auto, mockUserId);
await keyService.clearStoredUserKey(mockUserId);
expect(stateService.setUserKeyAutoUnlock).toHaveBeenCalledWith(null, {
userId: mockUserId,
});
expect(pinService.clearPinKeyEncryptedUserKeyEphemeral).not.toHaveBeenCalled();
});
});
describe("with PIN key suffix", () => {
it("pin keys are cleared and user key auto unlock not", async () => {
await keyService.clearStoredUserKey(KeySuffixOptions.Pin, mockUserId);
expect(stateService.setUserKeyAutoUnlock).not.toHaveBeenCalled();
expect(pinService.clearPinKeyEncryptedUserKeyEphemeral).toHaveBeenCalledWith(mockUserId);
});
});
});
@@ -448,24 +383,6 @@ describe("keyService", () => {
});
});
describe("clearPinKeys", () => {
test.each([null as unknown as UserId, undefined as unknown as UserId])(
"throws when the provided userId is %s",
async (userId) => {
await expect(keyService.clearPinKeys(userId)).rejects.toThrow("UserId is required");
},
);
it("calls pin service to clear", async () => {
const userId = "someOtherUser" as UserId;
await keyService.clearPinKeys(userId);
expect(pinService.clearPinKeyEncryptedUserKeyPersistent).toHaveBeenCalledWith(userId);
expect(pinService.clearPinKeyEncryptedUserKeyEphemeral).toHaveBeenCalledWith(userId);
expect(pinService.clearUserKeyEncryptedPin).toHaveBeenCalledWith(userId);
});
});
describe("userPrivateKey$", () => {
let mockUserKey: UserKey;
let mockUserPrivateKey: Uint8Array;
@@ -1262,7 +1179,6 @@ describe("keyService", () => {
expect(result).toEqual(mockUserKey);
expect(validateUserKeySpy).toHaveBeenCalledWith(mockUserKey, mockUserId);
expect(logService.warning).toHaveBeenCalledWith("Invalid key, throwing away stored keys");
expect(pinService.clearPinKeyEncryptedUserKeyEphemeral).toHaveBeenCalledWith(mockUserId);
expect(stateService.setUserKeyAutoUnlock).toHaveBeenCalledWith(null, {
userId: mockUserId,
});

61
libs/key-management/src/key.service.ts
View File

@@ -27,7 +27,6 @@ import {
EncryptedString,
} from "@bitwarden/common/key-management/crypto/models/enc-string";
import { InternalMasterPasswordServiceAbstraction } from "@bitwarden/common/key-management/master-password/abstractions/master-password.service.abstraction";
import { PinServiceAbstraction } from "@bitwarden/common/key-management/pin/pin.service.abstraction";
import { WrappedSigningKey } from "@bitwarden/common/key-management/types";
import { VaultTimeoutStringType } from "@bitwarden/common/key-management/vault-timeout";
import { VAULT_TIMEOUT } from "@bitwarden/common/key-management/vault-timeout/services/vault-timeout-settings.state";
@@ -72,7 +71,6 @@ export class DefaultKeyService implements KeyServiceAbstraction {
readonly activeUserOrgKeys$: Observable<Record<OrganizationId, OrgKey>>;
constructor(
protected pinService: PinServiceAbstraction,
protected masterPasswordService: InternalMasterPasswordServiceAbstraction,
protected keyGenerationService: KeyGenerationService,
protected cryptoFunctionService: CryptoFunctionService,
@@ -105,6 +103,13 @@ export class DefaultKeyService implements KeyServiceAbstraction {
await this.stateProvider.setUserState(USER_EVER_HAD_USER_KEY, true, userId);
await this.storeAdditionalKeys(key, userId);
// Await the key actually being set. This ensures that any subsequent callers know the key is already in state.
// There were bugs related to the stateprovider observables in the past that caused issues around this.
const userKey = await firstValueFrom(this.userKey$(userId).pipe(filter((k) => k != null)));
if (userKey == null) {
throw new Error("Failed to set user key");
}
}
async setUserKeys(
@@ -223,17 +228,12 @@ export class DefaultKeyService implements KeyServiceAbstraction {
await this.clearAllStoredUserKeys(userId);
}
async clearStoredUserKey(keySuffix: KeySuffixOptions, userId: UserId): Promise<void> {
async clearStoredUserKey(userId: UserId): Promise<void> {
if (userId == null) {
throw new Error("UserId is required");
}
if (keySuffix === KeySuffixOptions.Auto) {
await this.stateService.setUserKeyAutoUnlock(null, { userId: userId });
}
if (keySuffix === KeySuffixOptions.Pin) {
await this.pinService.clearPinKeyEncryptedUserKeyEphemeral(userId);
}
await this.stateService.setUserKeyAutoUnlock(null, { userId: userId });
}
/**
@@ -513,16 +513,6 @@ export class DefaultKeyService implements KeyServiceAbstraction {
await this.stateProvider.setUserState(USER_KEY_ENCRYPTED_SIGNING_KEY, null, userId);
}
async clearPinKeys(userId: UserId): Promise<void> {
if (userId == null) {
throw new Error("UserId is required");
}
await this.pinService.clearPinKeyEncryptedUserKeyPersistent(userId);
await this.pinService.clearPinKeyEncryptedUserKeyEphemeral(userId);
await this.pinService.clearUserKeyEncryptedPin(userId);
}
async makeSendKey(keyMaterial: CsprngArray): Promise<SymmetricCryptoKey> {
return await this.keyGenerationService.deriveKeyFromMaterial(
keyMaterial,
@@ -546,7 +536,6 @@ export class DefaultKeyService implements KeyServiceAbstraction {
await this.clearProviderKeys(userId);
await this.clearKeyPair(userId);
await this.clearSigningKey(userId);
await this.clearPinKeys(userId);
await this.stateProvider.setUserState(USER_EVER_HAD_USER_KEY, null, userId);
}
@@ -678,32 +667,6 @@ export class DefaultKeyService implements KeyServiceAbstraction {
} else {
await this.stateService.setUserKeyAutoUnlock(null, { userId: userId });
}
const storePin = await this.shouldStoreKey(KeySuffixOptions.Pin, userId);
if (storePin) {
// Decrypt userKeyEncryptedPin with user key
const pin = await this.encryptService.decryptString(
(await this.pinService.getUserKeyEncryptedPin(userId))!,
key,
);
const pinKeyEncryptedUserKey = await this.pinService.createPinKeyEncryptedUserKey(
pin,
key,
userId,
);
const noPreExistingPersistentKey =
(await this.pinService.getPinKeyEncryptedUserKeyPersistent(userId)) == null;
await this.pinService.storePinKeyEncryptedUserKey(
pinKeyEncryptedUserKey,
noPreExistingPersistentKey,
userId,
);
} else {
await this.pinService.clearPinKeyEncryptedUserKeyPersistent(userId);
await this.pinService.clearPinKeyEncryptedUserKeyEphemeral(userId);
}
}
protected async shouldStoreKey(keySuffix: KeySuffixOptions, userId: UserId) {
@@ -720,11 +683,6 @@ export class DefaultKeyService implements KeyServiceAbstraction {
shouldStoreKey = vaultTimeout == VaultTimeoutStringType.Never;
break;
}
case KeySuffixOptions.Pin: {
const userKeyEncryptedPin = await this.pinService.getUserKeyEncryptedPin(userId);
shouldStoreKey = !!userKeyEncryptedPin;
break;
}
}
return shouldStoreKey;
}
@@ -744,7 +702,6 @@ export class DefaultKeyService implements KeyServiceAbstraction {
protected async clearAllStoredUserKeys(userId: UserId): Promise<void> {
await this.stateService.setUserKeyAutoUnlock(null, { userId: userId });
await this.pinService.clearPinKeyEncryptedUserKeyEphemeral(userId);
}
private async hashPhrase(hash: Uint8Array, minimumEntropy = 64) {