Add websockets TLS support

[0x00002a]

This add support for "secure" websockets. On both the client and server sides. As it stands this is not ready for merging imo. There are a few issues which I think need discussing, and places where the API has been updated non-uniformly to minimise initial clutter in this PR.

Stuff for discussion

1. client::controller has a new run method. This calls start, then runs the io context on the current thread. To prevent this simply blocking forever, the work guard has been moved to only server::controller. The reason for this change was mostly so it was actually testable without having to pass a std::promise down the call stack, however it is even more relevent for more complex programs. asio already does tracking of when there is no more work to be done, we should utalise that. This is an uneven API change, server does not have a run method, imo it should have one, that would remove the need for spinning endlessly at the end of main like in the examples (and my server code currently).
2. The method to make a tls websocket request is called wss_connect, this is at odds with the current make_websocket_connection. Before #17 a template argument was used for this, so I'm guessing the plan was to just keep that. imo it is cleaner to have two methods to mirror the http methods and make_secure_websocket_connection is very long. I am personally fond of changing it to ws_connect but I'm happy to change wss_connect if thats needed/wanted
3. The tests load the TLS keychain from memory, rather than a file. This is so their outcome does not depend on external factors like what path they are run relative to. imo the examples should also use this.

Changes

• The server will now upgrade to TLS websockets if enabled
• The client can make TLS websocket connections via wss_connect
• client::controller now supports adding CA's to the keychain (via add_ca)
• websocket::stream now supports making TLS handshakes
• client::controller has a run() method which causes it to block until there are no more queued actions (see above)

Note that changes should be formatted inline with the .clang-format file, but the CI will fail because the rest of the codebase is not currently.

Feedback is welcome, especially regarding the TLS stuff, I've never done it for websockets before.

Closes: #36

[Tectu]

Great work - thanks for another PR!

To prevent this simply blocking forever, the work guard has been moved to only server::controller.

I don't understand why this is/was necessary. Can you elaborate a bit more?

The method to make a tls websocket request is called wss_connect, this is at odds with the current make_websocket_connection. Before Non string_body requests #17 a template argument was used for this, so I'm guessing the plan was to just keep that. imo it is cleaner to have two methods to mirror the http methods and make_secure_websocket_connection is very long. I am personally fond of changing it to ws_connect but I'm happy to change wss_connect if thats needed/wanted

I am a true sucker for consistency so yeah - I am also pro keeping it similar to the HTTP side of things. Personally I think ws_connect() and wss_connect() are good (better) choices than make_websocket_connection() with the corresponding template parameter. However, semantically it is important to differentiate between "make connection" and "connect". If the function in question only returns a WS connection but doesn't actually connect, it should be make_ws_connection(). If it also connects, it should be ws_connect().

As it stands this is not ready for merging imo.

I didn't properly review this yet but based on your PR description I think that the situation is not that bad? Or am I missing some big "whoopsie"-moment here? :p

[0x00002a]

However, semantically it is important to differentiate between "make connection" and "connect". If the function in question only returns a WS connection but doesn't actually connect, it should be make_ws_connection(). If it also connects, it should be ws_connect().

It does (unlike the server side) do the full handshake; the handler is invoked with a connection ready for io.

I didn't properly review this yet but based on your PR description I think that the situation is not that bad? Or am I missing some big "whoopsie"-moment here? :p

Because theres bits of it which I know need discussion was my point :p

To prevent this simply blocking forever, the work guard has been moved to only server::controller. I don't understand why this is/was necessary. Can you elaborate a bit more?

The work guard ensures that even when the io_context runs out of queued actions it will still not count as "done" and will wait for more. i.e. if there is a work guard active then io_context::run will never return ever. We could disable it directly before calling io_context::run but then there is essentially no point to it, and I can't think of a use case on the client-side for it (not sure if I'm missing something here?).

[Tectu]

Because theres bits of it which I know need discussion was my point :p

There's a chance you'll find me to be an excellent discussion partner :p

Because theres bits of it which I know need discussion was my point :p

mhmmmmm... >__>

[...] and I can't think of a use case on the client-side for it (not sure if I'm missing something here?).

I didn't check out the commits of this PR yet so I might be wrong here - in case of a client, your application might like to setup a controller, make an HTTP request, then do something else, then launch another HTTP request. The I/O context needs to be kept alive for this, no?

[0x00002a]

[..] in case of a client, your application might like to setup a controller, make an HTTP request, then do something else, then launch another HTTP request. The I/O context needs to be kept alive for this, no?

🤦‍♀️ right. In that case I suggest keeping run(), but moving the work guard back up to malloy::controller and just cancelling it before

[Tectu]

Yes, please put it back in. We certainly need that work guard (especially on the client side - I just figured that the overhead is minimal so we can keep the code base as generic as possible on the server side).

[Tectu]

Do you currently still see any reason not to merge this? Something that needs further discussion in your opinion?

[0x00002a]

Not really. The example and the server side stuff can be their own PR/issues.