More robust verification method selection by did #3279
Conversation
Signed-off-by: aritroCoder <[email protected]>
Signed-off-by: aritroCoder <[email protected]>
added multikey support Co-authored-by: Patrick St-Louis <[email protected]> Signed-off-by: Aritra Bhaduri <[email protected]>
added multikey support Co-authored-by: Patrick St-Louis <[email protected]> Signed-off-by: Aritra Bhaduri <[email protected]>
added multikey support Co-authored-by: Patrick St-Louis <[email protected]> Signed-off-by: Aritra Bhaduri <[email protected]>
Signed-off-by: Daniel Bluhm <[email protected]>
Signed-off-by: Daniel Bluhm <[email protected]>
|
@PatStLouis please reivew (I couldn't formally request your review for some reason) |
|
Because of the changes to the selection strategy from 3138, it's possible I haven't fully accounted for the original issue yet. I will take a closer look. I suspect we just need the prover side of the exchange to use the correct proof purpose when determining vm id. Edit: I think I've address this now with the most recent commit. |
Signed-off-by: Daniel Bluhm <[email protected]>
|
@jamshale your input on BDD interop test failure would be appreciated. Is this an OWF migration artifact or something else? |
Ya. I changed a reference I shouldn't have. Should have a fix ready soon. |
| @@ -55,10 +88,50 @@ async def get_verification_method_id_for_did( | |||
| :params proof_purpose: the verkey relationship (assertionMethod, keyAgreement, ..) | |||
| :returns Optional[str]: the current verkey ID | |||
| """ | |||
| proof_type = proof_type or "Ed25519Signature2018" | |||
There was a problem hiding this comment.
I would like to suggest a slight nudge towards Ed25519Signature2020 as default option simply to hint at driving the community forward. Ed25519Signature2018 is a thing of the past.
| proof_type = proof_type or "Ed25519Signature2018" | |
| proof_type = proof_type or "Ed25519Signature2020" |
There was a problem hiding this comment.
I don't disagree that we should be heading towards bigger and better; this default was motivated by backwards compatibility. I don't know how much of an impact it would have to change the default so I'm reluctant to do so until that's well known.
There was a problem hiding this comment.
That's fair. This is not likely a feature I will be using so I'm not too opinionated about it.
|
@dbluhm thanks for doing this work. @aritroCoder does this cover your use case? |
|
|
I need to fix unit tests. Changes should be captured and tested at least in the json-ld scenario integration test but might be interesting/a good idea to have a more directed test of the functionality. |
|
Want to defer a merge until we get 1.1.0 release out. |
This PR is intended to supersede #3138 by adding a default verification method selection strategy for all DID methods.
This strategy relies on proof type and proof purpose to select the most appropriate verification method. For example, if proof type is
Ed25519Signature2020and purpose isassertionMethod, the changes in this PR will select the first matching method in theassertionMethodrelationship that can produce aEd25519Signature2020.This places more expectations on the DID -- specifically that it is well formatted (which many aren't). By well formatted I mean that verification methods used for issuance are properly referenced in
assertionMethod, methods used to authenticate the DID owner are inauthentication, etc. In practice, this may cause challenges but it is possible for users who depend on a DID method/document that isn't well formatted to create their own strategy and plug it in. So I'm comfortable making the assertion that we can expect the DIDs we're working with to be well formatted.This PR does adjust the interface for the VerificationKeyStrategy to better account for this. This might be painful to some plugin users who've added DID Methods by plugin and added a strategy to match. However, my hope is that those same users won't need to plug in their own strategy anymore with these changes since they should be robust enough to cover most use cases.
Shout out to @aritroCoder for his original work on #3138!