Add support for COSE for VCs. #593
Conversation
There was a problem hiding this comment.
I would like to see more tests with more key types.
Also, do you think you can try and have a go adding https://github.com/w3c/vc-jose-cose-test-suite/ to didkit-http?
|
|
||
| /// Public key. | ||
| #[non_exhaustive] | ||
| pub enum PublicKey { |
There was a problem hiding this comment.
Was it necessary to start refactoring ssi_crypto in the PR? Now we have multiple keys/signing/verification implementations, and depending on the credential format it will use one of the other
There was a problem hiding this comment.
I did not refactor it pre se, but I added support for crypto keys and crypto algorithms. Currently all of that is done exclusively by the ssi-jwk and ssi-jws libraries, I would have had to basically copy everything for COSE. Instead I decided to make a common foundation that can be used by both JOSE and COSE. For now of course only COSE uses it, but in the future I imagine the JOSE part using it as a foundation as well (same for verification methods) so we can avoid redundancies.
| pub trait CoseKeyResolver { | ||
| /// Fetches the COSE key associated to the give identifier. | ||
| #[allow(async_fn_in_trait)] | ||
| async fn fetch_public_cose_key( |
There was a problem hiding this comment.
Would it be better to "new-type" CoseKey, rather than use traits to add implementation? I'm particularly concerned about traits with async methods like this causing issues downstream due to the lack of Send on the future.
There was a problem hiding this comment.
Rust is able to tell if the future of an async function implementation like this implements Send or not. Its not like a dyn Future where you have to make the Send bound explicit.
The advantage of the trait is that you don't even have to new-type CoseKey. Since it implements CoseKeyResolver you can use it directly. It makes the interface with coset seamless.
There was a problem hiding this comment.
But it does add another layer of indirection which can be confusing. In this regard, it doesn't seem to be re-exporting coset at the moment?
There was a problem hiding this comment.
Yes coset is publicly re-exported in ssi-cose, along with a few important types defined in coset like CoseKey, CoseSign1, Header, etc.
ssi/crates/claims/crates/cose/src/lib.rs
Line 69 in 35413bf
| /// CBOR-encoded `COSE_Sign1` object. | ||
| /// | ||
| /// This is the borrowed equivalent of [`CompactCoseSign1Buf`]. | ||
| #[derive(Debug)] | ||
| #[repr(transparent)] | ||
| pub struct CompactCoseSign1([u8]); |
There was a problem hiding this comment.
Could you give a bit more description on what the difference is between this and coset::CoseSign1, i.e. what makes it "Compact"?
There was a problem hiding this comment.
Ah yes, I'll take some time to improve the documentation. It's basically a serialized CoseSign1. I tried to reflect the JOSE implementation. With JOSE they call the serialized form of a JWS, a "compact JWS" that's why I'm reusing this vocabulary, but I agree its unclear by itself.
There was a problem hiding this comment.
I'm a bit weary of us inventing our own terms, even if they're borrowed from other specs
There was a problem hiding this comment.
I'm open to finding a better name. Some ideas: SerializedCoseSign1, CoseSign1Bytes, CoseSign1Slice.
There was a problem hiding this comment.
I've opted for CoseSign1Bytes for now.
| /// `COSE_Sign1` object without the signature. | ||
| #[derive(Clone, PartialEq)] | ||
| pub struct UnsignedCoseSign1<T> { | ||
| /// Protected header. | ||
| pub protected: ProtectedHeader, | ||
|
|
||
| /// Unprotected header. | ||
| pub unprotected: Header, | ||
|
|
||
| /// Payload. | ||
| pub payload: PayloadBytes<T>, | ||
| } |
There was a problem hiding this comment.
Could you add support for detached payloads?
There was a problem hiding this comment.
I feel this should be provided by another type.
|
I've added more doc comments, a couple of examples, and more COSE-key encoding/decoding tests (which is the real addition from |
It turns out this test suite hadn't been updated in a while and doesn't use the same system as the other test suites. |
...and `CompactCoseSign1Buf` into `CoseSign1BytesBuf`.
|
Very happy to see this landed. Could you cut a new release with this? (I did my own version, but it was very hacky.) |
This PR adds basic support for COSE_Sign1 and COSE keys using
cosetandciboriumand implements the COSE part of the JOSE-COSE spec.Since I needed the
base64library I also upgraded it to the latest version.