openwallet-foundation / acapy

ACA-Py is a foundation for building decentralized identity applications and services running in non-mobile environments.
https://aca-py.org
Apache License 2.0
421 stars 515 forks source link

Missing fields from connection record in aca-py version 0.8.1 #2217

Closed WadeBarnes closed 1 year ago

WadeBarnes commented 1 year ago

These two messages are from BC Wallet 1.0.8 (813) on iOS against both two aca-py instances. These are the messages received by the controller.

aca-py 0.8.1

{
  "accept":"manual",
  "connection_id":"b06c8e7b-f0e8-4115-a137-b975f6b1fb8e",
  "connection_protocol":"connections/1.0",
  "created_at":"2023-04-25T00:36:02.981025Z",
  "invitation_key":"2eHpQFonMEhmzoaeEmALKWqdqyRWYfA6LQyjJJtgb2WT",
  "invitation_mode":"once",
  "my_did":"DiuXEWLWWq9nBca4vHsjWp",
  "rfc23_state":"request-received",
  "routing_state":"none",
  "state":"request",
  "their_role":"invitee",
  "updated_at":"2023-04-25T00:36:02.981025Z"
}

aca-py 0.7.4

{
  "accept":"manual",
  "connection_id":"905c4d2c-e4ff-4b64-a96b-4a63c365905c",
  "connection_protocol":"connections/1.0",
  "created_at":"2023-04-25T01:33:06.130393Z",
  "invitation_key":"ChJbCM5YJS1oxSAMV5MocRypMmQZtxQjpoJXFiLvg1C9",
  "invitation_mode":"once",
  "my_did":"DQmKuQfHvX8nE1ni5vFW2g",
  "rfc23_state":"completed",
  "routing_state":"none",
  "state":"active",
  "their_did":"Kymix7Fxh6oPyivbCVPuxy",
  "their_label":"BC Wallet",
  "their_role":"invitee",
  "updated_at":"2023-04-25T01:33:07.355253Z"
}

Fields missing from the aca-py 0.8.1 message are their_did and their_label.

Settings for the two instances are identical, except for the fact that the 0.8.1 instance also includes ACAPY_REQUESTS_THROUGH_PUBLIC_DID = true. Based on the notes for this change; https://github.com/hyperledger/aries-cloudagent-python/pull/2034

- name: ACAPY_INVITE_PUBLIC
  value: 'true'
- name: ACAPY_PUBLIC_INVITES
  value: 'true'
- name: ACAPY_AUTO_ACCEPT_INVITES
  value: 'false'
- name: ACAPY_AUTO_ACCEPT_REQUESTS
  value: 'false'
- name: ACAPY_AUTO_PING_CONNECTION
  value: 'true'
- name: ACAPY_MONITOR_PING
  value: 'false'
- name: ACAPY_AUTO_RESPOND_MESSAGES
  value: 'false'
- name: ACAPY_AUTO_RESPOND_CREDENTIAL_OFFER
  value: 'false'
- name: ACAPY_AUTO_RESPOND_CREDENTIAL_REQUEST
  value: 'false'
- name: ACAPY_AUTO_VERIFY_PRESENTATION
  value: 'true'
- name: ACAPY_AUTO_PROVISION
  value: 'true'
- name: ACAPY_NOTIFY_REVOCATION
  value: 'false'
- name: ACAPY_ENDORSER_ALIAS
  value: Endorser
- name: ACAPY_ENDORSER_ROLE
  value: author
- name: ACAPY_AUTO_REQUEST_ENDORSEMENT
  value: 'true'
- name: ACAPY_AUTO_WRITE_TRANSACTIONS
  value: 'true'
- name: ACAPY_CREATE_REVOCATION_TRANSACTIONS
  value: 'true'
- name: ACAPY_WALLET_TYPE
  value: askar
- name: ACAPY_WALLET_STORAGE_TYPE
  value: postgres_storage
- name: ACAPY_REQUESTS_THROUGH_PUBLIC_DID
  value: 'true'
swcurran commented 1 year ago

Is this an issuer/verifier ACA-Py instance or a Mediator?

swcurran commented 1 year ago

Other questions:

Thanks!

marcos-carretero commented 1 year ago

@swcurran , this is the BC Wallet connecting to the Person Credential issuer in the custom flow. The IDIM DID is embedded in the BC Wallet...

Pre-requisites A mobile device with

Steps to reproduce

  1. Open the BC Wallet
  2. Tap on the Credentials tab (lower right)
  3. Tap on the plus button on the top right
  4. Choose "Get your Person credential" option in the toaster menu
  5. Tap Get your Person credential on the Person Credential screen
  6. Tap Continue on the "BCWallet wants to use... to sign in" dialog

Expected behaviour Secure browser tab opens to the IDIM Issuer Credential Overview page

Actual behaviour

Notes: Investigation has shown that "their_did" is missing from the connection handshake.

marcos-carretero commented 1 year ago

@swcurran , some clarification... for each IDIM environment, a multi-use invitation was created, and provided to the BC Wallet team. Each invitation is embedded in the BC Wallet source code.

swcurran commented 1 year ago

Thanks — that helps.

@cvarjao — what protocol is BC Wallet using to establish the connection? Is it 0160 Connections or 0023 DID Exchange?

Just to confirm @marcos-carretero — the IDIM environment created an invitation? It did not just provide a public DID?

The setting that Wade talked about above (ACAPY_REQUESTS_THROUGH_PUBLIC_DID) suggest that it is just a DID that was shared, not an invitation. If it was an invitation, I don’t think that setting should be used.

In either case, the message from the Wallet to the IDIM issuer should be a request, and in that the label and the DID of the Wallet should be included and used by the IDIM Issuer, automagically in ACA-Py.

Thanks

marcos-carretero commented 1 year ago

Yes. the invitations were created manually. e.g. curl -X 'POST' \ 'https://<host>/connections/create-invitation? alias=wallet-idsit-public &auto_accept=false &multi_use=true &public=false' \

I'm wondering if the invitation needs to be regenerated since the upgrade to 0.8.1

cvarjao commented 1 year ago

@swcurran , I think it supports both, but I am not sure. I don't think we have enabled AIP 2 Interop tests for AFJ: https://allure.vonx.io/api/allure-docker-service/projects/javascript/reports/latest/index.html?redirect=false#behaviors

marcos-carretero commented 1 year ago

@swcurran , the impact is that this is delaying our upgrade from acapy 0.7.x to 0.8.1, which we understand is required for revocation to work correctly.

swcurran commented 1 year ago

Yes. the invitations were created manually. e.g. curl -X 'POST' \ 'https://<host>/connections/create-invitation? alias=wallet-idsit-public &auto_accept=false &multi_use=true &public=false' \

I'm wondering if the invitation needs to be regenerated since the upgrade to 0.8.1

Shouldn’t be needed unless the data for the IDIM agent was reset — I assume it was not.

WadeBarnes commented 1 year ago

@swcurran, Correct, it was not reset.

marcos-carretero commented 1 year ago

I have confirmed that with a new invitation, that IDIM Dev Acapy receives their_did in the request.

swcurran commented 1 year ago

That’s interesting. That seems very odd….

So the workaround is to get that new invitation into the BC Wallet.

But we still have to understand why the old invitation doesn’t work. Can we query the IDIM issuer to make sure that the old invitation is still there?

marcos-carretero commented 1 year ago

That’s interesting. That seems very odd….

So the workaround is to get that new invitation into the BC Wallet.

But we still have to understand why the old invitation doesn’t work. Can we query the IDIM issuer to make sure that the old invitation is still there?

@swcurran , I don't see a way to query invitations in the API docs. Is that something @WadeBarnes can do on the back end?

swcurran commented 1 year ago

Someone with access to the Swagger API for the instance might be able to query the connections to see if the old multi-use invitation still exists.

If you notice, in the ACA_Py 0.7.4 example, the connection state is active, but in the 0.8.1, the connection state is "request-received” — which suggests that the connection did not complete for some reason. That in turn might explain why there is no their_did— the connection establishment didn’t get that far. The problem may not be their_did, but rather than the connection failed for some other reason and thus their_did did not get created.

When you got it working with the new invitation, was the their_did present?

I’m also kind of interested in what the invitation_key is and if it should be the same in both cases with a multi-use invitation. But that is just me speculating…

swcurran commented 1 year ago

I’m not sure exactly what was different in the two examples in the first submission, but I tested out using a multi-use invite and if the same invite is used, the invitation_key should be the same. Why are they different in the examples above? If the invite key is built into the BC Wallet, shouldn’t they be the same?

marcos-carretero commented 1 year ago

@swcurran , the examples are from two different issuer environments (DEV, and SIT), but the same wallet instance. The BC Wallet has 3 invitations coded into https://github.com/bcgov/bc-wallet-mobile/blob/main/app/src/store.tsx.

I have access to the Swagger API console for all 3 environments, and believe that this is the corresponding invitation that is not working: { "accept": "manual", "alias": "wallet-iddev-public", "state": "invitation", "created_at": "2022-07-07T22:56:59.083910Z", "their_role": "invitee", "updated_at": "2023-04-14T20:13:03.384591Z", "connection_protocol": "connections/1.0", "invitation_mode": "multi", "routing_state": "none", "rfc23_state": "invitation-sent", "invitation_key": "2eHpQFonMEhmzoaeEmALKWqdqyRWYfA6LQyjJJtgb2WT", "connection_id": "78b521d6-0991-4c13-b9d4-d47d1e6289e3" }

marcos-carretero commented 1 year ago

Someone with access to the Swagger API for the instance might be able to query the connections to see if the old multi-use invitation still exists.

If you notice, in the ACA_Py 0.7.4 example, the connection state is active, but in the 0.8.1, the connection state is "request-received” — which suggests that the connection did not complete for some reason. That in turn might explain why there is no their_did— the connection establishment didn’t get that far. The problem may not be their_did, but rather than the connection failed for some other reason and thus their_did did not get created.

When you got it working with the new invitation, was the their_did present?

I’m also kind of interested in what the invitation_key is and if it should be the same in both cases with a multi-use invitation. But that is just me speculating…

Further, the reason the SIT connection is active and the DEV connection is not, is because the SIT connection has their_did, and the DEV one does not. IDIM only accepts a connection if the other agent provides a DID and has a unique connection ID

swcurran commented 1 year ago

@WadeBarnes — Why was the change made to set the ACAPY_REQUESTS_THROUGH_PUBLIC_DID = true? I think I have traced through the code and found a place where that might cause a problem (although I wouldn’t trust my code reading skills... :-) ). On the other hand, AFAIK, there is not a reason to set that flag to true for this use case. IDIM is not using a Public DID in the invite, so that should not be set to true.

That might not be the problem — but could you please restart the instance without that flag set and see if it fixes the issue?

marcos-carretero commented 1 year ago

The ACAPY_REQUESTS_THROUGH_PUBLIC_DID flag was set due to our initial triage, and seeing the breaking change in https://github.com/hyperledger/aries-cloudagent-python/pull/2034.

The issue existed prior to the change to that flag, but only since the upgrade.

esune commented 1 year ago

@marcos-carretero and myself just went through a round of troubleshooting. testing was done against the SIT agent to gather a baseline, and then we targeted the DEV agent to compare. The flag ACAPY_REQUESTS_THROUGH_PUBLIC_DID was set to false for the test, since we are not using public DIDs.

We identified a difference in the sequence of webhooks returned by ACA-Py to the controller: in particular the agent running on 0.7.4 returns an initial webhook with "state ": "invitation" that appears to be skipped by the agent running on 0.8.1. The initial webhook never expected the body to have their_did as attribute, however the unexpected combination of state/format is causing the controller to throw an error.

