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

sock_dtls: move common code into sock_dtls_establish_session() #19142

Merged
merged 3 commits into from
Feb 20, 2023
Merged

sock_dtls: move common code into sock_dtls_establish_session() #19142

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4676472
sys/net/sock_util: add sock_dtls_establish_session()
benpicco Jan 13, 2023
5134b5c
nanocoap_sock: make use of sock_dtls_establish_session()
benpicco Jan 13, 2023
df4ef80
sock_dodtls: make use of sock_dtls_establish_session()
benpicco Jan 13, 2023
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
14 changes: 0 additions & 14 deletions sys/include/net/nanocoap_sock.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,20 +145,6 @@
extern "C" {
#endif

/**
* @brief Timeout for CoAP over DTLS queries in milliseconds
*/
#ifndef CONFIG_NANOCOAP_SOCK_DTLS_TIMEOUT_MS
#define CONFIG_NANOCOAP_SOCK_DTLS_TIMEOUT_MS (1000U)
#endif

/**
* @brief Number of CoAP over DTLS handshake retries
*/
#ifndef CONFIG_NANOCOAP_SOCK_DTLS_RETRIES
#define CONFIG_NANOCOAP_SOCK_DTLS_RETRIES (2)
#endif

/**
* @brief Credman tag used for NanoCoAP
* Tag together with the credential type (PSK) needs to be unique
Expand Down
42 changes: 42 additions & 0 deletions sys/include/net/sock/util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@
#include "net/sock/udp.h"
#include "net/sock/tcp.h"

#ifdef MODULE_SOCK_DTLS
#include "net/credman.h"
#include "net/sock/dtls.h"
#endif

#ifdef __cplusplus
extern "C" {
#endif
Expand Down Expand Up @@ -268,6 +273,29 @@ static inline bool sock_udp_ep_equal(const sock_udp_ep_t *a,
return sock_tl_ep_equal(a, b);
}

#if defined(MODULE_SOCK_DTLS) || DOXYGEN
/**
* @brief Helper function to establish a DTLS connection
*
* @param[out] sock_udp Struct to store the underlying UDP socket
* @param[out] sock_dtls Struct for the actual DTLS socket
* @param[out] session Struct to store DTLS session information
* @param[in] tag Credential tag to use
* @param[in] local Local endpoint, must not be NULL
* @param[in] remote Server endpoint to connect to
* @param[in] work_buf Buffer used to negotiate connection
* @param[in] work_buf_len Size of @p work buf. Should be at least
* 160 bytes for AES_128_CCM_8 with PSK
*
* @return 0 on success
* @return negative error otherwise (see @ref sock_dtls_recv_aux)
*/
int sock_dtls_establish_session(sock_udp_t *sock_udp, sock_dtls_t *sock_dtls,
sock_dtls_session_t *session, credman_tag_t tag,
sock_udp_ep_t *local, const sock_udp_ep_t *remote,
void *work_buf, size_t work_buf_len);
#endif

/**
* @defgroup net_sock_util_conf SOCK utility functions compile configurations
* @ingroup net_sock_conf
Expand Down Expand Up @@ -297,6 +325,20 @@ static inline bool sock_udp_ep_equal(const sock_udp_ep_t *a,
#endif
/** @} */

/**
* @brief Timeout in milliseconds for sock_dtls_establish_session()
*/
#ifndef CONFIG_SOCK_DTLS_TIMEOUT_MS
#define CONFIG_SOCK_DTLS_TIMEOUT_MS (1000U)
#endif

/**
* @brief Number of DTLS handshake retries for sock_dtls_establish_session()
*/
#ifndef CONFIG_SOCK_DTLS_RETRIES
#define CONFIG_SOCK_DTLS_RETRIES (2)
#endif

#ifdef __cplusplus
}
#endif
Expand Down
81 changes: 6 additions & 75 deletions sys/net/application_layer/nanocoap/sock.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,92 +62,23 @@ typedef struct {
bool more;
} _block_ctx_t;

#if IS_USED(MODULE_NANOCOAP_DTLS)
int nanocoap_sock_dtls_connect(nanocoap_sock_t *sock, sock_udp_ep_t *local,
const sock_udp_ep_t *remote, credman_tag_t tag)
{
int res;
uint32_t timeout_ms = CONFIG_NANOCOAP_SOCK_DTLS_TIMEOUT_MS;
uint8_t retries = CONFIG_NANOCOAP_SOCK_DTLS_RETRIES;

bool auto_port = local->port == 0;
do {
if (auto_port) {
/* choose random ephemeral port, since DTLS requires a local port */
local->port = random_uint32_range(IANA_DYNAMIC_PORTRANGE_MIN,
IANA_DYNAMIC_PORTRANGE_MAX);
}
/* connect UDP socket */
res = nanocoap_sock_connect(sock, local, remote);
} while (auto_port && (res == -EADDRINUSE));

if (res < 0) {
return res;
}
#if IS_USED(MODULE_NANOCOAP_DTLS)
uint8_t buf[CONFIG_NANOCOAP_DTLS_HANDSHAKE_BUF_SIZE];

/* create DTLS socket on to of UDP socket */
res = sock_dtls_create(&sock->dtls, &sock->udp, tag,
SOCK_DTLS_1_2, SOCK_DTLS_CLIENT);
if (res < 0) {
DEBUG("Unable to create DTLS sock: %s\n", strerror(-res));
nanocoap_sock_close(sock);
return res;
}
sock->type = COAP_SOCKET_TYPE_DTLS;

while (1) {
uint8_t buf[CONFIG_NANOCOAP_DTLS_HANDSHAKE_BUF_SIZE];
mutex_t lock = MUTEX_INIT_LOCKED;
ztimer_t timeout;

/* unlock lock after timeout */
ztimer_mutex_unlock(ZTIMER_MSEC, &timeout, timeout_ms, &lock);

/* create DTLS session */
res = sock_dtls_session_init(&sock->dtls, remote, &sock->dtls_session);
if (res >= 0) {
/* handle handshake */
res = sock_dtls_recv(&sock->dtls, &sock->dtls_session, buf,
sizeof(buf), timeout_ms * US_PER_MS);
if (res == -SOCK_DTLS_HANDSHAKE) {
DEBUG("DTLS handshake successful\n");
ztimer_remove(ZTIMER_MSEC, &timeout);
return 0;
}
DEBUG("Unable to establish DTLS handshake: %s\n", strerror(-res));

} else {
DEBUG("Unable to initialize DTLS session: %s\n", strerror(-res));
}

sock_dtls_session_destroy(&sock->dtls, &sock->dtls_session);

if (retries--) {
/* wait for timeout to expire */
mutex_lock(&lock);
} else {
ztimer_remove(ZTIMER_MSEC, &timeout);
break;
}

/* see https://datatracker.ietf.org/doc/html/rfc6347#section-4.2.4.1 */
timeout_ms *= 2U;
}

nanocoap_sock_close(sock);
return res;
}
return sock_dtls_establish_session(&sock->udp, &sock->dtls, &sock->dtls_session,
tag, local, remote, buf, sizeof(buf));
#else
int nanocoap_sock_dtls_connect(nanocoap_sock_t *sock, const sock_udp_ep_t *local,
const sock_udp_ep_t *remote, credman_tag_t tag)
{
(void)sock;
(void)local;
(void)remote;
(void)tag;
return -ENOTSUP;
}
#endif
}

static int _get_error(const coap_pkt_t *pkt)
{
Expand Down Expand Up @@ -179,7 +110,7 @@ static int _sock_sendv(nanocoap_sock_t *sock, const iolist_t *snips)
#if IS_USED(MODULE_NANOCOAP_DTLS)
case COAP_SOCKET_TYPE_DTLS:
return sock_dtls_sendv(&sock->dtls, &sock->dtls_session, snips,
CONFIG_NANOCOAP_SOCK_DTLS_TIMEOUT_MS);
CONFIG_SOCK_DTLS_TIMEOUT_MS);
#endif
default:
assert(0);
Expand Down
64 changes: 6 additions & 58 deletions sys/net/application_layer/sock_dodtls/sock_dodtls.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include "net/iana/portrange.h"
#include "net/sock/dtls.h"
#include "net/sock/udp.h"
#include "net/sock/util.h"
#include "net/sock/dodtls.h"
#include "random.h"
#include "ztimer.h"
Expand Down Expand Up @@ -170,28 +171,16 @@ static void _close_session(credman_tag_t creds_tag, credman_type_t creds_type)
static int _connect_server(const sock_udp_ep_t *server,
const credman_credential_t *creds)
{
int res = -EADDRINUSE;
uint32_t start, try_start, timeout = SOCK_DODTLS_SESSION_RECV_TIMEOUT_MS;
int res;
sock_udp_ep_t local = SOCK_IPV6_EP_ANY;

/* server != NULL is checked in sock_dodtls_set_server() */
assert(creds != NULL);
mutex_lock(&_server_mutex);
while (res == -EADDRINUSE) {
/* choose random ephemeral port, since DTLS requires a local port */
local.port = IANA_DYNAMIC_PORTRANGE_MIN +
(random_uint32() % (IANA_SYSTEM_PORTRANGE_MAX - IANA_DYNAMIC_PORTRANGE_MIN));
if ((res = sock_udp_create(&_udp_sock, &local, server, 0)) < 0) {
if (res != -EADDRINUSE) {
DEBUG("Unable to create UDP sock\n");
goto exit;
}
}
}

benpicco marked this conversation as resolved.
Show resolved Hide resolved
res = credman_add(creds);
if (res < 0 && res != CREDMAN_EXIST) {
DEBUG("Unable to add credential to credman\n");
_close_session(creds->tag, creds->type);
switch (res) {
case CREDMAN_NO_SPACE:
res = -ENOSPC;
Expand All @@ -205,55 +194,14 @@ static int _connect_server(const sock_udp_ep_t *server,
}
goto exit;
}
if ((res = sock_dtls_create(&_dtls_sock, &_udp_sock, creds->tag,
SOCK_DTLS_1_2, SOCK_DTLS_CLIENT)) < 0) {
puts("Unable to create DTLS sock\n");
_close_session(creds->tag, creds->type);
goto exit;
}

start = _now_ms();
try_start = start;
while (((try_start = _now_ms()) - start) < SOCK_DODTLS_SESSION_TIMEOUT_MS) {
memset(&_server_session, 0, sizeof(_server_session));
if ((res = sock_dtls_session_init(&_dtls_sock, server,
&_server_session)) >= 0) {
uint32_t try_duration;

res = sock_dtls_recv(&_dtls_sock, &_server_session, _dns_buf,
sizeof(_dns_buf), timeout * US_PER_MS);
if (res == -SOCK_DTLS_HANDSHAKE) {
break;
}
DEBUG("Unable to establish DTLS handshake: %d (timeout: %luus)\n",
-res, (long unsigned)timeout * US_PER_MS);
sock_dtls_session_destroy(&_dtls_sock, &_server_session);
try_duration = _now_ms() - try_start;
if (try_duration < timeout) {
_sleep_ms(timeout - try_duration);
}
/* see https://datatracker.ietf.org/doc/html/rfc6347#section-4.2.4.1 */
timeout *= 2U;
}
else {
DEBUG("Unable to initialize DTLS session: %d\n", -res);
sock_dtls_session_destroy(&_dtls_sock, &_server_session);
}
}
if (res != -SOCK_DTLS_HANDSHAKE) {
res = -ETIMEDOUT;
_close_session(creds->tag, creds->type);
goto exit;
}
else {
res = 0;
}

res = sock_dtls_establish_session(&_udp_sock, &_dtls_sock, &_server_session,
fabian18 marked this conversation as resolved.
Show resolved Hide resolved
creds->tag, &local, server, _dns_buf,
sizeof(_dns_buf));
fabian18 marked this conversation as resolved.
Show resolved Hide resolved
_cred_type = creds->type;
_cred_tag = creds->tag;
_id = (uint16_t)(random_uint32() & 0xffff);
exit:
memset(_dns_buf, 0, sizeof(_dns_buf)); /* flush-out unencrypted data */
mutex_unlock(&_server_mutex);
return (res > 0) ? 0 : res;
Copy link
Contributor

Choose a reason for hiding this comment

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

@benpicco: the credman_delete(creds_tag, creds_type); got removed from line 184 maybe we need that in l. 206 if the connection could not be established since in that case it would not be closed -> that credman slot would be taken until reboot.

Copy link
Contributor

Choose a reason for hiding this comment

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

Kind of what I thought here.

Copy link
Contributor

Choose a reason for hiding this comment

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

missed that
but somehow i am still not fully convinced maybe the credman_delete in
_close_session is also wrong

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe that whole file should never call any credman function (but find or get) since the add should be done in another part of RIOT

Copy link
Contributor Author

Choose a reason for hiding this comment

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

credman is weird - it's true that the credentials are only stored for the lifetime of the session.
Since they are added in _connect_server() they should also be removed if _connect_server() fails - unless they were already added to credman before.

Same goes for _close_session().

Now what I don't get is why credman is needed at all.

Copy link
Contributor

@kfessel kfessel Feb 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

seems more like it is not used as intended

intention:
init:
add all your creeds to credman

connect:
use a cred by tag

done here:

connect
add this creed to credman
use the cred just added

but there where applications before that that did the credential handling themself and these are now add cred to credman
and that use the cred

if we use it as intended we would just have one place where all the creds are managed this would make the review of the creds on a device simpler)

seems like the move to cred man is just incomplete

Copy link
Contributor

Choose a reason for hiding this comment

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

#11564 (comment)

there is the reasoning why credman and not just a pointer to credentials

}
Expand Down
86 changes: 86 additions & 0 deletions sys/net/sock/sock_util.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,23 @@

