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

6.x backport: crypto: support OPENSSL_CONF again (and its dependencies) #11583

Closed
wants to merge 33 commits into from
Closed

6.x backport: crypto: support OPENSSL_CONF again (and its dependencies) #11583

wants to merge 33 commits into from

Conversation

sam-github
Copy link
Contributor

Backport #11345 to 6.x

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. labels Feb 27, 2017
@sam-github sam-github changed the base branch from master to v6.x-staging February 27, 2017 18:45
@sam-github sam-github added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. lts-agenda semver-minor PRs that contain new features and should be released in the next minor version. labels Feb 27, 2017
@sam-github sam-github mentioned this pull request Feb 27, 2017
Closed
4 tasks
@sam-github
Copy link
Contributor Author

/to @nodejs/lts

@sam-github
Copy link
Contributor Author

ci is down ATM, so I can't kick a test run off for this, I'll try again later.

@jasnell
Copy link
Member

jasnell commented Feb 27, 2017

LGTM once CI is up and green

@sam-github
Copy link
Contributor Author

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

@gibfahn gibfahn changed the title 7.x backport: crypto: support OPENSSL_CONF again (and its dependencies 6.x backport: crypto: support OPENSSL_CONF again (and its dependencies) Feb 27, 2017
richardlau
richardlau reviewed Mar 1, 2017
View reviewed changes
src/node.cc
@@ -4321,6 +4331,9 @@ void Init(int* argc,
V8::SetFlagsFromString(NODE_V8_OPTIONS, sizeof(NODE_V8_OPTIONS) - 1);
#endif

if (openssl_config.empty())
SafeGetenv("OPENSSL_CONF", &openssl_config);
Copy link
Member

Choose a reason for hiding this comment

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

See #11618

@gibfahn
Copy link
Member

gibfahn commented Mar 1, 2017

As @richardlau says, this should be backported with #11618 once that lands.

@gibfahn gibfahn removed the meta Issues and PRs related to the general management of the project. label Mar 4, 2017
@gibfahn gibfahn mentioned this pull request Mar 4, 2017
Closed
@MylesBorins MylesBorins force-pushed the v6.x-staging branch 2 times, most recently from b3f32f9 to e71fc70 Compare March 8, 2017 09:27
@sam-github
Copy link
Contributor Author

rebased again to resolve conflicts in usage message

@mscdex
Copy link
Contributor

mscdex commented Mar 22, 2017

This doesn't look like it's rebased properly? There's tons of unrelated commits.

@gibfahn
Copy link
Member

gibfahn commented Mar 22, 2017

Probably rebased before 6.10.1

@sam-github
Copy link
Contributor Author

weird that it was claimed to conflict, it rebased clean

sam-github and others added 23 commits April 13, 2017 15:30
@sam-github @MylesBorins
doc: new TLSSocket has no secure context options
3855504 
Unlike all the other tls APIs, if any secure context configuration is
required, the caller is responsible for creating the context.

Corrects a doc regression introduced in caa7fa9.

PR-URL: nodejs#10545
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@zbjornson @MylesBorins
doc: clarify memory sharing behavior of buffer ctor
a6ab9f2 
* Clarify that memory is always shared and never copied.
* Fix wording that sounded like ArrayBuffer has a buffer property.

PR-URL: nodejs#10778
Ref: nodejs#10770
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@edsadr @MylesBorins
test: improve test-fs-access
b85d15f 
* use const and let instead of var
* use common.mustCall to control functions execution
* use assert.ifError instead of assert.strictEqual for errors
* use arrow functions

PR-URL: nodejs#10542
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@cjihrig @MylesBorins
test: refactor test-beforeexit-event-exit.js
3cde69a 
PR-URL: nodejs#10577
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@gibfahn @MylesBorins
test: s/assert.equal/assert.strictEqual/
a654502 
Use assert.strictEqual instead of assert.equal in tests, manually
convert types where necessary.

Backport-PR-URL: nodejs#11795
PR-URL: nodejs#10698
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
@gibfahn @MylesBorins
assert, tools: enforce strict (not)equal in eslint
acd9bc3 
Extend no-restricted-properties to catch use of assert.equal() and
assert.notEqual() and require assert.strictEqual() or
assert.notStrictEqual() instead.

Also update the eslint-ignore in lib/assert.js to avoid
assert.equal/notEqual linter errors in their definitions.

Backport-PR-URL: nodejs#11795
PR-URL: nodejs#10698
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
@DavidGoussev @MylesBorins
test: refactor test-stream2-readable-wrap.js
42d1fb1 
Use common.mustCall() where appropriate, var to const/let,
assert.equal() -> assert.strictEqual(), explicit time provided to
setTimeout()

Backport-PR-URL: nodejs#11797
PR-URL: nodejs#10551
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@sam-github @MylesBorins
test: getgroups() may contain duplicate GIDs
f23c818 
Some systems may have multiple group names with the same group ID, in
which case getgroups() returns duplicate values, where `id -G` will
filter the duplicates. Unique and sort the arrays so they can be
compared.

Backport-PR-URL: nodejs#12468
PR-URL: nodejs#10389
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@sam-github @MylesBorins
doc,test: tls .ca option supports multi-PEM files
c555f53 
Backport-PR-URL: nodejs#12468
PR-URL: nodejs#10389
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@sam-github @MylesBorins
test: tls cert chain completion scenarios
54486c0 
Backport-PR-URL: nodejs#12468
PR-URL: nodejs#10389
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@sam-github @MylesBorins
doc: use correct tls certificate property name
b631966 
Docs referred to an `issuer` property being optionally present, when it
should have referred to the `issuerCertificate` property.

Backport-PR-URL: nodejs#12468
PR-URL: nodejs#10389
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@sam-github @MylesBorins
test: check tls server verification with addCACert
cdc354a 
SecureContext.addCACert() adds to the existing root store,
preserving root cert entries. option.ca is applied without
calling SecureContext.addRootCerts() so should add to
the default, empty, root store.

This test confirms that the built-in root CAs are not included
when options.ca is used.

Based on:

shigeki@acd5837

Backport-PR-URL: nodejs#12468
PR-URL: nodejs#10389
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@sam-github @MylesBorins
test: move common tls connect setup into fixtures
b0daa9b 
TLS connection setup boilerplate is common to many TLS tests, factor it
into a test fixture so tests are clearer to read and faster to write.

Backport-PR-URL: nodejs#12468
PR-URL: nodejs#10389
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@evanlucas @MylesBorins
doc: make os api doc more consistent
7b121f1 
This adds a missing Returns to os.arch() as well as a missing added in
version to os.constants.

PR-URL: nodejs#10994
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@seishun @MylesBorins
doc: fix typo in STYLE_GUIDE.md
dcf20bc 
PR-URL: nodejs#11615
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jsumners @MylesBorins
doc: use common malformed instead of misformatted
7a6683e 
PR-URL: nodejs#11518
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@joyeecheung @MylesBorins
doc: add link to references in net.Socket
de13d62 
PR-URL: nodejs#11625
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@thefourtheye @MylesBorins
test: skip tests with common.skip
1427c0e 
The `common.skip` function adds proper message in TAP format to skipped
tests. It is better not to have the message rewritten in the tests.

PR-URL: nodejs#11585
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
@vsemozhetbyt @MylesBorins
doc: fixup errors.md
ea719a1 
* add semicolons in examples
* fix indentation in code example
* add spaces in code examples
* console.log() -> console.error()
* fix level of headings
* update comment code example
* delete obsolete info and example

Fixes: nodejs#11558
PR-URL: nodejs#11566
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@bnoordhuis @MylesBorins
net: remove misleading comment
c60fd7f 
The allowHalfOpen comment was added in commit 8a3befa ("net: Refactor
to use streams2") from 2012 but it wasn't true even then as far as I
can tell: Node.js simply always does a shutdown(2) first.

It is true that streams2 withholds the 'end' event when allowHalfOpen
is true but the comment is about a callback that hangs off the 'finish'
event that is emitted after calling `socket.end()`.

PR-URL: nodejs#11573
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@bnoordhuis @sam-github
src: make copies of startup environment variables
6765c88 
Mutations of the environment can invalidate pointers to environment
variables, so make `secure_getenv()` copy them out instead of returning
pointers.

PR-URL: nodejs#11051
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
@sam-github
src: add SafeGetenv() to internal API
0dc1d85 
Allow it to be used anywhere in src/ that env variables with security
implications are accessed.

PR-URL: nodejs#11006
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@sam-github
crypto: support OPENSSL_CONF again
b89ea8a 
A side-effect of https://github.com/nodejs/node-private/pull/82
was to remove support for OPENSSL_CONF, as well as removing the default
read of a configuration file on startup.

Partly revert this, allowing OPENSSL_CONF to be used to specify a
configuration file to read on startup, but do not read a file by
default.

If the --openssl-config command line option is provided, its value is
used, not the OPENSSL_CONF environment variable.

Fix: nodejs#10938
PR-URL: nodejs#11006
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@sam-github sam-github mentioned this pull request Apr 26, 2017
Closed
4 tasks
@sam-github
Copy link
Contributor Author

@nodejs/lts did we decide this is going to land in the next 6.x minor? If so, can we label it land-on-6.x?

It its intended to land I'll rework #12677 to use the SafeGetenv() introduced here.

This makes me wonder if we need two staging branches, one for the next patch, one for the next minor. Or perhaps no backporting should occur until the next 6.x patch is released from -staging. PRs build on each other, so its better if they land in order.

@MylesBorins
Copy link
Contributor

MylesBorins commented May 16, 2017 •
edited
Loading

landed in dc7cbf6, 276f3e6, 0fc4955

@sam-github sam-github deleted the v6.x-backport-pr-11345 branch October 16, 2018 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau left review comments

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations. 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.