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

Add extensibility to Credential Response #386

Closed
paulbastian opened this issue Sep 5, 2024 · 22 comments · Fixed by #391
Closed

Add extensibility to Credential Response #386

paulbastian opened this issue Sep 5, 2024 · 22 comments · Fixed by #391

Comments

@paulbastian
Copy link
Contributor

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

c2bo commented Sep 6, 2024

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

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

bc-pi commented Sep 10, 2024

+1 for Option 2

@babisRoutis
Copy link
Contributor

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

  • Always return an array
  • Each object in the array could have at least one of "credential" or "transaction_id" (mutually exclusive)

@paulbastian
Copy link
Contributor Author

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

c2bo commented Sep 11, 2024

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

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

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

c2bo commented Sep 11, 2024

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

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)

  • HATEOAS for pagination and links

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

@paulbastian
Copy link
Contributor Author

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

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

I would ask you to open a new issue please!

@Sakurann
Copy link
Collaborator

Sakurann commented Sep 12, 2024

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

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 :

  • The element of credentials to be an extensible JSON Object with a mandatory attribute credential and an optional attribute notification_id.
  • Removal of credential and notification_id top-level attributes from Credential Response
  • Change the aforementioned text of the Deferred Credential Response to point to credentials array.

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

Option 2 seems by far the best to me.

@awoie
Copy link
Contributor

awoie commented Sep 12, 2024

Option 2 seems to be most reasonable.

@deshmukhrajvardhan
Copy link
Contributor

+1 for option 2

@millenc
Copy link

millenc commented Sep 13, 2024

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

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

mickrau commented Sep 13, 2024

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

c2bo commented Sep 13, 2024

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

awoie commented Sep 17, 2024

This would also enable credential version IDs as proposed here: #278

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.