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

tls: allow TLSSocket construction with a list of ALPNProtocols #16655

Closed
wants to merge 1 commit into from
Closed

tls: allow TLSSocket construction with a list of ALPNProtocols #16655

wants to merge 1 commit into from

Conversation

qubyte
Copy link
Contributor

@qubyte qubyte commented Oct 31, 2017

Brings the ALPNProtocols option of TLSSocket into line with the
documentation. i.e. a list of strings for protocols may be used, and
not only a buffer.

Fixes: #16643

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)
  • tls

@nodejs-github-bot nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label Oct 31, 2017
@qubyte
Copy link
Contributor Author

qubyte commented Oct 31, 2017

I have some doubt over the tls.convertALPNProtocols method. For example, if I feed it:

var out = {};
tls.convertALPNProtocols(['h2', 'http/1.1'], out);

I expect out.ALPNProtocols to be a buffer of 0x02h20x08http/1.1. What I get instead is a buffer of \u0002h2\bhttp/1.1. Can someone with familiarity shed some light on the discrepancy?

@qubyte
Copy link
Contributor Author

qubyte commented Oct 31, 2017

Ignore me. I understand now.

lib/_tls_wrap.js Outdated
if (this._tlsOptions.NPNProtocols)
tls.convertNPNProtocols(this._tlsOptions.NPNProtocols, this._tlsOptions);
if (this._tlsOptions.ALPNProtocols)
tls.convertALPNProtocols(this._tlsOptions.ALPNProtocols, this._tlsOptions);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to remove the calls from downstream functions now.

As well, can you make it so (and add a test to that effect) that the user's option object isn't modified? Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, that had been bothering me too.

new tls.TLSSocket(null, {
ALPNProtocols: ['http/1.1'],
NPNProtocols: ['http/1.1']
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean is that options.ALPNProtocols and options.NPNProtocols should still be arrays-of-strings afterwards. Making a copy with Object.assign() should do it.

Would it be possible to flesh out the test somewhat and verify that the options are present in the TLS handhake? You'll need to guard on process.features.tls_alpn and process.features.tls_npn.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got the skeleton of this, but I'm not sure where the TLS handshake itself happens (the docs have me tied in knots a bit). I think I should be looking at the secureConnection event. If that's the case, I need to create a server via the TLSSocket constructor and net.createServer. Am I on the right track?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. If you grep for TLSSocket in test/parallel, you should find some existing examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. That's where I am now (I'll move this test file to that directory too if you've no objections).

});

tlsSocket.on('secure', common.mustCall((...args) => {
// TODO
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bnoordhuis It seems like this event is the one I should be listening for, but it's undocumented. Am I correct? If so, which object/properties do I check to complete the test?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably want 'secureConnect', 'secure' is an older, deprecated version.

Once the connection is established, verify the .alpnProtocol and .npnProtocol properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'secureConnect' is only called when the socket is created using tls.connect (and not using the TLSSocket constructor directly), so that event will never be emitted. However, the code in tls.connect appears to listen for 'secure' and then do some unrelated synchronous work before emitting 'secureConnect'.

Brings the ALPNProtocols option of TLSSocket into line with the
documentation. i.e. a list of strings for protocols may be used, and
not only a buffer.

Fixes: #16643
@qubyte
Copy link
Contributor Author

qubyte commented Nov 1, 2017

@bnoordhuis I reckon this is good for another review now. I was unable to use the secureConnect event since it only occurs when using tls.connect. See also this issue.

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new tls.TLSSocket(null, {
ALPNProtocols: ['http/1.1'],
NPNProtocols: ['http/1.1']
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the idea here that the constructor call should work even if ALPN and NPN are disabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the original test I created for this PR. On master this’ll throw because the constructor expects a buffer (where each option is used). The particular usecase which led to me encountering this issue is a proxy which needs to avoid HTTP/2 (so these options are given without “h2”).

@qubyte
Copy link
Contributor Author

qubyte commented Nov 1, 2017

The CI link indicates a failure (I think), but I’m not sure how to interpret the output...

@apapirovski
Copy link
Member

apapirovski commented Nov 1, 2017

Failures on the CI are unrelated.

Where does this land as far as semverness? The TLSSocket is documented and the "incorrect" behaviour has been around for a while, it's possible there are users relying on it? Or maybe not... (Yeah, no... I'm a dummy.)

@qubyte
Copy link
Contributor Author

qubyte commented Nov 1, 2017

As I understand it this’ll fit in a semver patch. Existing behaviour is preserved, and some cases which threw when they should not have will now behave as documented. In other words, this doesn’t break existing usage (which is also consistent with the documentation).

@apapirovski
Copy link
Member

@qubyte Knew I should've checked the signature for those convert functions... you're absolutely right, you can just ignore me. 😆

@qubyte
Copy link
Contributor Author

qubyte commented Nov 1, 2017

Upon clicking through, the checks below all appear to have passed, but the statuses have not been updated. A known quirk?

@apapirovski
Copy link
Member

Yeah, that's broken... If you're good with build & CI type of stuff, there's an issue and I think they're looking for help nodejs/build#790 :)

@qubyte
Copy link
Contributor Author

qubyte commented Nov 1, 2017

Heh, after years of fighting Jenkins my solution was to not use Jenkins. ;)

@qubyte
Copy link
Contributor Author

qubyte commented Nov 2, 2017

Is there anything remaining to do for this PR?

@apapirovski
Copy link
Member

Landed in 291ff72

@qubyte Thanks for the fix and congrats on becoming a Contributor!

@apapirovski apapirovski closed this Nov 4, 2017
apapirovski pushed a commit that referenced this pull request 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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
@qubyte
Copy link
Contributor Author

qubyte commented Nov 4, 2017

Sweet! Thank you. :)

@qubyte qubyte deleted the alpn-protocols-fix branch November 6, 2017 15:17
cjihrig pushed a commit to cjihrig/node that referenced this pull request Nov 6, 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: nodejs#16655
Fixes: https://github.com/node/issues/16643
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
@cjihrig cjihrig mentioned this pull request Nov 6, 2017
@gibfahn
Copy link
Member

gibfahn commented Nov 14, 2017

Could someone backport this to v8.x-staging? Info is in the guide, you just raise a backport PR.

@MylesBorins
Copy link
Contributor

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

BethGriggs pushed a commit to BethGriggs/node that referenced this pull request Jul 9, 2018
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: nodejs#16655
Fixes: https://github.com/node/issues/16643
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 1, 2018
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.

Backport-PR-URL: #21721
PR-URL: #16655
Fixes: https://github.com/node/issues/16643
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
rvagg pushed a commit that referenced this pull request Aug 16, 2018
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.

Backport-PR-URL: #21721
PR-URL: #16655
Fixes: https://github.com/node/issues/16643
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2018
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 this pull request may close these issues.

ALPNProtocols option of TLSSocket constructor is broken.
7 participants