Websocket support #1271

jukkar · 2017-08-28T08:00:14Z

Initial websocket server side support.

dbkinder · 2017-08-28T16:11:13Z

include/net/websocket.h

there is no parameter "type" or "mask"

dbkinder · 2017-08-28T16:21:30Z

samples/net/ws_echo_server/README.rst

I'd change this to:

The default make BOARD configuration for this sample is ``qemu_x86``.

dbkinder

doxygen comment problem, and a suggested edit for README

dbkinder · 2017-08-29T16:00:29Z

include/net/websocket.h

no parameter "flag"

dbkinder · 2017-08-29T16:02:17Z

include/net/websocket.h

missing @param for final

dbkinder

couple of doxygen errors...

jukkar · 2017-08-29T17:14:05Z

Thanks David, I have tried couple of times to setup doxygen so I could do documentation pre-checks myself but I have so far failed.

mike-scott · 2017-08-29T16:50:57Z

subsys/net/ip/utils.c

Seems like the logic of this function could probably be simplified by checking for a :[port] first and then using what's left in the combined (defined(CONFIG_NET_IPV4) && defined(CONFIG_NET_IPV6)) section.

I will address this in PR #1198 as that contains these patches. I was forced to add unrelated patches to this one in order to compile the code.

mike-scott · 2017-08-29T16:53:31Z

tests/net/utils/src/main.c

Why would this test not pass vs. the test above for "192.0.2.3/foobar" which should pass? And the test below for "192.0.2.3:80/foobar" which should pass?

This will not pass as the port number that is tried to parse is "80/foobar" which is not an integer.

mike-scott · 2017-08-29T17:04:37Z

subsys/net/lib/websocket/websocket.c

This block seems to be duplicated quite a lot. Can we #define PORT_STR at the top? Also, PORT_STR seems to indicate it's a string. Not a sizeof a string.

I will refactor this

mike-scott · 2017-08-29T17:07:53Z

subsys/net/ip/net_shell.c

There's a placeholder function defined (#define http_server_cb(...) NULL) when CONFIG_NET_DEBUG_HTTP_CONN && CONFIG_HTTP_SERVER aren't defined, but we're checking for them here? Should we remove the placeholder or remove the checks here? (and below)

Yes, will fix

mike-scott · 2017-08-29T17:08:43Z

subsys/net/ip/net_shell.c

Seems like we can remove some of the #if defined checks below if we create a placeholder for this function similar to http_server_cb() above.

Indeed, I will change this.

mike-scott · 2017-08-29T17:13:13Z

samples/net/ws_echo_server/prj.conf

Should the MBEDTLS settings be removed from this .conf?

This is needed for sha1 and base64 functions that are used even if we do not have TLS supported.

Perhaps a comment line in this prj.conf would help then.

pfalcon · 2017-11-08T12:38:28Z

include/net/websocket.h

Is "to send" expected here? I'd think the function is applied to recved packets.

pfalcon · 2017-11-08T12:42:02Z

include/net/websocket.h

Looking just at the header, it's strange to have such a function as part of the public API. More specifically, I don't think it can work at all: masking_value is actually sequence on 4 bytes, applied with wrap-around to a stream of bytes. Thus, it needs to maintain an offset in 4-byte seq, or it won't be able to apply xform correctly to e.g. pkt of 5 bytes followed by pkt of 1 byte.

The function works just fine in my tests, it properly loops over masking_value. Why do you say it cannot work, have you tried it and it failed?

No, it's based on the speculative review and knowledge of websocket protocol/experience with implementing it. There's always a chance of reviewer's error.

But what I see in 273e113#diff-4b79f0bbb3bc74c2d5bbeb4209f6490eR664 is a typical layering mis-assumption. Websocket records is a stream of bytes, wrapped in TCP stream of bytes, wrapped in IP packets. You receive an IP packet and think that it represents a complete Websocket message. It does not. 1st packet you receive contains 1st byte of Websocket header, 2nd byte - 2nd, etc., one packet per each byte. That's the picture someone working with a streaming protocol like TCP should have in mind, not something else. So, you can't even "read" a mask value from a pkt - you should buffer a stream of bytes of enough length to hold an entire Websocket header, then when you fill it in, you should parse it. Then next packets won't even contain WS header, they will contain WS data (with arbitrary unmasking offset as was pointed out). Then finally one of the next headers will contain 1000 bytes of data followed by next WS header (in the same pkt).

