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

RFC: Replace TinyCBOR with ZCBOR within Zephyr #40591

Closed
de-nordic opened this issue Nov 23, 2021 · 25 comments · Fixed by #44635
Closed

RFC: Replace TinyCBOR with ZCBOR within Zephyr #40591

de-nordic opened this issue Nov 23, 2021 · 25 comments · Fixed by #44635
Assignees
Labels
RFC Request For Comments: want input from the community

Comments

@de-nordic
Copy link
Collaborator

de-nordic commented Nov 23, 2021

Problem description

Currently Zephyr uses specific variant of TinyCBOR (https://github.com/zephyrproject-rtos/tinycbor), that has been imported to support mcumgr library from internals of Mynewt (https://github.com/apache/mynewt-core/tree/master/encoding/tinycbor); the Zephyr/mynewt variant is not fully API compatible with the original implementation that comes from Intel (https://github.com/intel/tinycbor).
In short; the TinyCBOR module we have has been made internal source code of Mynewt then extracted as module/library to Zephyr.
The mcuboot (and tfm) have already moved to ZCBOR (https://github.com/NordicSemiconductor/zcbor/) , for SMP protocol implementation) and currently the only in-tree usage of TinyCBOR is mcumgr, so also SMP implementation.
This means that we already, considering the mcuboot to be Zephyr application, have two CBOR libraries/implementations.

The zcbor is lighter in code than the TinyCBOR and quite API compatible with the latter. Also the zcbor allows to use CDDL to describe frames and auto generate code for CBOR stream parsing.

Replacement would also mean removal of the TinyCBOR as a module and inclusion of zcbor in its place.

An implementation of OSCORE+EDHOC is available for Zephyr using zcbor.

Concerns and Unresolved Questions

How to deal with 3'rd party modules that use different CBOR implementations?

Reference materials

Zephyr TinyCBOR fork: https://github.com/zephyrproject-rtos/tinycbor
Zephyr TinyCBOR fork origins: https://github.com/apache/mynewt-core/tree/master/encoding/tinycbor
Intel TinyCBOR (the real one): https://github.com/intel/tinycbor
CDDL: https://github.com/NordicSemiconductor/cddl-gen
CDDL is now zcbor: https://github.com/NordicSemiconductor/zcbor/
QCBOR: https://github.com/laurencelundblade/QCBOR

Other CBOR libarary introduction requests:

QCBOR (as a module) #40297

@de-nordic de-nordic added the RFC Request For Comments: want input from the community label Nov 23, 2021
@carlescufi
Copy link
Member

carlescufi commented Dec 10, 2021

Note: TF-M is using QCBOR: https://git.trustedfirmware.org/TF-M/trusted-firmware-m.git/tree/lib/ext/qcbor
Note: MCUBoot uses zcbor

@de-nordic de-nordic changed the title RFC: So many CBOR implementations RFC: Replace TinyCBOR with ZCBOR within Zephyr Feb 7, 2022
@de-nordic de-nordic self-assigned this Feb 7, 2022
@microbuilder
Copy link
Member

Something we'd been working with internally at Linaro is getting this up to date to work with MbedTLS 3.0, and enable COSE support for SIGN and ENCRYPT on top of CBOR: https://github.com/lindemer/cozy

But after going down that route, I think we've going to try to use QCBOR on the Zephyr side as well, plus https://github.com/laurencelundblade/t_cose for the COSE signature validation on top of QCBOR.

Given the important of TF-M to Nordic, and to other members like Linaro, it might make more sense to be investing in a common library on both sides of the S/NS divide, and in t_cose on top of that for COSE functionality?

@microbuilder
Copy link
Member

@d3zd3z Curious if you have some feedback on the effort required to migrate MCUBoot to QCBOR in the future to align MCUBoot/TF-M/Zephyr on a common library? Was that ruled out as an option in the past for a specific reason?

@rerickson1
Copy link
Member

@lairdjm

@carlescufi
Copy link
Member

@d3zd3z Curious if you have some feedback on the effort required to migrate MCUBoot to QCBOR in the future to align MCUBoot/TF-M/Zephyr on a common library? Was that ruled out as an option in the past for a specific reason?

The original migration from TinyCBOR to zcbor was done by @oyvindronningstad FYI, so it might be better to ask him. Also, MCUBoot AFAIK uses the advanced CDDL codegen features from zcbor, something that QCBOR doesn't support.

@microbuilder
Copy link
Member

Is there a COSE option for zcbor? It seems like something worth keeping in mind if you care about signing or encrypting CBOR payloads, or decoding them inside Zephyr.

@de-nordic
Copy link
Collaborator Author

The original migration from TinyCBOR to zcbor was done by @oyvindronningstad FYI, so it might be better to ask him. Also, MCUBoot AFAIK uses the advanced CDDL codegen features from zcbor, something that QCBOR doesn't support.

Yes, the MCUBOOT uses CDDL to generate CBOR decoder for the requests.
This is also something that would be useful for the MCUMGR where cborattr provides similar feature, where the decoding is done basing on request description done with specific C structures that describe expected structure and how to handle fields.

@de-nordic
Copy link
Collaborator Author

de-nordic commented Feb 8, 2022

Is there a COSE option for zcbor? It seems like something worth keeping in mind if you care about signing or encrypting CBOR payloads, or decoding them inside Zephyr.

As far as I can tell the COSE supports NanoCBOR only. (oh no... another CBOR)

@microbuilder
Copy link
Member

microbuilder commented Feb 8, 2022

Is there a COSE option for zcbor? It seems like something worth keeping in mind if you care about signing or encrypting CBOR payloads, or decoding them inside Zephyr.

As far as I can tell the COSE supports NanoCBOR only. (oh no... another CBOR)

t_cose is built on top of MbedTLS and QCBOR, and we're looking at extending that with more than SIGN support on the TF-M side, but whatever solution ends up making the most sense for Zephyr should take COSE into consideration.

@carlescufi
Copy link
Member

Is there a COSE option for zcbor? It seems like something worth keeping in mind if you care about signing or encrypting CBOR payloads, or decoding them inside Zephyr.

There is an implementation of COSE in the OSCORE library that will be submitted to Zephyr. This is the COSE code that we plan to use for Zephyr instead of t_cose.

@microbuilder
Copy link
Member

Is there a COSE option for zcbor? It seems like something worth keeping in mind if you care about signing or encrypting CBOR payloads, or decoding them inside Zephyr.

There is an implementation of COSE in the OSCORE library that will be submitted to Zephyr. This is the COSE code that we plan to use for Zephyr instead of t_cose.

Do you know what it supports? SIGN1, SIGN, ENCRYPT, ENCRYPT0? There are a lot of incomplete COSE libraries out there, and most need some additions to support both SIGN and ENCRYPT, so if we have to add code to more than one implementation it's better to have a clear understanding of that beforehand.

@microbuilder
Copy link
Member

It looks like it only support ENCRYPT0 (https://datatracker.ietf.org/doc/html/rfc8613#section-5), meaning a single recipient. That's better than nothing, but limits if you have multiple recipients where you will have to include one shared secret for each recipient.

I don't see any mention of SIGN1 or SIGN, but presumably SIGN1 is used during the encryption process (single signature attached to the basic COSE structure).

@nordicjm
Copy link
Collaborator

nordicjm commented Feb 8, 2022

It looks like it only support ENCRYPT0 (https://datatracker.ietf.org/doc/html/rfc8613#section-5), meaning a single recipient. That's better than nothing, but limits if you have multiple recipients where you will have to include one shared secret for each recipient.

I don't see any mention of SIGN1 or SIGN, but presumably SIGN1 is used during the encryption process (single signature attached to the basic COSE structure).

mcumgr is command and response over the interface it was received on, so I'm not understanding why multiple recipients would be a problem if there is only a single recipient?

@carlescufi
Copy link
Member

carlescufi commented Feb 8, 2022

mcumgr is command and response over the interface it was received on, so I'm not understanding why multiple recipients would be a problem if there is only a single recipient?

I think the concern is not about mcumgr, but rather about future protocols using these CBOR/COSE libraries

@StefanHri
Copy link

OSCORE and EDHOC use both COSE and CBOR. In our implementation (uoscore-uedhoc ) we use zcbor to build the required COSE structures, which are a subset of the COSE specification. The COSE-related functions are however not exposed to the uoscore-uedhoc user and therefore they are not directly usable. For OSCORE and EDHOC is in my opinion sufficient to have only a CBOR library.

@carlescufi
Copy link
Member

For OSCORE and EDHOC is in my opinion sufficient to have only a CBOR library.

Thanks for the input. Let's be conservative about this. Right now we have no need for a fully-fledged COSE library in Zephyr. If we introduce one in the future then OSCORE can make use of it instead of implementing it internally.

@microbuilder
Copy link
Member

For OSCORE and EDHOC is in my opinion sufficient to have only a CBOR library.

Thanks for the input. Let's be conservative about this. Right now we have no need for a fully-fledged COSE library in Zephyr. If we introduce one in the future then OSCORE can make use of it instead of implementing it internally.

It's something we were actively working on right now, to try to enable decoding COSE SIGNed and ENCRYPTed payloads in Zephyr. Given that this already exists in TF-M (COSE signatures are used in attestation tokens, for example), reusing QCBOR and extending t_cose seemed the least amount of work on the NS side, but I'm not opposed to something different in Zephyr.

But from the position of maintaining TF-M, I'd lightly object to something where there wasn't a viable path to enable COSE support on the NS/Zephyr side. It sounds like OSCORE is doing all the COSE encoding by hand, which seems cumbersome and error-prone if we have to implement all the cryptographic calls ourselves and format things byte by byte.

I know this is of consequence to the TF-M Firmware Update service, for example, since there has been some discussion of using COSE ENCRYPTed firmware images, although it's still at the research or proof of concept phase.

I'm not going to block some other library going in, obviously, but I do think it would be a shame to be working on two different CBOR and COSE libraries.

@microbuilder
Copy link
Member

To clarify ... I don't object to migrating to zcbor, which is a better situation that what we have today, but for COSE we may end up with one of:

  • Two CBOR libraries later to enable t_cose (etc.), which depends on QCBOR
  • Writing a new COSE library that makes used of zcbor
  • Updating something like t_cose to make use of zcbor beneath the hood

They're all valid options if moving to QCBOR isn't feasible today.

But I do think COSE is something that will show up in a handful of TF-M related projects in the near future, so we should keep it in mind.

@StefanHri
Copy link

I think having a COSE library in Zephyr will be very nice because there are as well other drafts going on in IETF that use COSE which may become interesting for Zephyr as well. However for now I am perfectly fine with zcbor. If at some point a COSE library is available we can migrate the related code in our OSCORE and EDHOC implementations.

@carlescufi
Copy link
Member

carlescufi commented Feb 9, 2022

Thanks for the input @microbuilder, you make good points.

To clarify ... I don't object to migrating to zcbor, which is a better situation that what we have today, but for COSE we may end up with one of:

  • Two CBOR libraries later to enable t_cose (etc.), which depends on QCBOR

Given that Zephyr doesn't allow multiple implementation of the same functionality to coexist (unless one is deprecated of course), this is unlikely to be an option.

  • Writing a new COSE library that makes used of zcbor

Depending on how things evolve this might be a good option.

  • Updating something like t_cose to make use of zcbor beneath the hood

This would probably work as well.

They're all valid options if moving to QCBOR isn't feasible today.

It's not that moving to QCBOR is not feasible, is that QCBOR and t_cose are small projects that are not widely used and that are outside of Zephyr's control. Given that there is no immediate requirement for the functionality they offer in the non-secure image (aside from OSCORE and EDHOC) I would rather be cautious and not pull in a 3rd-party project like QCBOR at this point.

But I do think COSE is something that will show up in a handful of TF-M related projects in the near future, so we should keep it in mind.

Definitely. But I don't want to put the cart before the horses, we've been bitten by this in the past.

@de-nordic
Copy link
Collaborator Author

I have looked at the https://github.com/lindemer/cozy and its use of NanoCBOR.
The NanoCBOR looks quite similar to zcbor and TinyCBOR: all of them use context/state and have API calls that add simple types.
I was not able yet to look at the decode side of cozy, but the encode side seems quite neat and should be easy to make CBOR implementation agnostic. It should be possible to redefine the cbor context/state type according to used CBOR implementation and provide header files that would be defining intermediate CBOR API calls inlining used API (zcbor, NanoCBOR) function calls.

The https://github.com/laurencelundblade/t_cose looks more complicated in that matter, and uses some QCBOR specific calls that does not seem to have equivalence in other implementations (for example QCBORDecode_GetNthTagOfLast).

@d3zd3z
Copy link
Collaborator

d3zd3z commented Feb 9, 2022

As far as MCUboot, we're unlikely to want to use CBOR out of the Zephyr tree, anyway, since we support more than just Zephyr (even if the current code is Zephyr specific, it'd be nice if it wasn't).

@microbuilder
Copy link
Member

@de-nordic We were able to update cozy to work with MbedTLS 3.0 without too much trouble, where we needed to decode a COSE packet and verify the signature. The library seems to have been abandonned, but adopting it in the zephyr community is an option.

I'm OK with almost any sensible library ... I'd just like to avoid a situation where three(+) different companies are working on three different libraries, such as Arm on t_cose and QCBOR for TF-M, Nordic with zcbor and ??? for COSE, Linaro with cozy and NanoCBOR (although we have been looking at moving to t_cose and QCBOR just to align with Arm engineering efforts in TF-M).

@microbuilder
Copy link
Member

As far as MCUboot, we're unlikely to want to use CBOR out of the Zephyr tree, anyway, since we support more than just Zephyr (even if the current code is Zephyr specific, it'd be nice if it wasn't).

I understand MCUBoot won't want to pull in the Zephyr dependency, but it does seem like a shame not to pool engineering effort to improve ONE set of CBOR + COSE libraries since, at least on the COSE side, they all tend to be quite weak (and neglected) today and have large holes such as supporting SIGN but not ENCRYPT, or only one cipher for ENCRYPT or only ENCRYPT1, etc.

@d3zd3z
Copy link
Collaborator

d3zd3z commented Feb 10, 2022

I understand MCUBoot won't want to pull in the Zephyr dependency, but it does seem like a shame not to pool engineering effort to improve ONE set of CBOR + COSE libraries since, at least on the COSE side, they all tend to be quite weak (and neglected) today and have large holes such as supporting SIGN but not ENCRYPT, or only one cipher for ENCRYPT or only ENCRYPT1, etc.

This will become more relevant to MCUboot if/when it starts to get SUIT support, which is built around COSE.

carlescufi added a commit to carlescufi/tinycbor that referenced this issue Apr 7, 2022
See zephyrproject-rtos/zephyr#40591 for
additional details.

Signed-off-by: Carles Cufi <[email protected]>
carlescufi added a commit to carlescufi/tinycbor that referenced this issue Apr 7, 2022
See zephyrproject-rtos/zephyr#40591 for
additional details.

Signed-off-by: Carles Cufi <[email protected]>
carlescufi added a commit to zephyrproject-rtos/tinycbor that referenced this issue Apr 7, 2022
See zephyrproject-rtos/zephyr#40591 for
additional details.

Signed-off-by: Carles Cufi <[email protected]>
carlescufi added a commit to carlescufi/zephyr that referenced this issue Apr 7, 2022
See zephyrproject-rtos#40591 for details, TinyCBOR (or rather the fork of TinyCBOR that we
had) is being replaced by zcbor.

Closes zephyrproject-rtos#40591.

Signed-off-by: Carles Cufi <[email protected]>
carlescufi added a commit that referenced this issue Apr 8, 2022
See #40591 for details, TinyCBOR (or rather the fork of TinyCBOR that we
had) is being replaced by zcbor.

Closes #40591.

Signed-off-by: Carles Cufi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC Request For Comments: want input from the community
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants