[PM-20492] Refactor symmetric keys - remove key buffer representation, migrate consumers to .toEncoded() (#14371)

* Refactor encrypt service to expose key wrapping * Fix build * Undo ts strict removal * Fix wrong method being used to encrypt key material * Rename parameters and remove todo * Add summary to encrypt * Update libs/common/src/key-management/crypto/services/encrypt.service.implementation.ts Co-authored-by: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com> * Update libs/common/src/key-management/crypto/services/encrypt.service.implementation.ts Co-authored-by: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com> * Update libs/common/src/key-management/crypto/services/encrypt.service.implementation.ts Co-authored-by: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com> * Update libs/common/src/key-management/crypto/services/encrypt.service.implementation.ts Co-authored-by: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com> * Update libs/common/src/key-management/crypto/abstractions/encrypt.service.ts Co-authored-by: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com> * Update libs/common/src/key-management/crypto/services/encrypt.service.implementation.ts Co-authored-by: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com> * Add tests for unhappy paths * Add test coverage * Add links * Remove direct buffer access * Fix build on cli --------- Co-authored-by: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com>
2025-12-06 00:13:28 +00:00 · 2025-04-25 19:26:39 +02:00
parent b4c4eea229
commit fd0db40f79
15 changed files with 58 additions and 45 deletions
--- a/apps/cli/src/admin-console/commands/confirm.command.ts
+++ b/apps/cli/src/admin-console/commands/confirm.command.ts
@@ -57,7 +57,7 @@ export class ConfirmCommand {
      }
      const publicKeyResponse = await this.apiService.getUserPublicKey(orgUser.userId);
      const publicKey = Utils.fromB64ToArray(publicKeyResponse.publicKey);
-      const key = await this.encryptService.rsaEncrypt(orgKey.key, publicKey);
+      const key = await this.encryptService.encapsulateKeyUnsigned(orgKey, publicKey);
      const req = new OrganizationUserConfirmRequest();
      req.key = key.encryptedString;
      await this.organizationUserApiService.postOrganizationUserConfirm(
--- a/bitwarden_license/bit-web/src/app/billing/providers/setup/setup-business-unit.component.ts
+++ b/bitwarden_license/bit-web/src/app/billing/providers/setup/setup-business-unit.component.ts
@@ -84,10 +84,8 @@ export class SetupBusinessUnitComponent extends BaseAcceptComponent {

    const organizationKey = await firstValueFrom(organizationKey$);

-    const { encryptedString: encryptedOrganizationKey } = await this.encryptService.encrypt(
-      organizationKey.key,
-      providerKey,
-    );
+    const { encryptedString: encryptedOrganizationKey } =
+      await this.encryptService.wrapSymmetricKey(organizationKey, providerKey);

    if (!encryptedProviderKey || !encryptedOrganizationKey) {
      return await fail();
--- a/libs/auth/src/common/services/auth-request/auth-request.service.spec.ts
+++ b/libs/auth/src/common/services/auth-request/auth-request.service.spec.ts
@@ -123,7 +123,9 @@ describe("AuthRequestService", () => {
    });

    it("should use the user key if the master key and hash do not exist", async () => {
-      keyService.getUserKey.mockResolvedValueOnce({ key: new Uint8Array(64) } as UserKey);
+      keyService.getUserKey.mockResolvedValueOnce(
+        new SymmetricCryptoKey(new Uint8Array(64)) as UserKey,
+      );

      await sut.approveOrDenyAuthRequest(
        true,
@@ -131,7 +133,7 @@ describe("AuthRequestService", () => {
      );

      expect(encryptService.encapsulateKeyUnsigned).toHaveBeenCalledWith(
-        { key: new Uint8Array(64) },
+        new SymmetricCryptoKey(new Uint8Array(64)),
        expect.anything(),
      );
    });
--- a/libs/auth/src/common/services/pin/pin.service.spec.ts
+++ b/libs/auth/src/common/services/pin/pin.service.spec.ts
@@ -434,7 +434,7 @@ describe("PinService", () => {
        .fn()
        .mockResolvedValue(pinKeyEncryptedUserKeyPersistant);
      sut.makePinKey = jest.fn().mockResolvedValue(mockPinKey);
-      encryptService.decryptToBytes.mockResolvedValue(mockUserKey.key);
+      encryptService.decryptToBytes.mockResolvedValue(mockUserKey.toEncoded());
    }

    function mockPinEncryptedKeyDataByPinLockType(pinLockType: PinLockType) {
--- a/libs/common/src/auth/services/webauthn-login/webauthn-login-prf-key.service.spec.ts
+++ b/libs/common/src/auth/services/webauthn-login/webauthn-login-prf-key.service.spec.ts
@@ -21,7 +21,7 @@ describe("WebAuthnLoginPrfKeyService", () => {

      const result = await service.createSymmetricKeyFromPrf(randomBytes(32));

-      expect(result.key.length).toBe(64);
+      expect(result.toEncoded().length).toBe(64);
    });
  });
 });
--- a/libs/common/src/key-management/crypto/services/encrypt.service.implementation.ts
+++ b/libs/common/src/key-management/crypto/services/encrypt.service.implementation.ts
@@ -47,7 +47,7 @@ export class EncryptServiceImplementation implements EncryptService {
    }

    if (this.blockType0) {
-      if (key.inner().type === EncryptionType.AesCbc256_B64 || key.key.byteLength < 64) {
+      if (key.inner().type === EncryptionType.AesCbc256_B64) {
        throw new Error("Type 0 encryption is not supported.");
      }
    }
@@ -105,7 +105,7 @@ export class EncryptServiceImplementation implements EncryptService {
      throw new Error("No wrappingKey provided for wrapping.");
    }

-    return await this.encryptUint8Array(keyToBeWrapped.key, wrappingKey);
+    return await this.encryptUint8Array(keyToBeWrapped.toEncoded(), wrappingKey);
  }

  private async encryptUint8Array(
@@ -147,7 +147,7 @@ export class EncryptServiceImplementation implements EncryptService {
    }

    if (this.blockType0) {
-      if (key.inner().type === EncryptionType.AesCbc256_B64 || key.key.byteLength < 64) {
+      if (key.inner().type === EncryptionType.AesCbc256_B64) {
        throw new Error("Type 0 encryption is not supported.");
      }
    }
--- a/libs/common/src/key-management/crypto/services/encrypt.service.spec.ts
+++ b/libs/common/src/key-management/crypto/services/encrypt.service.spec.ts
@@ -184,10 +184,9 @@ describe("EncryptService", () => {
      (encryptService as any).blockType0 = true;
      const key = new SymmetricCryptoKey(makeStaticByteArray(32));
      const mock32Key = mock<SymmetricCryptoKey>();
-      mock32Key.key = makeStaticByteArray(32);
      mock32Key.inner.mockReturnValue({
        type: 0,
-        encryptionKey: mock32Key.key,
+        encryptionKey: makeStaticByteArray(32),
      });

      await expect(encryptService.encrypt(null!, key)).rejects.toThrow(
@@ -270,10 +269,9 @@ describe("EncryptService", () => {
      (encryptService as any).blockType0 = true;
      const key = new SymmetricCryptoKey(makeStaticByteArray(32));
      const mock32Key = mock<SymmetricCryptoKey>();
-      mock32Key.key = makeStaticByteArray(32);
      mock32Key.inner.mockReturnValue({
        type: 0,
-        encryptionKey: mock32Key.key,
+        encryptionKey: makeStaticByteArray(32),
      });

      await expect(encryptService.encryptToBytes(plainValue, key)).rejects.toThrow(
@@ -571,7 +569,7 @@ describe("EncryptService", () => {
        const actual = await encryptService.encapsulateKeyUnsigned(testKey, publicKey);

        expect(cryptoFunctionService.rsaEncrypt).toBeCalledWith(
-          expect.toEqualBuffer(testKey.key),
+          expect.toEqualBuffer(testKey.toEncoded()),
          expect.toEqualBuffer(publicKey),
          "sha1",
        );
@@ -622,7 +620,7 @@ describe("EncryptService", () => {
          "sha1",
        );

-        expect(actual.key).toEqualBuffer(data);
+        expect(actual.toEncoded()).toEqualBuffer(data);
      });
    });
  });
--- a/libs/common/src/key-management/device-trust/services/device-trust.service.implementation.ts
+++ b/libs/common/src/key-management/device-trust/services/device-trust.service.implementation.ts
@@ -221,8 +221,8 @@ export class DeviceTrustService implements DeviceTrustServiceAbstraction {
          }

          const newEncryptedPublicKey = await this.encryptService.encrypt(publicKey, newUserKey);
-          const newEncryptedUserKey = await this.encryptService.rsaEncrypt(
-            newUserKey.key,
+          const newEncryptedUserKey = await this.encryptService.encapsulateKeyUnsigned(
+            newUserKey,
            publicKey,
          );

--- a/libs/common/src/key-management/device-trust/services/device-trust.service.spec.ts
+++ b/libs/common/src/key-management/device-trust/services/device-trust.service.spec.ts
@@ -450,7 +450,7 @@ describe("deviceTrustService", () => {

        // RsaEncrypt must be called w/ a user key array buffer of 64 bytes
        const userKey = cryptoSvcRsaEncryptSpy.mock.calls[0][0];
-        expect(userKey.key.byteLength).toBe(64);
+        expect(userKey.inner().type).toBe(EncryptionType.AesCbc256_HmacSha256_B64);

        expect(encryptServiceWrapDecapsulationKeySpy).toHaveBeenCalledTimes(1);
        expect(encryptServiceWrapEncapsulationKeySpy).toHaveBeenCalledTimes(1);
@@ -706,7 +706,9 @@ describe("deviceTrustService", () => {
        );
        encryptService.decryptToBytes.mockResolvedValue(null);
        encryptService.encrypt.mockResolvedValue(new EncString("test_encrypted_data"));
-        encryptService.rsaEncrypt.mockResolvedValue(new EncString("test_encrypted_data"));
+        encryptService.encapsulateKeyUnsigned.mockResolvedValue(
+          new EncString("test_encrypted_data"),
+        );

        const protectedDeviceResponse = new ProtectedDeviceResponse({
          id: "id",
@@ -861,8 +863,8 @@ describe("deviceTrustService", () => {

          // Mock the decryption of the public key with the old user key
          encryptService.decryptToBytes.mockImplementationOnce((_encValue, privateKeyValue) => {
-            expect(privateKeyValue.key.byteLength).toBe(64);
-            expect(new Uint8Array(privateKeyValue.key)[0]).toBe(FakeOldUserKeyMarker);
+            expect(privateKeyValue.inner().type).toBe(EncryptionType.AesCbc256_HmacSha256_B64);
+            expect(new Uint8Array(privateKeyValue.toEncoded())[0]).toBe(FakeOldUserKeyMarker);
            const data = new Uint8Array(250);
            data.fill(FakeDecryptedPublicKeyMarker, 0, 1);
            return Promise.resolve(data);
@@ -870,8 +872,8 @@ describe("deviceTrustService", () => {

          // Mock the encryption of the new user key with the decrypted public key
          encryptService.encapsulateKeyUnsigned.mockImplementationOnce((data, publicKey) => {
-            expect(data.key.byteLength).toBe(64); // New key should also be 64 bytes
-            expect(new Uint8Array(data.key)[0]).toBe(FakeNewUserKeyMarker); // New key should have the first byte be '1';
+            expect(data.inner().type).toBe(EncryptionType.AesCbc256_HmacSha256_B64); // New key should also be 64 bytes
+            expect(new Uint8Array(data.toEncoded())[0]).toBe(FakeNewUserKeyMarker); // New key should have the first byte be '1';

            expect(new Uint8Array(publicKey)[0]).toBe(FakeDecryptedPublicKeyMarker);
            return Promise.resolve(new EncString("4.ZW5jcnlwdGVkdXNlcg=="));
@@ -882,7 +884,7 @@ describe("deviceTrustService", () => {
            expect(plainValue).toBeInstanceOf(Uint8Array);
            expect(new Uint8Array(plainValue as Uint8Array)[0]).toBe(FakeDecryptedPublicKeyMarker);

-            expect(new Uint8Array(key.key)[0]).toBe(FakeNewUserKeyMarker);
+            expect(new Uint8Array(key.toEncoded())[0]).toBe(FakeNewUserKeyMarker);
            return Promise.resolve(
              new EncString("2.ZW5jcnlwdGVkcHVibGlj|ZW5jcnlwdGVkcHVibGlj|ZW5jcnlwdGVkcHVibGlj"),
            );
--- a/libs/common/src/platform/models/domain/symmetric-crypto-key.spec.ts
+++ b/libs/common/src/platform/models/domain/symmetric-crypto-key.spec.ts
@@ -19,7 +19,6 @@ describe("SymmetricCryptoKey", () => {
      const cryptoKey = new SymmetricCryptoKey(key);

      expect(cryptoKey).toEqual({
-        key: key,
        keyB64: "AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8=",
        innerKey: {
          type: EncryptionType.AesCbc256_B64,
@@ -33,7 +32,6 @@ describe("SymmetricCryptoKey", () => {
      const cryptoKey = new SymmetricCryptoKey(key);

      expect(cryptoKey).toEqual({
-        key: key,
        keyB64:
          "AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8gISIjJCUmJygpKissLS4vMDEyMzQ1Njc4OTo7PD0+Pw==",
        innerKey: {
--- a/libs/common/src/platform/models/domain/symmetric-crypto-key.ts
+++ b/libs/common/src/platform/models/domain/symmetric-crypto-key.ts
@@ -24,7 +24,6 @@ export type Aes256CbcKey = {
 export class SymmetricCryptoKey {
  private innerKey: Aes256CbcHmacKey | Aes256CbcKey;

-  key: Uint8Array;
  keyB64: string;

  /**
@@ -40,7 +39,6 @@ export class SymmetricCryptoKey {
        type: EncryptionType.AesCbc256_B64,
        encryptionKey: key,
      };
-      this.key = key;
      this.keyB64 = this.toBase64();
    } else if (key.byteLength === 64) {
      this.innerKey = {
@@ -48,7 +46,6 @@ export class SymmetricCryptoKey {
        encryptionKey: key.slice(0, 32),
        authenticationKey: key.slice(32),
      };
-      this.key = key;
      this.keyB64 = this.toBase64();
    } else {
      throw new Error(`Unsupported encType/key length ${key.byteLength}`);
--- a/libs/common/src/platform/services/key-generation.service.spec.ts
+++ b/libs/common/src/platform/services/key-generation.service.spec.ts
@@ -4,6 +4,7 @@ import { PBKDF2KdfConfig, Argon2KdfConfig } from "@bitwarden/key-management";

 import { CryptoFunctionService } from "../../key-management/crypto/abstractions/crypto-function.service";
 import { CsprngArray } from "../../types/csprng";
+import { EncryptionType } from "../enums";

 import { KeyGenerationService } from "./key-generation.service";

@@ -52,7 +53,7 @@ describe("KeyGenerationService", () => {

        expect(salt).toEqual(inputSalt);
        expect(material).toEqual(inputMaterial);
-        expect(derivedKey.key.length).toEqual(64);
+        expect(derivedKey.inner().type).toEqual(EncryptionType.AesCbc256_HmacSha256_B64);
      },
    );
  });
@@ -67,7 +68,7 @@ describe("KeyGenerationService", () => {

      const key = await sut.deriveKeyFromMaterial(material, salt, purpose);

-      expect(key.key.length).toEqual(64);
+      expect(key.inner().type).toEqual(EncryptionType.AesCbc256_HmacSha256_B64);
    });
  });

@@ -81,7 +82,7 @@ describe("KeyGenerationService", () => {

      const key = await sut.deriveKeyFromPassword(password, salt, kdfConfig);

-      expect(key.key.length).toEqual(32);
+      expect(key.inner().type).toEqual(EncryptionType.AesCbc256_B64);
    });

    it("should derive a 32 byte key from a password using argon2id", async () => {
@@ -94,7 +95,7 @@ describe("KeyGenerationService", () => {

      const key = await sut.deriveKeyFromPassword(password, salt, kdfConfig);

-      expect(key.key.length).toEqual(32);
+      expect(key.inner().type).toEqual(EncryptionType.AesCbc256_B64);
    });
  });
 });
--- a/libs/common/src/platform/services/key-generation.service.ts
+++ b/libs/common/src/platform/services/key-generation.service.ts
@@ -1,5 +1,6 @@
 // FIXME: Update this file to be type safe and remove this and next line
 // @ts-strict-ignore
+import { MasterKey, PinKey } from "@bitwarden/common/types/key";
 import { KdfConfig, PBKDF2KdfConfig, Argon2KdfConfig, KdfType } from "@bitwarden/key-management";

 import { CryptoFunctionService } from "../../key-management/crypto/abstractions/crypto-function.service";
@@ -78,10 +79,21 @@ export class KeyGenerationService implements KeyGenerationServiceAbstraction {
    return new SymmetricCryptoKey(key);
  }

-  async stretchKey(key: SymmetricCryptoKey): Promise<SymmetricCryptoKey> {
+  async stretchKey(key: MasterKey | PinKey): Promise<SymmetricCryptoKey> {
    const newKey = new Uint8Array(64);
-    const encKey = await this.cryptoFunctionService.hkdfExpand(key.key, "enc", 32, "sha256");
-    const macKey = await this.cryptoFunctionService.hkdfExpand(key.key, "mac", 32, "sha256");
+    // Master key and pin key are always 32 bytes
+    const encKey = await this.cryptoFunctionService.hkdfExpand(
+      key.inner().encryptionKey,
+      "enc",
+      32,
+      "sha256",
+    );
+    const macKey = await this.cryptoFunctionService.hkdfExpand(
+      key.inner().encryptionKey,
+      "mac",
+      32,
+      "sha256",
+    );

    newKey.set(new Uint8Array(encKey));
    newKey.set(new Uint8Array(macKey), 32);
--- a/libs/key-management/src/key.service.spec.ts
+++ b/libs/key-management/src/key.service.spec.ts
@@ -497,7 +497,7 @@ describe("keyService", () => {
      const output = new Uint8Array(64);
      output.set(encryptedPrivateKey.dataBytes);
      output.set(
-        key.key.subarray(0, 64 - encryptedPrivateKey.dataBytes.length),
+        key.toEncoded().subarray(0, 64 - encryptedPrivateKey.dataBytes.length),
        encryptedPrivateKey.dataBytes.length,
      );
      return output;
@@ -827,7 +827,7 @@ describe("keyService", () => {
        masterPasswordService.masterKeyHashSubject.next(storedMasterKeyHash);

        cryptoFunctionService.pbkdf2
-          .calledWith(masterKey.key, masterPassword as string, "sha256", 2)
+          .calledWith(masterKey.inner().encryptionKey, masterPassword as string, "sha256", 2)
          .mockResolvedValue(Utils.fromB64ToArray(mockReturnedHash));

        const actualDidMatch = await keyService.compareKeyHash(
--- a/libs/key-management/src/key.service.ts
+++ b/libs/key-management/src/key.service.ts
@@ -26,7 +26,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, HashPurpose } from "@bitwarden/common/platform/enums";
+import { KeySuffixOptions, HashPurpose, EncryptionType } from "@bitwarden/common/platform/enums";
 import { convertValues } from "@bitwarden/common/platform/misc/convert-values";
 import { Utils } from "@bitwarden/common/platform/misc/utils";
 import { EFFLongWordList } from "@bitwarden/common/platform/misc/wordlist";
@@ -346,7 +346,12 @@ export class DefaultKeyService implements KeyServiceAbstraction {
    }

    const iterations = hashPurpose === HashPurpose.LocalAuthorization ? 2 : 1;
-    const hash = await this.cryptoFunctionService.pbkdf2(key.key, password, "sha256", iterations);
+    const hash = await this.cryptoFunctionService.pbkdf2(
+      key.inner().encryptionKey,
+      password,
+      "sha256",
+      iterations,
+    );
    return Utils.fromBufferToB64(hash);
  }

@@ -823,13 +828,13 @@ export class DefaultKeyService implements KeyServiceAbstraction {
    newSymKey: SymmetricCryptoKey,
  ): Promise<[T, EncString]> {
    let protectedSymKey: EncString;
-    if (encryptionKey.key.byteLength === 32) {
+    if (encryptionKey.inner().type === EncryptionType.AesCbc256_B64) {
      const stretchedEncryptionKey = await this.keyGenerationService.stretchKey(encryptionKey);
      protectedSymKey = await this.encryptService.wrapSymmetricKey(
        newSymKey,
        stretchedEncryptionKey,
      );
-    } else if (encryptionKey.key.byteLength === 64) {
+    } else if (encryptionKey.inner().type === EncryptionType.AesCbc256_HmacSha256_B64) {
      protectedSymKey = await this.encryptService.wrapSymmetricKey(newSymKey, encryptionKey);
    } else {
      throw new Error("Invalid key size.");