GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — tls, crypto: add ALPN Support by shigeki · Pull Request #2564 · nodejs/node

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bnoordhuis Oct 25, 2015

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

shigeki Oct 26, 2015

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

indutny commented Aug 26, 2015

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

shigeki added this to the 4.0.0 milestone

Aug 26, 2015

shigeki force-pushed the alpn_support branch from 65af1da to 0640d36 Compare

August 27, 2015 03:02

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

indutny reviewed

Aug 27, 2015

View reviewed changes

Comment thread

lib/_tls_wrap.js Outdated

indutny Aug 27, 2015

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

shigeki Aug 27, 2015

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

rvagg force-pushed the master branch from 11c25c2 to ba02bd0 Compare

September 6, 2015 11:55

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

shigeki force-pushed the alpn_support branch from 7b6d4f1 to 9625a82 Compare

October 22, 2015 06:27

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

shigeki commented Oct 22, 2015

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

argon commented Oct 22, 2015

Copy link

Copy Markdown

Contributor

Thanks for the update!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bnoordhuis reviewed

Oct 22, 2015

View reviewed changes

Comment thread

doc/api/tls.markdown Outdated

bnoordhuis Oct 22, 2015

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Space before (.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bnoordhuis mentioned this pull request

Nov 2, 2015

TLS ALPN? #3632

Closed

stevenvachon mentioned this pull request

Nov 2, 2015

Is ALPN supported with this module on io.js? molnarg/node-http2#126

Closed

MylesBorins mentioned this pull request

Apr 1, 2016

Backport of #5706 to v4.x #5983

Closed

MylesBorins added lts-agenda labels

Jan 6, 2017

MylesBorins mentioned this pull request

Jan 6, 2017

Potential Semver Minor Backports nodejs/Release#177

Closed

9 tasks done

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

shigeki commented Jan 14, 2017

Copy link

Copy Markdown

Contributor Author

Okay, I can work it next week.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

shigeki commented Jan 16, 2017

Copy link

Copy Markdown

Contributor Author

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

MylesBorins reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>

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 <[email protected]>

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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>

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 <[email protected]>

MylesBorins mentioned this pull request

Jan 24, 2017

v4.8.0 proposal #10973

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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

This was referenced Sep 12, 2024

doc: add documentation for process.features #54897

Merged

doc: fix history of process.features #54982

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 <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>

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 <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>

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 <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>

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 <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
PR-URL: #54897
Refs: #54295

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

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.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.