[proposal] Add interface for changing the DID controller

[mirceanis]

Veramo should have a common API for changing a DID controller. As a prerequisite, the AbstractIdentifierProvider class should require implementers to also implement a changeController method.
A proposal for this new method signature is this:

abstract changeController(
    args: { identifier: IIdentifier; newController: string, options?: any },
    context: IAgentContext<IKeyManager & IDIDManager>,
  ): Promise<string>

Of all the did-provider-* packages that are currently implemented in this repository, this method would only have a concrete implementation in did-provider-ethr, in which case the newController would be an ethereum address and the returned string would be the transaction hash.
The options parameter can be used to provide transaction value overrides (gasPrice, gasLimit, nonce, etc..).

It is expected that the context.agent.keyManagerSign() method would be called to sign the transaction and that the controllerKeyId of the identifier would be updated.

Originally discussed in #574

^{Originally posted by atz3n June 17, 2021}
Hi all 👋,

Is there an interface for changing the DID controller? I couldn't find such a function within the AbstractIdentifierProvider class. If not, maybe it is a good idea to define one to have a convenient API for this task.

Cheers,
Attila

[lleifermann]

@mirceanis Lets pick this topic up again. I really like the idea of the proposed interface you had there. Thinking about did:ethr and other methods one thing comes to my mind.

Right now did:ethr does not support adding multiple owners, while other methods may support this (did:web may just add multiple keys to the controller field) - so the Interface changeOwner always suggest that it replaces some existing value. This may conflict with other implementations.

How about we add 3 abstract methods to the Abstract that an IdentifierProvider may leverage.

• changeController - For replacing an owner (can be implemented in did:ethr)
• addController - For adding a controller did and keeping the existing one (cant be implemented in did:ethr)
• removeController - For removing a controller did (also cant be implemented in ethr)

ethr-did would only implement changeController then. The only reason i see the add and remove methods is to keep consistent with the current methods, since 'change' suggest replacement why may not be wanted by other methods. Then we are in the territory of passing options again 😄

[lleifermann]

@mirceanis Any update on this? 😄

[mirceanis]

@mirceanis Any update on this? 😄

no progress yet.

How about we add 3 abstract methods to the Abstract that an IdentifierProvider may leverage.

It's always better to only make the required changes as needed instead of adding features that are not yet demanded but would still have to be maintained.
Let's start with the changeController method since it's better defined for did:ethr.

did:key, did:pkh can't update anything.
did:web documents hosted by veramo don't use a controller concept.
did:ion has different concepts for controller/recovery keys that are not necessarily visible in the DIDDocument. The existing implementation uses the metadata attached to keys to determine this.

This will be a breaking change, since any existing implementations of the AbstractIdentifierProvider class will have to be updated.

---

That being said, there is a difference between the controllerKey concept used by Veramo (starting from did:ethr assumptions where it translates to the owner address on the smart contract) and the controller concept from DID core spec where it represents an optional set of other DIDs who should be treated as having the same authority.

Coming from the DID spec, updateController suggests that it will list a different set of DIDs as the controller property of the DID document, which is not what will happen here.
With this in mind, perhaps it's best to name this method to something like: updateControllerKey or updateControllerMetadata, which would make it usable by the did:ion provider too.

@lleifermann @nklomp what do you think?

[strumswell]

@mirceanis I started implementing this proposal (see here) and would love to hear your thoughts on it.

What I was wondering:

1. Am missing something?
2. The key table has a identifierDid column that seems to be a 1:1 relationship to an identifier. But in our case a key now has potentially multiple identifiers it is being used at. Do we need to change the db schema here or did I miss the meaning behind the column?

I still have to write some tests to test the whole flow + if the new key is being used afterwards for future DID operations. I have not checked yet if further changes are needed for this to happen. Oh, and I probably have to remove the DID from old keys identifierDid column. Just wanted to share a WIP with some questions.

[mirceanis]

Based on the discussion above, you don't seem to be missing anything.

However, it's not a solution that I really like because it still imposes a set of did:ethr assumptions on other DID method implementations, requiring every other provider to implement a method that throws an error.
This is of course debatable, and I don't really know how most other DID methods out there represent their control logic.
IMO, since this is very did:ethr specific, I'd add this kind of functionality as an independent veramo plugin that only deals with did:ethr identifiers.

The way this would look like is that instead of veramo.didManagerUpdateControllerKey(...), users would call another method like veramo.ethrChangeControllerKey(...) that is provided by this new plugin.

The reason why I think this is a better solution is that some other DID methods don't use the same controller/owner concept, or control is not provided by a key, or they have multiple levels of control, like an update controller and a recovery controller, etc...
I can't think of a way to create a generic interface for all these situations which makes me think that it shouldn't even be considered a generic situation.

---

Regarding point 2, I think eventually we'll be removing some of the relationships between entities currently defined by the data-store plugin.
This is because it creates this kind of friction that you're running into right now, but also because it can lead to undefined behavior or errors in case these relationships aren't managed exclusively through the entity system.

It's possible that using one database engine and deleting an identifier also deletes the associated keys but using another database engine it doesn't...

Anyway, the shorter answer for question 2 would be to instead use the metadata property of keys to specify which DID they "belong to".

[strumswell]

Thanks for your answer and I totally get your reasoning. I thought about different naming options for the method as well and wasn't happy with any of them tbh. Your suggested didManagerUpdateControllerKey was the best one.

IMO, since this is very did:ethr specific, I'd add this kind of functionality as an independent veramo plugin that only deals with did:ethr identifiers.

I am going to go with the separate did:ethr extension plugin. Just to validate how I change the db entities for a controller/ owner change, in my example implementation above, I change the identifier entity like this:

• remove current controllerKeyId key from keys array
• update controllerKeyId with id of new key
• add new key to keys array

Reading your thoughts on point 2, what changes should I do on the key table? Because you intend to drop the relationship, I would not touch the identifierDid field of the keys and merely add something to the metadata field, as you suggested. Something like { "controls": [ "did:ethr:..." ] }. Is the identifierDid field even being used somewhere? I might have to make a PR for ethr-did-provider nevertheless to add this new metadata attribute when calling createIdentifier. Thoughts?

[mirceanis]

It should be possible to re-import both identifiers and keys. This should make it easier to think about these operations as
get-modify-reimport instead of thinking about changes to the database or entities.
You correctly identified the steps needed for the bookkeeping, but there are a few more if you consider the whole process:

public part of the update

• generate or import the new controller key to the keyManager, to set or to automatically compute a new controller key ID
• compute the ethereumAddress for the new key
• sign and publish the transaction to update the owner to the new address

private bookkeeping part of the update

• after successfully publishing the transaction to update the owner
• remove current controllerKeyId key from the keys array
• set the controllerKeyId to the ID of the new key
• add new key to keys array
• re-import the identifier
• (optional) re-import the previous controller key (if it's still in use anywhere, or if you modify any metadata for it)

I believe that in this case the internal connection between the keys and DID would work without having to update key metadata.
In a future update, we will probably replace the controllerKeyId property of identifiers (which is a did:ethr specific concept) with a more flexible metadata property that would allow the DID manager to maintain a private list of controller or recovery keys or other more specific tokens needed for DID management.

[mirceanis]

Also, if this is a new plugin, it should not use the didManager prefix for methods, to avoid collisions and confusion.

[strumswell]

Most of the stuff you describe were already part of the implementation I linked above. But thanks for reiterating to make sure we are on the same page.

public part of the update

• generate or import the new controller key to the keyManager, to set or to automatically compute a new controller key ID

For me, this is out of scope as I expect the new key being created before and given into the method.

export interface IEthrChangeControllerKeyArgs {
  did: string,
  kid: string,
  options?: TransactionOptions
}

• compute the ethereumAddress for the new key

Check. Had this already here

• sign and publish the transaction to update the owner to the new address

Check. Had this already here

private bookkeeping part of the update

• after successfully publishing the transaction to update the owner
• remove current controllerKeyId key from the keys array

Done. See here

• set the controllerKeyId to the ID of the new key

Done. See here

• add new key to keys array

Done. See here

• re-import the identifier

Done. See here

• (optional) re-import the previous controller key (if it's still in use anywhere, or if you modify any metadata for it)

That's what I still have to do. I will probably just update the metadata as described in a prior message and set the identifierDid field to null.

Also, if this is a new plugin, it should not use the didManager prefix for methods, to avoid collisions and confusion.

Check! :)

I already started pressing this into a new plugin, thanks for the great plugin template repo btw.

[strumswell]

I am currently writing tests and now noticed the implications changeOwner has on the DID document. Changing the owner makes the resolver (correctly) resets all keys in verificationMethod, authentication, and assertionMethod. Afterwards, it adds the new EcdsaSecp256k1RecoveryMethod2020 referencing the new owners blockchainAccountId in the verificationMethod which is referenced in authentication and assertionMethod.

Now, obviously, the resolver can't retrieve the new owners public key hex from the new owners address or the DID string, as it correctly contains the public key hex from the previous owner to maintain a constant identifier. My question would be what implications a missing EcdsaSecp256k1VerificationKey2019 has and if VC/ VP issuance and verification work just fine with just the EcdsaSecp256k1RecoveryMethod2020 referenced everywhere. (I'm guessing it will be all right as did:ethr:ethAddress is also a valid form and if VC/VP signature verification works similar to how signature verification works on-chain)

PS:
Calling didManagerAddKey after changing the DIDs owner with the new owners key adds in the EcdsaSecp256k1VerificationKey2019 as #delegate-1 (kk, it's just wording I know). Now the contract has access to the public key hex. The authentication in the DID document is not touched though which would potentially mean changes to ethr-did and possibly did-provider-ethr to add support for adding an authentication key. We should have this behavior in mind for did:eth :)

This is how a DID document would look like after changing the owner and adding a key for reference:

{
  "@context": [
    "https://www.w3.org/ns/did/v1",
    "https://w3id.org/security/suites/secp256k1recovery-2020/v2"
  ],
  "id": "did:ethr:ganache:0x0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798",
  "verificationMethod": [
    {
      "id": "did:ethr:ganache:0x0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798#controller",
      "type": "EcdsaSecp256k1RecoveryMethod2020",
      "controller": "did:ethr:ganache:0x0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798",
      "blockchainAccountId": "eip155:1337:0x2B5AD5c4795c026514f8317c7a215E218DcCD6cF"
    },
    {
      "id": "did:ethr:ganache:0x0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798#delegate-1",
      "type": "EcdsaSecp256k1VerificationKey2019",
      "controller": "did:ethr:ganache:0x0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798",
      "publicKeyHex": "04c6047f9441ed7d6d3045406e95c07cd85c778e4b8cef3ca7abac09b95c709ee51ae168fea63dc339a3c58419466ceaeef7f632653266d0e1236431a950cfe52a"
    }
  ],
  "authentication": [
    "did:ethr:ganache:0x0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798#controller"
  ],
  "assertionMethod": [
    "did:ethr:ganache:0x0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798#controller",
    "did:ethr:ganache:0x0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798#delegate-1"
  ]
}

[mirceanis]

My question would be what implications a missing EcdsaSecp256k1VerificationKey2019 has and if VC/ VP issuance and verification work just fine with just the EcdsaSecp256k1RecoveryMethod2020 referenced everywhere.

Technically, the VerificationKey is more powerful than RecoveryMethod since you can compute the latter from the former.
I'm not aware of all the implementations out there, but wherever I've seen a RecoveyMethod used, it was also possible to do the verification with the VerificationKey.

PS: Calling didManagerAddKey after changing the DIDs owner with the new owners key adds in the EcdsaSecp256k1VerificationKey2019 as #delegate-1 (kk, it's just wording I know). Now the contract has access to the public key hex. The authentication in the DID document is not touched though which would potentially mean changes to ethr-did and possibly did-provider-ethr to add support for adding an authentication key. We should have this behavior in mind for did:eth :)

