websocket: 200x faster generate mask

[tsctx]

The disadvantage is increased memory usage.

benchmark                  time (avg)             (min … max)       p75       p99      p999
------------------------------------------------------------- -----------------------------
• generate
------------------------------------------------------------- -----------------------------
generateMask            15.69 ns/iter      (9.77 ns … 332 ns)  15.14 ns  72.66 ns    180 ns
crypto.randomBytes(4)   3'874 ns/iter   (2'971 ns … 6'260 ns)  4'528 ns  6'086 ns  6'260 ns

summary for generate
  generateMask
   246.99x faster than crypto.randomBytes(4)

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 73.33333% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 94.16%. Comparing base (9f26aff) to head (720e8d7).
Report is 2 commits behind head on main.

Files	Patch %	Lines
lib/web/websocket/frame.js	73.33%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3204      +/-   ##
==========================================
- Coverage   94.19%   94.16%   -0.03%     
==========================================
  Files          90       90              
  Lines       24492    24513      +21     
==========================================
+ Hits        23070    23083      +13     
- Misses       1422     1430       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ronag]

You don't actually need the mask. You can just use a 0 mask. It's only necessary for browsers.

[ronag]

I open end an issue for this over at ws some time ago.

[KhafraDev]

You don't actually need the mask. You can just use a 0 mask. It's only necessary for browsers.

WebSocket is a browser spec, but it's also part of the RFC ;)

   The masking key is a 32-bit value chosen at random by the client.
   When preparing a masked frame, the client MUST pick a fresh masking
   key from the set of allowed 32-bit values.  The masking key needs to
   be unpredictable

[ronag]

Just saying. That's one place you could deviate and gain significant performance with no practical side effects. A zero mask is a noop when applied.

[KhafraDev]

I don't disagree that masking is pretty much useless, but I'd prefer to follow the spec at least until WebSocket is mature. Even then I'd prefer if it was opt-in behavior, similar to ws iirc, maybe via a WebSocketDispatcher or similar.

[Uzlopak]

LGTM

[tsctx]

@mcollina ptal

[tsctx]

@mcollina ptal

[mcollina]

lgtm

[lpinca]

I think that using a non-cryptographically strong random number generator by default is not a wise decision.

1. It violates the specification (the RFC, not the WHATWG spec) that all implementations should follow.
2. It completely defeats the point of masking. Using a 0 mask is more or less the same.
3. It is true that the user has full control over the bytes that are sent over the wire when using the client programmatically, but the client (Node.js) might be embedded in applications that run in the end user devices and that work exactly like a browser.

ws moved in the opposite direction a long time ago (websockets/ws#832).

Be prepared to receive security disclosures about this or add documentation to prevent them.

[tsctx]

Non-encrypted random number generation is for fallback when there is no cryptographic module.
By default, encrypted secure random number generation is performed.

[lpinca]

Oh, sorry, I did not notice that. Out of curiosity, when is the crypto module not used?

[tsctx]

See Document https://nodejs.org/api/crypto.html#determining-if-crypto-support-is-unavailable

[lpinca]

Ok, it's from the Node.js build. I thought undici did something special.