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.

Sign up for GitHub

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

Jump to bottom

tls: add min/max protocol version options #24405

Closed
wants to merge 2 commits into from
Closed

tls: add min/max protocol version options #24405

wants to merge 2 commits into from

Conversation

sam-github
Copy link
Contributor

@sam-github sam-github commented Nov 16, 2018 •
edited
Loading

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

@sam-github 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
Copy link
Collaborator

@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

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Nov 16, 2018
refack
refack approved these changes Nov 16, 2018
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

LGTM % semverity

@refack
Copy link
Contributor

refack commented Nov 16, 2018

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

@refack refack added the crypto Issues and PRs related to the crypto subsystem. label Nov 16, 2018
vsemozhetbyt
vsemozhetbyt reviewed Nov 16, 2018
View reviewed changes
doc/api/tls.md Outdated Show resolved Hide resolved
doc/api/tls.md Outdated Show resolved Hide resolved
doc/api/tls.md Outdated Show resolved Hide resolved
doc/api/tls.md Outdated Show resolved Hide resolved
mscdex
mscdex reviewed Nov 17, 2018
View reviewed changes
doc/api/tls.md Outdated Show resolved Hide resolved
@sam-github 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
bnoordhuis reviewed Nov 19, 2018
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

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.

doc/api/tls.md Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
test/parallel/test-https-agent-getname.js Show resolved Hide resolved
rvagg
rvagg suggested changes Nov 19, 2018
View reviewed changes
doc/api/tls.md Outdated Show resolved Hide resolved
lib/tls.js Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
test/parallel/test-https-agent-getname.js Show resolved Hide resolved
test/parallel/test-tls-cli-min-version-1.0.js Show resolved Hide resolved
@sam-github 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 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
Copy link
Contributor Author

@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.

vsemozhetbyt
vsemozhetbyt reviewed Nov 19, 2018
View reviewed changes
doc/api/tls.md Outdated Show resolved Hide resolved
@sam-github
Copy link
Contributor Author

@rvagg @bnoordhuis PTAL

@sam-github 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
tls: add min/max protocol version options
28ea4ee 
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.
BridgeAR pushed a commit that referenced this pull request Dec 7, 2018
@sam-github @BridgeAR
tls: add min/max protocol version options
acb7351 
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>
BridgeAR pushed a commit that referenced this pull request Dec 7, 2018
@sam-github @BridgeAR
tls: add min/max protocol version options
181bdf7 
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>
BridgeAR added a commit that referenced this pull request Dec 7, 2018
@BridgeAR
2018-12-07, Version 11.4.0 (Current)
4f1297f 
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
BridgeAR added a commit that referenced this pull request Dec 7, 2018
@BridgeAR
2018-12-07, Version 11.4.0 (Current)
bcef949 
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
@targos targos added this to Backported in v11.x Dec 7, 2018
This was referenced Dec 7, 2018
Closed
Closed
Closed
Closed
Closed
@sam-github sam-github mentioned this pull request Dec 12, 2018
Closed
4 tasks
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@sam-github @refack
tls: add min/max protocol version options
08d5d35 
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>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@BridgeAR @refack
2018-12-07, Version 11.4.0 (Current)
5912f6d 
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
sam-github added a commit to sam-github/node that referenced this pull request Feb 22, 2019
@sam-github
tls: add min/max protocol version options
a8a0889 
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>
@sam-github sam-github mentioned this pull request Feb 23, 2019
Closed
4 tasks
sam-github added a commit to sam-github/node that referenced this pull request Feb 28, 2019
@sam-github
tls: add min/max protocol version options
be6a3a1 
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>
BethGriggs pushed a commit that referenced this pull request Mar 28, 2019
@sam-github @BethGriggs
tls: add min/max protocol version options
d3ebad2 
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>
@BethGriggs BethGriggs mentioned this pull request May 1, 2019
Merged
This was referenced May 29, 2019
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mscdex mscdex mscdex left review comments

@refack refack refack approved these changes

@bnoordhuis bnoordhuis bnoordhuis left review comments

@addaleax addaleax addaleax left review comments

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@rvagg rvagg rvagg approved these changes

@shigeki shigeki Awaiting requested review from shigeki

Assignees
No one assigned
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. semver-minor PRs that contain new features and should be released in the next minor version. tls Issues and PRs related to the tls subsystem.
Projects
No open projects
v11.x
  
Backported
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@sam-github @nodejs-github-bot @refack @targos @mscdex @bnoordhuis @rvagg @addaleax @vsemozhetbyt