Agreed!
It's a known limitation of did:ethr, that a new key can only be linked to one verification relationship per transaction, so if you need assertion and authentication for the same key, you'd have to add it twice.
This can be fixed at the resolver level (see decentralized-identity/ethr-did-resolver#110) but we should make it possible from the start with the new did:eth.
The advice from specialists when it comes to keys is to never reuse them, so from this perspective the current did:ethr is doing things better by forcing you to add a key twice :)
An even more correct way to use it, according to this advice, would be to never use the owner key for assertions, but to always add assertion keys separately. This would break the free creation model, though, so I don't see it as practical.

BTW, if you ever need a credential to remain valid even after the issuer changes ownership, you can use query parameters to point to a specific version of the DID document (see decentralized-identity/ethr-did-resolver#119). I'm not advocating for this, just mentioning that it's possible ;)

[strumswell]

Just wanted to mention that I released a package for changeOwner and changeOwnerSigned yesterday. https://www.npmjs.com/package/@spherity/did-extension-ethr
https://github.com/spherity/did-extension-ethr

The removal of endpoints etc. I mentioned yesterday was a bad on my side. I just didn't notice that https://github.com/decentralized-identity/ethr-did-resolver/blob/516f5a06aaa2bf921b1da42e82ee4f4eda0fba2c/src/resolver.ts#L257 was in a separate else haha.

[mirceanis]

That's very cool!

I'd suggest trying to use some of the top-level plugins instead for some operations.
Your plugin depends on a DIDStore and is porting over the KMS ETH signer class.

You should be able to use the methods of the DIDManager plugin instead of the DIDStore, and the methods of the KeyManager plugin for the signing part, no?
Using the DIDStore directly forces your plugin to only be usable in setups with a typeorm based storage and is also susceptible to misconfiguration.

What do you think?

[strumswell]

Thanks so much for your feedback! I am aware of me copying over some things which is a tradeoff I made to get something out quickly that we can use.

Originally, I also wanted to use didManagerImport but ultimately couldn't do it as its args expect a MinimalImportableIdentifier with a keys array containing their privatKeyHex which I don't have access to in this context. Maybe I am misunderstanding you though.
A similar thing can be said about the KmsEthereumSigner as this class is not exported by did-provider-ethr. I need an instance of this class though to create an EthrDID instance. This would be a very simple PR probably by exporting the class in the did-provider-ethr's index.ts.

Edit: Or we export the getEthrDidController and createMetaSignature from the did-provider-ethr.

[mirceanis]

Originally, I also wanted to use didManagerImport but ultimately couldn't do it as its args expect a MinimalImportableIdentifier with a keys array containing their privatKeyHex which I don't have access to in this context. Maybe I am misunderstanding you though.

You weren't misunderstanding me. It was an error on my part for not realizing the requirements of the didManagerImport method. I think it would still work, even if you don't provide the private keys (and ignore some of the typescript complaints), but I haven't actually tested it. The potential side-effect that I foresee is that the keyManager ends up overwriting something and losing the private keys.

This kind of modification should be a bit more easy to do and I'm inclined more and more toward adding a didManagerUpdateMetadata method that would allow you to update the locally known links without having to re-import.
This way, any DID update could be done in 2 optional steps:

• a signature from one of the existing keys, resulting in a broadcast to whatever backing mechanism that DID method has.
• an update to the metadata, to remember that change locally as well.

The metadata update could be used for other properties too, thus not binding it necessarily to a particular DID method or blockchain. It would only handle the local bookkeeping, and would essentially act as a re-import, but with fewer potential side-effects.

A similar thing can be said about the KmsEthereumSigner as this class is not exported by did-provider-ethr. I need an instance of this class though to create an EthrDID instance. This would be a very simple PR probably by exporting the class in the did-provider-ethr's index.ts.

Edit: Or we export the getEthrDidController and createMetaSignature from the did-provider-ethr.

It's easiest to export KmsEthereumSigner.

Edit: See #1124