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

pkg: add support for libcose #8895

Merged
merged 2 commits into from Apr 16, 2018

Conversation

2 participants
@bergzand
Copy link
Member

commented Apr 6, 2018

Contribution description

libcose is a library implementing parts of the COSE spec. As of v0.2 Signing and verification with ed25519 based keys is supported. This package relies on cn-cbor, a block allocator and a crypto library (either TweetNaCl or LibSodium at this moment). Part of the tests used by libcose is reworked as a unittest test to provide RIOT specific tests.

COSE is used by both SUIT and OSCORE for security. The reason to keep this library out of the RIOT tree is to ensure it's stand alone and generic nature so that other projects can benefit from it.

Issues/PRs references

Depends on #7651, had to rebase that for cn-cbor fixes, so commit history is a bit messy at the moment.

@kYc0o
Copy link
Contributor

left a comment

First pass. Some comments here and there. I'll test ASAP.

@@ -0,0 +1,221 @@
/*
*

This comment has been minimized.

Copy link
@kYc0o

kYc0o Apr 6, 2018

Contributor

No copyright?

This comment has been minimized.

Copy link
@bergzand

bergzand Apr 9, 2018

Author Member

Oops

/* First signer */
printf("Payload added, generating keypair\n");
crypto_sign_keypair(pk, sk);
//cose_crypto_keypair_ed25519(pk, sk);

This comment has been minimized.

Copy link
@kYc0o

kYc0o Apr 6, 2018

Contributor

Please remove C++ comments.

This comment has been minimized.

Copy link
@bergzand

bergzand Apr 9, 2018

Author Member

Fixed


static void cose_free(void *ptr, void *memblock)
{
cur_alloc--;

This comment has been minimized.

Copy link
@kYc0o

kYc0o Apr 6, 2018

Contributor

Is it really necessary to keep track of the allocations? I understand why you do it but I wonder if you can't just "trust" the memory allocator on this. Anyways I'll try to test it with valgrind.

This comment has been minimized.

Copy link
@bergzand

bergzand Apr 9, 2018

Author Member

This is more a test on whether the libcose code correctly allocates and frees all used blocks. As the block allocater here uses a list of static memory and not the heap I'm not sure whether valgrind is able to analyze the memory usage of the block allocator.

Futhermore I prefer a hard test failure when COSE doesn't correctly free all memory and not depend on an external tool for it.

* @brief MANDATORY function for collecting random Bytes
* required by the tweetnacl package
*/
extern void randombytes(uint8_t *target, uint64_t n);

This comment has been minimized.

Copy link
@kYc0o

kYc0o Apr 6, 2018

Contributor

Hmm... I think this should be embedded in RIOT, but AFAIR there was a lot of discussion which ended into "prefer not having one instead of a non-truly-super-well-seeded-without-flaws random function"... But well, anyways it's not clear that the function is ALSO implemented in the tweetnacl package.

This comment has been minimized.

Copy link
@bergzand

bergzand Apr 9, 2018

Author Member

Removed the declaration here as it was indeed not needed, already taken care of by the tweetnacl package.

@bergzand bergzand force-pushed the bergzand:pr/pkg/libcose branch from ad758cc to 21a0571 Apr 10, 2018

@bergzand

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2018

Rebased, doesn't depend on any other PR anymore

@kYc0o

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2018

It's still WIP?

@bergzand bergzand removed the State: WIP label Apr 11, 2018

@bergzand bergzand changed the title WIP: pkg: add support for libcose pkg: add support for libcose Apr 11, 2018

@bergzand

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2018

It's still WIP?

No

@kYc0o

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2018

I didn't finish the review yet but I started testing, these are my results so far:

On a samr21-xpro, running unittests:

2018-04-12 17:40:05,237 - INFO # main(): This is RIOT! (Version: 2018.07-devel-853-g21a05-snake.local-pr/pkg/libcose)
2018-04-12 17:40:05,243 - INFO # Running tests
2018-04-12 17:40:05,243 - INFO # .Running test 01
2018-04-12 17:40:05,244 - INFO # Start init
2018-04-12 17:40:05,245 - INFO # Initialized, adding payload
2018-04-12 17:40:05,247 - INFO # Payload added, generating keypair

And it hangs there.

When I activate ps, I discovered that you are in the limit of the stack, which is quickly surpassed by the usage of tweetnacl:

2018-04-12 17:51:41,018 - INFO # main(): This is RIOT! (Version: 2018.07-devel-853-g21a05-snake.local-pr/pkg/libcose)
2018-04-12 17:51:41,019 - INFO # Running tests
2018-04-12 17:51:41,020 - INFO # .Running test 01
2018-04-12 17:51:41,022 - INFO # Start init
2018-04-12 17:51:41,024 - INFO # Initialized, adding payload
2018-04-12 17:51:41,033 - INFO # 	pid | name                 | state    Q | pri | stack  ( used) | base addr  | current     
2018-04-12 17:51:41,040 - INFO # 	  - | isr_stack            | -        - |   - |    512 (  112) | 0x20000000 | 0x200001b8
2018-04-12 17:51:41,049 - INFO # 	  1 | idle                 | pending  Q |  15 |    256 (  124) | 0x200003d8 | 0x2000045c 
2018-04-12 17:51:41,060 - INFO # 	  2 | main                 | running  Q |   7 |   1536 ( 1504) | 0x200004d8 | 0x20000a5c 
2018-04-12 17:51:41,063 - INFO # 	    | SUM                  |            |     |   2304 ( 1740)
2018-04-12 17:51:41,066 - INFO # Payload added, generating keypair

So anyways, if it's not increased it will hang there forever (though I wonder why there's no kernel panic at this moment).

