http2: initial support for originSet #17935

jasnell · 2018-01-01T19:27:58Z

In preparation for client side connection Pool and ORIGIN frame support, introduce new properties on Http2Session to identify if the session is secure, provide the alpnProtocol identifier, and generate the initial originSet .

Includes a fix to tls_wrap where the SNI servername was not being set on the client side.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

http2, tls

jasnell · 2018-01-01T19:28:12Z

Ping @nodejs/http2

sebdeckers · 2018-01-02T03:16:35Z

lib/internal/http2/core.js

+
+  get originSet() {
+    return this[kState].originSet ?
+      Array.from(this[kState].originSet) : undefined;


This property's name implies a Set instance yet this returns an Array.

The origin set terminology is adopted from the ORIGIN frame spec. It is a bit confusing. Do you have a suggested alternative?

The name is great; I'm just wondering if it could return the Set (or clone thereof).

"Origin Set" is the name within the spec, I don't think that should determine whether we use Array or Set as the data structure. I don't know if we have any APIs that currently return a Set, I would keep as is.

Returning a Set may imply that it's mutable by the user, which is not the case. Yes, an Array is still mutable, but the property returns a new Array instance on every call. I'd prefer to keep the name and the array return.

mcollina · 2018-01-02T08:33:33Z

doc/api/http2.md

+-->
+
+Value will be `undefined` if the `Http2Session` is not yet connected to a
+socket, `h2c` if the `Http2Session` is not connected to a `TLSSocket`, or


why 'h2c'?

h2c === HTTP/2 over plain text. I'm using it generically because, typically, when using a plain text connection, there's no alpnProtocol property we can use to source it. So if the socket is not a TLSSocket, we assume it's plain text.

mcollina · 2018-01-02T08:34:53Z

lib/internal/http2/core.js

+    this[kAlpnProtocol] = socket.alpnProtocol;
+    this[kEncrypted] = true;
+    const origin =
+      (new URL(`https://${socket.servername}:${socket.remotePort}`)).origin;


Can we avoid parsing a URL for every connected socket?

To my knowlege sockets can be also connected via Unix Domain sockets/windows pipes. There is no remotePort in that case.

Good point. In those cases, there won't be a proper initial origin. I'll update to account for that.

@mcollina ... the ORIGIN spec is pretty clear that the origin must be a validly serialized ASCII-origin, which means our servername must be a pct-encoded ASCII host. The spec also requires that the SNI servername is used to construct the origin. I can see if we can make this more efficient, but I don't think we can completely avoid this.

We could potentially defer the parse such that it occurs the first time the originSet property getter is accessed.

We would potentially defer the parse such that it occurs the first time the originSet property getter is accessed.

I like that idea. 👍

the ORIGIN spec is pretty clear that the origin must be a validly serialized ASCII-origin, which means our servername must be a pct-encoded ASCII host. The spec also requires that the SNI servername is used to construct the origin. I can see if we can make this more efficient, but I don't think we can completely avoid this.

I'm guessing we do not need to generate a new string and then parse it to generate a new pct-encoded ASCII host.

We could potentially defer the parse such that it occurs the first time the originSet property getter is accessed.

+1 with this approach.

Unfortunately, as far as I know, the servername isn't guaranteed to be ASCII or percent encoded so we need to at least pass that through.

Unfortunately, as far as I know, the servername isn't guaranteed to be ASCII or percent encoded so we need to at least pass that through.

can that be cached and filled in from the server?

mcollina · 2018-01-02T08:36:53Z

lib/internal/http2/core.js

+    this[kEncrypted] = true;
+    const origin =
+      (new URL(`https://${socket.servername}:${socket.remotePort}`)).origin;
+    this[kState].originSet = new Set([origin]);


Can you please add a TODO here that the Set will be used by the ORIGIN frame support?

apapirovski

LGTM, just minor nits.

apapirovski · 2018-01-02T15:32:30Z

lib/internal/http2/core.js

+
+  get originSet() {
+    return this[kState].originSet ?
+      Array.from(this[kState].originSet) : undefined;


"Origin Set" is the name within the spec, I don't think that should determine whether we use Array or Set as the data structure. I don't know if we have any APIs that currently return a Set, I would keep as is.

apapirovski · 2018-01-02T15:33:35Z

test/parallel/test-http2-create-client-secure-session.js

+      assert(client.encrypted);
+      assert.strictEqual(client.alpnProtocol, 'h2');
+      const originSet = client.originSet;
+      assert(originSet);


Can this be assert(Array.isArray(originSet))?

apapirovski · 2018-01-02T15:33:45Z

test/parallel/test-http2-create-client-secure-session.js

+  assert(stream.session.encrypted);
+  assert(stream.session.alpnProtocol, 'h2');
+  const originSet = stream.session.originSet;
+  assert(originSet);


Can this be assert(Array.isArray(originSet))?

apapirovski · 2018-01-02T15:40:54Z

lib/internal/http2/core.js

+  }
+
+  get originSet() {
+    return this[kState].originSet ?


Can we just check this[kEncrypted] here? It's slightly more efficient.

apapirovski · 2018-01-02T15:43:02Z

lib/internal/http2/core.js

@@ -717,6 +719,17 @@ function setupHandle(socket, type, options) {

  this[kHandle] = handle;

+  if (socket.encrypted) {
+    this[kAlpnProtocol] = socket.alpnProtocol;
+    this[kEncrypted] = true;


Should we define these on the initial class (as undefined) to avoid thrashing of the hidden class? (Maybe originSet too, on kState.)

jasnell · 2018-01-02T18:24:53Z

Updated. originSet is now initialized on the first call to the originSet getter. The initial origin is only added if socket.servername is not == null. remotePort is only added if socket.remotePort is not == null. We still need to do the URL parse, but it's not automatic on every connection. PTAL

sebdeckers · 2018-01-02T22:43:02Z

lib/internal/http2/core.js

+    if (originSet === undefined) {
+      const socket = this[kSocket];
+      originSet = new Set();
+      if (socket.servername != null) {


Non-strict comparison 😱

;) an intentional one

jasnell · 2018-01-03T00:10:15Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12383/

jasnell · 2018-01-03T00:59:17Z

CI is good, but build bots failed on arm. running again https://ci.nodejs.org/job/node-test-commit-arm/12895/

TimothyGu · 2018-01-03T01:51:16Z

lib/internal/http2/core.js

+    let originSet = this[kState].originSet;
+    if (originSet === undefined) {
+      const socket = this[kSocket];
+      originSet = new Set();


Should this be stored on this[kState]?

heh. looks like an edit didn't get saved.

TimothyGu

One doc nit.

TimothyGu · 2018-01-03T02:12:25Z

doc/api/http2.md

+<!-- YAML
+added: REPLACEME
+-->
+


There should be a type annotation here, like

* {string|undefined}

Ditto below.

TimothyGu · 2018-01-03T13:17:56Z

doc/api/http2.md

@@ -388,6 +392,8 @@ A prototype-less object describing the current local settings of this
 added: REPLACEME
 -->

+* Value: {Array|undefined}


{string[]|undefined}?

Add new properties to `Http2Session` to identify alpnProtocol, and indicator about whether the session is TLS or not, and initial support for origin set (preparinng for `ORIGIN` frame support and the client-side `Pool` implementation. The `originSet` is the set of origins for which an `Http2Session` may be considered authoritative. Per the `ORIGIN` frame spec, the originSet is only valid on TLS connections, so this is only exposed when using a `TLSSocket`.

PR-URL: #17935 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

Add new properties to `Http2Session` to identify alpnProtocol, and indicator about whether the session is TLS or not, and initial support for origin set (preparinng for `ORIGIN` frame support and the client-side `Pool` implementation. The `originSet` is the set of origins for which an `Http2Session` may be considered authoritative. Per the `ORIGIN` frame spec, the originSet is only valid on TLS connections, so this is only exposed when using a `TLSSocket`. PR-URL: #17935 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

jasnell · 2018-01-03T19:31:57Z

Landed in b25b1ef and 060babd

Add new properties to `Http2Session` to identify alpnProtocol, and indicator about whether the session is TLS or not, and initial support for origin set (preparinng for `ORIGIN` frame support and the client-side `Pool` implementation. The `originSet` is the set of origins for which an `Http2Session` may be considered authoritative. Per the `ORIGIN` frame spec, the originSet is only valid on TLS connections, so this is only exposed when using a `TLSSocket`. PR-URL: nodejs#17935 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

Backport-PR-URL: #18050 PR-URL: #17935 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

Add new properties to `Http2Session` to identify alpnProtocol, and indicator about whether the session is TLS or not, and initial support for origin set (preparinng for `ORIGIN` frame support and the client-side `Pool` implementation. The `originSet` is the set of origins for which an `Http2Session` may be considered authoritative. Per the `ORIGIN` frame spec, the originSet is only valid on TLS connections, so this is only exposed when using a `TLSSocket`. Backport-PR-URL: #18050 PR-URL: #17935 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

Notable change: * async_hooks: - deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither API were documented. (Andreas Madsen) #16972 * deps: - update nghttp2 to 1.29.0 (James M Snell) #17908 - upgrade npm to 5.6.0 (Kat Marchán) #17535 - cherry-pick 50f7455 from upstream V8 (Michaël Zasso) #16591 * events: - remove reaches into _events internals (Anatoli Papirovski) #17440 * http: - add rawPacket in err of `clientError` event (XadillaX) #17672 * http2: - implement maxSessionMemory (James M Snell) #17967 - add initial support for originSet (James M Snell) #17935 - add altsvc support (James M Snell) #17917 - perf_hooks integration (James M Snell) #17906 * net: - remove Socket.prototype.write (Anna Henningsen) #17644 - remove Socket.prototype.listen (Ruben Bridgewater) #13735 * repl: - show lexically scoped vars in tab completion (Michaël Zasso) #16591 * stream: - rm {writeable/readable}State.length (Calvin Metcalf) #12857 - add flow and buffer properties to streams (Calvin Metcalf) #12855 * util: - allow wildcards in NODE_DEBUG variable (Tyler) #17609 * zlib: - add ArrayBuffer support (Jem Bezooyen) #16042 * Addedew collaborator** - [starkwang](https://github.com/starkwang) Weijia Wang * Addedew TSC member** - [danbev](https://github.com/danbev) Daniel Bevenius PR-URL: #18069

Backport-PR-URL: #18050 PR-URL: #17935 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

Add new properties to `Http2Session` to identify alpnProtocol, and indicator about whether the session is TLS or not, and initial support for origin set (preparinng for `ORIGIN` frame support and the client-side `Pool` implementation. The `originSet` is the set of origins for which an `Http2Session` may be considered authoritative. Per the `ORIGIN` frame spec, the originSet is only valid on TLS connections, so this is only exposed when using a `TLSSocket`. Backport-PR-URL: #18050 PR-URL: #17935 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

Notable change: * async_hooks: - deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither API were documented. (Andreas Madsen) #16972 * deps: - update nghttp2 to 1.29.0 (James M Snell) #17908 - upgrade npm to 5.6.0 (Kat Marchán) #17535 - cherry-pick 50f7455 from upstream V8 (Michaël Zasso) #16591 * events: - remove reaches into _events internals (Anatoli Papirovski) #17440 * http: - add rawPacket in err of `clientError` event (XadillaX) #17672 * http2: - implement maxSessionMemory (James M Snell) #17967 - add initial support for originSet (James M Snell) #17935 - add altsvc support (James M Snell) #17917 - perf_hooks integration (James M Snell) #17906 - Refactoring and cleanup of Http2Session and Http2Stream destroy (James M Snell) #17406 * net: - remove Socket.prototype.write (Anna Henningsen) #17644 - remove Socket.prototype.listen (Ruben Bridgewater) #13735 * repl: - show lexically scoped vars in tab completion (Michaël Zasso) #16591 * stream: - rm {writeable/readable}State.length (Calvin Metcalf) #12857 - add flow and buffer properties to streams (Calvin Metcalf) #12855 * util: - allow wildcards in NODE_DEBUG variable (Tyler) #17609 * zlib: - add ArrayBuffer support (Jem Bezooyen) #16042 * Addedew collaborator** - [starkwang](https://github.com/starkwang) Weijia Wang * Addedew TSC member** - [danbev](https://github.com/danbev) Daniel Bevenius PR-URL: #18069

Backport-PR-URL: nodejs#18050 PR-URL: nodejs#17935 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

Add new properties to `Http2Session` to identify alpnProtocol, and indicator about whether the session is TLS or not, and initial support for origin set (preparinng for `ORIGIN` frame support and the client-side `Pool` implementation. The `originSet` is the set of origins for which an `Http2Session` may be considered authoritative. Per the `ORIGIN` frame spec, the originSet is only valid on TLS connections, so this is only exposed when using a `TLSSocket`. Backport-PR-URL: nodejs#18050 PR-URL: nodejs#17935 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

Backport-PR-URL: #18050 Backport-PR-URL: #20456 PR-URL: #17935 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

Add new properties to `Http2Session` to identify alpnProtocol, and indicator about whether the session is TLS or not, and initial support for origin set (preparinng for `ORIGIN` frame support and the client-side `Pool` implementation. The `originSet` is the set of origins for which an `Http2Session` may be considered authoritative. Per the `ORIGIN` frame spec, the originSet is only valid on TLS connections, so this is only exposed when using a `TLSSocket`. Backport-PR-URL: #18050 Backport-PR-URL: #20456 PR-URL: #17935 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

nodejs-github-bot added dont-land-on-v4.x http2 Issues or PRs related to the http2 subsystem. tls Issues and PRs related to the tls subsystem. labels Jan 1, 2018

jasnell mentioned this pull request Jan 1, 2018

[Tracking Issue]: Some http2 todos #17746

Closed

11 tasks

sebdeckers reviewed Jan 2, 2018

View reviewed changes

mcollina reviewed Jan 2, 2018

View reviewed changes

apapirovski approved these changes Jan 2, 2018

View reviewed changes

jasnell force-pushed the http2-originset branch from c48aeaa to 3c18eda Compare January 2, 2018 17:46

sebdeckers suggested changes Jan 2, 2018

View reviewed changes

sebdeckers approved these changes Jan 2, 2018

View reviewed changes

TimothyGu reviewed Jan 3, 2018

View reviewed changes

TimothyGu approved these changes Jan 3, 2018

View reviewed changes

doc/api/http2.md



Copy link

Member

TimothyGu Jan 3, 2018

Choose a reason for hiding this comment

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

There should be a type annotation here, like

* {string|undefined}

Ditto below.

TimothyGu reviewed Jan 3, 2018

View reviewed changes

jasnell force-pushed the http2-originset branch from 34487a0 to 073e5ea Compare January 3, 2018 17:02

jasnell added 2 commits January 3, 2018 09:02

tls: set servername on client side too

c0e4945

jasnell force-pushed the http2-originset branch from 073e5ea to 816964a Compare January 3, 2018 17:02

jasnell added a commit that referenced this pull request Jan 3, 2018

tls: set servername on client side too

b25b1ef

PR-URL: #17935 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

jasnell closed this Jan 3, 2018

MylesBorins mentioned this pull request Jan 10, 2018

v9.4.0 proposal #18069

Merged

MylesBorins mentioned this pull request May 2, 2018

v8.11.2 proposal #20478

Merged

drewswanner mentioned this pull request Oct 11, 2020

Incorrectly computes HTTP/2 client origin set #35356

Closed

http2: initial support for originSet #17935

http2: initial support for originSet #17935

Conversation

jasnell commented Jan 1, 2018

Checklist

Affected core subsystem(s)

jasnell commented Jan 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski Jan 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell Jan 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski left a comment

Choose a reason for hiding this comment

apapirovski Jan 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Jan 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Jan 3, 2018

jasnell commented Jan 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Jan 3, 2018

apapirovski Jan 2, 2018 •

edited

Loading

jasnell Jan 2, 2018 •

edited

Loading

apapirovski Jan 2, 2018 •

edited

Loading