sys: add credman (D)TLS credential management module #11564

pokgak · 2019-05-22T15:54:04Z

Contribution description

This PR is the first part to add sock_dtls module for (D)TLS support in RIOT. credman can also be used
to handle credentials for TLS if TLS is supported in RIOT in the future.

To use DTLS, users must first tell the DTLS sock which credential needs to be used for the
encryption and decryption process. There are two use cases here, 1) the user has direct access to the
DTLS sock and can actually give the pointer of the credential buffer to the sock, 2) the user does not have access to the sock because it is located in a lower leveled library used by user application (e.g. gcoap + DTLS).

credman handles both of these by providing a unified way to pass credential to the sock. Even though this adds few more steps to add a credential to a sock (add credential, register tag to use), it avoids having multiple ways to tell sock which credential to use depending on the use case, which could be confusing to a new user.

As there can be multiple credentials of the same type added into the system, the (tag, type) -> credential buffer mapping is used by credman to identify which credential to use with which sock.

Testing procedure

Added unit tests under tests/unittests/tests-credman.

Issues/PRs references

See also #10897

miri64

Some initial review.

Regarding naming, what do you think about removing the credential in most function names. It seems redundant to me and makes the names somewhat clunky.

sys/include/net/credman.h

sys/net/credman/credman.c

sys/include/net/credman.h

sys/net/credman/credman.c

tests/unittests/tests-credman/tests-credman.c

jcarrano

If I get this right, this is a sort of in-memory index to user supplied buffers. That is, it creates a mapping of (tag, type) -> (buffer with key) and except the data definitions, nothing seems to be specific to credentials.

I think this may be over-specialized. If the user must remember a (32 bit) numerical tag and this does not copy the buffers, how is this different from just remembering a pointer to the buffer.

Also, why mix the storage and validation logic. If what is needed is an int->pointer mapping (thread safe and all) why not make that a module and implement the validation and retrieval on top of it.

The design of this does not allow for persisting the credentials in any way.

If credentials are known at compile time, why not use constfs. And if they are dynamically added in run-time, where will they be allocated?

sys/include/net/credman.h

sys/net/credman/credman.c

pokgak · 2019-05-28T18:21:16Z

I think this may be over-specialized. If the user must remember a (32 bit) numerical tag and this does not copy the buffers, how is this different from just remembering a pointer to the buffer.

To use DTLS, users must first tell the DTLS sock which credential needs to be used for the
encryption and decryption process. There are two use cases here, 1) the user has direct access to the
DTLS sock and can actually give the pointer of the credential buffer to the sock, 2) the user does not have access to the sock because it is located in a lower leveled library used by user application (e.g. gcoap + DTLS).

credman handles both of these by providing a unified way to pass credential to the sock. Even though this adds few more steps to add a credential to a sock (add credential, register tag to use), it avoids having multiple ways to tell sock which credential to use depending on the use case, which could be confusing to a new user.

Also, why mix the storage and validation logic. If what is needed is an int->pointer mapping (thread safe and all) why not make that a module and implement the validation and retrieval on top of it.

As there can be multiple credentials of the same type added into the system, the (tag, type) -> credential buffer mapping is used by credman to identify which credential to use with which sock.

If this module are intended for more general usage, mixing the storage and validation logic may not be good here but for (D)TLS it is easier to check it once when added if the credential is valid than to check it later every time the credential will be used in the (D)TLS libraries. Also, I don't know if such module (int->pointer mapping) is needed currently other than here.

The design of this does not allow for persisting the credentials in any way.

This is intentional. Copying the buffer into the system might use a larger amount of memory than needed e.g. credman reserves memory enough for a longer credential but the user only uses part of the buffer to save the credential. That is why I decided to only save pointer to the buffer.

If credentials are known at compile time, why not use constfs. And if they are dynamically added in run-time, where will they be allocated?

I'm not aware of constfs before this. Thanks for pointing that out. I'll look into it. For the dynamically added in run-time, the users have to supply their own buffer for the credentials.

[1]: sock_dtls is a wrapper around libraries that implements DTLS e.g. tinyDTLS, wolfSSL.

pokgak · 2019-07-15T15:48:42Z

@miri64 @jcarrano I updated my comment and the PR description. I hope it is easier to understand now. Also fixed requested changes.

As for constfs, credman may need to handle dynamically added credentials during runtime if something like 1 is implemented in the future. So I think it is better to not make the assumption that the credentials is known at compile-time.

pokgak · 2019-07-16T15:30:53Z

Rebased to current master.

sys/net/credman/credman.c

sys/include/net/credman.h

tests/unittests/tests-credman/tests-credman.c

tests/unittests/tests-credman/tests-credman.h

pokgak · 2019-07-18T09:20:05Z

Thanks for the review @miri64. I pushed some commits addressing the review comments.

miri64 · 2019-07-18T09:34:13Z

Had a look at your changes. Code design looks good now.

sys/net/credman/credman.c

miri64 · 2019-07-18T09:43:55Z

Reviewed: 2-code-design holds after latest change.

miri64 · 2019-07-18T10:28:49Z

I ran the unittests for

native (tests-credman and all) on a quite Ubuntu 19.04.

Operating System Environment
-----------------------------
       Operating System: "Ubuntu" "19.04 (Disco Dingo)"
                 Kernel: Linux 5.0.0-20-generic x86_64 x86_64

Installed compiler toolchains
-----------------------------
             native gcc: gcc (Ubuntu 8.3.0-6ubuntu1) 8.3.0
      arm-none-eabi-gcc: arm-none-eabi-gcc (GNU Tools for Arm Embedded Processors 8-2018-q4-major) 8.2.1 20181213 (release) [gcc-8-branch revision 267074]
                avr-gcc: avr-gcc (GCC) 5.4.0
       mips-mti-elf-gcc: missing
             msp430-gcc: msp430-gcc (GCC) 4.6.3 20120301 (mspgcc LTS 20120406 unpatched)
   riscv-none-embed-gcc: missing
   xtensa-esp32-elf-gcc: missing
   xtensa-lx106-elf-gcc: missing
                  clang: clang version 8.0.0-3 (tags/RELEASE_800/final)

Installed compiler libs
-----------------------
   arm-none-eabi-newlib: "3.0.0"
    mips-mti-elf-newlib: missing
riscv-none-embed-newlib: missing
xtensa-esp32-elf-newlib: missing
xtensa-lx106-elf-newlib: missing
               avr-libc: "2.0.0" ("20150208")

Installed development tools
---------------------------
                  cmake: cmake version 3.13.4
               cppcheck: missing
                doxygen: 1.8.13
                 flake8: 3.6.0 (mccabe: 0.6.1, pycodestyle: 2.4.0, pyflakes: 2.0.0) CPython 3.7.3 on Linux
                    git: git version 2.20.1
                   make: GNU Make 4.2.1
                openocd: Open On-Chip Debugger 0.10.0+dev-00910-g4dbcb1e7 (2019-06-17-16:24)
                 python: Python 2.7.16
                python2: Python 2.7.16
                python3: Python 3.7.3
             coccinelle: missing

iotlab-m3 (tests-credman and all, for the latter I had to merge in the current master, see tests/devfs: move tests-devfs out of unittests #11855)
samr21-xpro (only tests-credman, all` does not fit)
pba-d-01-kw2x (only tests-credman, all does not fit)
nrf52dk (tests-credman and all)
msba2 (tests-credman and all, for the latter I had to merge in the current master, see tests/devfs: move tests-devfs out of unittests #11855; still some [unrelated] tests fail, I confirmed that they also fail on master, see msba2: floating point tests fail #11861)

2019-07-18 12:15:10,577 - INFO # main(): This is RIOT! (Version: 2019.10-devel-91-gaa463-HEAD)
2019-07-18 12:15:18,423 - INFO # .................................................................................................................................................................................................................................................................
2019-07-18 12:15:18,427 - INFO # scanf_float_tests.test_f (tests/unittests/tests-scanf_float/tests-scanf_float.c 37) exp 0 was 1
2019-07-18 12:15:18,427 - INFO # .
2019-07-18 12:15:18,431 - INFO # scanf_float_tests.test_e (tests/unittests/tests-scanf_float/tests-scanf_float.c 38) exp 0 was 1
2019-07-18 12:15:18,431 - INFO # .
2019-07-18 12:15:18,458 - INFO # scanf_float_tests.test_E (tests/unittests/tests-scanf_float/tests-scanf_float.c 39) exp 0 was 1
2019-07-18 12:15:18,459 - INFO # .
2019-07-18 12:15:18,461 - INFO # scanf_float_tests.test_g (tests/unittests/tests-scanf_float/tests-scanf_float.c 40) exp 0 was 1
2019-07-18 12:15:18,461 - INFO # .
2019-07-18 12:15:18,463 - INFO # scanf_float_tests.test_sign (tests/unittests/tests-scanf_float/tests-scanf_float.c 45) exp 0 was 1
2019-07-18 12:15:18,463 - INFO # .
2019-07-18 12:15:18,465 - INFO # scanf_float_tests.test_scientific (tests/unittests/tests-scanf_float/tests-scanf_float.c 53) exp 0 was 1

z1 (only tests-credman, all does not fit)
arduino-mega2560 (only tests-credman, all does not fit)

All tests except the noted one on msba2 ran successful.

For msba2, nrf52dk, and arduino-mega2560 I used the nodes connected to https://ci-ilab.imp.fu-berlin.de, everything else I tested locally.

miri64 · 2019-07-18T10:29:50Z

Please squash. I will look at style and documentation later.

pokgak · 2019-07-18T11:13:41Z

Rebased and squashed.

miri64

My doc and style review

sys/include/net/credman.h

sys/net/credman/credman.c

tests/unittests/tests-credman/tests-credman.c

miri64 · 2019-07-19T07:41:52Z

Only #11564 (comment) left.

pokgak · 2019-07-19T09:59:55Z

@miri64 if you noticed, I removed CREDMAN_TAG_GLOBAL in this 2a0244a. This is my original idea to use with the second use case described in in this PR:

the user does not have access to the sock because it is located in a lower leveled library used by user application (e.g. gcoap + DTLS).

But while trying to explain it, I noticed that doing it that way may not be scalable if there is other application like gcoap using it in the future. Basically, my idea was a list of reserved credman_tag_t containing the tag used in each application. I already have a new idea how to implement it but maybe it is better to discuss it in a new PR as this PR is full of comments already.

That means, this PR (+ sock_dtls PR which is coming) will only enable support for the first use case:

the user has direct access to the DTLS sock and can actually give the pointer of the credential buffer to the sock

If you're okay with that, I'll proceed with rebasing and squashing the commits.

sys/net/credman/credman.c

miri64 · 2019-07-19T10:19:48Z

But while trying to explain it, I noticed that doing it that way may not be scalable if there is other application like gcoap using it in the future.

Good. Another reason why doc is important. Trying to explain stuff oftentimes helps me rethink stuff :-).

If you're okay with that, I'll proceed with rebasing and squashing the commits.

Yepp. In case it is needed later it can always be re-added.

I give this a final round of tests, and then I'll ACK [edit for the pedantics]when they succeed[/edit]. You may squash in the meantime.

miri64

I repeated my tests from #11564 (comment) for native, samr21-xpro, and z1. All succeeded. ACK

pokgak · 2019-07-19T10:31:20Z

Rebased and squashed.

miri64 · 2019-07-19T11:59:14Z

ACK holds

pokgak · 2019-07-19T12:01:05Z

Just noticed I messed up the commit a bit. Fixed and re-pushed the commits.

miri64 · 2019-07-19T12:20:16Z

I noticed it already when I ACK'd :-)

miri64 · 2019-07-19T14:38:44Z

And go!

benpicco · 2023-02-20T16:16:13Z

I'm trying to understand the idea behind the credman tag.
Shouldn't credentials be tied to a specific server? So do we need a separate database to map credman Tags to IP addresses / hostnames?

PeterKietzmann requested a review from miri64 May 23, 2019 06:32

PeterKietzmann added Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT labels May 23, 2019

miri64 requested changes May 24, 2019

View reviewed changes

jcarrano reviewed May 28, 2019

View reviewed changes

sys/include/net/credman.h Outdated Show resolved Hide resolved

sys/net/credman/credman.c Outdated Show resolved Hide resolved

pokgak force-pushed the credman branch from a2775ce to 4a6b86d Compare July 15, 2019 13:39

pokgak force-pushed the credman branch from 5072f2c to cfc8789 Compare July 16, 2019 15:30

miri64 requested changes Jul 17, 2019

View reviewed changes

miri64 added the Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines label Jul 17, 2019

miri64 added the Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines label Jul 18, 2019

miri64 reviewed Jul 18, 2019

View reviewed changes

sys/net/credman/credman.c Show resolved Hide resolved

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Jul 18, 2019

miri64 added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Jul 18, 2019

pokgak force-pushed the credman branch from 3c59483 to bf16ec9 Compare July 18, 2019 11:13

miri64 requested changes Jul 18, 2019

View reviewed changes

miri64 added Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Jul 19, 2019

miri64 reviewed Jul 19, 2019

View reviewed changes

sys/net/credman/credman.c Outdated Show resolved Hide resolved

miri64 approved these changes Jul 19, 2019

View reviewed changes

pokgak force-pushed the credman branch from d1fee82 to e48445a Compare July 19, 2019 10:30

Aiman Ismail added 2 commits July 19, 2019 13:56

sys/net: add credman (D)TLS credential manager

a1a6dcd

tests: add credman unit tests

0a3f12e

pokgak force-pushed the credman branch from e48445a to 0a3f12e Compare July 19, 2019 11:57

miri64 merged commit 7d2cb71 into RIOT-OS:master Jul 19, 2019

pokgak mentioned this pull request Jul 24, 2019

sys: new sock submodule for DTLS #11909

Merged

pokgak mentioned this pull request Aug 6, 2019

sock_dtls: a DTLS sock API #10897

Open

8 tasks

kb2ma added this to the Release 2019.10 milestone Sep 16, 2019

kfessel mentioned this pull request Feb 20, 2023

sock_dtls: move common code into sock_dtls_establish_session() #19142

Merged

2 tasks

sys: add credman (D)TLS credential management module #11564

sys: add credman (D)TLS credential management module #11564

Uh oh!

Conversation

pokgak commented May 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pokgak commented May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pokgak commented Jul 15, 2019

Uh oh!

pokgak commented Jul 16, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pokgak commented Jul 18, 2019

Uh oh!

miri64 commented Jul 18, 2019

Uh oh!

Uh oh!

miri64 commented Jul 18, 2019

Uh oh!

miri64 commented Jul 18, 2019

Uh oh!

miri64 commented Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pokgak commented Jul 18, 2019

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miri64 commented Jul 19, 2019

Uh oh!

pokgak commented Jul 19, 2019

Uh oh!

Uh oh!

miri64 commented Jul 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 left a comment

pokgak commented May 22, 2019 •

edited

Loading

pokgak commented May 28, 2019 •

edited

Loading

miri64 commented Jul 18, 2019 •

edited

Loading

miri64 commented Jul 19, 2019 •

edited

Loading

benpicco commented Feb 20, 2023 •

edited

Loading