openid / OpenID4VCI

68 stars 20 forks source link

Add extensibility to Credential Response #386

Closed paulbastian closed 1 month ago

paulbastian commented 2 months ago

It seems there could be benefit to have the ability to add auxilliary/associated/additional data/metadata to an issued credential. This could apply to a single credential but specifically also to each instance of Credential if multiple are issued using batch issuance with proofs and credentials.

This may be used for example key handles of ARKG or other advanced key derivation schemes, like described in #359 where each credential is using a public key that is derived by the issuer, so there is a need to pass a keyhandle for each credential instance. This could also be used for other features in the future, where there is data associated to a credential instance useful to the Wallet that should not be included into the credential itself.

I see some options:

Option 1:

Adding a credentials_metadata array in non-breaking way that includes all potential associated data, assuming same ordering as credentials array.

{
  "credentials": [
    "LUpixVCWJk0eOt4CXQe1NXK....WZwmhmn9OQp6YxX0a2L",
    "YXNkZnNhZGZkamZqZGFza23....29tZTIzMjMyMzIzMjMy"
  ],
  "credentials_metadata" :  [
    { 
      "keyhandle" : "..." //keyhandle for credential1,
      "some-other-metadata" : ".."
    },
    { 
      "keyhandle" : "..." //keyhandle for credential2,
      "some-other-metadata" : ".."
    }
  ]
}

Option 2:

Enhance credentials array in breaking way that includes all potential associated data.

{
"credentials": [
    { 
      "credential" : "LUpixVCWJk0eOt4CXQe1NXK....WZwmhmn9OQp6YxX0a2L",
      "keyhandle" : "..." //keyhandle,
      "some-other-metadata" : ".."
    },
    { 
      "credential" : "LUpixVCWJk0eOt4CXQe1NXK....WZwmhmn9OQp6YxX0a2L",
      "keyhandle" : "..." //keyhandle,
      "some-other-metadata" : ".."
    }
  ]
}

Option 3:

Each feature that uses/requires associated data introduces its own parameter array. This means though there are proof type specific parameters.

{
  "credentials": [
    "LUpixVCWJk0eOt4CXQe1NXK....WZwmhmn9OQp6YxX0a2L",
    "YXNkZnNhZGZkamZqZGFza23....29tZTIzMjMyMzIzMjMy"
  ],
  "keyhandles" :  [
    //keyhandle for credential1,
    //keyhandle for credential2
  ]
}
c2bo commented 2 months ago

I'd say Option 2 is the cleanest with the highest confidence that people will not accidentally mismatch credentials and other metadata - but it is a breaking change. I would expect that it will be a somewhat common case that some additional data associated with a credential needs to be passed to the wallet. Some Credential Formats might be doing that inside the format (e.g., unsigned header), but Option 2 looks like the cleanest way to provide this for every kind of format.

My vote would be to introduce this as a breaking change with 1.0 and in the same change also mandate that we always return an array (and remove the "credential" return parameter in favor of "credentials") since we are already introducing a breaking change. That would simplify the spec and prevent implementation mistakes.

There is also the open question of transaction_id batch handling (#58) which might use the same data structure returning the "credentials" as defined for credential endpoint here automatically supporting batch issuance and other metadata.

alenhorvat commented 2 months ago

What other use cases (except from key derivation) might appear?

If key derivation-related information is public, it might be encoded with the signature (in the protected or unprotected header).

Option 2 enables the most consistent processing of responses. Here I'd like to raise an additional question: should the /credential endpoint be paginated? Namely, it already contains all the elements required - you can fetch all VCs, you can fetch them by transaction id.

If pagination is something that might be considered in the future, option 2 is the most appropriate way to go. (e.g., if a wallet starts fetching the credentials and the connection is interrupted or the connectivity is poor, there should be a way to resume the operation).

bc-pi commented 2 months ago

+1 for Option 2

babisRoutis commented 2 months ago

:+1: for Option 2, for me but taking into account the points raised by @c2bo comment.

paulbastian commented 2 months ago
  • Each object in the array could have at least one of "credential" or "transaction_id" (mutually exclusive)

The Credential Request is only for one Credential Dataset, therefore I assume there is only one transaction_id for the whole Credential Response. This means, according to @c2bo proposal, Credential Response would return either credentials or transaction_id.

c2bo commented 2 months ago
  • Each object in the array could have at least one of "credential" or "transaction_id" (mutually exclusive)

The Credential Request is only for one Credential Dataset, therefore I assume there is only one transaction_id for the whole Credential Response. This means, according to @c2bo proposal, Credential Response would return either credentials or transaction_id.

Yes, Credential Response would contain credentials or transaction_id and if you get a response with HTTP Status Code 202 and transaction_id in the payload, you can get the "normal" credentials structure at the deferred endpoint.

That would make things quite a bit easier IMHO.

babisRoutis commented 2 months ago
  • Each object in the array could have at least one of "credential" or "transaction_id" (mutually exclusive)

The Credential Request is only for one Credential Dataset, therefore I assume there is only one transaction_id for the whole Credential Response. This means, according to @c2bo proposal, Credential Response would return either credentials or transaction_id.

Yes, Credential Response would contain credentials or transaction_id and if you get a response with HTTP Status Code 202 and transaction_id in the payload, you can get the "normal" credentials structure at the deferred endpoint.

That would make things quite a bit easier IMHO.

@c2bo & @paulbastian

I guess you are right. Sorry for creating such a confusion. It seems that I missed the change in the deferred endpoint where you can exchange the transaction_id to an credentials array.

babisRoutis commented 2 months ago

What about notification_id?

Currently, notification_id can be provided only if there is a credential

With option 2 and following the proposal to take the opportunity and remove credential, perhaps we should consider move notification_id inside each element of the credentials array.

For instance something like

{
"credentials": [
    { 
      "credential" : "LUpixVCWJk0eOt4CXQe1NXK....WZwmhmn9OQp6YxX0a2L",
      "notification_id" : "notId1",
      "keyhandle" : "..." ,
      "some-other-metadata" : ".."
    },
    { 
      "credential" : "LUpixVCWJk0eOt4CXQe1NXK....WZwmhmn9OQp6YxX0a2L",
     "notification_id" : "notId2",
      "keyhandle" : "..." ,
      "some-other-metadata" : ".."
    }
]
}

PS I assume that the granularity of notification_id for each credential is 0..1. Each credential can have zero or one notification_id

c2bo commented 2 months ago

Agreed @babisRoutis. It would IMHO make sense to also include notification_id in that object if we go with Option 2. notification_id also seems to not be fully specified for batch issuance right now?

This seems to become a pretty big change, but one that might help clean up quite a bit IMHO. Are people happy with a draft PR that makes it easier to understand the changes and discuss there?

alenhorvat commented 2 months ago

Is this an opportunity to make the endpoint REST+HATEOAS (pagination, links)?

"GET /credentials → collection" "GET /credentials/notification-id → resource" "notification-id" -> resource identifier (it would be meaningful to either rename the collection or resource identifier to make the naming consistent)

Especially for batch issuance can be useful, so that response size can be controlled by the client (wallet).

paulbastian commented 2 months ago

I'm not sure I understand the motivation and concept of pagination here and if it's really connected to this issue, do you mind to elaborate?

alenhorvat commented 2 months ago

@paulbastian, of course, I can try. If you check the proposed design of the response and discussion about "notification_id" the design essentially has almost all the elements of a REST API. Since the proposed changes are creating a breaking change, I see an opportunity to further improve the endpoint, so that the wallet can control the size of the response it can accept (via pagination/page size).

Is it connected to the issue? Partially, namely, the proposed changes in this issue triggered the idea for the proposed improvement. Is the improvement blocking the evolution of this issue? No, hence it can be addressed in a separate issue, but only if people express an interest.

paulbastian commented 2 months ago

I would ask you to open a new issue please!

Sakurann commented 2 months ago

Feels like there is an agreement forming around option 2. But there are more than one change being proposed/discussed and we should agree if they want all of them and if all of them should go into one PR:

  1. change the "credentials" to be array of objects (that can contain extra "metadata") for both credential and deferred endpoints [option 2 in this original issue]
  2. remove "credential" parameter for both credential and deferred endpoints?
  3. Clarify how "notification_id" works with batches of credentials?
  4. Not sure if there are any specific breaking changes being proposed for the deferred endpoint other than points 1 and 2
babisRoutis commented 2 months ago

Hi @Sakurann

Currently, the following is also problematic

The Deferred Credential Response uses the credential parameter as defined in Section 7.3.

It implies that transaction_id can be exchanged with a single credential.

So, IMHO, a possible PR should include :

These changes would make the semantics of Credential Response very clear: Issued, Deferred, Failed, regardless of single or batch request.

Finally, with regards to notification_id and batch issuance. To my understanding, currently, notification_id and related events are associated with an issued (instance) of a credential. I think that no changes are needed for batch issuance, since issuer could encode the fact that an instance is part of a batch (if he has to) to the notification_id. For example, batchXYZ#01 batchXYZ#02

David-Chadwick commented 2 months ago

Option 2 seems by far the best to me.

awoie commented 2 months ago

Option 2 seems to be most reasonable.

deshmukhrajvardhan commented 2 months ago

+1 for option 2

millenc commented 2 months ago

@alenhorvat

What other use cases (except from key derivation) might appear?

Another potential use case is allowing the issuer to tell the wallet what's the preferred disclosure policy for each issued credential, as described on:

https://github.com/openid/OpenID4VCI/issues/384

Support for embedded disclosure policies is mandated by the latest Implementing Act drafts. The issue linked above proposes several solutions to this requirement and option 2. aligns nicely with this proposal. Even though it's not an "embedded" policy per se (embedded in the credential payload itself, that is), the mechanism proposed in this issue would allow to support disclosure policies agnostic to each credential format.

+1 for option 2. for me too

mickrau commented 2 months ago

+1 for option 2.

@alenhorvat

What other use cases (except from key derivation) might appear?

Another potential use case is adding revocation-related information that the holder needs to retrieve a credential status assertion or to revoke the credential.

c2bo commented 2 months ago

What other use cases (except from key derivation) might appear?

Sorry I forgot to answer, but @mickrau already mentioned one point I had in mind. Not every credential format supports or will support unsigned data and I am pretty sure there will be requirements to attach data to a (specific) credential. One very concrete example would be revocation information (for example cryptographic accumulator based revocation approaches - if they ever get adopted / scale well enough).

awoie commented 2 months ago

This would also enable credential version IDs as proposed here: https://github.com/openid/OpenID4VCI/issues/278