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

[Update] Update AATH backchannel to build image #1221

Merged
merged 22 commits into from
Jun 19, 2024
Merged

Conversation

gmulhearn-anonyome
Copy link
Contributor

@gmulhearn-anonyome gmulhearn-anonyome commented Jun 10, 2024

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:

  • re-release/replace an image tagged with main whenever we push to main branch. the main tagged image will therefore be the "latest and greatest"
  • publish an image tagged with some tag version whenever we create a tagged release. e.g. 0.64. these are our "stable release" tag images

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

  • remove unnecessary Cargo.lock in the aath-backchannel (unused)
  • fix the "version" returned from the AATH controller. this version string is what is used on the AATH reports. we will need to make sure to keep it in sync with the aries-vcx crate
  • update some aath deps
  • removed some old artifacts from the github CI (hashing dirs that don't exist, etc)
  • update the seed creation approach. the previous approach only worked in some environments, as some times you would end up with an ENDORSER issuer_did vs a USER issuer_did ROLE. a USER issuer_did cannot write schemas, causing some tests to fail. changes ensure that we end up with an ENDORSER issuer_did.
  • minor changes to the image. The previous image just straight up didn't work on newer aries-vcx code.

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.

  • fix did peer key format
  • fix did peer service key reference
  • fix DID Peer 3 & 4 handling
  • fix in-tolerant URL_SAFE decoding

new approach is tested upstream in the AATH: hyperledger/aries-agent-test-harness#833

Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
@gmulhearn-anonyome gmulhearn-anonyome changed the title [Update] Update AATH backchannel to build image #1220 [Update] Update AATH backchannel to build image Jun 10, 2024
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
@JamesKEbert
Copy link
Contributor

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
Copy link
Contributor

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.

Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
@gmulhearn gmulhearn marked this pull request as ready for review June 12, 2024 05:56
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
@gmulhearn
Copy link
Contributor

gmulhearn commented Jun 12, 2024

@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.

Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
@gmulhearn
Copy link
Contributor

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: a3c1984

Will test in acapy now.

Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
@gmulhearn
Copy link
Contributor

gmulhearn commented Jun 13, 2024

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
Copy link
Contributor

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 👍

@JamesKEbert
Copy link
Contributor

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
Copy link
Contributor

gmulhearn commented Jun 17, 2024

@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: #1227

Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
@gmulhearn
Copy link
Contributor

Supporting DIDExchange v1.1 will also improve our interop with acapy: #1228

Copy link
Contributor

@JamesKEbert JamesKEbert left a comment

Choose a reason for hiding this comment

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

LGTM! Good work on this!

@JamesKEbert JamesKEbert merged commit 912fc2d into main Jun 19, 2024
22 checks passed
@JamesKEbert JamesKEbert deleted the gm/aath-064-update branch June 19, 2024 21:13
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.

3 participants