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

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

Closed
WadeBarnes opened this issue Apr 25, 2023 · 28 comments · Fixed by #2223
Closed

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

WadeBarnes opened this issue Apr 25, 2023 · 28 comments · Fixed by #2223

Comments

@WadeBarnes
Copy link
Contributor

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; #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'
@WadeBarnes WadeBarnes changed the title Missing fields from connection request in aca-py version 0.8.1 Missing fields from connection record in aca-py version 0.8.1 Apr 25, 2023
@swcurran
Copy link
Contributor

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

@swcurran
Copy link
Contributor

Other questions:

  • What is the impact of this?
  • What is the urgency in investigating?
  • Why the change to public DIDs for invites?
  • Please provide information about where this is happening so we can investigate. What is the deployment/use case? That should be helpful in reproducing the issue.

Thanks!

@marcos-carretero
Copy link

@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

  • BC Wallet 1.0.8 (813) installed
  • Configured to IDIM Development environment (developer options)
  • Connected to SPAN or VPN
  • IDIM Development Agent - ACA-Py 0.8.1

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

  • Secure browser tab opens to an error page "The link is not valid"

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

@marcos-carretero
Copy link

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

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

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

cvarjao commented Apr 25, 2023

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

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

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

@swcurran, Correct, it was not reset.

@marcos-carretero
Copy link

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

@swcurran
Copy link
Contributor

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

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

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

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

marcos-carretero commented Apr 25, 2023

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

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

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

The ACAPY_REQUESTS_THROUGH_PUBLIC_DID flag was set due to our initial triage, and seeing the breaking change in #2034.

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

@esune
Copy link
Member

esune commented Apr 26, 2023

@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
// 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
// 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
Copy link

@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
Copy link
Member

esune commented Apr 27, 2023

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

PR #2099 was originally included in 0.8.0-rc0

@esune
Copy link
Member

esune commented May 1, 2023

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

swcurran commented May 1, 2023

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

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

esune commented May 2, 2023

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

swcurran commented May 5, 2023

Closed by #2223

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 a pull request may close this issue.

6 participants