https: disallow boolean types for `key` and `cert` options by jimmycann · Pull Request #14807 · nodejs/node · GitHub

This is my first PR on the Node repository, your feedback is greatly appreciated.

When using https.createServer, passing boolean values for key and cert properties in the options object parameter doesn't throw an error an could lead to potential issues if they're accidentally passed.

This PR aims to throw a reasonable error if a boolean was passed to either of those properties.

Fixes: #12802

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

https

When using https.createServer, passing boolean values for `key` and `cert` properties in the options object parameter doesn't throw an error an could lead to potential issues if they're accidentally passed. This PR aims to throw a reasonable error if a boolean was passed to either of those properties. Fixes: nodejs#12802

By the way, we try to migrate to a new error system that allows for more flexibility in the future, it would be cool if you think you could take a look at using that.

We don’t need the copyright header for new files.

Hello @yjimk and welcome. Thank you very much for you contribution 🥇.
You've chosen an interesting case... While your change is correct, it checks for a very specific scenario so I'll give you a few of my observations:

I just followed the the documentation and found that some of the options passed to https.createServer are then passed to tls.createSecureContext, so it's probably a better place to put the validation.

node/lib/_tls_common.js

Lines 84 to 107 in 262d4b2

    
           if (options.cert) { 
        
             if (Array.isArray(options.cert)) { 
        
               for (i = 0; i < options.cert.length; i++) 
        
                 c.context.setCert(options.cert[i]); 
        
             } else { 
        
               c.context.setCert(options.cert); 
        
             } 
        
           } 
        
           // NOTE: It is important to set the key after the cert. 
        
           // `ssl_set_pkey` returns `0` when the key does not match the cert, but 
        
           // `ssl_set_cert` returns `1` and nullifies the key in the SSL structure 
        
           // which leads to the crash later on. 
        
           if (options.key) { 
        
             if (Array.isArray(options.key)) { 
        
               for (i = 0; i < options.key.length; i++) { 
        
                 const key = options.key[i]; 
        
                 const passphrase = key.passphrase || options.passphrase; 
        
                 c.context.setKey(key.pem || key, passphrase); 
        
               } 
        
             } else { 
        
               c.context.setKey(options.key, options.passphrase); 
        
             } 
        
           }

The documentation state that cert and key can be <string> | <string[]> | <Buffer> | <Buffer[]> | <Object[]> so it is probably better to assert that the option is one of those type, rather than trying to exclude everything else.
We are migration from using simply throw new Error() to a more structured error mechanism that includes an error code like -

node/lib/path.js

Line 28 in 262d4b2

throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'path', 'string');

the full guide is in https://github.com/nodejs/node/blob/master/doc/guides/using-internal-errors.md

Hi @yjimk! Welcome and thanks for the PR! I left a few comments with requested changes. Thanks again!

Line 41 guarantee that opts is an object so there is no need to check opts here or in line 46.

I don't think checking against all invalid values is the way to go. I think it should confirm it is a valid value and reject everything else.

These regular expressions would be better if they matched the entire message (that is, if they started with ^ and ended with $).

This and the other Error should be a TypeError.

LGTM with some cleanup.

You can reduce code duplication a lot here. All of these assert.throws() calls are essentially the same, with only the value of key, cert, and the regular expression changing. You can do something like this instead:

const key = fs.readFileSync(`${common.fixturesDir}/keys/agent1-key.pem`);
const cert = fs.readFileSync(`${common.fixturesDir}/keys/agent1-cert.pem`);
const invalidKeyRE = /^"options\.key" must not be a boolean$/;
const invalidCertRE = /^"options\.cert" must not be a boolean$/;

[
  [true, cert, invalidKeyRE],
  // Fill in other cases here
].forEach((params) => {
  assert.throws(() => {
    https.createServer({
      key: params[0],
      cert: params[1]
    });
  }, params[2]);
});

I agree with @Trott, better verify for validity instead of checking single invalid values like booleans (e.g. whitelist over blacklist approach).

Also, I think the checks should also be done on tls.createServer and tls.createSecureContext, so I'd say the change should be done in the lowest level possible, which is tls.createSecureContext.

Thank you all for the super helpful and detailed feedback! I'll make those changes and see if I can get closer.

Taking on board the review from the initial PR, multiple changes have been made. - Type checking now a whitelist as opposed to only checking for a boolean - Type checking moved to the underlying `_tls_common` , `tls` and `https` modules now affected - Arrays are now iterated using `map` instead of `for` for affected sections -Testing added for the `tls` module Fixes: nodejs#12802

comments addressed, thanks

I believe I might have addressed all of the comments left with my latest commit. The new code is a bit of a departure from the old, so I hope it is OK and would appreciate any tips.

Particularly worried about using that function validateKeyCert from a naming perspective and also whether it conforms to the right method for type checking.

There are a few other things in that file that I noticed are a different style compared to other parts of the code, such as usage of for loops and var over let and const.

Thanks again for your great feedback and comments, it's been a fantastic learning experience.

I think this is too strict, and instanceof Buffer is not a foolproof way to check for Buffers. Can you use ArrayBuffer.isView(value) instead and adjust the message accordingly (Buffers are just special Uint8Arrays, but all Uint8Arrays and other ArrayBufferViews should work, I think.).

Ah, brilliant! Replacing the two check works perfectly. I'll change this over and add some additional tests.

Regarding the new error message, it is currently ['string', 'buffer'] and I'm thinking it should change to ['string', 'ArrayBuffer'] with perhaps string => String. Would you have an opinion on how that should look?

I don’t think we can list ArrayBuffer, as those are not ArrayBufferViews themselves.

Maybe ['string', 'Buffer', 'TypedArray', 'DataView']? That would match the message here:

node/lib/zlib.js

Line 231 in cde272a

'Invalid dictionary: it should be a Buffer, TypedArray, or DataView');

Perfect! Thank you, I'll create a commit for this change.

outdated

There's been some recent work to simplify fixtures access...

example...

const fixtures = require('../common/fixtures');

const keyBuff = fixtures.readKey('agent1-key.pem');

Ah OK, cool. Thanks, I'll change that over too. The original test file was copied over from another and amended, hence the previous inclusion of the license comments and usage of the fs.readFileSync method of including the pem files. Is there are issue/task that you're aware of to update the older tests to use the newer methods? Something like that I feel like would be a useful task for me to take up in future.

Since this PR relates to #12802 which is marked as "good first contribution", I'm going to ask a question (as I also intended on grabbing this). If this is inappropriate, delete my question and I'll take it to IRC or SO

From e92e64a extended commit message:

Arrays are now iterated using map instead of for for affected sections

What's the benefit of changing the loop type? I can't seem to find related review comment that (probably) requested the change.

RE: @fl0w, mostly due to personal preference toward map and other functional programming methods rather than for or forEach loops.

This tweet (although it mentions forEach instead of for) is the basis for my way of thinking.

https://twitter.com/_ericelliott/status/710571765844615168

I'd love to hear the fors and againsts on it though

...sorry

Additional changes in line with PR review - Loosen type checking for buffers using the ArrayBuffer method - Require pem files using updated fixture access method - Add tests for ArrayBuffer and DataView types Fixes: nodejs#12802

I've changed the type checking for buffer, but my knowledge of ArrayBuffers and DataViews is pretty limited.

This map can be changed to a forEach because you're not using the return value.

Same here.

And here.

Few nits, overall LGTM.

RE: @fl0w, mostly due to personal preference toward map and other functional programming methods rather than for or forEach loops.

Sorry, just saw your comment after review. I personally see no benefit of using map over forEach if you're not going to return something/use the return value.

Node.js uses for a lot because as far as we know, it is still considerably faster than the prototype methods, thought that might have changed with TurboFan.

No problem, I'll change it over to use the forEach method. :)

- Change iteration method from map -> forEach Fixes: nodejs#12802

thought that might have changed with TurboFan

It does not appear to have changed by much.

Landed in a7dccd0

PR-URL: #14807 Fixes: #12802 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: nodejs/node#14807 Fixes: nodejs/node#12802 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

* **async_hooks** * Older experimental `async_hooks` APIs have been removed [[`d731369b1d`](d731369b1d)] **(SEMVER-MAJOR)** [#14414](#14414) * **Errors** * Multiple built in modules have been migrated to use static error codes * **Domains** * The long deprecated `.dispose()` method has been removed [[`602fd36d95`](602fd36d95)] **(SEMVER-MAJOR)** [#15412](#15412) * **File system** * `fs.ReadStream` and `fs.WriteStream` now make use of `.destroy()` [[`e5c290bed9`](e5c290bed9)] **(SEMVER-MAJOR)** [#15407](#15407) * `fs` callbacks are now invoked with an undefined `this` context [[`2249234fee`](2249234fee)] **(SEMVER-MAJOR)** [#14645](#14645) * **HTTP** * Socket timeout is set when the socket connects [[`10be20a0e8`](10be20a0e8)] **(SEMVER-MAJOR)** [#8895](#8895) * A bug causing the request `error` event to fire twice has been fixed [[`620ba41694`](620ba41694)] **(SEMVER-MAJOR)** [#14659](#14659) * The `pipe` method on `OutgoingMessage` has been disabled [[`156549d8ff`](156549d8ff)] **(SEMVER-MAJOR)** [#14358](#14358) * **HTTP/2** * The `--expose-http2` command-line argument is no longer required [[`f55ee6e24a`](f55ee6e24a)] **(SEMVER-MAJOR)** [#15535](#15535) * **Internationalization** * The `Intl.v8BreakIterator` class has been removed [[`668ad44922`](668ad44922)] **(SEMVER-MAJOR)** [#15238](#15238) * **OS** * `os.EOL` is now read-only [[`f6caeb9526`](f6caeb9526)] **(SEMVER-MAJOR)** [#14622](#14622) * **Process** * It is now possible to pass additional flags to `dlopen` [[`5f22375922`](5f22375922)] **(SEMVER-MAJOR)** [#12794](#12794) * **Timers** * Using a timeout duration larger than 32-bits will now emit a warning [[`ce3586da31`](ce3586da31)] **(SEMVER-MAJOR)** [#15627](#15627) * **TLS** * `parseCertString` has been deprecated [[`468110b327`](468110b327)] **(SEMVER-MAJOR)** [#14249](#14249) * Type-checking for `key`, `cert`, and `ca` options has been added [[`a7dccd040d`](a7dccd040d)] **(SEMVER-MAJOR)** [#14807](#14807)

nodejs-github-bot added the https Issues or PRs related to the https subsystem. label Aug 13, 2017

addaleax previously approved these changes Aug 13, 2017

View reviewed changes

Trott previously requested changes Aug 13, 2017

View reviewed changes

cjihrig approved these changes Aug 13, 2017

View reviewed changes

addaleax reviewed Aug 15, 2017

View reviewed changes

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 16, 2017

jasnell reviewed Aug 16, 2017

View reviewed changes

tls: type checking for key, cert and ca options

b132c96

Additional changes in line with PR review - Loosen type checking for buffers using the ArrayBuffer method - Require pem files using updated fixture access method - Add tests for ArrayBuffer and DataView types Fixes: nodejs#12802

silverwind reviewed Aug 16, 2017

View reviewed changes

silverwind approved these changes Aug 16, 2017

View reviewed changes

addaleax approved these changes Aug 16, 2017

View reviewed changes

tls: type checking for key, cert and ca options

c46fdd4

- Change iteration method from map -> forEach Fixes: nodejs#12802

BridgeAR approved these changes Aug 27, 2017

View reviewed changes

BridgeAR closed this Aug 27, 2017

mscdex mentioned this pull request Aug 27, 2017

tls: replace forEach with for #15053

Closed

2 tasks

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

+              // NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+              // DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+              // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+              // USE OR OTHER DEALINGS IN THE SOFTWARE.

+                https.createServer({
+                  key: true,
+                  cert: fs.readFileSync(`${common.fixturesDir}/keys/agent1-cert.pem`)
+                }), /"options\.key" must not be a boolean/);

+                    options.ca.map((ca) => {
+                      validateKeyCert(ca, 'ca');
+                      c.context.addCACert(ca);
+                    });

+                    options.cert.map((cert) => {
+                      validateKeyCert(cert, 'cert');
+                      c.context.setCert(cert);
+                    });

+                    options.key.map((k) => {
+                      validateKeyCert(k.pem || k, 'key');
+                      c.context.setKey(k.pem || k, k.passphrase || options.passphrase);
+                    });

+                  throw new Error('"options.key" must not be a boolean');
+                if (opts && typeof opts.cert === 'boolean')
+                  throw new Error('"options.cert" must not be a boolean');

                 }
                 opts = util._extend({}, opts);
+                if (opts && typeof opts.key === 'boolean')

               }
+              function validateKeyCert(value, type) {
+                if (typeof value !== 'string' && !(value instanceof Buffer))

+              const https = require('https');
+              const fs = require('fs');
+              const keyBuff = fs.readFileSync(`${common.fixturesDir}/keys/agent1-key.pem`);

https: disallow boolean types for `key` and `cert` options by jimmycann · Pull Request #14807 · nodejs/node · GitHub

Uh oh!

Uh oh!

https: disallow boolean types for key and cert options - #14807

https: disallow boolean types for key and cert options#14807jimmycann wants to merge 4 commits intonodejs:masternodejs/node:masterfrom jimmycann:issue/12802jimmycann/node:issue/12802Copy head branch name to clipboard

Conversation

jimmycann commented Aug 13, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Aug 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverwind commented Aug 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jimmycann commented Aug 13, 2017

Uh oh!

jimmycann commented Aug 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fl0w commented Aug 16, 2017

Uh oh!

jimmycann commented Aug 16, 2017

Uh oh!

jimmycann commented Aug 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverwind left a comment

Choose a reason for hiding this comment

Uh oh!

silverwind commented Aug 16, 2017

Uh oh!

jimmycann commented Aug 16, 2017

Uh oh!

jasnell commented Aug 23, 2017

Uh oh!

BridgeAR commented Aug 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

https: disallow boolean types for key and cert options#14807
jimmycann wants to merge 4 commits into
nodejs:masterfrom
jimmycann:issue/12802

refack commented Aug 13, 2017 •
edited
Loading

silverwind commented Aug 13, 2017 •
edited
Loading