Webhook sequence for 0.7.4 ```json // 26 Apr 2023 14:17:50,500 [TRACE] https-openssl-nio-142.34.250.80-4102-exec-56 ca.bc.gov.idim.aries.api.AriesWebhooks - receiveMessage /connections: { "routing_state": "none", "created_at": "2023-04-26T21:17:50.280016Z", "invitation_key": "ChJbCM5YJS1oxSAMV5MocRypMmQZtxQjpoJXFiLvg1C9", "invitation_mode": "once", "connection_id": "a1fd3cad-e525-4036-a1ec-201cb4652767", "rfc23_state": "invitation-sent", "their_role": "invitee", "connection_protocol": "connections/1.0", "updated_at": "2023-04-26T21:17:50.280016Z", "my_did": "9gcezmb3XJGoXLHqKzNK9p", "state": "invitation", "accept": "manual" } // 26 Apr 2023 14:17:50,502 [TRACE] https-openssl-nio-142.34.250.81-4102-exec-9 ca.bc.gov.idim.aries.api.AriesWebhooks - receiveMessage /connections: { "routing_state": "none", "created_at": "2023-04-26T21:17:50.280016Z", "their_did": "5TH1fkUtoKysW7bdP8o4Pd", "invitation_key": "ChJbCM5YJS1oxSAMV5MocRypMmQZtxQjpoJXFiLvg1C9", "their_label": "BC Wallet", "invitation_mode": "once", "connection_id": "a1fd3cad-e525-4036-a1ec-201cb4652767", "rfc23_state": "request-received", "their_role": "invitee", "connection_protocol": "connections/1.0", "updated_at": "2023-04-26T21:17:50.396601Z", "my_did": "9gcezmb3XJGoXLHqKzNK9p", "state": "request", "accept": "manual" } // 26 Apr 2023 14:17:50,929 [TRACE] https-openssl-nio-142.34.250.81-4102-exec-12 ca.bc.gov.idim.aries.api.AriesWebhooks - receiveMessage /connections: { "routing_state": "none", "created_at": "2023-04-26T21:17:50.280016Z", "their_did": "5TH1fkUtoKysW7bdP8o4Pd", "invitation_key": "ChJbCM5YJS1oxSAMV5MocRypMmQZtxQjpoJXFiLvg1C9", "their_label": "BC Wallet", "invitation_mode": "once", "connection_id": "a1fd3cad-e525-4036-a1ec-201cb4652767", "rfc23_state": "response-sent", "their_role": "invitee", "connection_protocol": "connections/1.0", "updated_at": "2023-04-26T21:17:50.894927Z", "my_did": "9gcezmb3XJGoXLHqKzNK9p", "state": "response", "accept": "manual" } // 26 Apr 2023 14:17:51,375 [TRACE] https-openssl-nio-142.34.250.80-4102-exec-59 ca.bc.gov.idim.aries.api.AriesWebhooks - receiveMessage /connections: { "routing_state": "none", "created_at": "2023-04-26T21:17:50.280016Z", "their_did": "5TH1fkUtoKysW7bdP8o4Pd", "invitation_key": "ChJbCM5YJS1oxSAMV5MocRypMmQZtxQjpoJXFiLvg1C9", "their_label": "BC Wallet", "invitation_mode": "once", "connection_id": "a1fd3cad-e525-4036-a1ec-201cb4652767", "rfc23_state": "completed", "their_role": "invitee", "connection_protocol": "connections/1.0", "updated_at": "2023-04-26T21:17:51.332508Z", "my_did": "9gcezmb3XJGoXLHqKzNK9p", "state": "active", "accept": "manual" } ```
Webhook sequence for 0.8.1 ```json // 26 Apr 2023 14:24:03,121 [TRACE] https-openssl-nio-142.34.250.80-2102-exec-91 ca.bc.gov.idim.aries.api.AriesWebhooks - receiveMessage /connections: { "state": "request", "updated_at": "2023-04-26T21:24:02.977672Z", "connection_protocol": "connections/1.0", "created_at": "2023-04-26T21:24:02.977672Z", "my_did": "8LSJyVUy9yB9nWpj7Up62g", "accept": "manual", "invitation_key": "2eHpQFonMEhmzoaeEmALKWqdqyRWYfA6LQyjJJtgb2WT", "routing_state": "none", "rfc23_state": "request-received", "their_role": "invitee", "invitation_mode": "once", "connection_id": "7c0a270a-fb2f-48c2-8ed8-31789fad7e10" } ```

