kilt-node icon indicating copy to clipboard operation
kilt-node copied to clipboard

refactor: split up DID creation #2

Open ntn-x2 opened this issue 3 years ago • 4 comments

This PR fixes https://github.com/KILTprotocol/ticket/issues/1884.

Nevertheless, this new approach requires, on the SDK, the creation of two different DID signatures for the creation operation now, in case details other than the authentication key are specified in a batch operation for the creation.

This is because, currently, the DID creation extrinsic requires a signed origin, but with a DidSignature as one of its parameters. Plus, if more information is to be added via a subsequent update operation, they would go through the submit_did_call extrinsic which would require a second additional DID signature over the encoded call or batch thereof.

I will open a different PR which will address this issue by allowing also the DID creation extrinsic to go through the submit_did_call, meaning that a batched creation + update operation would then need only one signature over the whole batch, which will have to be generated using the authentication key specified in the creation operation.

Anyway, this PR makes the creation operation much more lightweight, meaning that we can get rid of a lot of error checks in the creation, and delegate them to the extrinsics where those things are updated, e.g., when adding a service endpoint.

Do not merge

As this PR is kind of incomplete until the next one is also merged, I would suggest to review this PR, while I open a new one on top of this. Once both have been reviewed, it means we have a complete and valid solution to this issue, and we can merge both at the same time.

Checklist

  • [ ] Re-run benchmarks for pallets and runtimes

ntn-x2 avatar Apr 06 '22 07:04 ntn-x2

And I would also like to set the delegation&attestation key by default to the same key as the authorization key. IMHO security is not compromised by doing this. It is just an easy way to make things more convenient to use. I would think that the default use case is, that those keys are the same?

I am not entirely sure this is a good idea. We are creating unnecessary relationship between the attestation/delegation keys and the authentication key, which means that if any of the others is compromised, the authentication key is also compromised. Of course people can still use the same key, but it should at least be made explicit in the creation operation whether that is intended or not.

ntn-x2 avatar Apr 06 '22 09:04 ntn-x2

Just wanted to mention that the did creation extrinsic is still documented as

		/// The new keys added with this operation are stored under the DID
		/// identifier along with the block number in which the operation was
		/// executed.

but as far as I can see we don't setting any keys, right? The authenticationMethod is just the key derived from the identifier?

rflechtner avatar Aug 09 '22 12:08 rflechtner

@rflechtner yes, the idea of the creation would be to just initialise all the initial storage (i.e., marking the DID as existing), with no additional keys attached. The key is indeed derived from the account.

ntn-x2 avatar Aug 09 '22 12:08 ntn-x2

@rflechtner yes, the idea of the creation would be to just initialise all the initial storage (i.e., marking the DID as existing), with no additional keys attached. The key is indeed derived from the account.

I'd suggest removing that bit then.

rflechtner avatar Aug 09 '22 12:08 rflechtner

This will actually be solved with: https://github.com/KILTprotocol/kilt-node/pull/551

weichweich avatar Aug 31 '23 14:08 weichweich