gcoap: add coaps forward proxy #20454
Conversation
github-actions
bot
added
Area: network
mariemC
force-pushed
the
mariem/coap-coaps-forward-proxy
branch
from
3b88481
to
9bb0a63
Compare
kfessel
added
the
CI: ready for build
kfessel
changed the title
|
It is a really cool feature! We need two PreparationBefore flashing I did some changes to the Makefiles of ssize_t res = sock_dtls_recv(sock, &socket.ctx_dtls_session, _listen_buf,
sizeof(_listen_buf), 0);
if (res <= 0) {
DEBUG("gcoap: DTLS recv failure: %" PRIdSIZE "\n", res);
return;
}
diff --git a/examples/gcoap/Makefile b/examples/gcoap/Makefile
index 93bfc25e1b..c5573e5f8f 100644
--- a/examples/gcoap/Makefile
+++ b/examples/gcoap/Makefile
@@ -73,6 +73,15 @@ ifeq (1,$(USE_ZEP))
USEMODULE += socket_zep
endif
+# Uncomment the following 2 lines to specify static link lokal IPv6 address
+# this might be useful for testing, in cases where you cannot or do not want to
+# run a shell with ifconfig to get the real link lokal address.
+IPV6_STATIC_LLADDR ?= '"fe80::cafe:cafe:cafe:1"'
+CFLAGS += -DCONFIG_GNRC_IPV6_STATIC_LLADDR=$(IPV6_STATIC_LLADDR)
+# uncomment to not increment the last digit by the inderface number
+CFLAGS += -DCONFIG_GNRC_IPV6_STATIC_LLADDR_IS_FIXED=1
+CFLAGS += -DCONFIG_GNRC_NETIF_IPV6_ADDRS_NUMOF=4
+
include $(RIOTBASE)/Makefile.include
# For now this goes after the inclusion of Makefile.include so Kconfig symbols
diff --git a/examples/gcoap_dtls/Makefile b/examples/gcoap_dtls/Makefile
index 0776185964..75d86c2be7 100644
--- a/examples/gcoap_dtls/Makefile
+++ b/examples/gcoap_dtls/Makefile
@@ -78,6 +78,7 @@ endif
COAP_COAPS_PROXY_ENABLE ?= 0
ifeq (1,$(COAP_COAPS_PROXY_ENABLE))
USEMODULE += coap_coaps_proxy
+ USEMODULE += gcoap_forward_proxy
endif
# Instead of simulating an Ethernet connection, we can also simulate
@@ -90,6 +91,15 @@ ifeq (1,$(USE_ZEP))
USEMODULE += socket_zep
endif
+# Uncomment the following 2 lines to specify static link lokal IPv6 address
+# this might be useful for testing, in cases where you cannot or do not want to
+# run a shell with ifconfig to get the real link lokal address.
+IPV6_STATIC_LLADDR ?= '"fe80::cafe:cafe:cafe:1"'
+CFLAGS += -DCONFIG_GNRC_IPV6_STATIC_LLADDR=$(IPV6_STATIC_LLADDR)
+# uncomment to not increment the last digit by the inderface number
+CFLAGS += -DCONFIG_GNRC_IPV6_STATIC_LLADDR_IS_FIXED=1
+CFLAGS += -DCONFIG_GNRC_NETIF_IPV6_ADDRS_NUMOF=4
+
include $(RIOTBASE)/Makefile.include
# For now this goes after the inclusion of Makefile.include so Kconfig symbols
diff --git a/sys/include/net/gcoap.h b/sys/include/net/gcoap.h
index 8c63709491..77cf88224d 100644
--- a/sys/include/net/gcoap.h
+++ b/sys/include/net/gcoap.h
@@ -460,7 +460,7 @@ extern "C" {
* @brief Size of the buffer used to build a CoAP request or response
*/
#ifndef CONFIG_GCOAP_PDU_BUF_SIZE
-#define CONFIG_GCOAP_PDU_BUF_SIZE (128)
+#define CONFIG_GCOAP_PDU_BUF_SIZE (256)
#endif
/**Flash the CoAP Client
Flash the CoAP ProxyThe coap-coaps proxy will be
NetworkI put this script in #!/bin/sh
set -x
TAPNUM=${TAPNUM:-"0"}
TAPID=$(printf "%02d" ${TAPNUM})
TAPNAME=${TAPNAME:-"tap${TAPNUM}"}
TAPPREFIX=${TAPPREFIX:-"fd${TAPID}::/16"}
TAPSUBNET=${TAPSUBNET:-"fd${TAPID}::/64"}
TAPNEXTHOP=${TAPNEXTHOP:-"fe80::cafe:cafe:cafe:1"}
TAPADDRESS=${TAPADDRESS:-"fd${TAPID}::1/16"}
CURRENT_DIR="$(dirname $(readlink -f $0))"
RADVD_PIDFILE="/tmp/radvd-${TAPNAME}.pid"
tap_exists() {
ip link show ${TAPNAME} 2> /dev/null
}
tap_up() {
ip link set ${TAPNAME} up
}
tap_down() {
ip link set ${TAPNAME} down
}
tap_add_address() {
ip address add ${TAPADDRESS} dev ${TAPNAME}
}
tap_del_address() {
ip address del ${TAPADDRESS} dev ${TAPNAME}
}
tap_add_routes() {
ip route add ${TAPPREFIX} dev ${TAPNAME}
ip route add ${TAPSUBNET} via ${TAPNEXTHOP} dev ${TAPNAME}
}
tap_del_routes() {
ip route del ${TAPPREFIX} dev ${TAPNAME}
ip route del ${TAPSUBNET} dev ${TAPNAME}
}
tap_start_radvd() {
export TAP=${TAPNAME}
export PREFIX=${TAPSUBNET}
cat ${CURRENT_DIR}/radvd.conf | envsubst | radvd -C /dev/stdin -u ${SUDO_USER} -p ${RADVD_PIDFILE}
if [ $? -ne 0 ]; then
echo "radvd failed to start on ${TAPNAME} with prefix ${TAPSUBNET}"
exit 1
else
echo "radvd running on ${TAP}"
fi
}
tap_stop_radvd() {
if [ -f "${RADVD_PIDFILE}" ]; then
PID=$(cat ${RADVD_PIDFILE})
fi
if [ -n "${PID}" ]; then
kill ${PID}
rm ${RADVD_PIDFILE}
echo "radvd stopped"
fi
}
enable_forwarding () {
sysctl -w net.ipv6.conf.all.forwarding=1
}
if [ "$1" = "up" ]; then
enable_forwarding
if [ tap_exists ]; then
tap_up
tap_add_address
tap_add_routes
tap_start_radvd
fi
elif [ "$1" = "down" ]; then
if [ tap_exists ]; then
tap_stop_radvd
tap_del_routes
tap_del_address
tap_down
fi
else
echo "Usage: $0 [up|down]"
exit 1
fiAnd I copied Use the script to configure
Use the script to configure Meanwhile on Linux Test ReachabilityTry to ping Set a Proxy server for the ClientIn the terminal of Start a coaps serverI use the latest It is bound to the address of
Test the coap-coaps proxyNow |
|
There is an issue when no response is coming from the |
Yes the GCOAP_PDU _BUF_SIZE needs to be increased, I added that as a CFLAGS in the Makefile of |
There was a problem hiding this comment.
We do not need the coap_coaps_proxy module.
The port selection should not be done by any module constellation, because even if DTLS is used a user still may want to use unsecured coap.
Rather it should be selected by an input URI scheme.
For that, the client must be changed to not take <host>[:port] <path> but accept a full URI instead.
The communication to the proxy should also be determined by <scheme>://<proxy host>[:proxy port].
I would want split this out in another PR which you can rebase on.
There are also a couple of _free_client_ep() missing in the code.
I would want to fix it as well in another PR.
When no reponse is received and the client context has already been passed to the proxy thread it will never get deallocated. Thats why I would add a timestamp to the allocated client and when a new client needs to be allocated, old ones get deleted. This avoids race conditions between the coap and the proxy thread because only the coap thread needs to operate in the client context.
static client_ep_t *_allocate_client_ep(const sock_udp_ep_t *ep)
{
ztimer_now_t now = ztimer_now(ZTIMER_MSEC);
client_ep_t *cep;
for (cep = _client_eps;
cep < (_client_eps + CONFIG_GCOAP_REQ_WAITING_MAX);
cep++) {
if (_cep_in_use(cep) &&
(now - cep->t_allocated_ms) > CONFIG_GCOAP_FORWARD_PROXY_CLIENT_LIFETIME_MS) {
_free_client_ep(cep);
}
if (!_cep_in_use(cep)) {
DEBUG("gcoap_forward_proxy: allocating client context %p\n", (void *)cep);
_cep_set_req_etag_len(cep, 0);
memcpy(&cep->ep, ep, sizeof(*ep));
_cep_set_in_use(cep, now);
return cep;
}
}
return NULL;
}
|
In if (urip->port != 0) {
remote->port = urip->port;
}
else if (!strncmp(urip->scheme, "coaps", 5)) {
remote->port = COAPS_PORT;
}
else {
remote->port = COAP_PORT;
} |
Why is this PR then needed? #18107 should also support coap to coaps, coaps to coap and everything else (just communicate with the respective endpoint with the respective Proxy-Uri option). Sure, there is a structural bug in |
This all given that for CoAPS communication the proper credentials are in place, of course. |
I believe maybe the current approach for integrating a new module to handle CoAP/CoAPs may not be optimal. Instead, I could implement a configurable flag which offers a more straightforward solution ( I am open to suggestions as well). Mainly I had this PR to circumvent the issue of the gcoap blocked thread, by introducing a forward proxy thread. Furthermore, it's important to note that PR #18107 may encounter difficulties without addressing the gcoap blocked thread. and I think this could solve the issue. (regardless of coaps/coaps or coap/coaps, the new proxy thread is needed) I could only introduce the proxy thread as well |
As a workaround, that might be the more viable option, however, in the interest of keeping within resource constraints, an an asynchronous handling of the DTLS handshake, as also proposed by @benpicco in #18107, might be the more solid option.
|
Do you suggest then having the workaround with the proxy thread or rather going with async approach? |
If you have the capacity, I would prefer the async approach, but if you are short on time, I am fine with the workaround. |
Time-wise currently is tight, I could for now provide the proxy thread and then have a follow-up with the Async |
miri64
dismissed
their stale review
I am fine with the change and will rebase #18107 on top of this, once it is adapted for the proxy thread-only approach.
mariemC
force-pushed
the
mariem/coap-coaps-forward-proxy
branch
from
9bb0a63
to
a3ddaf2
Compare
| static char _forward_proxy_thread[PROXY_STACK_SIZE]; | ||
| static kernel_pid_t _forward_proxy_pid; |
There was a problem hiding this comment.
IMHO, this should be in an optional sub-module (e.g. forward_proxy_thread.c). For an unencrypted proxy the extra thread is not needed, so this memory usage should not be pulled in.
| /* DTLS communication is blocking the gcoap thread, | ||
| * therefore the communication should be handled in the proxy thread */ | ||
|
|
||
| msg_t msg = { .type = FORWARD_PROXY_MSG_SEND, | ||
| .content.ptr = client_ep }; | ||
| msg_try_send(&msg, _forward_proxy_pid); |
There was a problem hiding this comment.
You can then use
| /* DTLS communication is blocking the gcoap thread, | |
| * therefore the communication should be handled in the proxy thread */ | |
| msg_t msg = { .type = FORWARD_PROXY_MSG_SEND, | |
| .content.ptr = client_ep }; | |
| msg_try_send(&msg, _forward_proxy_pid); | |
| if (IS_USED(MODULE_GCOAP_FORWARD_PROXY_THREAD)) { | |
| /* WORKAROUND: DTLS communication is blocking the gcoap thread, | |
| * therefore the communication should be handled in the proxy thread */ | |
| msg_t msg = { .type = FORWARD_PROXY_MSG_SEND, | |
| .content.ptr = client_ep }; | |
| msg_try_send(&msg, _forward_proxy_pid); | |
| } | |
| else { | |
| len = gcoap_req_send((uint8_t *)pkt.hdr, len, | |
| &origin_server_ep, | |
| _forward_resp_handler, (void *)client_ep, | |
| GCOAP_SOCKET_TYPE_UNDEF); | |
| } |
What worries me a bit is that this changes the semantic of len, but this is a workaround after all and is already the case in your current code.
| _forward_proxy_pid = thread_create(_forward_proxy_thread, sizeof(_forward_proxy_thread), | ||
| THREAD_PRIORITY_MAIN - 1, | ||
| THREAD_CREATE_STACKTEST, _forward_proxy_thread_start, | ||
| NULL, "proxy"); |
There was a problem hiding this comment.
And
| _forward_proxy_pid = thread_create(_forward_proxy_thread, sizeof(_forward_proxy_thread), | |
| THREAD_PRIORITY_MAIN - 1, | |
| THREAD_CREATE_STACKTEST, _forward_proxy_thread_start, | |
| NULL, "proxy"); | |
| if IS_USED(MODULE_GCOAP_FORWARD_PROXY_THREAD) { | |
| _forward_proxy_pid = thread_create(_forward_proxy_thread, sizeof(_forward_proxy_thread), | |
| THREAD_PRIORITY_MAIN - 1, | |
| THREAD_CREATE_STACKTEST, _forward_proxy_thread_start, | |
| NULL, "proxy"); | |
| } |
(probably needs to be adapted for the fact that _forward_proxy_thread_start will become at least an extern)
mariemC
force-pushed
the
mariem/coap-coaps-forward-proxy
branch
from
a3ddaf2
to
4700081
Compare
mariemC
force-pushed
the
mariem/coap-coaps-forward-proxy
branch
from
4700081
to
4d8c173
Compare
| #include "debug.h" | ||
|
|
||
| static char _forward_proxy_thread[GCOAP_PROXY_STACK_SIZE]; | ||
| kernel_pid_t _forward_proxy_pid; |
There was a problem hiding this comment.
| kernel_pid_t _forward_proxy_pid; | |
| static kernel_pid_t _forward_proxy_pid; |
| @@ -250,7 +244,7 @@ static void _set_response_type(coap_pkt_t *pdu, uint8_t resp_type) | |||
| } | |||
| } | |||
|
|
|||
| static void _forward_resp_handler(const gcoap_request_memo_t *memo, | |||
| void _forward_resp_handler(const gcoap_request_memo_t *memo, | |||
There was a problem hiding this comment.
| void _forward_resp_handler(const gcoap_request_memo_t *memo, | |
| void _gcoap_forward_resp_handler(const gcoap_request_memo_t *memo, |
Just to be on the save side naming-wise. ;-)
| static char _forward_proxy_thread[GCOAP_PROXY_STACK_SIZE]; | ||
| kernel_pid_t _forward_proxy_pid; | ||
|
|
||
| extern void _forward_resp_handler(const gcoap_request_memo_t *memo, |
There was a problem hiding this comment.
| extern void _forward_resp_handler(const gcoap_request_memo_t *memo, | |
| extern void _gcoap_forward_resp_handler(const gcoap_request_memo_t *memo, |
| } | ||
| else { | ||
| len = gcoap_req_send((uint8_t *)client_ep->pdu.hdr, coap_get_total_len(&client_ep->pdu), | ||
| &client_ep->server_ep, _forward_resp_handler, client_ep, |
There was a problem hiding this comment.
| &client_ep->server_ep, _forward_resp_handler, client_ep, | |
| &client_ep->server_ep, _gcoap_forward_resp_handler, client_ep, |
| switch (msg.type) { | ||
| case GCOAP_FORWARD_PROXY_MSG_SEND: { | ||
| if (gcoap_req_send((uint8_t *)cep->pdu.hdr, coap_get_total_len(&cep->pdu), | ||
| &cep->server_ep, _forward_resp_handler, cep, |
There was a problem hiding this comment.
| &cep->server_ep, _forward_resp_handler, cep, | |
| &cep->server_ep, _gcoap_forward_resp_handler, cep, |
| if (gcoap_req_send((uint8_t *)cep->pdu.hdr, coap_get_total_len(&cep->pdu), | ||
| &cep->server_ep, _forward_resp_handler, cep, | ||
| GCOAP_SOCKET_TYPE_UNDEF) < 0) { | ||
| DEBUG_PUTS("_forward_proxy_thread_start: gcoap_req_send failed\n"); | ||
| } |
There was a problem hiding this comment.
This somewhat looks like a duplication of https://github.com/RIOT-OS/RIOT/pull/20454/files#diff-29f9b7660bbd785a3db5ce4ed3ebd3f5f396b1bd51193576e232a526b6808b85R445 now. Wouldn't it be better to expose a function that uses the client_ep_t pointer as an input instead of exposing the response handler?
There was a problem hiding this comment.
Yes either create a wrapping API for example gcoap_forward_proxy_req_send() or create an internal header file.
mariemC
force-pushed
the
mariem/coap-coaps-forward-proxy
branch
from
d0c6ef5
to
f2d4cd5
Compare
mariemC
force-pushed
the
mariem/coap-coaps-forward-proxy
branch
from
f2d4cd5
to
768ce5e
Compare
|
For testing the client should provide a coaps URI with GCOAPS port ( for that I tweaked the gcoap example) but I think this could be used from Fabian. #20554 |
The changes here look good for me. There is just the thing that @mariemC hard coded whether to use either coap or coaps to talk to the proxy. You can say that it is decided by ports 5683 or 5684 but also a non standard port could be used and that motivated me to create the liked PR. So I wanted to get that in first actually. If that is not feasible, then we would need to add the "port maps schema" assumption here. |
I think any notion of proxying was actually removed in this PR. #18107 uses the URI (and hopefully at some point CRI) schema to identify the protocol. If the port is different, that should also be encoded in the URI (and covered in #18107), so I am not sure what the problem is. All I need to do is rebase #18107 and make the module introduced here a dependency, once this PR is merged. |
|
Or is the problem that the gcoap example currently does not accept URIs as input for its commands (which, as far as I understand it is fixed by #20554; thanks for that btw!) |
Yes, Exactly. When you run the PR as is, the protocol towards the proxy is chosen implicitly here.: bytes_sent = gcoap_req_send(buf, len, remote, _resp_handler, NULL,
GCOAP_SOCKET_TYPE_UNDEF);followed by static int _tl_init_coap_socket(gcoap_socket_t *sock, gcoap_socket_type_t type)
{
switch (type) {
#if !IS_USED(MODULE_GCOAP_DTLS)
case GCOAP_SOCKET_TYPE_UNDEF:
#endif
case GCOAP_SOCKET_TYPE_UDP:
sock->type = GCOAP_SOCKET_TYPE_UDP;
sock->socket.udp = &_sock_udp;
break;
#if IS_USED(MODULE_GCOAP_DTLS)
case GCOAP_SOCKET_TYPE_UNDEF:
case GCOAP_SOCKET_TYPE_DTLS:
sock->type = GCOAP_SOCKET_TYPE_DTLS;
sock->socket.dtls = &_sock_dtls;
break;
#else
default:
return -1;
#endif
}
return 0;
}where the socket is selected based on used modules. The second place is here: if (_proxied) {
#ifdef SOCK_HAS_IPV6
char addrstr[IPV6_ADDR_MAX_STR_LEN];
#else
char addrstr[IPV4_ADDR_MAX_STR_LEN];
#endif
inet_ntop(remote.family, &remote.addr, addrstr, sizeof(addrstr));
if (remote.family == AF_INET6) {
uri_len = snprintf(proxy_uri, sizeof(proxy_uri), "coap://[%s]:%d%s",
addrstr, remote.port, uri);
}
else {
uri_len = snprintf(proxy_uri, sizeof(proxy_uri), "coap://%s:%d%s",
addrstr, remote.port, uri);
}
uri = proxy_uri;
}@mariemC has a hard coded |
(for future reference: can you please use code links, please? If you use an absolute SHA reference (which you can generate by hitting “y” in the GitHub code view) the code from the current repo will be shown; makes it easier to follow which code you are talking about) Yes this needs to be replaced by the corresponding SOCKET_TYPE from the schema, of course! |
IMHO, the proxy should be configured as a URI outright. I thought this was already the case, but apparently it isn't: Lines 245 to 272 in ee8569f Must have been my own app I was thinking of ^^" |
Contribution description
Testing procedure
I tested on native, so I created two taps: tap0 for the client which sets the remote proxy and tap1: for the proxy server which sets the route to the remote server.
coap-server-gnutls -A fdea:dbee:f::1 -k secretPSK -h Client_identity -v20
I have added fdea:dbee:f::1 to dev lo
Wireshark:
Issues/PRs references
I think this PR, could be helpful here: #18107 ( I didn't test coaps-coaps)
Mainly the issue is that gcoap thread gets stuck in the DTLS handshake therefore the handshake should happen into a different thread ( proxy thread)