Enable compression in OpenSSL and add opt-in certificate compression support for TLS connections#62217
Enable compression in OpenSSL and add opt-in certificate compression support for TLS connections#62217pimterry wants to merge 7 commits into
Conversation
|
Review requested:
|
536ac85 to
b7d557b
Compare
|
Review from @nodejs/crypto also helpful. Although this is a deps change, it's primarily a TLS feature. As noted in the description: the number of changes files is intimidating, but the vast vast majority are autogenerated updates! It's actually a fairly reasonable changeset 😄 |
|
Could you split the |
b7d557b to
aee82a2
Compare
|
@aduh95 Good idea. Now split into three commits: the first reconfigures the OpenSSL build, the 2nd contains all the auto-generated output from that, the 3rd contains the Node changes which use this to enable cert compression. |
|
It would be great to keep this moving, as it's going to have some large & painful conflicts to resolve if there are any other OpenSSL changes in the meantime. Cert compression has some nice benefits, I'd love to get the option in for v26 next month. Any other reviews from @nodejs/crypto or maybe @nodejs/net? |
|
@pimterry this is not part of crypto that i can provide feedback/review for. Sorry. |
dc67a6f to
867a64a
Compare
|
Now updated. I've:
That means there's 4 commits to review:
@jasnell can you take a look at this? This is useful for TLS generally, but specifically solves the large certificates problem for QUIC you're listing in the docs, by enabling certificate compression so we can avoid handshake round trips. This has been sat open for a while, but worth prioritising for QUIC imo. Disabled by default here for normal TLS so it's optional & definitely non-breaking, but once it's merged we could test out enabling it by default in QUIC contexts. |
|
Yeah, I've been meaning to come back to this. Record-level compression has long been actively problematic due to CRIME and similar types of exploits. Certificate-compression should be fine but last I checked openssl was gating both behind the same flag. As long as we're certain that enabling this won't accidentally enable record-level compression, then I'm all for it. |
Signed-off-by: Tim Perry <pimterry@gmail.com> PR-URL: #62217 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This changes enables compression within OpenSSL *without* enabling record compression, so this only affects compression of certificates delivered within the TLS handshake. This certificate compression remains disabled by default for now, but becomes available via the new certificateCompression option in TLS context APIs. Enabling this shrinks handshakes significantly, and also reduces fingerprintability of Node.js client handshakes, as these are enabled in all modern browsers by default. Signed-off-by: Tim Perry <pimterry@gmail.com> PR-URL: #62217 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com>
|
Landed in 21310cb...e52ec44 |
Signed-off-by: Tim Perry <pimterry@gmail.com> PR-URL: #62217 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This changes enables compression within OpenSSL *without* enabling record compression, so this only affects compression of certificates delivered within the TLS handshake. This certificate compression remains disabled by default for now, but becomes available via the new certificateCompression option in TLS context APIs. Enabling this shrinks handshakes significantly, and also reduces fingerprintability of Node.js client handshakes, as these are enabled in all modern browsers by default. Signed-off-by: Tim Perry <pimterry@gmail.com> PR-URL: #62217 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Notable changes: build, doc: * generate node.1 with doc-kit (Aviv Keller) #62044 deps: * (SEMVER-MINOR) update OpenSSL build config to support compression (Tim Perry) #62217 doc: * (SEMVER-MINOR) update `blockList` stability status to release candidate (alphaleadership) #63050 fs: * (SEMVER-MINOR) support caller-supplied readFile() buffers (Matteo Collina) #63634 http: * (SEMVER-MINOR) close pre-request sockets in closeIdleConnections (semimikoh) #63470 loader: * (SEMVER-MINOR) implement package maps (Maël Nison) #62239 net: * (SEMVER-MINOR) support TCP_KEEPINTVL and TCP_KEEPCNT in setKeepAlive (Guy Bedford) #63825 tls: * (SEMVER-MINOR) add certificateCompression option (Tim Perry) #62217 vfs: * (SEMVER-MINOR) dispatch fs/promises to mounted VFS instances (Matteo Collina) #63537 * (SEMVER-MINOR) add minimal node:vfs subsystem (Matteo Collina) #63115 PR-URL: #64058
Notable changes: deps: * (SEMVER-MINOR) update OpenSSL build config to support compression (Tim Perry) #62217 doc: * (SEMVER-MINOR) update `blockList` stability status to release candidate (alphaleadership) #63050 fs: * (SEMVER-MINOR) support caller-supplied `readFile()` buffers (Matteo Collina) #63634 http: * (SEMVER-MINOR) close pre-request sockets in `closeIdleConnections` (semimikoh) #63470 loader: * (SEMVER-MINOR) implement package maps (Maël Nison) #62239 net: * (SEMVER-MINOR) support `TCP_KEEPINTVL` and `TCP_KEEPCNT` in `setKeepAlive` (Guy Bedford) #63825 tls: * (SEMVER-MINOR) add `certificateCompression` option (Tim Perry) #62217 vfs: * (SEMVER-MINOR) dispatch `node:fs/promises` to mounted VFS instances (Matteo Collina) #63537 * (SEMVER-MINOR) add minimal `node:vfs` subsystem (Matteo Collina) #63115 PR-URL: #64058
Notable changes: deps: * (SEMVER-MINOR) update OpenSSL build config to support compression (Tim Perry) #62217 doc: * (SEMVER-MINOR) update `blockList` stability status to release candidate (alphaleadership) #63050 fs: * (SEMVER-MINOR) support caller-supplied `readFile()` buffers (Matteo Collina) #63634 http: * (SEMVER-MINOR) close pre-request sockets in `closeIdleConnections` (semimikoh) #63470 loader: * (SEMVER-MINOR) implement package maps (Maël Nison) #62239 net: * (SEMVER-MINOR) support `TCP_KEEPINTVL` and `TCP_KEEPCNT` in `setKeepAlive` (Guy Bedford) #63825 tls: * (SEMVER-MINOR) add `certificateCompression` option (Tim Perry) #62217 vfs: * (SEMVER-MINOR) dispatch `node:fs/promises` to mounted VFS instances (Matteo Collina) #63537 * (SEMVER-MINOR) add minimal `node:vfs` subsystem (Matteo Collina) #63115 PR-URL: #64058
Signed-off-by: Tim Perry <pimterry@gmail.com> PR-URL: nodejs#62217 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This changes enables compression within OpenSSL *without* enabling record compression, so this only affects compression of certificates delivered within the TLS handshake. This certificate compression remains disabled by default for now, but becomes available via the new certificateCompression option in TLS context APIs. Enabling this shrinks handshakes significantly, and also reduces fingerprintability of Node.js client handshakes, as these are enabled in all modern browsers by default. Signed-off-by: Tim Perry <pimterry@gmail.com> PR-URL: nodejs#62217 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Notable changes: deps: * (SEMVER-MINOR) update OpenSSL build config to support compression (Tim Perry) nodejs#62217 doc: * (SEMVER-MINOR) update `blockList` stability status to release candidate (alphaleadership) nodejs#63050 fs: * (SEMVER-MINOR) support caller-supplied `readFile()` buffers (Matteo Collina) nodejs#63634 http: * (SEMVER-MINOR) close pre-request sockets in `closeIdleConnections` (semimikoh) nodejs#63470 loader: * (SEMVER-MINOR) implement package maps (Maël Nison) nodejs#62239 net: * (SEMVER-MINOR) support `TCP_KEEPINTVL` and `TCP_KEEPCNT` in `setKeepAlive` (Guy Bedford) nodejs#63825 tls: * (SEMVER-MINOR) add `certificateCompression` option (Tim Perry) nodejs#62217 vfs: * (SEMVER-MINOR) dispatch `node:fs/promises` to mounted VFS instances (Matteo Collina) nodejs#63537 * (SEMVER-MINOR) add minimal `node:vfs` subsystem (Matteo Collina) nodejs#63115 PR-URL: nodejs#64058
Signed-off-by: Tim Perry <pimterry@gmail.com> PR-URL: #62217 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This changes enables compression within OpenSSL *without* enabling record compression, so this only affects compression of certificates delivered within the TLS handshake. This certificate compression remains disabled by default for now, but becomes available via the new certificateCompression option in TLS context APIs. Enabling this shrinks handshakes significantly, and also reduces fingerprintability of Node.js client handshakes, as these are enabled in all modern browsers by default. Signed-off-by: Tim Perry <pimterry@gmail.com> PR-URL: #62217 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Notable changes: buffer: * (SEMVER-MINOR) implement blob.textStream() (Matthew Aitken) #64036 deps: * (SEMVER-MINOR) update OpenSSL build config to support compression (Tim Perry) #62217 doc: * (SEMVER-MINOR) update `blockList` stability status to release candidate (alphaleadership) #63050 * mark stream.compose stable (Matteo Collina) #62562 esm: * (SEMVER-MINOR) add `--experimental-import-text` flag (Efe) #62300 fs: * (SEMVER-MINOR) support caller-supplied readFile() buffers (Matteo Collina) #63634 http: * (SEMVER-MINOR) add httpValidation option to configure header value validation (RajeshKumar11) #61597 net: * (SEMVER-MINOR) support TCP_KEEPINTVL and TCP_KEEPCNT in setKeepAlive (Guy Bedford) #63825 stream: * (SEMVER-MINOR) expose ReadableStreamTee (Matteo Collina) #64195 tls: * (SEMVER-MINOR) report negotiated TLS groups (Filip Skokan) #64119 * (SEMVER-MINOR) add certificateCompression option (Tim Perry) #62217 PR-URL: #64654
Signed-off-by: Tim Perry <pimterry@gmail.com> PR-URL: #62217 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This changes enables compression within OpenSSL *without* enabling record compression, so this only affects compression of certificates delivered within the TLS handshake. This certificate compression remains disabled by default for now, but becomes available via the new certificateCompression option in TLS context APIs. Enabling this shrinks handshakes significantly, and also reduces fingerprintability of Node.js client handshakes, as these are enabled in all modern browsers by default. Signed-off-by: Tim Perry <pimterry@gmail.com> PR-URL: #62217 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Notable changes: buffer: * (SEMVER-MINOR) implement `blob.textStream()` (Matthew Aitken) #64036 deps: * (SEMVER-MINOR) update OpenSSL build config to support compression (Tim Perry) #62217 doc: * (SEMVER-MINOR) update `blockList` stability status to release candidate (alphaleadership) #63050 * mark `stream.compose` stable (Matteo Collina) #62562 esm: * (SEMVER-MINOR) add `--experimental-import-text` flag (Efe) #62300 fs: * (SEMVER-MINOR) support caller-supplied `readFile()` buffers (Matteo Collina) #63634 http: * (SEMVER-MINOR) add `httpValidation` option to configure header value validation (RajeshKumar11) #61597 net: * (SEMVER-MINOR) support `TCP_KEEPINTVL` and `TCP_KEEPCNT` in setKeepAlive (Guy Bedford) #63825 perf_hooks: * (SEMVER-MINOR) sample delay per event loop iteration (Pablo Erhard) #62935 stream: * (SEMVER-MINOR) expose `ReadableStreamTee` (Matteo Collina) #64195 tls: * (SEMVER-MINOR) report negotiated TLS groups (Filip Skokan) #64119 * (SEMVER-MINOR) add `certificateCompression` option (Tim Perry) #62217 PR-URL: #64654
Notable changes: buffer: * (SEMVER-MINOR) implement `blob.textStream()` (Matthew Aitken) #64036 deps: * (SEMVER-MINOR) update OpenSSL build config to support compression (Tim Perry) #62217 doc: * (SEMVER-MINOR) update `blockList` stability status to release candidate (alphaleadership) #63050 * mark `stream.compose` stable (Matteo Collina) #62562 esm: * (SEMVER-MINOR) add `--experimental-import-text` flag (Efe) #62300 fs: * (SEMVER-MINOR) support caller-supplied `readFile()` buffers (Matteo Collina) #63634 http: * (SEMVER-MINOR) add `httpValidation` option to configure header value validation (RajeshKumar11) #61597 net: * (SEMVER-MINOR) support `TCP_KEEPINTVL` and `TCP_KEEPCNT` in setKeepAlive (Guy Bedford) #63825 perf_hooks: * (SEMVER-MINOR) sample delay per event loop iteration (Pablo Erhard) #62935 src: * (SEMVER-MINOR) allow empty `--experimental-config-file` (Marco Ippolito) #61610 stream: * (SEMVER-MINOR) expose `ReadableStreamTee` (Matteo Collina) #64195 tls: * (SEMVER-MINOR) report negotiated TLS groups (Filip Skokan) #64119 * (SEMVER-MINOR) add `certificateCompression` option (Tim Perry) #62217 PR-URL: #64654
Signed-off-by: Tim Perry <pimterry@gmail.com> PR-URL: nodejs#62217 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This changes enables compression within OpenSSL *without* enabling record compression, so this only affects compression of certificates delivered within the TLS handshake. This certificate compression remains disabled by default for now, but becomes available via the new certificateCompression option in TLS context APIs. Enabling this shrinks handshakes significantly, and also reduces fingerprintability of Node.js client handshakes, as these are enabled in all modern browsers by default. Signed-off-by: Tim Perry <pimterry@gmail.com> PR-URL: nodejs#62217 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Signed-off-by: Tim Perry <pimterry@gmail.com> PR-URL: #62217 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This changes enables compression within OpenSSL *without* enabling record compression, so this only affects compression of certificates delivered within the TLS handshake. This certificate compression remains disabled by default for now, but becomes available via the new certificateCompression option in TLS context APIs. Enabling this shrinks handshakes significantly, and also reduces fingerprintability of Node.js client handshakes, as these are enabled in all modern browsers by default. Signed-off-by: Tim Perry <pimterry@gmail.com> PR-URL: #62217 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Notable changes: buffer: * (SEMVER-MINOR) implement `blob.textStream()` (Matthew Aitken) #64036 deps: * (SEMVER-MINOR) update OpenSSL build config to support compression (Tim Perry) #62217 doc: * (SEMVER-MINOR) update `blockList` stability status to release candidate (alphaleadership) #63050 * mark `stream.compose` stable (Matteo Collina) #62562 esm: * (SEMVER-MINOR) add `--experimental-import-text` flag (Efe) #62300 fs: * (SEMVER-MINOR) support caller-supplied `readFile()` buffers (Matteo Collina) #63634 http: * (SEMVER-MINOR) add `httpValidation` option to configure header value validation (RajeshKumar11) #61597 net: * (SEMVER-MINOR) support `TCP_KEEPINTVL` and `TCP_KEEPCNT` in setKeepAlive (Guy Bedford) #63825 perf_hooks: * (SEMVER-MINOR) sample delay per event loop iteration (Pablo Erhard) #62935 src: * (SEMVER-MINOR) allow empty `--experimental-config-file` (Marco Ippolito) #61610 stream: * (SEMVER-MINOR) expose `ReadableStreamTee` (Matteo Collina) #64195 tls: * (SEMVER-MINOR) report negotiated TLS groups (Filip Skokan) #64119 * (SEMVER-MINOR) add `certificateCompression` option (Tim Perry) #62217 PR-URL: #64654
OpenSSL allocates the built-in record-compression methods while creating its global context. Clearing the stack drops the only pointers without freeing the entries, so LeakSanitizer reports a 24-byte leak at process shutdown. OpenSSL disables record compression on every new SSL_CTX by default. Node does not expose SSL_CTX_clear_options(), so the extra process-wide clearing is not needed to protect against CRIME. Refs: nodejs#62217 Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>
EDIT: This diff size looks scary, but the vast majority is autogenerated from the build config update. Changes are now split into separate commits, all the auto-generated output is now in the 3rd commit, so the other parts can be reviewed more easily.
Until now, we've fully disabled all compression features in OpenSSL via
no-comp. This PR:certificateCompressionoption for TLS contexts, exposing the OpenSSL API for certificate compression, defaulting to disabled (i.e. no visible change). We might want to enable this by default in future but we can debate that separately, no need to do so immediately I think. The new API throws an error if a non-empty option is passed when using a shared OpenSSL without compression support.There's a lot of changes here but the vast majority (everything in
deps/openssl/config/archs) are auto-generated based on the OpenSSL config changes and the actual source changes are quite simple.This is my first time touching the OpenSSL dep setup directly, so hopefully I've rebuilt it correctly! Extra eyes there very helpful.
Notably you can't actually rebuild directly onI've now added a commit to fix this here, addingmainviamake gen-opensslright now, because/deps/openssl/openssl/test/doesn't exist (removed since #57835). That's fine for OpenSSL dep updates (which pull the whole tree anyway) but it breaks simple regeneration. I just manually included the test dir locally, which seems to have rebuilt OK without changes. I'll open a separate PR to fix that properly later, but there's too many conflicts with the changes in here to open that at the same time.no-teststo the OpenSSL config, you can now runmake gen-opensslto regenerate manually and (nearly, except various timestamps) reproduce this commit yourself.There's also a small test change required to the config for 3 unusual addon tests: on Mac only, directly linked our internal libopenssl.a into a shared library, without the rest of Node, which no longer works (because OpenSSL now references the compression libs). I don't think this is sensible or supported approach, I can't find any examples of this anywhere else, and these tests cover a feature (OpenSSL custom engines) which is deprecated and will be removed in OpenSSL v4 anyway. A small config change here updates them to instead dynamically look up symbols which should give the same result without issues.
Upsides:
Binary size
Since the compression libraries are already included in Node anyway, this is minimal: a standard Node build on my machine is 131MB, and increases 40KB (0.03%) with this addition.
Compression risks in TLS e.g. CRIME
No-comp was originally here in part to disable TLS record compression (compression during the connection itself, broken by CRIME). In this PR we're not enabling that, we're enabling certificate compression (how certs are transferred during the initial handshake only). There's no known vulnerabilities around certificate compression itself AFAIK, and it's enabled in all modern browsers and user-facing backends like NGINX, HAProxy, Envoy, etc so seems like a safe bet. This PR also keeps it disabled by default - it just becomes optionally available.
Record compression here is not enabled by this change, despite removing
no-comp. It's independently disabled in many other ways:node/deps/openssl/openssl/ssl/ssl_lib.c
Lines 4207 to 4215 in 2b74812
node/src/crypto/crypto_util.cc
Lines 165 to 167 in 2b74812
Removing it completely from the OpenSSL build as well was a nice extra protection, but it's not strictly required and blocks other unrelated TLS compression features.