FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

tls, crypto: add ALPN Support by shigeki · Pull Request #2564 · nodejs/node · GitHub

/ node Public

tls, crypto: add ALPN Support - #2564

Closed
shigeki wants to merge 2 commits into
nodejs:masterfrom
shigeki:alpn_support
ConversationCommits (2)ChecksFiles changed
Closed

tls, crypto: add ALPN Support#2564
shigeki wants to merge 2 commits into
nodejs:masterfrom
shigeki:alpn_support

Conversation

shigeki commented Aug 26, 2015

Copy link
Copy Markdown
Contributor

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 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
indutny reviewed Aug 26, 2015
View reviewed changes
Comment thread src/node_crypto.cc Outdated

indutny Aug 26, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

bnoordhuis Oct 25, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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.

shigeki Oct 26, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Aug 26, 2015

Copy link
Copy Markdown
Member

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

shigeki added this to the 4.0.0 milestone Aug 26, 2015

shigeki commented Aug 27, 2015

Copy link
Copy Markdown
Contributor Author

@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?

indutny reviewed Aug 27, 2015
View reviewed changes
Comment thread lib/_tls_wrap.js Outdated

indutny Aug 27, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please put it on the next line.

shigeki Aug 27, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Okay, I will fix this.

shigeki removed this from the 4.0.0 milestone Aug 28, 2015

shigeki commented Aug 28, 2015

Copy link
Copy Markdown
Contributor Author

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 commented Oct 9, 2015

Copy link
Copy Markdown
Contributor

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 commented Oct 22, 2015

Copy link
Copy Markdown
Contributor Author

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 commented Oct 22, 2015

Copy link
Copy Markdown
Contributor Author

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

argon commented Oct 22, 2015

Copy link
Copy Markdown
Contributor

Thanks for the update!

bnoordhuis reviewed Oct 22, 2015
View reviewed changes
Comment thread doc/api/tls.markdown Outdated

bnoordhuis Oct 22, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Space before (.

bnoordhuis mentioned this pull request Nov 2, 2015
Closed
stevenvachon mentioned this pull request Nov 2, 2015
Closed
MylesBorins mentioned this pull request Apr 1, 2016
Closed
MylesBorins mentioned this pull request Jan 6, 2017
Closed
9 tasks

MylesBorins commented Jan 14, 2017

Copy link
Copy Markdown
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 commented Jan 14, 2017

Copy link
Copy Markdown
Contributor Author

Okay, I can work it next week.

shigeki commented Jan 16, 2017

Copy link
Copy Markdown
Contributor Author

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

MylesBorins pushed a commit that referenced this pull request Jan 19, 2017
tls, crypto: add ALPN Support
1a40f2d 
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
tls,crypto: move NPN protcol data to hidden value
d706c0d 
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
tls, crypto: add ALPN Support
2b91385 
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
tls,crypto: move NPN protcol data to hidden value
db49213 
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 mentioned this pull request Jan 24, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Feb 1, 2017
tls, crypto: add ALPN Support
e750f14 
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
tls,crypto: move NPN protcol data to hidden value
ef547f3 
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
2017-02-21, Version 4.8.0 'Argon' (LTS)
58ab54f 
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
2017-02-21, Version 4.8.0 'Argon' (LTS)
49c5ff4 
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
2017-02-21, Version 4.8.0 'Argon' (LTS)
bddd5cc 
    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
2017-02-21, Version 4.8.0 'Argon' (LTS)
2ee49f0 
    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>
This was referenced Sep 12, 2024
Merged
Merged
nodejs-github-bot pushed a commit that referenced this pull request Sep 17, 2024
doc: fix history of process.features
421977c 
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
doc: fix history of process.features
f2d74a2 
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>
marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request Feb 10, 2025
doc: fix history of process.features
910c30f 
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>
marco-ippolito pushed a commit that referenced this pull request Feb 10, 2025
doc: fix history of process.features
697a392 
PR-URL: #54982
Backport-PR-URL: #56948
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>
PR-URL: #54897
Refs: #54295
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL