decentralized-identity / veramo

A JavaScript Framework for Verifiable Data
https://veramo.io
Apache License 2.0
443 stars 133 forks source link

Local Store Fails to Update Added Key #1362

Closed radleylewis closed 5 months ago

radleylewis commented 8 months ago

Bug severity 4

Describe the bug When adding a key, using the didManagerAddKey method, the Identifier is being successfully transmitted to the network but is not stored faithfully in the local store (i.e. the resolved DIDDoc from the network includes the X25519 key being added, however, the local store using didManagerGet resolves a DIDDoc that does not included the added key).

To Reproduce Steps to reproduce the behaviour: I have create a repository here which isolates the relevant logical calls and configures a minimal veramo agent to demonstrate the issue. There are instructions included in the README.

Observed behaviour When using a db connection to sqlite or postgres the local DIDDoc does not include the added key (despite it having been transmitted successfully to the network and available on the network resolvable DIDDoc). When using the MemoryDidStore and MemoryKeyStore (as opposed to the keystores with the persistent db connection), the correct DIDDoc is returned (i.e. the local DIDDoc includes the added X25519 key).

Expected behaviour The local DIDDoc should reflect the updated changes (added key) when using the persistent database connections (sqlite, postgres).

Details Expected Local DIDDoc (includes two keys: secp256k1, x25519 just added):

{
  "did": "did:ethr:sepolia:0x0371ab768c9d855e3b5c2bd94ba425d9105e122c1b424467f79df53e3175b30b3d",
  "controllerKeyId": "0471ab768c9d855e3b5c2bd94ba425d9105e122c1b424467f79df53e3175b30b3d3fe249e26270596f79915e6743661e4a2f587fdfcd442fd524ce8d0ddd65e35d",
  "provider": "did:ethr:sepolia",
  "services": [],
  "keys": [
    {
      "kid": "0471ab768c9d855e3b5c2bd94ba425d9105e122c1b424467f79df53e3175b30b3d3fe249e26270596f79915e6743661e4a2f587fdfcd442fd524ce8d0ddd65e35d",
      "type": "Secp256k1",
      "kms": "local",
      "publicKeyHex": "0471ab768c9d855e3b5c2bd94ba425d9105e122c1b424467f79df53e3175b30b3d3fe249e26270596f79915e6743661e4a2f587fdfcd442fd524ce8d0ddd65e35d",
      "meta": {
        "algorithms": [
          "ES256K",
          "ES256K-R",
          "eth_signTransaction",
          "eth_signTypedData",
          "eth_signMessage",
          "eth_rawSign"
        ]
      }
    },
    {
      "kms": "local",
      "kid": "didcomm-enc-key",
      "type": "X25519",
      "privateKeyHex": "87e81ef7539e97c5857b513e26718eeb84111f92f50d0f76d561b401bb22c5d1e2789bf35f6ed4864c69da6bbba1d0112d67ee5bc2292d5937115ac587daf529",
      "publicKeyHex": "e2789bf35f6ed4864c69da6bbba1d0112d67ee5bc2292d5937115ac587daf529",
      "meta": {
        "algorithms": [
          "ECDH",
          "ECDH-ES",
          "ECDH-1PU"
        ]
      }
    }
  ]
}

Actual Output (missing x25519 key):

{
  "did": "did:ethr:sepolia:0x0371ab768c9d855e3b5c2bd94ba425d9105e122c1b424467f79df53e3175b30b3d",
  "controllerKeyId": "0471ab768c9d855e3b5c2bd94ba425d9105e122c1b424467f79df53e3175b30b3d3fe249e26270596f79915e6743661e4a2f587fdfcd442fd524ce8d0ddd65e35d",
  "provider": "did:ethr:sepolia",
  "services": [],
  "keys": [
    {
      "kid": "0471ab768c9d855e3b5c2bd94ba425d9105e122c1b424467f79df53e3175b30b3d3fe249e26270596f79915e6743661e4a2f587fdfcd442fd524ce8d0ddd65e35d",
      "type": "Secp256k1",
      "kms": "local",
      "publicKeyHex": "0471ab768c9d855e3b5c2bd94ba425d9105e122c1b424467f79df53e3175b30b3d3fe249e26270596f79915e6743661e4a2f587fdfcd442fd524ce8d0ddd65e35d",
      "meta": {
        "algorithms": [
          "ES256K",
          "ES256K-R",
          "eth_signTransaction",
          "eth_signTypedData",
          "eth_signMessage",
          "eth_rawSign"
        ]
      }
    }
  ]
}

Additional context Repo to reproduce

Versions (please complete the following information):

radleylewis commented 8 months ago

I want to provide an update on this bug. Specifically, I have identified the variation in the behaviour between using the memory stores and the persistent databases (sqlite, postgres etc).

The issue pertains to the DIDStore. When the MemoryDIDStore saves an identifier, it is not concerned with any relations (i.e. keys, services) and so simply applies the save in importDID as follows:

  async importDID(args: IIdentifier) {
    const identifier = { ...args }
    for (const key of identifier.keys) {
      if (key.privateKeyHex) {
        delete key.privateKeyHex
      }
    }
    this.identifiers[args.did] = identifier // this line here
    return true
  }

However, when dealing with the persistent datastores, if the key itself has not first been imported using agent.keyManagerImport then the operation will fail. So any call to didManagerAddKey needs to be preempted with keyManagerImport.

Its arguable that this is not a bug, however, it isn't at all intuitive and I was stuck on this for a fair while. I am happy to raise a PR with a fix, however the typing in the didStore doesn't allow for easy patching.

stale[bot] commented 5 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.