gcoap: add simple forward-proxy #13790

cgundogan · 2020-04-01T23:44:26Z

Contribution description

This PR adds a simple forward-proxy to gcoap.
See #13637 for related info.

A forward proxy parses a Proxy-Uri option of incoming requests and constructs a new request with the proper Uri-Path and Uri-Query options. This newly created request is then sent to an endpoint that is specified in the Proxy-Uri option of the initial request.

The second method using the Proxy-Scheme option is not implemented.

Currently, gcoap does not support standalone acknowledgements. Therefore, the forward proxy is not able to stop the retransmissions on the client side. Hence, the client and the proxy will both do retransmissions for confirmable requests. I re-used the _find_req_memo() function to check for open requests with the same token and client_endpoint. The proxy ignores incoming requests, if the proxy is already doing retransmissions for them.

~~Depends on #13789~~
~~Depends on #14029~~

Testing procedure

use the gcoap example
Compile the forward-proxy with USEMODULE+=GCOAP_FORWARD_PROXY
Start three nodes (native is also okay).
On the client:

coap proxy set proxy-ipv6-address 5683

On the client:

coap get server-ipv6-address 5683 /cli/stats
coap put server-ipv6-address 5683 /cli/stats 42
coap get -c server-ipv6-address 5683 /cli/stats

Wireshark should reveal that each request and response packet traverses the proxy first

Issues/PRs references

kb2ma · 2020-04-03T11:26:27Z

Very cool, @cgundogan! This capability is really useful. I also agree this use case would benefit from support for separate ACKs. Initial comments below.

I agree that it makes sense to contain the forward proxy in a separate submodule. I'd like to think through how best to support such a thing. For example, can we accommodate the submodule's need to send a message without the addition of a public gcoap_dispatch() function?

chrysn · 2020-04-03T11:32:42Z

Do you see much value in supporting the Proxy-Uri option? My impression of that option is that it was primarily added to accommodate cross-proxying to protocols with incompatible URI schemes. Everyone else pretty please just use the destructured URIs. (I don't have objections to it being there, but it should be optional, and in the sense of educating CoAP users, shouldn't be the first thing they see when they look for a RIOT proxy).

The look-into-memos thing looks like a good workaround until we send separate ACKs.

(That's on the description; comments on the code to follow.)

kb2ma · 2020-04-03T12:01:37Z

Do you see much value in supporting the Proxy-Uri option?

Yes, I work for a company with a significant number of incompatible devices. I expect this mechanism will allow us to evolve toward use of CoAP by putting a compatible device in front of the incompatible ones.

Everyone else pretty please just use the destructured URIs.

By "destructured" do you mean use of the individual Proxy-Scheme and Uri-* options?

in the sense of educating CoAP users, shouldn't be the first thing they see when they look for a RIOT proxy

I am not an expert in the proxy world and would appreciate it if you would summarize your thoughts on priorities in an RFC issue.

Also @cgundogan, can you share your motivation for this work?

cgundogan · 2020-04-03T12:12:53Z

Also @cgundogan, can you share your motivation for this work?

that's more or less for research purposes :)

Regarding the forward-proxy: I actually was planning to also work on a reverse-proxy. This would make the CoAP clients unaware of the whole proxying process. I just wanted to have the forward-proxy out there to get an early feedback from you more knowledgeable coap experts.

chrysn · 2020-04-03T12:13:14Z

compatible device in front of the incompatible ones

Fine with me -- but those good CoAP citizens should IMO produce easy to parse messages (which is is more a comment for #13637). Mind you that parsing URIs is something that often goes wrong.

destructured URIs?

Yes. It makes little difference difference on the wire, but allows proxies to operate without a URI parser. Especially the most constrained proxies (the term used there is "intermediaries") will probably not support Proxy-Uri.

summarizing your thoughts

I've done so in the unofficial CoAP FAQ -- just don't take it as a third party opinion, because most of that is things I've written. The items in there might eventually wind up in a corrections-and-clarifications document that has been discussed in the WG, but that'll take some time.

cgundogan · 2020-04-03T12:49:28Z

the last rebase removed the now superfluous commit of #13789

cgundogan · 2020-04-03T12:50:50Z

@kb2ma @chrysn the two nanocoap related commits can actually be reviewed separately. If you wish, then I could open a separate PR for that.

kb2ma · 2020-04-05T13:22:33Z

I could open a separate PR for that.

Thanks for going ahead with #13818. It's next on my list to review.

sys/net/application_layer/gcoap/forward_proxy/forward_proxy.c

sys/include/net/gcoap.h

chrysn · 2020-04-07T09:38:55Z

sys/include/net/gcoap.h

+ * @param[in] buf           Buffer containing the PDU
+ * @param[in] len           Length of the buffer
+ * @param[in] remote        Destination for the packet
+ * @param[in] report_memo   Pointer to the memo used for sending the request


How long is that pointer valid, especially when the callback is NULL?

Agree that memo lifetime can be very short. However, if the memo context pointer references the client endpoint, then we don't need gcoap_req_send_report(). This is an example of the value of using the memo context pointer.

sys/include/net/gcoap/forward_proxy.h

chrysn · 2020-04-07T09:49:02Z

sys/include/net/nanocoap.h

+ * @return      -ENOENT if Proxy-Uri option not found
+ * @return      -EINVAL if Proxy-Uri option cannot be parsed
+ */
+static inline ssize_t coap_get_proxy_uri(const coap_pkt_t *pkt, char **target)


Probably more a general nanocoap than this-patch-in-particular question: Do we really want a per-option convenience function?

chrysn · 2020-04-07T09:55:38Z

sys/net/application_layer/gcoap/forward_proxy/forward_proxy.c

+static uint8_t proxy_req_buf[CONFIG_GCOAP_PDU_BUF_SIZE];
+static uint8_t scratch[48];
+
+static bool _parse_endpoint(sock_udp_ep_t *remote,


This is something I can easily see becoming public (or publicly accessible as "create a coap request to that URI"), especially after the discussion in the CLI client on having coap get coap://[fe80::99%2]/foo, and because I have hopes that uri_parser_result can become populatable by CoRAL applications.

I'd move some of it into the URI parsing, but for reasons unrelated to this PR, so that's no objection for here but an item from #13827.

Agree. @cgundogan and I have discussed use by the gcoap and wakaama examples. I also agree that generation of an endpoint is not CoAP specific.

I have a similar piece of code in #16705 which I can spin out as a public function. The only question I have is where to put it ^^. Doesn't seem to fit either well uri_parser or sock_util.

We could try to refactor this as soon as the DoC client leaves the draft status (:

chrysn · 2020-04-07T10:46:35Z

sys/net/application_layer/gcoap/forward_proxy/forward_proxy.c

+    (void) remote; /* this is the origin server */
+
+    /* forward the response packet as-is to the client */
+    gcoap_dispatch((uint8_t *)pdu->hdr,


There will need to be some compensation for mismatching token lengths (see the other comment about the empty token).

chrysn · 2020-04-07T10:48:47Z

sys/net/application_layer/gcoap/forward_proxy/forward_proxy.c

+        return -EINVAL;
+    }
+
+    if (urip->query) {


Adding Uri-Path and query will need to happen in the options copying step where that option number is reached; otherwise, coap_opt_add_opaque will fail when the client sends an ETag, for example.

chrysn · 2020-04-07T10:50:06Z

sys/net/application_layer/gcoap/forward_proxy/forward_proxy.c

+    uint8_t *value;
+    for (int i = 0; i < client_pkt->options_len; i++) {
+        ssize_t optlen = coap_opt_get_next(client_pkt, &opt, &value, !i);
+        if (optlen >= 0) {


Encountering a non-safe-to-forward option needs to result in an error response here.

chrysn · 2020-04-07T10:51:31Z

sys/include/net/gcoap.h

+ * @param[in] remote        Destination for the packet
+ * @param[in] report_memo   Pointer to the memo used for sending the request
+ * @param[in] resp_handler  Callback when response received, may be NULL
+ * @param[in] context       User defined context passed to the response handler


As the memo is return, it might make sense not to have setting the context here in the first place (especially if any of the unions mentioned above is used).

kb2ma · 2020-04-10T11:31:57Z

FWIW, I am thinking this through. A proxy is kind of its own world yet also has to operate within the larger context of gcoap. With fundamental issues like this, I find it helps to think through the issues a few times to layer on a good understanding.

One thing is for sure. This PR would benefit from an implementation of separate response handling for a confirmable request. It's an incremental step with the existing infrastructure, and on my short list to do.

kb2ma

I wonder if there is a more integrated way to implement this work within gcoap. Conceptually, a forwarding proxy is a specialized listener on the server side. Let's call it a proxy listener. When a message arrives with a Proxy-Uri option, the proxy listener forwards it to the origin server. And when a response arrives from the origin server, the proxy listener forwards it to the requesting client.

Could we define a proxy listener as an application listener (gcoap_listener_t)? This approach means we are developing an API for extending the concept of a listener that can be used by other kinds of proxies.

Think about the code currently at the top of _handle_req(). What if in _find_resource(), we extend how a match is done. In the listener struct, add a function pointer -- request_matcher. This extends how we use coap_match_path() but also includes the coap_pkt_t. By default a listener uses coap_match_path(), as is done currently. But a proxy listener would implement a custom function to also check for a proxy option in the coap_pkt_t. If no proxy option, _find_resource() will move on to the next listener. Also, we would have to add a gcoap_add_listener_pos() which would allow insertion of the proxy first in the list of listeners.

One other request regardless of the listener idea above. I'd like to limit the common public API, and gcoap_dispatch() and gcoap_find_req_memo() are only needed in this proxy context. So how about a gcoap_proxy_...() (or something like that) header for these functions. The implementation would just be minimal; mostly one liners to call the internal implementation. In this way, the current gcoap internal uses of sock_udp_send() for example also could remain as they are rather than use gcoap_dispatch().

cgundogan · 2020-05-07T14:06:19Z

I wonder if there is a more integrated way to implement this work within gcoap. Conceptually, a forwarding proxy is a specialized listener on the server side. Let's call it a proxy listener. When a message arrives with a Proxy-Uri option, the proxy listener forwards it to the origin server. And when a response arrives from the origin server, the proxy listener forwards it to the requesting client.

@kb2ma @chrysn I have updated the integration to address both your comments: the proxy is now an application and requires almost no modifications to gcoap.

The only diff I had to add is here. I wish I could also get rid of the modified call to resource->handler(), but I could not come up with a cleaner solution to pass the remote to the proxy handler. I wonder if passing the memo or the endpoint to the application would be something we want per default?

What's still missing is the implementation of Proxy-Scheme and the consistency check for non-safe-option when copying the options. If we can acknowledge the current way we are moving, then I can add those features (I am aiming to keep the diff as small as possible for review).

miri64 · 2021-11-11T17:36:17Z

Just for context: I was asking @cgundogan to rebase the PR to have it ready for some experiments of mine. AFAIK, there are still some building blocks missing, to get it in a mergeable state. Did I understood you correctly there, @cgundogan? Should we mark this PR WIP?

benpicco · 2021-12-03T12:10:02Z

What building blocks are missing here though?
And if you are already using this code, doesn't that imply some readiness 😉?

sys/net/application_layer/gcoap/forward_proxy.c

miri64

I basically tested this over the course of my DoC experiments extensively. I have one comment. Once that is addressed, I think we can merge this.

sys/net/application_layer/gcoap/forward_proxy.c

sys/include/net/gcoap/forward_proxy.h

sys/net/application_layer/gcoap/forward_proxy.c

miri64 · 2022-03-11T12:02:57Z

Apart from #13790 (comment), please squash.

sys/net/application_layer/gcoap/forward_proxy.c

miri64

Please squash.

As said, I ran extensive experiments with this proxy implementation (on 2022.01). API changes to gCoAP since 2022.01 were addressed and further should be cought by Murdock.

Extensions like Proxy-Scheme, reverse proxies, or DTLS support can be added as a follow-up.

cgundogan · 2022-03-11T13:55:10Z

Thanks @miri64 and others for this comprehensive review! Squashed the current state.

miri64 · 2022-03-11T20:06:59Z

🚀

cgundogan added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking Area: CoAP Area: Constrained Application Protocol implementations labels Apr 1, 2020

cgundogan requested review from chrysn, miri64, kb2ma and leandrolanzieri April 1, 2020 23:44

cgundogan requested review from haukepetersen and PeterKietzmann as code owners April 1, 2020 23:44

cgundogan force-pushed the coap_proxy_server branch from 83b41f8 to 39faacf Compare April 3, 2020 12:48

cgundogan mentioned this pull request Apr 5, 2020

nanocoap: allow coap_opt_add_string() for non-zero-terminated strings #13818

Merged

chrysn mentioned this pull request Apr 7, 2020

Align uri_parser's output with CRIs #13827

Open

chrysn reviewed Apr 7, 2020

View reviewed changes

kb2ma reviewed Apr 11, 2020

View reviewed changes

cgundogan mentioned this pull request Apr 17, 2020

nanocoap: cache for CoAP responses #13889

Merged

cgundogan force-pushed the coap_proxy_server branch from 39faacf to 86227c7 Compare April 23, 2020 07:04

cgundogan mentioned this pull request Apr 23, 2020

nanocoap: add coap_get_proxy_uri() #13935

Merged

cgundogan mentioned this pull request May 5, 2020

gcoap: add a handler for request matching #14029

Merged

cgundogan force-pushed the coap_proxy_server branch 2 times, most recently from dc4d97d to d4794ef Compare May 7, 2020 13:56

github-actions bot removed the Area: build system Area: Build system label Nov 11, 2021

cgundogan force-pushed the coap_proxy_server branch from e8eb403 to 1b470ef Compare November 11, 2021 16:31

github-actions bot added the Area: build system Area: Build system label Nov 11, 2021

cgundogan force-pushed the coap_proxy_server branch from 1b470ef to 6d0d7f0 Compare November 11, 2021 16:32

benpicco requested review from janosbrodbeck and benpicco November 11, 2021 16:32

cgundogan force-pushed the coap_proxy_server branch from 6d0d7f0 to 4111493 Compare November 11, 2021 17:56

miri64 reviewed Dec 10, 2021

View reviewed changes

sys/net/application_layer/gcoap/forward_proxy.c Outdated Show resolved Hide resolved

miri64 reviewed Jan 1, 2022

View reviewed changes

sys/net/application_layer/gcoap/forward_proxy.c Outdated Show resolved Hide resolved

cgundogan force-pushed the coap_proxy_server branch from a48fe14 to a7b8ddd Compare March 11, 2022 11:04