With increased stack, it advances a bit more:

2018-04-12 17:53:32,542 - INFO # main(): This is RIOT! (Version: 2018.07-devel-853-g21a05-snake.local-pr/pkg/libcose)
2018-04-12 17:53:32,544 - INFO # Running tests
2018-04-12 17:53:32,546 - INFO # .Running test 01
2018-04-12 17:53:32,547 - INFO # Start init
2018-04-12 17:53:32,549 - INFO # Initialized, adding payload
2018-04-12 17:53:32,557 - INFO # 	pid | name                 | state    Q | pri | stack  ( used) | base addr  | current     
2018-04-12 17:53:32,565 - INFO # 	  - | isr_stack            | -        - |   - |    512 (  112) | 0x20000000 | 0x200001b8
2018-04-12 17:53:32,573 - INFO # 	  1 | idle                 | pending  Q |  15 |    256 (  124) | 0x200003d8 | 0x2000045c 
2018-04-12 17:53:32,599 - INFO # 	  2 | main                 | running  Q |   7 |   4608 ( 1504) | 0x200004d8 | 0x2000165c 
2018-04-12 17:53:32,601 - INFO # 	    | SUM                  |            |     |   5376 ( 1740)
2018-04-12 17:53:32,605 - INFO # Payload added, generating keypair
2018-04-12 17:53:36,574 - INFO # Keypair ready, building signer
2018-04-12 17:53:40,571 - INFO # Encoded size for sign1: 103
2018-04-12 17:53:40,592 - INFO # 8443A10127A104506B6F656E4072696F742D6F732E6F72674C496E70757420737472696E6758402B3FEBBD007532BFC52B53B0F6FF53FD396B391B5387007536E69AA1E54665E8168D5574A14006B130AE66D708DDEA63E09BBE54E43F18D4618AAE4BCB3A440A
2018-04-12 17:53:40,606 - INFO # Signature: 2B3FEBBD007532BFC52B53B0F6FF53FD396B391B5387007536E69AA1E54665E8168D5574A14006B130AE66D708DDEA63E09BBE54E43F18D4618AAE4BCB3A440A
2018-04-12 17:53:48,587 - INFO # Verification: 0
2018-04-12 17:53:48,587 - INFO # 
2018-04-12 17:53:48,589 - INFO # Context before hardfault:
2018-04-12 17:53:48,591 - INFO #    r0: 0x2000146c
2018-04-12 17:53:48,592 - INFO #    r1: 0x20002bd9
2018-04-12 17:53:48,593 - INFO #    r2: 0x00000000
2018-04-12 17:53:48,595 - INFO #    r3: 0x0000163d
2018-04-12 17:53:48,597 - INFO #   r12: 0x0000000f
2018-04-12 17:53:48,598 - INFO #    lr: 0x0000066f
2018-04-12 17:53:48,600 - INFO #    pc: 0x0000165c
2018-04-12 17:53:48,602 - INFO #   psr: 0x41000000
2018-04-12 17:53:48,602 - INFO # 
2018-04-12 17:53:48,602 - INFO # Misc
2018-04-12 17:53:48,604 - INFO # EXC_RET: 0xfffffffd
2018-04-12 17:53:48,608 - INFO # Attempting to reconstruct state for debugging...
2018-04-12 17:53:48,609 - INFO # In GDB:
2018-04-12 17:53:48,610 - INFO #   set $pc=0x165c
2018-04-12 17:53:48,611 - INFO #   frame 0
2018-04-12 17:53:48,612 - INFO #   bt
2018-04-12 17:53:48,612 - INFO # 
2018-04-12 17:53:48,616 - INFO # ISR stack overflowed by at least 8 bytes.

And finally I get my beautiful hardfault :-)

@kYc0o
Copy link
Contributor

left a comment

I found some minor things, but I'd prefer to focus on having passing tests.

#include <stdlib.h>
#include <string.h>

#include <tweetnacl.h>

This comment has been minimized.

Copy link
@kYc0o

kYc0o Apr 12, 2018

Contributor

Why it's included with <>? Since it's not a system header I'd say it needs to be with "".

This comment has been minimized.

Copy link
@bergzand

bergzand Apr 13, 2018

Author Member

Fixed

#include "tests-libcose.h"

static char kid[] = "koen@riot-os.org";
static char kid2[] = "paco@riot-os.org";

This comment has been minimized.

Copy link
@kYc0o

kYc0o Apr 12, 2018

Contributor

Bedankt! :-P

This comment has been minimized.

Copy link
@bergzand

bergzand Apr 13, 2018

Author Member

Changed :(

memset(block, 0, sizeof(cn_cbor));
cur_alloc++;
if (cur_alloc > max_alloc)
{

This comment has been minimized.

Copy link
@kYc0o

kYc0o Apr 12, 2018

Contributor

Braces up!

This comment has been minimized.

Copy link
@bergzand

bergzand Apr 13, 2018

Author Member

uncrustified

.calloc_func = cose_calloc,
.free_func = cose_free,
.context = &storage,
};

This comment has been minimized.

Copy link
@kYc0o

kYc0o Apr 12, 2018

Contributor

I'd advice to either put it at the beginning (with indentation) where you declare all the global variables, or you add some indentation to identify it better. At a first glance it looks like a malformed function.

This comment has been minimized.

Copy link
@bergzand

bergzand Apr 13, 2018

Author Member

Changed it a bit, can you check if it looks less like a malformed function now?

This comment has been minimized.

Copy link
@kYc0o

kYc0o Apr 13, 2018

Contributor

It's OK now.

@bergzand

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2018

@kYc0o I've minimized the tests to include only the bare minimum to verify functionality of the library. I've added the unittest to the list of tests requiring a larger stack, the same way as is done with TweetNaCl.

@kYc0o
Copy link
Contributor

left a comment

Found two other minor things. Afterwards I think it can be merged (if I succeed with the tests).

@@ -1,4 +1,4 @@
DEVELHELP ?= 0
DEVELHELP ?= 1

This comment has been minimized.

Copy link
@kYc0o

kYc0o Apr 13, 2018

Contributor

Are you sure about this?

This comment has been minimized.

Copy link
@bergzand

bergzand Apr 13, 2018

Author Member

No, fixed


USEMODULE += random
USEPKG += libcose
USEMODULE += libcose_crypt_tweetnacl

This comment has been minimized.

Copy link
@kYc0o

kYc0o Apr 13, 2018

Contributor

Why this is duplicated in the Makefile too? Plus, I don't see where this Makefile gets included...

This comment has been minimized.

Copy link
@bergzand

bergzand Apr 13, 2018

Author Member

Removed it from the tests/unittests/tests-libcose/Makefile. This file is included here

@bergzand

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2018

Removed memarray from the package deps as it is technically not a hard dependency, but just one of the possible ways to supply the underlying cn-cbor with blocks.

@bergzand bergzand force-pushed the bergzand:pr/pkg/libcose branch from b682ad1 to 1bf4531 Apr 13, 2018

@kYc0o

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2018

Well, this time works correctly, but it takes quite long time. I propose to warn this at the beginning of the test, so the tester waits long enough to see the results.

2018-04-13 15:03:23,023 - INFO # main(): This is RIOT! (Version: 2018.07-devel-875-g1bf45-snake.local-pr/pkg/libcose)
2018-04-13 15:04:38,832 - INFO # ..
2018-04-13 15:04:38,834 - INFO # OK (2 tests)
@bergzand

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2018

Well, this time works correctly, but it takes quite long time. I propose to warn this at the beginning of the test, so the tester waits long enough to see the results.

I've added some printf statements. It might be a bit verbose now, but at least the tester is warned now :)

.calloc_func = cose_calloc,
.free_func = cose_free,
.context = &storage,
};

