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

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

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
10 participants
@sam-github
Copy link
Member

commented May 1, 2019

Switch added in v11.x, add it to master/12.x for consistency and
compatibility.

See: #26951, commit bf2c283

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

This comment has been minimized.

@sam-github

This comment has been minimized.

Copy link
Member Author

commented May 1, 2019

@cjihrig

cjihrig approved these changes May 1, 2019

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

This comment has been minimized.

Copy link
@Trott

Trott May 2, 2019

Member

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?

This comment has been minimized.

Copy link
@sam-github

sam-github May 2, 2019

Author Member

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

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');

This comment has been minimized.

Copy link
@Trott

Trott May 2, 2019

Member

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

This comment has been minimized.

Copy link
@sam-github

sam-github May 2, 2019

Author Member

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

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR May 3, 2019

Member

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.

This comment has been minimized.

Copy link
@Trott

Trott May 3, 2019

Member

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

This comment has been minimized.

Copy link
@Trott

Trott May 3, 2019

Member

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

@Trott

Trott approved these changes May 2, 2019

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@ChALkeR

ChALkeR approved these changes May 3, 2019

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

tls: add --tls-min-v1.2 CLI switch
Switch added in v11.x, add it to master/12.x for consistency and
compatibility.

See: #26951, commit bf2c283

@sam-github sam-github force-pushed the sam-github:cli-tls-min-v1.2 branch from 7ffedbd to ea56e47 May 3, 2019

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment has been minimized.

Copy link

commented May 5, 2019

@Trott

This comment has been minimized.

Copy link
Member

commented May 5, 2019

Landed in 3d98051

@Trott Trott closed this May 5, 2019

Trott added a commit to Trott/io.js that referenced this pull request May 5, 2019

tls: add --tls-min-v1.2 CLI switch
Switch added in v11.x, add it to master/12.x for consistency and
compatibility.

See: nodejs#26951, commit bf2c283

PR-URL: nodejs#27520
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

targos added a commit that referenced this pull request May 6, 2019

tls: add --tls-min-v1.2 CLI switch
Switch added in v11.x, add it to master/12.x for consistency and
compatibility.

See: #26951, commit bf2c283

PR-URL: #27520
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

@targos targos added the semver-minor label May 6, 2019

@targos targos referenced this pull request May 6, 2019

Merged

v12.2.0 proposal #27578

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.