net: zstream API to abstract socket transport protocols (e.g. TCP vs TLS)

[pfalcon]

This is iteratitive (iterations now squashed) work on #5900

• Implement socket stream wrapper
• Implement TLS stream wrapper
• Convert simple TCP client to the stream API (http_get)
• Convert more advanced TCP client to the stream API (big_http_download)
• Convert simple TCP server to the stream API (echo)
• Convert more advanced TCP server to the stream API (echo_async)
• Convert MQTT client lib ([WIP][RFC] net: mqtt: Port MQTT library to BSD sockets #5854) to the stream API
• Apply TLS configuration ideas from Ideas/requirements for improved TLS support in Zephyr #6086

[codecov-io]

Codecov Report

Merging #5985 into master will increase coverage by 0.47%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #5985      +/-   ##
==========================================
+ Coverage   64.61%   65.08%   +0.47%     
==========================================
  Files         421      422       +1     
  Lines       40296    39443     -853     
  Branches     6803     6651     -152     
==========================================
- Hits        26037    25673     -364     
+ Misses      11126    10687     -439     
+ Partials     3133     3083      -50

Impacted Files	Coverage Δ
subsys/net/lib/tls_conf/tls_conf.c	0% <0%> (ø)
subsys/net/lib/zstream/zstream_sock.c	0% <0%> (ø)
subsys/net/lib/zstream/zstream.c	0% <0%> (ø)
include/net/zstream.h	0% <0%> (ø)
tests/net/lib/mqtt_packet/src/mqtt_packet.c
subsys/net/lib/mqtt/mqtt_pkt.c

---

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 080e32e...57c0716. Read the comment docs.

[pfalcon]

Note that this includes #5981 as an inter-dependency.

[pfalcon]

So, that's how it looks. #5900 mentioned this matter, now we can consider exact variants of look&feel:

stream->vt->write(stream, ...) - from "vtable"
stream->meth->write(stream, ...) - from "method"

These aren't much better/different from api IMHO. Note that besides these (repeating), and extra var also needs to be introduced: stream = (zstream)&stream_sock.

CALL_METH(stream, write, ...)

This still looks uglier IMHO. Also, realistically that would be ZSTREAM_CALL_METH().

[lpereira]

Naming something ZSTREAM_CALL_METH() would make it uncallable: meth, not even once.

[tbursztyka]

Using the stream struct like it is super ugly actually. I'd rather see inline functions to hide this.

[pfalcon]

I'd rather see inline functions to hide this.

Indeed, the winner is: zstream_write(), zstream_read(), etc. I'll add them.

[lpereira]

Are we still maintaining Makefiles?

[pfalcon]

Maintaining? No. They just work ;-).

P.S. This Makefile is for POSIX, not for Zephyr. Obviously, there're no cmakes and ninjas in POSIX.

[lpereira]

I like the general idea of a streaming API, but I'm not really sold on the idea of having it only for sockets. It looks kind of like WinSock, where some things are available for sockets, but not for regular files. We might be better off with a discussion on file descriptions/file descriptors that involve the whole system (the FS API, networking, etc).

[pfalcon]

@lpereira

but I'm not really sold on the idea of having it only for sockets.

Well, this WIP PR comes out of pretty extensive discussion in #5900. In summary, the usecase is to abstract away differences of communicating between TCP and TLS sockets. But the proposed interface is exactly generalized to support any method of peer-to-peer communication (#5900 gives an example talking TLS over SPI). So, I'm not sure where idea that it's "only for sockets" comes from.

[GAnthony]

So, I'm not sure where idea that it's "only for sockets" comes from

Perhaps from the PR's title: "net: zstream API to abstract socket transport protocols (e.g. TCP vs TLS)"

This PR appears to be trying to solve a different problem than bridging the missing gaps between net_app and BSD sockets (i.e., mainly, adding secure sockets), as there is no client/server setup, TLS configuration or connection management functionality here (yet?).

A new generic stream interface for Zephyr might be a fine idea, but it would help to understand what current problems in Zephyr it is solving.

As the updated http_get sample shows, it results in mixed abstraction levels, calling some lower level socket APIs, then maybe secure socket setup APIs (TBD?), then some stream APIs, all in the same program.
It would look more consistent if all the operations where subsumed under a common API.

The link provided in #5900 (https://www.openbsd.org/papers/linuxconfau2017-libtls/#slide-16) seems to be a reasonable API that tries to address the problem at hand. Maybe it can be generalize somewhat, but something like this seems more intuitive.

[tbursztyka]

I did not see anything about mbedtls, but it is going to be exposing tls from mbedtls right? It already provides a way to link a socket to its context (the adaptation needs to be done though). Would mbedtls get link to socket via zstream_socket, and itself be exposed to user via (some) zstream_tls?

Overall, the base idea is actually solving a bit more than just tls. net_app tried to be the application level API for networking, but failed as it gathered everything in one struct, so it was bloated etc... Here it's split it 2: a common API, and a specialized "object" (I don't like this wording in C but well) implementing it. That specialization could be tls in our case. That reminds me some python package actually. Btw: zstream_ prefix: a bit too lengthy. What about zio_ ?

The problem is that it's taking the problem by the easiest part: read, write, flush and close.
But TLS will needs way more than that, and dedicated stuff such as configure and handshake for instance, are annoying parts. How is this approach going to address such requirements?
Other specialization would also come up with their own requirements. This will become quickly a nightmare to keep things generalized. In the end you'll have specialization of a base block, so different than others that besides the prefix name, nothing will be really compatible.

Trying to abstract too much sometime is an open door for bloating up things (we are supposed to run on constrained system). I am not entirely sure we need that kind of approach. Maybe one api doing one thing and doing it well would be just enough, as @GAnthony pointed it.

[tbursztyka]

Using the stream struct like it is super ugly actually. I'd rather see inline functions to hide this.

[pfalcon]

So, I'm not sure where idea that it's "only for sockets" comes from
Perhaps from the PR's title: "net: zstream API to abstract socket transport protocols (e.g. TCP vs TLS)"

I'm not sure if it's worth to go down this word-picking way, because formally, that title doesn't contain the word "only" still. I tried to answer @lpereira concerns, explaining that this PR is a WIP for #5900. I'm sorry for not marking the title as "[WIP]" the day it was submitted, which might caused confusion (now done). If I can disambiguate the intent of this PR even better, I'd gladly accept suggestions.

then maybe secure socket setup APIs (TBD?)

Yes, as the description of this PR says, it's "This is first iteration of work on #5900". Of course, first iteration means it's very initial steps on implementing it. If that's not clear enough, please suggest a better wording. I'm on the other hand a strong believer in the "release early, release often" process, which may allow more streamlined development cycle for all interested parties.

[pfalcon]

The latest push implements @tbursztyka's hint of adding inline function wrappers for virtual method calls, so there's now ssize_t zstream_read(zstream stream, void *buf, size_t size). In retrospect, that's pretty obvious step, but I was fixated on other things, so thanks for the suggestion, "post for early review" thing works ;-).

That also pretty much goes half-way towards @GAnthony's idea of reusing POSIX calls for TLS operations. So yes, now potentially someone very keen could do #define read(a, b, c) zstream_read(a, b, c) and minimize other changes required to port pure socket code towards TLS awareness.

[pfalcon]

@GAnthony :

This PR appears to be trying to solve a different problem than bridging the missing gaps between net_app and BSD sockets (i.e., mainly, adding secure sockets),

I don't see my work as trying to "bridging the gap between net_app and BSD sockets". Adding the missing features of streamlined TLS support, yes.

as there is no client/server setup,

Client/server setup is part of BSD Sockets per se, many applications use it directly and don't need any special APIs. Such APIs can be added, but they're separate matter from abstracting TLS communication, I'm trying to tackle one problem at time.

TLS configuration or connection management functionality here (yet?).

TLS support in works, and as I mentioned, my plan is to focus on TLS communication, not configuration. On the latter front, we'll need help and support from our security people anyway. (To clarify how I see that, I definitely going to add something in that vein, and do my homework on that, but even after that I expect that security people may (among many other choices): a) shoot it down and say they rather do it themselves; b) us having a nice few-months go-between to get it right).

[pfalcon]

@tbursztyka

I did not see anything about mbedtls, but it is going to be exposing tls from mbedtls right?

Yup. As a status update, I was finishing writing that down and preparing to get my first crash with it, when I peeked at the mbedTLS code and found a couple of, well, issues. So, I'm taking excursion into that: #6025 , Mbed-TLS/mbedtls#1356 .

[pfalcon]

That reminds me some python package actually.

Yeah, as a full disclosure, the ideas put forward here were tested and tried in MicroPython previously.

Btw: zstream_ prefix: a bit too lengthy. What about zio_ ?

Well, let's consider them tentative names for now and discuss later. As for "zio", that's pretty precious, generic name, are you sure you'd like waste on this, relatively specific thing?

[pfalcon]

@GAnthony

A new generic stream interface for Zephyr might be a fine idea, but it would help to understand what current problems in Zephyr it is solving.

I'm sorry again that I didn't make that obvious enough. I'd think that you would be the party to whom the intent behind this PR (and its prompt posting) would be clear: it shows what kind of changes you would need to apply to your patch #5854 to get 90% of TLS support in (90% of our part, there's also security people's part). Let me give a specific example: you would need to replace this line: https://github.com/zephyrproject-rtos/zephyr/pull/5854/files#diff-5364c0b04abf6f5bdd7374608c7f5f1eR270 with (per the latest API iteration as of today) with zsock_read(). cb_entry->sock needs to be made of type zstream. send() calls need to be changed to zsock_write(). close(), similarly. And you'd need to insert calls to zstream_flush() when you finished writing a complete protocol message into the stream (maybe, in parts). It's effectively a checkpoint asking data to be delivered to peer (anything before that can be buffered).

The remaining 10% is answering questions like cb_entry->sock gets its values. We'll get there too. There're known solutions, for different tastes.

[pfalcon]

The latest push add initial prototype of TLS stream implementation. Few points:

• Test only with Linux so far (that's true for the entire patchset)
• At this stage, there's no need to comment on how TLS connection is setup - as mentioned above, that's a separate stage of work, on TODO. The current prototype shows how TLS communication works (and it works in exactly the same manner as communication over any other stream, that's the whole idea of the stream design). If there's interest in discussing TLS configuration part, we can create a separate ticket to throw in ideas/requirements (I personally would need to do more homework before productively participating in such a discussion).
• Otherwise, it "works" (in the sense, that it prints decoded data, as expected).

[pfalcon]

@GAnthony:

As the updated http_get sample shows, it results in mixed abstraction levels, calling some lower level socket APIs, then maybe secure socket setup APIs (TBD?), then some stream APIs, all in the same program.

I don't think this assessment is fair. After all, as long as there're abstraction levels at all, they will be mixed in the same program. "Mixed" is however the wrong word here, they aren't "mixed", but "layered" and "structured". Such approach is know as "software design using patterns" (https://en.wikipedia.org/wiki/Software_design_pattern). And that's what being used here.

http_get is very simple program, so indeed uses all levels in a single function. In more complex programs, different abstractions are intended to be used in different functions. To emphasize that distinction and alleviate confusion, in the latest commit, "samples: sockets: http_get: Further conversion to use use TLS stream.", I split the single function in 2. http_request() accepts a stream object as a parameter and works solely with it, implementing "business logic" (HTTP GET request handling in this case). The purpose of the main() function is thus to perform the needed object construction and initialization. This is known is https://en.wikipedia.org/wiki/Dependency_injection design pattern, and indeed the one I'm trying to follow when designing this API.

It would look more consistent if all the operations where subsumed under a common API.

That's how net_app was designed - pack everything under a "common API". Now concerns with that API are well known (one is "it packs too much things"). As I mentioned, I'd like to take a chance to redesign things, based on the experiences with that API. And it's fair to say that so far I'm trying to keep separate things separate, while abstracting common behavior to polymorphic interfaces. It's also fair to expect we may eventually meet with net_app's approach somewhere in the middle. But premature commonality complicates the design and analysis.

[pfalcon]

If there's interest in discussing TLS configuration part, we can create a separate ticket to throw in ideas/requirements (I personally would need to do more homework before productively participating in such a discussion).

I now did a bit of this homework and created such a ticket: #6086

[nashif]

is this still relevant?

[nashif]

@pfalcon ping

[pfalcon]

Closing given that #9007 was merged upstream.