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

CDDL contradictions #262

Closed
dthaler opened this issue Oct 18, 2022 · 9 comments
Closed

CDDL contradictions #262

dthaler opened this issue Oct 18, 2022 · 9 comments
Assignees

Comments

@dthaler
Copy link
Collaborator

dthaler commented Oct 18, 2022

PR #248 introduced a regression regarding the manifest-list field of the Update message.

Section 4.4 has:
? manifest-list => [ + bstr .cbor SUIT_Envelope ],

Section 4.4.1 has:

    |     manifest-list: [                                    |
    |       += suit-manifest "tc-uuid.suit" (TC Developer) =+ |
    |       | SUIT_Envelope({                               | |

and same with 4.4.2, 4.4.3, and 4.4.4.

However, Appendix C incorrectly has:
? manifest-list => [ + bstr .cbor SUIT_Envelope_Tagged ],
instead of
? manifest-list => [ + bstr .cbor SUIT_Envelope ],

which regression came from commit f7115c0 in PR 248.

Per IETF discussion on #147 we do not need the CBOR tag.

@dthaler
Copy link
Collaborator Author

dthaler commented Oct 18, 2022

@kentakayama can you comment?

@dthaler dthaler mentioned this issue Oct 18, 2022
@dthaler dthaler changed the title CDDL contradiction CDDL contradictions Oct 18, 2022
@dthaler
Copy link
Collaborator Author

dthaler commented Oct 18, 2022

Another contradiction:

Section 4.6 has:
err-code: uint (0..23)

but Appendix C instead has:
err-code: 0..23

@dthaler
Copy link
Collaborator Author

dthaler commented Oct 18, 2022

Someone needs to go through all of PR #248 again and fix the contradictions.

@kentakayama
Copy link
Contributor

@dthaler
I think using + bstr .cbor SUIT_Envelope_Tagged is better because:

  • we can distinguish SUIT Manifest from other CBOR-based manifests (currently there is no such manifest though)
  • the tag consumes only 2 bytes
  • existing SUIT Manifest generator (suit-tool by Arm and libcsuit by me) creates tagged manifest binary

uint (0..23) is not correct. Like $teep-type they could be uint .size 1.

@dthaler
Copy link
Collaborator Author

dthaler commented Oct 19, 2022

@dthaler I think using + bstr .cbor SUIT_Envelope_Tagged is better because:

  • we can distinguish SUIT Manifest from other CBOR-based manifests (currently there is no such manifest though)

No other format is currently permitted. SUIT_Envelope in the production requires it to be SUIT.
Are you saying you think we should allow for other manifest formats?
That's possible if we change lots of other things in the spec, but would be a big enough change I wouldn't do it without a WG discussion on that topic first. Hence at present there is no need for SUIT_Envelope_Tagged.

  • the tag consumes only 2 bytes

We can save 2 bytes per manifest by not adding the tag which is not needed, just like we removed the COSE tag.

  • existing SUIT Manifest generator (suit-tool by Arm and libcsuit by me) creates tagged manifest binary

That's an issue for the suit-tool to fix by adding an appropriate option.

uint (0..23) is not correct. Like $teep-type they could be uint .size 1.

Acknowledged.

@dthaler
Copy link
Collaborator Author

dthaler commented Oct 19, 2022

@mcd500
Copy link
Collaborator

mcd500 commented Oct 21, 2022

@dthaler @kentakayama
The main reason we decided to not to use COSE Tags on TEEP-Message was that we did not have the bandwidth for creating a new tag number and register to IANA registry that time.

The SUIT Manifest already has an assigned tag by the effort of SUIT working group, so I am perfectly fine using the tag only for the SUIT Manifest which do not effect on implementation which uses a tag on SUIT manifest and not using any tag on entire TEEP massage.

dthaler added a commit that referenced this issue Oct 22, 2022
Addresses #262

Signed-off-by: Dave Thaler <[email protected]>
@mcd500
Copy link
Collaborator

mcd500 commented Oct 23, 2022

This is just a clarification comment.
The reason for preferring uint .size 1 is that in CBOR, the uint expression from 0 to 23 is one byte and over 23 is two bytes or more.
So I wanted to express clearly that it will only use one byte by writing uint (0..23) but the current cddl tool did not recognize this expression, so changed to uint .size 1.

When I was adding examples of CBOR format in the draft, I wanted to show the benefit of using CBOR over other methods, for example json.
By knowing the converted binaries in cbor expressions, it is possible to estimate the exact binary size of each element in TEEP messages except the place uses variable length, such as tstr, bstr and etc.
Then the implementer would easily estimate how much memory size needed to be allocated which is one of the benefits of the cbor.

I personally feel the cbor is a bit complicated than I expected and probably json is acceptable on most cases, however, we have decided to use CBOR for the TEEP protocol at the Hackathon in Berlin in February 2020, it is good to show who is reading the TEEP Protocol spec would understand the benefit of the CBOR.

After many iterations many of my comments are disappearing but I will try to add them by expressing the byte size in the comments section in the CDDL pages.

@dthaler
Copy link
Collaborator Author

dthaler commented Oct 24, 2022

Fixed in draft-11

@dthaler dthaler removed their assignment Oct 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants