tls: permit null as a pfx value by CallMeLaNN · Pull Request #41170 · nodejs/node · GitHub

Allow null along with undefined for pfx value.

This is to avoid breaking change when upgrading v14 to v16 and
third party library passing null to https option, pfx.

Review requested:

@nodejs/crypto
@nodejs/net

This would make it look like less of a mistake (using loose quality instead of strict equality) since strict equality is used more often than not:

Suggested change

      
              if (pfx != null) {
          
              if (pfx !== undefined && pfx !== null) {

I prefer this pattern as well.

I quite like != null pattern, especially since optional chaining and null coalescing are a thing 🤷‍♂️

Agreed. I prefer the same because strict equality more verbose. I was following the previous PR in the issue... So I'll include the strict for ciphers option also.

Hold on, let me include dhparam, crl and sessionIdContext to allow null soon as per comment in the issue.

Allow null along with undefined for pfx value. This is to avoid breaking change when upgrading v14 to v16 and 3rd party library passing null to pfx Fixes: nodejs#36292

Allow the expected null along with undefined for options value. This is to avoid breaking change when upgrading v14 to v16 and 3rd party library passing null to options Fixes: nodejs#36292

@mcollina done. I just check null value against the remaining options above but it seems like most of the options throw TypeError [ERR_INVALID_ARG_TYPE] due to null not considered as default undefined. So I fix all similar options to skip null just like undefined.

There are a couple of changes I can't write a test like when passphrase: null the line context.loadPKCS12(toBuf(pfx)); is always throw Error unsupported. I guess unencrypted pfx without passphrase is not supported in the native function but the null handling works as expected before and after this changes.

lgtm, could you also adjust the docs?

CI: https://ci.nodejs.org/job/node-test-pull-request/41497/

CI: https://ci.nodejs.org/job/node-test-pull-request/41498/

CI: https://ci.nodejs.org/job/node-test-pull-request/41502/

@mcollina please suggest the doc update. Shall I comment on the code and describe in this API? I think I can mention the optional options accept null value.

I'm not sure how to test or patch this on v16, it should be non breaking change.

Anything I can do with the failing checks?

I don't think we need a doc update, the falsy values are not documented at the moment, and this is a restoration of previous behaviour we missed to have test covered.

CI: https://ci.nodejs.org/job/node-test-pull-request/41511/

Yup, thanks @panva

Commit Queue failed

- Loading data for nodejs/node/pull/41170 ✔ Done loading data for nodejs/node/pull/41170 ----------------------------------- PR info ------------------------------------ Title tls: permit null as a pfx value (#41170) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch CallMeLaNN:fix/pfx-cant-load -> nodejs:master Labels tls, author ready Commits 2 - tls: permit null as a pfx value - tls: permit null as an options value Committers 1 - CallMeLaNN PR-URL: https://github.com/nodejs/node/pull/41170 Fixes: https://github.com/nodejs/node/issues/36292 Reviewed-By: Matteo Collina Reviewed-By: Filip Skokan ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/41170 Fixes: https://github.com/nodejs/node/issues/36292 Reviewed-By: Matteo Collina Reviewed-By: Filip Skokan -------------------------------------------------------------------------------- ℹ This PR was created on Tue, 14 Dec 2021 13:57:39 GMT ✔ Approvals: 2 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/41170#pullrequestreview-832783699 ✔ - Filip Skokan (@panva): https://github.com/nodejs/node/pull/41170#pullrequestreview-832785694 ✔ Last GitHub Actions successful ℹ Last Full PR CI on 2021-12-16T09:05:25Z: https://ci.nodejs.org/job/node-test-pull-request/41511/ - Querying data for job/node-test-pull-request/41511/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/master up to date... From https://github.com/nodejs/node * branch master -> FETCH_HEAD ✔ origin/master is now up-to-date - Downloading patch for 41170 From https://github.com/nodejs/node * branch refs/pull/41170/merge -> FETCH_HEAD ✔ Fetched commits as a182a2163606..f474aa5f722c -------------------------------------------------------------------------------- [master 398b32f020] tls: permit null as a pfx value Author: CallMeLaNN Date: Fri Dec 10 20:52:41 2021 +0800 2 files changed, 18 insertions(+), 1 deletion(-) [master 20ba07821f] tls: permit null as an options value Author: CallMeLaNN Date: Wed Dec 15 15:01:36 2021 +0800 2 files changed, 33 insertions(+), 18 deletions(-) ✔ Patches applied There are 2 commits in the PR. Attempting autorebase. Rebasing (2/4)

Executing: git node land --amend --yes
⚠ Found Fixes: #36292, skipping..
--------------------------------- New Message ----------------------------------
tls: permit null as a pfx value

Allow null along with undefined for pfx value.

This is to avoid breaking change when upgrading v14 to v16 and
3rd party library passing null to pfx

Fixes: #36292

PR-URL: #41170
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Filip Skokan panva.ip@gmail.com

[detached HEAD 9db1bebe0d] tls: permit null as a pfx value
Author: CallMeLaNN git@lann.me
Date: Fri Dec 10 20:52:41 2021 +0800
2 files changed, 18 insertions(+), 1 deletion(-)
Rebasing (3/4)
Rebasing (4/4)

Executing: git node land --amend --yes
⚠ Found Fixes: #36292, skipping..
--------------------------------- New Message ----------------------------------
tls: permit null as an options value

Allow the expected null along with undefined for options value.

This is to avoid breaking change when upgrading v14 to v16 and
3rd party library passing null to options

Fixes: #36292

PR-URL: #41170
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Filip Skokan panva.ip@gmail.com

[detached HEAD b82b3833e3] tls: permit null as an options value
Author: CallMeLaNN git@lann.me
Date: Wed Dec 15 15:01:36 2021 +0800
2 files changed, 33 insertions(+), 18 deletions(-)

Successfully rebased and updated refs/heads/master.

ℹ Use --fixupAll option, squash the PR manually or land the PR from the command line.

https://github.com/nodejs/node/actions/runs/1591586400

Can we use optional chaining? It's very wordy as is.

Suggested change

      
                      val !== undefined && val !== null &&
          
                      val.pem !== undefined ? val.pem : val);
          
                      val?.pem !== undefined ? val.pem : val);

Suggested change

      
                      val !== undefined && val !== null &&
          
                      val.passphrase !== undefined ? val.passphrase : passphrase);
          
                      val?.passphrase !== undefined ? val.passphrase : passphrase);

