sock_dtls, dsm: Add possibility to determine the credentials that were used for a DTLS connection #19838

MarcelStenzel · 2023-07-19T12:21:54Z

Contribution description

I'm currently working on an implementation of the ACE-OAuth DTLS profile for RIOT. For this I need to know which credentials were used to authorize the client.
This PR adds that the DTLS socket sets the credentials of the session with the help of DSM.
To not interfere with any other flow I added a new session state that will be set if the session is stored as a new session by the socket. This is the case on server side.
On client side the session is already saved to DSM at the point of my addition. That's why the check to update the state in dsm_store() is changed to not overwrite the already set handshake state.

Testing procedure

I added a resource to the gcoap example with which the new functionality can be tested.
This function prints on success the credentials which were used for the DTLS connection. These credentials should be the same as the credentials added to credman before in the example.

Issues/PRs references

Fixes #19776

leandrolanzieri · 2023-08-09T13:49:21Z

sys/net/dsm/dsm.c

@@ -61,7 +61,7 @@ dsm_state_t dsm_store(sock_dtls_t *sock, sock_dtls_session_t *session,
    }

    prev_state = session_slot->state;
-    if (session_slot->state != SESSION_STATE_ESTABLISHED) {
+    if (new_state > session_slot->state) {


I don't think we should be comparing enums like this.

The reason I brought this in was to avoid that the state can be set back from SESSION_STATE_HANDSHAKE to SESSION_STATE_SOCK_INIT. This would occur on client side and mess up the gcoap flow in the handshake.
The only solution that comes to my mind right now would be another if nesting to prevent this exact case. Which doesn't seem to me like a nice solution.
Would there be something better to do this or is that solution ok?

So, AFAIU this is checking that the state transition is legal. Maybe the best would be to concentrate such logic in its own function, that way it could scale in case we need to add more states.

Something like this?

bool _legal_state_change(dsm_state_t new_state, dsm_state_t old_state) { switch(old_state) { case SESSION_STATE_NONE: return true; case SESSION_STATE_SOCK_INIT: return ((new_state == SESSION_STATE_HANDSHAKE) || (new_state == SESSION_STATE_ESTABLISHED)); case SESSION_STATE_HANDSHAKE: return new_state == SESSION_STATE_ESTABLISHED; case SESSION_STATE_ESTABLISHED: /* Fall through */ default: return false; } }

maybe the function could be _set_state and actually apply the change if legal.

sys/include/net/dsm.h

sys/net/dsm/dsm.c

leandrolanzieri · 2023-08-10T07:36:44Z

examples/gcoap/server.c

@@ -60,11 +60,17 @@ static ssize_t _encode_link(const coap_resource_t *resource, char *buf,
                            size_t maxlen, coap_link_encoder_ctx_t *context);
 static ssize_t _stats_handler(coap_pkt_t* pdu, uint8_t *buf, size_t len, coap_request_ctx_t *ctx);
 static ssize_t _riot_board_handler(coap_pkt_t* pdu, uint8_t *buf, size_t len, coap_request_ctx_t *ctx);
+#if IS_USED(MODULE_GCOAP_DTLS)


Another thing: I believe there's value in having the example code of how to use this feature, but seeing how much the example app is growing, do you think you can find a good spot in the docs to place it instead?

Sure, I can move the code somewhere else and maybe put a bit of additional explanation text. Should this go into the readme of the example or somewhere with CoAP, DTLS or DSM?

Hmm I think adding some section to gcoaps doc would be enough

Or maybe add another example app?
There is a lot of value in being able to compile the code on CI, if it's just hidden in the docs somewhere it might not even more anymore after some time without anyone noticing.

Later on this functionality will be used inside the example that I'm writing for the ACE DTLS implementation. Would that be enough (maybe + some addition in the doc?) or is a dedicated example better?

… were used for a DTLS connection

leandrolanzieri · 2023-08-11T07:24:21Z

examples/gcoap/server.c

+
+    return gcoap_response(pdu, buf, len, COAP_CODE_CREATED);
+#else
+    puts("gcoap_cli: Retrieving credentials is currently only implemented for PSK");


What is preventing us from doing this with ECC as well?

Just that I didn't get to it yet. And very likely I will need to add this later anyway.
Would it make sense to make this PR a draft for now and add it in here?

We can hold this until you implement ECC, as it looks it shouldn't be a lot of trouble IMO.

ECC is now implemented. I also have some code for the example that I tested this with. But I didn't push that since the discussion about the example is not resolved yet. It also makes the example a lot longer. Before, the example had nothing with any ECC key in it and I had to bring in some init function to be able to give different key pairs to the different instances.

Also, there is another issue that is not directly connected to this PR but to DTLS with ECC. Since the Makefile for tinydtls was changed it sets now the buffer size for DTLS. Now I can't, or at least I don't know how to, change the buffer size without changing it directly in the tinydtls Makefile. The solution to set CFLAGS += -DDTLS_MAX_BUF=512 in the application Makefile worked before but not anymore. The default 128 byte for the buffer is too small for the ECC handshake. The handshake fails at the Server Key Exchange message.

#19892 should allow to use the cflag again

Moves it from the dtls session to dsm session

The function checks if a state change is legal and sets the new state if that is the case. This replaces the state change check in dsm_store to make it more scalable.

leandrolanzieri · 2023-08-14T06:50:36Z

sys/net/dsm/dsm.c

+ * Checks if a state change is legal and changes it if that is the case.
+ * Otherwise the state will stay as it is.
+*/
+static void _set_state(dsm_state_t new_state, dsm_session_t *session_slot) {


I think you need to return some value to indicate that the state change was valid and done.

leandrolanzieri · 2023-08-14T06:52:04Z

sys/net/dsm/dsm.c

-    if (session_slot->state != SESSION_STATE_ESTABLISHED) {
-        session_slot->state = new_state;
-    }
+    _set_state(new_state, session_slot);


Once implemented, I'd suggest you add a check on the returned value to verify that the state change was valid.

For dsm_store it doesn't matter if the state was changed. Calling it without a valid state change is ok (e.g. even in the normal CoAP/DTLS flow it gets called withSESSION_STATE_HANDSHAKE while SESSION_STATE_ESTABLISHED is already set). So, I'm not sure what I would include in that check. It would just continue the same way with or without a changed state. That's why I didn't include any check in the first place.

leandrolanzieri · 2023-08-14T06:57:02Z

examples/gcoap/server.c

+
+    return gcoap_response(pdu, buf, len, COAP_CODE_CREATED);
+#else
+    puts("gcoap_cli: Retrieving credentials is currently only implemented for PSK");


We can hold this until you implement ECC, as it looks it shouldn't be a lot of trouble IMO.

The public key of the other peer and the own credman tag is saved in dsm during the DTLS handshake. With a new function an application can retrieve the public key of the other peer

The function resets now variables that are used for credential determination

19892: pkg/tinydtls: allow to set buffer size from application again r=leandrolanzieri a=leandrolanzieri ### Contribution description Currently the buffer size on tinydtls is set in its Makefile whenever `gcoap` module is present. This limits the ability of the user to override the value. This adds a pre-check of the `CFLAGS` to see if it was set before. ### Testing procedure Try setting `CFLAGS += -DDTLS_MAX_BUF=<some_value>` on `examples/gcoap_dtls`, you should be able to override the default value without errors. ### Issues/PRs references Reported in #19838 Co-authored-by: Leandro Lanzieri <[email protected]>

19886: cpu/efm32: fix DAC configuration r=benpicco a=gschorcht ### Contribution description The EFM32 MCU allows the reference voltage to be configured per DAC device, not per DAC channel. Also, the DAC reference voltage was defined in the configuration but not used anywhere. At the moment we have only defined one board (`stwstk6220a`) that uses the DAC, so changing the configuration interface shouldn't be critical. ### Testing procedure `tests/periph/dac` should still work for the `stwstk6220a` ``` BOARD=slwstk6220a make -j8 -C tests/periph/dac flash ``` I don't have a `stwstk6220a` board (EFM32 Series 0) so that I can't test it. I could only test it for the `sltb009a` board (EFM32 Series 1) with the change for VDAC in PR #19887. ### Issues/PRs references 19892: pkg/tinydtls: allow to set buffer size from application again r=benpicco a=leandrolanzieri ### Contribution description Currently the buffer size on tinydtls is set in its Makefile whenever `gcoap` module is present. This limits the ability of the user to override the value. This adds a pre-check of the `CFLAGS` to see if it was set before. ### Testing procedure Try setting `CFLAGS += -DDTLS_MAX_BUF=<some_value>` on `examples/gcoap_dtls`, you should be able to override the default value without errors. ### Issues/PRs references Reported in #19838 Co-authored-by: Gunar Schorcht <[email protected]> Co-authored-by: Leandro Lanzieri <[email protected]>

19892: pkg/tinydtls: allow to set buffer size from application again r=benpicco a=leandrolanzieri ### Contribution description Currently the buffer size on tinydtls is set in its Makefile whenever `gcoap` module is present. This limits the ability of the user to override the value. This adds a pre-check of the `CFLAGS` to see if it was set before. ### Testing procedure Try setting `CFLAGS += -DDTLS_MAX_BUF=<some_value>` on `examples/gcoap_dtls`, you should be able to override the default value without errors. ### Issues/PRs references Reported in #19838 Co-authored-by: Leandro Lanzieri <[email protected]>

19881: drivers/stmpe811: fix initialization if callback function parameter is NULL r=benpicco a=gschorcht ### Contribution description This PR fixes the `stmpe811` driver initialization if the callback function parameter `cb` is `NULL. This might be the case for example if the application uses the touch device in polling mode. If the interrupt pin is initialized if the callback function parameter `cb` is `NULL`, the driver crashes the first time an interrupt is triggered. Therefore, the INT pin must be initialized only if also the callback function parameter `cb` is not `NULL`. To be able to test the polling mode, this PR also includes a change of the `tests/drivers/stmpe811` application which introduces the environment variables `STMPE811_POLLING_MODE` `STMPE811_POLLING_PERIOD` and in the makefile. ### Testing procedure 1. Use a `stm32f429i-disc1` board and test it in polling mode: ``` STMPE811_POLLING_MODE=1 BOARD=stm32f429i-disc1 make -C tests/drivers/stmpe811 flash term ``` It should work as expected. ``` main(): This is RIOT! (Version: 2023.10-devel-119-g26e7a-drivers/stmpe811_fix_cb_null) STMPE811 test application +------------Initializing------------+ Initialization successful Pressed! X: 113, Y:135 X: 113, Y:135 X: 113, Y:136 Released! ``` 2. Checkout master branch and cerry-pick commit 691a5e6. The test application `tests/drivers/stmpe811` will crash once a touch event occur: ``` main(): This is RIOT! (Version: 2023.10-devel-117-g91441) STMPE811 test application +------------Initializing------------+ Initialization successful Stack pointer corrupted, reset to top of stack FSR/FAR: CFSR: 0x00020000 HFSR: 0x40000000 DFSR: 0x00000008 AFSR: 0x00000000 Misc EXC_RET: 0xfffffff1 *** RIOT kernel panic: HARD FAULT HANDLER ``` ### Issues/PRs references 19892: pkg/tinydtls: allow to set buffer size from application again r=benpicco a=leandrolanzieri ### Contribution description Currently the buffer size on tinydtls is set in its Makefile whenever `gcoap` module is present. This limits the ability of the user to override the value. This adds a pre-check of the `CFLAGS` to see if it was set before. ### Testing procedure Try setting `CFLAGS += -DDTLS_MAX_BUF=<some_value>` on `examples/gcoap_dtls`, you should be able to override the default value without errors. ### Issues/PRs references Reported in #19838 Co-authored-by: Gunar Schorcht <[email protected]> Co-authored-by: Leandro Lanzieri <[email protected]>

leandrolanzieri · 2023-11-04T18:33:29Z

sys/net/dsm/dsm.c

+#ifdef CONFIG_DTLS_ECC
+    unsigned char other_pub_x[DTLS_EC_KEY_SIZE];
+    unsigned char other_pub_y[DTLS_EC_KEY_SIZE];
+    size_t key_size;
+#endif


I understand why you need to save the public key, but I believe this is not the place to do so, the storage should be done somewhere else (ideally we already know the public key we should be getting #20048 )

MarcelStenzel requested review from miri64, haukepetersen, PeterKietzmann, cgundogan and leandrolanzieri as code owners July 19, 2023 12:21

github-actions bot added Area: network Area: Networking Area: pkg Area: External package ports Area: sys Area: System Area: examples Area: Example Applications labels Jul 19, 2023

leandrolanzieri requested review from benpicco and janosbrodbeck July 19, 2023 12:23

benpicco changed the title ~~Dtls cred determ~~ sock_dtls, dsm: Add possibility to determine the PSK credentials that were used for a DTLS connection Jul 19, 2023

MarcelStenzel changed the title ~~sock_dtls, dsm: Add possibility to determine the PSK credentials that were used for a DTLS connection~~ sock_dtls, dsm: Add functionality to determine credentials used for DTLS connection Jul 19, 2023

MarcelStenzel changed the title ~~sock_dtls, dsm: Add functionality to determine credentials used for DTLS connection~~ sock_dtls, dsm: Add possibility to determine the PSK credentials that were used for a DTLS connection Jul 19, 2023

leandrolanzieri requested changes Aug 9, 2023

View reviewed changes

leandrolanzieri reviewed Aug 10, 2023

View reviewed changes

MarcelStenzel added 3 commits August 10, 2023 13:52

sock_dtls, dsm: Add possibility to determine the PSK credentials that…

ef31483

… were used for a DTLS connection

examples/gcoap: Add example to determine DTLS PSK credentials

651a066

dsm: fix trailing whitespaces and comma error

aa7b3d1

MarcelStenzel force-pushed the dtls_cred_determ branch from 0e1f4f3 to aa7b3d1 Compare August 10, 2023 16:06

leandrolanzieri added the Type: new feature The issue requests / The PR implemements a new feature for RIOT label Aug 11, 2023

leandrolanzieri reviewed Aug 11, 2023

View reviewed changes

sock_dtls, dsm: Move saving of credential information

e34bca7

Moves it from the dtls session to dsm session

MarcelStenzel force-pushed the dtls_cred_determ branch from f5a1777 to d6da79c Compare August 11, 2023 11:24

dsm: Add function _set_state

bb458aa

The function checks if a state change is legal and sets the new state if that is the case. This replaces the state change check in dsm_store to make it more scalable.

MarcelStenzel force-pushed the dtls_cred_determ branch from d6da79c to bb458aa Compare August 11, 2023 11:33

leandrolanzieri reviewed Aug 14, 2023

View reviewed changes

MarcelStenzel added 2 commits August 20, 2023 18:44

sock_dtls, dsm: Add credential determination for ECC

efba176

The public key of the other peer and the own credman tag is saved in dsm during the DTLS handshake. With a new function an application can retrieve the public key of the other peer

dsm: extend function dsm_remove

85dbf24

The function resets now variables that are used for credential determination

MarcelStenzel changed the title ~~sock_dtls, dsm: Add possibility to determine the PSK credentials that were used for a DTLS connection~~ sock_dtls, dsm: Add possibility to determine the credentials that were used for a DTLS connection Aug 20, 2023

leandrolanzieri mentioned this pull request Aug 21, 2023

pkg/tinydtls: allow to set buffer size from application again #19892

Merged

leandrolanzieri mentioned this pull request Nov 4, 2023

net/sock_dtls: add public key verification #20048

Merged

leandrolanzieri reviewed Nov 4, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sock_dtls, dsm: Add possibility to determine the credentials that were used for a DTLS connection #19838

sock_dtls, dsm: Add possibility to determine the credentials that were used for a DTLS connection #19838

MarcelStenzel commented Jul 19, 2023

leandrolanzieri Aug 9, 2023

MarcelStenzel Aug 9, 2023

leandrolanzieri Aug 10, 2023

MarcelStenzel Aug 10, 2023

leandrolanzieri Aug 11, 2023

MarcelStenzel Aug 11, 2023

leandrolanzieri Aug 10, 2023

MarcelStenzel Aug 10, 2023

leandrolanzieri Aug 11, 2023

benpicco Aug 11, 2023

MarcelStenzel Aug 11, 2023

leandrolanzieri Aug 11, 2023

MarcelStenzel Aug 11, 2023

leandrolanzieri Aug 14, 2023

MarcelStenzel Aug 20, 2023

leandrolanzieri Aug 21, 2023

leandrolanzieri Aug 14, 2023

leandrolanzieri Aug 14, 2023

MarcelStenzel Aug 14, 2023

leandrolanzieri Aug 14, 2023

leandrolanzieri Nov 4, 2023

sock_dtls, dsm: Add possibility to determine the credentials that were used for a DTLS connection #19838

Are you sure you want to change the base?

sock_dtls, dsm: Add possibility to determine the credentials that were used for a DTLS connection #19838

Conversation

MarcelStenzel commented Jul 19, 2023

Contribution description

Testing procedure

Issues/PRs references

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment