From a66e224d3298a6600dc1c0acd0b6b37030c9e1e1 Mon Sep 17 00:00:00 2001 From: Jared Snider <116684653+JaredSnider-Bitwarden@users.noreply.github.com> Date: Tue, 26 Mar 2024 18:41:14 -0400 Subject: [PATCH] Auth/PM-7072 - Token Service - Access Token Secure Storage Refactor (#8412) * PM-5263 - TokenSvc - WIP on access token secure storage refactor * PM-5263 - Add key generation svc to token svc. * PM-5263 - TokenSvc - more progress on encrypt access token work. * PM-5263 - TokenSvc TODO cleanup * PM-5263 - TokenSvc - rename * PM-5263 - TokenSvc - decryptAccess token must return null as that is a valid case. * PM-5263 - Add EncryptSvc dep to TokenSvc * PM-5263 - Add secure storage to token service * PM-5263 - TokenSvc - (1) Finish implementing accessTokenKey stored in secure storage + encrypted access token stored on disk (2) Remove no longer necessary migration flag as the presence of the accessTokenKey now serves the same purpose. Co-authored-by: Jake Fink * PM-5263 - TokenSvc - (1) Tweak return structure of decryptAccessToken to be more debuggable (2) Add TODO to add more error handling. * PM-5263 - TODO: update tests * PM-5263 - add temp logs * PM-5263 - TokenSvc - remove logs now that I don't need them. * fix tests for access token * PM-5263 - TokenSvc test cleanup - small tweaks / cleanup * PM-5263 - TokenService - per PR feedback from Justin - add error message to error message if possible. Co-authored-by: Justin Baur <19896123+justindbaur@users.noreply.github.com> --------- Co-authored-by: Jake Fink Co-authored-by: Justin Baur <19896123+justindbaur@users.noreply.github.com> --- .../token-service.factory.ts | 20 +- .../browser/src/background/main.background.ts | 3 + apps/cli/src/bw.ts | 7 +- apps/desktop/src/main.ts | 29 +- .../illegal-secure-storage.service.ts | 28 ++ .../src/services/jslib-services.module.ts | 5 +- .../src/auth/services/token.service.spec.ts | 291 +++++++----------- .../common/src/auth/services/token.service.ts | 189 +++++++++--- .../src/auth/services/token.state.spec.ts | 2 - libs/common/src/auth/services/token.state.ts | 8 - 10 files changed, 354 insertions(+), 228 deletions(-) create mode 100644 apps/desktop/src/platform/services/illegal-secure-storage.service.ts diff --git a/apps/browser/src/auth/background/service-factories/token-service.factory.ts b/apps/browser/src/auth/background/service-factories/token-service.factory.ts index 25c30460f0..ba42998209 100644 --- a/apps/browser/src/auth/background/service-factories/token-service.factory.ts +++ b/apps/browser/src/auth/background/service-factories/token-service.factory.ts @@ -1,6 +1,10 @@ import { TokenService as AbstractTokenService } from "@bitwarden/common/auth/abstractions/token.service"; import { TokenService } from "@bitwarden/common/auth/services/token.service"; +import { + EncryptServiceInitOptions, + encryptServiceFactory, +} from "../../../platform/background/service-factories/encrypt-service.factory"; import { FactoryOptions, CachedServices, @@ -10,6 +14,14 @@ import { GlobalStateProviderInitOptions, globalStateProviderFactory, } from "../../../platform/background/service-factories/global-state-provider.factory"; +import { + KeyGenerationServiceInitOptions, + keyGenerationServiceFactory, +} from "../../../platform/background/service-factories/key-generation-service.factory"; +import { + LogServiceInitOptions, + logServiceFactory, +} from "../../../platform/background/service-factories/log-service.factory"; import { PlatformUtilsServiceInitOptions, platformUtilsServiceFactory, @@ -29,7 +41,10 @@ export type TokenServiceInitOptions = TokenServiceFactoryOptions & SingleUserStateProviderInitOptions & GlobalStateProviderInitOptions & PlatformUtilsServiceInitOptions & - SecureStorageServiceInitOptions; + SecureStorageServiceInitOptions & + KeyGenerationServiceInitOptions & + EncryptServiceInitOptions & + LogServiceInitOptions; export function tokenServiceFactory( cache: { tokenService?: AbstractTokenService } & CachedServices, @@ -45,6 +60,9 @@ export function tokenServiceFactory( await globalStateProviderFactory(cache, opts), (await platformUtilsServiceFactory(cache, opts)).supportsSecureStorage(), await secureStorageServiceFactory(cache, opts), + await keyGenerationServiceFactory(cache, opts), + await encryptServiceFactory(cache, opts), + await logServiceFactory(cache, opts), ), ); } diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index 452624d77e..14ded13c3e 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -443,6 +443,9 @@ export default class MainBackground { this.globalStateProvider, this.platformUtilsService.supportsSecureStorage(), this.secureStorageService, + this.keyGenerationService, + this.encryptService, + this.logService, ); const migrationRunner = new MigrationRunner( diff --git a/apps/cli/src/bw.ts b/apps/cli/src/bw.ts index 360ac6ffc4..e610f39954 100644 --- a/apps/cli/src/bw.ts +++ b/apps/cli/src/bw.ts @@ -318,11 +318,16 @@ export class Main { this.accountService, ); + this.keyGenerationService = new KeyGenerationService(this.cryptoFunctionService); + this.tokenService = new TokenService( this.singleUserStateProvider, this.globalStateProvider, this.platformUtilsService.supportsSecureStorage(), this.secureStorageService, + this.keyGenerationService, + this.encryptService, + this.logService, ); const migrationRunner = new MigrationRunner( @@ -343,8 +348,6 @@ export class Main { migrationRunner, ); - this.keyGenerationService = new KeyGenerationService(this.cryptoFunctionService); - this.cryptoService = new CryptoService( this.keyGenerationService, this.cryptoFunctionService, diff --git a/apps/desktop/src/main.ts b/apps/desktop/src/main.ts index 5cb6abac58..67f08839c5 100644 --- a/apps/desktop/src/main.ts +++ b/apps/desktop/src/main.ts @@ -6,11 +6,15 @@ import { firstValueFrom } from "rxjs"; import { TokenService as TokenServiceAbstraction } from "@bitwarden/common/auth/abstractions/token.service"; import { AccountServiceImplementation } from "@bitwarden/common/auth/services/account.service"; import { TokenService } from "@bitwarden/common/auth/services/token.service"; +import { EncryptService } from "@bitwarden/common/platform/abstractions/encrypt.service"; +import { KeyGenerationService as KeyGenerationServiceAbstraction } from "@bitwarden/common/platform/abstractions/key-generation.service"; import { StateService } from "@bitwarden/common/platform/abstractions/state.service"; import { DefaultBiometricStateService } from "@bitwarden/common/platform/biometrics/biometric-state.service"; import { StateFactory } from "@bitwarden/common/platform/factories/state-factory"; import { GlobalState } from "@bitwarden/common/platform/models/domain/global-state"; +import { EncryptServiceImplementation } from "@bitwarden/common/platform/services/cryptography/encrypt.service.implementation"; import { DefaultEnvironmentService } from "@bitwarden/common/platform/services/default-environment.service"; +import { KeyGenerationService } from "@bitwarden/common/platform/services/key-generation.service"; import { MemoryStorageService } from "@bitwarden/common/platform/services/memory-storage.service"; import { MigrationBuilderService } from "@bitwarden/common/platform/services/migration-builder.service"; import { MigrationRunner } from "@bitwarden/common/platform/services/migration-runner"; @@ -45,6 +49,7 @@ import { ELECTRON_SUPPORTS_SECURE_STORAGE } from "./platform/services/electron-p import { ElectronStateService } from "./platform/services/electron-state.service"; import { ElectronStorageService } from "./platform/services/electron-storage.service"; import { I18nMainService } from "./platform/services/i18n.main.service"; +import { IllegalSecureStorageService } from "./platform/services/illegal-secure-storage.service"; import { ElectronMainMessagingService } from "./services/electron-main-messaging.service"; import { isMacAppStore } from "./utils"; @@ -62,6 +67,8 @@ export class Main { desktopSettingsService: DesktopSettingsService; migrationRunner: MigrationRunner; tokenService: TokenServiceAbstraction; + keyGenerationService: KeyGenerationServiceAbstraction; + encryptService: EncryptService; windowMain: WindowMain; messagingMain: MessagingMain; @@ -153,11 +160,28 @@ export class Main { this.environmentService = new DefaultEnvironmentService(stateProvider, accountService); + this.mainCryptoFunctionService = new MainCryptoFunctionService(); + this.mainCryptoFunctionService.init(); + + this.keyGenerationService = new KeyGenerationService(this.mainCryptoFunctionService); + + this.encryptService = new EncryptServiceImplementation( + this.mainCryptoFunctionService, + this.logService, + true, // log mac failures + ); + + // Note: secure storage service is not available and should not be called in the main background process. + const illegalSecureStorageService = new IllegalSecureStorageService(); + this.tokenService = new TokenService( singleUserStateProvider, globalStateProvider, ELECTRON_SUPPORTS_SECURE_STORAGE, - this.storageService, + illegalSecureStorageService, + this.keyGenerationService, + this.encryptService, + this.logService, ); this.migrationRunner = new MigrationRunner( @@ -239,9 +263,6 @@ export class Main { this.clipboardMain = new ClipboardMain(); this.clipboardMain.init(); - - this.mainCryptoFunctionService = new MainCryptoFunctionService(); - this.mainCryptoFunctionService.init(); } bootstrap() { diff --git a/apps/desktop/src/platform/services/illegal-secure-storage.service.ts b/apps/desktop/src/platform/services/illegal-secure-storage.service.ts new file mode 100644 index 0000000000..12f86226be --- /dev/null +++ b/apps/desktop/src/platform/services/illegal-secure-storage.service.ts @@ -0,0 +1,28 @@ +import { AbstractStorageService } from "@bitwarden/common/platform/abstractions/storage.service"; +import { StorageOptions } from "@bitwarden/common/platform/models/domain/storage-options"; + +export class IllegalSecureStorageService implements AbstractStorageService { + constructor() {} + + get valuesRequireDeserialization(): boolean { + throw new Error("Method not implemented."); + } + has(key: string, options?: StorageOptions): Promise { + throw new Error("Method not implemented."); + } + save(key: string, obj: T, options?: StorageOptions): Promise { + throw new Error("Method not implemented."); + } + async get(key: string): Promise { + throw new Error("Method not implemented."); + } + async set(key: string, obj: T): Promise { + throw new Error("Method not implemented."); + } + async remove(key: string): Promise { + throw new Error("Method not implemented."); + } + async clear(): Promise { + throw new Error("Method not implemented."); + } +} diff --git a/libs/angular/src/services/jslib-services.module.ts b/libs/angular/src/services/jslib-services.module.ts index cab71631da..b2aebe20f4 100644 --- a/libs/angular/src/services/jslib-services.module.ts +++ b/libs/angular/src/services/jslib-services.module.ts @@ -503,7 +503,10 @@ const typesafeProviders: Array = [ SingleUserStateProvider, GlobalStateProvider, SUPPORTS_SECURE_STORAGE, - AbstractStorageService, + SECURE_STORAGE, + KeyGenerationServiceAbstraction, + EncryptService, + LogService, ], }), safeProvider({ diff --git a/libs/common/src/auth/services/token.service.spec.ts b/libs/common/src/auth/services/token.service.spec.ts index a7b953f928..63c581910a 100644 --- a/libs/common/src/auth/services/token.service.spec.ts +++ b/libs/common/src/auth/services/token.service.spec.ts @@ -1,7 +1,10 @@ -import { mock } from "jest-mock-extended"; +import { MockProxy, mock } from "jest-mock-extended"; import { FakeSingleUserStateProvider, FakeGlobalStateProvider } from "../../../spec"; import { VaultTimeoutAction } from "../../enums/vault-timeout-action.enum"; +import { EncryptService } from "../../platform/abstractions/encrypt.service"; +import { KeyGenerationService } from "../../platform/abstractions/key-generation.service"; +import { LogService } from "../../platform/abstractions/log.service"; import { AbstractStorageService } from "../../platform/abstractions/storage.service"; import { StorageLocation } from "../../platform/enums"; import { StorageOptions } from "../../platform/models/domain/storage-options"; @@ -12,7 +15,6 @@ import { DecodedAccessToken, TokenService } from "./token.service"; import { ACCESS_TOKEN_DISK, ACCESS_TOKEN_MEMORY, - ACCESS_TOKEN_MIGRATED_TO_SECURE_STORAGE, API_KEY_CLIENT_ID_DISK, API_KEY_CLIENT_ID_MEMORY, API_KEY_CLIENT_SECRET_DISK, @@ -28,7 +30,10 @@ describe("TokenService", () => { let singleUserStateProvider: FakeSingleUserStateProvider; let globalStateProvider: FakeGlobalStateProvider; - const secureStorageService = mock(); + let secureStorageService: MockProxy; + let keyGenerationService: MockProxy; + let encryptService: MockProxy; + let logService: MockProxy; const memoryVaultTimeoutAction = VaultTimeoutAction.LogOut; const memoryVaultTimeout = 30; @@ -74,12 +79,19 @@ describe("TokenService", () => { userId: userIdFromAccessToken, }; + const accessTokenKeyB64 = { keyB64: "AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8" }; + beforeEach(() => { jest.clearAllMocks(); singleUserStateProvider = new FakeSingleUserStateProvider(); globalStateProvider = new FakeGlobalStateProvider(); + secureStorageService = mock(); + keyGenerationService = mock(); + encryptService = mock(); + logService = mock(); + const supportsSecureStorage = false; // default to false; tests will override as needed tokenService = createTokenService(supportsSecureStorage); }); @@ -89,8 +101,8 @@ describe("TokenService", () => { }); describe("Access Token methods", () => { - const accessTokenPartialSecureStorageKey = `_accessToken`; - const accessTokenSecureStorageKey = `${userIdFromAccessToken}${accessTokenPartialSecureStorageKey}`; + const accessTokenKeyPartialSecureStorageKey = `_accessTokenKey`; + const accessTokenKeySecureStorageKey = `${userIdFromAccessToken}${accessTokenKeyPartialSecureStorageKey}`; describe("setAccessToken", () => { it("should throw an error if the access token is null", async () => { @@ -150,18 +162,22 @@ describe("TokenService", () => { tokenService = createTokenService(supportsSecureStorage); }); - it("should set the access token in secure storage, null out data on disk or in memory, and set a flag to indicate the token has been migrated", async () => { + it("should set an access token key in secure storage, the encrypted access token in disk, and clear out the token in memory", async () => { // Arrange: - // For testing purposes, let's assume that the access token is already in disk and memory - singleUserStateProvider - .getFake(userIdFromAccessToken, ACCESS_TOKEN_DISK) - .stateSubject.next([userIdFromAccessToken, accessTokenJwt]); - + // For testing purposes, let's assume that the access token is already in memory singleUserStateProvider .getFake(userIdFromAccessToken, ACCESS_TOKEN_MEMORY) .stateSubject.next([userIdFromAccessToken, accessTokenJwt]); + keyGenerationService.createKey.mockResolvedValue("accessTokenKey" as any); + + const mockEncryptedAccessToken = "encryptedAccessToken"; + + encryptService.encrypt.mockResolvedValue({ + encryptedString: mockEncryptedAccessToken, + } as any); + // Act await tokenService.setAccessToken( accessTokenJwt, @@ -170,27 +186,22 @@ describe("TokenService", () => { ); // Assert - // assert that the access token was set in secure storage + // assert that the AccessTokenKey was set in secure storage expect(secureStorageService.save).toHaveBeenCalledWith( - accessTokenSecureStorageKey, - accessTokenJwt, + accessTokenKeySecureStorageKey, + "accessTokenKey", secureStorageOptions, ); - // assert data was migrated out of disk and memory + flag was set + // assert that the access token was encrypted and set in disk expect( singleUserStateProvider.getFake(userIdFromAccessToken, ACCESS_TOKEN_DISK).nextMock, - ).toHaveBeenCalledWith(null); + ).toHaveBeenCalledWith(mockEncryptedAccessToken); + + // assert data was migrated out of memory expect( singleUserStateProvider.getFake(userIdFromAccessToken, ACCESS_TOKEN_MEMORY).nextMock, ).toHaveBeenCalledWith(null); - - expect( - singleUserStateProvider.getFake( - userIdFromAccessToken, - ACCESS_TOKEN_MIGRATED_TO_SECURE_STORAGE, - ).nextMock, - ).toHaveBeenCalledWith(true); }); }); }); @@ -216,7 +227,13 @@ describe("TokenService", () => { }); describe("Memory storage tests", () => { - it("should get the access token from memory with no user id specified (uses global active user)", async () => { + test.each([ + [ + "should get the access token from memory for the provided user id", + userIdFromAccessToken, + ], + ["should get the access token from memory with no user id provided", undefined], + ])("%s", async (_, userId) => { // Arrange singleUserStateProvider .getFake(userIdFromAccessToken, ACCESS_TOKEN_MEMORY) @@ -228,37 +245,28 @@ describe("TokenService", () => { .stateSubject.next([userIdFromAccessToken, undefined]); // Need to have global active id set to the user id - globalStateProvider - .getFake(ACCOUNT_ACTIVE_ACCOUNT_ID) - .stateSubject.next(userIdFromAccessToken); + if (!userId) { + globalStateProvider + .getFake(ACCOUNT_ACTIVE_ACCOUNT_ID) + .stateSubject.next(userIdFromAccessToken); + } // Act - const result = await tokenService.getAccessToken(); + const result = await tokenService.getAccessToken(userId); // Assert expect(result).toEqual(accessTokenJwt); }); - - it("should get the access token from memory for the specified user id", async () => { - // Arrange - singleUserStateProvider - .getFake(userIdFromAccessToken, ACCESS_TOKEN_MEMORY) - .stateSubject.next([userIdFromAccessToken, accessTokenJwt]); - - // set disk to undefined - singleUserStateProvider - .getFake(userIdFromAccessToken, ACCESS_TOKEN_DISK) - .stateSubject.next([userIdFromAccessToken, undefined]); - - // Act - const result = await tokenService.getAccessToken(userIdFromAccessToken); - // Assert - expect(result).toEqual(accessTokenJwt); - }); }); describe("Disk storage tests (secure storage not supported on platform)", () => { - it("should get the access token from disk with no user id specified", async () => { + test.each([ + [ + "should get the access token from disk for the specified user id", + userIdFromAccessToken, + ], + ["should get the access token from disk with no user id specified", undefined], + ])("%s", async (_, userId) => { // Arrange singleUserStateProvider .getFake(userIdFromAccessToken, ACCESS_TOKEN_MEMORY) @@ -269,28 +277,14 @@ describe("TokenService", () => { .stateSubject.next([userIdFromAccessToken, accessTokenJwt]); // Need to have global active id set to the user id - globalStateProvider - .getFake(ACCOUNT_ACTIVE_ACCOUNT_ID) - .stateSubject.next(userIdFromAccessToken); + if (!userId) { + globalStateProvider + .getFake(ACCOUNT_ACTIVE_ACCOUNT_ID) + .stateSubject.next(userIdFromAccessToken); + } // Act - const result = await tokenService.getAccessToken(); - // Assert - expect(result).toEqual(accessTokenJwt); - }); - - it("should get the access token from disk for the specified user id", async () => { - // Arrange - singleUserStateProvider - .getFake(userIdFromAccessToken, ACCESS_TOKEN_MEMORY) - .stateSubject.next([userIdFromAccessToken, undefined]); - - singleUserStateProvider - .getFake(userIdFromAccessToken, ACCESS_TOKEN_DISK) - .stateSubject.next([userIdFromAccessToken, accessTokenJwt]); - - // Act - const result = await tokenService.getAccessToken(userIdFromAccessToken); + const result = await tokenService.getAccessToken(userId); // Assert expect(result).toEqual(accessTokenJwt); }); @@ -302,7 +296,16 @@ describe("TokenService", () => { tokenService = createTokenService(supportsSecureStorage); }); - it("should get the access token from secure storage when no user id is specified and the migration flag is set to true", async () => { + test.each([ + [ + "should get the encrypted access token from disk, decrypt it, and return it when user id is provided", + userIdFromAccessToken, + ], + [ + "should get the encrypted access token from disk, decrypt it, and return it when no user id is provided", + undefined, + ], + ])("%s", async (_, userId) => { // Arrange singleUserStateProvider .getFake(userIdFromAccessToken, ACCESS_TOKEN_MEMORY) @@ -310,76 +313,35 @@ describe("TokenService", () => { singleUserStateProvider .getFake(userIdFromAccessToken, ACCESS_TOKEN_DISK) - .stateSubject.next([userIdFromAccessToken, undefined]); + .stateSubject.next([userIdFromAccessToken, "encryptedAccessToken"]); - secureStorageService.get.mockResolvedValue(accessTokenJwt); + secureStorageService.get.mockResolvedValue(accessTokenKeyB64); + encryptService.decryptToUtf8.mockResolvedValue("decryptedAccessToken"); // Need to have global active id set to the user id - globalStateProvider - .getFake(ACCOUNT_ACTIVE_ACCOUNT_ID) - .stateSubject.next(userIdFromAccessToken); - - // set access token migration flag to true - singleUserStateProvider - .getFake(userIdFromAccessToken, ACCESS_TOKEN_MIGRATED_TO_SECURE_STORAGE) - .stateSubject.next([userIdFromAccessToken, true]); + if (!userId) { + globalStateProvider + .getFake(ACCOUNT_ACTIVE_ACCOUNT_ID) + .stateSubject.next(userIdFromAccessToken); + } // Act - const result = await tokenService.getAccessToken(); - // Assert - expect(result).toEqual(accessTokenJwt); - }); - - it("should get the access token from secure storage when user id is specified and the migration flag set to true", async () => { - // Arrange - - singleUserStateProvider - .getFake(userIdFromAccessToken, ACCESS_TOKEN_MEMORY) - .stateSubject.next([userIdFromAccessToken, undefined]); - - singleUserStateProvider - .getFake(userIdFromAccessToken, ACCESS_TOKEN_DISK) - .stateSubject.next([userIdFromAccessToken, undefined]); - - secureStorageService.get.mockResolvedValue(accessTokenJwt); - - // set access token migration flag to true - singleUserStateProvider - .getFake(userIdFromAccessToken, ACCESS_TOKEN_MIGRATED_TO_SECURE_STORAGE) - .stateSubject.next([userIdFromAccessToken, true]); - - // Act - const result = await tokenService.getAccessToken(userIdFromAccessToken); - // Assert - expect(result).toEqual(accessTokenJwt); - }); - - it("should fallback and get the access token from disk when user id is specified and the migration flag is set to false even if the platform supports secure storage", async () => { - // Arrange - singleUserStateProvider - .getFake(userIdFromAccessToken, ACCESS_TOKEN_MEMORY) - .stateSubject.next([userIdFromAccessToken, undefined]); - - singleUserStateProvider - .getFake(userIdFromAccessToken, ACCESS_TOKEN_DISK) - .stateSubject.next([userIdFromAccessToken, accessTokenJwt]); - - // set access token migration flag to false - singleUserStateProvider - .getFake(userIdFromAccessToken, ACCESS_TOKEN_MIGRATED_TO_SECURE_STORAGE) - .stateSubject.next([userIdFromAccessToken, false]); - - // Act - const result = await tokenService.getAccessToken(userIdFromAccessToken); + const result = await tokenService.getAccessToken(userId); // Assert - expect(result).toEqual(accessTokenJwt); - - // assert that secure storage was not called - expect(secureStorageService.get).not.toHaveBeenCalled(); + expect(result).toEqual("decryptedAccessToken"); }); - it("should fallback and get the access token from disk when no user id is specified and the migration flag is set to false even if the platform supports secure storage", async () => { + test.each([ + [ + "should fallback and get the unencrypted access token from disk when there isn't an access token key in secure storage and a user id is provided", + userIdFromAccessToken, + ], + [ + "should fallback and get the unencrypted access token from disk when there isn't an access token key in secure storage and no user id is provided", + undefined, + ], + ])("%s", async (_, userId) => { // Arrange singleUserStateProvider .getFake(userIdFromAccessToken, ACCESS_TOKEN_MEMORY) @@ -390,23 +352,19 @@ describe("TokenService", () => { .stateSubject.next([userIdFromAccessToken, accessTokenJwt]); // Need to have global active id set to the user id - globalStateProvider - .getFake(ACCOUNT_ACTIVE_ACCOUNT_ID) - .stateSubject.next(userIdFromAccessToken); + if (!userId) { + globalStateProvider + .getFake(ACCOUNT_ACTIVE_ACCOUNT_ID) + .stateSubject.next(userIdFromAccessToken); + } - // set access token migration flag to false - singleUserStateProvider - .getFake(userIdFromAccessToken, ACCESS_TOKEN_MIGRATED_TO_SECURE_STORAGE) - .stateSubject.next([userIdFromAccessToken, false]); + // No access token key set // Act - const result = await tokenService.getAccessToken(); + const result = await tokenService.getAccessToken(userId); // Assert expect(result).toEqual(accessTokenJwt); - - // assert that secure storage was not called - expect(secureStorageService.get).not.toHaveBeenCalled(); }); }); }); @@ -426,7 +384,16 @@ describe("TokenService", () => { tokenService = createTokenService(supportsSecureStorage); }); - it("should clear the access token from all storage locations for the specified user id", async () => { + test.each([ + [ + "should clear the access token from all storage locations for the provided user id", + userIdFromAccessToken, + ], + [ + "should clear the access token from all storage locations for the global active user", + undefined, + ], + ])("%s", async (_, userId) => { // Arrange singleUserStateProvider .getFake(userIdFromAccessToken, ACCESS_TOKEN_MEMORY) @@ -436,6 +403,13 @@ describe("TokenService", () => { .getFake(userIdFromAccessToken, ACCESS_TOKEN_DISK) .stateSubject.next([userIdFromAccessToken, accessTokenJwt]); + // Need to have global active id set to the user id + if (!userId) { + globalStateProvider + .getFake(ACCOUNT_ACTIVE_ACCOUNT_ID) + .stateSubject.next(userIdFromAccessToken); + } + // Act await tokenService.clearAccessToken(userIdFromAccessToken); @@ -448,39 +422,7 @@ describe("TokenService", () => { ).toHaveBeenCalledWith(null); expect(secureStorageService.remove).toHaveBeenCalledWith( - accessTokenSecureStorageKey, - secureStorageOptions, - ); - }); - - it("should clear the access token from all storage locations for the global active user", async () => { - // Arrange - singleUserStateProvider - .getFake(userIdFromAccessToken, ACCESS_TOKEN_MEMORY) - .stateSubject.next([userIdFromAccessToken, accessTokenJwt]); - - singleUserStateProvider - .getFake(userIdFromAccessToken, ACCESS_TOKEN_DISK) - .stateSubject.next([userIdFromAccessToken, accessTokenJwt]); - - // Need to have global active id set to the user id - globalStateProvider - .getFake(ACCOUNT_ACTIVE_ACCOUNT_ID) - .stateSubject.next(userIdFromAccessToken); - - // Act - await tokenService.clearAccessToken(); - - // Assert - expect( - singleUserStateProvider.getFake(userIdFromAccessToken, ACCESS_TOKEN_MEMORY).nextMock, - ).toHaveBeenCalledWith(null); - expect( - singleUserStateProvider.getFake(userIdFromAccessToken, ACCESS_TOKEN_DISK).nextMock, - ).toHaveBeenCalledWith(null); - - expect(secureStorageService.remove).toHaveBeenCalledWith( - accessTokenSecureStorageKey, + accessTokenKeySecureStorageKey, secureStorageOptions, ); }); @@ -2232,6 +2174,9 @@ describe("TokenService", () => { globalStateProvider, supportsSecureStorage, secureStorageService, + keyGenerationService, + encryptService, + logService, ); } }); diff --git a/libs/common/src/auth/services/token.service.ts b/libs/common/src/auth/services/token.service.ts index 4e9722614e..a1dc7ecf21 100644 --- a/libs/common/src/auth/services/token.service.ts +++ b/libs/common/src/auth/services/token.service.ts @@ -1,11 +1,17 @@ import { firstValueFrom } from "rxjs"; +import { Opaque } from "type-fest"; import { decodeJwtTokenToJson } from "@bitwarden/auth/common"; import { VaultTimeoutAction } from "../../enums/vault-timeout-action.enum"; +import { EncryptService } from "../../platform/abstractions/encrypt.service"; +import { KeyGenerationService } from "../../platform/abstractions/key-generation.service"; +import { LogService } from "../../platform/abstractions/log.service"; import { AbstractStorageService } from "../../platform/abstractions/storage.service"; import { StorageLocation } from "../../platform/enums"; +import { EncString, EncryptedString } from "../../platform/models/domain/enc-string"; import { StorageOptions } from "../../platform/models/domain/storage-options"; +import { SymmetricCryptoKey } from "../../platform/models/domain/symmetric-crypto-key"; import { GlobalState, GlobalStateProvider, @@ -19,7 +25,6 @@ import { ACCOUNT_ACTIVE_ACCOUNT_ID } from "./account.service"; import { ACCESS_TOKEN_DISK, ACCESS_TOKEN_MEMORY, - ACCESS_TOKEN_MIGRATED_TO_SECURE_STORAGE, API_KEY_CLIENT_ID_DISK, API_KEY_CLIENT_ID_MEMORY, API_KEY_CLIENT_SECRET_DISK, @@ -101,8 +106,14 @@ export type DecodedAccessToken = { jti?: string; }; +/** + * A symmetric key for encrypting the access token before the token is stored on disk. + * This key should be stored in secure storage. + * */ +type AccessTokenKey = Opaque; + export class TokenService implements TokenServiceAbstraction { - private readonly accessTokenSecureStorageKey: string = "_accessToken"; + private readonly accessTokenKeySecureStorageKey: string = "_accessTokenKey"; private readonly refreshTokenSecureStorageKey: string = "_refreshToken"; @@ -117,10 +128,17 @@ export class TokenService implements TokenServiceAbstraction { private globalStateProvider: GlobalStateProvider, private readonly platformSupportsSecureStorage: boolean, private secureStorageService: AbstractStorageService, + private keyGenerationService: KeyGenerationService, + private encryptService: EncryptService, + private logService: LogService, ) { this.initializeState(); } + // pivoting to an approach where we create a symmetric key we store in secure storage + // which is used to protect the data before persisting to disk. + // We will also use the same symmetric key to decrypt the data when reading from disk. + private initializeState(): void { this.emailTwoFactorTokenRecordGlobalState = this.globalStateProvider.get( EMAIL_TWO_FACTOR_TOKEN_RECORD_DISK_LOCAL, @@ -155,6 +173,84 @@ export class TokenService implements TokenServiceAbstraction { } } + private async getAccessTokenKey(userId: UserId): Promise { + const accessTokenKeyB64 = await this.secureStorageService.get< + ReturnType + >(`${userId}${this.accessTokenKeySecureStorageKey}`, this.getSecureStorageOptions(userId)); + + if (!accessTokenKeyB64) { + return null; + } + + const accessTokenKey = SymmetricCryptoKey.fromJSON(accessTokenKeyB64) as AccessTokenKey; + return accessTokenKey; + } + + private async createAndSaveAccessTokenKey(userId: UserId): Promise { + const newAccessTokenKey = (await this.keyGenerationService.createKey(512)) as AccessTokenKey; + + await this.secureStorageService.save( + `${userId}${this.accessTokenKeySecureStorageKey}`, + newAccessTokenKey, + this.getSecureStorageOptions(userId), + ); + + return newAccessTokenKey; + } + + private async clearAccessTokenKey(userId: UserId): Promise { + await this.secureStorageService.remove( + `${userId}${this.accessTokenKeySecureStorageKey}`, + this.getSecureStorageOptions(userId), + ); + } + + private async getOrCreateAccessTokenKey(userId: UserId): Promise { + if (!this.platformSupportsSecureStorage) { + throw new Error("Platform does not support secure storage. Cannot obtain access token key."); + } + + if (!userId) { + throw new Error("User id not found. Cannot obtain access token key."); + } + + // First see if we have an accessTokenKey in secure storage and return it if we do + let accessTokenKey: AccessTokenKey = await this.getAccessTokenKey(userId); + + if (!accessTokenKey) { + // Otherwise, create a new one and save it to secure storage, then return it + accessTokenKey = await this.createAndSaveAccessTokenKey(userId); + } + + return accessTokenKey; + } + + private async encryptAccessToken(accessToken: string, userId: UserId): Promise { + const accessTokenKey = await this.getOrCreateAccessTokenKey(userId); + + return await this.encryptService.encrypt(accessToken, accessTokenKey); + } + + private async decryptAccessToken( + encryptedAccessToken: EncString, + userId: UserId, + ): Promise { + const accessTokenKey = await this.getAccessTokenKey(userId); + + if (!accessTokenKey) { + // If we don't have an accessTokenKey, then that means we don't have an access token as it hasn't been set yet + // and we have to return null here to properly indicate the the user isn't logged in. + return null; + } + + const decryptedAccessToken = await this.encryptService.decryptToUtf8( + encryptedAccessToken, + accessTokenKey, + ); + + return decryptedAccessToken; + } + /** * Internal helper for set access token which always requires user id. * This is useful because setTokens always will have a user id from the access token whereas @@ -173,26 +269,33 @@ export class TokenService implements TokenServiceAbstraction { ); switch (storageLocation) { - case TokenStorageLocation.SecureStorage: - await this.saveStringToSecureStorage(userId, this.accessTokenSecureStorageKey, accessToken); + case TokenStorageLocation.SecureStorage: { + // Secure storage implementations have variable length limitations (Windows), so we cannot + // store the access token directly. Instead, we encrypt with accessTokenKey and store that + // in secure storage. + + const encryptedAccessToken: EncString = await this.encryptAccessToken(accessToken, userId); + + // Save the encrypted access token to disk + await this.singleUserStateProvider + .get(userId, ACCESS_TOKEN_DISK) + .update((_) => encryptedAccessToken.encryptedString); // TODO: PM-6408 - https://bitwarden.atlassian.net/browse/PM-6408 - // 2024-02-20: Remove access token from memory and disk so that we migrate to secure storage over time. - // Remove these 2 calls to remove the access token from memory and disk after 3 releases. - - await this.singleUserStateProvider.get(userId, ACCESS_TOKEN_DISK).update((_) => null); + // 2024-02-20: Remove access token from memory so that we migrate to encrypt the access token over time. + // Remove this call to remove the access token from memory after 3 releases. await this.singleUserStateProvider.get(userId, ACCESS_TOKEN_MEMORY).update((_) => null); - // Set flag to indicate that the access token has been migrated to secure storage (don't remove this) - await this.setAccessTokenMigratedToSecureStorage(userId); - return; + } case TokenStorageLocation.Disk: + // Access token stored on disk unencrypted as platform does not support secure storage await this.singleUserStateProvider .get(userId, ACCESS_TOKEN_DISK) .update((_) => accessToken); return; case TokenStorageLocation.Memory: + // Access token stored in memory due to vault timeout settings await this.singleUserStateProvider .get(userId, ACCESS_TOKEN_MEMORY) .update((_) => accessToken); @@ -226,15 +329,14 @@ export class TokenService implements TokenServiceAbstraction { throw new Error("User id not found. Cannot clear access token."); } - // TODO: re-eval this once we get shared key definitions for vault timeout and vault timeout action data. + // TODO: re-eval this implementation once we get shared key definitions for vault timeout and vault timeout action data. // we can't determine storage location w/out vaultTimeoutAction and vaultTimeout - // but we can simply clear all locations to avoid the need to require those parameters + // but we can simply clear all locations to avoid the need to require those parameters. if (this.platformSupportsSecureStorage) { - await this.secureStorageService.remove( - `${userId}${this.accessTokenSecureStorageKey}`, - this.getSecureStorageOptions(userId), - ); + // Always clear the access token key when clearing the access token + // The next set of the access token will create a new access token key + await this.clearAccessTokenKey(userId); } // Platform doesn't support secure storage, so use state provider implementation @@ -249,36 +351,48 @@ export class TokenService implements TokenServiceAbstraction { return undefined; } - const accessTokenMigratedToSecureStorage = - await this.getAccessTokenMigratedToSecureStorage(userId); - if (this.platformSupportsSecureStorage && accessTokenMigratedToSecureStorage) { - return await this.getStringFromSecureStorage(userId, this.accessTokenSecureStorageKey); - } - // Try to get the access token from memory const accessTokenMemory = await this.getStateValueByUserIdAndKeyDef( userId, ACCESS_TOKEN_MEMORY, ); - if (accessTokenMemory != null) { return accessTokenMemory; } // If memory is null, read from disk - return await this.getStateValueByUserIdAndKeyDef(userId, ACCESS_TOKEN_DISK); - } + const accessTokenDisk = await this.getStateValueByUserIdAndKeyDef(userId, ACCESS_TOKEN_DISK); + if (!accessTokenDisk) { + return null; + } - private async getAccessTokenMigratedToSecureStorage(userId: UserId): Promise { - return await firstValueFrom( - this.singleUserStateProvider.get(userId, ACCESS_TOKEN_MIGRATED_TO_SECURE_STORAGE).state$, - ); - } + if (this.platformSupportsSecureStorage) { + const accessTokenKey = await this.getAccessTokenKey(userId); - private async setAccessTokenMigratedToSecureStorage(userId: UserId): Promise { - await this.singleUserStateProvider - .get(userId, ACCESS_TOKEN_MIGRATED_TO_SECURE_STORAGE) - .update((_) => true); + if (!accessTokenKey) { + // We know this is an unencrypted access token because we don't have an access token key + return accessTokenDisk; + } + + try { + const encryptedAccessTokenEncString = new EncString(accessTokenDisk as EncryptedString); + + const decryptedAccessToken = await this.decryptAccessToken( + encryptedAccessTokenEncString, + userId, + ); + return decryptedAccessToken; + } catch (error) { + // If an error occurs during decryption, return null for logout. + // We don't try to recover here since we'd like to know + // if access token and key are getting out of sync. + this.logService.error( + `Failed to decrypt access token: ${error?.message ?? "Unknown error."}`, + ); + return null; + } + } + return accessTokenDisk; } // Private because we only ever set the refresh token when also setting the access token @@ -417,7 +531,7 @@ export class TokenService implements TokenServiceAbstraction { const storageLocation = await this.determineStorageLocation( vaultTimeoutAction, vaultTimeout, - false, + false, // don't use secure storage for client id ); if (storageLocation === TokenStorageLocation.Disk) { @@ -484,7 +598,7 @@ export class TokenService implements TokenServiceAbstraction { const storageLocation = await this.determineStorageLocation( vaultTimeoutAction, vaultTimeout, - false, + false, // don't use secure storage for client secret ); if (storageLocation === TokenStorageLocation.Disk) { @@ -567,6 +681,7 @@ export class TokenService implements TokenServiceAbstraction { }); } + // TODO: stop accepting optional userIds async clearTokens(userId?: UserId): Promise { userId ??= await firstValueFrom(this.activeUserIdGlobalState.state$); diff --git a/libs/common/src/auth/services/token.state.spec.ts b/libs/common/src/auth/services/token.state.spec.ts index f4089a73fb..24eddc73f5 100644 --- a/libs/common/src/auth/services/token.state.spec.ts +++ b/libs/common/src/auth/services/token.state.spec.ts @@ -3,7 +3,6 @@ import { KeyDefinition } from "../../platform/state"; import { ACCESS_TOKEN_DISK, ACCESS_TOKEN_MEMORY, - ACCESS_TOKEN_MIGRATED_TO_SECURE_STORAGE, API_KEY_CLIENT_ID_DISK, API_KEY_CLIENT_ID_MEMORY, API_KEY_CLIENT_SECRET_DISK, @@ -17,7 +16,6 @@ import { describe.each([ [ACCESS_TOKEN_DISK, "accessTokenDisk"], [ACCESS_TOKEN_MEMORY, "accessTokenMemory"], - [ACCESS_TOKEN_MIGRATED_TO_SECURE_STORAGE, true], [REFRESH_TOKEN_DISK, "refreshTokenDisk"], [REFRESH_TOKEN_MEMORY, "refreshTokenMemory"], [REFRESH_TOKEN_MIGRATED_TO_SECURE_STORAGE, true], diff --git a/libs/common/src/auth/services/token.state.ts b/libs/common/src/auth/services/token.state.ts index 022f56f7aa..55471e1627 100644 --- a/libs/common/src/auth/services/token.state.ts +++ b/libs/common/src/auth/services/token.state.ts @@ -8,14 +8,6 @@ export const ACCESS_TOKEN_MEMORY = new KeyDefinition(TOKEN_MEMORY, "acce deserializer: (accessToken) => accessToken, }); -export const ACCESS_TOKEN_MIGRATED_TO_SECURE_STORAGE = new KeyDefinition( - TOKEN_DISK, - "accessTokenMigratedToSecureStorage", - { - deserializer: (accessTokenMigratedToSecureStorage) => accessTokenMigratedToSecureStorage, - }, -); - export const REFRESH_TOKEN_DISK = new KeyDefinition(TOKEN_DISK, "refreshToken", { deserializer: (refreshToken) => refreshToken, });