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

[T1B0]

• Version:

node v15.3.0

• Platform:

Reproduced on Linux 5.9.0-3-amd64 #1 SMP Debian 5.9.9-1 (2020-11-19) x86_64 GNU/Linux - but probably applicable on all platform

• Subsystem:

tls.connect() options

What steps will reproduce the bug?

Before version 15.3.0 tls.connect (also accepted by https.request() ) option value null was accepted as falsy value for the cipthers option.
As of version 15.3.0, passing option.ciphers = null throw an error.

case.js (tweaked from https://nodejs.org/api/https.html#https_https_request_options_callback )

const https = require('https');

const options = {
  hostname: 'nodejs.org',
  port: 443,
  path: '/en/',
  ciphers: null,
  method: 'GET'
};

const req = https.request(options, (res) => {
  console.log('statusCode:', res.statusCode);
  console.log('headers:', res.headers);

  res.on('data', (d) => {
    process.stdout.write(d);
  });
});

req.on('error', (e) => {
  console.error(e);
});

req.end();

You get a connection that end up with 200 OK using this v15.2.1 dockerfile

FROM node:15.2.1-buster-slim

COPY ./case.js ./

CMD node case.js

but it will throw an error with a v15.3.0 dockerfile

FROM node:15.3.0-buster-slim

COPY ./case.js ./

CMD node case.js

How often does it reproduce? Is there a required condition?

throw an error 100% of the time on v15.3.0 with options.ciphers = null

What is the expected behavior?

I have no doubt it is a changing behavior, but i don't know what was the expected behavior of an undocumented cipher option value in the first place either. i just know that it used to work.

What do you see instead?

behavior changed, it now throw an error :

node:internal/validators:123
    throw new ERR_INVALID_ARG_TYPE(name, 'string', value);
    ^

TypeError [ERR_INVALID_ARG_TYPE]: The "options.ciphers" property must be of type string. Received null
    at new NodeError (node:internal/errors:278:15)
    at validateString (node:internal/validators:123:11)
    at Object.createSecureContext (node:_tls_common:267:5)
    at Object.connect (node:_tls_wrap:1581:48)
    at Agent.createConnection (node:https:129:22)
    at Agent.createSocket (node:_http_agent:323:26)
    at Agent.addRequest (node:_http_agent:274:10)
    at new ClientRequest (node:_http_client:318:16)
    at Object.request (node:https:313:10)
    at Object.<anonymous> (/case.js:11:19) {
  code: 'ERR_INVALID_ARG_TYPE'

Additional information

• I stumbled upon this because it breaks real world npm module elasticsearch-legacy-client on v15.3.0 build (see https://github.com/elastic/elasticsearch-js-legacy/blob/16.x/src/lib/host.js#L44 )
• the new version's error message is clear and seems well-adapted to the given context.
• this change seem to be introduced by https://github.com/nodejs/node/pull/35368/files

The point of this report is to warn about this non-obvious breaking behavior change and not to say it's not acceptable/legit api change.

ps: Thanks to @jasnell for encouraging me to write an issue 👍

[T1B0]

cc @spalger for the https://github.com/elastic/elasticsearch-js-legacy/ part ^^

[Trott]

I did a bisect and the commit that introduced this issue is 35274cb.

[jasnell]

Yep, already planning on working on it on Monday.

[Trott]

Yep, already planning on working on it on Monday.

It's a one-line fix so I hope I'm not ruining your Monday plans/strategy.: #36318

[tmadeira]

Yep, already planning on working on it on Monday.

It's a one-line fix so I hope I'm not ruining your Monday plans/strategy.: #36318

I think the same fix is needed for dhparam !== undefined, crl !== undefined, sessionIdContext !== undefined, pfx !== undefined. Even setting ciphers here I'm getting "Error: Unable to load PFX certificate" in a code that used to work before node v15.3.0.

[Trott]

Reopening based on #36292 (comment). /ping @jasnell

[CallMeLaNN]

Can we look into this. This is the only thing left before I can switch to latest LTS due to Error: Unable to load PFX certificate

The APNS library https://github.com/parse-community/node-apn expect pfx: null if I configure to use pem cert. This is reproduceable in 16.13.1:

http2.connect(`https://api.push.apple.com/`, { cert, key, pfx: null })

The lib/_tls_common.js structure from the PR above probably changed. I guess the remaining fixes should be in this file right?

node/lib/internal/tls/secure-context.js

Line 264 in ecf4114

if (pfx !== undefined) {

[Trott]

Can we look into this. This is the only thing left before I can switch to latest LTS due to Error: Unable to load PFX certificate

The APNS library https://github.com/parse-community/node-apn expect pfx: null if I configure to use pem cert. This is reproduceable in 16.13.1:

http2.connect(`https://api.push.apple.com/`, { cert, key, pfx: null })

The lib/_tls_common.js structure from the PR above probably changed. I guess the remaining fixes should be in this file right?

node/lib/internal/tls/secure-context.js

Line 264 in ecf4114

if (pfx !== undefined) {

@nodejs/crypto @nodejs/http2 Can anyone comment?

[CallMeLaNN]

I think I can submit PR if null is acceptable.

v14 and latest doc doesn't mentioned specifically either undefined or null.

[mcollina]

The APNS library https://github.com/parse-community/node-apn expect pfx: null if I configure to use pem cert.

Where does it do this? I could not find a reference to pfx in that library.

I see no problem in allowing null for pfx, go for the PR!

[CallMeLaNN]

Where does it do this? I could not find a reference to pfx in that library.

https://github.com/parse-community/node-apn/blob/master/doc/provider.markdown
Basically it passes their options to http2.connect(address, options) but somehow options.pfx is always null.

How about dhparam, crl, sessionIdContext as mentioned #36292 (comment)?

I'm not sure of those options, but the pattern is very similar. Let me know if I can include those options in the PR.

[mcollina]

I would just include them all, thanks.

[fholzer]

In case anyone got here looking for a workaround, but for some reason you don't have the luxury of being able to set pfx to undefined: As an alternative you can set pfx to an empty array []. That resolved the issue for me and my elasticsearch client. (So the elasticsearch client options would look like this: { "ssl": { "pfx": [] } })

[alolis]

Thanks @fholzer , you saved the day.

[fholzer]

To anyone still finding this when using the elasticsearch client...

Your options:

• Upgrade to new ES client (includes breaking changes in how to use the client)
• Upgrade NodeJS to v16.17/v17.4 (fixed in nodejs via tls: permit null as a pfx value #41170)
• Provide { "ssl": { "pfx": [] } } option to ES client: tls.connect() options ciphers no longer accept null as a valid value in node v15.3.0 #36292 (comment)