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

Securing Verifiable Credentials using JOSE and COSE 2022-09-15 #125

Closed
awoie opened this issue Sep 15, 2023 · 9 comments
Closed

Securing Verifiable Credentials using JOSE and COSE 2022-09-15 #125

awoie opened this issue Sep 15, 2023 · 9 comments
Assignees
Labels
REVIEW REQUESTED SR Self-review to be checked

Comments

@awoie
Copy link

awoie commented Sep 15, 2023

We have conducted a self-review of our spec Securing Verifiable Credentials using JOSE and COSE, and the results can be found at w3c/vc-jose-cose#93.

Please check our findings.

@awoie awoie added pending This issue needs to get a reviewer assigned to it REVIEW REQUESTED SR Self-review to be checked labels Sep 15, 2023
@kdenhartog
Copy link

kdenhartog commented Oct 26, 2023

VC JOSE/COSE spec

Notes for PING Reviewers

Noting that the terms First Party is not shared between the different VC data model specs. This isn't really an issue, but in general I think of the First party as the issuer, the holder as a UA, and the verifier as a third party so will refer to them within this context. This is the model that was used in the VC Data Model self review as well, but doesn't necessarily align with the specific definitions that are normally used within the browser and PING. It appears based on previous reviews that they're aligning these concepts more without delineating between 3P/1P and instead sticking with their roles and recognizing each role has a UA associated with it.

This spec is one of two classes of proof suites (the other is VC-Data Integrity reviewed here #120) that is used to produce a signature of the claims. The proofs can be used either to prove the assertions came from the issuer, or to assert the presentation by a holder to the verifier. Specifically with the VC-JOSE-COSE spec, this spec is standardizing the method of using JOSE and COSE related specs to produce signatures to assert claims about the subject in a VC.

Summary of Review

  1. The spec should point back to the vc-data-model privacy and security sections, as well as the JWT and CWT privacy and security considerations, and the SD-JWT security considerations sections. I see the privacy considerations does this and the sec considerations points back to RF7515/7519. This would avoid needing to repeat a lot of the same things across Data-integrity/VC-Data-Model/This spec.

  2. Editorial nit: It would be useful to split these considerations up with separate headers

  3. The specification should highlight the tradeoff discussed in: https://medium.com/@markus.sabadello/json-ld-vcs-are-not-just-json-4488d279be43 and how semantic security is not a guarantee and considered an accepted tradeoff here.

  4. The processing details of section 3.2 and 3.2.1 seem rather loose. Is it using COSE or is it using CBOR? It currently says it MAY use COSE, but if it's using just CBOR it's rather ambiguous how that signature should be produced and verified. I'd recommend sticking with COSE only here to narrow the focus of the spec and not allow unnecessary ambiguity.

  • I'm noting that the similar issue with JOSE definitions in section 3.1. It should be more explicit about the signature production and verification and reference these specs directly or otherwise specify what is happening if this is not the case.
  • As @jyasskin noted in Pre-CR review from Jeffrey Yasskin w3c/vc-data-model#1285 (comment) we really should be confident about the processing models here to have a high confidence of interoperability and ensure no security or privacy issues arise from improper validation of signatures.
  1. The spec should forbid the usage of algorithms which are "Prohibited" in https://www.iana.org/assignments/jose/jose.xhtml and "Deprecated" in https://www.iana.org/assignments/cose/cose.xhtml

  2. Editorial nit: does it make sense to redefine the terminology in this spec or reference back to it in other specs? Most of these are defined elsewhere anyways and look to exactly match. FWIW, I should have noted that in the DI review as well. cc @msporny

@npdoty npdoty removed the pending This issue needs to get a reviewer assigned to it label Nov 15, 2023
@jyasskin
Copy link
Member

jyasskin commented Dec 7, 2023

The group has started to address (4) in w3c/vc-jose-cose#190.

@jyasskin
Copy link
Member

jyasskin commented Dec 7, 2023

The biggest privacy risk that I noticed is that the spec doesn't rule out the possibility that a verifier will fetch some of the JWT fields while verifying the credential. Some of the VC use cases want to avoid telling an issuer that a particular subject has visited a particular verifier, and without seeing the algorithms, we can't evaluate whether those use cases are satisfied. It looks like other use cases don't require this, and may even require that fields be fetched dynamically, so I don't think it should be a requirement that the algorithms never fetch anything during verification, but they should be clear how callers can enforce the requirements of each type of use case.

@kdenhartog
Copy link

kdenhartog commented Dec 7, 2023

Sorry, I wasn't able to make the call today. I still need to read through the notes. One of the broader points that needs to be made though goes along with @jyasskin point is that currently this spec is non-declaritive about it's purpose to the point it creates privacy risks. For this reason, I do think we should be considering whether or not this spec is even ready for CR status in the first place if it's not possible to be certain that an implementation is using specifically JWTs or CWTs. Based on this current language specified in 3.2:

COSE [[RFC9052](https://www.w3.org/TR/vc-jose-cose/#bib-rfc9052)] is a common approach to encoding and securing 
information using CBOR [[RFC8949](https://www.w3.org/TR/vc-jose-cose/#bib-rfc8949)]. Verifiable credentials MAY be 
secured using COSE [[RFC9052](https://www.w3.org/TR/vc-jose-cose/#bib-rfc9052)] and SHOULD be identified through 
use of content types as outlined in this section.

This could be interpreted that a conformant implementation chooses to support CBOR, but not use RFC9052 as long as it uses the content types. This doesn't seem specific enough to me to produce a conformant implementation let alone one that we can reasonably deduce the privacy properties of it. This is largely what I was trying to point out in number 4, but I didn't do a good enough job articulating in my previous comment.

Hopefully others in the group were able to understand this based on what @pes10k relayed for me on the call, but if not I wanted to make sure it was clear this is what I meant by number 4.

@jyasskin
Copy link
Member

jyasskin commented Dec 8, 2023

I think that was fairly clear. I'm pretty sure the WG means that an issuer MUST secure each VC (from the core data model spec), and one of the ways it MAY secure them is COSE. If the issuer does that, it SHOULD identify that fact using content types.

+1 for saying in our wide review comments that the WG needs to write algorithms that actually specify this. We can also formally object through our individual AC reps if the WG doesn't do that by AC review time, but I'm hopeful that we won't have to.

@msporny
Copy link

msporny commented Dec 8, 2023

We can also formally object through our individual AC reps if the WG doesn't do that by AC review time, but I'm hopeful that we won't have to.

Comments below are with my VCWG Member hat on (I'm not an editor of this specification):

I expect that certain members of the VCWG will object to taking the vc-jose-cose specification to CR if it does not address all of the PING review comments. It was made clear to the group by W3M that the expectation is that we address ALL Horizontal Review comments to the satisfaction of the reviewing group before transitioning any specification to CR.

I don't expect that you will have to FO through your individual AC rep unless the VCWG disagrees with the review comments, and to date, the WG has agreed with and made changes based on all the prior PING reviews. I don't expect this specification to be different in that regard.

@msporny
Copy link

msporny commented Dec 8, 2023

Editorial nit: does it make sense to redefine the terminology in this spec or reference back to it in other specs? Most of these are defined elsewhere anyways and look to exactly match. FWIW, I should have noted that in the DI review as well. cc @msporny

Yes, it makes sense.

That said, the Editors of the vc-jose-cose spec have been adamant about copy-pasting content into their spec to make it as independent as possible from the other specifications. IOW, the vc-jose-cose spec should reference other specs, but it doesn't and instead copy-pastes content into their document, which is an anti-pattern.

I'll also note this conversation, where we're trying to figure out a better way of "sharing" these definitions across W3C specifications.

https://github.com/w3c/respec/issues/4522#issuecomment-1831981202

There is hope that we might be able to do something like this before any of the VC specs transition to the next stage:

https://github.com/w3c/respec/issues/4522#issuecomment-1842628036

@kdenhartog
Copy link

I think that was fairly clear. I'm pretty sure the WG means that an issuer MUST secure each VC (from the core data model spec), and one of the ways it MAY secure them is COSE. If the issuer does that, it SHOULD identify that fact using content types.

Ok, that's good to hear. Yeah I could definitely see someone going "Hey I don't want to use COSE here for X, Y, and Z reason, but CBOR looks nice. I'll just use this unspecified content type that I made up and isn't defined anywhere and call it conformant with the VC-JOSE-COSE spec". Based on the usage of MAY and SHOULD here, they'd technically still be conformant even though it's wildly divergent from what's defined and I wanted to make sure that was called out to spec authors. This one should be as simple as just making the spec language a bit stronger. One example would be:

"If an implementer wishes to use CBOR as a serialization format they MUST USE RFC9052 and MUST specify the content type as...".

My language might still face tradeoffs here, but it at least remains narrowly defined to make it easy for someone to implement later. I'll open an issue on the spec to make sure people are aware of this concern.

@pes10k
Copy link
Collaborator

pes10k commented Jan 22, 2024

This was discussed on PING call on Dec 7, 2023. Closing as discussed

https://github.com/w3c/ping/blob/main/summaries/PING-minutes-20231207.md

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
REVIEW REQUESTED SR Self-review to be checked
Projects
None yet
Development

No branches or pull requests

6 participants