From c0e5339edfa32df92f23fb9c920796b4b59adf52 Mon Sep 17 00:00:00 2001 From: Ariel Gentile Date: Mon, 20 Feb 2023 09:04:20 -0300 Subject: [PATCH] fix: seed and private key validation and return type in registrars (#1324) Signed-off-by: Ariel Gentile --- packages/askar/src/wallet/AskarWallet.ts | 12 ++++++++- .../src/wallet/__tests__/AskarWallet.test.ts | 2 +- .../tests/bbs-signing-provider.e2e.test.ts | 4 +-- packages/core/src/crypto/index.ts | 1 + packages/core/src/crypto/keyUtils.ts | 27 +++++++++++++++++++ .../dids/__tests__/dids-registrar.e2e.test.ts | 8 +++--- .../dids/methods/key/KeyDidRegistrar.ts | 26 ++---------------- .../key/__tests__/KeyDidRegistrar.test.ts | 9 ++++--- .../dids/methods/peer/PeerDidRegistrar.ts | 26 ++---------------- .../peer/__tests__/PeerDidRegistrar.test.ts | 9 ++++--- packages/indy-sdk/package.json | 2 +- .../src/dids/IndySdkSovDidRegistrar.ts | 6 ++--- .../__tests__/IndySdkSovDidRegistrar.test.ts | 8 +++--- packages/indy-sdk/src/wallet/IndySdkWallet.ts | 12 ++++++++- .../tests/sov-did-registrar.e2e.test.ts | 2 +- .../src/dids/IndyVdrIndyDidRegistrar.ts | 26 ++---------------- 16 files changed, 84 insertions(+), 96 deletions(-) create mode 100644 packages/core/src/crypto/keyUtils.ts diff --git a/packages/askar/src/wallet/AskarWallet.ts b/packages/askar/src/wallet/AskarWallet.ts index 0fc11a3237..b11c5f9371 100644 --- a/packages/askar/src/wallet/AskarWallet.ts +++ b/packages/askar/src/wallet/AskarWallet.ts @@ -14,6 +14,8 @@ import type { import type { Session } from '@hyperledger/aries-askar-shared' import { + isValidSeed, + isValidPrivateKey, JsonTransformer, RecordNotFoundError, RecordDuplicateError, @@ -344,7 +346,15 @@ export class AskarWallet implements Wallet { public async createKey({ seed, privateKey, keyType }: WalletCreateKeyOptions): Promise { try { if (seed && privateKey) { - throw new AriesFrameworkError('Only one of seed and privateKey can be set') + throw new WalletError('Only one of seed and privateKey can be set') + } + + if (seed && !isValidSeed(seed, keyType)) { + throw new WalletError('Invalid seed provided') + } + + if (privateKey && !isValidPrivateKey(privateKey, keyType)) { + throw new WalletError('Invalid private key provided') } if (keyTypeSupportedByAskar(keyType)) { diff --git a/packages/askar/src/wallet/__tests__/AskarWallet.test.ts b/packages/askar/src/wallet/__tests__/AskarWallet.test.ts index e5ac122786..37d28b14b6 100644 --- a/packages/askar/src/wallet/__tests__/AskarWallet.test.ts +++ b/packages/askar/src/wallet/__tests__/AskarWallet.test.ts @@ -36,7 +36,7 @@ const walletConfig: WalletConfig = { describe('AskarWallet basic operations', () => { let askarWallet: AskarWallet - const seed = TypedArrayEncoder.fromString('sample-seed') + const seed = TypedArrayEncoder.fromString('sample-seed-min-of-32-bytes-long') const privateKey = TypedArrayEncoder.fromString('2103de41b4ae37e8e28586d84a342b67') const message = TypedArrayEncoder.fromString('sample-message') diff --git a/packages/bbs-signatures/tests/bbs-signing-provider.e2e.test.ts b/packages/bbs-signatures/tests/bbs-signing-provider.e2e.test.ts index e956c633d7..67e2112e96 100644 --- a/packages/bbs-signatures/tests/bbs-signing-provider.e2e.test.ts +++ b/packages/bbs-signatures/tests/bbs-signing-provider.e2e.test.ts @@ -26,7 +26,7 @@ const walletConfig: WalletConfig = { describeSkipNode17And18('BBS Signing Provider', () => { let wallet: Wallet - const seed = TypedArrayEncoder.fromString('sample-seed') + const seed = TypedArrayEncoder.fromString('sample-seed-min-of-32-bytes-long') const message = TypedArrayEncoder.fromString('sample-message') beforeEach(async () => { @@ -41,7 +41,7 @@ describeSkipNode17And18('BBS Signing Provider', () => { test('Create bls12381g2 keypair', async () => { await expect(wallet.createKey({ seed, keyType: KeyType.Bls12381g2 })).resolves.toMatchObject({ publicKeyBase58: - 't54oLBmhhRcDLUyWTvfYRWw8VRXRy1p43pVm62hrpShrYPuHe9WNAgS33DPfeTK6xK7iPrtJDwCHZjYgbFYDVTJHxXex9xt2XEGF8D356jBT1HtqNeucv3YsPLfTWcLcpFA', + '25TvGExLTWRTgn9h2wZuohrQmmLafXiacY4dhv66wcbY8pLbuNTBRMTgWVcPKh2wsEyrRPmnhLdc4C7LEcJ2seoxzBkoydJEdQD8aqg5dw8wesBTS9Twg8EjuFG1WPRAiERd', keyType: KeyType.Bls12381g2, }) }) diff --git a/packages/core/src/crypto/index.ts b/packages/core/src/crypto/index.ts index 449f3e537f..45f8e62972 100644 --- a/packages/core/src/crypto/index.ts +++ b/packages/core/src/crypto/index.ts @@ -2,6 +2,7 @@ export { Jwk } from './JwkTypes' export { JwsService } from './JwsService' export * from './jwtUtils' +export * from './keyUtils' export { KeyType } from './KeyType' export { Key } from './Key' diff --git a/packages/core/src/crypto/keyUtils.ts b/packages/core/src/crypto/keyUtils.ts new file mode 100644 index 0000000000..9022d2095d --- /dev/null +++ b/packages/core/src/crypto/keyUtils.ts @@ -0,0 +1,27 @@ +import { Buffer } from '../utils' + +import { KeyType } from './KeyType' + +export function isValidSeed(seed: Buffer, keyType: KeyType): boolean { + const minimumSeedLength: Record = { + [KeyType.Ed25519]: 32, + [KeyType.X25519]: 32, + [KeyType.Bls12381g1]: 32, + [KeyType.Bls12381g2]: 32, + [KeyType.Bls12381g1g2]: 32, + } + + return Buffer.isBuffer(seed) && seed.length >= minimumSeedLength[keyType] +} + +export function isValidPrivateKey(privateKey: Buffer, keyType: KeyType): boolean { + const privateKeyLength: Record = { + [KeyType.Ed25519]: 32, + [KeyType.X25519]: 32, + [KeyType.Bls12381g1]: 32, + [KeyType.Bls12381g2]: 32, + [KeyType.Bls12381g1g2]: 32, + } + + return Buffer.isBuffer(privateKey) && privateKey.length === privateKeyLength[keyType] +} diff --git a/packages/core/src/modules/dids/__tests__/dids-registrar.e2e.test.ts b/packages/core/src/modules/dids/__tests__/dids-registrar.e2e.test.ts index 70aa731310..590933f882 100644 --- a/packages/core/src/modules/dids/__tests__/dids-registrar.e2e.test.ts +++ b/packages/core/src/modules/dids/__tests__/dids-registrar.e2e.test.ts @@ -90,12 +90,14 @@ describe('dids', () => { ], id: 'did:key:z6MkpGR4gs4Rc3Zph4vj8wRnjnAxgAPSxcR8MAVKutWspQzc', }, - secret: { privateKey: '96213c3d7fc8d4d6754c7a0fd969598e' }, + secret: { privateKey: TypedArrayEncoder.fromString('96213c3d7fc8d4d6754c7a0fd969598e') }, }, }) }) it('should create a did:peer did', async () => { + const privateKey = TypedArrayEncoder.fromString('e008ef10b7c163114b3857542b3736eb') + const did = await agent.dids.create({ method: 'peer', options: { @@ -103,7 +105,7 @@ describe('dids', () => { numAlgo: PeerDidNumAlgo.InceptionKeyWithoutDoc, }, secret: { - privateKey: TypedArrayEncoder.fromString('e008ef10b7c163114b3857542b3736eb'), + privateKey, }, }) @@ -153,7 +155,7 @@ describe('dids', () => { ], id: 'did:peer:0z6Mkuo91yRhTWDrFkdNBcLXAbvtUiq2J9E4QQcfYZt4hevkh', }, - secret: { privateKey: 'e008ef10b7c163114b3857542b3736eb' }, + secret: { privateKey }, }, }) }) diff --git a/packages/core/src/modules/dids/methods/key/KeyDidRegistrar.ts b/packages/core/src/modules/dids/methods/key/KeyDidRegistrar.ts index 645deee3a1..d4c8c239c9 100644 --- a/packages/core/src/modules/dids/methods/key/KeyDidRegistrar.ts +++ b/packages/core/src/modules/dids/methods/key/KeyDidRegistrar.ts @@ -30,28 +30,6 @@ export class KeyDidRegistrar implements DidRegistrar { } } - if (seed && (typeof seed !== 'object' || seed.length !== 32)) { - return { - didDocumentMetadata: {}, - didRegistrationMetadata: {}, - didState: { - state: 'failed', - reason: 'Invalid seed provided', - }, - } - } - - if (privateKey && (typeof privateKey !== 'object' || privateKey.length !== 32)) { - return { - didDocumentMetadata: {}, - didRegistrationMetadata: {}, - didState: { - state: 'failed', - reason: 'Invalid private key provided', - }, - } - } - try { const key = await agentContext.wallet.createKey({ keyType, @@ -81,8 +59,8 @@ export class KeyDidRegistrar implements DidRegistrar { // we can only return it if the seed was passed in by the user. Once // we have a secure method for generating seeds we should use the same // approach - seed: options.secret?.seed?.toString(), - privateKey: options.secret?.privateKey?.toString(), + seed: options.secret?.seed, + privateKey: options.secret?.privateKey, }, }, } diff --git a/packages/core/src/modules/dids/methods/key/__tests__/KeyDidRegistrar.test.ts b/packages/core/src/modules/dids/methods/key/__tests__/KeyDidRegistrar.test.ts index 9f084a5b8d..d3bf481409 100644 --- a/packages/core/src/modules/dids/methods/key/__tests__/KeyDidRegistrar.test.ts +++ b/packages/core/src/modules/dids/methods/key/__tests__/KeyDidRegistrar.test.ts @@ -5,6 +5,7 @@ import { KeyType } from '../../../../../crypto' import { Key } from '../../../../../crypto/Key' import { TypedArrayEncoder } from '../../../../../utils' import { JsonTransformer } from '../../../../../utils/JsonTransformer' +import { WalletError } from '../../../../../wallet/error' import { DidDocumentRole } from '../../../domain/DidDocumentRole' import { DidRepository } from '../../../repository/DidRepository' import { KeyDidRegistrar } from '../KeyDidRegistrar' @@ -53,7 +54,7 @@ describe('DidRegistrar', () => { did: 'did:key:z6MksLeew51QS6Ca6tVKM56LQNbxCNVcLHv4xXj4jMkAhPWU', didDocument: didKeyz6MksLeFixture, secret: { - privateKey: '96213c3d7fc8d4d6754c712fd969598e', + privateKey, }, }, }) @@ -78,10 +79,10 @@ describe('DidRegistrar', () => { }) }) - it('should return an error state if an invalid private key is provided', async () => { + it('should return an error state if a key creation error is thrown', async () => { + mockFunction(walletMock.createKey).mockRejectedValueOnce(new WalletError('Invalid private key provided')) const result = await keyDidRegistrar.create(agentContext, { method: 'key', - options: { keyType: KeyType.Ed25519, }, @@ -95,7 +96,7 @@ describe('DidRegistrar', () => { didRegistrationMetadata: {}, didState: { state: 'failed', - reason: 'Invalid private key provided', + reason: expect.stringContaining('Invalid private key provided'), }, }) }) diff --git a/packages/core/src/modules/dids/methods/peer/PeerDidRegistrar.ts b/packages/core/src/modules/dids/methods/peer/PeerDidRegistrar.ts index 83b171e978..fcae22119e 100644 --- a/packages/core/src/modules/dids/methods/peer/PeerDidRegistrar.ts +++ b/packages/core/src/modules/dids/methods/peer/PeerDidRegistrar.ts @@ -41,28 +41,6 @@ export class PeerDidRegistrar implements DidRegistrar { } } - if (seed && (typeof seed !== 'object' || seed.length !== 32)) { - return { - didDocumentMetadata: {}, - didRegistrationMetadata: {}, - didState: { - state: 'failed', - reason: 'Invalid seed provided', - }, - } - } - - if (privateKey && (typeof privateKey !== 'object' || privateKey.length !== 32)) { - return { - didDocumentMetadata: {}, - didRegistrationMetadata: {}, - didState: { - state: 'failed', - reason: 'Invalid private key provided', - }, - } - } - const key = await agentContext.wallet.createKey({ keyType, seed, @@ -119,8 +97,8 @@ export class PeerDidRegistrar implements DidRegistrar { // we can only return it if the seed was passed in by the user. Once // we have a secure method for generating seeds we should use the same // approach - seed: options.secret?.seed?.toString(), - privateKey: options.secret?.privateKey?.toString(), + seed: options.secret?.seed, + privateKey: options.secret?.privateKey, }, }, } diff --git a/packages/core/src/modules/dids/methods/peer/__tests__/PeerDidRegistrar.test.ts b/packages/core/src/modules/dids/methods/peer/__tests__/PeerDidRegistrar.test.ts index b974cff303..50b862f772 100644 --- a/packages/core/src/modules/dids/methods/peer/__tests__/PeerDidRegistrar.test.ts +++ b/packages/core/src/modules/dids/methods/peer/__tests__/PeerDidRegistrar.test.ts @@ -5,6 +5,7 @@ import { KeyType } from '../../../../../crypto' import { Key } from '../../../../../crypto/Key' import { TypedArrayEncoder } from '../../../../../utils' import { JsonTransformer } from '../../../../../utils/JsonTransformer' +import { WalletError } from '../../../../../wallet/error' import { DidCommV1Service, DidDocumentBuilder } from '../../../domain' import { DidDocumentRole } from '../../../domain/DidDocumentRole' import { getEd25519VerificationMethod } from '../../../domain/key-type/ed25519' @@ -54,7 +55,7 @@ describe('DidRegistrar', () => { did: 'did:peer:0z6MksLeew51QS6Ca6tVKM56LQNbxCNVcLHv4xXj4jMkAhPWU', didDocument: didPeer0z6MksLeFixture, secret: { - privateKey: '96213c3d7fc8d4d6754c712fd969598e', + privateKey, }, }, }) @@ -79,7 +80,9 @@ describe('DidRegistrar', () => { }) }) - it('should return an error state if an invalid private key is provided', async () => { + it('should return an error state if a key creation error is thrown', async () => { + mockFunction(walletMock.createKey).mockRejectedValueOnce(new WalletError('Invalid private key provided')) + const result = await peerDidRegistrar.create(agentContext, { method: 'peer', options: { @@ -96,7 +99,7 @@ describe('DidRegistrar', () => { didRegistrationMetadata: {}, didState: { state: 'failed', - reason: 'Invalid private key provided', + reason: expect.stringContaining('Invalid private key provided'), }, }) }) diff --git a/packages/indy-sdk/package.json b/packages/indy-sdk/package.json index 8e6acc74c4..12d019bdae 100644 --- a/packages/indy-sdk/package.json +++ b/packages/indy-sdk/package.json @@ -27,13 +27,13 @@ "@aries-framework/anoncreds": "0.3.3", "@aries-framework/core": "0.3.3", "@types/indy-sdk": "1.16.26", + "@stablelib/ed25519": "^1.0.3", "class-transformer": "^0.5.1", "class-validator": "^0.14.0", "rxjs": "^7.2.0", "tsyringe": "^4.7.0" }, "devDependencies": { - "@stablelib/ed25519": "^1.0.3", "rimraf": "^4.0.7", "typescript": "~4.9.4" } diff --git a/packages/indy-sdk/src/dids/IndySdkSovDidRegistrar.ts b/packages/indy-sdk/src/dids/IndySdkSovDidRegistrar.ts index 7f7cf38ebd..4c88f84427 100644 --- a/packages/indy-sdk/src/dids/IndySdkSovDidRegistrar.ts +++ b/packages/indy-sdk/src/dids/IndySdkSovDidRegistrar.ts @@ -13,7 +13,7 @@ import type { } from '@aries-framework/core' import type { NymRole } from 'indy-sdk' -import { DidDocumentRole, DidRecord, DidRepository } from '@aries-framework/core' +import { KeyType, isValidPrivateKey, DidDocumentRole, DidRecord, DidRepository } from '@aries-framework/core' import { IndySdkError } from '../error' import { isIndyError } from '../error/indyError' @@ -34,7 +34,7 @@ export class IndySdkSovDidRegistrar implements DidRegistrar { const { alias, role, submitterDid, indyNamespace } = options.options const privateKey = options.secret?.privateKey - if (privateKey && (typeof privateKey !== 'object' || privateKey.length !== 32)) { + if (privateKey && !isValidPrivateKey(privateKey, KeyType.Ed25519)) { return { didDocumentMetadata: {}, didRegistrationMetadata: {}, @@ -120,7 +120,7 @@ export class IndySdkSovDidRegistrar implements DidRegistrar { // we can only return it if the seed was passed in by the user. Once // we have a secure method for generating seeds we should use the same // approach - privateKey: options.secret?.privateKey?.toString(), + privateKey: options.secret?.privateKey, }, }, } diff --git a/packages/indy-sdk/src/dids/__tests__/IndySdkSovDidRegistrar.test.ts b/packages/indy-sdk/src/dids/__tests__/IndySdkSovDidRegistrar.test.ts index ab3f31bd4f..29ea34618c 100644 --- a/packages/indy-sdk/src/dids/__tests__/IndySdkSovDidRegistrar.test.ts +++ b/packages/indy-sdk/src/dids/__tests__/IndySdkSovDidRegistrar.test.ts @@ -131,7 +131,7 @@ describe('IndySdkSovDidRegistrar', () => { }) it('should correctly create a did:sov document without services', async () => { - const privateKey = '96213c3d7fc8d4d6754c712fd969598e' + const privateKey = TypedArrayEncoder.fromString('96213c3d7fc8d4d6754c712fd969598e') const registerPublicDidSpy = jest.spyOn(indySdkSovDidRegistrar, 'registerPublicDid') registerPublicDidSpy.mockImplementationOnce(() => Promise.resolve('R1xKJw17sUoXhejEpugMYJ')) @@ -144,7 +144,7 @@ describe('IndySdkSovDidRegistrar', () => { role: 'STEWARD', }, secret: { - privateKey: TypedArrayEncoder.fromString(privateKey), + privateKey, }, }) @@ -206,7 +206,7 @@ describe('IndySdkSovDidRegistrar', () => { }) it('should correctly create a did:sov document with services', async () => { - const privateKey = '96213c3d7fc8d4d6754c712fd969598e' + const privateKey = TypedArrayEncoder.fromString('96213c3d7fc8d4d6754c712fd969598e') const registerPublicDidSpy = jest.spyOn(indySdkSovDidRegistrar, 'registerPublicDid') registerPublicDidSpy.mockImplementationOnce(() => Promise.resolve('R1xKJw17sUoXhejEpugMYJ')) @@ -227,7 +227,7 @@ describe('IndySdkSovDidRegistrar', () => { }, }, secret: { - privateKey: TypedArrayEncoder.fromString(privateKey), + privateKey, }, }) diff --git a/packages/indy-sdk/src/wallet/IndySdkWallet.ts b/packages/indy-sdk/src/wallet/IndySdkWallet.ts index 16561bbcf1..d65b95ada2 100644 --- a/packages/indy-sdk/src/wallet/IndySdkWallet.ts +++ b/packages/indy-sdk/src/wallet/IndySdkWallet.ts @@ -33,6 +33,8 @@ import { Key, SigningProviderRegistry, TypedArrayEncoder, + isValidSeed, + isValidPrivateKey, } from '@aries-framework/core' import { inject, injectable } from 'tsyringe' @@ -415,7 +417,15 @@ export class IndySdkWallet implements Wallet { public async createKey({ seed, privateKey, keyType }: WalletCreateKeyOptions): Promise { try { if (seed && privateKey) { - throw new AriesFrameworkError('Only one of seed and privateKey can be set') + throw new WalletError('Only one of seed and privateKey can be set') + } + + if (seed && !isValidSeed(seed, keyType)) { + throw new WalletError('Invalid seed provided') + } + + if (privateKey && !isValidPrivateKey(privateKey, keyType)) { + throw new WalletError('Invalid private key provided') } // Ed25519 is supported natively in Indy wallet diff --git a/packages/indy-sdk/tests/sov-did-registrar.e2e.test.ts b/packages/indy-sdk/tests/sov-did-registrar.e2e.test.ts index de121b462d..eef3b5c8dd 100644 --- a/packages/indy-sdk/tests/sov-did-registrar.e2e.test.ts +++ b/packages/indy-sdk/tests/sov-did-registrar.e2e.test.ts @@ -117,7 +117,7 @@ describe('dids', () => { id: `did:sov:${indyDid}`, }, secret: { - privateKey: privateKey.toString(), + privateKey, }, }, }) diff --git a/packages/indy-vdr/src/dids/IndyVdrIndyDidRegistrar.ts b/packages/indy-vdr/src/dids/IndyVdrIndyDidRegistrar.ts index cf41881cad..50735a67eb 100644 --- a/packages/indy-vdr/src/dids/IndyVdrIndyDidRegistrar.ts +++ b/packages/indy-vdr/src/dids/IndyVdrIndyDidRegistrar.ts @@ -44,28 +44,6 @@ export class IndyVdrIndyDidRegistrar implements DidRegistrar { const seed = options.secret?.seed const privateKey = options.secret?.privateKey - if (privateKey && (typeof privateKey !== 'object' || privateKey.length !== 32)) { - return { - didDocumentMetadata: {}, - didRegistrationMetadata: {}, - didState: { - state: 'failed', - reason: 'Invalid privateKey provided', - }, - } - } - - if (seed && (typeof seed !== 'object' || seed.length !== 32)) { - return { - didDocumentMetadata: {}, - didRegistrationMetadata: {}, - didState: { - state: 'failed', - reason: 'Invalid seed provided', - }, - } - } - const { alias, role, submitterDid, submitterVerkey, services, useEndpointAttrib } = options.options let verkey = options.options.verkey let did = options.did @@ -202,8 +180,8 @@ export class IndyVdrIndyDidRegistrar implements DidRegistrar { // we can only return it if the seed was passed in by the user. Once // we have a secure method for generating seeds we should use the same // approach - seed: options.secret?.seed?.toString(), - privateKey: options.secret?.privateKey?.toString(), + seed: options.secret?.seed, + privateKey: options.secret?.privateKey, }, }, }