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.

Sign up for GitHub

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

Jump to bottom

Extendable ca certs v4 #9771

Closed
wants to merge 116 commits into from
Closed

Extendable ca certs v4 #9771

wants to merge 116 commits into from

Conversation

sam-github
Copy link
Contributor

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

@nodejs-github-bot 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 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
@Trott @MylesBorins
test,lib,benchmark: match function names
4d3e158 
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 <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
ofrobots and others added 18 commits January 12, 2017 17:29
@ofrobots @MylesBorins
deps: V8: fix debug backtrace for symbols
2677b9b 
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 <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins
deps: backport d800a65 from V8 upstream
47d18d4 
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
  R=jochen@chromium.org

  Review-Url: https://codereview.chromium.org/2078403002
  Cr-Commit-Position: refs/heads/master@{nodejs#37108}

PR-URL: nodejs#10668
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
@MylesBorins
deps: backport 7a88ff3 from V8 upstream
83144af 
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/

  R=jochen@chromium.org
  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 <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
@MylesBorins
deps: backport a715957 from V8 upstream
e0db108 
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
  R=hpayer@chromium.org

  Review-Url: https://codereview.chromium.org/2102243002
  Cr-Commit-Position: refs/heads/master@{nodejs#37366}

PR-URL: nodejs#10668
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
@MylesBorins
deps: update patch level in V8
20bee0f 
PR-URL: nodejs#10668
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
@bripkens @MylesBorins
v8,src: expose statistics about heap spaces
504b01b 
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 <cjihrig@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@cjihrig @MylesBorins
child_process: add shell option to spawn()
1c81530 
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 <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@ralt @MylesBorins
fs: add the fs.mkdtemp() function.
12ede48 
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 <thechargingvolcano@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@indutny @MylesBorins
process: add process.memoryUsage.external
2912da4 
PR-URL: nodejs#9587
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@kmccmk9 @MylesBorins
test: refactor test-net-keepalive.js
6b66647 
- 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 <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@mattcphillips @MylesBorins
test: check for error on invalid signal
821498e 
Asserts that an error should be thrown when
an invalid signal is passed to process.kill().

PR-URL: nodejs#10026
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@danbev @MylesBorins
tools: add macosx-firwall script to avoid popups
25fea45 
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 <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@italoacasas @MylesBorins
test: stream readableListening internal state
3bd7ab1 
PR-URL: nodejs#9864
Refs: nodejs#8683
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@Trott @MylesBorins
test: refactor test-handle-wrap-close-abort
b302358 
* 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 <santiago.gimeno@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@jasnell
process: add process.cpuUsage() - implementation, doc, tests
151cca6 
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 <robert.lindstaedt@gmail.com>
  Reviewed-By: James M Snell <jasnell@gmail.com>
  Reviewed-By: Trevor Norris <trev.norris@gmail.com>
  Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

PR-URL: nodejs#10796
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@MylesBorins
tls, crypto: add ALPN Support
1a40f2d 
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 <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins
tls,crypto: move NPN protcol data to hidden value
d706c0d 
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 <info@bnoordhuis.nl>
@MylesBorins
test: fix alpn tests for openssl1.0.2h
43ee08f 
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 <myles.borins@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
jhwohlgemuth and others added 11 commits January 23, 2017 10:25
@jhwohlgemuth @targos
test: test: refactor test-sync-fileread
657d3f3 
change equal to strictEqual and var to const

PR-URL: nodejs#9941
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos
test: refactor test-tls-0-dns-altname
fb9a704 
* var -> const, let
* assert.equal() -> assert.strictEqual()

PR-URL: nodejs#9948
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos
tools: add ESLint rule for assert.throws arguments
6625373 
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 <cjihrig@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos
tools: forbid template literals in assert.throws
f0ede65 
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 <princejohnwesley@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
@russokj @targos
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 <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@furnox @targos
test: refactoring test-pipe-head
869b0a6 
- Updated assert.equal to assert.strictEqual
- Updated 'var' to 'const'
- Using template literals

PR-URL: nodejs#10036
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
@targos
test: invalid package.json causes error when require()ing in directory
2c33751 
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 <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott @targos
test: refactor test-preload
50f7e2f 
* 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 <princejohnwesley@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@targos
lib,test: use consistent operator linebreak style
fa8f128 
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 <info@bnoordhuis.nl>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@targos
tools: enforce consistent operator linebreak style
84b312c 
Adds the `operator-linebreak` rule to our ESLint config.

PR-URL: nodejs#10178
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@italoacasas @targos
test: cleanup stream tests
f5c57c7 
const and let instead var
assert.strictEqual instead assert.equal

PR-URL: nodejs#8668
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@MylesBorins
Copy link
Contributor

@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

@MylesBorins MylesBorins mentioned this pull request Jan 24, 2017
Merged
@sam-github
Copy link
Contributor Author

Looking

@sam-github
crypto: allow adding extra certs to well-known CAs
b342679 
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 <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
@sam-github
Copy link
Contributor Author

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

sam-github
sam-github commented Jan 24, 2017
View reviewed changes
src/node_crypto.cc
root_cert_store,
extra_root_certs_file.c_str());
if (err) {
fprintf(stderr,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

MylesBorins commented Jan 24, 2017 via email

@jasnell
Copy link
Member

jasnell commented Jan 24, 2017

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

@MylesBorins
Copy link
Contributor

MylesBorins commented Jan 24, 2017 via email

@sam-github
Copy link
Contributor Author

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
Copy link
Member

jasnell commented Jan 27, 2017

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

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

landed in 1595328

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

@MylesBorins MylesBorins closed this Feb 6, 2017
@sam-github sam-github deleted the extendable-ca-certs-v4 branch February 14, 2017 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.