Sort out OOB Handshake Protocols

loneil commented 4 months ago

OOB proofs are working to the BC Wallet provided we do not put anything in handshake_protocols

The call to ACA-Py to make the OOB includes the handshake_protocols in the response, but we were not including that in our model (by mistake!) so it does not end up in the QR Code/Deep Link payload. Fixing that and having it go through as [] causes an invalid QR code with an error in the wallet

Setting content in there, didcomm or connections causes the wallet to spin.

Some context here: https://github.com/bcgov/vc-authn-oidc/pull/582

I don't know if we are missing some other fields in our OOB invitation creation in order to be using handshake_protocols as a blank, or filled array (or which of those cases we'd want for sure) Or not sure if this is an issue on the BC Wallet side, or Credo or something... need some investigation then can add back in handshake_protocols to our model.

esune commented 4 months ago

cvarjao commented 4 months ago

The error message is raised from: https://github.com/search?q=repo%3Aopenwallet-foundation%2Fcredo-ts+%22Handshake+protocols%22&type=code

esune commented 4 months ago

The error message is raised from: https://github.com/search?q=repo%3Aopenwallet-foundation%2Fcredo-ts+%22Handshake+protocols%22&type=code

Thanks @cvarjao. Do you think we could expect an empty array to be handled/treated as if the option was not set (this is what ACA-Py seems to do), or do we need to gather consensus on what "optional" attribute means first? Trying to figure out where to route this issue next, since VC-AuthN is just a "consumer".

cvarjao commented 4 months ago

I do not understand the protocol at the level to provide any meaningful feedback, but sounds like credo/aca-py need to have some alignment on how it is implemented though.

wadeking98 commented 4 months ago

I'm running into this after my fix for the _url deeplink issue (https://github.com/bcgov/bc-wallet-mobile/issues/2053). I can fix the original error but after that I run into this error because the handshake_protocols is just an empty array in the message body

esune commented 4 months ago

I'm running into this after my fix for the _url deeplink issue (bcgov/bc-wallet-mobile#2053). I can fix the original error but after that I run into this error because the handshake_protocols is just an empty array in the message body

Please pull/use the latest vc-authn code, the change adding the handshake protocols was temporarily reverted until we sort out what needs to be done to fix it fully. This should hopefully unblock you for now.

genaris commented 4 months ago

The error message is raised from: https://github.com/search?q=repo%3Aopenwallet-foundation%2Fcredo-ts+%22Handshake+protocols%22&type=code

Thanks @cvarjao. Do you think we could expect an empty array to be handled/treated as if the option was not set (this is what ACA-Py seems to do), or do we need to gather consensus on what "optional" attribute means first? Trying to figure out where to route this issue next, since VC-AuthN is just a "consumer".

As per Aries RFC 0434 I don't see any meaningful case where handshake_protocols contains an empty array. I guess ACA-Py tries to not send anything because your flow is a connection-less one (i.e. no connection reuse is expected, as I see in #513).

Although this can be fixed in ACA-Py and also in Credo's invitation acceptance method, I think your suggestion about considering an empty array from ACA-Py response as None is correct (and maybe faster to implement).

In any case, I'll create a PR in Credo to handle this situation properly.

swcurran commented 4 months ago

So the protocol problem is that in the RFC, handshake_protocols is marked as optional, and ACA-Py is always including it, but leaving the array as empty for connectionless, while Credo is assuming that optional means it will not be included at all if it is not relevant. The fix that is being done in the code is that either is valid.

The clarification in the protocol should be that by optional, either is valid and mean the same thing -- not including handshake_protocols or including it as an empty array [] when there are no handshake_protocols needed.

Not ideal -- the protocol should have been clear from the start in saying that "optional" means don't include it when it is not needed. I think that was intended, but obviously others interpreted it differently.

loneil commented 4 months ago

Thanks @genaris !

For this, unless anyone has a better idea I think we can leave this issue open, once there ends up being a BC Wallet release that integrates the Credo change reimplement the fix in VCAuth-N that will allow handshake protocols back into the OOB message.

(other option is config that toggles it if someone needs the handshake protocols and is using this codebase in the meantime)

swcurran commented 4 months ago

I got some help from ChatGPT on clarifying the protocol — including what a sender SHOULD do and what a recipient MUST do. What do you think?

Certainly! Here is the protocol description in Markdown:

Messages

The out-of-band protocol consists of a single message that is sent by the sender.

