feat(rust/signed-doc): CBOR encoder using minicbor #351
Conversation
|
|
||
| anyhow = "1.0.95" | ||
| serde = { version = "1.0.217", features = ["derive"] } | ||
| serde_json = "1.0.134" | ||
| coset = "0.3.8" | ||
| minicbor = { version = "0.25.1", features = ["half"] } | ||
| coset = { version = "0.3.8", features = ["std"] } |
There was a problem hiding this comment.
But we have the decoding side still. Shouldn't it come form coset until that's merged and only then fully migrated to minicbor (having been tested)?
| @@ -66,7 +66,7 @@ impl Cli { | |||
| // Possibly encode if Metadata has an encoding set. | |||
| let payload = serde_json::to_vec(&json_doc)?; | |||
| // Start with no signatures. | |||
| let signed_doc = Builder::new() | |||
| let signed_doc = CoseSignBuilder::new() | |||
There was a problem hiding this comment.
I just am not seeing the need for a builder.
We should just be creating a CatalystSignedDocument type,
Adding whatever fields to it we like.
Adding a payload.
And then encoding it (it stores its encoded data internally).
Once encoded, we can call a function on the object to attach a signature, given a catalyst-id and a private key.
I think its a huge over-complication to build a generic COSE builder when we only need a way to serialize catalyst signed documents.
If we can encode the payload and metadata, this builder is literally just encoding:
[
headers,
payload,
[ array of signatures ]
]
which is just a single byte defining an array of 3 entries. Which does seem overkill for a general COSE builder in our use case.
There was a problem hiding this comment.
One thing that @Mr-Leshiy pointed out to me is that an unrestricted builder could be handy in tests.
In other words, a generic builder allows for making a valid COSE that isn't a valid Catalyst Signed Doc.
And being generic and abstracted it could simplify the process of adding slight deviations to the doc structure, and checking these to be rejected by CatalystSignedDocument::decode.
For example, as you mentioned, we want to keep field ordering deterministic. This can be checked trivially if there's some abstraction.
There was a problem hiding this comment.
It doesn't have to be a builder, it could as well be an extension trait, which would allow to encode a type as a protected header. Or at least something that abstracts enough, so that devs don't have to look up the RFC.
For example, this seems to me like a misuse of an encoder. And this would have to be replaced (so could as well be abstracted for both the signature protected_headers and document protected_headers.
The links point to the new PR, where abstractions over minicbor encoder aren't used.
| } | ||
|
|
||
| encoder.into_writer() | ||
| } |
There was a problem hiding this comment.
Given we have a fixed set of headers, and we will only be using those, seems overcomplex to maintain a general COSE header builder which already requires the keys and values to be encoded. its just complication for no gain.
There was a problem hiding this comment.
Yeah, I agree on this. This function exists, because @Mr-Leshiy and I used to want the generic builder, and since these were all immediately encoded and stored in a map, I had to make a concatenating function.
There was a problem hiding this comment.
Given the generic builder is deprecated, I'm sure this can be done way simpler and better.
|
Done under this PR #353 |
Description
Migrating
signed-docencoding fromcosettominicbor.Related Issue(s)
Closes #332
Description of Changes
ContentType,ContentEncodingandDocumentReftominicbor.Breaking Changes
serdesupport removed forsigned_doc.ciboriumandcosetsupport removed.Buildertype is refactored a lot.Related Pull Requests
#338
Please confirm the following checks