Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to specify the masking key #1989

Closed
wants to merge 1 commit into from
Closed

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Dec 19, 2021

Refs: #1986
Refs: #1988

@lpinca
Copy link
Member Author

lpinca commented Dec 19, 2021

cc: @ronag @e3dio

@lpinca
Copy link
Member Author

lpinca commented Dec 19, 2021

There are two issues with this

  1. There is no way to provide a masking key for ws.close().

  2. The user provided masking key is not copied. If data is buffered and the user mutates the Buffer's contents, then the masking key might not be the expected one. For example:

    const mask = Buffer.alloc(4);
    
    mask.fill('a');
    
    ws.send(message, { mask });
    
    mask.fill('b');
    
    ws.send(message, { mask });
    
    mask.fill('b');

    In this case, all buffered messages will be masked with the same masking key. I'm not sure if it is better to document this or copy the user provided key. What do you think?

@e3dio e3dio mentioned this pull request Dec 19, 2021
@lpinca
Copy link
Member Author

lpinca commented Dec 19, 2021

Another possible alternative, which might be simpler, could be a randomMask/randomFill? option on the WebSocket constructor. This option specifies a user provided function to fill the mask buffer.

const ws = new WebSocket(url, {
  randomMask(mask) {
    // Do nothing. 
  }
})

It would not have a per message granularity, but I think it is not required.

@ronag
Copy link
Contributor

ronag commented Dec 19, 2021

I think copying should also be fine. It’s 4 bytes.

@e3dio
Copy link

e3dio commented Dec 19, 2021

Another possible alternative, which might be simpler could be an option on the WebSocket constructor

This is a good idea, I would not want to require adding mask details to all my ws.send() calls individually, a new WebSocket(url, { options }) mask option would be better, a function that takes the mask buffer as parameter would be good:

const ws = new WebSocket(url, {
  maskBuffer: buf => require('crypto').randomFillSync(buf) // default function
})

const ws = new WebSocket(url, {
  maskBuffer: buf => buf.writeUInt32BE(Math.random()*2**32) // Math.random example
})

const ws = new WebSocket(url, {
  maskBuffer: Buffer.alloc(4) // clear mask example
})

maskBuffer can take a Buffer or a Function: if a Buffer is provided it is used for the masks, if a Function the function is called to update the mask on every send

@ronag
Copy link
Contributor

ronag commented Dec 19, 2021

I’m good with either option.

@e3dio
Copy link

e3dio commented Dec 19, 2021

FYI @alexhultman added clear mask checking on already optimized uWS server uNetworking/uWebSockets@0c8be4d and is reporting 10% performance improvement for large messages which is pretty good so it's ready and compatible with this PR, and you will probably see better improvement for this Node client which currently does crypto and bit xor

lpinca added a commit that referenced this pull request Dec 19, 2021
The `generateMask` option specifies a function that can be used to
generate custom masking keys.

Refs: #1986
Refs: #1988
Refs: #1989
lpinca added a commit that referenced this pull request Dec 19, 2021
The `generateMask` option specifies a function that can be used to
generate custom masking keys.

Refs: #1986
Refs: #1988
Refs: #1989
@lpinca
Copy link
Member Author

lpinca commented Dec 19, 2021

See #1990.

lpinca added a commit that referenced this pull request Dec 20, 2021
The `generateMask` option specifies a function that can be used to
generate custom masking keys.

Refs: #1986
Refs: #1988
Refs: #1989
lpinca added a commit that referenced this pull request Dec 20, 2021
The `generateMask` option specifies a function that can be used to
generate custom masking keys.

Refs: #1986
Refs: #1988
Refs: #1989
@lpinca
Copy link
Member Author

lpinca commented Dec 20, 2021

Closing in favor of #1990.

@lpinca lpinca closed this Dec 20, 2021
@lpinca lpinca deleted the use/user-provided-mask branch December 20, 2021 19:31
lpinca added a commit that referenced this pull request Dec 20, 2021
The `generateMask` option specifies a function that can be used to
generate custom masking keys.

Refs: #1986
Refs: #1988
Refs: #1989
th37rose added a commit to th37rose/websocket_server that referenced this pull request May 24, 2022
The `generateMask` option specifies a function that can be used to
generate custom masking keys.

Refs: websockets/ws#1986
Refs: websockets/ws#1988
Refs: websockets/ws#1989
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants