Skip to content

Commit

Permalink
fix(PrivateKey): Expose provider instead of entire Crypto object (#478)
Browse files Browse the repository at this point in the history
  • Loading branch information
gnarea authored Jun 29, 2022
1 parent 5f3d549 commit dea502a
Show file tree
Hide file tree
Showing 14 changed files with 138 additions and 67 deletions.
14 changes: 7 additions & 7 deletions src/lib/crypto_wrappers/PrivateKey.spec.ts
Original file line number Diff line number Diff line change
@@ -1,19 +1,19 @@
import { Crypto } from '@peculiar/webcrypto';

import { PrivateKey } from './PrivateKey';
import { MockAesKwProvider } from './webcrypto/_test_utils';
import { AwalaAesKwProvider } from './webcrypto/AwalaAesKwProvider';

describe('constructor', () => {
const CRYPTO = new Crypto();
const PROVIDER = new AwalaAesKwProvider(new MockAesKwProvider());

test('Key type should be private', () => {
const key = new PrivateKey(CRYPTO);
const key = new PrivateKey(PROVIDER);

expect(key.type).toEqual('private');
});

test('Crypto should be honoured', () => {
const key = new PrivateKey(CRYPTO);
test('Provider should be honoured', () => {
const key = new PrivateKey(PROVIDER);

expect(key.crypto).toEqual(CRYPTO);
expect(key.provider).toEqual(PROVIDER);
});
});
4 changes: 2 additions & 2 deletions src/lib/crypto_wrappers/PrivateKey.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { CryptoKey } from 'webcrypto-core';
import { CryptoKey, ProviderCrypto } from 'webcrypto-core';

export class PrivateKey extends CryptoKey {
constructor(public readonly crypto: Crypto) {
constructor(public readonly provider: ProviderCrypto) {
super();

this.type = 'private';
Expand Down
26 changes: 2 additions & 24 deletions src/lib/crypto_wrappers/_utils.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,9 @@ import { Crypto } from '@peculiar/webcrypto';
import * as asn1js from 'asn1js';
import bufferToArray from 'buffer-to-arraybuffer';
import * as pkijs from 'pkijs';
import { mockSpy } from '../_test_utils';

import {
derDeserialize,
generateRandom64BitValue,
getPkijsCrypto,
getEngineFromPrivateKey,
} from './_utils';
import { PrivateKey } from './PrivateKey';
import { mockSpy } from '../_test_utils';
import { derDeserialize, generateRandom64BitValue, getPkijsCrypto } from './_utils';

const stubCrypto = new Crypto();

Expand All @@ -32,22 +26,6 @@ describe('getPkijsCrypto', () => {
});
});

describe('getPkijsEngineFromCrypto', () => {
test('undefined should be returned if CryptoKey is used', () => {
const engine = getEngineFromPrivateKey(null as any);

expect(engine).toBeUndefined();
});

test('Nameless engine should be returned if PrivateKey is used', () => {
const engine = getEngineFromPrivateKey(new PrivateKey(stubCrypto));

expect(engine?.name).toBeEmpty();
expect(engine?.crypto).toBe(stubCrypto);
expect(engine?.subtle).toBe(stubCrypto.subtle);
});
});

describe('deserializeDer', () => {
test('should return ASN.1 object given a valid DER-encoded buffer', () => {
const asn1Value = new asn1js.Integer({ value: 3 });
Expand Down
11 changes: 1 addition & 10 deletions src/lib/crypto_wrappers/_utils.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import * as asn1js from 'asn1js';
import { CryptoEngine, getCrypto } from 'pkijs';

import { PrivateKey } from './PrivateKey';
import { getCrypto } from 'pkijs';

export function getPkijsCrypto(): SubtleCrypto {
const cryptoEngine = getCrypto();
Expand All @@ -11,13 +9,6 @@ export function getPkijsCrypto(): SubtleCrypto {
return cryptoEngine;
}

export function getEngineFromPrivateKey(key: CryptoKey | PrivateKey): CryptoEngine | undefined {
if (key instanceof PrivateKey) {
return new CryptoEngine({ crypto: key.crypto });
}
return undefined;
}

export function derDeserialize(derValue: ArrayBuffer): asn1js.AsnType {
const asn1Value = asn1js.fromBER(derValue);
if (asn1Value.offset === -1) {
Expand Down
10 changes: 5 additions & 5 deletions src/lib/crypto_wrappers/cms/signedData.spec.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
// tslint:disable:no-object-mutation

import { Crypto } from '@peculiar/webcrypto';

import * as asn1js from 'asn1js';
import * as pkijs from 'pkijs';

Expand All @@ -18,6 +16,8 @@ import { CMS_OIDS } from '../../oids';
import { HashingAlgorithm } from '../algorithms';
import { generateRSAKeyPair } from '../keys';
import { PrivateKey } from '../PrivateKey';
import { MockAesKwProvider } from '../webcrypto/_test_utils';
import { getEngineForPrivateKey } from '../webcrypto/engine';
import Certificate from '../x509/Certificate';
import { deserializeContentInfo, serializeContentInfo } from './_test_utils';
import CMSError from './CMSError';
Expand Down Expand Up @@ -47,9 +47,9 @@ describe('sign', () => {
});

test('Crypto in private key should be used if set', async () => {
const crypto = new Crypto();
const privateKey = new PrivateKey(crypto);
const signSpy = jest.spyOn(crypto.subtle, 'sign');
const privateKey = new PrivateKey(new MockAesKwProvider());
const engine = getEngineForPrivateKey(privateKey);
const signSpy = jest.spyOn(engine!.crypto.subtle, 'sign');
privateKey.algorithm = keyPair.privateKey.algorithm;
privateKey.usages = keyPair.privateKey.usages;
privateKey.extractable = keyPair.privateKey.extractable;
Expand Down
5 changes: 3 additions & 2 deletions src/lib/crypto_wrappers/cms/signedData.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ import bufferToArray from 'buffer-to-arraybuffer';
import * as pkijs from 'pkijs';

import { CMS_OIDS } from '../../oids';
import { getPkijsCrypto, getEngineFromPrivateKey } from '../_utils';
import { getPkijsCrypto } from '../_utils';
import { getEngineForPrivateKey } from '../webcrypto/engine';
import Certificate from '../x509/Certificate';
import { deserializeContentInfo } from './_utils';
import CMSError from './CMSError';
Expand Down Expand Up @@ -100,7 +101,7 @@ export class SignedData {
0,
hashingAlgorithmName,
encapsulatePlaintext ? undefined : plaintext,
getEngineFromPrivateKey(privateKey),
getEngineForPrivateKey(privateKey),
);

return SignedData.reDeserialize(pkijsSignedData);
Expand Down
7 changes: 1 addition & 6 deletions src/lib/crypto_wrappers/webcrypto/AwalaAesKwProvider.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { Crypto } from '@peculiar/webcrypto';
import bufferToArray from 'buffer-to-arraybuffer';
import { AesKwProvider, SubtleCrypto } from 'webcrypto-core';
import { arrayBufferFrom } from '../../_test_utils';
import { MockAesKwProvider } from './_test_utils';

import { AwalaAesKwProvider } from './AwalaAesKwProvider';

Expand Down Expand Up @@ -113,9 +114,3 @@ describe('onDecrypt', () => {
expect(unwrappedKey).toEqual(unwrappedKeySerialized);
});
});

class MockAesKwProvider extends AesKwProvider {
public readonly onGenerateKey = jest.fn();
public readonly onExportKey = jest.fn();
public readonly onImportKey = jest.fn();
}
17 changes: 15 additions & 2 deletions src/lib/crypto_wrappers/webcrypto/AwalaCrypto.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { getCiphers } from 'crypto';
import { SubtleCrypto } from 'webcrypto-core';

import { getMockInstance } from '../../_test_utils';
import { MockAesKwProvider } from './_test_utils';
import { AwalaAesKwProvider } from './AwalaAesKwProvider';
import { AwalaCrypto } from './AwalaCrypto';

Expand All @@ -16,10 +17,12 @@ const CIPHERS: readonly string[] = [
'aes-128-ofb',
];

beforeEach(() => {
getMockInstance(getCiphers).mockReturnValue(CIPHERS);
});

describe('Constructor', () => {
test("Pure JavaScript AES-KW provider should be used if Node doesn't support cipher", () => {
getMockInstance(getCiphers).mockReturnValue(CIPHERS);

const crypto = new AwalaCrypto();

const aesKwProvider = (crypto.subtle as SubtleCrypto).providers.get('AES-KW');
Expand All @@ -35,4 +38,14 @@ describe('Constructor', () => {
expect(aesKwProvider).toBeTruthy();
expect(aesKwProvider).not.toBeInstanceOf(AwalaAesKwProvider);
});

test('Custom providers should be registered', () => {
const providerName = 'COOL-PROVIDER';
const customProvider = new (class extends MockAesKwProvider {
override readonly name = providerName as any;
})();
const crypto = new AwalaCrypto([customProvider]);

expect((crypto.subtle as SubtleCrypto).providers.get(providerName)).toBe(customProvider);
});
});
9 changes: 6 additions & 3 deletions src/lib/crypto_wrappers/webcrypto/AwalaCrypto.ts
Original file line number Diff line number Diff line change
@@ -1,20 +1,23 @@
import { Crypto as BaseCrypto } from '@peculiar/webcrypto';
import { getCiphers } from 'crypto';
import { AesKwProvider, SubtleCrypto } from 'webcrypto-core';
import { AesKwProvider, ProviderCrypto, SubtleCrypto } from 'webcrypto-core';

import { AwalaAesKwProvider } from './AwalaAesKwProvider';

export class AwalaCrypto extends BaseCrypto {
constructor() {
constructor(customProviders: readonly ProviderCrypto[] = []) {
super();

const providers = (this.subtle as SubtleCrypto).providers;

const doesNodejsSupportAesKw = getCiphers().includes('id-aes128-wrap');
if (!doesNodejsSupportAesKw) {
// This must be running on Electron, so let's use a pure JavaScript implementation of AES-KW:
// https://github.com/relaycorp/relaynet-core-js/issues/367
const providers = (this.subtle as SubtleCrypto).providers;
const nodejsAesKwProvider = providers.get('AES-KW') as AesKwProvider;
providers.set(new AwalaAesKwProvider(nodejsAesKwProvider));
}

customProviders.forEach((p) => providers.set(p));
}
}
7 changes: 7 additions & 0 deletions src/lib/crypto_wrappers/webcrypto/_test_utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { AesKwProvider } from 'webcrypto-core';

export class MockAesKwProvider extends AesKwProvider {
public readonly onGenerateKey = jest.fn();
public readonly onExportKey = jest.fn();
public readonly onImportKey = jest.fn();
}
52 changes: 52 additions & 0 deletions src/lib/crypto_wrappers/webcrypto/engine.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import { SubtleCrypto } from 'webcrypto-core';

import { PrivateKey } from '../PrivateKey';
import { MockAesKwProvider } from './_test_utils';
import { getEngineForPrivateKey } from './engine';

describe('getEngine', () => {
const PROVIDER = new MockAesKwProvider();

test('undefined should be returned if CryptoKey is used', () => {
const engine = getEngineForPrivateKey(null as unknown as CryptoKey);

expect(engine).toBeUndefined();
});

test('Nameless engine should be returned if PrivateKey is used', () => {
const key = new PrivateKey(PROVIDER);

const engine = getEngineForPrivateKey(key);

expect(engine?.name).toBeEmpty();
});

test('Engine crypto should use provider from private key', () => {
const key = new PrivateKey(PROVIDER);

const engine = getEngineForPrivateKey(key);

expect((engine?.crypto.subtle as SubtleCrypto).providers.get(PROVIDER.name)).toBe(PROVIDER);
});

test('Same engine should be returned if multiple keys share provider', () => {
// This is to check engines are being cached
const key1 = new PrivateKey(PROVIDER);
const key2 = new PrivateKey(PROVIDER);

const engine1 = getEngineForPrivateKey(key1);
const engine2 = getEngineForPrivateKey(key2);

expect(engine1).toBe(engine2);
});

test('Different engines should be returned if keys use different providers', () => {
const key1 = new PrivateKey(PROVIDER);
const key2 = new PrivateKey(new MockAesKwProvider());

const engine1 = getEngineForPrivateKey(key1);
const engine2 = getEngineForPrivateKey(key2);

expect(engine1).not.toBe(engine2);
});
});
28 changes: 28 additions & 0 deletions src/lib/crypto_wrappers/webcrypto/engine.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { CryptoEngine } from 'pkijs';
import { ProviderCrypto } from 'webcrypto-core';

import { PrivateKey } from '../PrivateKey';
import { AwalaCrypto } from './AwalaCrypto';

const ENGINE_BY_PROVIDER = new WeakMap<ProviderCrypto, CryptoEngine>();

/**
* Generate and cache PKI.js engine for specified private key.
*/
export function getEngineForPrivateKey(
privateKey: PrivateKey | CryptoKey,
): CryptoEngine | undefined {
if (!(privateKey instanceof PrivateKey)) {
return undefined;
}

const cachedEngine = ENGINE_BY_PROVIDER.get(privateKey.provider);
if (cachedEngine) {
return cachedEngine;
}

const crypto = new AwalaCrypto([privateKey.provider]);
const engine = new CryptoEngine({ crypto });
ENGINE_BY_PROVIDER.set(privateKey.provider, engine);
return engine;
}
10 changes: 6 additions & 4 deletions src/lib/crypto_wrappers/x509/Certificate.spec.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { Crypto } from '@peculiar/webcrypto';
import * as asn1js from 'asn1js';
import bufferToArrayBuffer from 'buffer-to-arraybuffer';
import { addDays, addSeconds, setMilliseconds, subSeconds } from 'date-fns';
Expand All @@ -14,6 +13,8 @@ import {
getPrivateAddressFromIdentityKey,
} from '../keys';
import { PrivateKey } from '../PrivateKey';
import { MockAesKwProvider } from '../webcrypto/_test_utils';
import { getEngineForPrivateKey } from '../webcrypto/engine';
import Certificate from './Certificate';
import CertificateError from './CertificateError';

Expand Down Expand Up @@ -110,8 +111,7 @@ describe('issue()', () => {
});

test('should use crypto engine in private key if set', async () => {
const crypto = new Crypto();
const privateKey = new PrivateKey(crypto);
const privateKey = new PrivateKey(new MockAesKwProvider());
// tslint:disable-next-line:no-object-mutation
privateKey.algorithm = subjectKeyPair.privateKey.algorithm;
jest.spyOn(pkijs.Certificate.prototype, 'sign');
Expand All @@ -124,10 +124,12 @@ describe('issue()', () => {
}),
).toReject();

const engine = getEngineForPrivateKey(privateKey);
expect(engine).toBeInstanceOf(pkijs.CryptoEngine);
expect(pkijs.Certificate.prototype.sign).toBeCalledWith(
expect.anything(),
expect.anything(),
expect.toSatisfy<pkijs.CryptoEngine>((e) => e.crypto === crypto),
engine,
);
});

Expand Down
5 changes: 3 additions & 2 deletions src/lib/crypto_wrappers/x509/Certificate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,9 @@ import { min, setMilliseconds } from 'date-fns';
import * as pkijs from 'pkijs';

import * as oids from '../../oids';
import { derDeserialize, generateRandom64BitValue, getEngineFromPrivateKey } from '../_utils';
import { derDeserialize, generateRandom64BitValue } from '../_utils';
import { getPrivateAddressFromIdentityKey, getPublicKeyDigest } from '../keys';
import { getEngineForPrivateKey } from '../webcrypto/engine';
import CertificateError from './CertificateError';
import FullCertificateIssuanceOptions from './FullCertificateIssuanceOptions';
import { assertPkiType, assertUndefined } from '../cms/_utils';
Expand Down Expand Up @@ -109,7 +110,7 @@ export default class Certificate {

const signatureHashAlgo = (options.issuerPrivateKey.algorithm as RsaHashedKeyGenParams)
.hash as Algorithm;
const engine = getEngineFromPrivateKey(options.issuerPrivateKey);
const engine = getEngineForPrivateKey(options.issuerPrivateKey);
await pkijsCert.sign(options.issuerPrivateKey, signatureHashAlgo.name, engine);
return new Certificate(pkijsCert);
}
Expand Down

0 comments on commit dea502a

Please sign in to comment.