1
0
mirror of https://github.com/bitwarden/browser synced 2025-12-15 15:53:27 +00:00

Ps/pm 5533/migrate decrypted user key (#7970)

* Move user key memory state to state providers

Note: state service observable change is because these updates are no longer internal to the class, but reporter directly to account service through crypto service on update of a user key

* remove decrypted user key state

Note, we're going to move the encrypted cryptoSymmetric key (and associated master key encrypted user keys)  as part of the master key service creation. Crypto service will no longer be responsible for the encrypted forms of user key.

* Deprecate notices belong on abstraction

* Allow for single-direction status updates

This is necessary since we don't want to have to guarantee that the update to logged out occurs after the update to locked.

* Remove deprecated subject

It turns out the set for cryptoMasterKey was also unused 🎉
This commit is contained in:
Matt Gibson
2024-02-22 15:07:26 -05:00
committed by GitHub
parent 7a9a9a0c22
commit 56bffb04bb
19 changed files with 224 additions and 183 deletions

View File

@@ -12,10 +12,13 @@ import { EncString } from "../models/domain/enc-string";
import { SymmetricCryptoKey } from "../models/domain/symmetric-crypto-key";
export abstract class CryptoService {
activeUserKey$: Observable<UserKey>;
/**
* Sets the provided user key and stores
* any other necessary versions (such as auto, biometrics,
* or pin)
*
* @throws when key is null. Use {@link clearUserKey} instead
* @param key The user key to set
* @param userId The desired user
*/

View File

@@ -16,7 +16,7 @@ import { UsernameGeneratorOptions } from "../../tools/generator/username";
import { SendData } from "../../tools/send/models/data/send.data";
import { SendView } from "../../tools/send/models/view/send.view";
import { UserId } from "../../types/guid";
import { DeviceKey, MasterKey, UserKey } from "../../types/key";
import { DeviceKey, MasterKey } from "../../types/key";
import { UriMatchType } from "../../vault/enums";
import { CipherData } from "../../vault/models/data/cipher.data";
import { LocalData } from "../../vault/models/data/local.data";
@@ -50,6 +50,9 @@ export type InitOptions = {
export abstract class StateService<T extends Account = Account> {
accounts$: Observable<{ [userId: string]: T }>;
activeAccount$: Observable<string>;
/**
* @deprecated use accountService.activeAccount$ instead
*/
activeAccountUnlocked$: Observable<boolean>;
addAccount: (account: T) => Promise<void>;
@@ -82,14 +85,6 @@ export abstract class StateService<T extends Account = Account> {
setClearClipboard: (value: number, options?: StorageOptions) => Promise<void>;
getConvertAccountToKeyConnector: (options?: StorageOptions) => Promise<boolean>;
setConvertAccountToKeyConnector: (value: boolean, options?: StorageOptions) => Promise<void>;
/**
* gets the user key
*/
getUserKey: (options?: StorageOptions) => Promise<UserKey>;
/**
* Sets the user key
*/
setUserKey: (value: UserKey, options?: StorageOptions) => Promise<void>;
/**
* Gets the user's master key
*/
@@ -150,10 +145,6 @@ export abstract class StateService<T extends Account = Account> {
* @deprecated For migration purposes only, use getUserKeyMasterKey instead
*/
getEncryptedCryptoSymmetricKey: (options?: StorageOptions) => Promise<string>;
/**
* @deprecated For migration purposes only, use setUserKeyMasterKey instead
*/
setEncryptedCryptoSymmetricKey: (value: string, options?: StorageOptions) => Promise<void>;
/**
* @deprecated For legacy purposes only, use getMasterKey instead
*/

View File

@@ -19,7 +19,7 @@ import { UsernameGeneratorOptions } from "../../../tools/generator/username/user
import { SendData } from "../../../tools/send/models/data/send.data";
import { SendView } from "../../../tools/send/models/view/send.view";
import { DeepJsonify } from "../../../types/deep-jsonify";
import { MasterKey, UserKey } from "../../../types/key";
import { MasterKey } from "../../../types/key";
import { UriMatchType } from "../../../vault/enums";
import { CipherData } from "../../../vault/models/data/cipher.data";
import { CipherView } from "../../../vault/models/view/cipher.view";
@@ -113,7 +113,6 @@ export class AccountData {
}
export class AccountKeys {
userKey?: UserKey;
masterKey?: MasterKey;
masterKeyEncryptedUserKey?: string;
deviceKey?: ReturnType<SymmetricCryptoKey["toJSON"]>;
@@ -146,7 +145,6 @@ export class AccountKeys {
return null;
}
return Object.assign(new AccountKeys(), obj, {
userKey: SymmetricCryptoKey.fromJSON(obj?.userKey),
masterKey: SymmetricCryptoKey.fromJSON(obj?.masterKey),
deviceKey: obj?.deviceKey,
cryptoMasterKey: SymmetricCryptoKey.fromJSON(obj?.cryptoMasterKey),

View File

@@ -4,6 +4,7 @@ import { firstValueFrom } from "rxjs";
import { FakeAccountService, mockAccountServiceWith } from "../../../spec/fake-account-service";
import { FakeActiveUserState, FakeSingleUserState } from "../../../spec/fake-state";
import { FakeStateProvider } from "../../../spec/fake-state-provider";
import { AuthenticationStatus } from "../../auth/enums/authentication-status";
import { CsprngArray } from "../../types/csprng";
import { UserId } from "../../types/guid";
import { UserKey, MasterKey, PinKey } from "../../types/key";
@@ -17,7 +18,7 @@ import { EncString } from "../models/domain/enc-string";
import { SymmetricCryptoKey } from "../models/domain/symmetric-crypto-key";
import { CryptoService } from "../services/crypto.service";
import { USER_EVER_HAD_USER_KEY } from "./key-state/user-key.state";
import { USER_EVER_HAD_USER_KEY, USER_KEY } from "./key-state/user-key.state";
describe("cryptoService", () => {
let cryptoService: CryptoService;
@@ -57,42 +58,50 @@ describe("cryptoService", () => {
describe("getUserKey", () => {
let mockUserKey: UserKey;
let stateSvcGetUserKey: jest.SpyInstance;
beforeEach(() => {
const mockRandomBytes = new Uint8Array(64) as CsprngArray;
mockUserKey = new SymmetricCryptoKey(mockRandomBytes) as UserKey;
});
stateSvcGetUserKey = jest.spyOn(stateService, "getUserKey");
it("retrieves the key state of the requested user", async () => {
await cryptoService.getUserKey(mockUserId);
expect(stateProvider.mock.getUserState$).toHaveBeenCalledWith(USER_KEY, mockUserId);
});
it("returns the User Key if available", async () => {
stateSvcGetUserKey.mockResolvedValue(mockUserKey);
stateProvider.singleUser.getFake(mockUserId, USER_KEY).nextState(mockUserKey);
const userKey = await cryptoService.getUserKey(mockUserId);
expect(stateSvcGetUserKey).toHaveBeenCalledWith({ userId: mockUserId });
expect(userKey).toEqual(mockUserKey);
});
it("sets the Auto key if the User Key if not set", async () => {
it("sets from the Auto key if the User Key if not set", async () => {
const autoKeyB64 =
"IT5cA1i5Hncd953pb00E58D2FqJX+fWTj4AvoI67qkGHSQPgulAqKv+LaKRAo9Bg0xzP9Nw00wk4TqjMmGSM+g==";
stateService.getUserKeyAutoUnlock.mockResolvedValue(autoKeyB64);
const setKeySpy = jest.spyOn(cryptoService, "setUserKey");
const userKey = await cryptoService.getUserKey(mockUserId);
expect(stateService.setUserKey).toHaveBeenCalledWith(expect.any(SymmetricCryptoKey), {
userId: mockUserId,
});
expect(setKeySpy).toHaveBeenCalledWith(expect.any(SymmetricCryptoKey), mockUserId);
expect(setKeySpy).toHaveBeenCalledTimes(1);
expect(userKey.keyB64).toEqual(autoKeyB64);
});
it("returns nullish if there is no auto key and the user key is not set", async () => {
const userKey = await cryptoService.getUserKey(mockUserId);
expect(userKey).toBeFalsy();
});
});
describe("getUserKeyWithLegacySupport", () => {
let mockUserKey: UserKey;
let mockMasterKey: MasterKey;
let stateSvcGetUserKey: jest.SpyInstance;
let stateSvcGetMasterKey: jest.SpyInstance;
beforeEach(() => {
@@ -100,23 +109,22 @@ describe("cryptoService", () => {
mockUserKey = new SymmetricCryptoKey(mockRandomBytes) as UserKey;
mockMasterKey = new SymmetricCryptoKey(new Uint8Array(64) as CsprngArray) as MasterKey;
stateSvcGetUserKey = jest.spyOn(stateService, "getUserKey");
stateSvcGetMasterKey = jest.spyOn(stateService, "getMasterKey");
});
it("returns the User Key if available", async () => {
stateSvcGetUserKey.mockResolvedValue(mockUserKey);
stateProvider.singleUser.getFake(mockUserId, USER_KEY).nextState(mockUserKey);
const getKeySpy = jest.spyOn(cryptoService, "getUserKey");
const userKey = await cryptoService.getUserKeyWithLegacySupport(mockUserId);
expect(stateSvcGetUserKey).toHaveBeenCalledWith({ userId: mockUserId });
expect(getKeySpy).toHaveBeenCalledWith(mockUserId);
expect(stateSvcGetMasterKey).not.toHaveBeenCalled();
expect(userKey).toEqual(mockUserKey);
});
it("returns the user's master key when User Key is not available", async () => {
stateSvcGetUserKey.mockResolvedValue(null);
stateSvcGetMasterKey.mockResolvedValue(mockMasterKey);
const userKey = await cryptoService.getUserKeyWithLegacySupport(mockUserId);
@@ -201,6 +209,19 @@ describe("cryptoService", () => {
});
});
it("throws if key is null", async () => {
await expect(cryptoService.setUserKey(null, mockUserId)).rejects.toThrow("No key provided.");
});
it("should update the user's lock state", async () => {
await cryptoService.setUserKey(mockUserKey, mockUserId);
expect(accountService.mock.setAccountStatus).toHaveBeenCalledWith(
mockUserId,
AuthenticationStatus.Unlocked,
);
});
describe("Pin Key refresh", () => {
let cryptoSvcMakePinKey: jest.SpyInstance;
const protectedPin =
@@ -259,4 +280,36 @@ describe("cryptoService", () => {
});
});
});
describe("clearUserKey", () => {
it.each([mockUserId, null])("should clear the User Key for id %2", async (userId) => {
await cryptoService.clearUserKey(false, userId);
expect(stateProvider.mock.setUserState).toHaveBeenCalledWith(USER_KEY, null, userId);
});
it("should update status to locked", async () => {
await cryptoService.clearUserKey(false, mockUserId);
expect(accountService.mock.setMaxAccountStatus).toHaveBeenCalledWith(
mockUserId,
AuthenticationStatus.Locked,
);
});
it.each([true, false])(
"should clear stored user keys if clearAll is true (%s)",
async (clear) => {
const clearSpy = (cryptoService["clearAllStoredUserKeys"] = jest.fn());
await cryptoService.clearUserKey(clear, mockUserId);
if (clear) {
expect(clearSpy).toHaveBeenCalledWith(mockUserId);
expect(clearSpy).toHaveBeenCalledTimes(1);
} else {
expect(clearSpy).not.toHaveBeenCalled();
}
},
);
});
});

View File

@@ -6,6 +6,7 @@ import { ProfileOrganizationResponse } from "../../admin-console/models/response
import { ProfileProviderOrganizationResponse } from "../../admin-console/models/response/profile-provider-organization.response";
import { ProfileProviderResponse } from "../../admin-console/models/response/profile-provider.response";
import { AccountService } from "../../auth/abstractions/account.service";
import { AuthenticationStatus } from "../../auth/enums/authentication-status";
import { KdfConfig } from "../../auth/models/domain/kdf-config";
import { Utils } from "../../platform/misc/utils";
import { OrganizationId, ProviderId, UserId } from "../../types/guid";
@@ -52,9 +53,11 @@ import {
USER_EVER_HAD_USER_KEY,
USER_PRIVATE_KEY,
USER_PUBLIC_KEY,
USER_KEY,
} from "./key-state/user-key.state";
export class CryptoService implements CryptoServiceAbstraction {
private readonly activeUserKeyState: ActiveUserState<UserKey>;
private readonly activeUserEverHadUserKey: ActiveUserState<boolean>;
private readonly activeUserEncryptedOrgKeysState: ActiveUserState<
Record<OrganizationId, EncryptedOrganizationKeyData>
@@ -68,6 +71,8 @@ export class CryptoService implements CryptoServiceAbstraction {
private readonly activeUserPrivateKeyState: DerivedState<UserPrivateKey>;
private readonly activeUserPublicKeyState: DerivedState<UserPublicKey>;
readonly activeUserKey$: Observable<UserKey>;
readonly activeUserOrgKeys$: Observable<Record<OrganizationId, OrgKey>>;
readonly activeUserProviderKeys$: Observable<Record<ProviderId, ProviderKey>>;
readonly activeUserPrivateKey$: Observable<UserPrivateKey>;
@@ -84,6 +89,8 @@ export class CryptoService implements CryptoServiceAbstraction {
protected stateProvider: StateProvider,
) {
// User Key
this.activeUserKeyState = stateProvider.getActive(USER_KEY);
this.activeUserKey$ = this.activeUserKeyState.state$;
this.activeUserEverHadUserKey = stateProvider.getActive(USER_EVER_HAD_USER_KEY);
this.everHadUserKey$ = this.activeUserEverHadUserKey.state$.pipe(map((x) => x ?? false));
@@ -131,13 +138,15 @@ export class CryptoService implements CryptoServiceAbstraction {
}
async setUserKey(key: UserKey, userId?: UserId): Promise<void> {
// TODO: make this non-nullable in signature
userId ??= (await firstValueFrom(this.accountService.activeAccount$))?.id;
if (key != null) {
// Key should never be null anyway
await this.stateProvider.getUser(userId, USER_EVER_HAD_USER_KEY).update(() => true);
if (key == null) {
throw new Error("No key provided. Use ClearUserKey to clear the key");
}
await this.stateService.setUserKey(key, { userId: userId });
// Set userId to ensure we have one for the account status update
[userId, key] = await this.stateProvider.setUserState(USER_KEY, key, userId);
await this.stateProvider.setUserState(USER_EVER_HAD_USER_KEY, true, userId);
await this.accountService.setAccountStatus(userId, AuthenticationStatus.Unlocked);
await this.storeAdditionalKeys(key, userId);
}
@@ -147,7 +156,7 @@ export class CryptoService implements CryptoServiceAbstraction {
}
async getUserKey(userId?: UserId): Promise<UserKey> {
let userKey = await this.stateService.getUserKey({ userId: userId });
let userKey = await firstValueFrom(this.stateProvider.getUserState$(USER_KEY, userId));
if (userKey) {
return userKey;
}
@@ -197,7 +206,7 @@ export class CryptoService implements CryptoServiceAbstraction {
}
async hasUserKeyInMemory(userId?: UserId): Promise<boolean> {
return (await this.stateService.getUserKey({ userId: userId })) != null;
return (await firstValueFrom(this.stateProvider.getUserState$(USER_KEY, userId))) != null;
}
async hasUserKeyStored(keySuffix: KeySuffixOptions, userId?: UserId): Promise<boolean> {
@@ -215,7 +224,9 @@ export class CryptoService implements CryptoServiceAbstraction {
}
async clearUserKey(clearStoredKeys = true, userId?: UserId): Promise<void> {
await this.stateService.setUserKey(null, { userId: userId });
// Set userId to ensure we have one for the account status update
[userId] = await this.stateProvider.setUserState(USER_KEY, null, userId);
await this.accountService.setMaxAccountStatus(userId, AuthenticationStatus.Locked);
if (clearStoredKeys) {
await this.clearAllStoredUserKeys(userId);
}

View File

@@ -1,8 +1,9 @@
import { UserPrivateKey, UserPublicKey } from "../../../types/key";
import { UserPrivateKey, UserPublicKey, UserKey } from "../../../types/key";
import { CryptoFunctionService } from "../../abstractions/crypto-function.service";
import { EncryptService } from "../../abstractions/encrypt.service";
import { EncString, EncryptedString } from "../../models/domain/enc-string";
import { KeyDefinition, CRYPTO_DISK, DeriveDefinition } from "../../state";
import { SymmetricCryptoKey } from "../../models/domain/symmetric-crypto-key";
import { KeyDefinition, CRYPTO_DISK, DeriveDefinition, CRYPTO_MEMORY } from "../../state";
import { CryptoService } from "../crypto.service";
export const USER_EVER_HAD_USER_KEY = new KeyDefinition<boolean>(CRYPTO_DISK, "everHadUserKey", {
@@ -57,3 +58,6 @@ export const USER_PUBLIC_KEY = DeriveDefinition.from<
return (await cryptoFunctionService.rsaExtractPublicKey(privateKey)) as UserPublicKey;
},
});
export const USER_KEY = new KeyDefinition<UserKey>(CRYPTO_MEMORY, "userKey", {
deserializer: (obj) => SymmetricCryptoKey.fromJSON(obj) as UserKey,
});

View File

@@ -1,4 +1,4 @@
import { BehaviorSubject, concatMap } from "rxjs";
import { BehaviorSubject, Observable, map } from "rxjs";
import { Jsonify, JsonValue } from "type-fest";
import { OrganizationData } from "../../admin-console/models/data/organization.data";
@@ -20,7 +20,7 @@ import { UsernameGeneratorOptions } from "../../tools/generator/username";
import { SendData } from "../../tools/send/models/data/send.data";
import { SendView } from "../../tools/send/models/view/send.view";
import { UserId } from "../../types/guid";
import { DeviceKey, MasterKey, UserKey } from "../../types/key";
import { DeviceKey, MasterKey } from "../../types/key";
import { UriMatchType } from "../../vault/enums";
import { CipherData } from "../../vault/models/data/cipher.data";
import { LocalData } from "../../vault/models/data/local.data";
@@ -87,8 +87,7 @@ export class StateService<
protected activeAccountSubject = new BehaviorSubject<string | null>(null);
activeAccount$ = this.activeAccountSubject.asObservable();
protected activeAccountUnlockedSubject = new BehaviorSubject<boolean>(false);
activeAccountUnlocked$ = this.activeAccountUnlockedSubject.asObservable();
activeAccountUnlocked$: Observable<boolean>;
private hasBeenInited = false;
protected isRecoveredSession = false;
@@ -109,22 +108,11 @@ export class StateService<
private migrationRunner: MigrationRunner,
protected useAccountCache: boolean = true,
) {
// If the account gets changed, verify the new account is unlocked
this.activeAccountSubject
.pipe(
concatMap(async (userId) => {
if (userId == null && this.activeAccountUnlockedSubject.getValue() == false) {
return;
} else if (userId == null) {
this.activeAccountUnlockedSubject.next(false);
}
// FIXME: This should be refactored into AuthService or a similar service,
// as checking for the existence of the crypto key is a low level
// implementation detail.
this.activeAccountUnlockedSubject.next((await this.getUserKey()) != null);
}),
)
.subscribe();
this.activeAccountUnlocked$ = this.accountService.activeAccount$.pipe(
map((a) => {
return a?.status === AuthenticationStatus.Unlocked;
}),
);
}
async init(initOptions: InitOptions = {}): Promise<void> {
@@ -522,68 +510,6 @@ export class StateService<
return account?.keys?.cryptoMasterKey;
}
/**
* @deprecated Do not save the Master Key. Use the User Symmetric Key instead
*/
async setCryptoMasterKey(value: SymmetricCryptoKey, options?: StorageOptions): Promise<void> {
const account = await this.getAccount(
this.reconcileOptions(options, await this.defaultInMemoryOptions()),
);
account.keys.cryptoMasterKey = value;
await this.saveAccount(
account,
this.reconcileOptions(options, await this.defaultInMemoryOptions()),
);
const nextStatus = value != null ? AuthenticationStatus.Unlocked : AuthenticationStatus.Locked;
await this.accountService.setAccountStatus(options.userId as UserId, nextStatus);
if (options.userId == this.activeAccountSubject.getValue()) {
const nextValue = value != null;
// Avoid emitting if we are already unlocked
if (this.activeAccountUnlockedSubject.getValue() != nextValue) {
this.activeAccountUnlockedSubject.next(nextValue);
}
}
}
/**
* user key used to encrypt/decrypt data
*/
async getUserKey(options?: StorageOptions): Promise<UserKey> {
const account = await this.getAccount(
this.reconcileOptions(options, await this.defaultInMemoryOptions()),
);
return account?.keys?.userKey as UserKey;
}
/**
* user key used to encrypt/decrypt data
*/
async setUserKey(value: UserKey, options?: StorageOptions): Promise<void> {
const account = await this.getAccount(
this.reconcileOptions(options, await this.defaultInMemoryOptions()),
);
account.keys.userKey = value;
await this.saveAccount(
account,
this.reconcileOptions(options, await this.defaultInMemoryOptions()),
);
const nextStatus = value != null ? AuthenticationStatus.Unlocked : AuthenticationStatus.Locked;
await this.accountService.setAccountStatus(options.userId as UserId, nextStatus);
if (options?.userId == this.activeAccountSubject.getValue()) {
const nextValue = value != null;
// Avoid emitting if we are already unlocked
if (this.activeAccountUnlockedSubject.getValue() != nextValue) {
this.activeAccountUnlockedSubject.next(nextValue);
}
}
}
/**
* User's master key derived from MP, saved only if we decrypted with MP
*/
@@ -885,33 +811,6 @@ export class StateService<
);
}
/**
* @deprecated Use UserKey instead
*/
async getDecryptedCryptoSymmetricKey(options?: StorageOptions): Promise<SymmetricCryptoKey> {
const account = await this.getAccount(
this.reconcileOptions(options, await this.defaultInMemoryOptions()),
);
return account?.keys?.cryptoSymmetricKey?.decrypted;
}
/**
* @deprecated Use UserKey instead
*/
async setDecryptedCryptoSymmetricKey(
value: SymmetricCryptoKey,
options?: StorageOptions,
): Promise<void> {
const account = await this.getAccount(
this.reconcileOptions(options, await this.defaultInMemoryOptions()),
);
account.keys.cryptoSymmetricKey.decrypted = value;
await this.saveAccount(
account,
this.reconcileOptions(options, await this.defaultInMemoryOptions()),
);
}
@withPrototypeForArrayMembers(GeneratedPasswordHistory)
async getDecryptedPasswordGenerationHistory(
options?: StorageOptions,
@@ -1565,20 +1464,6 @@ export class StateService<
)?.keys.cryptoSymmetricKey.encrypted;
}
/**
* @deprecated Use UserKey instead
*/
async setEncryptedCryptoSymmetricKey(value: string, options?: StorageOptions): Promise<void> {
const account = await this.getAccount(
this.reconcileOptions(options, await this.defaultOnDiskOptions()),
);
account.keys.cryptoSymmetricKey.encrypted = value;
await this.saveAccount(
account,
this.reconcileOptions(options, await this.defaultOnDiskOptions()),
);
}
@withPrototypeForArrayMembers(GeneratedPasswordHistory)
async getEncryptedPasswordGenerationHistory(
options?: StorageOptions,

View File

@@ -22,6 +22,7 @@ export const ACCOUNT_MEMORY = new StateDefinition("account", "memory");
export const BILLING_BANNERS_DISK = new StateDefinition("billingBanners", "disk");
export const CRYPTO_DISK = new StateDefinition("crypto", "disk");
export const CRYPTO_MEMORY = new StateDefinition("crypto", "memory");
export const SSO_DISK = new StateDefinition("ssoLogin", "disk");