[RFC][WIP] SOCKS5 proxy server implementation #12180

tgorochowik · 2018-12-19T13:13:55Z

This PR adds an initial lib implementation of SOCKS proxy (version 5).

What is working:

TCP connections without any authentication
IPv4 addressing

Sample test/network setup:

Zephyr node running a HTTP server (e.g. 192.168.0.100)
Zephyr node running this proxy server (e.g. 192.168.0.150)
A PC with curl installed.

With that setup, it should be possible to download what's the http server is serving through the proxy node with:

$ curl --socks5 192.168.0.150:1080 192.168.0.100
<html>
  <head>
    <meta charset="UTF-8">
    <title>Zephyr HTTP server sample</title>
  </head>

  <body>
    <div>
      <p>It works!</p>
    </div>
  </body>
</html>

I am opening this PR early in hope to:

Receive early general review that to avoid that when the socks5 server is complete and there is more code with possibly the same issues
Trigger some discussion about how the network app api could be improved

About the API:
The most problematic thing in my opinion is that all the callbacks lack per-connection private data. What I have in mind is a server application with multiple clients connected at once.

For the recv or send callback, we do get the pkt which has net_context - which is you could say could act as an identifier of the peer that is connected.

For the connect callback, there is no such thing. And it could be very useful when using it with some protocols that require initializing a state machine.
The close callback is not even called for each peer closing its connection. Its called only once, after all the connections have been closed.

In the current socks5 code, you can see these issues are worked around. The per-connection private data pointers are selected based on their state (to initialize: socks_find_free_client_slot) and then using the remote data to see which private data should be used (recv cb: socks_find_client). Checking which client disconnected is guesswork. We don't even get the callback if there is another client that did not terminate its connection yet.

I think it would be the best not to have to implement such helpers at all. It would be the best if API could do this for us.

Here are some ideas that I hope to receive some comments to:

The net_app_ctx struct should get something like: void *connection_data[CONFIG_NET_APP_SERVER_NUM_CONN]. All the callbacks should be able to use this pointer (e.g. via an argument). Users could then set this to point to their app specific per-peer data in the connect callback, and then use it in the recv, send, and close callbacks
The close callback should be called for EACH peer that closes its connection. That was actually the case prior to: 88e8119078f - I think that the problem this commit solved could be solved differently (in the http code)

The problem with this approach is that the client implementations in net app api use the same interface for callbacks. Being a client implicates being the only peer, so the per-connection pointers are completely redundant in this case.

I am open to all comments on about the current SOCKS5 implementation and about the API issues (and possible solutions) that this implementation uncovers. If there is an easier and more reliable way to work-around those issues I would be glad to hear about it as well.

Thanks!

pfalcon · 2018-12-19T13:20:13Z

net_app is deprecated. BSD Sockets is the networking API in Zephyr.

tbursztyka

Hum, there is a big issue with this lib: it uses net_app, net_context, net_pkt, etc...

It should be using socket exclusively. It was decided (almost a year ago in fact) that socket should be the unique end point for the user (and libraries etc...). All the other are net core only. (net_app is probably going to /dev/null)
There is an going work to change how net_pkt works (see PR #11775) and that's going to affect your code heavily. You would be using socket: it wouldn't affect at all.

I am sorry if the communication was unclear about this big moves.

tgorochowik · 2018-12-19T13:37:31Z

@pfalcon @tbursztyka ok thanks! It is quite confusing actually, especially that the net_app API is marked as EXPERIMENTAL - it looks almost like it is the next thing.

I am glad we did not continue with other features of SOCKS with this implementation then! Better now than later.

Could we somehow explicitly mark the net_app API as deprecated then? Be it in Kconfig, or with some comments at the beginning of the source files?

codecov-io · 2018-12-19T13:50:25Z

Codecov Report

Merging #12180 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #12180   +/-   ##
=======================================
  Coverage   53.94%   53.94%           
=======================================
  Files         242      242           
  Lines       27654    27654           
  Branches     6717     6717           
=======================================
  Hits        14917    14917           
  Misses       9932     9932           
  Partials     2805     2805

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 416d397...b008119. Read the comment docs.

tbursztyka · 2018-12-19T14:02:01Z

Could we somehow explicitly mark the net_app API as deprecated then? Be it in Kconfig, or with some comments at the beginning of the source respective files?

The process of deprecating an API is actually a bit harder than this in Zephyr. It requires to put __deprecated in the front of all exposed macros/functions of the API. And that affects CI then: if something is using it, the generated warning makes CI marking it as a no-go. I guess if we deprecate net_app, we'll need to do so for http, sntp etc...

pfalcon · 2018-12-19T14:06:49Z

@tgorochowik :

net_app API is marked as EXPERIMENTAL - it looks almost like it is the next thing.

Well, EXPERIMENTAL just means it's subject to change, a change may include going away too...

Could we somehow explicitly mark the net_app API as deprecated then? Be it in Kconfig, or with some comments at the beginning of the source files?

So, the actual state of net_app I'd describe as "pre-deprecation". The idea is:

All previous networking facilities weren't migrated to sockets yet, so socket networking is not yet "there".
With that in mind, and even without that, the idea is to not "disturb" users. If net_app-based protocol implementations work for them, they continue to use them, until socket-based implementations appear.
But given already spent effort to migrate protocol libs to sockets, which only continues, it would be quite fair to not accept implementations based on net_app into mainline, and in general, encourage users to start new implementations with sockets instead.

The migration process requirements/status is tracked via #7591

tgorochowik · 2018-12-19T14:13:25Z

Ok thanks, that makes perfect sense!

Of course I do understand what experimental means, all I'm saying that it is just quite easy to misinterpret it (which I did) I also do agree that it is more than fair not to accept new implementations with that API now that I know what you know (especially that I believe this API is not fully functional).

What about at least adding a note to the beginning of:
https://docs.zephyrproject.org/1.13.0/subsystems/networking/net-app-api.html
Saying to at least consider (like you said encourage) using sockets instead?

Anyway: that is just a suggestion in hope nobody else falls into the same trap. I am closing this PR for now, thanks for your time!

pfalcon · 2018-12-19T14:19:16Z

@tgorochowik: I agree that we definitely need to do something to "spead the word". If you have very quick idea to implement in that regard, like adding a notice like you propose, feel free to shoot a PR (dunno if such would be accepted right away, but worth a try). Otherwise, I consider it to be in my queue to do various things in that regard (tickets to refer people to, posts to maillists, etc.).

For now, only TCP connections with IPv4 addresses are supported. Signed-off-by: Tomasz Gorochowik <tgorochowik@antmicro.com>

Just start the proxy server at the default port and do nothing. Signed-off-by: Tomasz Gorochowik <tgorochowik@antmicro.com>

tgorochowik · 2019-01-21T12:28:45Z

Just pushed an initial sockets-based rewrite.

Note that not all features are implemented, not everything works yet, some architectural changes are probably needed (e.g. buffers handling), and there are still some debugging leftovers.

The basic example (with curl) described in the original message should work.

zephyrbot · 2019-01-21T12:32:39Z

Found the following issues, please fix and resubmit:

commit message syntax issues

Commit b93bc9c2e1:
1: T5 Title contains the word 'wip' (case-insensitive): "WIP: samples: Add SOCKS5 server sample application"

Commit 1744d456eb:
1: T5 Title contains the word 'wip' (case-insensitive): "WIP: net: lib: SOCKS5 proxy server implementation"

Checkpatch issues

-:42: ERROR:C99_COMMENTS: do not use C99 // comments
#42: FILE: include/net/socks.h:36:
+	struct socks_client clients[3] ; //CONFIG_NET_APP_SERVER_NUM_CONN];

-:42: WARNING:SPACING: space prohibited before semicolon
#42: FILE: include/net/socks.h:36:
+	struct socks_client clients[3] ; //CONFIG_NET_APP_SERVER_NUM_CONN];

-:58: ERROR:C99_COMMENTS: do not use C99 // comments
#58: FILE: include/net/socks.h:52:
+	//int port;

-:63: WARNING:LINE_SPACING: Missing a blank line after declarations
#63: FILE: include/net/socks.h:57:
+	struct k_thread thread;
+	K_THREAD_STACK_MEMBER(stack, SOCKS5_STACK_SIZE);

- total: 2 errors, 2 warnings, 801 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

pfalcon · 2019-02-06T09:49:24Z

@tgorochowik: I hope that this was closed by mistake or to trigger CI. The PR itself is even marked for 1.14.0 (though I'm not sure if keep pushing more and more stuff into it), and lack of reply is due to all-hands work on other areas before 1.14 freeze.

tgorochowik · 2019-02-06T10:12:14Z

@pfalcon Thanks for your reply. I closed it because it will not be needed for now, and the implementation was far from being ready anyway.

The more important use-case (for 1.14) is a SOCKS5 client (MQTT connection through a SOCKS5 proxy to be more exact) - I just opened a new PR with its implementation - #13089

pfalcon · 2019-02-06T10:18:21Z

@tgorochowik: Ok, thanks for explanation, sounds good. It's just that just yesterday at the networking meeting we were talking that the more samples we have the better, because otherwise we can't exercise socket API and networking in general. So, any code written is useful and precious, though of course each of us should be ready that it may take extra time to get it reviewed (and changes may be requested, as usual).

tgorochowik added area: Networking RFC Request For Comments: want input from the community DNM This PR should not be merged (Do Not Merge) labels Dec 19, 2018

tgorochowik requested review from jukkar and tbursztyka December 19, 2018 13:13

tgorochowik requested a review from pfalcon as a code owner December 19, 2018 13:13

tbursztyka requested changes Dec 19, 2018

View reviewed changes

tgorochowik closed this Dec 19, 2018

tgorochowik added 2 commits January 16, 2019 12:02

WIP: net: lib: SOCKS5 proxy server implementation

1babe99

For now, only TCP connections with IPv4 addresses are supported. Signed-off-by: Tomasz Gorochowik <tgorochowik@antmicro.com>

WIP: samples: Add SOCKS5 server sample application

b008119

Just start the proxy server at the default port and do nothing. Signed-off-by: Tomasz Gorochowik <tgorochowik@antmicro.com>

tgorochowik reopened this Jan 21, 2019

tgorochowik force-pushed the socks5 branch from efc9659 to b008119 Compare January 21, 2019 12:27

jukkar modified the milestone: v1.14.0 Jan 30, 2019

tgorochowik closed this Feb 6, 2019

tgorochowik deleted the socks5 branch May 28, 2019 09:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC][WIP] SOCKS5 proxy server implementation #12180

[RFC][WIP] SOCKS5 proxy server implementation #12180

tgorochowik commented Dec 19, 2018

pfalcon commented Dec 19, 2018

tbursztyka left a comment

tgorochowik commented Dec 19, 2018 •

edited

Loading

codecov-io commented Dec 19, 2018 •

edited

Loading

tbursztyka commented Dec 19, 2018

pfalcon commented Dec 19, 2018

tgorochowik commented Dec 19, 2018

pfalcon commented Dec 19, 2018

tgorochowik commented Jan 21, 2019 •

edited

Loading

zephyrbot commented Jan 21, 2019 •

edited

Loading

pfalcon commented Feb 6, 2019

tgorochowik commented Feb 6, 2019

pfalcon commented Feb 6, 2019

[RFC][WIP] SOCKS5 proxy server implementation #12180

[RFC][WIP] SOCKS5 proxy server implementation #12180

Conversation

tgorochowik commented Dec 19, 2018

pfalcon commented Dec 19, 2018

tbursztyka left a comment

Choose a reason for hiding this comment

tgorochowik commented Dec 19, 2018 • edited Loading

codecov-io commented Dec 19, 2018 • edited Loading

Codecov Report

tbursztyka commented Dec 19, 2018

pfalcon commented Dec 19, 2018

tgorochowik commented Dec 19, 2018

pfalcon commented Dec 19, 2018

tgorochowik commented Jan 21, 2019 • edited Loading

zephyrbot commented Jan 21, 2019 • edited Loading

commit message syntax issues

Checkpatch issues

pfalcon commented Feb 6, 2019

tgorochowik commented Feb 6, 2019

pfalcon commented Feb 6, 2019

tgorochowik commented Dec 19, 2018 •

edited

Loading

codecov-io commented Dec 19, 2018 •

edited

Loading

tgorochowik commented Jan 21, 2019 •

edited

Loading

zephyrbot commented Jan 21, 2019 •

edited

Loading