(Yeah, when you implement the code infra to support correct handling of a stream protocol, you'll end up duplicating a lot of BSD Sockets functionality, that point was already shared.)

Yes, I see your point. I did not understand your first comment.

pfalcon · 2017-11-08T12:45:55Z

include/net/websocket.h

Not sure if the function name corresponds to its functionality: "The function will check if this is a valid websocket connection.". If the function indeed does that, it should be named accordingly, otherwise, if name is right, does it have to be part of public API? (Maybe this header file needs to be split in "public API" and "private API" parts, if there's no choice of having 2 header files?)

Some of the functions could be marked as internal as they are very specific to the handshake phase. I will separate them.

pfalcon · 2017-11-08T12:47:44Z

subsys/net/lib/http/Kconfig

Is such a high value based on something? My visual memory says that no. of headers in websocket establishment request was under a dozen.

For example Firefox sends lot of headers, we need to be prepared to receive all of them as otherwise the handshake will fail.

pfalcon · 2017-11-08T12:55:50Z

subsys/net/lib/http/http_app_server.c

This won't work correctly, as mentioned in ws_mask_pkt() prototype review.

pfalcon · 2017-11-08T13:20:30Z

subsys/net/lib/websocket/websocket.c

There should be websocket_ctx object, what is "i" in this function, should be part of this context.

There is no need for separate websocket_ctx, all context info is handled currently by http_ctx.

Couple of points:

For a server implementation (e.g. HTTP server), there should be a context object. However, for each request it serves, there should be a separate request context object. The need for that should be obvious, considering a server may serve several independent requests (== clients). I had a feeling that HTTP library that uses net-app #4243 doesn't have that, didn't raise it before to not procrastinate it further, but without a proper implementation of the above, it's an alpha version.

Websocket is a separate protocol from HTTP, so need a separate WS request (connection) context from HTTP request ctx. Implementation-wise, those can be the same underlying structure (i.e. if WS support enabled, every "mere HTTP" req ctx gets overhead of WS fields).

pfalcon · 2017-11-08T13:22:13Z

subsys/net/lib/websocket/websocket.c

Similar comment to the above (count belongs to websocket_ctx).

I lost the context here, what do you mean by this?

pfalcon

While I appreciate the idea of getting more useful protocols in the next release around the corner, per my review comments, I believe this doesn't implement a correct received data handling.

IMHO, instead of hasting and delivering half-bakes solutions, which will require a lot of rework going forward (possibly, breaking user APIs), we'd better do it bottom up, adding new layers on top of well-working existing layers. In this regard, I think the new HTTP API could use more work to get it more stable for the release, instead of hasting with e.g. Websocket support.

jukkar · 2017-11-08T13:47:21Z

While I appreciate the idea of getting more useful protocols in the next release around the corner, per my review comments, I believe this doesn't implement a correct received data handling.

I will send a new version shortly. And we can certainly postpone this to next version, I was not really expecting this to be accepted by you anyway. The API is currently marked as experimental so we should be able to change it at will. The client functionality would be nice to have and it might require changes to the APIs.

pfalcon · 2017-11-08T13:54:52Z

to be accepted by you anyway

I hope the keyword is "accepted" and not "by you" ;-). I really appreciate your work, but I don't think it scales to have a churn of one API implantation replace by another, repeated. So, I really think we should spend more homework on each piece (and in the process, learn the best way to layer things).

jukkar · 2017-11-08T15:49:22Z

New version:

moved some of the "internal" functions to separate header file that is not public
changed the receiving side to continue reading stream until all data has been read

jukkar · 2017-11-08T15:57:12Z

This needs still more testing, there was some issues when client is sending lot of data. I will continue with this tomorrow.

dbkinder · 2017-11-14T17:22:15Z

samples/net/ws_echo_server/README.rst

the cmake changes introduced a new way to document build steps

jukkar · 2017-11-15T10:56:58Z

Updates:

code to support one byte at a time handling
readme file fix
unit test added

Note that this is not to be applied to v1.10

Do not allow :: or ANY address in client when sending data. Signed-off-by: Jukka Rissanen <[email protected]>

This commit creates a websocket library that can be used by applications. The websocket library implements currently only server role and it uses services provided by net-app API. The library supports TLS if enabled in configuration file. This also adds websocket calls to HTTP app server if websocket connection is established. Signed-off-by: Jukka Rissanen <[email protected]>

This is a http(s) server that supports also websocket. It sends back any data sent to it over a websocket. Signed-off-by: Jukka Rissanen <[email protected]>

This tests websocket by creating a websocket support http server and sending data to it and verifying the returned data is the same. Signed-off-by: Jukka Rissanen <[email protected]>

jukkar · 2017-11-16T09:58:32Z

Updates:

Add IPv4 tests
Fix sanity checks

nashif · 2018-02-13T18:32:17Z

@jukkar any update on this?

rveerama1 · 2018-02-14T08:36:37Z

@nashif It's out of sync now. I am maintaining these patches in #5035. But Websocket requires #6167 (http api change). So api change is not planned for 1.11, these PR's are now interdependent. @jukkar can you close this PR? we can create another one when #6167 is accepted to upstream.

jukkar · 2018-02-14T08:38:17Z

Yes, lets close this one in order to avoid too much confusion.

jukkar added the net label Aug 28, 2017

jukkar self-assigned this Aug 28, 2017

jukkar requested review from mike-scott, pfalcon and tbursztyka August 28, 2017 08:00

jukkar requested a review from dbkinder as a code owner August 28, 2017 08:00

jukkar force-pushed the websocket branch 2 times, most recently from 762c8b3 to 48d4f0c Compare August 28, 2017 15:00

dbkinder reviewed Aug 28, 2017

View reviewed changes

include/net/websocket.h Outdated

Copy link

Contributor

dbkinder Aug 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no parameter "type" or "mask"

dbkinder reviewed Aug 28, 2017

View reviewed changes

dbkinder suggested changes Aug 28, 2017

View reviewed changes

jukkar force-pushed the websocket branch from 48d4f0c to f11e6aa Compare August 29, 2017 13:55

dbkinder reviewed Aug 29, 2017

View reviewed changes

include/net/websocket.h Outdated

Copy link

Contributor

dbkinder Aug 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no parameter "flag"

dbkinder reviewed Aug 29, 2017

View reviewed changes

include/net/websocket.h Outdated

Copy link

Contributor

dbkinder Aug 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing @param for final

dbkinder suggested changes Aug 29, 2017

View reviewed changes

jukkar force-pushed the websocket branch from f11e6aa to 0ea1953 Compare August 29, 2017 17:12

jukkar changed the title ~~[WIP] Initial websocket support~~ Initial websocket support Aug 29, 2017

mike-scott reviewed Aug 29, 2017

View reviewed changes

jukkar force-pushed the websocket branch 4 times, most recently from 5698b2d to 15b16a5 Compare August 30, 2017 13:38

jukkar added this to the v1.10 milestone Aug 30, 2017

jukkar force-pushed the websocket branch 6 times, most recently from 25bb70e to 604a078 Compare September 3, 2017 20:21

pfalcon reviewed Nov 8, 2017

View reviewed changes

include/net/websocket.h Outdated

Copy link

Contributor

pfalcon Nov 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is "to send" expected here? I'd think the function is applied to recved packets.

pfalcon reviewed Nov 8, 2017

View reviewed changes

subsys/net/lib/http/http_app_server.c Outdated

Copy link

Contributor

pfalcon Nov 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work correctly, as mentioned in ws_mask_pkt() prototype review.

pfalcon reviewed Nov 8, 2017

View reviewed changes

pfalcon suggested changes Nov 8, 2017

View reviewed changes

jukkar force-pushed the websocket branch from 1326532 to fa1bacc Compare November 8, 2017 15:47

jukkar removed this from the v1.10.0 milestone Nov 10, 2017

dbkinder reviewed Nov 14, 2017

View reviewed changes

samples/net/ws_echo_server/README.rst Outdated

Copy link

Contributor

dbkinder Nov 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the cmake changes introduced a new way to document build steps

jukkar force-pushed the websocket branch from fa1bacc to 22da220 Compare November 15, 2017 10:55

jukkar requested a review from SebastianBoe as a code owner November 15, 2017 10:55

jukkar added 4 commits November 16, 2017 11:24

net: app: Do not allow local unspecified address in client

4ad5fe9

Do not allow :: or ANY address in client when sending data. Signed-off-by: Jukka Rissanen <[email protected]>

samples: net: websocket echo-server application

f6c9ce4

This is a http(s) server that supports also websocket. It sends back any data sent to it over a websocket. Signed-off-by: Jukka Rissanen <[email protected]>

tests: net: websocket: Add tests for websocket

3215312

This tests websocket by creating a websocket support http server and sending data to it and verifying the returned data is the same. Signed-off-by: Jukka Rissanen <[email protected]>

jukkar force-pushed the websocket branch from 22da220 to 3215312 Compare November 16, 2017 09:57

SebastianBoe removed their request for review November 16, 2017 10:27

jukkar closed this Feb 14, 2018

jukkar mentioned this pull request Mar 5, 2018

Websocket support #6456

Merged

Websocket support #1271

Websocket support #1271

Uh oh!

Conversation

jukkar commented Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbkinder left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbkinder left a comment

Choose a reason for hiding this comment

Uh oh!

jukkar commented Aug 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pfalcon left a comment

jukkar commented Aug 28, 2017 •

edited

Loading