Landed in 077c75b

Allow null along with undefined for pfx value. This is to avoid breaking change when upgrading v14 to v16 and 3rd party library passing null to pfx Fixes: #36292 PR-URL: #41170 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>

nodejs-github-bot added needs-ci PRs that need a full CI run. tls Issues and PRs related to the tls subsystem. labels Dec 14, 2021

mscdex reviewed Dec 14, 2021

View reviewed changes

CallMeLaNN added 2 commits December 15, 2021 15:01

tls: permit null as a pfx value

8fd11f9

Allow null along with undefined for pfx value. This is to avoid breaking change when upgrading v14 to v16 and 3rd party library passing null to pfx Fixes: nodejs#36292

tls: permit null as an options value

f474aa5

Allow the expected null along with undefined for options value. This is to avoid breaking change when upgrading v14 to v16 and 3rd party library passing null to options Fixes: nodejs#36292

CallMeLaNN force-pushed the fix/pfx-cant-load branch from 7c0accb to f474aa5 Compare December 15, 2021 07:03

mcollina approved these changes Dec 15, 2021

View reviewed changes

panva approved these changes Dec 15, 2021

View reviewed changes

github-actions Bot mentioned this pull request Dec 16, 2021

CI Reliability 2021-12-16 nodejs/reliability#146

Open

20 tasks

panva added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. and removed needs-ci PRs that need a full CI run. labels Dec 16, 2021

github-actions Bot mentioned this pull request Dec 17, 2021

CI Reliability 2021-12-17 nodejs/reliability#147

Open

22 tasks

panva added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 17, 2021

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 17, 2021

