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 did:ethr and EIP-712-based linked data proof type #99

Merged
merged 4 commits into from
Feb 24, 2021
Merged

Add did:ethr and EIP-712-based linked data proof type #99

merged 4 commits into from
Feb 24, 2021

Conversation

clehner
Copy link
Contributor

@clehner clehner commented Feb 24, 2021

Based on: #94

Implements did:ethr. There are some some differences compared to the did:ethr spec, due to updates to DID Core and using EcdsaSecp256k1RecoveryMethod2020, as mentioned in decentralized-identity/ethr-did-resolver#99; and to add a second verification method using a new experimental linked data proof type based on EIP-712.

The linked data proof type based on EIP-712 is named Eip712Method2021. The signing payload includes the URDNA2015-canonicalized linked data document (credential) and options (proof) rendered as an array of RDF statements where each statement (quad/triple) is an array of 3 or 4 strings.

Eip712Method2021 is not yet fully implemented here as the EIP-712 hashing is incomplete. So it can only be used via external signing, and does not yet have verification working.

@wyc
Copy link
Contributor

wyc commented Feb 24, 2021

Hi, is this ready for review? The CI seems to have failed--but also we have just merged in #94.

@clehner
Copy link
Contributor Author

clehner commented Feb 24, 2021

@wyc ready for review. I just pushed a fixup commit to try to fix the CI failure.

@clehner
Copy link
Contributor Author

clehner commented Feb 24, 2021

Rebased.

@wyc wyc changed the base branch from external-signing to main February 24, 2021 20:14
}

/// [`encodeData`](https://github.com/ethereum/EIPs/blob/master/EIPS/eip-712.md#definition-of-encodedata)
pub fn encode(&self, _type: &Struct, _types: &Types) -> Result<Vec<u8>, TypedDataHashError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm, will we need to complete this section for verification?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, unless using another EIP-712 implementation.

@@ -159,6 +176,12 @@ impl LinkedDataProofs {
x509_thumbprint_sha256: _,
} if ec_params.curve == Some("secp256k1".to_string()) => {
if algorithm.as_ref() == Some(&Algorithm::ES256KR) {
#[cfg(feature = "keccak-hash")]
if let Some(ref vm) = options.verification_method {
if vm.ends_with("#Eip712Method2021") {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the best way to document our VM support? Are we okay with treating it as EcdsaSecp256k1RecoverySignature2020 if we don't compile in keccak support?

Copy link
Contributor

Choose a reason for hiding this comment

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

Could this cause problems in some circumstances? I'm thinking that perhaps the feature flag should specify eip712 and require keccak-hash, but happy to merge these changes and discuss this in an issue.

Copy link
Contributor Author

@clehner clehner Feb 24, 2021

Choose a reason for hiding this comment

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

This should probably instead fail here (if VM ends in #Eip712Method2021 but the feature is not enabled), since passing a VM ending in #Eip712Method2021 is currently the only way for the caller to choose Eip712Signature2021. Ideally the proof type should be chosen by the verificationMethod option entirely instead of by the key type as this function currently does. During signing, we would then deference the verificationMethod option to a verification method object (map) in the DID document, and then select the corresponding proof type, or error if it is unsupported.

VM support could be documented in a readme, or in some markdown files listing supported functionality/standards, or in the docs repo, and/or in the Rust docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to failing when in doubt

@@ -196,6 +219,12 @@ impl LinkedDataProofs {
return EcdsaSecp256k1Signature2019::prepare(document, options, public_key).await
}
Algorithm::ES256KR => {
#[cfg(feature = "keccak-hash")]
if let Some(ref vm) = options.verification_method {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same note here.

Copy link
Contributor

@wyc wyc left a comment

Choose a reason for hiding this comment

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

Opened two issues to track some items we can revisit in the future. Changes look fine as-is to achieve basic did:ethr EIP-712 functionality enough for credential issuance from Ethereum txs.

#101
#102

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.

None yet

2 participants