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

fix: add public key fetcher to pd match #19

Merged
merged 15 commits into from
Mar 2, 2023
Merged

fix: add public key fetcher to pd match #19

merged 15 commits into from
Mar 2, 2023

Conversation

0xHansLee
Copy link
Contributor

If we give public key fetcher option to pd.Match(), there is no need to verify each credential additionally.

When you see pd.Match(), VCs are selected.
And in the process of selecting, each VC is parsed from bytes using verifiable.ParseCredential() with option, which is also used in our VerifyCredential().

Thus gave the public key fetcher option to pd.Match() and remove duplicated verification.

@0xHansLee 0xHansLee self-assigned this Feb 24, 2023
Comment on lines 48 to 63
var vms []did.VerificationMethod
for _, vm := range doc.VerificationMethod {
var verificationMethod did.VerificationMethod
verificationMethod = vm
if btcec.IsCompressedPubKey(vm.Value) {
pubKey, err := btcec.ParsePubKey(vm.Value, btcec.S256())
if err != nil {
return nil, fmt.Errorf("invalid secp256k1 public key: %w", err)
}

verificationMethod.Value = pubKey.SerializeUncompressed()
}
vms = append(vms, verificationMethod)
}

doc.VerificationMethod = vms
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we discussed in Slack, currently the public key of DID from Panacea is compressed version, but aries use uncompressed version of public key for proof verification.
Thus I added this converting logic.

@0xHansLee 0xHansLee marked this pull request as ready for review February 24, 2023 07:56
Copy link
Contributor

@gyuguen gyuguen left a comment

Choose a reason for hiding this comment

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

LGTM
I left a comment that is not important

go.mod Outdated Show resolved Hide resolved
pkg/vdr/panacea_vdr_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@youngjoon-lee youngjoon-lee left a comment

Choose a reason for hiding this comment

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

lgtm

pkg/vdr/panacea_vdr.go Outdated Show resolved Hide resolved
Comment on lines 63 to 75
var auths []did.Verification
for _, auth := range doc.Authentication {
if btcec.IsCompressedPubKey(auth.VerificationMethod.Value) {
pubKey, err := btcec.ParsePubKey(auth.VerificationMethod.Value, btcec.S256())
if err != nil {
return nil, fmt.Errorf("invalid secp256k1 public key of authentication: %w", err)
}
auth.VerificationMethod.Value = pubKey.SerializeUncompressed()
}
auths = append(auths, auth)
}

doc.Authentication = auths
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also added this code block as temporal workaround (test completed).
When authentication exists (like panacea DID doc), public key fetcher fetches the public key from authentication.
I figured out how it works while debugging, but I haven't found the right solution yet.
I'll look into it a bit more and if there's a better way, I'll share.

Document: &didtypes.DIDDocument{
Id: issuerDID,
Contexts: &didtypes.JSONStringOrStrings{"https://www.w3.org/ns/did/v1"},
Authentications: []didtypes.VerificationRelationship{didtypes.NewVerificationRelationship(fmt.Sprintf("%s#key1", issuerDID))},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This Authentications field is added to make it similar to that of panacea.

Copy link
Contributor

@youngjoon-lee youngjoon-lee left a comment

Choose a reason for hiding this comment

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

LGTM. If you feel it's annoying to iterate/convert all veriMethods such as authentications, you just can alternatively extend the pubkeyFetcher like 28ed1b1, which is an incomplete example. If you want to use this idea, i think you can make it better by mixing this with your code

@0xHansLee
Copy link
Contributor Author

LGTM. If you feel it's annoying to iterate/convert all veriMethods such as authentications, you just can alternatively extend the pubkeyFetcher like 28ed1b1, which is an incomplete example. If you want to use this idea, i think you can make it better by mixing this with your code

Thank you for your guides 🙏. I also considered to implement PublicKeyFetcher directly, but I wasn't sure how to cover all the VerificationRelation such as authentication, assertion, capability delegation, and so on (VDRKeyResolver fetches public key from several VerificationRelation). So I first implemented it in a simple way.

If I understand correctly, the usage of VerificationRelation depends on user (whether to use several verification relations), so can we just fetch the public key only from VerificationMethod?

@0xHansLee
Copy link
Contributor Author

I found alternative workaround.

When verification relations (such as authentication, assertion, etc.) exist in DID document, corresponding verification methods are found in VerificationMethod in ParseDocument (ref). Thus, the compressed public key is converted to uncompressed public key before ParseDocument.

@gyuguen
Copy link
Contributor

gyuguen commented Feb 28, 2023

I found alternative workaround.

When verification relations (such as authentication, assertion, etc.) exist in DID document, corresponding verification methods are found in VerificationMethod in ParseDocument (ref). Thus, the compressed public key is converted to uncompressed public key before ParseDocument.

Does that mean We just need to find it in the verificationMethod?

@0xHansLee
Copy link
Contributor Author

0xHansLee commented Feb 28, 2023

Does that mean We just need to find it in the verificationMethod?

Maybe.
I mean yes.

@gyuguen
Copy link
Contributor

gyuguen commented Feb 28, 2023

Does that mean We just need to find it in the verificationMethod?

Maybe. I mean yes.

If it is true, that would be good :)

Copy link
Contributor

@gyuguen gyuguen left a comment

Choose a reason for hiding this comment

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

Good

Copy link

@audtlr24 audtlr24 left a comment

Choose a reason for hiding this comment

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

LGTM!

@0xHansLee 0xHansLee merged commit e5c210b into main Mar 2, 2023
@0xHansLee 0xHansLee deleted the han/fix-pd-match branch March 2, 2023 08:21
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.

4 participants