hyperledger / aries-vcx

aries-vcx is set of crates to work with DIDs, DID Documents, DIDComm, Verifiable Credentials and Hyperledger Aries.
https://didcomm.org
Apache License 2.0
125 stars 83 forks source link

[Update] Update AATH backchannel to build image #1221

Closed gmulhearn-anonyome closed 5 months ago

gmulhearn-anonyome commented 5 months ago

The main change here is to add a new CI/CD job to build and publish our backchannel image. then this image can be consumed in the AATH repo, rather than us having to synchronise code between here and AATH repo. The CI/CD should do the following:

Note that i haven't done github workflows before, so please carefully check that what i've done is sane... i've mostly tried to match some of the existing patterns in the workflows main yaml.

Other Changes

Fixes for ACAPY compatibility

I've also made some changes to get a bit more interop with ACApy. particularly around the DIDExchange suite of interop tests.

new approach is tested upstream in the AATH: https://github.com/hyperledger/aries-agent-test-harness/pull/833

JamesKEbert commented 5 months ago

This looks really good IMO. Good work and thanks for working on it! One question--this looks to run every time anything gets pushed/merged into main vs deploying by Aries VCX version. It means the backchannel is really up to date to main, but it doesn't reflect the interop status of the latest release (most of the time)--in particular if we make breaking changes to main. May not be a problem (and I definitely like having any image vs having to duplicate code between the repos), but figured I'd bring it for discussion.

JamesKEbert commented 5 months ago

I poked Shel on Discord--if we wanted we could add a second image that only gets build on release. It's a little bit more work (and could be a separate item if we want), but could be a good idea potentially.

gmulhearn commented 5 months ago

@JamesKEbert hey, this one is ready for review. I believe the updated CI/CD should let us support the multi-version testing style that you suggested. Pls see my MR overview.

So i think the changes for this PR are complete, more changes needed in the upstream AATH PR to support the multi-version testing.

tbh, we won't know for sure that this is all working until we merge it in and start getting tagged versions of the image. If it fails for whatever reason, i will iterate on it quickly of course.

gmulhearn commented 5 months ago

Forgive me for making indirectly related changes, but when testing acapy, i found some issues with our did:peer implementations. My latest commit should address them: https://github.com/hyperledger/aries-vcx/pull/1221/commits/a3c19840b7f1ee831337bdfefe77ca449f1bd894

Will test in acapy now.

gmulhearn commented 5 months ago

ok with my latest changes, we get very close to successful RFC0023 tests with acapy. we fail on the final step when we try to decode their DIDDoc within the DIDExchange Request they sent us... we fail here because they are using legacy DIDDocs when we are expecting a modern diddoc structure.. i suspect this is related to acapy using unqualified DIDs in these requests. i'll see if there's a way to run the tests with acapy using qualified did:peers rather than unqualified.

for instance, acapy will send me back this request:

2024-06-13 18:19:14 [2024-06-13T08:19:14Z INFO  aries_vcx::protocols::did_exchange::state_machine::responder::response_sent] DidExchangeResponder<ResponseSent>::receive_request >> request: {"@id":"0a9f1378-334e-4a6b-a9e4-20267e20349a","label":"aca-py.Acme","goal_code":null,"goal":null,"did":"BwCkrx4Lftd4h7hffCZTAK","did_doc~attach":{"@id":"3cf28db1-2626-4173-aef1-ec6e3b059df3","mime-type":"application/json","data":{"jws":{"signature":"GAYa0Ix63EcXfMnbpyxg9INwWRYPBb_9qHm9ZK587XzY1QtnkUVpylyBnaGWC-AQWW-XyxnDmC9r498gnOvoCw","header":{"kid":"did:key:z6MkkQvSe7H5QCig3wehMnFCXaoGswZzvE9RGeZ89vCUnxgv"},"protected":"eyJhbGciOiAiRWREU0EiLCAiandrIjogeyJrdHkiOiAiT0tQIiwgImNydiI6ICJFZDI1NTE5IiwgIngiOiAiV0l6TGt3dV9YbUpqLWx5WGhVTGx0QjNEc2dLaURMaUwzbFRvVnlNdWxaOCIsICJraWQiOiAiZGlkOmtleTp6Nk1ra1F2U2U3SDVRQ2lnM3dlaE1uRkNYYW9Hc3daenZFOVJHZVo4OXZDVW54Z3YifX0"},"base64":"eyJAY29udGV4dCI6ICJodHRwczovL3czaWQub3JnL2RpZC92MSIsICJpZCI6ICJkaWQ6c292OkJ3Q2tyeDRMZnRkNGg3aGZmQ1pUQUsiLCAicHVibGljS2V5IjogW3siaWQiOiAiZGlkOnNvdjpCd0Nrcng0TGZ0ZDRoN2hmZkNaVEFLIzEiLCAidHlwZSI6ICJFZDI1NTE5VmVyaWZpY2F0aW9uS2V5MjAxOCIsICJjb250cm9sbGVyIjogImRpZDpzb3Y6QndDa3J4NExmdGQ0aDdoZmZDWlRBSyIsICJwdWJsaWNLZXlCYXNlNTgiOiAiNnhmUTNzMmU0ZkVDd1NvemdESE1nVkZINE5KOVdMdTRhZGVDS2VFVHNqdVkifV0sICJhdXRoZW50aWNhdGlvbiI6IFt7InR5cGUiOiAiRWQyNTUxOVNpZ25hdHVyZUF1dGhlbnRpY2F0aW9uMjAxOCIsICJwdWJsaWNLZXkiOiAiZGlkOnNvdjpCd0Nrcng0TGZ0ZDRoN2hmZkNaVEFLIzEifV0sICJzZXJ2aWNlIjogW3siaWQiOiAiZGlkOnNvdjpCd0Nrcng0TGZ0ZDRoN2hmZkNaVEFLO2luZHkiLCAidHlwZSI6ICJJbmR5QWdlbnQiLCAicHJpb3JpdHkiOiAwLCAicmVjaXBpZW50S2V5cyI6IFsiNnhmUTNzMmU0ZkVDd1NvemdESE1nVkZINE5KOVdMdTRhZGVDS2VFVHNqdVkiXSwgInNlcnZpY2VFbmRwb2ludCI6ICJodHRwOi8vaG9zdC5kb2NrZXIuaW50ZXJuYWw6OTAyMSJ9XX0="}},"~thread":{"thid":"0a9f1378-334e-4a6b-a9e4-20267e20349a","pthid":"06bb2430-debc-4da7-961e-165af91ca738"}}

the did is BwCkrx4Lftd4h7hffCZTAK and the inner DID Doc is an abnormal structure:

{"@context": "https://w3id.org/did/v1", "id": "did:sov:BwCkrx4Lftd4h7hffCZTAK", "publicKey": [{"id": "did:sov:BwCkrx4Lftd4h7hffCZTAK#1", "type": "Ed25519VerificationKey2018", "controller": "did:sov:BwCkrx4Lftd4h7hffCZTAK", "publicKeyBase58": "6xfQ3s2e4fECwSozgDHMgVFH4NJ9WLu4adeCKeETsjuY"}], "authentication": [{"type": "Ed25519SignatureAuthentication2018", "publicKey": "did:sov:BwCkrx4Lftd4h7hffCZTAK#1"}], "service": [{"id": "did:sov:BwCkrx4Lftd4h7hffCZTAK;indy", "type": "IndyAgent", "priority": 0, "recipientKeys": ["6xfQ3s2e4fECwSozgDHMgVFH4NJ9WLu4adeCKeETsjuY"], "serviceEndpoint": "http://host.docker.internal:9021"}]}

this matches our AriesDidDoc structure... i'll need to look into what it should be

JamesKEbert commented 5 months ago

The AATH CI pieces look good to me, even if we accidentally did something wrong with the CI I am A-okay with us doing followup fixes there :+1:

JamesKEbert commented 5 months ago

As for the DID Exchange issues w/ACA-Py, the DIDDoc appears to be eventually resolved in /aries_vcx/src/protocols/did_exchange/state_machine/responder/response_sent/mod.rs receive_request(), which calls let their_ddo = resolve_ddo_from_request(&resolver_registry, &request).await?;, which attempts to map the attachment to a DIDDoc, but if that fails it will attempt to resolve it via the resolver registry (which the AATH agent has did:sov and did:peer registered, but since in the request the DID is unqualified the parser/resolver would fail I'd assume).

The DIDDoc format corresponds to RFC 0067, but that RFC hasn't been officially accepted/adopted, so I am unsure as to whether or not this is acceptable/expected/ideal behavior. From your POV, is this a sufficient DIDDoc inside of Aries VCX?

gmulhearn commented 5 months ago

@JamesKEbert thanks for taking a look. yea i think we are doing what the DIDExchange spec defines. Essentially:

  1. if the request contains a did_doc attachment, then decode that as the DIDDocument. If that decode fails then fail early (.transpose()?)
  2. else if there was no did_doc, then try use the resolver to resolve the DID.

The RFC from my reading says that implementors should either:

  1. have an unqualified DID and a did_doc attachment set in their request, or
  2. have a qualified (resolvable) DID and do not attach a did_doc attachment

So in the case of doing pure did:peer:4 exchanges with ACApy, we should be good, as we will hit case 2 and be able to properly resolve the DID; i've also looked at acapy code to confirm this. However when acapy is using an unqualified DID and goes with case 1, then they will attach the weird old/legacy/aries version of a DIDDocument, which does not match the real spec: https://www.w3.org/TR/did-core/#did-document-properties

I had a look at how acapy handles this situation (i.e. acapy handling receiving and processing a DIDExchange request in case 2). The following happens:

  1. The request is funnelled into acapy's _extract_and_record_did_doc_info function.
  2. This function extracts the DIDDoc as a raw JSON/dict (no conversion yet) and stores it in the wallet against the unqualified DID (all in raw JSON/dict)
  3. Eventually when acapy needs to retrieve the doc, i believe it is retrieved via the LegacyPeerDIDResolver implementation, which fetches the doc from the wallet.
  4. After fetching the raw dict/JSON doc from the wallet, it will pass it thru a LegacyDocCorrections implementation to convert the weird/legacy/aries DIDDoc into a real DIDDoc.

long story short; acapy will proactively handle both by converting the legacy format into the correct format.

So i suppose aries-vcx should consider whether it should handle this legacy doc and try convert it? i think it probably should... ~I'll have a go at implementing this now. if it's too much, then i'll create another ticket~ I created another ticket for dealing with this: https://github.com/hyperledger/aries-vcx/issues/1227

gmulhearn commented 5 months ago

Supporting DIDExchange v1.1 will also improve our interop with acapy: https://github.com/hyperledger/aries-vcx/issues/1228