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, crypto: add ALPN Support #2564

Closed
wants to merge 2 commits into from
Closed

Conversation

shigeki
Copy link
Contributor

@shigeki shigeki commented Aug 26, 2015

ALPN is added to tls according to RFC7301, which supersedes NPN.
When the server receives both NPN and ALPN extensions from the client, ALPN takes precedence over NPN and the server does not send NPN extension to the client. alpnProtocol in TLSSocket always returns false when no selected protocol exists by ALPN.
In https server, http/1.1 token is always set when no options.ALPNProtocols exists.

Here exist all 16x3 test cases of combination of ALPN and NPN in a server and a client. Tests shows that there are some inconsistent returns in NPN between the server and the client but they are not changed for compatibility.

NPN in Chrome will be deprecated in early 2016 as in http://blog.chromium.org/2015/02/hello-http2-goodbye-spdy-http-is_9.html so it is better to include ALPN support in Node 4.0.

R= @nodejs/crypto

@shigeki shigeki added tls Issues and PRs related to the tls subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels Aug 26, 2015
@@ -1154,6 +1163,12 @@ void SSLWrap<Base>::AddMethods(Environment* env, Handle<FunctionTemplate> t) {
env->SetProtoMethod(t, "setNPNProtocols", SetNPNProtocols);
#endif

#ifdef TLSEXT_TYPE_application_layer_protocol_negotiation
NODE_SET_PROTOTYPE_METHOD(t, "getALPNNegotiatedProtocol",
Copy link
Member

Choose a reason for hiding this comment

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

What if we will always set and declare them, but just leave their implementations empty in case of absent ALPN support?

Copy link
Member

Choose a reason for hiding this comment

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

@shigeki I guess you can leave the #ifdef out. I remember from the last review that it looked like it wouldn't compile with ALPN disabled but maybe I saw that wrong.

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 removed #ifdef here and fixed to have declaration to avoid compile errors and also changed NODE_SET_PROTOTYPE_METHOD macro into env->SetProtoMethod.

@indutny
Copy link
Member

indutny commented Aug 26, 2015

@shigeki this test is awesome, thanks! Some nits, otherwise looking good.

@shigeki shigeki added this to the 4.0.0 milestone Aug 26, 2015
@shigeki
Copy link
Contributor Author

shigeki commented Aug 27, 2015

@indutny Thanks very much for your quick review. I fixed this according your comments and I also found that Test12 was wrong to be duplicated from Test11 and fixed it to the test case of Server: NPN, Client: Nothing.
CI of https://jenkins-iojs.nodesource.com/job/node-test-commit-linux/315/ is all green.
I also made h2 server tests connected from Chrome(boringSSL) and Firefox(nss) with only ALPN and ALPN/NPN and it works fine. Unfortunately I don't have MS Edge so did not test it yet.

Can I land this?

@@ -951,9 +962,10 @@ exports.connect = function(/* [port, host], options, cb */) {
options.host ||
(options.socket && options.socket._host) ||
'localhost',
NPN = {},
NPN = {}, ALPN = {},
Copy link
Member

Choose a reason for hiding this comment

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

Please put it on the next line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I will fix this.

@shigeki shigeki removed this from the 4.0.0 milestone Aug 28, 2015
@shigeki
Copy link
Contributor Author

shigeki commented Aug 28, 2015

The milestone was deleted not to block 4.0 release.

@indutny Sorry, I am in a vacation from today and will work on this on the next Saturday.

@argon
Copy link
Contributor

argon commented Oct 9, 2015

What's the status of this? I may have a future dependency on http2 in a project and no ALPN may be a blocker.

@shigeki
Copy link
Contributor Author

shigeki commented Oct 22, 2015

In Chrome, "Disable HTTP/2 over NPN (with OpenSSL)" https://codereview.chromium.org/1387363004 was landed today. It is better to include this in 5.0

I've rebased this against the latest master and CI is https://ci.nodejs.org/job/node-test-commit/912/ . Something wrong with tests environments is on freebsd and windows and plinux test failures are tick-processor.js and test-debug-args.js that are nothing to do with this PR. Otherwise tests are green.

@indutny Your comments are included in shigeki@9625a82. LGTM?

@shigeki
Copy link
Contributor Author

shigeki commented Oct 22, 2015

@argon Sorry for being late. I've just update this. Thanks for patience.

@argon
Copy link
Contributor

argon commented Oct 22, 2015

Thanks for the update!


<!-- type=misc -->

NPN (Next Protocol Negotiation) and SNI (Server Name Indication) are TLS
ALPN(Application-Layer Protocol Negotiation Extension), NPN (Next
Copy link
Member

Choose a reason for hiding this comment

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

Space before (.

rvagg added a commit to rvagg/io.js that referenced this pull request Oct 29, 2015
Notable changes:

* buffer: (Breaking) Removed both 'raw' and 'raws' encoding types from Buffer,
  these have been deprecated for a long time (Sakthipriyan Vairamani) nodejs#2859.
* console: (Breaking) Values reported by console.time() now have 3 decimals of
  accuracy added (Michaël Zasso) nodejs#3166.
* fs:
  - fs.readFile*(), fs.writeFile*(), and fs.appendFile*() now also accept a file
    descriptor as their first argument (Johannes Wüller) nodejs#3163.
  - (Breaking) In fs.readFile(), if an encoding is specified and the internal
    toString() fails the error is no longer thrown but is passed to the callback
    (Evan Lucas) nodejs#3485.
  - (Breaking) In fs.read() (using the fs.read(fd, length, position, encoding,
    callback) form), if the internal toString() fails the error is no longer
    thrown but is passed to the callback (Evan Lucas) nodejs#3503.
* http:
  - Fixed a bug where pipelined http requests would stall (Fedor Indutny) nodejs#3342.
  - (Breaking) When parsing HTTP, don't add duplicates of the following headers:
    Retry-After, ETag, Last-Modified, Server, Age, Expires. This is in addition
    to the following headers which already block duplicates: Content-Type,
    Content-Length, User-Agent, Referer, Host, Authorization,
    Proxy-Authorization, If-Modified-Since, If-Unmodified-Since, From, Location,
    Max-Forwards (James M Snell) nodejs#3090.
  - (Breaking) The callback argument to OutgoingMessage#setTimeout() must be a
    function or a TypeError is thrown (James M Snell) nodejs#3090.
  - (Breaking) HTTP methods and header names must now conform to the RFC 2616
    "token" rule, a list of allowed characters that excludes control characters
    and a number of separator characters. Specifically, methods and header names
    must now match /^[a-zA-Z0-9_!#$%&'*+.^`|~-]+$/ or a TypeError will be thrown
    (James M Snell) nodejs#2526.
* node:
  - (Breaking) Deprecated the _linklist module (Rich Trott) nodejs#3078.
  - (Breaking) Removed require.paths and require.registerExtension(), both had
    been previously set to throw Error when accessed
    (Sakthipriyan Vairamani) nodejs#2922.
* npm: Upgraded to version 3.3.6 from 2.14.7, see
  https://github.com/npm/npm/releases/tag/v3.3.6 for more details. This is a
  major version bump for npm and it has seen a significant amount of change.
  Please see the original npm v3.0.0 release notes for a list of major changes
  (Rebecca Turner) nodejs#3310.
* src: (Breaking) Bumped NODE_MODULE_VERSION to 47 from 46, this is necessary
  due to the V8 upgrade. Native add-ons will need to be recompiled
  (Rod Vagg) nodejs#3400.
* timers: Attempt to reuse the timer handle for setTimeout().unref(). This fixes
  a long-standing known issue where unrefed timers would perviously hold
  beforeExit open (Fedor Indutny) nodejs#3407.
* tls:
  - Added ALPN Support (Shigeki Ohtsu) nodejs#2564.
  - TLS options can now be passed in an object to createSecurePair()
    (Коренберг Марк) nodejs#2441.
  - (Breaking) The default minimum DH key size for tls.connect() is now 1024
    bits and a warning is shown when DH key size is less than 2048 bits. This a security consideration to prevent "logjam" attacks. A new minDHSize TLS
    option can be used to override the default. (Shigeki Ohtsu) nodejs#1831.
* util:
  - (Breaking) util.p() was deprecated for years, and has now been removed
    (Wyatt Preul) nodejs#3432.
  - (Breaking) util.inherits() can now work with ES6 classes. This is considered
    a breaking change because of potential subtle side-effects caused by a
    change from directly reassigning the prototype of the constructor using
    `ctor.prototype = Object.create(superCtor.prototype, { constructor: { ... } })`
    to using `Object.setPrototypeOf(ctor.prototype, superCtor.prototype)`
    (Michaël Zasso) nodejs#3455.
* v8: (Breaking) Upgraded to 4.6.85.25 from 4.5.103.35 (Ali Ijaz Sheikh) nodejs#3351.
  - Implements the spread operator, see
    https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_operator
    for further information.
  - Implements new.target, see
    https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/new.target
    for further information.
* zlib: Decompression now throws on truncated input (e.g. unexpected end of
  file) (Yuval Brik) nodejs#2595.

PR-URL: nodejs#3466
rvagg added a commit that referenced this pull request Oct 29, 2015
Notable changes:

* buffer: (Breaking) Removed both 'raw' and 'raws' encoding types from Buffer,
  these have been deprecated for a long time (Sakthipriyan Vairamani) #2859.
* console: (Breaking) Values reported by console.time() now have 3 decimals of
  accuracy added (Michaël Zasso) #3166.
* fs:
  - fs.readFile*(), fs.writeFile*(), and fs.appendFile*() now also accept a file
    descriptor as their first argument (Johannes Wüller) #3163.
  - (Breaking) In fs.readFile(), if an encoding is specified and the internal
    toString() fails the error is no longer thrown but is passed to the callback
    (Evan Lucas) #3485.
  - (Breaking) In fs.read() (using the fs.read(fd, length, position, encoding,
    callback) form), if the internal toString() fails the error is no longer
    thrown but is passed to the callback (Evan Lucas) #3503.
* http:
  - Fixed a bug where pipelined http requests would stall (Fedor Indutny) #3342.
  - (Breaking) When parsing HTTP, don't add duplicates of the following headers:
    Retry-After, ETag, Last-Modified, Server, Age, Expires. This is in addition
    to the following headers which already block duplicates: Content-Type,
    Content-Length, User-Agent, Referer, Host, Authorization,
    Proxy-Authorization, If-Modified-Since, If-Unmodified-Since, From, Location,
    Max-Forwards (James M Snell) #3090.
  - (Breaking) The callback argument to OutgoingMessage#setTimeout() must be a
    function or a TypeError is thrown (James M Snell) #3090.
  - (Breaking) HTTP methods and header names must now conform to the RFC 2616
    "token" rule, a list of allowed characters that excludes control characters
    and a number of separator characters. Specifically, methods and header names
    must now match /^[a-zA-Z0-9_!#$%&'*+.^`|~-]+$/ or a TypeError will be thrown
    (James M Snell) #2526.
* node:
  - (Breaking) Deprecated the _linklist module (Rich Trott) #3078.
  - (Breaking) Removed require.paths and require.registerExtension(), both had
    been previously set to throw Error when accessed
    (Sakthipriyan Vairamani) #2922.
* npm: Upgraded to version 3.3.6 from 2.14.7, see
  https://github.com/npm/npm/releases/tag/v3.3.6 for more details. This is a
  major version bump for npm and it has seen a significant amount of change.
  Please see the original npm v3.0.0 release notes for a list of major changes
  (Rebecca Turner) #3310.
* src: (Breaking) Bumped NODE_MODULE_VERSION to 47 from 46, this is necessary
  due to the V8 upgrade. Native add-ons will need to be recompiled
  (Rod Vagg) #3400.
* timers: Attempt to reuse the timer handle for setTimeout().unref(). This fixes
  a long-standing known issue where unrefed timers would perviously hold
  beforeExit open (Fedor Indutny) #3407.
* tls:
  - Added ALPN Support (Shigeki Ohtsu) #2564.
  - TLS options can now be passed in an object to createSecurePair()
    (Коренберг Марк) #2441.
  - (Breaking) The default minimum DH key size for tls.connect() is now 1024
    bits and a warning is shown when DH key size is less than 2048 bits. This a security consideration to prevent "logjam" attacks. A new minDHSize TLS
    option can be used to override the default. (Shigeki Ohtsu) #1831.
* util:
  - (Breaking) util.p() was deprecated for years, and has now been removed
    (Wyatt Preul) #3432.
  - (Breaking) util.inherits() can now work with ES6 classes. This is considered
    a breaking change because of potential subtle side-effects caused by a
    change from directly reassigning the prototype of the constructor using
    `ctor.prototype = Object.create(superCtor.prototype, { constructor: { ... } })`
    to using `Object.setPrototypeOf(ctor.prototype, superCtor.prototype)`
    (Michaël Zasso) #3455.
* v8: (Breaking) Upgraded to 4.6.85.25 from 4.5.103.35 (Ali Ijaz Sheikh) #3351.
  - Implements the spread operator, see
    https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_operator
    for further information.
  - Implements new.target, see
    https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/new.target
    for further information.
* zlib: Decompression now throws on truncated input (e.g. unexpected end of
  file) (Yuval Brik) #2595.

PR-URL: #3466
@bnoordhuis bnoordhuis mentioned this pull request Nov 2, 2015
@MylesBorins
Copy link
Contributor

@nodejs/crypto I attempted a backport of this but all the tests blew up. This is touching far too much code for me to comfortable backport. Would someone be able to get a one in next week?

@shigeki
Copy link
Contributor Author

shigeki commented Jan 14, 2017

Okay, I can work it next week.

@shigeki
Copy link
Contributor Author

shigeki commented Jan 16, 2017

@MylesBorins I've just submitted the PR of #10831 .

MylesBorins pushed a commit that referenced this pull request Jan 19, 2017
cherry-pick 802a2e7 from v6-staging.

ALPN is added to tls according to RFC7301, which supersedes NPN.
When the server receives both NPN and ALPN extensions from the client,
ALPN takes precedence over NPN and the server does not send NPN
extension to the client. alpnProtocol in TLSSocket always returns
false when no selected protocol exists by ALPN.
In https server, http/1.1 token is always set when no
options.ALPNProtocols exists.

PR-URL: #2564
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2017
cherry-pick 7eee372 from v6-staging.

This fix is to be consistent implementation with ALPN. Tow NPN
protocol data in the persistent memebers move to hidden variables in
the wrap object.

PR-URL: #2564
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
cherry-pick 802a2e7 from v6-staging.

ALPN is added to tls according to RFC7301, which supersedes NPN.
When the server receives both NPN and ALPN extensions from the client,
ALPN takes precedence over NPN and the server does not send NPN
extension to the client. alpnProtocol in TLSSocket always returns
false when no selected protocol exists by ALPN.
In https server, http/1.1 token is always set when no
options.ALPNProtocols exists.

PR-URL: #2564
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
cherry-pick 7eee372 from v6-staging.

This fix is to be consistent implementation with ALPN. Tow NPN
protocol data in the persistent memebers move to hidden variables in
the wrap object.

PR-URL: #2564
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2017
MylesBorins pushed a commit that referenced this pull request Feb 1, 2017
cherry-pick 802a2e7 from v6-staging.

ALPN is added to tls according to RFC7301, which supersedes NPN.
When the server receives both NPN and ALPN extensions from the client,
ALPN takes precedence over NPN and the server does not send NPN
extension to the client. alpnProtocol in TLSSocket always returns
false when no selected protocol exists by ALPN.
In https server, http/1.1 token is always set when no
options.ALPNProtocols exists.

PR-URL: #2564
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Feb 1, 2017
cherry-pick 7eee372 from v6-staging.

This fix is to be consistent implementation with ALPN. Tow NPN
protocol data in the persistent memebers move to hidden variables in
the wrap object.

PR-URL: #2564
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins added a commit that referenced this pull request Feb 21, 2017
Notable Changes:

* child_process: add shell option to spawn() (cjihrig)
  #4598
* crypto:
  * add ALPN Support (Shigeki Ohtsu)
    #2564
  * allow adding extra certs to well-known CAs (Sam Roberts)
    #9139
* deps:
  * v8: expose statistics about heap spaces (Ben Ripkens)
    #4463
* fs: add the fs.mkdtemp() function. (Florian MARGAINE)
  #5333
* process:
  * add `externalMemory` to `process` (Fedor Indutny)
    #9587
  * add process.cpuUsage() (Patrick Mueller)
    #10796
MylesBorins added a commit that referenced this pull request Feb 22, 2017
Notable Changes:

* child_process: add shell option to spawn() (cjihrig)
  #4598
* crypto:
  * add ALPN Support (Shigeki Ohtsu)
    #2564
  * allow adding extra certs to well-known CAs (Sam Roberts)
    #9139
* deps:
  * v8: expose statistics about heap spaces (Ben Ripkens)
    #4463
* fs: add the fs.mkdtemp() function. (Florian MARGAINE)
  #5333
* process:
  * add `externalMemory` to `process` (Fedor Indutny)
    #9587
  * add process.cpuUsage() (Patrick Mueller)
    #10796

PR-URL: #10973
imyller added a commit to imyller/meta-nodejs that referenced this pull request Mar 2, 2017
    Notable Changes:

    * child_process: add shell option to spawn() (cjihrig)
      nodejs/node#4598
    * crypto:
      * add ALPN Support (Shigeki Ohtsu)
        nodejs/node#2564
      * allow adding extra certs to well-known CAs (Sam Roberts)
        nodejs/node#9139
    * deps:
      * v8: expose statistics about heap spaces (Ben Ripkens)
        nodejs/node#4463
    * fs: add the fs.mkdtemp() function. (Florian MARGAINE)
      nodejs/node#5333
    * process:
      * add `externalMemory` to `process` (Fedor Indutny)
        nodejs/node#9587
      * add process.cpuUsage() (Patrick Mueller)
        nodejs/node#10796

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Mar 2, 2017
    Notable Changes:

    * child_process: add shell option to spawn() (cjihrig)
      nodejs/node#4598
    * crypto:
      * add ALPN Support (Shigeki Ohtsu)
        nodejs/node#2564
      * allow adding extra certs to well-known CAs (Sam Roberts)
        nodejs/node#9139
    * deps:
      * v8: expose statistics about heap spaces (Ben Ripkens)
        nodejs/node#4463
    * fs: add the fs.mkdtemp() function. (Florian MARGAINE)
      nodejs/node#5333
    * process:
      * add `externalMemory` to `process` (Fedor Indutny)
        nodejs/node#9587
      * add process.cpuUsage() (Patrick Mueller)
        nodejs/node#10796

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
nodejs-github-bot pushed a commit that referenced this pull request Sep 17, 2024
PR-URL: #54982
Refs: aa0308d
Refs: 9010f5f
Refs: 52a40e0
Refs: b3ef289
Refs: #2564
Refs: #25819
Refs: #27311
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
targos pushed a commit that referenced this pull request Oct 4, 2024
PR-URL: #54982
Refs: aa0308d
Refs: 9010f5f
Refs: 52a40e0
Refs: b3ef289
Refs: #2564
Refs: #25819
Refs: #27311
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
PR-URL: nodejs#54982
Refs: nodejs@aa0308d
Refs: nodejs@9010f5f
Refs: nodejs@52a40e0
Refs: nodejs@b3ef289
Refs: nodejs#2564
Refs: nodejs#25819
Refs: nodejs#27311
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
PR-URL: nodejs#54982
Refs: nodejs@aa0308d
Refs: nodejs@9010f5f
Refs: nodejs@52a40e0
Refs: nodejs@b3ef289
Refs: nodejs#2564
Refs: nodejs#25819
Refs: nodejs#27311
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor PRs that contain new features and should be released in the next minor version. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants