http,https: increase server headers timeout by timcosta · Pull Request #30071 · nodejs/node · GitHub

Fixes: #24980
Refs: eb43bc04b1

Increases the default server headers timeout from 40 seconds to 60 seconds in order to not break compatibility with AWS ELBs in the default configuration.

Over the last several months, I have helped no fewer than 20 different groups of people in Slack teams resolve this issue. I believe that this is worth of a change even though it was originally a security fix because this change keeps intact the intended fix, but restores compatibility in the default configuration with any load balancer that prewarms TCP connections and holds them for over 40 seconds, just like AWS ELBs do.

Node in its default configuration should not be broken with AWS ELBs.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Fixes: nodejs#24980 Refs: nodejs@eb43bc04b1

@nodejs/http

Also @nodejs/security since the original introduction of this feature was in a security release

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

LGTM

Seems like a reasonable change to me. But.

I remember thinking at the time that 40 seconds was a good default because lots of load balancers and reverse proxies probably operate with 30 second timeouts (because that's how people work, they're attracted to Schelling points.)

Perhaps then it's a good idea to set the timeout to just over 60 seconds, e.g., 65?

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

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

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

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

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

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

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

Landed in e17403e

Fixes: #24980 Refs: eb43bc04b1 PR-URL: #30071 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com>

Notable Changes: * cli: * add --trace-exit cli option (legendecas) #30516 * http,https: * increase server headers timeout (Tim Costa) #30071 * readline: * update ansi-regex (Ruben Bridgewater) #30907 * promote \_getCursorPos to public api (Jeremy Albright) #30687 * repl: * add completion preview (Ruben Bridgewater) #30907 * util: * add Set and map size to inspect output (Ruben Bridgewater) #30225 * wasi: * require CLI flag to require() wasi module (Colin Ihrig) #30963 PR-URL: #31010

For compatibility with ELB, not only headersTimeout but also server.keepAliveTiemout must be changed from 5s to over 60s, right?

In order to prevent errors due to the race condition of each other's connection timeout, all the connection timeouts inside the ELB must be greater than ELB's idle timeout.

@Ajido actually no, at least not in the context of this PR. This PR handles the fact that the AWS ELBs pre-warm TCP connections to the backing servers. This is not a connection that is subject to keep-alive until later on the connection lifecycle. The headersTimeout affects the connection only before the first request has been made over it. If you know of any other issues you may want to create a proof-of-concept and write them up. I've never seen an issue with keepAlive in the ~7 years i've been running node behind ELBs on AWS.

This is an interesting issue, thought I might summarize it.

The ‘headerstimeout’ was added to prevent a Slowloris kind of attack where an attacker can max out the server’s connections by slowly sending few bytes of the headers at an interval thus keeping the connection “active” and drain resources. The default for ‘headerstimeout’ was set to 40s when this was first implemented.

Clients that prewarms or pool connections (e.g. reverse proxies) Might get an error if their client idle timeout is greater than the ’headerstimeout’. The connection will be terminated as a result after the default 40s elapses with no client activity.

Increasing the ‘headerstimeout’ to 60s seems like a good default. However, users might still run into this bug if their reverse proxy uses a higher idle timeout (> 60s) to communicate with a node js backend.

It is important to mention that this problem is specific for clients that pre-warms connections, specially layer 7 proxies. Layer 4 streaming proxies should not run into this bug.

@timcosta could you please manually backport this to v12.x?

This now lands cleanly on v12.x. I'll include it in the next minor.

Fixes: nodejs#24980 Refs: nodejs@eb43bc04b1 PR-URL: nodejs#30071 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com>

Fixes: #24980 Refs: eb43bc04b1 PR-URL: #30071 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com>

Notable changes: * [d609881] - http,https: increase server headers timeout (Tim Costa) #30071 PR-URL: #33197

http,https: increase server headers timeout

7a907d7

Fixes: nodejs#24980 Refs: nodejs@eb43bc04b1

nodejs-github-bot added http Issues or PRs related to the http subsystem. https Issues or PRs related to the https subsystem. labels Oct 22, 2019

addaleax approved these changes Nov 30, 2019

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Nov 30, 2019

mcollina approved these changes Nov 30, 2019

View reviewed changes

trivikr approved these changes Nov 30, 2019

View reviewed changes

cjihrig approved these changes Nov 30, 2019

View reviewed changes

bnoordhuis approved these changes Dec 1, 2019

View reviewed changes

BridgeAR approved these changes Dec 10, 2019

View reviewed changes

Trott approved these changes Dec 14, 2019

View reviewed changes

Trott added the notable-change PRs with changes that should be highlighted in changelogs. label Dec 14, 2019

Trott closed this Dec 14, 2019

MylesBorins mentioned this pull request Dec 17, 2019

v13.5.0 proposal #31010

Merged

targos added baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v12.x labels Jan 14, 2020

codebytere added the backport-requested-v12.x label Mar 14, 2020

MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Apr 3, 2020

targos removed backport-requested-v12.x labels Apr 25, 2020

targos removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Apr 25, 2020

targos added a commit that referenced this pull request May 1, 2020

2020-05-26, Version 12.17.0 'Erbium' (LTS)

d4bda0c

Notable changes: * [d609881] - http,https: increase server headers timeout (Tim Costa) #30071 PR-URL: #33197

targos mentioned this pull request May 2, 2020

v12.17.0 release proposal #33197

Merged

mshustov mentioned this pull request Oct 18, 2020

Missing HTTP status codes for cancelled search requests elastic/kibana#73849

Closed

mshustov mentioned this pull request Nov 16, 2020

Remove temporary workaround for HTTP headers timeout elastic/kibana#83419

Merged

http,https: increase server headers timeout by timcosta · Pull Request #30071 · nodejs/node · GitHub

Uh oh!

Uh oh!

http,https: increase server headers timeout - #30071

http,https: increase server headers timeout#30071timcosta wants to merge 1 commit intonodejs:masternodejs/node:masterfrom timcosta:increase_server_headers_timeouttimcosta/node:increase_server_headers_timeoutCopy head branch name to clipboard

Conversation

timcosta commented Oct 22, 2019

Checklist

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Nov 30, 2019

Uh oh!

nodejs-github-bot commented Nov 30, 2019

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 1, 2019

Uh oh!

nodejs-github-bot commented Dec 10, 2019

Uh oh!

nodejs-github-bot commented Dec 10, 2019

Uh oh!

nodejs-github-bot commented Dec 14, 2019

Uh oh!

nodejs-github-bot commented Dec 14, 2019

Uh oh!

nodejs-github-bot commented Dec 14, 2019

Uh oh!

nodejs-github-bot commented Dec 14, 2019

Uh oh!

Trott commented Dec 14, 2019

Uh oh!

Ajido commented Dec 19, 2019

Uh oh!

timcosta commented Dec 19, 2019

Uh oh!

hnasr commented Jan 28, 2020

Uh oh!

codebytere commented Mar 14, 2020

Uh oh!

targos commented Apr 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

http,https: increase server headers timeout#30071
timcosta wants to merge 1 commit into
nodejs:masterfrom
timcosta:increase_server_headers_timeout