Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ADR for W3C-DID Identity Proof #17

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

AdamJLemmon
Copy link

No description provided.

Copy link
Contributor

@yehjxraymond yehjxraymond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Adam, great work on the ADR! It is very well thought out and structured. I'm just uncomfortable in the use of tx.origin because of the controversy surrounding it. Wonder if you prefer to deal with that discussion in this PR or would you like this PR to be merged first before we append this document again after our discussion?

- This presents the opportunity to verify the issuer against the `ethereumAddress` associated with this `publicKey`.
From above: `ethereumAddress: 0xb9c5714089478a327f09197987f16f9e5d936e8a`
- In order to verify the issuer we need some proof that the issuer actually controls this address.
- If the `tx.origin` that sent the transaction to issue the document in question matches the `ethereumAddress` associated with the `publicKey` the issuer may be verified.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the idea of using tx.origin is quite controversial for multiple reason:

  1. tx.origin has been associated to multiple vulnerabilities and there are calls to deprecate the usage of tx.origin
  2. the use of tx.origin breaks the general composability of Ethereum contracts

These are explained in greater details here

I will suggest the use of the public key directly to prove the identity claim.

I'm not familiar of the topic but is there a way for non-EOA to control a DID or DID document to hold arbitrary paylod? Because if so, we can possibly have the DID document directly claiming the ownership of the document store without signatures (similar to DNS approach where the DNS record claims ownership of document store)

That, or we can figure a method for us to sign something with the private key so that the receiver of the document may verify the signature indeed came from the owner of the DID.

Copy link
Author

@AdamJLemmon AdamJLemmon Feb 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @yehjxraymond

Thanks very much for the review and feedback. The concerns regarding tx.origin as raised are generally in the context of Solidity and use within smart contracts, which I completely agree with.

However externally, traversing blocks etc from an off-chain perspective, I am not sure these concerns still hold? Also, if tx.origin were removed from Solidity that would not affect our implementation as by design all Ethereum transaction receipts (at least at present) require an origin (From) and we are not using Solidity to get this.

If we were to note the certificate store we would then need to prove ownership of it somehow, ie. the owner of the contract but that can be changed and not tightly bound to the specific certificate that is being verified, or by verifying the sender of some transaction related to that contract which brings us to tx.origin again.

I agree signing the certificate directly would alleviate this problem which is the separate ADR I am (slowly sorry) drafting, but would require some further work to integrate for issuers.

This felt the shortest path to integrate another Identity Proof :)

Please let me know your thoughts! Thanks!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding merging that is up to you! If you feel this a suitable v1 for example we can merge in and then continue this discussion for the next iteration. Up to you guys :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants