Extendable ca certs v4#9771

sam-github · 2016-11-23T17:02:44Z

Backport of #9139 to v4 to resolve conflicts.

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

tls,https

In most cases, named functions match the variable or property to which they are being assigned. That also seems to be the practice in a series of PRs currently being evaluated that name currently-anonymous functions. This change applies that rule to instances in the code base that don't comply with that practice. This will be enforceable with a lint rule once we upgrade to ESLint 3.8.0. PR-URL: nodejs#9113 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>

The cherry-pick of nodejs#7612 to v4.x (4369055) added in nodejs#9298 wasn't quite correct as it depends on a runtime function %SymbolDescriptiveString that doesn't exist on v4.x. We can use %SymbolDescription instead. Ref: nodejs#7612 Ref: nodejs#9298 PR-URL: nodejs#10732 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

This backport does not include the original changes to SLOW_DCHECK as it does not exist in the V8 in node v4.x Original commit message: Filter out stale left-trimmed handles BUG=chromium:620553 LOG=N [email protected] Review-Url: https://codereview.chromium.org/2078403002 Cr-Commit-Position: refs/heads/master@{nodejs#37108} PR-URL: nodejs#10668 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]>

This backport does not include the changes to `src/heap/scavenger.cc` as it does not exist in the V8 included in the v4.x stream. Original commit message: Filter out stale left-trimmed handles for scavenges The missing part from https://codereview.chromium.org/2078403002/[email protected] BUG=chromium:621869 LOG=N Review-Url: https://codereview.chromium.org/2077353004 Cr-Commit-Position: refs/heads/master@{nodejs#37184} PR-URL: nodejs#10668 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]>

This commit does not include the changes to `src/heap/scavenger.cc`. These changes would revert the changes that should have come in 086bd5aede, meaning that there is no issue with that change missing in the previous commit. Original commit message: Iterate handles with special left-trim visitor BUG=chromium:620553 LOG=N [email protected] Review-Url: https://codereview.chromium.org/2102243002 Cr-Commit-Position: refs/heads/master@{nodejs#37366} PR-URL: nodejs#10668 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]>

PR-URL: nodejs#10668 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]>

Provide means to inspect information about the separate heap spaces via a callable API. This is helpful to analyze memory issues. Fixes: nodejs#2079 PR-URL: nodejs#4463 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

This commit adds a shell option, to spawn() and spawnSync(). This option allows child processes to be spawned with or without a shell. The option also allows a custom shell to be defined, for compatibility with exec()'s shell option. Fixes: nodejs#1009 PR-URL: nodejs#4598 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

This uses libuv's mkdtemp function to provide a way to create a temporary folder, using a prefix as the path. The prefix is appended six random characters. The callback function will receive the name of the folder that was created. Usage example: fs.mkdtemp('/tmp/foo-', function(err, folder){console.log(folder); // Prints: /tmp/foo-Tedi42 }); The fs.mkdtempSync version is also provided. Usage example: console.log(fs.mkdtemp('/tmp/foo-')); // Prints: tmp/foo-Tedi42 This pull request also includes the relevant documentation changes and tests. PR-URL: nodejs#5333 Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Trevor Norris <[email protected]>

PR-URL: nodejs#9587 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

- Replace require() vars with const. - Replace assert.equal() with assert.strictEqual(). - Add common.mustCall() to the setTimeout() callback. PR-URL: nodejs#9995 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Asserts that an error should be thrown when an invalid signal is passed to process.kill(). PR-URL: nodejs#10026 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Currently, there are a number of popups that get displayed when running the tests asking to accept incoming network connections. Rules can be added manually to the socket firewall on Mac OS X but getting this right might not be obvious and quite a lot of time can be wasted trying to get the rules right. This script hopes to simplify things a little so that it can be re-run when needed. The script should be runnable from both the projects root directory and from the tools directory, for example: $ sudo ./tools/macosx-firewall.sh Fixes: nodejs#8911 PR-URL: nodejs#10114 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

PR-URL: nodejs#9864 Refs: nodejs#8683 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

* use common.mustCall() to confirm number of uncaught exceptions * var -> const * specify duration of 1ms for setTimeout() and setInterval() PR-URL: nodejs#10188 Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

Backport to v4.x Original commit message: Add process.cpuUsage() method that returns the user and system CPU time usage of the current process PR-URL: nodejs#6157 Reviewed-By: Robert Lindstaedt <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> PR-URL: nodejs#10796 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

cherry-pick 802a2e7 from v6-staging. ALPN is added to tls according to RFC7301, which supersedes NPN. When the server receives both NPN and ALPN extensions from the client, ALPN takes precedence over NPN and the server does not send NPN extension to the client. alpnProtocol in TLSSocket always returns false when no selected protocol exists by ALPN. In https server, http/1.1 token is always set when no options.ALPNProtocols exists. PR-URL: nodejs#2564 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

cherry-pick 7eee372 from v6-staging. This fix is to be consistent implementation with ALPN. Tow NPN protocol data in the persistent memebers move to hidden variables in the wrap object. PR-URL: nodejs#2564 Reviewed-By: Ben Noordhuis <[email protected]>

cherry-pick 65030c7 from v6-staging. openssl/openssl@af2db04 changed some ALPN behaviors. The tests when ALPN has no selection should be fixed because openssl was changed NPN callback to be invoked in this case. Fixes: nodejs#6458 PR-URL: nodejs#6550 Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

change equal to strictEqual and var to const PR-URL: nodejs#9941 Reviewed-By: Prince John Wesley <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rich Trott <[email protected]>

* var -> const, let * assert.equal() -> assert.strictEqual() PR-URL: nodejs#9948 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Italo A. Casas <[email protected]> Reviewed-By: James M Snell <[email protected]>

The second argument to "assert.throws" is usually a validation RegExp or function for the thrown error. However, the function also accepts a string and in this case it is interpreted as a message for the AssertionError and not used for validation. It is common for people to forget this and pass a validation string by mistake. This new rule checks that we never pass a string literal as a second argument to "assert.throws". Additionally, there is an option to enforce the function to be called with at least two arguments. It is currently off because we have many tests that do not comply with this rule. PR-URL: nodejs#10089 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Extend the assert-throws-arguments custom ESLint rule to also check for the use of template literals as a second argument to assert.throws. PR-URL: nodejs#10301 Ref: nodejs#10282 (comment) Reviewed-By: Prince John Wesley <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Italo A. Casas <[email protected]>

The assert.throws() calls in test-event-emitter-max-listeners.js should include a constructor or RegExp as a second argument. PR-URL: nodejs#9987 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

- Updated assert.equal to assert.strictEqual - Updated 'var' to 'const' - Using template literals PR-URL: nodejs#10036 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Italo A. Casas <[email protected]>

Requiring a file from a directory that contains an invalid package.json file should throw an error. PR-URL: nodejs#10044 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

* assert.equal() -> assert.strictEqual() * replace template string with a string; no variable substitution or concatenation or anything like that PR-URL: nodejs#9803 Reviewed-By: Prince John Wesley <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

We have a tacit rule that for multiline statements, the operator should be placed before the linebreak. This commit commit fixes the few violations of this rule in the code base. This allows us to enable the corresponding ESLint rule. PR-URL: nodejs#10178 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

Adds the `operator-linebreak` rule to our ESLint config. PR-URL: nodejs#10178 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

const and let instead var assert.strictEqual instead assert.equal PR-URL: nodejs#8668 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ilkka Myller <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

MylesBorins · 2017-01-24T07:34:11Z

@sam-github I'm seeing a failure here on v4.x using osx 10.10

/usr/local/opt/python/bin/python2.7 tools/test.py --mode=release doctool message pseudo-tty parallel sequential -J === release test-tls-env-bad-extra-ca === Path: parallel/test-tls-env-bad-extra-ca assert.js:85 throw new assert.AssertionError({^ AssertionError: null == true at ChildProcess.<anonymous> (/Users/thealphanerd/code/node/v4.x/test/parallel/test-tls-env-bad-extra-ca.js:36:5) at ChildProcess.<anonymous> (/Users/thealphanerd/code/node/v4.x/test/common.js:408:15) at emitTwo (events.js:87:13) at ChildProcess.emit (events.js:172:7) at maybeClose (internal/child_process.js:854:16) at Process.ChildProcess._handle.onexit (internal/child_process.js:222:5) Command: out/Release/node /Users/thealphanerd/code/node/v4.x/test/parallel/test-tls-env-bad-extra-ca.js

sam-github · 2017-01-24T19:47:31Z

Looking

In closed environments, self-signed or privately signed certificates are commonly used, and rejected by Node.js since their root CAs are not well-known. Allow extending the set of well-known compiled-in CAs via environment, so they can be set as a matter of policy. PR-URL: nodejs#9139 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]>

sam-github · 2017-01-24T20:38:16Z

There was no emitWarning() in 4.x, reworking the PR.

sam-github · 2017-01-24T21:28:20Z

src/node_crypto.cc

+ root_cert_store,
+ extra_root_certs_file.c_str());
+if (err){
+fprintf(stderr,


@nodejs/lts @nodejs/crypto there is no process.emitWarning() in 4.x (why?) so I think the options are print to stderr (recall this should be quite unusual, and its so that you know your ENV var is not being respected, and why), or to silently ignore.
Opinions? I'm OK with either. stderr is more informative, but then, writing to stderr is to be shunned. Not to sure what to do here.

MylesBorins · 2017-01-24T21:33:57Z

@jasnell would it be possible to backport process.emitWarning?

…

On Tue, Jan 24, 2017, 1:28 PM Sam Roberts ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/node_crypto.cc <#9771 (review)>: > @@ -782,6 +816,18 @@ void SecureContext::AddRootCerts(const FunctionCallbackInfo<Value>& args){BIO_free_all(bp); X509_free(x509)} + + if (!extra_root_certs_file.empty()){+ unsigned long err = AddCertsFromFile( // NOLINT(runtime/int) + root_cert_store, + extra_root_certs_file.c_str()); + if (err){+ fprintf(stderr, @nodejs/lts <https://github.com/orgs/nodejs/teams/lts> @nodejs/crypto <https://github.com/orgs/nodejs/teams/crypto> there is no process.emitWarning() in 4.x (why?) so I think the options are print to stderr (recall this should be quite unusual, and its so that you know your ENV var is not being respected, and why), or to silently ignore. Opinions? I'm OK with either. stderr is more informative, but then, writing to stderr is to be shunned. Not to sure what to do here. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#9771 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecV5_I2C0C-FokjRggiSkKbdIHuWEeks5rVmz5gaJpZM4K61Jx> .

jasnell · 2017-01-24T21:40:47Z

Yeah that's possible. I'll work on that over the next week

MylesBorins · 2017-01-24T21:45:53Z

/cc @nodejs/lts does anyone have a concern with landing process.emitWarning? I think this will be a good backport to include

…

On Tue, Jan 24, 2017, 1:40 PM James M Snell ***@***.***> wrote: Yeah that's possible. I'll work on that over the next week — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#9771 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecV0RmDcp_xuUtsJE0jNwWB-fItu7dks5rVm_jgaJpZM4K61Jx> .

sam-github · 2017-01-24T21:52:43Z

OK, I'll rework this again once emitWarning is available. Or you can land now, and once emitWarning is available, I can PR a small change to remove the stderr? Or anything else you suggest.

jasnell · 2017-01-27T20:57:54Z

Thought that I would be able to get to porting emitWarning back to v4 this week, didn't have the time afterall. Have it in my short list of must do items for next week

sam-github · 2017-01-27T21:21:34Z

@nodejs/crypto @jasnell @MylesBorins We don't need to backport emit warning to land this, I can do either what this PR does now (fprintf(stderr, ...), or just ignore the error. Both are OK solutions IMO.

MylesBorins · 2017-02-06T19:17:06Z

landed in 1595328

@jasnell did you ever look into the backport of process.emiWarning?

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. v4.x labels Nov 23, 2016

MylesBorins added blocked PRs that are blocked by other issues or PRs. lts-agenda semver-minor PRs that contain new features and should be released in the next minor version. and removed blocked PRs that are blocked by other issues or PRs. labels Nov 28, 2016

jasnell approved these changes Dec 5, 2016
View reviewed changes

MylesBorins force-pushed the v4.x-staging branch from d860f6e to c77ba8cCompare December 21, 2016 19:34

sam-github force-pushed the extendable-ca-certs-v4 branch from 4efeeb5 to b16d231Compare January 3, 2017 21:44

MylesBorins force-pushed the v4.x-staging branch from 5dee225 to 4357591Compare January 4, 2017 00:04

sam-github force-pushed the extendable-ca-certs-v4 branch from b16d231 to 67da738Compare January 12, 2017 20:08

ofrobotsand others added 18 commits January 12, 2017 17:29

deps: update patch level in V8
20bee0f
PR-URL: nodejs#10668 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]>

test: stream readableListening internal state
3bd7ab1
PR-URL: nodejs#9864 Refs: nodejs#8683 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

jhwohlgemuthand others added 11 commits January 23, 2017 10:25

test: test: refactor test-sync-fileread
657d3f3
change equal to strictEqual and var to const PR-URL: nodejs#9941 Reviewed-By: Prince John Wesley <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rich Trott <[email protected]>

test: add second argument to assert.throws()
87463bf
The assert.throws() calls in test-event-emitter-max-listeners.js should include a constructor or RegExp as a second argument. PR-URL: nodejs#9987 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

MylesBorins mentioned this pull request Jan 24, 2017
v4.8.0 proposal #10973
Merged

sam-github force-pushed the extendable-ca-certs-v4 branch from 67da738 to c173bf0Compare January 24, 2017 19:50

fixup! crypto: allow adding extra certs to well-known CAs
0b2d0eb

sam-github force-pushed the extendable-ca-certs-v4 branch from c173bf0 to 0b2d0ebCompare January 24, 2017 21:16

sam-github commented Jan 24, 2017
View reviewed changes

MylesBorins force-pushed the v4.x-staging branch from f5c57c7 to 735119cCompare February 1, 2017 01:04

MylesBorins closed this Feb 6, 2017

sam-github deleted the extendable-ca-certs-v4 branch February 14, 2017 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Extendable ca certs v4#9771

Extendable ca certs v4 #9771

Uh oh!

sam-github commented Nov 23, 2016

Uh oh!

MylesBorins commented Jan 24, 2017

Uh oh!

sam-github commented Jan 24, 2017

Uh oh!

sam-github commented Jan 24, 2017

Uh oh!

sam-githubJan 24, 2017

Uh oh!

MylesBorins commented Jan 24, 2017 via email

Uh oh!

jasnell commented Jan 24, 2017

Uh oh!

MylesBorins commented Jan 24, 2017 via email

Uh oh!

sam-github commented Jan 24, 2017

Uh oh!

jasnell commented Jan 27, 2017

Uh oh!

sam-github commented Jan 27, 2017

Uh oh!

MylesBorins commented Feb 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

20 participants

Uh oh!

Extendable ca certs v4#9771

Extendable ca certs v4 #9771

Uh oh!

Conversation

sam-github commented Nov 23, 2016

Checklist

Affected core subsystem(s)

Uh oh!

MylesBorins commented Jan 24, 2017

Uh oh!

sam-github commented Jan 24, 2017

Uh oh!

sam-github commented Jan 24, 2017

Uh oh!

sam-githubJan 24, 2017

Choose a reason for hiding this comment

Uh oh!

MylesBorins commented Jan 24, 2017 via email

Uh oh!

jasnell commented Jan 24, 2017

Uh oh!

MylesBorins commented Jan 24, 2017 via email

Uh oh!

sam-github commented Jan 24, 2017

Uh oh!

jasnell commented Jan 27, 2017

Uh oh!

sam-github commented Jan 27, 2017

Uh oh!

MylesBorins commented Feb 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

20 participants