From a6e7efddeb5a6263053118b5f211e7d2b0c13a91 Mon Sep 17 00:00:00 2001 From: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com> Date: Thu, 21 Aug 2025 15:49:19 -0500 Subject: [PATCH] [PM-23627] Require publicKey for keyService getFingerprint (#15933) * require public key on keyService getFingerprint * Update consumers and add error handling & logging --- .../account-security.component.spec.ts | 2 - .../settings/account-security.component.ts | 16 ++++-- apps/cli/src/commands/get.command.ts | 3 + apps/desktop/src/app/app.component.ts | 18 ++++-- .../settings/account/profile.component.html | 13 +++-- .../settings/account/profile.component.ts | 24 ++++++-- .../src/abstractions/key.service.ts | 10 ++-- libs/key-management/src/key.service.spec.ts | 57 +++++++++++++++++++ libs/key-management/src/key.service.ts | 12 +--- 9 files changed, 119 insertions(+), 36 deletions(-) diff --git a/apps/browser/src/auth/popup/settings/account-security.component.spec.ts b/apps/browser/src/auth/popup/settings/account-security.component.spec.ts index 014f2a7638..63666440a7 100644 --- a/apps/browser/src/auth/popup/settings/account-security.component.spec.ts +++ b/apps/browser/src/auth/popup/settings/account-security.component.spec.ts @@ -24,7 +24,6 @@ import { EnvironmentService } from "@bitwarden/common/platform/abstractions/envi import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; 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 { ValidationService } from "@bitwarden/common/platform/abstractions/validation.service"; import { MessageSender } from "@bitwarden/common/platform/messaging"; import { Utils } from "@bitwarden/common/platform/misc/utils"; @@ -79,7 +78,6 @@ describe("AccountSecurityComponent", () => { { provide: PlatformUtilsService, useValue: platformUtilsService }, { provide: PolicyService, useValue: policyService }, { provide: PopupRouterCacheService, useValue: mock() }, - { provide: StateService, useValue: mock() }, { provide: ToastService, useValue: mock() }, { provide: UserVerificationService, useValue: mock() }, { provide: VaultTimeoutService, useValue: mock() }, diff --git a/apps/browser/src/auth/popup/settings/account-security.component.ts b/apps/browser/src/auth/popup/settings/account-security.component.ts index b41cfe14c4..72a389ecf7 100644 --- a/apps/browser/src/auth/popup/settings/account-security.component.ts +++ b/apps/browser/src/auth/popup/settings/account-security.component.ts @@ -44,9 +44,9 @@ import { import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; import { EnvironmentService } from "@bitwarden/common/platform/abstractions/environment.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; +import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; -import { StateService } from "@bitwarden/common/platform/abstractions/state.service"; import { ValidationService } from "@bitwarden/common/platform/abstractions/validation.service"; import { DialogRef, @@ -149,7 +149,6 @@ export class AccountSecurityComponent implements OnInit, OnDestroy { public messagingService: MessagingService, private environmentService: EnvironmentService, private keyService: KeyService, - private stateService: StateService, private userVerificationService: UserVerificationService, private dialogService: DialogService, private changeDetectorRef: ChangeDetectorRef, @@ -159,6 +158,7 @@ export class AccountSecurityComponent implements OnInit, OnDestroy { private vaultNudgesService: NudgesService, private validationService: ValidationService, private configService: ConfigService, + private logService: LogService, ) {} async ngOnInit() { @@ -683,10 +683,16 @@ export class AccountSecurityComponent implements OnInit, OnDestroy { } async openAcctFingerprintDialog() { - const activeUserId = await firstValueFrom( - this.accountService.activeAccount$.pipe(map((a) => a?.id)), - ); + const activeUserId = await firstValueFrom(getUserId(this.accountService.activeAccount$)); const publicKey = await firstValueFrom(this.keyService.userPublicKey$(activeUserId)); + if (publicKey == null) { + this.logService.error( + "[AccountSecurityComponent] No public key available for the user: " + + activeUserId + + " fingerprint can't be displayed.", + ); + return; + } const fingerprint = await this.keyService.getFingerprint(activeUserId, publicKey); const dialogRef = FingerprintDialogComponent.open(this.dialogService, { diff --git a/apps/cli/src/commands/get.command.ts b/apps/cli/src/commands/get.command.ts index 58181cff6c..a994ad3117 100644 --- a/apps/cli/src/commands/get.command.ts +++ b/apps/cli/src/commands/get.command.ts @@ -606,6 +606,9 @@ export class GetCommand extends DownloadCommand { if (id === "me") { const activeUserId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId)); const publicKey = await firstValueFrom(this.keyService.userPublicKey$(activeUserId)); + if (publicKey == null) { + return Response.error("No public key available for the active user."); + } fingerprint = await this.keyService.getFingerprint(activeUserId, publicKey); } else if (Utils.isGuid(id)) { try { diff --git a/apps/desktop/src/app/app.component.ts b/apps/desktop/src/app/app.component.ts index 197290cf69..c4e60e4ff9 100644 --- a/apps/desktop/src/app/app.component.ts +++ b/apps/desktop/src/app/app.component.ts @@ -299,12 +299,22 @@ export class AppComponent implements OnInit, OnDestroy { break; case "showFingerprintPhrase": { const activeUserId = await firstValueFrom( - this.accountService.activeAccount$.pipe(map((a) => a?.id)), + getUserId(this.accountService.activeAccount$), ); const publicKey = await firstValueFrom(this.keyService.userPublicKey$(activeUserId)); - const fingerprint = await this.keyService.getFingerprint(activeUserId, publicKey); - const dialogRef = FingerprintDialogComponent.open(this.dialogService, { fingerprint }); - await firstValueFrom(dialogRef.closed); + if (publicKey == null) { + this.logService.error( + "[AppComponent] No public key available for the user: " + + activeUserId + + " fingerprint can't be displayed.", + ); + } else { + const fingerprint = await this.keyService.getFingerprint(activeUserId, publicKey); + const dialogRef = FingerprintDialogComponent.open(this.dialogService, { + fingerprint, + }); + await firstValueFrom(dialogRef.closed); + } break; } case "deleteAccount": diff --git a/apps/web/src/app/auth/settings/account/profile.component.html b/apps/web/src/app/auth/settings/account/profile.component.html index 74e9cf08f8..a49e6c31d2 100644 --- a/apps/web/src/app/auth/settings/account/profile.component.html +++ b/apps/web/src/app/auth/settings/account/profile.component.html @@ -46,11 +46,14 @@ - - + @if (fingerprintMaterial && userPublicKey) { + + + } diff --git a/apps/web/src/app/auth/settings/account/profile.component.ts b/apps/web/src/app/auth/settings/account/profile.component.ts index a0572b846d..54f9ac5829 100644 --- a/apps/web/src/app/auth/settings/account/profile.component.ts +++ b/apps/web/src/app/auth/settings/account/profile.component.ts @@ -12,7 +12,10 @@ import { UpdateProfileRequest } from "@bitwarden/common/auth/models/request/upda import { getUserId } from "@bitwarden/common/auth/services/account.service"; import { ProfileResponse } from "@bitwarden/common/models/response/profile.response"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; +import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; +import { UserPublicKey } from "@bitwarden/common/types/key"; import { DialogService, ToastService } from "@bitwarden/components"; +import { KeyService } from "@bitwarden/key-management"; import { DynamicAvatarComponent } from "../../../components/dynamic-avatar.component"; import { SharedModule } from "../../../shared"; @@ -29,6 +32,7 @@ export class ProfileComponent implements OnInit, OnDestroy { loading = true; profile: ProfileResponse; fingerprintMaterial: string; + userPublicKey: UserPublicKey; managingOrganization$: Observable; private destroy$ = new Subject(); @@ -44,16 +48,24 @@ export class ProfileComponent implements OnInit, OnDestroy { private dialogService: DialogService, private toastService: ToastService, private organizationService: OrganizationService, + private keyService: KeyService, + private logService: LogService, ) {} async ngOnInit() { this.profile = await this.apiService.getProfile(); - this.loading = false; - this.fingerprintMaterial = await firstValueFrom( - this.accountService.activeAccount$.pipe(map((a) => a?.id)), - ); - const userId = await firstValueFrom(getUserId(this.accountService.activeAccount$)); + this.fingerprintMaterial = userId; + const publicKey = await firstValueFrom(this.keyService.userPublicKey$(userId)); + if (publicKey == null) { + this.logService.error( + "[ProfileComponent] No public key available for the user: " + + userId + + " fingerprint can't be displayed.", + ); + } else { + this.userPublicKey = publicKey; + } this.managingOrganization$ = this.organizationService .organizations$(userId) @@ -70,6 +82,8 @@ export class ProfileComponent implements OnInit, OnDestroy { .subscribe((name) => { this.profile.name = name; }); + + this.loading = false; } openChangeAvatar = async () => { diff --git a/libs/key-management/src/abstractions/key.service.ts b/libs/key-management/src/abstractions/key.service.ts index 4f14f5523b..1685938de3 100644 --- a/libs/key-management/src/abstractions/key.service.ts +++ b/libs/key-management/src/abstractions/key.service.ts @@ -318,14 +318,14 @@ export abstract class KeyService { ): Observable<{ privateKey: UserPrivateKey; publicKey: UserPublicKey } | null>; /** - * Generates a fingerprint phrase for the user based on their public key + * Generates a fingerprint phrase for the public key provided. * - * @throws Error when publicKey is null and there is no active user, or the active user does not have a public key + * @throws Error when publicKey is null or undefined. * @param fingerprintMaterial Fingerprint material - * @param publicKey The user's public key - * @returns The user's fingerprint phrase + * @param publicKey The public key to generate the fingerprint phrase for. + * @returns The fingerprint phrase */ - abstract getFingerprint(fingerprintMaterial: string, publicKey?: Uint8Array): Promise; + abstract getFingerprint(fingerprintMaterial: string, publicKey: Uint8Array): Promise; /** * Generates a new keypair * @param key A key to encrypt the private key with. If not provided, diff --git a/libs/key-management/src/key.service.spec.ts b/libs/key-management/src/key.service.spec.ts index 4d872e2cd5..6e1ee5d650 100644 --- a/libs/key-management/src/key.service.spec.ts +++ b/libs/key-management/src/key.service.spec.ts @@ -1227,6 +1227,63 @@ describe("keyService", () => { }); }); + describe("getFingerprint", () => { + const mockFingerprintMaterial = "test@example.com"; + const mockPublicKey = new Uint8Array(256); + const mockKeyFingerprint = Utils.fromB64ToArray("nfG2jTrJilBEsSrg7ffe9exE9PlClem4P2bxlQ6rNbs="); + const mockUserFingerprint = Utils.fromB64ToArray( + "V5AQSk83YXd6kZqCncC6d9J72R7UZ60Xl1eIoDoWgTc=", + ); + const expectedFingerprint = ["predefine", "hunting", "pastime", "enrich", "unhearing"]; + + beforeEach(() => { + cryptoFunctionService.hash.mockResolvedValue(mockKeyFingerprint); + cryptoFunctionService.hkdfExpand.mockResolvedValue(mockUserFingerprint); + }); + + test.each([null as unknown as Uint8Array, undefined as unknown as Uint8Array])( + "throws when publicKey is %s", + async (publicKey) => { + await expect(keyService.getFingerprint(mockFingerprintMaterial, publicKey)).rejects.toThrow( + "Public key is required to generate a fingerprint.", + ); + expect(cryptoFunctionService.hash).not.toHaveBeenCalled(); + expect(cryptoFunctionService.hkdfExpand).not.toHaveBeenCalled(); + }, + ); + + it("generates fingerprint successfully", async () => { + const result = await keyService.getFingerprint(mockFingerprintMaterial, mockPublicKey); + + expect(result).toEqual(expectedFingerprint); + expect(cryptoFunctionService.hash).toHaveBeenCalledWith(mockPublicKey, "sha256"); + expect(cryptoFunctionService.hkdfExpand).toHaveBeenCalledWith( + mockKeyFingerprint, + mockFingerprintMaterial, + 32, + "sha256", + ); + }); + + it("throws when entropy of hash function is too small", async () => { + const keyFingerprint = new Uint8Array(3); + cryptoFunctionService.hash.mockResolvedValue(keyFingerprint); + cryptoFunctionService.hkdfExpand.mockResolvedValue(new Uint8Array(3)); + + await expect( + keyService.getFingerprint(mockFingerprintMaterial, mockPublicKey), + ).rejects.toThrow("Output entropy of hash function is too small"); + + expect(cryptoFunctionService.hash).toHaveBeenCalledWith(mockPublicKey, "sha256"); + expect(cryptoFunctionService.hkdfExpand).toHaveBeenCalledWith( + keyFingerprint, + mockFingerprintMaterial, + 32, + "sha256", + ); + }); + }); + describe("makeKeyPair", () => { test.each([null as unknown as SymmetricCryptoKey, undefined as unknown as SymmetricCryptoKey])( "throws when the provided key is %s", diff --git a/libs/key-management/src/key.service.ts b/libs/key-management/src/key.service.ts index 92bee383a0..ed0b844a2a 100644 --- a/libs/key-management/src/key.service.ts +++ b/libs/key-management/src/key.service.ts @@ -473,19 +473,11 @@ export class DefaultKeyService implements KeyServiceAbstraction { .update(() => encPrivateKey); } - // TODO: Make public key required - async getFingerprint(fingerprintMaterial: string, publicKey?: Uint8Array): Promise { + async getFingerprint(fingerprintMaterial: string, publicKey: Uint8Array): Promise { if (publicKey == null) { - const activeUserId = await firstValueFrom(this.stateProvider.activeUserId$); - if (activeUserId == null) { - throw new Error("No active user found."); - } - publicKey = (await firstValueFrom(this.userPublicKey$(activeUserId))) as Uint8Array; + throw new Error("Public key is required to generate a fingerprint."); } - if (publicKey === null) { - throw new Error("No public key available."); - } const keyFingerprint = await this.cryptoFunctionService.hash(publicKey, "sha256"); const userFingerprint = await this.cryptoFunctionService.hkdfExpand( keyFingerprint,