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: permit null as a pfx value #41170

Merged
merged 2 commits into from
Dec 27, 2021

Conversation

CallMeLaNN
Copy link
Contributor

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.

Fixes: #36292

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/net

@nodejs-github-bot 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
@@ -261,7 +261,7 @@ function configSecureContext(context, options = {}, name = 'options') {
context.setSessionIdContext(sessionIdContext);
}

if (pfx !== undefined) {
if (pfx != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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) {

Copy link
Member

Choose a reason for hiding this comment

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

I prefer this pattern as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@CallMeLaNN
Copy link
Contributor Author

CallMeLaNN commented Dec 14, 2021

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
@CallMeLaNN
Copy link
Contributor Author

CallMeLaNN commented Dec 15, 2021

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

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm, could you also adjust the docs?

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@CallMeLaNN
Copy link
Contributor Author

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

@panva
Copy link
Member

panva commented Dec 16, 2021

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.

@panva panva added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Dec 16, 2021
@nodejs-github-bot
Copy link
Collaborator

@CallMeLaNN
Copy link
Contributor Author

Yup, thanks @panva

@panva panva added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 17, 2021
@nodejs-github-bot 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
@nodejs-github-bot
Copy link
Collaborator

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

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

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

Comment on lines +164 to +165
val !== undefined && val !== null &&
val.pem !== undefined ? val.pem : val);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Comment on lines +167 to +168
val !== undefined && val !== null &&
val.passphrase !== undefined ? val.passphrase : passphrase);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Dec 27, 2021
@nodejs-github-bot 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 nodejs-github-bot merged commit 077c75b into nodejs:master Dec 27, 2021
@nodejs-github-bot
Copy link
Collaborator

Landed in 077c75b

targos pushed a commit that referenced this pull request Jan 14, 2022
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>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
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>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
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

PR-URL: nodejs#41170
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
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>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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