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.

Sign up for GitHub

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

Jump to bottom

application: serial_lte_modem: Support DTLS connection ID #11244

Closed
wants to merge 1 commit into from
Closed

application: serial_lte_modem: Support DTLS connection ID #11244

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 53 additions & 2 deletions applications/serial_lte_modem/src/slm_at_udp_proxy.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include <string.h>
#include <zephyr/net/socket.h>
#include <zephyr/net/tls_credentials.h>
#include <nrf_socket.h>
#include "slm_util.h"
#include "slm_at_host.h"
#include "slm_at_udp_proxy.h"
Expand Down Expand Up @@ -47,6 +48,7 @@ static struct udp_proxy {
int sock; /* Socket descriptor. */
int family; /* Socket address family */
sec_tag_t sec_tag; /* Security tag of the credential */
int conn_id; /* Enable/Disable DTLS connection ID */
enum slm_udp_role role; /* Client or Server proxy */
union { /* remote host */
struct sockaddr_in remote; /* IPv4 host */
Expand Down Expand Up @@ -173,7 +175,6 @@ static int do_udp_client_connect(const char *url, uint16_t port)
ret = socket(proxy.family, SOCK_DGRAM, IPPROTO_UDP);
} else {
ret = socket(proxy.family, SOCK_DGRAM, IPPROTO_DTLS_1_2);

}
if (ret < 0) {
LOG_ERR("socket() failed: %d", -errno);
Expand All @@ -191,6 +192,25 @@ static int do_udp_client_connect(const char *url, uint16_t port)
ret = -errno;
goto cli_exit;
}

/* Best effort to enable/disable DTLS conn_id and load it */
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));
Comment on lines +197 to +201
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (ret) {
LOG_WRN("set conn id (%d) failed: %d", dtls_cid, -errno);
} 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");
}
}
Comment on lines +204 to +213
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@MarkusLassila MarkusLassila Jun 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

/* Connect to remote host */
Expand All @@ -210,6 +230,34 @@ static int do_udp_client_connect(const char *url, uint16_t port)
goto cli_exit;
}

/* Check conn_id status */
if (proxy.conn_id != 0) {
int status = -1;
int status_len = sizeof(status);

ret = nrf_getsockopt(proxy.sock, NRF_SOL_SECURE, NRF_SO_SEC_DTLS_CID_STATUS,
&status, &status_len);
if (ret) {
LOG_WRN("Unable to get DTLS connection ID status: %d", ret);
} else {
/**
* NRF_SO_SEC_DTLS_CID_STATUS_DISABLED 0
* NRF_SO_SEC_DTLS_CID_STATUS_DOWNLINK 1
* NRF_SO_SEC_DTLS_CID_STATUS_UPLINK 2
* NRF_SO_SEC_DTLS_CID_STATUS_BIDIRECTIONAL 3
*/
LOG_INF("DTLS connection ID status: %d", status);
if (status != NRF_SO_SEC_DTLS_CID_STATUS_DISABLED) {
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);
}
Comment on lines +251 to +256
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@junqingzou junqingzou Jun 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@MarkusLassila MarkusLassila Jun 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
}

udp_thread_id = k_thread_create(&udp_thread, udp_thread_stack,
K_THREAD_STACK_SIZEOF(udp_thread_stack),
udp_thread_func, NULL, NULL, NULL,
Expand Down Expand Up @@ -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>]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, it's optional.

* AT#XUDPCLI? READ command not supported
* AT#XUDPCLI=?
*/
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we can do that

}
}
proxy.family = (op == CLIENT_CONNECT) ? AF_INET : AF_INET6;
err = do_udp_client_connect(url, port);
Expand Down