aduh95 reviewed Dec 17, 2021

View reviewed changes

This was referenced Dec 18, 2021

CI Reliability 2021-12-18 nodejs/reliability#148

Open

CI Reliability 2021-12-19 nodejs/reliability#149

Open

CI Reliability 2021-12-20 nodejs/reliability#150

Open

github-actions Bot mentioned this pull request Dec 21, 2021

CI Reliability 2021-12-21 nodejs/reliability#151

Open

22 tasks

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 27, 2021

nodejs-github-bot merged commit 077c75b into nodejs:master Dec 27, 2021

aduh95 mentioned this pull request Dec 27, 2021

tls: use optional chaining to simplify checks #41337

Merged

targos mentioned this pull request Jan 16, 2022

v17.4.0 release proposal #41557

Merged

danielleadams mentioned this pull request Feb 1, 2022

v16.14.0 proposal #41804

Merged

fholzer mentioned this pull request Nov 29, 2022

tls.connect() options ciphers no longer accept null as a valid value in node v15.3.0 #36292

Closed

		val !== undefined && val !== null &&
		val.pem !== undefined ? val.pem : val);

	val !== undefined && val !== null &&
	val.pem !== undefined ? val.pem : val);
	val?.pem !== undefined ? val.pem : val);

		val !== undefined && val !== null &&
		val.passphrase !== undefined ? val.passphrase : passphrase);

	val !== undefined && val !== null &&
	val.passphrase !== undefined ? val.passphrase : passphrase);
	val?.passphrase !== undefined ? val.passphrase : passphrase);

tls: permit null as a pfx value by CallMeLaNN · Pull Request #41170 · nodejs/node · GitHub

Uh oh!

Uh oh!

tls: permit null as a pfx value - #41170

tls: permit null as a pfx value#41170nodejs-github-bot merged 2 commits intonodejs:masternodejs/node:masterfrom CallMeLaNN:fix/pfx-cant-loadCallMeLaNN/node:fix/pfx-cant-loadCopy head branch name to clipboard

Conversation

CallMeLaNN commented Dec 14, 2021

Uh oh!

nodejs-github-bot commented Dec 14, 2021

Uh oh!

mscdex Dec 14, 2021

Choose a reason for hiding this comment

Uh oh!

mcollina Dec 14, 2021

Choose a reason for hiding this comment

Uh oh!

aduh95 Dec 14, 2021

Choose a reason for hiding this comment

Uh oh!

CallMeLaNN Dec 14, 2021

Choose a reason for hiding this comment

Uh oh!

CallMeLaNN commented Dec 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CallMeLaNN commented Dec 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 15, 2021

Uh oh!

nodejs-github-bot commented Dec 15, 2021

Uh oh!

nodejs-github-bot commented Dec 15, 2021

Uh oh!

CallMeLaNN commented Dec 16, 2021

Uh oh!

panva commented Dec 16, 2021

Uh oh!

nodejs-github-bot commented Dec 16, 2021

Uh oh!

CallMeLaNN commented Dec 16, 2021

Uh oh!

nodejs-github-bot commented Dec 17, 2021

PR-URL: #41170 Reviewed-By: Matteo Collina matteo.collina@gmail.com Reviewed-By: Filip Skokan panva.ip@gmail.com

PR-URL: #41170 Reviewed-By: Matteo Collina matteo.collina@gmail.com Reviewed-By: Filip Skokan panva.ip@gmail.com

Successfully rebased and updated refs/heads/master.

Uh oh!

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

aduh95 Dec 17, 2021

Choose a reason for hiding this comment

Uh oh!

aduh95 Dec 17, 2021

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tls: permit null as a pfx value#41170
nodejs-github-bot merged 2 commits into
nodejs:masterfrom
CallMeLaNN:fix/pfx-cant-load

CallMeLaNN commented Dec 14, 2021 •
edited
Loading

CallMeLaNN commented Dec 15, 2021 •
edited
Loading

PR-URL: #41170
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Filip Skokan panva.ip@gmail.com

PR-URL: #41170
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Filip Skokan panva.ip@gmail.com