This comment has been minimized.

Copy link
@kYc0o

kYc0o Apr 13, 2018

Contributor

It's OK now.

void tests_libcose(void)
{
printf("Starting libcose test, performing multiple signature operations.\n");
printf("This can take a while (up to 2 minutes on the samr21-xpro\n");

This comment has been minimized.

Copy link
@kYc0o

kYc0o Apr 13, 2018

Contributor

Missing parenthesis at the end.

This comment has been minimized.

Copy link
@bergzand

bergzand Apr 13, 2018

Author Member

Fixed

@kYc0o

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2018

All good here. Please squash.

@kYc0o

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2018

I think I should have referenced #8947 here, since the changes of this PR modified the behaviour of the cn-cbor. Now that #8947 was merged you might rebase this one and I think it would be ready to go!

@bergzand bergzand force-pushed the bergzand:pr/pkg/libcose branch from 821da54 to c7e3be0 Apr 16, 2018

@bergzand

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2018

I bumped to libcose version v0.3.1 which has mbedtls and HaCl* support. Furthermore really experimental COSE encrypt support is included in this release which is reflected in the test.

I've enable HaCl as a default crypto lib for the test since it is much faster:

2018-04-16 16:50:22,678 - INFO # This can take a while (up to 2 minutes on the samr21-xpro)
2018-04-16 16:51:26,880 - INFO # ...
2018-04-16 16:51:26,882 - INFO # OK (3 tests)
2018-04-16 16:52:52,009 - INFO # Exiting Pyterm

@bergzand bergzand force-pushed the bergzand:pr/pkg/libcose branch from c7e3be0 to 1675780 Apr 16, 2018

@bergzand

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2018

Fixed a few lines that were longer than 80 chars

@kYc0o
Copy link
Contributor

left a comment

I still have some questions. I'll test again for native and samr21-xpro.

@@ -0,0 +1,3 @@
MODULE := libcose

This comment has been minimized.

Copy link
@kYc0o

kYc0o Apr 16, 2018

Contributor

Why here := and below SUBMODULES =? I think I saw more often the opposite. Though, there's a specific use for each one so I prefer to have a strong reason for using them.

This comment has been minimized.

Copy link
@bergzand

bergzand Apr 16, 2018

Author Member

Both := and = are used equally with MODULE in packages. Both work in this case. Anything you want me to do with them?

This comment has been minimized.

Copy link
@kYc0o

kYc0o Apr 16, 2018

Contributor

My comment it's not related to the fact if it works or not. I know both work, but not both are correct. You can have a hint here. If there are places where this is wrongly used, we might correct them in follow up PRs. I just want to have a clean PR here.

So, I'd say the correct way to do is MODULE = libcose and SUBMODULES := 1. In case I misunderstood the usages, can @cladmi give us a hand here?

This comment has been minimized.

Copy link
@bergzand

bergzand Apr 16, 2018

Author Member

As there is nothing to expand in these variables, in this case they are equivalent. If you want to have it consistent, I can change it so that both are set with :=

This comment has been minimized.

Copy link
@kYc0o

kYc0o Apr 16, 2018

Contributor

Ok, just leave it as it is. I agree here it doesn't matter, just for the sake of consistency at least there's a record.

@@ -215,6 +215,7 @@ ifneq (, $(filter $(AVR_BOARDS), $(BOARD)))
endif

LARGE_STACK_TESTS += tests-hacl
LARGE_STACK_TESTS += tests-libcose

This comment has been minimized.

Copy link
@kYc0o

kYc0o Apr 16, 2018

Contributor

What's the difference of putting them all in one line? I think this looks rather hacky and only for "good reading" purposes. So I'd say this variable, since is repeatedly needed, should be declared somewhere else and being set in the correspondent package. Actually how it was before it wasn't bad, but it depends on what's the purpose of the formatting.

This comment has been minimized.

Copy link
@bergzand

bergzand Apr 16, 2018

Author Member

The lines are only split for reading purposes. If you want them declared in the separate packages I suggest we do that in a follow up as it would need multiple line changes such as moving L220-L222 below L227 and that is going outside the scope of this PR.

This comment has been minimized.

Copy link
@kYc0o

kYc0o Apr 16, 2018

Contributor

OK.

* @author Koen Zandberg <koen@bergzand.net>
*/

#define MAX_NUMBER_BLOCKS 128

This comment has been minimized.

Copy link
@kYc0o

kYc0o Apr 16, 2018

Contributor

Any motivation to have 128? If there's a meaning it would be nice to have a small comment.

This comment has been minimized.

Copy link
@bergzand

bergzand Apr 16, 2018

Author Member

Added a small explanation about the meaning of the define


static void *cose_calloc(size_t count, size_t size, void *memblock)
{
(void)count;

This comment has been minimized.

Copy link
@kYc0o

kYc0o Apr 16, 2018

Contributor

You're not checking for correct allocations anymore?

This comment has been minimized.

Copy link
@bergzand

bergzand Apr 16, 2018

Author Member

If with correct allocations you mean the counters to keep track of the number of allocated and freed blocks, then no. I removed them in favor of a simple and small test.

This comment has been minimized.

Copy link
@kYc0o

kYc0o Apr 16, 2018

Contributor

Yes, I was asking for that. It's ok for me.

@bergzand

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2018

Decreased the number of cn-cbor blocks preallocated and added a description.

@bergzand bergzand force-pushed the bergzand:pr/pkg/libcose branch 2 times, most recently from 5f088f4 to 3d06466 Apr 16, 2018

bergzand added some commits Apr 16, 2018

@bergzand bergzand force-pushed the bergzand:pr/pkg/libcose branch from 3d06466 to ba809c2 Apr 16, 2018

@kYc0o

kYc0o approved these changes Apr 16, 2018

Copy link
Contributor

left a comment

ACK.

@kYc0o kYc0o merged commit 9e010a1 into RIOT-OS:master Apr 16, 2018

2 checks passed

Murdock The build succeeded. runtime: 5m:31s
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

maxvankessel added a commit to maxvankessel/RIOT that referenced this pull request Apr 20, 2018

@kYc0o kYc0o added this to the Release 2018.04 milestone Aug 13, 2018

@kYc0o kYc0o added this to Done in Software Updates Sep 21, 2018

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.