tls: add min/max protocol version options by sam-github · Pull Request #24405 · nodejs/node

sam-github · 2018-11-16T22:55:59Z

The existing secureProtocol option only allows setting the allowed
protocol to a specific version, or setting it to "all supported
versions". It also used obscure strings based on OpenSSL C API
functions. Directly setting the min or max is easier to use and explain.

This is a rework of tls: add min/max_version and their defaults from https://github.com/shigeki/node/commits/WIP_upgrade_openssl111_tls12_only onto master. The original conflicted with more recent commits to master, but while doing the docs for #24386 I realized it also broke #23814. I'm PRing this directly now because it doesn't have a dependency on OpenSSL 1.1.1. Getting it into master should make @shigeki 's work easier, and his openssl 1.1.1 branch shorter. Also, landing it will stop it from getting more conflicts.

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

nodejs-github-bot · 2018-11-16T22:56:00Z

@sam-github build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1643/pipeline

refack

LGTM % semverity

refack · 2018-11-16T23:08:14Z

Missing test for tls.createSecureContext({minVersion, maxVersion}) (trivial, validation, override by secureProtocol)

bnoordhuis

Just to make sure we're all on the same page: SecureContext is exported. We agree this is semver-minor, not semver-major? Tacking on parameters breaks code that passes in too many.

Sam, can you add a few more tests that verify that passing in the new options actually affects a TLS connection? Bonus points if it also tests the interaction with the --tls-v1.x flags.

sam-github · 2018-11-19T23:08:10Z

@shigeki I removed your name from the commit so you don't get git blamed, I don't think any of your code remains after changes above.

sam-github · 2018-11-19T23:10:11Z

@rvagg @bnoordhuis PTAL

The existing secureProtocol option only allows setting the allowed protocol to a specific version, or setting it to "all supported versions". It also used obscure strings based on OpenSSL C API functions. Directly setting the min or max is easier to use and explain.

Notable Changes: * console,util: * `console` functions now handle symbols as defined in the spec. #23708 * The inspection `depth` default is now back at 2. #24326 * dgram,net: * Added ipv6Only option for `net` and `dgram`. #23798 * http: * Chosing between the http parser is now possible per runtime flag. #24739 * readline: * The `readline` module now supports async iterators. #23916 * repl: * The multiline history feature is removed. #24804 * tls: * Added min/max protocol version options. #24405 * The X.509 public key info now includes the RSA bit size and the elliptic curve. #24358 * url: * `pathToFileURL()` now supports LF, CR and TAB. #23720 * Windows: * Tools are not installed using Boxstarter anymore. #24677 * The install-tools scripts or now included in the dist. #24233 * Added new collaborator: * [antsmartian](https://github.com/antsmartian) - Anto Aravinth. #24655 PR-URL: #24854

The existing secureProtocol option only allows setting the allowed protocol to a specific version, or setting it to "all supported versions". It also used obscure strings based on OpenSSL C API functions. Directly setting the min or max is easier to use and explain. Backport-PR-URL: #24676 PR-URL: #24405 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>

Notable Changes: * console,util: * `console` functions now handle symbols as defined in the spec. #23708 * The inspection `depth` default is now back at 2. #24326 * dgram,net: * Added ipv6Only option for `net` and `dgram`. #23798 * http: * Chosing between the http parser is now possible per runtime flag. #24739 * readline: * The `readline` module now supports async iterators. #23916 * repl: * The multiline history feature is removed. #24804 * tls: * Added min/max protocol version options. #24405 * The X.509 public key info now includes the RSA bit size and the elliptic curve. #24358 * url: * `pathToFileURL()` now supports LF, CR and TAB. #23720 * Windows: * Tools are not installed using Boxstarter anymore. #24677 * The install-tools scripts or now included in the dist. #24233 * Added new collaborator: * [antsmartian](https://github.com/antsmartian) - Anto Aravinth. #24655 PR-URL: #24854

The existing secureProtocol option only allows setting the allowed protocol to a specific version, or setting it to "all supported versions". It also used obscure strings based on OpenSSL C API functions. Directly setting the min or max is easier to use and explain. PR-URL: nodejs#24405 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>

Notable Changes: * console,util: * `console` functions now handle symbols as defined in the spec. nodejs#23708 * The inspection `depth` default is now back at 2. nodejs#24326 * dgram,net: * Added ipv6Only option for `net` and `dgram`. nodejs#23798 * http: * Chosing between the http parser is now possible per runtime flag. nodejs#24739 * readline: * The `readline` module now supports async iterators. nodejs#23916 * repl: * The multiline history feature is removed. nodejs#24804 * tls: * Added min/max protocol version options. nodejs#24405 * The X.509 public key info now includes the RSA bit size and the elliptic curve. nodejs#24358 * url: * `pathToFileURL()` now supports LF, CR and TAB. nodejs#23720 * Windows: * Tools are not installed using Boxstarter anymore. nodejs#24677 * The install-tools scripts or now included in the dist. nodejs#24233 * Added new collaborator: * [antsmartian](https://github.com/antsmartian) - Anto Aravinth. nodejs#24655 PR-URL: nodejs#24854

The existing secureProtocol option only allows setting the allowed protocol to a specific version, or setting it to "all supported versions". It also used obscure strings based on OpenSSL C API functions. Directly setting the min or max is easier to use and explain. Backport-PR-URL: nodejs#24676 PR-URL: nodejs#24405 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>

The existing secureProtocol option only allows setting the allowed protocol to a specific version, or setting it to "all supported versions". It also used obscure strings based on OpenSSL C API functions. Directly setting the min or max is easier to use and explain. Backport-PR-URL: #26270 PR-URL: #24405 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>

sam-github added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 16, 2018

nodejs-github-bot added the lib / src Issues and PRs involving general changes in the lib/ or src/ directories. label Nov 16, 2018

sam-github requested review from bnoordhuis, rvagg and shigeki November 16, 2018 22:56

refack reviewed Nov 16, 2018

View reviewed changes

Comment thread test/parallel/test-https-agent-getname.js Outdated

refack approved these changes Nov 16, 2018

View reviewed changes

refack added the crypto Issues and PRs related to the crypto subsystem. label Nov 16, 2018

vsemozhetbyt reviewed Nov 16, 2018

View reviewed changes

Comment thread doc/api/tls.md Outdated

Comment thread doc/api/tls.md Outdated

Comment thread doc/api/tls.md Outdated

Comment thread doc/api/tls.md Outdated

sam-github force-pushed the tls-min-max-version branch from 77ae2fe to 9b267ff Compare November 17, 2018 02:06

mscdex reviewed Nov 17, 2018

View reviewed changes

Comment thread doc/api/tls.md Outdated

sam-github added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-minor PRs that contain new features and should be released in the next minor version. labels Nov 17, 2018

bnoordhuis reviewed Nov 19, 2018

View reviewed changes

Comment thread doc/api/tls.md Outdated

Comment thread src/node_crypto.cc Outdated

Comment thread test/parallel/test-https-agent-getname.js Outdated

rvagg suggested changes Nov 19, 2018

View reviewed changes

Comment thread doc/api/tls.md Outdated

Comment thread lib/tls.js Outdated

Comment thread src/node_crypto.cc Outdated

Comment thread src/node_crypto.cc Outdated

Comment thread test/parallel/test-https-agent-getname.js Outdated

Comment thread test/parallel/test-tls-cli-min-version-1.0.js Outdated

sam-github force-pushed the tls-min-max-version branch 2 times, most recently from 017939b to 2cae9ab Compare November 19, 2018 22:58

sam-github added semver-minor PRs that contain new features and should be released in the next minor version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Nov 19, 2018

sam-github force-pushed the tls-min-max-version branch from 2cae9ab to 61e08ae Compare November 19, 2018 23:02

vsemozhetbyt reviewed Nov 19, 2018

View reviewed changes

Comment thread doc/api/tls.md Outdated

sam-github force-pushed the tls-min-max-version branch from 61e08ae to e07e955 Compare November 19, 2018 23:09

sam-github force-pushed the tls-min-max-version branch 3 times, most recently from 5646a55 to 1d3255d Compare November 20, 2018 05:19

sam-github mentioned this pull request Dec 12, 2018

[v10.x backport] tls: add min/max protocol version options #24979

Closed

4 tasks

sam-github mentioned this pull request Feb 23, 2019

[v10.x backport] Update openssl1.1.1a #26270

Closed

4 tasks

BethGriggs mentioned this pull request May 1, 2019

v10.16.0 proposal #27514

Merged

Navigation Menu

Uh oh!

Uh oh!

tls: add min/max protocol version options - #24405

tls: add min/max protocol version options#24405
sam-github wants to merge 2 commits into
nodejs:masterfrom
sam-github:tls-min-max-version

sam-github commented Nov 16, 2018 •

edited

Loading

nodejs-github-bot commented Nov 16, 2018

Uh oh!

refack left a comment

refack commented Nov 16, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bnoordhuis left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sam-github commented Nov 19, 2018

Uh oh!

sam-github commented Nov 19, 2018

Labels

9 participants

Uh oh!

Uh oh!

Conversation

sam-github commented Nov 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

nodejs-github-bot commented Nov 16, 2018

Uh oh!

refack left a comment

Choose a reason for hiding this comment

refack commented Nov 16, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sam-github commented Nov 19, 2018

Uh oh!

sam-github commented Nov 19, 2018

Labels

9 participants

sam-github commented Nov 16, 2018 •

edited

Loading