I am now trying to narrow down where/what is the root cause for the difference in behaviour, however the question of whether "state": "invitation" even makes sense for multi-use connection invitations as they can be used repeatedly by multiple parties without the inviter knowing ahead of time.

marcos-carretero commented 1 year ago

@esune , I've confirmed that we discard the "state": "invitation" webhook message on the controller, but for the "state":"request" we do expect their_did to be provided.

esune commented 1 year ago

It looks like #2099 changed the state of the first webhook fired when a connection invitation is responded to: this caused a webhook with "state": "request" to be fired twice (here and here where the state is being set), while previously webhooks with different states would have been fired.

The current state seems incorrect to me, as there is no easy way to follow the state-machine and determine which webhook is being received unless ALL of them have been received and kept track of.

We could revert that change, but that would cause the bug that the fix was intended for to show up again: I am going to look at how we can distinguish between the connection created for a multi-use invitation (should a connection even be created?) and the ACTUAL connection being instantiated at the time of response by the invitee.

WadeBarnes commented 1 year ago

PR #2099 was originally included in 0.8.0-rc0

esune commented 1 year ago

Tagging @reflectivedevelopment in the thread as he is the one who discovered the issue leading to the creation of #2099. It sounds like we might want to revert that change and add extra filtering to prevent that situation from occurring, if possible.

swcurran commented 1 year ago

@reflectivedevelopment — you can see the impact of the #2099 change in the web hook logging in this issue comment (expand the hidden text to see the logs). Basically, what used to be a web hook with state “invitation” now has the state “request”, does not include certain fields our controller is looking for (their_did their_role). The controller is “confused” (to use a technical term :-) ) and does not tell ACA-Py to proceed, and ultimately throws an error.

The easy fix on the controller side is to differentiate between the first and second web hook events (by detecting in the first that some fields are missing), treating the event in the same way as the previously handled “invitation” event — since it is the same event.

However, we were wondering if there should be an update to ACA-Py so as to accomplish the goal of #2099 without the side effect that has been created?

We’re not sure another way is needed, but given your knowledge of the problem, we wanted to show you the side effect created by #2099 and get your thoughts on if a “fix” should be in ACA-Py or in the controller?

Thanks

KimEbert42 commented 1 year ago

Perhaps the fix would not be to emit a new event on 488. I'm not sure it makes sense to emit an event here when accepting a multi use invitation. We aren't creating a new invitation, we are simply cloning the invitation for continue code re-use here.

So maybe something like this? Added the event=false ? I haven't tested the following change.


                new_connection = ConnRecord(
                    invitation_key=connection_key,
                    my_did=my_info.did,
                    state=ConnRecord.State.REQUEST.rfc160,
                    accept=connection.accept,
                    their_role=connection.their_role,
                    connection_protocol=CONN_PROTO,
                )
                async with self.profile.session() as session:
                    await new_connection.save(
                        session,
                        reason=(
                            "Received connection request from multi-use invitation DID"
                        ),
                        event=false,
esune commented 1 year ago

I think the proposal by @reflectivedevelopment makes sense, and results in a pretty clean solution overall - skimming quickly through the code I did not realize the event handler could be muted explicitly.

Will open a PR with the fix shortly.

swcurran commented 1 year ago

Closed by #2223