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

[v11.x backport] tls: add min/max protocol version options #24676

Closed

Conversation

sam-github
Copy link
Contributor

@sam-github sam-github commented Nov 27, 2018

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: #24405
Reviewed-By: Refael Ackermann refack@gmail.com
Reviewed-By: Rod Vagg rod@vagg.org

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
Copy link
Collaborator

@sam-github sadly an error occured when I tried to trigger a build :(

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. v11.x labels Nov 27, 2018
@sam-github sam-github changed the title tls: add min/max protocol version options [v11.x backport] tls: add min/max protocol version options Nov 27, 2018
@sam-github sam-github force-pushed the backport-24405-to-v11.x branch from da30b3f to 1e29a78 Compare November 27, 2018 17:54
@sam-github sam-github force-pushed the backport-24405-to-v11.x branch from 1e29a78 to deea5a0 Compare November 28, 2018 18:09
@sam-github
Copy link
Contributor Author

sam-github commented Nov 30, 2018

@nodejs/crypto @nodejs/lts Turns out backporting this is not a trivial git cherry-pick, cleanup, and ci, it will take a couple hours of work. I'll take a shot at it next week.

It depends on non-semver-major parts of 60eca6a:

  • the introduction of "TLS_method" as a valid 'secureProtocol:`
  • the master min/max code depends on the change of min default to TLSv1.2, and the CLI flags to revert that, so I'll have to rewrite that section of min/max so that it implements the current 11.x defaults instead.
  • EDIT: and the tests will have to change to reflect the 11.x default cipher support, and delete the cli-min-version tests.

Can I squash the addition of "TLS_method" into this commit? Is it still a "backport" if I do that?

@rvagg
Copy link
Member

rvagg commented Dec 1, 2018

Can I squash the addition of "TLS_method" into this commit? Is it still a "backport" if I do that?

Does it matter? I think do whatever is easiest, I can see how this is going to be complicated. Thanks for taking it on though!

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 force-pushed the backport-24405-to-v11.x branch from deea5a0 to eabf3e4 Compare December 3, 2018 21:45
@sam-github
Copy link
Contributor Author

@sam-github sam-github added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 3, 2018
@BridgeAR
Copy link
Member

BridgeAR commented Dec 5, 2018

Resumed build: https://ci.nodejs.org/job/node-test-pull-request/19226/ ✔️

@BridgeAR
Copy link
Member

BridgeAR commented Dec 5, 2018

@rvagg PTAL

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM

@BridgeAR
Copy link
Member

BridgeAR commented Dec 5, 2018

I guess it would actually be possible to also add the cli flags just that they would work the other way around in this case: they would set the minimum to a higher level instead of lowering that. But since that's also possible with the option, I guess it's fine this way and we could also go ahead and backport that later on if we feel like it.

BridgeAR pushed a commit that referenced this pull request Dec 5, 2018
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
Copy link
Member

BridgeAR commented Dec 5, 2018

Landed in 1fba610 🎉

@BridgeAR BridgeAR closed this Dec 5, 2018
BridgeAR pushed a commit that referenced this pull request Dec 7, 2018
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
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>
@sam-github sam-github deleted the backport-24405-to-v11.x branch December 11, 2018 21:32
sam-github added a commit to sam-github/node that referenced this pull request Feb 22, 2019
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 added a commit to sam-github/node that referenced this pull request Feb 28, 2019
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants