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

Tell implementers to use major type 2 encoding (not tag 64 -- Uint8Array) for CBOR #134

Closed
dlongley opened this issue Jan 17, 2024 · 4 comments
Assignees
Labels
pr-exists A Pull Request exists to address this issue.

Comments

@dlongley
Copy link
Contributor

The fact that JavaScript (on some platforms) uses Uint8Array to express byte strings / byte arrays should not escape into the proof serialization format. The proof data serializes bytes, it doesn't matter that JavaScript in a browser interacts with bytes using a Uint8Array interface. In other words, CBOR has a special tag that can be inserted to very specifically indicate that a Uint8Array is used -- when the simpler major type 2 expression (byte string) will do instead.

We should tell implementers to use major type 2 for simplicity (and not force implementers to work with Uint8Arrays, especially when working in other languages that express bytes in another way that wouldn't result in a special CBOR tag) but perhaps note that it does not affect the security / verifiability of the proofs as it's just a detail of the serialization format of the proof value.

See also: w3c/vc-di-ecdsa#52

@Wind4Greg
Copy link
Collaborator

@dlongley where do you think we should put such a note? In ECDSA-SD and BBS we use CBOR encoding for both base proof and derived proof. Do we need to say anything about CBOR decoding?

I think for the test vector section we need to say that the proofValue isn't unique since CBOR encodings are not unique and that one confirms their test vectors against the CBOR decoded values and not the proofValue string.

Also do you have some text? Otherwise I'll have to look into CBOR a bit more so I can say the above with the proper terminology, etc...

@dlongley
Copy link
Contributor Author

@dlongley where do you think we should put such a note? In ECDSA-SD and BBS we use CBOR encoding for both base proof and derived proof. Do we need to say anything about CBOR decoding?

Looking at the specs... in the vc-ecdsa spec we have an algorithms section with some normative requirements already:

https://w3c.github.io/vc-di-ecdsa/#algorithms

And we can mention this there, something like:

Any implementation of an algorithm in this specification that includes CBOR encoding MUST
encode byte strings using "Major Type 2" (link) and not "Tag 64" (link). The rationale for this
is that, while an implementations might use a `Uint8Array` to work with byte strings internally,
it is an internal implementation detail, not a part of the proof serialization format. This
requirement is to promote interoperability and is not related to security.

The vc-di-bbs spec similarly has an algorithms section where the same can be mentioned. There's already another normative "SHOULD" for something else there today:

https://w3c.github.io/vc-di-bbs/#algorithms

So we can just add the MUST about CBOR-encoding there.

If it's not too much trouble we can link back to this where we mention CBOR-encoding / decoding if we feel it's necessary with a "See this other section for further requirements on CBOR encoding".

I think for the test vector section we need to say that the proofValue isn't unique since CBOR encodings are not unique and that one confirms their test vectors against the CBOR decoded values and not the proofValue string.

+1

Also do you have some text? Otherwise I'll have to look into CBOR a bit more so I can say the above with the proper terminology, etc...

I don't have any text more specific than what I suggested above, so you might need to grab some other details to make it more robust.

@Wind4Greg Wind4Greg self-assigned this Jan 18, 2024
@Wind4Greg
Copy link
Collaborator

Thanks for the suggestions @dlongley. I'll put together a PR for this.

@Wind4Greg Wind4Greg added the pr-exists A Pull Request exists to address this issue. label Mar 1, 2024
@msporny
Copy link
Member

msporny commented Mar 7, 2024

PR #142 was raised and merged to address this issue, closing.

@msporny msporny closed this as completed Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-exists A Pull Request exists to address this issue.
Projects
None yet
Development

No branches or pull requests

3 participants