Invitation: `https://didcomm.org/out-of-band/%VER/invitation`

{
  "@type": "https://didcomm.org/out-of-band/%VER/invitation",
  "@id": "<id used for context as pthid>",
  "label": "Faber College",
  "goal_code": "issue-vc",
  "goal": "To issue a Faber College Graduate credential",
  "accept": [
    "didcomm/aip2;env=rfc587",
    "didcomm/aip2;env=rfc19"
  ],
  "handshake_protocols": [
    "https://didcomm.org/didexchange/1.0",
    "https://didcomm.org/connections/1.0"
  ],
  "requests~attach": [
    {
      "@id": "request-0",
      "mime-type": "application/json",
      "data": {
        "json": "<json of protocol message>"
      }
    }
  ],
  "services": ["did:sov:LjgpST2rjsoxYegQDRm7EL"]
}

Items in the Message

@type: The DIDComm message type.
@id: The unique ID of the message. The ID should be used as the parent thread ID (pthid) for the response message, rather than the more common thread ID (thid) of the response message. This enables multiple uses of a single out-of-band message.
label: [optional] A self-attested string that the receiver may want to display to the user, likely about who sent the out-of-band message.
goal_code: [optional] A self-attested code that the receiver may want to display to the user or use in automatically deciding what to do with the out-of-band message.
goal: [optional] A self-attested string that the receiver may want to display to the user about the context-specific goal of the out-of-band message.
accept: [optional] An array of media (mime) types in the order of preference of the sender that the receiver can use in responding to the message. If accept is not specified, the receiver uses its preferred choice to respond to the message. RFC 0044 provides a general discussion of media types.
handshake_protocols: [optional] An array of protocols in the order of preference of the sender that the receiver can use in responding to the message in order to create or reuse a connection with the sender. These are not arbitrary protocols but rather protocols that result in the establishment of a connection. One or both of handshake_protocols and requests~attach MUST be included in the message.
requests~attach: [optional] An attachment decorator containing an array of request messages in order of preference that the receiver can use in responding to the message. One or both of handshake_protocols and requests~attach MUST be included in the message.
- While the JSON form of the attachment is used in the example above, the sender could choose to use the base64 form.
services: An array of union types that the receiver uses when responding to the message. Each item is either a DIDComm service object (as per RFC0067) or a DID (as per Decentralized Identifiers v1.0). Additional details below.

Clarification on Optional Fields

Important: If an optional field is not needed, it should be omitted from the message entirely. Including an optional field with a value set to null or an empty value is not compliant with the protocol. The absence of the field is the correct way to indicate that it is not needed.

Examples of Correct Usage

Including an Optional Field:

{
"@type": "https://didcomm.org/out-of-band/%VER/invitation",
"@id": "<id used for context as pthid>",
"label": "Faber College",
"goal_code": "issue-vc",
"goal": "To issue a Faber College Graduate credential",
"services": ["did:sov:LjgpST2rjsoxYegQDRm7EL"]
}

Omitting an Optional Field:

{
"@type": "https://didcomm.org/out-of-band/%VER/invitation",
"@id": "<id used for context as pthid>",
"services": ["did:sov:LjgpST2rjsoxYegQDRm7EL"]
}

Incorrect Usage (Setting Optional Fields to `null`)

Incorrect:

{
"@type": "https://didcomm.org/out-of-band/%VER/invitation",
"@id": "<id used for context as pthid>",
"label": null,
"goal_code": null,
"goal": null,
"services": ["did:sov:LjgpST2rjsoxYegQDRm7EL"]
}

Handling Messages with Null or Empty Values

While the sender should omit optional fields that are not needed, those receiving such messages should be prepared to accept null or empty values for these optional fields. Receivers should not consider the message invalid solely based on the presence of null or empty values in optional fields. They should instead treat these fields as if they were omitted entirely.

By following these guidelines, both senders and receivers can ensure they correctly handle optional fields, enhancing protocol consistency and interoperability.

genaris commented 4 months ago

Wow @swcurran , it seems that Chat GPT is learning fast :smile: . I think it gave some good definitions about what 'optional' means. Maybe Aries RFC 0003 and DIDComm Book or could be a good place for them.

genaris commented 4 months ago

FYI credo-ts v0.5.8 has been released. It includes the fix for this issue.

loneil commented 1 month ago

https://github.com/bcgov/vc-authn-oidc/pull/650

esune commented 1 month ago

Resolved by #650

bcgov / vc-authn-oidc