application: serial_lte_modem: Support DTLS connection ID #11244

junqingzou · 2023-05-19T05:09:24Z

Support RFC9146 DTLS v1.2 Connection Identifier.

Requires lib_modem from NCS v2.4.0 and beyond.
Requires mfw-1.3.5 and beyond.

Supported in DTLS client proxy #XUDPCLI only.

junqingzou · 2023-05-19T05:11:09Z

Under testing with mfw_1.3.5 pre-release

NordicBuilder · 2023-05-19T05:12:58Z

Test specification

CI/Jenkins/NRF

Integration Platforms

CI/Jenkins/integration

Test Module	File based changes	Manually selected	West overwrite
test-fw-nrfconnect-nrf-iot_serial_lte_modem	X

Detailed information of selected test modules

Note: This message is automatically posted and updated by the CI

Support RFC9146 DTLS v1.2 Connection Identifier. - Requires lib_modem from NCS v2.4.0 and beyond. - Requires mfw-1.3.5 and beyond. Supported in DTLS client proxy #XUDPCLI only. Signed-off-by: Jun Qing Zou <jun.qing.zou@nordicsemi.no>

SeppoTakalo

Left my review remarks and proposals.

SeppoTakalo · 2023-05-25T07:17:57Z

applications/serial_lte_modem/src/slm_at_udp_proxy.c

@@ -481,7 +529,7 @@ int handle_at_udp_server(enum at_cmd_type cmd_type)
 }

 /**@brief handle AT#XUDPCLI commands
- * AT#XUDPCLI=<op>[,<url>,<port>[,<sec_tag>]
+ * AT#XUDPCLI=<op>[,<url>,<port>[,<sec_tag>,<connection_id>]


Suggested change

* AT#XUDPCLI=<op>[,<url>,<port>[,<sec_tag>,<connection_id>]

* AT#XUDPCLI=<op>[,<url>,<port>[,<sec_tag>[,<connection_id>]

Connection ID parameter seem optional.

You're right, it's optional.

SeppoTakalo · 2023-05-25T07:20:50Z

applications/serial_lte_modem/src/slm_at_udp_proxy.c

+ ret = nrf_setsockopt(proxy.sock, NRF_SOL_SECURE,
+ NRF_SO_SEC_DTLS_CONN_SAVE,
+ NULL, 0);
+ if (ret) {
+ LOG_WRN("Save DTLS connection failed: %d", ret);
+ }


Why do we blindly save the session if connection ID is enabled?
I would propose that we don't use that feature as it might run out of memory.

I think there will be only ONE session saved on modem side, as only SAVE/LOAD defined and no DELETE, so there should be no incremental memeory usage. SAVE after confirmation of CID active would only update the session data as I assumed. Finally if APP dones not SAVE then it could not LOAD, either.

I don't think that this SAVE/LOAD works as you expect. According to the documentation, SAVEd socket cannot be used, before it is LOADed. To me it seems like this should be used after we have transmitted the wanted data and are waiting for more data. Not sure how suitable this is for SLM.

https://developer.nordicsemi.com/nRF_Connect_SDK/doc/latest/nrfxlib/nrf_modem/doc/sockets.html:
...
Once the DTLS context is saved, the socket can’t be used before the DTLS context is loaded with [NRF_SO_SEC_DTLS_CONN_LOAD]
...
The option will fail with nrf_errno NRF_ENOMEM if the amount of saved connections exceeds four.
....

edit: Noticed that @SeppoTakalo addressed this as well, leaving this up.

SeppoTakalo · 2023-05-25T07:24:49Z

applications/serial_lte_modem/src/slm_at_udp_proxy.c

+ uint32_t dtls_cid = (proxy.conn_id != 0) ?
+ NRF_SO_SEC_DTLS_CID_ENABLED : NRF_SO_SEC_DTLS_CID_DISABLED;
+
+ ret = nrf_setsockopt(proxy.sock, NRF_SOL_SECURE, NRF_SO_SEC_DTLS_CID,
+ &dtls_cid, sizeof(dtls_cid));


Suggested change

uint32_t dtls_cid = (proxy.conn_id != 0) ?

NRF_SO_SEC_DTLS_CID_ENABLED : NRF_SO_SEC_DTLS_CID_DISABLED;

ret = nrf_setsockopt(proxy.sock, NRF_SOL_SECURE, NRF_SO_SEC_DTLS_CID,

&dtls_cid, sizeof(dtls_cid));

if (proxy.conn_id) {

ret = nrf_setsockopt(proxy.sock, NRF_SOL_SECURE, NRF_SO_SEC_DTLS_CID,

&proxy.conn_id, sizeof(proxy.conn_id));

}

I propose that we only set the socket option if it is other than zero.
Also require that AT parameter is 0, 1 or 2, so it is already same as what our socket API can take.

It is a bit awkward to set TLS_DTLS_CID_SUPPORTED, but will be allowed.
Then we'll need to differentiate TLS_DTLS_CID_SUPPORTED and TLS_DTLS_CID_ENABLED.

SeppoTakalo · 2023-05-25T07:26:15Z

applications/serial_lte_modem/src/slm_at_udp_proxy.c

+ } else if (proxy.conn_id != 0) {
+ ret = nrf_setsockopt(proxy.sock, NRF_SOL_SECURE,
+ NRF_SO_SEC_DTLS_CONN_LOAD,
+ NULL, 0);
+ if (ret) {
+ LOG_WRN("Load DTLS connection failed: %d", ret);
+ } else {
+ LOG_INF("DTLS connection resumed");
+ }
+ }


My proposal would be not to use to storing/loading, at least not in the first phase.

Either drop it, or have it as a separate option / AT command.

I don't think modem will automatically SAVE/LOAD CID so if APP does not do that, CID might not take effect for DTLS re-handshake.

Same CID should not be re-used in DTLS re-handshake.

https://datatracker.ietf.org/doc/html/rfc9146:

In DTLS 1.2, CIDs are exchanged at the beginning of the DTLS session only. There is no dedicated "CID update" message that allows new CIDs to be established mid-session, because DTLS 1.2 in general does not allow TLS 1.3-style post-handshake messages that do not themselves begin other handshakes. When a DTLS session is resumed or renegotiated, the "connection_id" extension is negotiated afresh.

Also:
https://developer.nordicsemi.com/nRF_Connect_SDK/doc/latest/nrfxlib/nrf_modem/doc/sockets.html

NRF_SO_SEC_DTLS_CONN_SAVE
Save DTLS connection. This option is write-only. This option require a DTLS v1.2 connection with renegotiation disabled.

edit: Noticed that @SeppoTakalo addressed this as well, leaving this up.

SeppoTakalo · 2023-05-25T07:26:58Z

applications/serial_lte_modem/src/slm_at_udp_proxy.c

@@ -516,6 +564,9 @@ int handle_at_udp_client(enum at_cmd_type cmd_type)
 proxy.sec_tag = INVALID_SEC_TAG;
 if (at_params_valid_count_get(&at_param_list) > 4) {
 at_params_unsigned_int_get(&at_param_list, 4, &proxy.sec_tag);
+ if (at_params_valid_count_get(&at_param_list) > 5) {
+ at_params_int_get(&at_param_list, 5, &proxy.conn_id);


We should verify here that conn_id is either 0, 1 or 2, so we can feed it directly to Socket API.

Yes we can do that

junqingzou · 2023-06-12T04:47:45Z

Left my review remarks and proposals.

Sorry to be late in replying due to local expo and tech tour.
This PR remains DNM as I cannot verify against a DTLS server that supports CID.
Openssl no, mbedTLS v3.3.0~ yes.

SeppoTakalo · 2023-06-12T06:41:25Z

Can you verify that there is no misunderstanding of connection ID SAVE/LOAD functionality, because I believe it works differently than what this code is using.
https://developer.nordicsemi.com/nRF_Connect_SDK/doc/latest/nrfxlib/nrf_modem/doc/sockets.html

My understanding of the documentation is that:

When you have a socket created, you can enable the CID functionality.
It might be enabled after you have connected. Depends on the server.
If you call CONN_SAVE, the existing DTLS session will be serialized into RAM and active connection is dropped to save some RAM from modem.
Socket is now unusable, until CONN_LOAD is called.
If you call close() on the socket, the saved CID will be removed. Nothing is permanently stored.

junqingzou · 2023-06-12T07:01:09Z

Can you verify that there is no misunderstanding of connection ID SAVE/LOAD functionality, because I believe it works differently than what this code is using. https://developer.nordicsemi.com/nRF_Connect_SDK/doc/latest/nrfxlib/nrf_modem/doc/sockets.html

My understanding of the documentation is that:

When you have a socket created, you can enable the CID functionality.

It might be enabled after you have connected. Depends on the server.
// It's enabled in Client Hello so during connect() for DTLS handshake I think (as seen in Wireshark)

If you call CONN_SAVE, the existing DTLS session will be serialized into RAM and active connection is dropped to save some RAM from modem.

Socket is now unusable, until CONN_LOAD is called.
// This was misunderstood by me. In my test, server does not responded with STATUS_ENABLED so I was not able to test out the socket unusable.

If you call close() on the socket, the saved CID will be removed. Nothing is permanently stored.

I get your point. I referred to lib_modem socket test code, but it does not really reflect the correct sequence of LOAD and SAVE.

MarkusLassila · 2023-06-13T07:38:49Z

applications/serial_lte_modem/src/slm_at_udp_proxy.c

+ ret = nrf_setsockopt(proxy.sock, NRF_SOL_SECURE,
+ NRF_SO_SEC_DTLS_CONN_SAVE,
+ NULL, 0);
+ if (ret) {
+ LOG_WRN("Save DTLS connection failed: %d", ret);
+ }


I don't think that this SAVE/LOAD works as you expect. According to the documentation, SAVEd socket cannot be used, before it is LOADed. To me it seems like this should be used after we have transmitted the wanted data and are waiting for more data. Not sure how suitable this is for SLM.

https://developer.nordicsemi.com/nRF_Connect_SDK/doc/latest/nrfxlib/nrf_modem/doc/sockets.html:
...
Once the DTLS context is saved, the socket can’t be used before the DTLS context is loaded with [NRF_SO_SEC_DTLS_CONN_LOAD]
...
The option will fail with nrf_errno NRF_ENOMEM if the amount of saved connections exceeds four.
....

edit: Noticed that @SeppoTakalo addressed this as well, leaving this up.

MarkusLassila · 2023-06-13T07:53:51Z

applications/serial_lte_modem/src/slm_at_udp_proxy.c

+ } else if (proxy.conn_id != 0) {
+ ret = nrf_setsockopt(proxy.sock, NRF_SOL_SECURE,
+ NRF_SO_SEC_DTLS_CONN_LOAD,
+ NULL, 0);
+ if (ret) {
+ LOG_WRN("Load DTLS connection failed: %d", ret);
+ } else {
+ LOG_INF("DTLS connection resumed");
+ }
+ }


Same CID should not be re-used in DTLS re-handshake.

https://datatracker.ietf.org/doc/html/rfc9146:

In DTLS 1.2, CIDs are exchanged at the beginning of the DTLS session only. There is no dedicated "CID update" message that allows new CIDs to be established mid-session, because DTLS 1.2 in general does not allow TLS 1.3-style post-handshake messages that do not themselves begin other handshakes. When a DTLS session is resumed or renegotiated, the "connection_id" extension is negotiated afresh.

Also:
https://developer.nordicsemi.com/nRF_Connect_SDK/doc/latest/nrfxlib/nrf_modem/doc/sockets.html

NRF_SO_SEC_DTLS_CONN_SAVE
Save DTLS connection. This option is write-only. This option require a DTLS v1.2 connection with renegotiation disabled.

edit: Noticed that @SeppoTakalo addressed this as well, leaving this up.

junqingzou · 2023-06-13T09:24:24Z

Thanks for the comments.
LOAD/SAVE has NOT been testable yet (even libmodem team) as mbedTLS v3.3.0 based DTLS server required.
Plan to move SAVE to do_udp_client_disconnect(), but unless testable the LOAD/SAVE scenario cannot be decided.

SeppoTakalo · 2023-06-13T09:36:27Z

@junqingzou Can you drop the SAVE/LOAD functionality.

Connection Identifier itself does not require it, it should be considered as a separate thing.
To enable CID, you only need to set one socket option. For example #11493

junqingzou · 2023-06-13T09:54:16Z

I can actually close this PR and keep the code as local test object.
If someone knows how to test with a known good DTLS server please teach me.
With openssl X.509-based DTLS is OK but server ignores extension 54 in Client_Hello.

junqingzou added the DNM label May 19, 2023

junqingzou requested review from SeppoTakalo, VTPeltoketo, MarkusLassila and rlubos as code owners May 19, 2023 05:09

github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label May 19, 2023

junqingzou force-pushed the pr_dtls_cid branch from 9c03702 to 9e95708 Compare May 22, 2023 02:50

SeppoTakalo requested changes May 25, 2023

View reviewed changes

MarkusLassila requested changes Jun 13, 2023

View reviewed changes

junqingzou closed this Jun 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

application: serial_lte_modem: Support DTLS connection ID #11244

application: serial_lte_modem: Support DTLS connection ID #11244

junqingzou commented May 19, 2023

junqingzou commented May 19, 2023

NordicBuilder commented May 19, 2023 •

edited

Loading

SeppoTakalo left a comment

SeppoTakalo May 25, 2023

junqingzou Jun 12, 2023

SeppoTakalo May 25, 2023

junqingzou Jun 12, 2023 •

edited

Loading

MarkusLassila Jun 13, 2023 •

edited

Loading

SeppoTakalo May 25, 2023

junqingzou Jun 12, 2023

SeppoTakalo May 25, 2023

junqingzou Jun 12, 2023

MarkusLassila Jun 13, 2023 •

edited

Loading

SeppoTakalo May 25, 2023

junqingzou Jun 12, 2023

junqingzou commented Jun 12, 2023 •

edited

Loading

SeppoTakalo commented Jun 12, 2023

junqingzou commented Jun 12, 2023

MarkusLassila Jun 13, 2023 •

edited

Loading

MarkusLassila Jun 13, 2023 •

edited

Loading

junqingzou commented Jun 13, 2023

SeppoTakalo commented Jun 13, 2023

junqingzou commented Jun 13, 2023

	* AT#XUDPCLI=<op>[,<url>,<port>[,<sec_tag>,<connection_id>]
	* AT#XUDPCLI=<op>[,<url>,<port>[,<sec_tag>[,<connection_id>]

application: serial_lte_modem: Support DTLS connection ID #11244

application: serial_lte_modem: Support DTLS connection ID #11244

Conversation

junqingzou commented May 19, 2023

junqingzou commented May 19, 2023

NordicBuilder commented May 19, 2023 • edited Loading

Test specification

SeppoTakalo left a 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

junqingzou Jun 12, 2023 • edited Loading

Choose a reason for hiding this comment

MarkusLassila Jun 13, 2023 • edited Loading

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

MarkusLassila Jun 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junqingzou commented Jun 12, 2023 • edited Loading

SeppoTakalo commented Jun 12, 2023

junqingzou commented Jun 12, 2023

MarkusLassila Jun 13, 2023 • edited Loading

Choose a reason for hiding this comment

MarkusLassila Jun 13, 2023 • edited Loading

Choose a reason for hiding this comment

junqingzou commented Jun 13, 2023

SeppoTakalo commented Jun 13, 2023

junqingzou commented Jun 13, 2023

NordicBuilder commented May 19, 2023 •

edited

Loading

junqingzou Jun 12, 2023 •

edited

Loading

MarkusLassila Jun 13, 2023 •

edited

Loading

MarkusLassila Jun 13, 2023 •

edited

Loading

junqingzou commented Jun 12, 2023 •

edited

Loading

MarkusLassila Jun 13, 2023 •

edited

Loading

MarkusLassila Jun 13, 2023 •

edited

Loading