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

Misc from manifest 12 review #33

Closed

Conversation

oyvindronningstad
Copy link
Contributor

@oyvindronningstad oyvindronningstad commented May 25, 2021

Various fixes/improvements/proposals, mostly in the CDDL. Look at the individual commit messages for more info.

Signed-off-by: Øyvind Rønningstad <oyvind.ronningstad@nordicsemi.no>
To avoid confusion (I first thought it was a typo).

Signed-off-by: Øyvind Rønningstad <oyvind.ronningstad@nordicsemi.no>
The nil / bstr [...] line is ambiguous with the above line.
This change simplifies parsing (no backtracking).

Signed-off-by: Øyvind Rønningstad <oyvind.ronningstad@nordicsemi.no>
4.2.3.  Length-First Map Key Ordering

Signed-off-by: Øyvind Rønningstad <oyvind.ronningstad@nordicsemi.no>
The previous definition could give rise to unordered keys if some
severable members were provided verbatim and some via digest.
This also prevents duplicate keys (both member and digest).

Signed-off-by: Øyvind Rønningstad <oyvind.ronningstad@nordicsemi.no>
Place all severable members before all unseverable members.

Signed-off-by: Øyvind Rønningstad <oyvind.ronningstad@nordicsemi.no>
…on info

compression info is a bstr-wrapped map that contains the algorithm.
Note: Only the long example is fixed.

Fixes suit-wg#19

Signed-off-by: Øyvind Rønningstad <oyvind.ronningstad@nordicsemi.no>
Signed-off-by: Øyvind Rønningstad <oyvind.ronningstad@nordicsemi.no>
@oyvindronningstad oyvindronningstad changed the title Manifest12 Misc from manifest 12 review May 25, 2021
@@ -76,8 +76,8 @@ h'00112233445566778899aabbccddeeff0123456789abcdeffedcba9876543210'
h'0123456789abcdeffedcba987654321000112233445566778899aabbccddeeff'
]),
/ image-size / 14:76834,
/ compression-info / 19:bstr .cbor ({1:1 / "gzip"}) /,
Copy link

@cabo cabo May 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't parse that

Copy link
Contributor Author

@oyvindronningstad oyvindronningstad May 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure it's not quite correct. Maybe @bremoran can help?

Copy link
Collaborator

@bremoran bremoran Jun 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe there's a better way to express what this means. What you'd normally see here is:

/ compression-info / 19 : h'A10101' / {
  \ suit-compression-algorithm \ 1 : 1 \ gzip \
} /

This is quite similar to what you see in RFC8152:

/ protected / h'a10126' / {
  \ alg \ 1:-7 \ ECDSA 256 \
} / ,

However this appears so much in SUIT and the bstrs are so large that this quickly becomes unmanagable. I proposed this notation at IETF108, and there was support for simplifying the printing, however it looks like I had an action item to adopt CDDL's version of diagnostic notation for encoded CBOR items that I didn't follow up. I'll make a PR that adopts https://datatracker.ietf.org/doc/html/rfc8610#appendix-G.3 so that this is more readable.

Copy link
Collaborator

@bremoran bremoran Jun 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #34

@oyvindronningstad
Copy link
Contributor Author

@oyvindronningstad oyvindronningstad commented Jun 23, 2021

@bremoran @cabo opinions on the CDDL changes?

Copy link
Collaborator

@bremoran bremoran left a comment

Otherwise, looks pretty good.

suit-run = 12
suit-text = 13
suit-coswid = 14
suit-text = 10
Copy link
Collaborator

@bremoran bremoran Jun 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why this section was reordered.

Copy link
Contributor Author

@oyvindronningstad oyvindronningstad Aug 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bremoran Because text and coswid appear before validate, load, and run in the manifest (severable member appear before unseverable).

bremoran added a commit that referenced this issue Jul 7, 2021
Some of the text fixes required changes to CDDL as well, which were not included in the text PRs. Those changes are included in this commit.

In addition, many of the changes from #33 are included in this commit.

Also fixed one typo in spec.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants