gcoap: allow proxied client requests by cgundogan · Pull Request #13637 · RIOT-OS/RIOT

cgundogan · 2020-03-15T23:56:10Z

Contribution description

This patch extends the gcoap_req_init() function to understand client-side proxied RFC7252#section-5.10.2 requests. A request to a proxy does not include an URI_PATH option, but a PROXY_URI option. A Proxy-URI is an absolute uri that contains the scheme, host part, and the path of the origin server.

(Proxy-Scheme is ignored for now)

The gcoap example is extended with a new shell command:

coap proxy set <ipv6addr> <port>
coap proxy unset

Once a proxy is set, all coap packets are sent to the proxy.

Example:

coap set proxy fe80::9032:29ff:fe2b:4b8%6 5683
coap info
coap get 2001:db8::1 5683 /my/resource

will yield a CoAP GET packet directed to the proxy address that contains the option

Proxy-URI: coap://[2001:db8::1 5683]:5683/my/resource

Testing procedure

Set a proxy via the shell command
Look at the output of wireshark
Packet should contain a Proxy-URI option, instead of a URI-PATH option

Issues/PRs references

n/a

cgundogan · 2020-03-16T10:07:36Z

The not-so-little change to the gcoap example mostly stems from the fact that Proxy-URI expects an absolute URI of the form scheme://host:port/resources. But our current shell command implementation uses different positional arguments for host, port, and the relative resource path.

Wouldn't it be much more consistent (also with our tools) to use the absolute path here? E.g.,

coap get coap://[2001:db8::1]:5683/.well-known/core

instead of

coap get 2001:db8::1 5683 /.well-known/core

kb2ma · 2020-03-18T13:06:34Z

I agree it is useful to support a client sending to a forward proxy. Let's start with my comment below on gcoap_req_init().

kb2ma · 2020-03-18T13:14:12Z

sys/net/application_layer/gcoap/gcoap.c


 int gcoap_req_init(coap_pkt_t *pdu, uint8_t *buf, size_t len,
-                   unsigned code, const char *path)
+                   unsigned code, const char *path, bool proxied)


Are you sure we want to specify use of a proxy here? The major issue I see is that the option number for Proxy-Uri is 35. If we add this option here, the user will be unable to specify use of an option with a lesser number. These lesser numbers include a large number of options, including Content-Format.

Why not just require the user to specify use of a forward proxy with coap_opt_add_string() themselves? This allows them to use either Proxy-Uri or Proxy-Scheme.

hmm, this would mean:

no changes in gcoap (maybe a little bit on the documentation on how to achieve proxied requests)

put more logic into the gcoap example

or did I misunderstand your comment?

Yes, right on both points -- and point to the example from the documentation.

You also could add an inline convenience function, coap_opt_add_proxy_uri(), similar to coap_opt_put_location_path().

You also could add an inline convenience function, coap_opt_add_proxy_uri(), similar to coap_opt_put_location_path().

I think that approach makes sense in this case. +1

kb2ma · 2020-03-19T11:52:46Z

Please add "proxy" to the printf usage statement on the next to the last line of gcoap_cli_cmd().

kb2ma

Aside from inline comments, I agree with how you organized the CLI for proxy. Thanks for the contribution!

examples/gcoap/gcoap_cli.c

kb2ma · 2020-03-20T13:27:45Z

Wouldn't it be much more consistent (also with our tools) to use the absolute path here? E.g.,
coap get coap://[2001:db8::1]:5683/.well-known/core
instead of
coap get 2001:db8::1 5683 /.well-known/core

I am neutral on this change to the CLI. It would make parsing the full command line easier because it is a single string, and we could use the scheme to infer use of a default port. On the other hand, parsing scheme + destination + port + path from a single string is more complicated than use of snprintf() to assemble them from the pieces for the Proxy-Uri option, as you did here.

IMO, URI parsing belongs at the system level though, in a separate PR. There should be One True Way to do it. For example, the Wakaama example needs it for the server URI.

@leandrolanzieri, @miri64 -- any opinion on use of a URI, especially as a default approach for CLI tools?

miri64 · 2020-03-20T13:34:39Z

@leandrolanzieri, @miri64 -- any opinion on use of a URI, especially as a default approach for CLI tools?

I would prefer URIs as well. I wanted to propose it a long time ago actually, but ended up very low on my list of priorities.

examples/gcoap/gcoap_cli.c

cgundogan · 2020-03-24T12:08:23Z

IMO, URI parsing belongs at the system level though, in a separate PR. There should be One True Way to do it. For example, the Wakaama example needs it for the server URI.

I would prefer URIs as well. I wanted to propose it a long time ago actually, but ended up very low on my list of priorities.

alright. Then let's go for now with the currently proposed solution and in a separate Pr we could think about using absolute URIs.

sys/net/application_layer/nanocoap/nanocoap.c

kb2ma

Overall this looks good except for the inline comments. I plan to test over the next day or so.

sys/include/net/gcoap.h

cgundogan · 2020-03-25T15:32:29Z

@kb2ma addressed your comments. Thanks for the doc enhancements (:

sys/include/net/gcoap.h

kb2ma · 2020-03-27T11:11:05Z

I'm not familiar enough with proxy use to know what is customary. Do you think it makes sense to allow specification of a Uri-Path for the proxy in the example, like /coap2http, when using the 'set proxy' command? I would only add it if it is your experience that a user would "expect" a proxy to include a path to use it.

hmm .. I do not quite understanding this :/

Perhaps the reason for the specific /coap2http resource is that the endpoint also supports a /coap2coap resource, and the handlers would be significantly different -- UDP vs. TCP, etc.

At any rate, let's defer any modifications for a path. The gcoap example is just an example and does not need to cover all possible uses of a proxy.

kb2ma · 2020-03-27T11:20:50Z

I think that covers all the issues from testing. Let me take one more look through.

kb2ma · 2020-03-27T16:09:34Z

Looks good and tests pass. Able to do a PUT with a payload and receive a Block2 response. Please squash!

kb2ma · 2020-03-27T16:14:23Z

hmm .. I do not quite understanding this :/

Perhaps the reason for the specific /coap2http resource is that the endpoint also supports a /coap2coap resource, and the handlers would be significantly different -- UDP vs. TCP, etc.

I see your point now! It's not legal to add a Uri-Path option with a Proxy-Uri option. In that case the cf-proxy app could parse the Proxy-Uri scheme to decide what to do.

At any rate, that app was really nice for testing. I was able to create two tap interfaces and use the proxy to communicate between them.

cgundogan · 2020-03-27T21:08:22Z

thanks @kb2ma for this review :) I just squashed and triggered murdock.

cgundogan · 2020-03-27T23:48:00Z

I amended a fix to the doc (unsinged => unsigned) and retriggered murdock.

sys/include/net/nanocoap.h

kb2ma · 2020-03-28T14:32:03Z

Good stuff @cgundogan, and great to finally work through a PR with you.

cgundogan added Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Mar 15, 2020

cgundogan requested review from kb2ma and leandrolanzieri March 15, 2020 23:56

cgundogan requested review from PeterKietzmann, haukepetersen and miri64 as code owners March 15, 2020 23:56

cgundogan force-pushed the coap_proxy branch 3 times, most recently from a1119fa to d047a15 Compare March 16, 2020 08:57

leandrolanzieri added the Area: CoAP Area: Constrained Application Protocol implementations label Mar 16, 2020

kb2ma mentioned this pull request Mar 16, 2020

net/gcoap: expose remote sock_udp_ep to request handlers #13621

Closed

kb2ma reviewed Mar 18, 2020

View reviewed changes

kb2ma reviewed Mar 20, 2020

View reviewed changes

examples/gcoap/gcoap_cli.c Outdated Show resolved Hide resolved

examples/gcoap/gcoap_cli.c Outdated Show resolved Hide resolved

cgundogan commented Mar 24, 2020

View reviewed changes

examples/gcoap/gcoap_cli.c Outdated Show resolved Hide resolved

kb2ma reviewed Mar 25, 2020

View reviewed changes

sys/net/application_layer/nanocoap/nanocoap.c Show resolved Hide resolved

kb2ma reviewed Mar 25, 2020

View reviewed changes

sys/include/net/gcoap.h Outdated Show resolved Hide resolved

sys/include/net/gcoap.h Outdated Show resolved Hide resolved

sys/include/net/gcoap.h Outdated Show resolved Hide resolved

cgundogan force-pushed the coap_proxy branch 2 times, most recently from d6abb70 to cc8ebc5 Compare March 25, 2020 15:27

cgundogan force-pushed the coap_proxy branch from cc8ebc5 to d38a434 Compare March 25, 2020 15:43

leandrolanzieri added this to the Release 2020.04 milestone Mar 25, 2020

kb2ma reviewed Mar 26, 2020

View reviewed changes

sys/include/net/gcoap.h Outdated Show resolved Hide resolved

kb2ma reviewed Mar 26, 2020

View reviewed changes

sys/include/net/gcoap.h Outdated Show resolved Hide resolved

coap: add Proxy-* options

97c5d2d

cgundogan force-pushed the coap_proxy branch from 12a83c1 to eb981c9 Compare March 27, 2020 21:07

cgundogan added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 27, 2020

cgundogan added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 27, 2020

cgundogan force-pushed the coap_proxy branch from eb981c9 to 5159c3d Compare March 27, 2020 23:45

kb2ma reviewed Mar 28, 2020

View reviewed changes

sys/include/net/nanocoap.h Outdated Show resolved Hide resolved

cgundogan added 3 commits March 28, 2020 14:50

nanocoap: add convenience function for PROXY_URI

50900a1

examples/gcoap: add handling of proxied requests

7cda842

net/gcoap: add note on proxy operation

b1f7281

cgundogan force-pushed the coap_proxy branch from 5159c3d to b1f7281 Compare March 28, 2020 13:51

kb2ma approved these changes Mar 28, 2020

View reviewed changes

kb2ma merged commit 7d624de into RIOT-OS:master Mar 28, 2020

cgundogan deleted the coap_proxy branch March 28, 2020 14:34

cgundogan mentioned this pull request Apr 1, 2020

gcoap: add simple forward-proxy #13790

Merged

kb2ma mentioned this pull request Apr 17, 2020

net/gcoap: add CoAP ping request #13894

Merged

Conversation

cgundogan commented Mar 15, 2020

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

cgundogan commented Mar 16, 2020

Uh oh!

kb2ma commented Mar 18, 2020

Uh oh!

kb2ma Mar 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgundogan Mar 18, 2020

Choose a reason for hiding this comment

Uh oh!

kb2ma Mar 19, 2020

Choose a reason for hiding this comment

Uh oh!

kb2ma Mar 19, 2020

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri Mar 19, 2020

Choose a reason for hiding this comment

Uh oh!

kb2ma commented Mar 19, 2020

Uh oh!

kb2ma left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kb2ma commented Mar 20, 2020

Uh oh!

miri64 commented Mar 20, 2020

Uh oh!

Uh oh!

cgundogan commented Mar 24, 2020

Uh oh!

Uh oh!

kb2ma left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cgundogan commented Mar 25, 2020

Uh oh!

Uh oh!

Uh oh!

kb2ma commented Mar 27, 2020

Uh oh!

kb2ma commented Mar 27, 2020

Uh oh!

kb2ma commented Mar 27, 2020

Uh oh!

kb2ma commented Mar 27, 2020

Uh oh!

cgundogan commented Mar 27, 2020

Uh oh!

cgundogan commented Mar 27, 2020

Uh oh!

Uh oh!

kb2ma commented Mar 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kb2ma Mar 18, 2020 •

edited

Loading