-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
tls: disallow conflicting TLS protocol options #27521
Conversation
@nodejs/crypto |
Shouldn't |
927ad79
to
c6533c3
Compare
Do not allow the minimum protocol level to be set higher than the max protocol level. See: nodejs#26951, 109c097
c6533c3
to
918c0ed
Compare
Reworked this to disallow the option combination, PTAL @bnoordhuis @cjihrig |
@@ -148,6 +148,11 @@ void EnvironmentOptions::CheckOptions(std::vector<std::string>* errors) { | |||
errors->push_back("invalid value for --unhandled-rejections"); | |||
} | |||
|
|||
if (tls_min_v1_3 && tls_max_v1_2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
node --tls-min-v1.3 --tls-max-v1.2 --tls-max-v1.3
currently does the right thing (sets min and max to 1.3) but this change makes it an error.
I'm okay with that, just pointing it out in case it's something we want to preserve. Changing the logic to tls_min_v1_3 && tls_max_v1_2 && !tls_max_v1_3
would do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would keep the check as it is. Having multiple flags that partially contradict each other could confuse people.
@@ -148,6 +148,11 @@ void EnvironmentOptions::CheckOptions(std::vector<std::string>* errors) { | |||
errors->push_back("invalid value for --unhandled-rejections"); | |||
} | |||
|
|||
if (tls_min_v1_3 && tls_max_v1_2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would keep the check as it is. Having multiple flags that partially contradict each other could confuse people.
Still LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. (Theoretically, throwing an error where there wasn't one before is generally semver-major
but I don't think that logic applies here.)
Landed in cb848b4 |
Do not allow the minimum protocol level to be set higher than the max protocol level. See: #26951, 109c097 PR-URL: #27521 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>
Do not allow the minimum protocol level to be set higher than the max protocol level. See: #26951, 109c097 PR-URL: #27521 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>
Do not allow the minimum protocol level to be set higher than the max protocol level.
See: #26951, 109c097
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes