fix: P256 key parity corrections (#1137)

* fix(data-store): decrypt keys before listing in `PrivateKeyStore.listKeys()` * fix(data-store-json): decrypt before listing in `PrivateKeyStoreJson.listKeys()` * fix(kms-local): include parity when computing publicKeyHex for P-256 curve; Existing keys still have to be updated manually * fix(did-provider-jwk): use correct parity for P-256 public keys fixes #1136 closes #1135
decentralized-identity · Feb 24, 2023 · d0eca2b · d0eca2b
1 parent 70ff72e
commit d0eca2b
Show file tree

Hide file tree

Showing 19 changed files with 227 additions and 71 deletions.
diff --git a/__tests__/data.migration.test.ts b/__tests__/data.migration.test.ts
@@ -0,0 +1,114 @@
+// noinspection ES6PreferShortImport
+
+import { KeyManagementSystem, SecretBox } from '../packages/kms-local/src'
+import { Entities, KeyStore, migrations, PrivateKeyStore } from '../packages/data-store/src'
+import { PrivateKeyStoreJson } from '../packages/data-store-json/src'
+
+import { DataSource } from 'typeorm'
+import * as fs from 'fs'
+
+import { jest } from '@jest/globals'
+import { fileURLToPath } from 'url'
+import { dirname } from 'path'
+
+// @ts-ignore TS1343
+const __filename = fileURLToPath(import.meta.url)
+const __dirname = dirname(__filename)
+
+jest.setTimeout(60000)
+
+const dbEncryptionKey = '29739248cad1bd1a0fc4d9b75cd4d2990de535baf5caadfdf8d8f86664aa830c'
+
+describe('data handling tests', () => {
+  describe('can recompute p256 keys from old database', () => {
+    const fixture = __dirname + '/fixtures/local-database-before-p256key-migration.sqlite'
+    const databaseFile = fixture + '.tmp'
+    // intentionally using DataSource instead of Promise<DataSource> to test compatibility
+    let dbConnection: DataSource
+
+    beforeAll(async () => {
+      await fs.promises.copyFile(fixture, databaseFile)
+      dbConnection = new DataSource({
+        name: 'test',
+        type: 'sqlite',
+        database: databaseFile,
+        synchronize: false,
+        migrations: migrations,
+        migrationsRun: true,
+        logging: false,
+        entities: Entities,
+      })
+    })
+
+    afterAll(async () => {
+      try {
+        await dbConnection?.destroy()
+      } catch (e: any) {
+        // nop
+      }
+      try {
+        await fs.promises.unlink(databaseFile)
+      } catch (e: any) {
+        // nop
+      }
+    })
+
+    it('should recompute p256 keys', async () => {
+      const kmsLocal = new KeyManagementSystem(
+        new PrivateKeyStore(dbConnection, new SecretBox(dbEncryptionKey)),
+      )
+      const managedKeyStore = new KeyStore(dbConnection)
+      // list known private keys. kms-local will compute the correct public keys
+      const allPrivKeys = await kmsLocal.listKeys()
+      const keyIds: string[] = []
+      for (const privKey of allPrivKeys) {
+        if (privKey.type === 'Secp256r1') {
+          const managedKey = await managedKeyStore.getKey({ kid: privKey.kid })
+          if (managedKey.publicKeyHex.length === 64) {
+            keyIds.push(privKey.kid)
+            managedKey.publicKeyHex = privKey.publicKeyHex
+          }
+          await managedKeyStore.importKey(managedKey)
+        }
+      }
+      for (const kid of keyIds) {
+        const managedKey = await managedKeyStore.getKey({ kid })
+        expect(managedKey.publicKeyHex.length).toEqual(66)
+        expect(managedKey.publicKeyHex).toMatch(/^(02|03).*/)
+      }
+    })
+  })
+  describe('kms-local maintains public key values for listKeys', () => {
+    it('when using data-store-json', async () => {
+      const memoryJsonStore = {
+        notifyUpdate: () => Promise.resolve(),
+      }
+      const kmsLocal = new KeyManagementSystem(
+        new PrivateKeyStoreJson(memoryJsonStore, new SecretBox(dbEncryptionKey)),
+      )
+      const key = await kmsLocal.createKey({ type: 'Secp256r1' })
+      const allPrivKeys = await kmsLocal.listKeys()
+      const foundKey = allPrivKeys.find((k) => k.kid === key.kid)
+      expect(foundKey?.publicKeyHex).toEqual(key.publicKeyHex)
+    })
+
+    it('when using data-store', async () => {
+      const dbConnection = new DataSource({
+        type: 'sqlite',
+        database: ':memory:',
+        entities: Entities,
+        synchronize: false,
+        migrations: migrations,
+        migrationsRun: true,
+        logging: false,
+      })
+      const kmsLocal = new KeyManagementSystem(
+        new PrivateKeyStore(dbConnection, new SecretBox(dbEncryptionKey)),
+      )
+      const key = await kmsLocal.createKey({ type: 'Secp256r1' })
+      const allPrivKeys = await kmsLocal.listKeys()
+      const foundKey = allPrivKeys.find((k) => k.kid === key.kid)
+      expect(foundKey?.publicKeyHex).toEqual(key.publicKeyHex)
+    })
+  })
+})
diff --git a/__tests__/fixtures/local-database-before-p256key-migration.sqlite b/__tests__/fixtures/local-database-before-p256key-migration.sqlite
diff --git a/__tests__/initial.migration.test.ts b/__tests__/initial.migration.test.ts
@@ -34,18 +34,19 @@ import { KeyManager } from '../packages/key-manager/src'
 import { DIDManager } from '../packages/did-manager/src'
 import { FakeDidProvider, FakeDidResolver } from '../packages/test-utils/src'
 
-import { DataSourceOptions, DataSource } from 'typeorm'
+import { DataSource, DataSourceOptions } from 'typeorm'
 import { Resolver } from 'did-resolver'
 import { getResolver as ethrDidResolver } from 'ethr-did-resolver'
 import { getResolver as webDidResolver } from 'web-did-resolver'
 import * as fs from 'fs'
 
 import { jest } from '@jest/globals'
-import { fileURLToPath } from 'url';
-import { dirname } from 'path';
+import { fileURLToPath } from 'url'
+import { dirname } from 'path'
 
-const __filename = fileURLToPath(import.meta.url);
-const __dirname = dirname(__filename);
+// @ts-ignore TS1343
+const __filename = fileURLToPath(import.meta.url)
+const __dirname = dirname(__filename)
 
 jest.setTimeout(60000)
 

diff --git a/__tests__/localAgent.test.ts b/__tests__/localAgent.test.ts
@@ -116,7 +116,7 @@ let databaseFile: string
 
 const setup = async (options?: IAgentOptions): Promise<boolean> => {
   databaseFile =
-    options?.context?.databaseFile || `./tmp/local-database-${Math.random().toPrecision(5)}.sqlite`
+    options?.context?.databaseFile || ':memory:'
   dbConnection = new DataSource({
     name: options?.context?.['dbName'] || 'test',
     type: 'sqlite',

diff --git a/__tests__/localMemoryStoreAgent.test.ts b/__tests__/localMemoryStoreAgent.test.ts
@@ -30,7 +30,9 @@ import {
   ICredentialIssuerLD,
   LdDefaultContexts,
   VeramoEcdsaSecp256k1RecoverySignature2020,
-  VeramoEd25519Signature2018, VeramoEd25519Signature2020, VeramoJsonWebSignature2020,
+  VeramoEd25519Signature2018,
+  VeramoEd25519Signature2020,
+  VeramoJsonWebSignature2020,
 } from '../packages/credential-ld/src'
 import { EthrDIDProvider } from '../packages/did-provider-ethr/src'
 import { WebDIDProvider } from '../packages/did-provider-web/src'
@@ -73,7 +75,6 @@ import credentialInterop from './shared/credentialInterop.js'
 
 jest.setTimeout(60000)
 
-const databaseFile = `./tmp/local-database2-${Math.random().toPrecision(5)}.sqlite`
 const infuraProjectId = '3586660d179141e3801c3895de1c2eba'
 
 let agent: TAgent<
@@ -96,7 +97,7 @@ const setup = async (options?: IAgentOptions): Promise<boolean> => {
   dbConnection = new DataSource({
     name: 'test',
     type: 'sqlite',
-    database: databaseFile,
+    database: ':memory:',
     synchronize: false,
     migrations: migrations,
     migrationsRun: true,
@@ -207,16 +208,10 @@ const setup = async (options?: IAgentOptions): Promise<boolean> => {
 
 const tearDown = async (): Promise<boolean> => {
   try {
-    await (await dbConnection).dropDatabase()
-    await (await dbConnection).close()
+    await dbConnection?.destroy()
   } catch (e) {
     // nop
   }
-  try {
-    fs.unlinkSync(databaseFile)
-  } catch (e) {
-    //nop
-  }
   return true
 }
 

diff --git a/packages/cli/src/__tests__/cli.test.ts b/packages/cli/src/__tests__/cli.test.ts
@@ -24,7 +24,7 @@ describe('cli version', () => {
   })
 
   // this seems to fail because of an incompatibility between jest and the `multiformats@11` transitive dependency
-  it.skip('should load the agent', async () => {
+  it('should load the agent', async () => {
     const { agent } = await createObjects(await getConfig('./packages/cli/default/default.yml'), {
       agent: '/agent',
     })

diff --git a/packages/data-store-json/src/identifier/private-key-store.ts b/packages/data-store-json/src/identifier/private-key-store.ts
@@ -26,8 +26,8 @@ export class PrivateKeyStoreJson extends AbstractPrivateKeyStore {
   private readonly notifyUpdate: DiffCallback
 
   /**
-   * @param jsonStore - This serves as the JSON object storing data in memory as well as providing an update notification
-   *   callback to persist this data. The JSON object does not have to be shared with other users of
+   * @param jsonStore - This serves as the JSON object storing data in memory as well as providing an update
+   *   notification callback to persist this data. The JSON object does not have to be shared with other users of
    *   {@link VeramoJsonStore}, but it can be.
    * @param secretBox - If this is used, then key material is encrypted, even in memory.
    */
@@ -66,10 +66,12 @@ export class PrivateKeyStoreJson extends AbstractPrivateKeyStore {
   async importKey(args: ImportablePrivateKey): Promise<ManagedPrivateKey> {
     debug('Saving private key data', args.alias)
     const alias = args.alias || uuid4()
-    const key: ManagedPrivateKey = deserialize(serialize({
-      ...args,
-      alias,
-    }))
+    const key: ManagedPrivateKey = deserialize(
+      serialize({
+        ...args,
+        alias,
+      }),
+    )
     if (this.secretBox && key.privateKeyHex) {
       const copy = key.privateKeyHex
       key.privateKeyHex = await this.secretBox.encrypt(copy)
@@ -89,6 +91,12 @@ export class PrivateKeyStoreJson extends AbstractPrivateKeyStore {
   }
 
   async listKeys(): Promise<Array<ManagedPrivateKey>> {
-    return deserialize(serialize(Object.values(this.cacheTree.privateKeys)))
+    const keys = Object.values(this.cacheTree.privateKeys)
+    if (this.secretBox) {
+      for (const key of keys) {
+        key.privateKeyHex = await this.secretBox.decrypt(key.privateKeyHex)
+      }
+    }
+    return deserialize(serialize(keys))
   }
 }
diff --git a/packages/data-store/src/identifier/private-key-store.ts b/packages/data-store/src/identifier/private-key-store.ts
@@ -64,7 +64,12 @@ export class PrivateKeyStore extends AbstractPrivateKeyStore {
   }
 
   async listKeys(): Promise<Array<ManagedPrivateKey>> {
-    const keys = await (await getConnectedDb(this.dbConnection)).getRepository(PrivateKey).find()
+    let keys = await (await getConnectedDb(this.dbConnection)).getRepository(PrivateKey).find()
+    if (this.secretBox) {
+      for (const key of keys) {
+        key.privateKeyHex = await this.secretBox?.decrypt(key.privateKeyHex) as string
+      }
+    }
     return keys
   }
 }
diff --git a/packages/did-comm/src/__tests__/coordinate-mediation-message-handler.test.ts b/packages/did-comm/src/__tests__/coordinate-mediation-message-handler.test.ts
@@ -42,8 +42,6 @@ const DIDCommEventSniffer: IEventListener = {
   onEvent: jest.fn(() => Promise.resolve()),
 }
 
-const databaseFile = `./tmp/local-database2-${Math.random().toPrecision(5)}.sqlite`
-
 describe('coordinate-mediation-message-handler', () => {
   let recipient: IIdentifier
   let mediator: IIdentifier
@@ -56,7 +54,7 @@ describe('coordinate-mediation-message-handler', () => {
     dbConnection = new DataSource({
       name: 'test',
       type: 'sqlite',
-      database: databaseFile,
+      database: ':memory:',
       synchronize: false,
       migrations: migrations,
       migrationsRun: true,
@@ -171,8 +169,14 @@ describe('coordinate-mediation-message-handler', () => {
   afterAll(async () => {
     try {
       await new Promise((resolve, reject) => didCommEndpointServer?.close(resolve))
-    } catch (e) {
-      //nop
+    } catch (e: any) {
+      // nop
+    }
+
+    try {
+      await dbConnection?.destroy()
+    } catch (e: any) {
+      // nop
     }
   })
 

diff --git a/packages/did-comm/src/__tests__/message-handler.test.ts b/packages/did-comm/src/__tests__/message-handler.test.ts
@@ -36,8 +36,6 @@ const DIDCommEventSniffer: IEventListener = {
   onEvent: jest.fn(() => Promise.resolve()),
 }
 
-const databaseFile = `./tmp/local-database2-${Math.random().toPrecision(5)}.sqlite`
-
 describe('did-comm-message-handler', () => {
   let sender: IIdentifier
   let recipient: IIdentifier
@@ -50,7 +48,7 @@ describe('did-comm-message-handler', () => {
     dbConnection = new DataSource({
       name: 'test',
       type: 'sqlite',
-      database: databaseFile,
+      database: ':memory:',
       synchronize: false,
       migrations: migrations,
       migrationsRun: true,
@@ -166,6 +164,11 @@ describe('did-comm-message-handler', () => {
     } catch (e) {
       //nop
     }
+    try {
+      await dbConnection?.destroy()
+    } catch (e: any) {
+      // nop
+    }
   })
 
   const expectMessageReceived = (id: string, packing: string) => {

diff --git a/packages/did-comm/src/__tests__/messagepickup-message-handler.test.ts b/packages/did-comm/src/__tests__/messagepickup-message-handler.test.ts
@@ -53,8 +53,6 @@ const DIDCommEventSniffer: IEventListener = {
   onEvent: jest.fn(() => Promise.resolve()),
 }
 
-const databaseFile = `./tmp/local-database2-${Math.random().toPrecision(5)}.sqlite`
-
 describe('messagepickup-message-handler', () => {
   describe('PickupMediatorMessageHandler', () => {
     let recipient: IIdentifier
@@ -73,7 +71,7 @@ describe('messagepickup-message-handler', () => {
       dbConnection = new DataSource({
         name: 'test',
         type: 'sqlite',
-        database: databaseFile,
+        database: ':memory:',
         synchronize: false,
         migrations: migrations,
         migrationsRun: true,
@@ -236,8 +234,13 @@ describe('messagepickup-message-handler', () => {
     afterAll(async () => {
       try {
         await new Promise((resolve, reject) => didCommEndpointServer?.close(resolve))
-      } catch (e) {
-        //nop
+      } catch (e: any) {
+        // nop
+      }
+      try {
+        await dbConnection?.destroy()
+      } catch (e: any) {
+        // nop
       }
     })
 
@@ -788,7 +791,7 @@ describe('messagepickup-message-handler', () => {
       dbConnection = new DataSource({
         name: 'test',
         type: 'sqlite',
-        database: databaseFile,
+        database: ':memory:',
         synchronize: false,
         migrations: migrations,
         migrationsRun: true,
@@ -951,8 +954,13 @@ describe('messagepickup-message-handler', () => {
     afterAll(async () => {
       try {
         await new Promise((resolve, reject) => didCommEndpointServer?.close(resolve))
-      } catch (e) {
-        //nop
+      } catch (e: any) {
+        // nop
+      }
+      try {
+        await dbConnection?.destroy()
+      } catch (e: any) {
+        // nop
       }
     })