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

tls: add --tls-min-v1.2 CLI switch #27520

Closed
wants to merge 1 commit into from
Closed

tls: add --tls-min-v1.2 CLI switch #27520

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions doc/api/cli.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -586,6 +586,15 @@ added: v12.0.0
Set default [`tls.DEFAULT_MIN_VERSION`][] to 'TLSv1.1'. Use for compatibility
with old TLS clients or servers.

### `--tls-min-v1.2`
<!-- YAML
added: REPLACEME
Copy link
Member

Choose a reason for hiding this comment

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

Will the REPLACEME value be the wrong version given that this has already landed in earlier versions? Should we put the actual 11.x version here now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

REPLACEME values are only accurate on the release branch they land in (last time I checked).

-->

Set default [`tls.DEFAULT_MIN_VERSION`][] to 'TLSv1.2'. This is the default for
12.x and later, but the option is supported for compatibility with older Node.js
versions.

### `--tls-min-v1.3`
<!-- YAML
added: v12.0.0
Expand Down
4 changes: 4 additions & 0 deletions doc/node.1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,10 @@ or servers.
Set default minVersion to 'TLSv1.1'. Use for compatibility with old TLS clients
or servers.
.
.It Fl -tls-min-v1.2
Set default minVersion to 'TLSv1.2'. This is the default for 12.x and later,
but the option is supported for compatibility with older Node.js versions.
.
.It Fl -tls-min-v1.3
Set default minVersion to 'TLSv1.3'. Use to disable support for TLSv1.2 in
favour of TLSv1.3, which is more secure.
Expand Down
2 changes: 2 additions & 0 deletions lib/tls.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ if (getOptionValue('--tls-min-v1.0'))
exports.DEFAULT_MIN_VERSION = 'TLSv1';
else if (getOptionValue('--tls-min-v1.1'))
exports.DEFAULT_MIN_VERSION = 'TLSv1.1';
else if (getOptionValue('--tls-min-v1.2'))
exports.DEFAULT_MIN_VERSION = 'TLSv1.2';
else if (getOptionValue('--tls-min-v1.3'))
exports.DEFAULT_MIN_VERSION = 'TLSv1.3';
else
Expand Down
4 changes: 4 additions & 0 deletions src/node_options.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,10 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
"set default TLS minimum to TLSv1.1 (default: TLSv1.2)",
&EnvironmentOptions::tls_min_v1_1,
kAllowedInEnvironment);
AddOption("--tls-min-v1.2",
"set default TLS minimum to TLSv1.2 (default: TLSv1.2)",
&EnvironmentOptions::tls_min_v1_2,
kAllowedInEnvironment);
AddOption("--tls-min-v1.3",
"set default TLS minimum to TLSv1.3 (default: TLSv1.2)",
&EnvironmentOptions::tls_min_v1_3,
Expand Down
1 change: 1 addition & 0 deletions src/node_options.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ class EnvironmentOptions : public Options {

bool tls_min_v1_0 = false;
bool tls_min_v1_1 = false;
bool tls_min_v1_2 = false;
bool tls_min_v1_3 = false;
bool tls_max_v1_2 = false;
bool tls_max_v1_3 = false;
Expand Down
15 changes: 15 additions & 0 deletions test/parallel/test-tls-cli-min-version-1.2.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Flags: --tls-min-v1.2
'use strict';
const common = require('../common');
if (!common.hasCrypto) common.skip('missing crypto');

// Check that node `--tls-min-v1.2` is supported.

const assert = require('assert');
const tls = require('tls');

assert.strictEqual(tls.DEFAULT_MAX_VERSION, 'TLSv1.3');
assert.strictEqual(tls.DEFAULT_MIN_VERSION, 'TLSv1.2');

// Check the min-max version protocol versions against these CLI settings.
require('./test-tls-min-max-version.js');
Copy link
Member

Choose a reason for hiding this comment

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

As with the other PR, totally not blocking, but using require() to load another test file does give me pause...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that it is the same pattern as the other 4 or 5 tests of TLS CLI options.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @Trott that this does not seem ideal. I would move the test-tls-min-max-version.js file into the fixtures folder and require that instead.

Copy link
Member

Choose a reason for hiding this comment

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

I can see the argument for it not being a fixture. It's test code, not a fixture loaded by a test. And so we want it linted and so on....

Maybe there's some way to abstract it into common? That's probably more trouble than it's worth.

Anyway, it gives me pause when I see it, but I'm OK with it, especially since it's the way it's already being done....

Copy link
Member

Choose a reason for hiding this comment

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

(I am going to spend half a second wondering if it should be a fixture every single time I come across it though. 😀)