feat(rumqttd): Support for websocket connections

[Nickztar]

I have been trying out the broker for a short while now and one feature that seems to be missing is the websocket support. This PR is a attempt at solving that. It currently is not 100% and will try to get it working smoothly. The project did previously have a websocket implementation for which I am unsure of the purple. Those parts have been slightly changed/commented for now. The current implementation in this PR works by wrapping the websocket connection and forwarding it to the the v3.1.1 protocol. Adding support for v5 should not be difficult. Please let me know your thoughts on this and any feedback/change is welcomed.

Type of change

New feature (non-breaking change which adds functionality)

Checklist:

• Formatted with cargo fmt
• Make an entry to CHANGELOG.md if its relevant of user of the library. If its not relevant mention why.

[swanandx]

Thank you so much for the PR 🚀 !!
I have added some comments, other than that, it looks good. But I need to try it out myself, so please give me lil time, will get back to you ASAP. ⚡

[Nickztar]

Added better control for TLS. Not the two TLS features of rumqttd configure the websocket TLS aswell. With support for both native and Rustls.

[swanandx]

Thank you for your patience, I did try it out using websocket example with the following change and it seemed to work

let mut mqttoptions = MqttOptions::new(
    "clientId-aSziq39Bp3",
    "ws://localhost:8080/",
    8080,
);

But you mentioned this:

It currently is not 100% and will try to get it working smoothly

would like to know what do you mean by working smoothly

[Nickztar]

That was mostly about TLS and the fact that I could not get my local MQTTX to work against it. Still a bit of a issue with the MQTTX desktop client. The web one works as expected. Will try to debug/test it a bit more later today. TLS works fine now with rumqttc 👍🏼

[Nickztar]

So have now tried on a different computer. MQTTX still doesn't work, getting 2023-06-07T15:48:35.308169Z ERROR rumqttd::server::broker: Remote link error, error: Network(Io(Custom { kind: ConnectionAborted, error: "connection closed by peer" })). Not sure if this is a issue with Rumqttd or not. Also tested with a different client, mainly a .NET one that I use at work and that works like a charm 👍🏼

[Nickztar]

Okey so an update on the MQTTX problem. It seems I forgot to read the spec for how websockets are supposed to work.
Turns out you need to append a correct header for the procotol. Will push the change after a bit of clean up and make sure to add an entry to the change log. 🚀

[swanandx]

That's awesome! GG 🚀

[Nickztar]

One thing to note, rumqttc currently does not validate this header. Might be something to note/look into?

[Nickztar]

All done now. Had some branch issues but should be all good now! 👍🏼

[swanandx]

rumqttc currently does not validate this header. Might be something to note/look into?

Do you mean the websocket subprotocol header? Can you please open an issue with context / other details? Thank you!

[Nickztar]

Yes exactly. Will do!

[h3nill]

@Nickztar can you change the name of feature websockets to websocket to match with rumqttc's feature name.

[Nickztar]

Absolutly. Bothered me aswell though thought it might be out of scope. 👍🏼

[Nickztar]

@henil Changed the feature. Kept the old one but now it just points to the new feature. This will allow users using the previous feature. Might not actually be anyone though?

[h3nill]

yup sounds good to me, we can remove the extra feature after few releases.

[swanandx]

we can remove the extra feature after few releases

Once we announce it and people start to use it, it might just increase the number of people who will get affected when we remove that feature!
So why not remove it and put it in breaking change?

[h3nill]

@swanandx when we release we can explicitly mention that use websocket feature instead of websockets, its for backward compatibility and will be removed in future. because there might be someone already using that feature flag.

[Nickztar]

Both are valid options. I highly doubt a lot of people are using the existing websockets feature but just in case it might be nice to keep it compatible.

[Nickztar]

There is a also a commented out part of what I am pretty sure is the old shadow implementation at https://github.com/Nickztar/rumqtt/blob/340667fd74014de821d64aa372ddb8a8bf2f81a5/rumqttd/src/server/broker.rs#L285

Maybe this should be removed?

[h3nill]

yup, can be removed.

[h3nill]

this is failing to compile when using websocket feature instead of websockets

$ cargo r --bin rumqttd --features websocket

error[E0596]: cannot borrow `root_store` as mutable, as it is not declared as mutable
  --> /home/henil/.local/share/cargo/registry/src/index.crates.io-6f17d22bba15001f/async-tungstenite-0.22.2/src/tokio/rustls.rs:53:29
   |
53 | / ...                   root_store
54 | | ...                       .add(&Certificate(cert.0))
   | |____________________________________________________^ cannot borrow as mutable
   |
help: consider changing this to be mutable
   |
44 |                     let mut root_store = RootCertStore::empty();
   |                         +++

For more information about this error, try `rustc --explain E0596`.
error: could not compile `async-tungstenite` (lib) due to previous error

which is weird because it can only happen when both tokio-rustls-manual-roots and tokio-rustls-native-certs are enabled.

But we are only enabling tokio-rustls-manual-roots here.

---

And even more weirdly this works:

$ cargo r --bin rumqttd --features websocket -p rumqttd

Even if we assume in the case where it fails, cargo trying to use rumqttc's "websocket" feature it still shouldn't error because we are not using tokio-rustls-native-certs there as well.

we are using it here

[Nickztar]

This is probably a problem between rumqttc and rumqttd. If you cd into rumqttd and try that again it will work. Maybe we should change rumqttd to use that same feature as rumqttc is using to prevent this issue from occurring?

[h3nill]

yeah its happening because of rumqttc using tokio-rustls-native-certs, and cargo picking up the features list from rumqttc instead of rumqttd

[h3nill]

@Nickztar any specific reason to use tokio-rustls-manual-roots instead of tokio-rustls-native-certs?

[Nickztar]

Probably had a reason. Will look into it, otherwise I Will change it to use native.

[h3nill]

Okay according to https://doc.rust-lang.org/cargo/reference/features.html#command-line-feature-options the correct way to specify feature for a workspace is package-name/feature

so if you are running it from root of repo:

$ cargo r --bin rumqttd --features rumqttd/websocket

[Nickztar]

Oh interesting. Still feels weird that it fails to compile since we are specifying the binary which it should run.

Could not find any interesting reason to use manual over native. Might be a reason but having them different still feels weird. Changing it back to native-certs.

[h3nill]

Still feels weird that it fails to compile since we are specifying the binary which it should run.

yeah i tried to find if there is already an open issue for it on cargo, but couldn't find it.

[Nickztar]

@henil Should I be worried about checks not passing? Seems to only be for macos and feels unrelated to this change. Previous checks have passed.

[h3nill]

Its one of the test that fails randomly, dont worry about it

[h3nill]

Thank you @Nickztar for all the work 🎉!

[Nickztar]

Thank you guys! @henil @swanandx