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

http2: support adding never-index header fields #34091

Closed
clshortfuse opened this issue Jun 27, 2020 · 5 comments
Closed

http2: support adding never-index header fields #34091

clshortfuse opened this issue Jun 27, 2020 · 5 comments
Labels
feature request Issues that request new features to be added to Node.js. http2 Issues or PRs related to the http2 subsystem.

Comments

@clshortfuse
Copy link
Contributor

As part of the HTTP2 (and HTTP3) spec, some headers can be sent as Literal Header Field Never Indexed

https://www.rfc-editor.org/rfc/rfc7541.html#section-6.2.3

This can be used for security reasons to avoid CRIME (Compression Ratio Info-leak Made Easy) attacks to expose sensitive information.

Points of interest are:

It'll help diagnose #28632

@jasnell jasnell added the feature request Issues that request new features to be added to Node.js. label Jun 30, 2020
@jasnell
Copy link
Member

jasnell commented Jun 30, 2020

This is definitely something that was considered early on when I did the initial implementation. It is hampered by the fact that we tried to keep the API for headers in http/2 as close as possible to that of http/1, which uses a simple object to express headers... e.g.

{
  ":method": "GET",
  "content-type": "text/plain",
  "abc": ["value 1", "value 2"]
}

The difficulty here becomes: how do we indicate which headers should not be indexed? Should it be specified per-request? Should it be specified separately from the actual header pairs?

One way we could do this could be to allow a list of never-index header names as options whenever headers are specified... e.g.

stream.respond({
  'content-type': 'text/plain',
  'abc': ['value 1', 'value 2']
}, { neverIndex: ['abc'] });

Each method that accepts headers would need to be updated to accept an options object. This seems like the most ergonomic way to go.

@jasnell jasnell added http2 Issues or PRs related to the http2 subsystem. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. mentor-available labels Jun 30, 2020
@addaleax
Copy link
Member

@jasnell Just for context, I am already working on this :)

@clshortfuse
Copy link
Contributor Author

clshortfuse commented Jun 30, 2020

@jasnell Personally, I prefer option #2. Whoops! Realized there's only one option. 😄 It's similar to HTTP1 and makes never-index an "upgraded" option.

But, I think there's a theoretical possibility that needs to be explored. What if you want some headers to be indexed, and some not that are of the same header-field name? You might want to do:

stream.respond({
  'content-type': 'application/json',
  'set-cookie': myArrayOfRegularCookies
}, {
  waitForTrailers: true,
  sensitiveHeaders: {
    'set-cookie': myArrayOfSensitiveCookies
 }
});

PS: I use the term "sensitive" because one of the names the spec refers to them by:

Implementations can also choose to protect sensitive header fields by not compressing them and instead encoding their value as literals.

@addaleax addaleax removed help wanted Issues that need assistance from volunteers or PRs that need help to proceed. mentor-available labels Jun 30, 2020
@jasnell
Copy link
Member

jasnell commented Jun 30, 2020

If we want to go with that kind of approach, a Symbol-based option may be better...

const { senstiveHeaderSymbol } = require('http2');

stream.respond({
  'content-type': 'text/plain',
  'set-cookie': ['a', 'b'],
  [sensitiveHeaderSymbol]: {
    'set-cookie': ['c', 'd']
  }
}, {
  waitForTrailers: true
});

@addaleax
Copy link
Member

Fwiw, I’m taking the Symbol-based approach (because I think information about headers should be passed in the headers object), but I’m not sure if we really want this kind of subdivision – if one of the cookie headers is sensitive, it’s probably okay to mark the others as sensitive as well.

addaleax added a commit to addaleax/node that referenced this issue Jun 30, 2020
Add support for “sensitive”/“never-indexed” HTTP2 headers.

Fixes: nodejs#34091
cjihrig pushed a commit that referenced this issue Jul 23, 2020
Add support for “sensitive”/“never-indexed” HTTP2 headers.

Fixes: #34091

PR-URL: #34145
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
targos pushed a commit that referenced this issue Aug 8, 2021
Add support for “sensitive”/“never-indexed” HTTP2 headers.

Fixes: #34091

PR-URL: #34145
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
BethGriggs pushed a commit that referenced this issue Aug 12, 2021
Add support for “sensitive”/“never-indexed” HTTP2 headers.

Fixes: #34091

PR-URL: #34145
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
MylesBorins pushed a commit that referenced this issue Aug 31, 2021
Add support for “sensitive”/“never-indexed” HTTP2 headers.

Fixes: #34091

PR-URL: #34145
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
foxxyz pushed a commit to foxxyz/node that referenced this issue Oct 18, 2021
Add support for “sensitive”/“never-indexed” HTTP2 headers.

Fixes: nodejs#34091

PR-URL: nodejs#34145
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants