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

suit: Initial minimal CBOR-based SUIT manifest parser #10315

Closed
wants to merge 2 commits into from

Conversation

@bergzand
Copy link
Member

commented Nov 2, 2018

Contribution description

Minimal version of the CBOR-based SUIT parser, provides minimal supports for version 1 of the draft specification.
Supported retrieval functions are:

  • manifest version
  • timestamp/sequence number
  • conditions
  • url
  • payloadinfo:
    • digest algorithm
    • digest
    • storage ID
    • size

Testing procedure

Unittest included :)

Issues/PRs references

Todo:

  • split uuid generation
@danpetry danpetry self-requested a review Nov 13, 2018
Copy link
Contributor

left a comment

Hello Koen! Here's a review of the high level aspects.

  1. Would you be able to update it to the latest SUIT version (v3, here)? I'll review it before v4 for sure.
  2. Not tested: suit_uuid_xxx functions (apart from init); suit_isnew. Could you please include explicit tests?
  3. This is a CBOR SUIT parser specifically; other serialization formats are possible. Could you please indicate this in the module name OR abstract away the CBOR-specific stuff so that we can use other serialization formats with this code?
  4. Maybe take the reference to block1 out of the brief, because we have block 2 now as well (i.e. so it just says you need an in-place buffer)
  5. Please split uuid_init as planned (although keeping this function to initialise everything together would also be useful)

Once these are addressed I'll do the other parts of the review together in a second pass. If you'd like me to help with any of the work, eg through a PR against your branch, then please let me know

@bergzand

This comment has been minimized.

Copy link
Member Author

commented Feb 4, 2019

Would you be able to update it to the latest SUIT version (v3, here)? I'll review it before v4 for sure.

After some offline online discussion with @emmanuelsearch we decided to keep this the v1.

Not tested: suit_uuid_xxx functions (apart from init); suit_isnew. Could you please include explicit tests?

Will do.

This is a CBOR SUIT parser specifically; other serialization formats are possible. Could you please indicate this in the module name OR abstract away the CBOR-specific stuff so that we can use other serialization formats with this code?

Version 3 of the draft explicitly calls is a "CBOR-based Firmware Manifest Serialisation Format". Despite this would you still like to have support for different serialization formats?

Maybe take the reference to block1 out of the brief, because we have block 2 now as well (i.e. so it just says you need an in-place buffer)

Done

Please split uuid_init as planned (although keeping this function to initialise everything together would also be useful)

On it.

@bergzand

This comment has been minimized.

Copy link
Member Author

commented Feb 4, 2019

I've reworked the suit parser to:

  1. Split of the conditionals and UUID handling. A separate module to handle more flow related actions will be in a follow up PR.
  2. The suit_manifest_t is reduced to only a pointer and length to the stored SUIT manifest. It is parsed on demand for the necessary information. This increases the processing burden, but reduced the required memory and flash a bit.
@bergzand bergzand requested a review from emmanuelsearch Feb 4, 2019
@bergzand bergzand removed the State: WIP label Feb 4, 2019
@bergzand

This comment has been minimized.

Copy link
Member Author

commented Feb 4, 2019

No longer WIP btw

@bergzand bergzand changed the title [WIP] suit: Initial minimal SUIT parser suit: Initial minimal SUIT parser Feb 4, 2019
Copy link
Contributor

left a comment

Code looks good. Will do some testing now.

#define SUIT_H

#ifdef __cplusplus
extern "C" {

This comment has been minimized.

Copy link
@kaspar030

kaspar030 Feb 26, 2019

Contributor

pls move this below the includes

This comment has been minimized.

Copy link
@bergzand

bergzand Feb 27, 2019

Author Member

Out of interested (don't want to spark a discussion on this), is there a rule/guideline for the order of this somewhere?

This comment has been minimized.

Copy link
@bergzand

bergzand Feb 27, 2019

Author Member

Changed btw

This comment has been minimized.

Copy link
@kaspar030

kaspar030 Feb 27, 2019

Contributor

is there a rule/guideline for the order of this somewhere?

"do as we always did" combined with "close in reversed open order":

  • copyright
  • doxygen open (with file info)
  • header guard open
  • includes
  • ifdef c++ open
  • code
  • ifdef c++ close
  • header guard close
  • doxygen close

for the includes it is somewhat important to be before the "extern C" as they might have other namespacing.

This comment has been minimized.

Copy link
@bergzand

bergzand Feb 27, 2019

Author Member

Thanks for the clarification

sys/include/suit.h Outdated Show resolved Hide resolved
sys/suit/suit.c Outdated Show resolved Hide resolved
sys/suit/suit.c Outdated Show resolved Hide resolved
sys/suit/suit.c Outdated Show resolved Hide resolved
@danpetry

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

Thanks for the nudge @kaspar030 . To comment on the above:

After some offline online discussion with @emmanuelsearch we decided to keep this the v1.

Just interested, what's the reasoning? We keep v1 until the non-draft RFC comes out, rather than updating several times? This seems reasonable to me.

Version 3 of the draft explicitly calls is a "CBOR-based Firmware Manifest Serialisation Format". Despite this would you still like to have support for different serialization formats?

Pagel is also working on a binary serialization format and corresponding RFC:
https://datatracker.ietf.org/wg/suit/documents/
On the mailing list there has been quite a lot of discussion about several potential formats. It's my understanding that there will come other formats. (Correct me if I'm wrong.) This is the main one atm though. What about keeping as is but calling the module suit_cbor? We can make it modular later when we wish to support other formats.

I also think it is cleaner to have this contribution focusing on getting the information from the manifest, with update application logic submitted later, as discussed offline.

Once the point about the module name is addressed, I'm happy with the high level aspects.

@emmanuelsearch

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

@danpetry

We keep v1 until the non-draft RFC comes out, rather than updating several times?

Yup, updating later is the plan, indeed. Depending on how long the standardisation process will last, we'll consider updating to draft version X>3.

What about keeping as is but calling the module suit_cbor?

+1 for something like this, makes sense to me.

@bergzand

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2019

What about keeping as is but calling the module suit_cbor? We can make it modular later when we wish to support other formats.

You've convinced me, will do.

@bergzand bergzand changed the title suit: Initial minimal SUIT parser suit: Initial minimal CBOR-based SUIT manifest parser Feb 27, 2019
@bergzand

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2019

@danpetry @emmanuelsearch renamed ~everything to suit_cbor

@kaspar030

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

@bergzand would you mind rebasing this? Let's see what Murdock says. I suspect it'll want some defines to be documented.

@bergzand bergzand force-pushed the bergzand:pr/suit/initial branch from 7ccd6c5 to cc50d0a Feb 27, 2019
@bergzand

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2019

@bergzand would you mind rebasing this?

Rebased!

I suspect it'll want some defines to be documented.

Not sure what you're talking about 😇

@danpetry

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

Section 3.7 in v1 of the draft gives a list of minimum features that MUST be supported by the target. It looks like, out of this list, these aren't implemented here:

  1. timestamp
  2. conditions

Is that the case? Strongly think that if we're saying this is adherent to the draft (as indicated in the header) then these should be implemented.

@danpetry

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

Sorry I missed this before.

@danpetry

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

@danpetry @emmanuelsearch renamed ~everything to suit_cbor

Thanks :)

@emmanuelsearch

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

thanks for your vigilance @danpetry

In practice, nodes with small micro-controllers such as those targeted by RIOT have no notion of the absolute time on-board.
Two options to solve this issue:

  1. assume some upstream computer (e.g. the software update repo) serves manifests only if the timestamp check passes;
  2. assume the node can poll a trusted remote server which can indicate the current time;

Both options are out of scope for this PR.

For the conditionals, I let @bergzand answer ;)

That said:

  • I agree we should indicate in the comments something like "the implementation complies with v1 specification, except BLAH and BLAH to be addressed later";
  • this is only a parser and not the full process of validation, so upcoming complementary PRs are expected anyhow.

The idea here is to get preliminary building blocks in master, to PR before the IETF104 Hackathon a ready-to-use example of RIOT firmware update using a suit-cbor metadata manifest.
Then at the hackathon, different people could split work to collaborate on next steps (support for more features, upgrade to v4 etc.)

Makes sense?

@bergzand bergzand force-pushed the bergzand:pr/suit/initial branch from b90f6e3 to 182a62f Feb 27, 2019
@danpetry

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

This makes sense to me for sure. In particular that we can use the opportunity of the IETF to push merging of a full main branch OTA update application example.

However I think we should still be careful of merging partial implementations of modules, which to the observer might appear as complete implementations before they discover later that they are only partial. (They might think, "which other specs are only partially implemented which we don't know about?")

Do we consider a note in the API reference to be enough to stop users from making this mistake? If not, what other alerts could we include?

@waehlisch

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

Agree with @danpetry that we should be very careful with announcing what we support. In this case, we cannot claim to provide a "Minimal version of the SUIT parser". I think a clear warning in the API documentation is the very minimum. But you are right @danpetry, the note in the API might be insufficient as the directory name of the implementation gives a different impression.

Anyhow, I don't understand why we are in a rush. For the hackathon, you can provide a ready-to-use example based on a branch that is not master.

Regarding the topic: how to handle cases where the target does not provide absolute time on-board: This should be discussed in the SUIT WG, and we should implement the outcome of this discussion. The result of the discssion might require updating the draft (e.g., in case of option 1).

@bergzand

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2019

@danpetry Thanks for the suggestions.

timestamp

If I'm correct with my interpretation of the draft, this is supported in this PR, the suit_cbor_get_seq_no provides the sequence number of the manifest.

conditions

Conditionals are intentionally not supported yet. My plan is to include this in a follow up PR, but as it is now, I have to think a bit on how to structure the api to make them accessible in a convenient way for an actual SUIT manifest updater.

As stated before by @emmanuelsearch this PR only provides a very minimal parser for SUIT CBOR manifests. There is no handling of the information contained in the manifest. I don't mind adding some comments on what is supported at the moment, but IMHO, the API reference should be enough to give developers an idea of what we support.

@emmanuelsearch

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

this should be discussed in the SUIT WG

+1 yes, that is part of the plan. I am preparing an email to the mailing list as we speak.

For the hackathon, you can provide a ready-to-use example based on a branch that is not SUITmaster.

@waehlisch at the SUIT workshop last June we also "had a branch". It's not necessarily the best approach, because it quickly becomes a jungle few can navigate, and it results in more time to extract elements from that which actually end in master.
Going purposely in smaller steps merging bits quicker in master (assuming they work) was the idea this time around.
We do not want to herald WE HAVE SUIT SUPPORT just yet, even when this PR gets merged -- the spec is work-in-progress anyways.
We want to put ourselves in the best position to have something more people can test and contribute to.

what other alerts could we include?

@danpetry Feel free to propose some complementary warnings ;)
let's just remember it's just a parser, so let's keep things into perspective?

@kaspar030

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

I think squashing is OK at this point.

@bergzand bergzand force-pushed the bergzand:pr/suit/initial branch from a927a3d to e0fb042 Mar 5, 2019
@bergzand

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2019

@kaspar030 Squashed into two commits, one for the parser and one for the unittest

@bergzand bergzand force-pushed the bergzand:pr/suit/initial branch from e0fb042 to 4833046 Mar 5, 2019
@bergzand

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2019

Amended an uncrustify for the unittest in

@danpetry

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

ACK on high level aspects - I'm happy with the approach and testing but haven't looked at the code and internal design in detail.

The only difference I can see between CBOR SUIT v1 and this is the nomenclature of the timestamp/seq no. Reasonably arguments have been given for the use of the term "sequence number". API documentation on the state of the PR seems good to me - people can be reasonably expected to read the documentation and this is now a minor difference. If there are further thoughts about what kind of things should elicit a build system warning, IMO this should be continued in a separate channel.

As discussed offline, this will be updated to more recent versions of the SUIT draft in subsequent PRs.

@danpetry danpetry dismissed their stale review Mar 6, 2019

Dismissing rather than acking because I am giving only a partial ACK as detailed above

@emmanuelsearch

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

tested on native and samr21

@emmanuelsearch

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

Looks like we're good for this PR. Looking forward to the next ones ;)
@kaspar030 I leave you the honor of hitting the green button!

@kaspar030

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

@waehlisch have your issues been resolved?

@waehlisch

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

@bergzand can you please update the description of this PR? "Minimal version of the SUIT parser, more to follow" is not helpful as discussed before.

Otherwise fine. Looking forward to quick alignement with more recent spec versions.

@bergzand

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2019

@bergzand can you please update the description of this PR? "Minimal version of the SUIT parser, more to follow" is not helpful as discussed before.

Updated to something more descriptive, thanks for the heads-up

@kaspar030

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

Updated to something more descriptive, thanks for the heads-up

@waehlisch please hit merge when satisfied!

* The parser is based on draft version 1 of the specification, restricted to
* handling CBOR based manifests.
*
* @see https://tools.ietf.org/html/draft-moran-suit-manifest-01

This comment has been minimized.

Copy link
@miri64

miri64 Mar 7, 2019

Member

Maybe add the @experimental tag as well, since this is based on a draft?

This comment has been minimized.

Copy link
@miri64

miri64 Mar 7, 2019

Member

see

ALIASES = experimental="@warning <strong class=\"text-danger\">This feature is experimental!</strong>\n"

This comment has been minimized.

Copy link
@bergzand

bergzand Mar 11, 2019

Author Member

Done

bergzand added 2 commits Mar 5, 2019
@bergzand bergzand force-pushed the bergzand:pr/suit/initial branch from 4833046 to ea15dd2 Mar 11, 2019
@bergzand

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2019

I've added the @experimental tag as suggested by @miri64. I've amended directly, see the force push diff for verification.

@kaspar030

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

@waehlisch please hit merge when satisfied!

Copy link
Contributor

left a comment

un-ACK, v4 is fundamentally different, let's see if we still need this.

@bergzand

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2019

un-ACK, v4 is fundamentally different, let's see if we still need this.

Agreed!

@emmanuelsearch

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

@bergzand closing this one since we are now planning to get SUIT support through the cluster of PRs around #11818 (but do feel free to reopen if you think it is necessary!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.