src: add --use-bundled-ca --use-openssl-ca check #12087

danbev · 2017-03-28T07:30:05Z

The --use-bundled-ca and --use-openssl-ca command line arguments are
mutually exclusive but can both be used on the same command line.

This commit adds a check if both options are used.

Fixes: #12083

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

src

danbev · 2017-03-28T07:30:57Z

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

danbev · 2017-03-28T07:53:38Z

test/osx/ failure looks unrelated:

not ok 1411 sequential/test-fs-readfile-tostring-fail
  ---
  duration_ms: 60.609
  severity: fail
  stack: |-
    timeout
  ...

bnoordhuis

LGTM modulo nits. It's nice we can have cctests for such things now.

bnoordhuis · 2017-03-28T08:20:38Z

src/node.cc

+#if HAVE_OPENSSL
+  if (use_openssl_ca && use_bundled_ca) {
+    fprintf(stderr,
+        "--use-openssl-ca or --use-bundled-ca can be used, not both\n");


Can you line up the arguments? There should probably be an "either" in front of the error message and we prefix it with argv[0] in most cases.

Ah I see that now, I'll fix that. Thx

richardlau · 2017-03-28T11:25:52Z

Are there any pros/cons for having the test written as a cctest as opposed to JavaScript?

danbev · 2017-03-28T11:37:05Z

@richardlau There are some situations where it might be easier to write unit tests, one example would be like the above when need to test command line options to Node. Another that we came across was when testing a feature that was intended to be used when Node is embedded (for example Electron). At least that is my take on it. There is a section in the writing-tests.md which mentions this.

richardlau · 2017-03-28T11:53:00Z

For testing embedded Node.js cctest makes sense. My concerns for things like the test in this PR is:

If you ever want to tweak/extend it you have to recompile the cctests.
The portion of the community who would be willing to look at the test if it was written in JavaScript compared to if the test was written in C++.

cc @nodejs/testing

cjihrig

I think this PR would be simpler, and more people would maintain this code in the future if the test were written in JS.

danbev · 2017-03-28T13:11:27Z

I actually thought using a C++ unit test in this case was making it simpler. The motivation for that is that it is testing command line arguments that are to be passed to Node upon startup. Maybe it is my lack of experience, but I can't think of a good way of testing that from JavaScript.

gibfahn · 2017-03-28T13:14:39Z

@danbev couldn't you just do something like this?

const execSync = require('child_process').execSync;
const output = execSync(`${process.execPath} --use-bundled-ca --use-openssl-ca`);
assert.strictEqual(output.toString(), "Error message...");

cjihrig · 2017-03-28T13:14:47Z

I think the simplest way would be to use child_process.spawnSync(process.execPath, ['--use-bundled-ca', '--use-openssl-ca']).

gibfahn · 2017-03-28T13:18:34Z

@cjihrig I recall that there is a reason to use one of spawnSync and execSync over the other, possibly to do with things working as expected on Windows. I can't remember which one it was though.

Is spawnSync the preferred one?

cjihrig · 2017-03-28T13:26:46Z

exec() runs a command to completion in a shell and then calls back with the result. execSync() is a blocking version of that. execFile() is the same, but without the shell.

spawn() is the most flexible, but usually requires the most effort/code to control. Under the hood, spawn() is used by the other asynchronous child process methods. spawnSync() takes the work out of using spawn(), if you can afford to block (which we can in a test like this). We don't need the shell in this case, so I'd go with spawnSync().

danbev · 2017-03-28T13:47:50Z

@gibfahn @cjihrig Thanks for that, I'll give that a go and see what people think.

richardlau · 2017-03-28T17:41:02Z

test/parallel/test-openssl-ca-options.js

+const result = child_process.spawnSync(process.execPath,
+                                      ['--use-bundled-ca', '--use-openssl-ca']);
+
+assert.strictEqual(result.stderr.toString(),


You can avoid having to call toString() here if you pass an encoding (e.g. utf8) option to child_process.spawnSync.

Ah nice, I'll do that. Thx

danbev · 2017-03-28T17:41:51Z

I've added a suggestion for the JavaScript test based on the suggestions from @gibfahn and @cjihrig.

My personal opinion is that the C++ unit test is just as readable/simple but I'm probably a little bias in this case :) It seems like the majority if for having this in JavaScript and I'll update the PR accordingly. Will let a few others chime and see if what the outcome is.

cjihrig · 2017-03-28T17:49:25Z

My personal opinion is that the C++ unit test is just as readable/simple but I'm probably a little bias in this case

I think the test itself is about equal in complexity, but it no longer requires changes to the gyp file and the fixture file. It also makes the test more accessible to a larger number of contributors.

cjihrig · 2017-03-28T17:55:00Z

test/parallel/test-openssl-ca-options.js

+'use strict';
+// This test check the usage of --use-bundled-ca and --use-openssl-ca arguments
+// to verify that both are not used at the same time.
+require('../common');


After this line, can you add:

if (!common.hasCrypto) { common.skip('missing crypto'); process.exit(); }

Ah yes, I'll add that. Thx

danbev · 2017-03-28T17:57:24Z

I think the test itself is about equal in complexity, but it no longer requires changes to the gyp file and the fixture file.

The change to the fixture would not normally be part of that. This was a bug that I introduced but did not discover until I actually used the arguments in this PR :( But there would still be a change required for the gyp file.

gibfahn · 2017-03-28T18:13:35Z

test/parallel/test-openssl-ca-options.js

@@ -0,0 +1,13 @@
+'use strict';
+// This test check the usage of --use-bundled-ca and --use-openssl-ca arguments


nit check -> checks

Fixed now, thanks!

Currently argv_[1] and argv_[2] are getting truncated by one character because of an incorrect addition of one to account for the null character. I only noticed this when working on nodejs#12087, but that fix will probably not get included in favor of a JavaScript test so I'm adding this separate commit for it. Refs: nodejs#12087

gibfahn · 2017-03-29T23:24:55Z

I think the test itself is about equal in complexity, but it no longer requires changes to the gyp file and the fixture file. It also makes the test more accessible to a larger number of contributors.

For me the accessibility is key, I think the vast majority of node collaborators are more familiar with tests written in js, not least for things like using the inspector for debugging.

The --use-bundled-ca and --use-openssl-ca command line arguments are mutually exclusive but can both be used on the same command line. This commit adds a check if both options are used. Fixes: #12083 Backport-PR-URL: #17783 PR-URL: #12087 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

Currently, the following warning will be reported when configuring without-ssl: ../src/node.cc:3653:8: warning: unused variable 'use_bundled_ca' [-Wunused-variable] bool use_bundled_ca = false; ^ ../src/node.cc:3654:8: warning: unused variable 'use_openssl_ca' [-Wunused-variable] bool use_openssl_ca = false; ^ I missed this when working on commit 8a7db9d ("src: add --use-bundled-ca --use-openssl-ca check"). Refs: #12087 Backport-PR-URL: #17783 PR-URL: #12302 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Currently argv_[1] and argv_[2] are getting truncated by one character because of an incorrect addition of one to account for the null character. I only noticed this when working on #12087, but that fix will probably not get included in favor of a JavaScript test so I'm adding this separate commit for it. Refs: #12087 Backport-PR-URL: #18113 PR-URL: #12110 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

The --use-bundled-ca and --use-openssl-ca command line arguments are mutually exclusive but can both be used on the same command line. This commit adds a check if both options are used. Fixes: #12083 Backport-PR-URL: #17783 PR-URL: #12087 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

Currently, the following warning will be reported when configuring without-ssl: ../src/node.cc:3653:8: warning: unused variable 'use_bundled_ca' [-Wunused-variable] bool use_bundled_ca = false; ^ ../src/node.cc:3654:8: warning: unused variable 'use_openssl_ca' [-Wunused-variable] bool use_openssl_ca = false; ^ I missed this when working on commit 8a7db9d ("src: add --use-bundled-ca --use-openssl-ca check"). Refs: #12087 Backport-PR-URL: #17783 PR-URL: #12302 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Currently argv_[1] and argv_[2] are getting truncated by one character because of an incorrect addition of one to account for the null character. I only noticed this when working on #12087, but that fix will probably not get included in favor of a JavaScript test so I'm adding this separate commit for it. Refs: #12087 Backport-PR-URL: #18113 PR-URL: #12110 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

This LTS release comes with 109 commits, 17 of which are considered Semver-Minor. This includes 32 which are doc related, 29 which are test related, 8 which are build / tool related and 1 commit which updates a dependency. Notable Changes: * console: - added console.count() and console.clear() (James M Snell) #12678 * crypto: - expose ECDH class (Bryan English) #8188 - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas) #10209 - warn on invalid authentication tag length (Tobias Nießen) #17566 * deps: - upgrade libuv to 1.16.1 (cjihrig) #16835 * dgram: - added socket.setMulticastInterface() (Will Young) #7855 * http: - add agent.keepSocketAlive and agent.reuseSocket as to allow overridable keep-alive behavior of `Agent` (Fedor Indutny) #13005 * lib: - return this from net.Socket.end() (Sam Roberts) #13481 * module: - add builtinModules api that provides list of all builtin modules in Node (Jon Moss) #16386 * net: - return this from getConnections() (Sam Roberts) #13553 * promises: - more robust stringification for unhandled rejections (Timothy Gu) #13784 * repl: - improve require() autocompletion (Alexey Orlenko) #14409 * src: - add openssl-system-ca-path configure option (Daniel Bevenius) #16790 - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius) #12087 - add process.ppid (cjihrig) #16839 * tls: - accept `lookup` option for `tls.connect()` (Fedor Indutny) #12839 * tools, build: - a new macOS installer! (JP Wesselink) #15179 * url: - WHATWG URL api support (James M Snell) #7448 * util: - add %i and %f formatting specifiers (Roman Reiss) #10308 PR-URL: #18342