Implement experimental websocket client #2552

p12tic · 2024-11-25T15:29:14Z

This PR consists of 2 parts:

general refactoring to make it easier for Websocket client to reuse existing code
implementation of the Websocket client itself

It probably makes sense to land the general refactorings in smaller PRs once Websocket client is approved in principle.

One way that the websocket client differs from what is prescribed by RFC 6455 is that the client masking key is not random. The practical reason for that is to avoid needing to pull randomization infrastructure into the Websocket client. This would need additional APIs to control, as it makes sense to turn off randomization in unit tests.

Below is an excerpt from commit description on why this is reasonable solution

https://datatracker.ietf.org/doc/html/rfc6455#section-5.3 requires that
the masking key must be unpredictable and derived from a strong source
of entropy. This requirement arose due to usage of WebSocket protocol in
the browsers, where both server and client payload generation code may
be malicious and the only trusted piece is the browser itself.
Consequently there was a need to make the bytes on the wire
unpredictable for the client code, so that it cannot run attacks against
intermediate proxies that do not understand WebSocket.

In the case of Seastar there is no security boundary between payload
generator and payload serializer in this class, accordingly in terms of
security impact it is sufficient to simply use predictable masking key.
Zero is chosen because it does not change the payload.

Websocket protocol handshake uses HTTP, thus it makes sense to reuse at least some of the abstractions and API style.

This will allow to implement Websocket client that reuses existing code.

Current implementation of websocket connection is specific to the server. Rename to more appropriate name.

Websocket connection handling is mostly not specific to the server and thus can be reused on the client easily.

Previous name was useful when it's used locally in translation unit. The new name is exposed in a header and should be more unique.

https://datatracker.ietf.org/doc/html/rfc6455#section-5.3 requires that the masking key must be unpredictable and derived from a strong source of entropy. This requirement arose due to usage of WebSocket protocol in the browsers, where both server and client payload generation code may be malicious and the only trusted piece is the browser itself. Consequently there was a need to make the bytes on the wire unpredictable for the client code, so that it cannot run attacks against intermediate proxies that do not understand WebSocket. In the case of Seastar there is no security boundary between payload generator and payload serializer in this class, accordingly in terms of security impact it is sufficient to simply use predictable masking key. Zero is chosen because it does not change the payload.

xemul · 2024-11-28T08:32:36Z

include/seastar/websocket/server.hh

 #include <seastar/core/when_all.hh>
+#include <seastar/websocket/common.hh>


So far only the parser is in there, so it makes sense to name it parser.hh

xemul · 2024-11-28T08:36:02Z

include/seastar/websocket/common.hh

+    /*!
+     * \param fd established socket used for communication
+     */
+    connection(connected_socket&& fd)


Shouldn't it be protected as well?

xemul · 2024-11-28T08:38:39Z

include/seastar/websocket/common.hh

@@ -243,16 +243,19 @@ protected:

    sstring _subprotocol;
    handler_t _handler;
+    bool _is_client;


It seems that std::optional<uint32_t> _masking_key would fit better here. Constructor and send_key() would need to be updated accordingly

xemul · 2024-11-28T08:39:24Z

src/websocket/common.cc

@@ -168,6 +172,26 @@ future<> connection::send_data(opcodes opcode, temporary_buffer<char>&& buff) {
        header[1] = uint8_t(buff.size());
    }

+    temporary_buffer<char> write_buf;


Unused in this patch. Maybe it's used later, but then it should as well be added "later"

xemul · 2024-11-28T08:40:07Z

include/seastar/websocket/common.hh

@@ -87,7 +87,7 @@ struct frame_header {
    }


Patch is missing descriptive comment

xemul · 2024-11-28T08:40:39Z

include/seastar/http/request.hh

@@ -313,10 +313,10 @@ struct request {
     */


What for? Should be in the patch comment

xemul · 2024-11-28T08:40:55Z

CMakeLists.txt

@@ -777,6 +777,7 @@ add_library (seastar
  src/util/read_first_line.cc


Patch comment is missing

xemul · 2024-11-28T08:41:32Z

include/seastar/websocket/client.hh

+     *
+     * \param addr -- host address to connect to
+     * \param creds -- credentials
+     * \param host -- optional host name


optional local host name ?

xemul · 2024-11-28T08:44:10Z

CMakeLists.txt

@@ -777,6 +777,7 @@ add_library (seastar
  src/util/read_first_line.cc
  src/util/tmp_file.cc
  src/util/short_streams.cc
+  src/websocket/client.cc


Test is missing as well

xemul · 2024-11-28T08:45:34Z

include/seastar/websocket/client.hh

+     */
+    future<> send_message(temporary_buffer<char> buf, bool flush);
+
+protected:


Why no private? Are there plans to implement more specific connections inherited from this class?

xemul · 2024-11-28T08:47:14Z

src/websocket/client.cc

+
+            auto con = seastar::make_shared<client_connection>(*this, std::move(cs), ws_key, handler);
+
+            auto sub = as ? as->subscribe([con] () noexcept { con->shutdown_input(); }) : std::nullopt;


This sub dies immediately after con->perform_handshake() returns, even if its future doesn't resolve immediately (likely it doesn't)

p12tic added 13 commits November 25, 2024 17:13

http: Expose connection_factory implementations

c440009

Websocket protocol handshake uses HTTP, thus it makes sense to reuse at least some of the abstractions and API style.

websocket: Extract code not specific to server to separate file

453a5ae

This will allow to implement Websocket client that reuses existing code.

websocket: Rename connection to server_connection

008c4bc

Current implementation of websocket connection is specific to the server. Rename to more appropriate name.

websocket: Extract parts of server_connection usable for client

65e1e75

Websocket connection handling is mostly not specific to the server and thus can be reused on the client easily.

websocket: Rename wlogger to websocket_logger

cd8153b

Previous name was useful when it's used locally in translation unit. The new name is exposed in a header and should be more unique.

websocket: extract encode_base64() function

44a20aa

websocket: Add support for parsing messages coming from server

793c5fb

http: Make request writing functions public

6f1d5ae

websocket: Add experimental client implementation

05e928f

demos: Add a way to set port from cmd line in websocket demo

4dcc838

demos: Rename websocket demo to websocket_server demo

12415c2

demos: Add websocket client demo

ec8a235

xemul reviewed Nov 28, 2024

View reviewed changes

include/seastar/websocket/common.hh

@@ -87,7 +87,7 @@ struct frame_header {

}

Copy link

Contributor

xemul Nov 28, 2024

Choose a reason for hiding this comment

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

Patch is missing descriptive comment

xemul reviewed Nov 28, 2024

View reviewed changes

include/seastar/http/request.hh

@@ -313,10 +313,10 @@ struct request {

*/

Copy link

Contributor

xemul Nov 28, 2024

Choose a reason for hiding this comment

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

What for? Should be in the patch comment

xemul reviewed Nov 28, 2024

View reviewed changes

CMakeLists.txt

@@ -777,6 +777,7 @@ add_library (seastar

src/util/read_first_line.cc

Copy link

Contributor

xemul Nov 28, 2024

Choose a reason for hiding this comment

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

Patch comment is missing

xemul reviewed Nov 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement experimental websocket client #2552

Implement experimental websocket client #2552

p12tic commented Nov 25, 2024

xemul Nov 28, 2024

xemul Nov 28, 2024

xemul Nov 28, 2024

xemul Nov 28, 2024

xemul Nov 28, 2024

xemul Nov 28, 2024

xemul Nov 28, 2024

xemul Nov 28, 2024

xemul Nov 28, 2024

xemul Nov 28, 2024

xemul Nov 28, 2024

		#include <seastar/core/when_all.hh>
		#include <seastar/websocket/common.hh>

		@@ -777,6 +777,7 @@ add_library (seastar
		src/util/read_first_line.cc


		auto con = seastar::make_shared<client_connection>(*this, std::move(cs), ws_key, handler);

		auto sub = as ? as->subscribe([con] () noexcept { con->shutdown_input(); }) : std::nullopt;

Implement experimental websocket client #2552

Are you sure you want to change the base?

Implement experimental websocket client #2552

Conversation

p12tic commented Nov 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment