fix: seed and private key validation and return type in registrars (o…

…penwallet-foundation#1324) Signed-off-by: Ariel Gentile <gentilester@gmail.com>
karimStekelenburg · Feb 20, 2023 · c0e5339 · c0e5339
1 parent edf392f
commit c0e5339
Show file tree

Hide file tree

Showing 16 changed files with 84 additions and 96 deletions.
diff --git 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<Key> {
     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
@@ -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
@@ -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
@@ -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
@@ -0,0 +1,27 @@
+import { Buffer } from '../utils'
+
+import { KeyType } from './KeyType'
+
+export function isValidSeed(seed: Buffer, keyType: KeyType): boolean {
+  const minimumSeedLength: Record<KeyType, number> = {
+    [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, number> = {
+    [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
@@ -90,20 +90,22 @@ 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<PeerDidNumAlgo0CreateOptions>({
       method: 'peer',
       options: {
         keyType: KeyType.Ed25519,
         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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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<Key> {
     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
@@ -117,7 +117,7 @@ describe('dids', () => {
           id: `did:sov:${indyDid}`,
         },
         secret: {
-          privateKey: privateKey.toString(),
+          privateKey,
         },
       },
     })