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

ALPNProtocols option of TLSSocket constructor is broken. #16643

Closed
qubyte opened this issue Oct 31, 2017 · 7 comments
Closed

ALPNProtocols option of TLSSocket constructor is broken. #16643

qubyte opened this issue Oct 31, 2017 · 7 comments
Assignees
Labels
tls Issues and PRs related to the tls subsystem.

Comments

@qubyte
Copy link
Contributor

qubyte commented Oct 31, 2017

  • Version: v8.8.1
  • Platform: Darwin sprite.local 17.0.0 Darwin Kernel Version 17.0.0: Thu Aug 24 21:48:19 PDT 2017; root:xnu-4570.1.46~2/RELEASE_X86_64 x86_64
  • Subsystem: _tls_wrap.js

Documentation for the ALPNProtocols option (found here) indicates that an array of strings should be acceptable:

\\ Broken
var socket = new tls.TLSSocket(null, { ALPNProtocols: ['http/1.1'] });

However, the example above produces this error:

TypeError: Must give a Buffer as first argument
    at TLSSocket._init (_tls_wrap.js:516:9)
    at new TLSSocket (_tls_wrap.js:304:8)
    at repl:1:14
    at ContextifyScript.Script.runInThisContext (vm.js:50:33)
    at REPLServer.defaultEval (repl.js:240:29)
    at bound (domain.js:301:14)
    at REPLServer.runBound [as eval] (domain.js:314:12)
    at REPLServer.onLine (repl.js:441:10)
    at emitOne (events.js:120:20)
    at REPLServer.emit (events.js:210:7)

From which I inferred that I had to use a buffer:

\\ Working
var buffer = Buffer.from([8, ...Buffer.from('http/1.1')]);
var socket = new tls.TLSSocket(null, { ALPNProtocols: buffer });

If this is indeed a documentation bug, I'd be happy to open a pull request. I'll take a shot at a code bug if the issue is determined to be there instead.

@mscdex mscdex added the tls Issues and PRs related to the tls subsystem. label Oct 31, 2017
@bnoordhuis
Copy link
Member

It sounds like the constructor should call tls.convertALPNProtocols() and tls.convertNPNProtocols() instead of letting tls.connect() and tls.createServer() do that before passing it on to new TLSSocket().

@qubyte
Copy link
Contributor Author

qubyte commented Oct 31, 2017

I wasn't aware of these methods, thanks for the tip. I'll probably have to apply them within the TLSSocket constructor though. Do you mean these should be applied before calling the Socket constructor?

@addaleax
Copy link
Member

@qubyte I think it’s more like, the TLSSocket constructor should call them, not userland code.

But for now calling them yourself should be fine as a workaround.

@addaleax addaleax added good first issue Issues that are suitable for first-time contributors. mentor-available labels Oct 31, 2017
@qubyte
Copy link
Contributor Author

qubyte commented Oct 31, 2017

@addaleax Hmm. I think I wasn't making myself clear. I mean to call these within the TLSSocket constructor, as you say. I think we're on the same page though. Do you mind if I assign myself? It'll be a nice way to ease myself into the codebase.

@addaleax
Copy link
Member

@qubyte Yes, I think we’re on the same page. Go for it if you like!

@qubyte
Copy link
Contributor Author

qubyte commented Oct 31, 2017

Thanks! Looks like I can't self-assign though. Would you mind doing that for me please?

@addaleax addaleax removed the good first issue Issues that are suitable for first-time contributors. label Oct 31, 2017
apapirovski referenced this issue Nov 4, 2017
Brings the ALPNProtocols & NPNProtocols options of TLSSocket in line
with the documentation. i.e. an array of strings for protocols may be
used, not only a buffer.

PR-URL: #16655
Fixes: https://github.com/node/issues/16643
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@apapirovski
Copy link
Member

Fixed by 291ff72

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants