search

mtrudel / bandit

Bandit is a pure Elixir HTTP server for Plug & WebSock applications
MIT License
1.69k stars 82 forks source link

Add config option to disable WebSocket masking #271

Closed crertel closed 11 months ago

crertel commented 11 months ago

This is a bit of a gnarly request, but I can justify it, sorta.

If you're already running SSL for your websockets, the whole masking part of the protocol is a lot less relevant I think.

Would it be possible to add a config mechanism to basically make the mask/2 function in Bandit.Websocket.Frame (here) a no-op? The RFC seems to be protecting against a threat model that doesn't really exist so much if SSL is guaranteed.

Obviously this would only work with clients that are expecting it, but for reducing overhead per-connection and frame it'd be nice--say, for a Raspberry Pi or embedded device.

(here's a link to the original paper, since for all their good talk in the RFC about security they don't mind linking to a page that now seems to be taken over by spam and who knows what else)

alisinabh commented 11 months ago

Hey @crertel,

I would like to add a few notes here. (@mtrudel please feel free to correct this)

Firstly, when talking about reducing overhead we need to consider the impact that we are trying to minimize. I would love to see if you already have benchmarks for something like this. By just skimming though the masking code, I personally really doubt skipping it would bring a lot of improvements here (I might be wrong but again, we can benchmark).

Secondly, I don't think using a secure connection (wss) will eliminate this issue in all cases as the cache might live in your ingress layer and still prune to poisoning.

Finally and most importantly, masking is required part of RFC-6455§5.3 and if you remove it, well, it is no longer called a websocket. Even if you do remove (un)masking in bandit (or any other web server), You also need to implement custom client code which does not mask the data sent to the server which is no longer RFC-6455 compatible.

crertel commented 11 months ago

Yep, I'm aware that this is departing from the spec--for the use-case I've got in mind, that's perfectly acceptable (and why this would be a totally optional thing).

The performance can be an issue on low-end embedded devices where touching all of the bytes and doing additional allocations for the mask is expensive. On desktop/server x86/x64/ARM, it's basically a non-issue.

You're correct that the resulting thing isn't an RFC-compliant websockets implementation and that it would be a weird thing for a niche usecase--that deviation from the standard is a feature, not a bug.

mtrudel commented 11 months ago

Back when I was doing Bandit's last perf workup (the 0.6.x series) I looked at this in some detail. Masking and UTF-8 validation were by far the two biggest contributors to WebSocket runtime, and both are unavoidable in practice (recall that Bandit's literal prime directive is correctness). @moogle19 and I ended up getting some decent wins here and here, among others. All to say that the perf cost of masking is something I'm well aware of. Even to the point of making UTF-8 validation optional.

That being said, I don't think your ask is something I want to bring into the library. To me, this is different than UTF-8 validation because it fundamentally changes the wire protocol to not be RFC compliant (in contrast to UTF-8 validation which just removed a validation but still presumed a compliant client). This may be for valid reasons as you point out, but they really aren't ones that are universally applicable.

I see a couple of paths forward though:

  1. @moogle19's early work on bandit-native support of NIF'ing hot paths would presumably tackle masking right off of the gate. This would necessitate a way to DI a masking implementation, meaning you could do this yourself nearly trivially.
  2. If there were a matching client & compelling use case layered on top I'd be more inclined to entertain this (mind, with some sort of negotiation on top (note that using extensions for this would REQUIRE a registered extension name per RFC6455§9.1).

I hope this is a worthy explanation of my rationale. I'm going close this issue for hygiene, but in the meantime if you're looking to push this forward I'd say that checking in with @moogle19 to see what plans he has for bandit-native is a logical next step.

Thanks for the issue!

crertel commented 11 months ago

Thank you! If we could DI a masking implementation, that'd be great. I'm curious about the native stuff as well. Thanks again! :)