Method for referencing stored verifiable credentials

brownoxford commented 1 year ago

In order to be able to implement /credentials/status, a client needs to know what value to use in the credentialId POST body, and an implementation needs to be able to look up the associated credential in its internal storage.

Any id provided in the request body for a /credentials/issue request cannot be used by an implementation for security reasons - a bad actor could block use of ids by another client by flooding the system with requests using specially crafted id values. A careless actor could fail to use unique IDs

@brownoxford and @OR13 discussed offline and arrived at the following proposal:

proposal:

When a client requests a revocable credential, if they provide an id or a credentialStatus they should receive a 400 error.
When a revocable credential is issued the server should set the id and credentialStatus not the client.
When a revocable credential has its status updated, the credential id should match the credential.id and the URL for the credential as a resource under the organization. (this last part might not be true for you if you use URNs)

Confirm all of this with a postman unit test.

Note: Implementations are responsible for generating a unique credential id when issuing a revocable credential, and clients are responsible for storing said id if they want to be able to later modify the credential through a request to /credentials/status.

brownoxford commented 1 year ago

Alternative Proposal (storedCredential property): This proposal is put forth to cover both the revocable credential scenario outlined above as well as other future scenarios where it may make sense to store a credential for later reference.

Add a new top-level property to the responses from /credentials/issue that contains the ID used to store the credential. This property would only be present if a credential is stored by the issuing implementation, and would most likely be required if the credential being issued is revocable:

{
  "verifiableCredential": ...
  "storedCredential": {
    "id": "urn:uuid:07aa969e-b40d-4c1b-ab46-ded252003ded"
  }
}

Under this proposal, it would NOT be an error to include credential.id in the request to /credentials/issue for a revocable credential. It would, however, still be an error to include credential.credentialStatus - this would be a good candidate for a separate issue as it does not relate to methods for referencing stored credentials.

ping @OR13

mprorock commented 1 year ago

Add a new top-level property to the responses from /credentials/issue that contains the ID used to store the credential. This property would only be present if a credential is stored by the issuing implementation, and would most likely be required if the credential being issued is revocable:

This second approach seems like it could be handled separately and has a great degree of utility in many scenarios beyond revocation.

Proposal 1 seems like the best overall way of handling the revocation issue.

brownoxford commented 1 year ago

@mprorock I would definitely not want to implement both proposals, and favor the second one as it covers the revocation scenario as well as future use.

mprorock commented 1 year ago

@mprorock I would definitely not want to implement both proposals, and favor the second one as it covers the revocation scenario as well as future use.

that is helpful - @OR13 any opposition to adding a storedCredential property?

brownoxford commented 1 year ago

Note: I have also asked @msporny to weigh in on this, but he is traveling and may not get to it for a bit.

OR13 commented 1 year ago

Make sure to read this section before responding:

https://w3c-ccg.github.io/traceability-vocab/#extensions-to-standard

The object MUST have an id property, and its value MUST be a valid IRI (URI, URN).

brownoxford commented 1 year ago

proposal a third proposal arose on the call today that we can forbid the credential.id from being provided to a /credentials/issue request, and that the server would always populate this field.

msporny commented 1 year ago

We had discussed this issue around a month ago in the VC API Work Item call. The result of that discussion is here:

https://github.com/w3c-ccg/vc-api/issues/126#issuecomment-1299032842

The general idea is that when you issue a VC, you can specify an options.credentialId value in the call to /credentials/issue. The value can by any URI, as long as it is unique to the Issuer Service. It's not yet clear if the Issuer Service is allowed to set verifiableCredential.id if options.credentialId isn't set. I'd presume it could, if it was configured to do that, and return the value in the credentialId field as well as the verifiableCredential.id field.

So, this allows the caller to:

Set the options.credentialId, which will NOT set the verifiableCredential.id field (this is for VCs that shouldn't have verifiableCredential.id set, such as a single-use bearer VC) -- credentialId will be returned set to the value of options.credentialId.
Not set options.credentialId, but set the verifiableCredential.id field to some value -- credentialId will be returned set to the value of verifiableCredential.id.
Set the options.credentialId, and the verifiableCredential.id field to the same value. Setting them to different values results in an error.

You can then use that credentialId for subsequent calls to /credentials/status (via options, presumably). This works for VCs with verifiableCredential.id set, and for VCs without verifiableCredential.id set.

I believe this is where the VC API Work Item group got to... and it's fairly close to the storedCredential proposal above.

brownoxford commented 1 year ago

~~@msporny I believe it is dangerous to allow the caller to specify an ID that will be used by the issuer to store credentials for several reasons:~~

~~It is not possible for the caller to provide unique IDs in the issuer's domain~~
If a malicious caller were to gain insight into the IDs that another caller were using, the malicious caller would be able to initiate a denial of service attack by using those IDs before the target caller

This is no longer true after comments below

paulfdietrich commented 1 year ago

Just to echo what I said in the meeting. Passing the entire credential to the update method may help remove the need to include a unique identifier explicitly.

brownoxford commented 1 year ago

To summarize the call from 2022-11-19, there are now several proposals on the table

Proposal 1:

When POSTing to /credentials/issue, a client MUST receive a 400 error response if a value is provided for credential.id.
An issuer MUST generate a unique (conformant) value and provide it in the verifiableCredential.id property in the response.

@nissimsan and @brownoxford suggested that the issuer injecting a value for id means modifying the credential so that it is not the same thing that the requestor provided. @OR13 pointed out that we already do this with properties like proof, and that we are not changing any data that the requestor actually provided (because we forbid them providing id).

@nissimsan also pointed out that by forbidding the requestor from providing a value for credential.id, we are removing functionality. I believe that @mprorock also called out a use case where it might be required that a requestor be able to provide their own ID (if that is the case, please provide that use case in a comment here).

Proposal 2:

If a provider stores a credential when responding to a /credentials/issue request, the response will contain a new top-level JSON property called storedCredential.id

@brownoxford pointed out that this would allow requestors to provide their own credential.id in /credentials/issue requests, but complicates things in the event that they do not (since a VC MUST have a value for credential.id).

Proposal 3:

Use the entire VC as a seed to generate an identifier that can be used to refer to the credential.

@paulfdietrich proposed that we bypass the need for the requestor to store an ID to use when referring to the credential for later use at the update endpoint. Instead, we would require that the entire credential be passed in the request body for the credential being updated.

@brownoxford notes that this runs into the same complications as Proposal 2 with respect to handling cases where the user did not provide a credential.id property in the /credentials/issue request. (if we need to generate an ID there anyway, we may as well favor Proposal #1).

Note: this proposal has been added based on comments made below by @msporny and @acarnagey

Proposal 4:

This proposal is similar to what @msporny previously posted regarding the consensus in the vc-api work item call, with the notable exception that it does not include options.credentialId in the request or credentialId in the response (to /credentials/issue). In our case, we do not need to support scenarios such as single-use bearer VCs and do not need these additional properties.

A caller MAY provide a value for credential.id.
An issuer MUST NOT modify a non-empty credential.id value provided by a caller.
An issuer MAY return a 400 error if the provided credential.id value is not unique for the caller. (Uniqueness cannot be determined if the issuer is not storing credentials by id, which may be the case when issuing non-revocable credentials).
An issuer MUST generate a unique, conformant value for credential.id if none was provided.

@brownoxford notes that initially this idea was rejected because it was assumed that the back-end store in an issuer would be global and share a domain for credential IDs across all tenants. This is why there was concern for denial of service type attacks (by consuming IDs with malicious issue requests).

@msporny has pointed out credential IDs can be scoped to a specific tenant, either because each issuer is a separate per-tenant instance, or because each client has a known client_id that can be combined with the credential id by the issuer (both to store credentials in a key/value store, and to later retrieve them by the status update endpoint).

@BenjaminMoe points out that an issuer will need to do a "full table read for every issue" in order to ensure that a credential id provided by a client is unique.

@brownoxford - I think that checking for uniqueness would be a required action regardless, as even keys generated by the issuer would need to be unique. In our case, this check probably would happen as the issuer attempts to store the VC using the ID as a key - if the key already exists, an error would occur. In cases where an issuer wanted to do this up front via a lookup, I would expect that the data store would be optimized for this use case, i.e., it would contain an index for the id field.

Note: this proposal has been added based on comments made below by @David-Chadwick

Proposal 5:

This proposal seeks to differentiate between the (optional) Credential id provided in the request body, and the (required) Verifiable Credential id returned in the response body (for the /credentials/issue endpoint).

This is similar to Proposal 2, but the verifiable credential ID is always generated by the issuer, and is returned as verifiableCredential.proof.id instead of a new top-level property.

An issuer MUST generate a unique, conformant value for proof.id

Note: this proposal has been added based on comments made below by @dlongley

Proposal 6:

This proposal specifies that a client is always responsible for including a credential ID property in the request body to /credentials/issue.

A caller MUST provide a unique, conformant value for credential.id

A benefit of this proposal is that issuers can (or maybe MUST?) generate a 409 Conflict response if the provided ID already exists. This allows callers to check to see whether a credential was indeed issued following a network drop or other client error that prevented the client from capturing the issuer's response. This benefit holds less weight if the issuer is not storing credentials.

@David-Chadwick has presented a use case wherein a university may seek to re-issue a credential after some time using the same credential.id. David has put forth Proposal 5 above, which differentiates between the credential ID and the verifiable credential (proof) ID.

@dlongley has suggested that issuing multiple VCs using the same credential.id is incorrect behavior and that doing so suggests a possible bug in software used in the JFF Plugfest.

msporny commented 1 year ago

@msporny I believe it is dangerous to allow the caller to specify an ID that will be used by the issuer to store credentials for several reasons:

Let's analyze the concerns:

It is not possible for the caller to provide unique IDs in the issuer's domain

The Issuer Service doesn't have to grant the request if it's malformed (id isn't unique -- remember, they don't have to provide the entire URL, just an ID, though by calling the endpoint, they also know the entire URL). If the ID isn't unique, the Issuer Service rejects it. Really, if the Issuer Service doesn't like the ID for any reason, it can reject the request.

There are use cases where the Issuer Coordinator (the piece of software calling the Issuer Service) does have some knowledge about the ID domain -- and if it doesn't, it can ask the Issuer Service to generate an appropriate ID (based on some out of band coordination).

Also note that the VC API work item group has the concept of issuer instances, so specific instances for specific domains, reducing the chances that the IDs will overlap or conflict (there isn't a global namespace shared across all issuer instances). All that to say, there are ways for the caller (Issuer Coordinator) to understand the Issuer Service namespace... and if it doesn't (for whatever reason), but still tries to pick an ID (like a UUID) that's already taken, the Issuer Service can reject the call (because the ID already exists).

If a malicious caller were to gain insight into the IDs that another caller were using, the malicious caller would be able to initiate a denial of service attack by using those IDs before the target caller

How? The attacker would have to be authorized on that issuer instance, and if it is, it isn't an attacker, right? It sounds like you're assuming a global namespace and not using issuer instances that can be scoped via OAuth2 tokens?

To put it another way, the above doesn't seem like an attack unless you have a global namespace shared across tenants, and that's typically a bad idea and leads to the sorts of attacks you're identifying above.

BenjaminMoe commented 1 year ago

My vote goes for Proposal 1. The simplest solution is generally the best.

ipbyrne commented 1 year ago

I vote for Proposal 1 as well. The other options seem to add too much complexity that will likely come back to bite us further down the line.

acarnagey commented 1 year ago

I'm in favor of proposal 1 out of those 3 mentioned. I agree that it is the most straightforward option out of those 3. This doesn't seem to be one of the proposals but @msporny mentioned that if the requestor doesn't pass the id the issuer can assign it but if the requestor does pass the id they would like to use, the issuer can check for whether the id is unique (if it is not it can 400) and use that one instead, that seems reasonable and gives more flexibility to the requestor.

2022-12-01: I'm in favor of the 4th proposal it allows the caller to set the id or the issuer and does not add much complexity

BenjaminMoe commented 1 year ago

if the requestor does pass the id they would like to use, the issuer can check for whether the id is unique (if it is not it can 400) and use that one instead, that seems reasonable and gives more flexibility to the requestor.

Originally i thought about including this. But that means the issuer would need to do a full table read for every issue.

brownoxford commented 1 year ago

I've added Proposal 4 based on additional clarifications from @msporny and comments from @acarnagey. I'm leaning towards this proposal because:

It preserves the optionality of credential.id in requests to /credentials/issue
It follows our spec which requires VCs returned from /credentials/issue requests to have a value for id
It closely aligns with the direction that the vc-api group is taking

dlongley commented 1 year ago

@brownoxford,

I wanted to respond to a few of your comments here ... @msporny touched on these too, but I felt it important to weigh in:

Any id provided in the request body for a /credentials/issue request cannot be used by an implementation for security reasons - a bad actor could block use of ids by another client by flooding the system with requests using specially crafted id values. A careless actor could fail to use unique IDs

The client is the Issuer's agent and is using the issuer service to accomplish their goals. The client and the issuer service all operate within the "Issuer's trust boundary". So this problem would involve attacking one's self or a rogue client whose credentials can be revoked.

I think it's vital for the Issuer (via their various agents like clients that use the VC API issuer service) to be able to control and set their own credential IDs. There are flows where, in order for the Issuer to create one-time-use simple exchanges, they need to bind credential IDs to user data prior to issuance.

The issuer service should reject requests that use credential IDs that have been used before. It is also responsible for ensuring that issuer IDs are scoped (when checking for uniqueness) to individual issuer instances on the service, so it should not be possible for tenant-based systems to experience uniqueness failures that cut across trust boundaries.

Note that this does not mean -- and cannot mean -- that someone else in the world could not possibly choose the same credential ID for a VC (speaking about people using totally disconnected and unrelated services). Benign and unintentional conflicts can be resolved by using sufficient randomness in VC IDs (such as UUIDs / 128-bit random numbers) or by using identifiers over which control can asserted or even proven (domain-bound, etc.). Maliciously generated, duplicative IDs can always be created in new VCs by other parties who have seen such IDs before in their original VCs or could reasonably guess ahead of time what someone else might use for an ID. That's just a fact of the ecosystem -- and software (e.g., digital wallets) need to be ready to deal with it.

I believe it is dangerous to allow the caller to specify an ID that will be used by the issuer to store credentials for several reasons:

It is not possible for the caller to provide unique IDs in the issuer's domain

The "caller" is within the issuer's trust boundary. The caller is always issuing a VC on behalf of the issuer -- that's how the issuer service works. Think about all of the other more serious trouble a malicious caller could do.

The issuer service must ensure that, for internal indexing purposes, that IDs are scoped to the particular issuer instance / configuration being used on the service. This would be true whether or not the caller provides the ID, otherwise one instance could overwrite the IDs of another.

If a malicious caller were to gain insight into the IDs that another caller were using, the malicious caller would be able to initiate a denial of service attack by using those IDs before the target caller

The "malicious caller" must be using the same issuer instance on the same issuer service as the target caller. A malicious caller would have to be a party that has credentials to use that issuer instance -- and those credentials should be revoked because the caller is malicious rather than trusted.

For my position, I think it would be best if the responsibility for generating IDs was placed entirely on the client.

The client acting on behalf of the system that needs to know what to do with those IDs externally, how to map them to user data so they can perform status changes in the future (if needed) and so on. Having the issuer service generate the ID also doesn't help in cases where responses are lost (network connection / software failures). The client will not know whether a VC was issued or not -- whereas if they had chosen the ID they could request the stored VC (presuming VC storage via the issuer service) or revoke it (presuming only status storage via the issuer service) and try again.

So, I'm not aware of a case where the issuer service needs to generate these IDs (vs. the client), and I think there are potential problems with the approach. If the idea for having the issuer service auto-generate the IDs grew out of the concerns listed above, I hope those have been assuaged -- and a different reason should be offered for the feature. Having the option for the issuer service to generate the IDs might not be the best way to go.

dlongley commented 1 year ago

@BenjaminMoe,

Originally i thought about including this. But that means the issuer would need to do a full table read for every issue.

Indexing can make uniqueness checks very fast (no full table read).

brownoxford commented 1 year ago

@dlongley I think we are now in agreement on most of these points (see updated Proposal 4 above), thank you for the excellent additional context and comments.

dlongley commented 1 year ago

@brownoxford,

If there's a circumstance underwhich issuer services are going to auto-generate IDs as this suggests:

An issuer MUST generate a unique, conformant value for credential.id if none was provided.

I recommend that whether an ID is generated or an error is thrown is up to the configuration of the particular issuer instance configuration.

As mentioned above, I think having the issuer service generate IDs is problematic for partitioned systems (which client + issuer service are an example). This is because clients cannot tell whether an error that occurs when communicating with the issuer service resulted in the issuance of a VC or not. If the client is always responsible for creating the ID, then when a network error occurs, they can retry the issuance call with the same ID. If the call fails with a conflict error then they know the VC was issued and the response was dropped after that. If the call succeeds, they know the error occurred before the VC was issued.

David-Chadwick commented 1 year ago

@brownoxford. Sorry I am late to this discussion because I am not closely following the traceability work. However, I agree with your very first post. The point you make about the id being generated by the issuer is exactly the same point that I have been making to the VC WG in a number of related issues (e.g. #973, #982), namely, the current top level "id" field in the DM is the id of the credential and not of the verifiable credential, and that the id of the verifiable credential should be part of the proof property or the JWT, and should be inserted there by the issuer. So my proposal is

in the v2DM we make it clear that the current top level 'id' field is the id of the credential, and not of the verifiable credential. The id of the VC must be part of the proof object and created by the issuer.

And a related point that has been discussed as part of the JFF Plugfest, is that what the VC API calls the issuer is not really the VC issuer but is only the signing component of it. The issuer role is the entirety of the issuing functionality and the client is the wallet, not some trusted broker that is logically part of the issuer role and that calls the signing component (known as the issuer in the VC-API work).

TallTed commented 1 year ago

@David-Chadwick — I am not sure where I fall on this issue. I have a feeling that it will be clearer to me, and hopefully to both sides of the discussion in the MANY current issues and PRs that touch on it, such that we can resolve the spec IF you and/or others can produce an image that shows where all these properties fit into the various layers being discussed. Such a pictorial presentation will be MUCH easier to digest and comprehend than the walls of text that are currently in play.

David-Chadwick commented 1 year ago

@TallTed . Here you are. The credential has metadata that describes it and the entire structure (credential + its metadata) can be cryptographically protected and made verifiable by adding a proof to it.

 --------------------
|                    |
| Proof              |
|                    |
| ------------------ |
||                  ||
|| Credential       ||
|| Metadata         ||
||                  ||
|| --------------   ||
|||              |  ||
||| Credential   |  ||
|||              |  ||
|| --------------   ||
| ------------------ |
 --------------------

Example properties in each component

credential : subject id + various claims,
credential metadata: validity period, @context, issuer, issuee, id, type
proof: type, validity period, algorithm identifier, revocation info, signature, signer's public key

David-Chadwick commented 1 year ago

@nissimsan also pointed out that by forbidding the requestor from providing a value for credential.id, we are removing functionality. I believe that @mprorock also called out a use case where it might be required that a requestor be able to provide their own ID (if that is the case, please provide that use case in a comment here).

In JFF this was the case, that multiple VCs had the same credential.id. I suggest this could happen as follows. The issuer (a university) issues paper based degree certificates to graduates at the graduation ceremony, and each certificate has a unique certificate number that corresponds to the entry in the university's database. When the graduate subsequently applies for a VC online with her digital wallet, the VC contains the same certificate number in the credential.id property and a different id in the credential.proof.id property. Note that the VC is only valid for one year, and a year later the same graduate applies for a refresh of the VC. She now gets a new VC with a new credential.proof.id and new proof.issuanceDate and proof.expirationDate whereas the credential.issuanceDate is the date of graduation and remains fixed for all the VCs that will be issued from this degree certificate.

OR13 commented 1 year ago

It might be worth considering the unprotocted header case, that is so popular in COSE ecosystem, and in JOSE in the context of encryption.

TallTed commented 1 year ago

It seems odd to me that the Proof includes its metadata, rather than having another enveloping layer...

 -------------------------- 
|                          |
|        Proof             |
|        Metadata          |
|                          |
| ------------------------ |
| |                      | |
| |      Proof           | |
| |                      | |
| |  ------------------  | |
| | |                  | | |
| | |    Credential    | | |
| | |    Metadata      | | |
| | |                  | | |
| | |  --------------  | | |
| | | |              | | | |
| | | |  Credential  | | | |
| | | |              | | | |
| | |  --------------  | | |
| |  ------------------- | |
|  ----------------------- |
 --------------------------

David-Chadwick commented 1 year ago

I specifically did not specify the proof metadata like the above diagram as everything has to be within the proof for it to be signed and protected. If you have metadata outside the proof (like validity period) then how do you know it is correct? The only thing that should be sent out of band is the public key, unless that is included inside the proof and can be obtained from there. I guess you could include things like algorithm id outside the proof but that provides an attack vector. So anything in the unprotected header cannot be relied upon and can be a source of attack.

OR13 commented 1 year ago

In JOSE and COSE we call "proof metadata" a header or headers :)

On Tue, Dec 6, 2022, 1:26 PM David Chadwick @.***> wrote:

I specifically did not specify the proof metadata like the above diagram as everything has to be within the proof for it to be signed and protected. If you have metadata outside the proof (like validity period) then how do you know it is correct? The only thing that should be sent out of band is the public key, unless that is included inside the proof and can be obtained from there. I guess you could include things like algorithm id outside the proof but that provides an attack vector. So anything in the unprotected header cannot be relied upon and can be a source of attack.

— Reply to this email directly, view it on GitHub https://github.com/w3c-ccg/traceability-interop/issues/468#issuecomment-1339881924, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB7JLMFVCWUK5MAOVXRE7NLWL6HPDANCNFSM6AAAAAASBHQSUI . You are receiving this because you were mentioned.Message ID: @.***>

nissimsan commented 1 year ago

Discussing this on the trace call: Chris outlining this conversation.

Discussions are trending towards simplicity, meaning only one identifier in play. That corresponds to @brownoxford's suggestions 1 (issuing server always assigns) or 6 (requesting client) always assigns.

General agreement on the call to go with the latter option 6.

brownoxford commented 1 year ago

Discussed on call and agreed that Proposal 4 is the appropriate fallback here.

brownoxford commented 1 year ago

I believe this work is already completed - I will review and comment.

msporny commented 1 year ago

This issue was a fun roller coaster... proposal 1... no proposal 3!... no, proposal X with modifications, no, proposal 4 is the winner!!! :P

Thanks to everyone that put in the hard work to get it resolved! :)

nissimsan commented 1 year ago

Update the respec with option 4 to close this issue

mkhraisha commented 1 year ago

@brownoxford Can you confirm that proposal 4 is in place?

if so please feel free to close this.

nissimsan commented 1 year ago

Include examples with status link to interop.

OR13 commented 1 year ago

Needs to be aligned with v2 test suite: https://github.com/w3c/vc-jose-cose-test-suite/tree/main/testcases/secured-vc-with-status

nissimsan commented 11 months ago

@brownoxford , confirm that we're doing the right thing, so we can close.

nissimsan commented 11 months ago

Meeting decision: This issue has become stale now. We should just close it.

brownoxford commented 11 months ago

I've reviewed the current conformance tests for the four bullet points in proposal 4:

A caller MAY provide a value for credential.id. Conformance test is up to speed here and makes requests with and without a value for credential.id. OpenAPI specification is also up to speed.

An issuer MUST NOT modify a non-empty credential.id value provided by a caller. Conformance tests are in place to ensure that issued credentials do not modify credential.id if it has been provided in the request.

An issuer MAY return a 400 error if the provided credential.id value is not unique for the caller. This is not something that we can test for

An issuer MUST generate a unique, conformant value for credential.id if none was provided. Conformance tests do validate responses against the OpenAPI schema, but the current schema does not have credential.id as a required value in a 201 response from the credentials/issue endpoint.

w3c-ccg / traceability-interop

Method for referencing stored verifiable credentials #468