#include "net/sock/udp.h"
#include "net/sock/util.h"
#include "net/iana/portrange.h"

#if defined(MODULE_DNS)
#include "net/dns.h"
#endif

#ifdef MODULE_RANDOM
fabian18 marked this conversation as resolved.
Show resolved Hide resolved
#include "random.h"
#endif

#ifdef MODULE_FMT
#include "fmt.h"
#endif

#define ENABLE_DEBUG 0
#include "debug.h"

#define PORT_STR_LEN (5)
#define NETIF_STR_LEN (5)

Expand Down Expand Up @@ -347,3 +355,81 @@ bool sock_tl_ep_equal(const struct _sock_tl_ep *a,
return false;
}
}

#if defined(MODULE_SOCK_DTLS)
int sock_dtls_establish_session(sock_udp_t *sock_udp, sock_dtls_t *sock_dtls,
sock_dtls_session_t *session, credman_tag_t tag,
sock_udp_ep_t *local, const sock_udp_ep_t *remote,
void *work_buf, size_t work_buf_len)
{
int res;
uint32_t timeout_ms = CONFIG_SOCK_DTLS_TIMEOUT_MS;
uint8_t retries = CONFIG_SOCK_DTLS_RETRIES;

bool auto_port = local->port == 0;
do {
if (auto_port) {
/* choose random ephemeral port, since DTLS requires a local port */
local->port = random_uint32_range(IANA_DYNAMIC_PORTRANGE_MIN,
IANA_DYNAMIC_PORTRANGE_MAX);
}
/* connect UDP socket */
res = sock_udp_create(sock_udp, local, remote, 0);
} while (auto_port && (res == -EADDRINUSE));

if (res < 0) {
return res;
}

/* create DTLS socket on to of UDP socket */
res = sock_dtls_create(sock_dtls, sock_udp, tag,
SOCK_DTLS_1_2, SOCK_DTLS_CLIENT);
if (res < 0) {
DEBUG("Unable to create DTLS sock: %s\n", strerror(-res));
sock_udp_close(sock_udp);
return res;
}

while (1) {
mutex_t lock = MUTEX_INIT_LOCKED;
ztimer_t timeout;

/* unlock lock after timeout */
ztimer_mutex_unlock(ZTIMER_MSEC, &timeout, timeout_ms, &lock);

/* create DTLS session */
res = sock_dtls_session_init(sock_dtls, remote, session);
if (res >= 0) {
/* handle handshake */
res = sock_dtls_recv(sock_dtls, session, work_buf,
work_buf_len, timeout_ms * US_PER_MS);
if (res == -SOCK_DTLS_HANDSHAKE) {
DEBUG("DTLS handshake successful\n");
ztimer_remove(ZTIMER_MSEC, &timeout);
return 0;
}
DEBUG("Unable to establish DTLS handshake: %s\n", strerror(-res));

} else {
DEBUG("Unable to initialize DTLS session: %s\n", strerror(-res));
}

sock_dtls_session_destroy(sock_dtls, session);

if (retries--) {
/* wait for timeout to expire */
mutex_lock(&lock);
} else {
ztimer_remove(ZTIMER_MSEC, &timeout);
break;
}

/* see https://datatracker.ietf.org/doc/html/rfc6347#section-4.2.4.1 */
timeout_ms *= 2U;
}

sock_dtls_close(sock_dtls);
sock_udp_close(sock_udp);
return res;
}
#endif