FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

7.x backport: crypto: support OPENSSL_CONF again (and its dependencies) by sam-github · Pull Request #11345 · nodejs/node · GitHub

/ node Public

7.x backport: crypto: support OPENSSL_CONF again (and its dependencies) - #11345

Closed
sam-github wants to merge 5 commits into
nodejs:v7.x-stagingfrom
sam-github:backport-pr-11051-11255-11006
ConversationCommits (5)ChecksFiles changed
Closed

7.x backport: crypto: support OPENSSL_CONF again (and its dependencies)#11345
sam-github wants to merge 5 commits into
nodejs:v7.x-stagingfrom
sam-github:backport-pr-11051-11255-11006

Conversation

sam-github commented Feb 13, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Backport of:

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 added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. i18n-api Issues and PRs related to the i18n implementation. process Issues and PRs related to the process subsystem. v7.x labels Feb 13, 2017

sam-github commented Feb 13, 2017

Copy link
Copy Markdown
Contributor Author

/to @italoacasas @bnoordhuis
/cc @gibfahn @mhdawson

sam-github commented Feb 13, 2017

Copy link
Copy Markdown
Contributor Author

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

italoacasas commented Feb 13, 2017 •
edited
Loading

Copy link
Copy Markdown

thanks @sam-github, I would like to fast-track this backport, potentially landing this tomorrow that way we can include this in the RC. Thoughts?

sam-github mentioned this pull request Feb 13, 2017
Merged

sam-github commented Feb 13, 2017

Copy link
Copy Markdown
Contributor Author

Agree on fast-tracking.

I didn't think the 48 hour delay applied to backports, but we should get a review by @bnoordhuis soon, or perhaps @jasnell can review these?

The reason they didn't cherrypick clean is that #11051 touched a CLI switch that was only introduced in #10116 (which is semver-major). I just removed the part of the commit that touch the switch that doesn't exist in 7.x.

sam-github mentioned this pull request Feb 14, 2017
Closed
4 tasks

gibfahn commented Feb 15, 2017

Copy link
Copy Markdown
Member

I assume v7.x-staging got rebased, this PR now has 110 commits. Could you rebase @sam-github ?

italoacasas commented Feb 15, 2017

Copy link
Copy Markdown

This is my bad, I had to force push to remove some commits with specs issues.

gibfahn commented Feb 15, 2017

Copy link
Copy Markdown
Member

@italoacasas I think that's pretty much unavoidable with the number of commits you're juggling.

bnoordhuis and others added 5 commits February 16, 2017 09:06
src: make copies of startup environment variables
dcdebc3 
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>
src: make --icu-data-dir= switch testable
ce10b06 
Move some code around so we can properly test whether the switch
actually does anything.

PR-URL: nodejs#11255
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
src: fix --icu-data-dir= regression
d0b9c34 
Commit a8734af ("src: make copies of startup environment variables")
from two weeks ago introduced a regression in the capturing of the
`--icu-data-dir=` switch: it captured the string up to the `=` instead
of what comes after it.

PR-URL: nodejs#11255
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
src: add SafeGetenv() to internal API
dc77c6e 
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>
crypto: support OPENSSL_CONF again
a3bf4a2 
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 force-pushed the backport-pr-11051-11255-11006 branch from 4f98413 to a3bf4a2 Compare February 16, 2017 17:07

sam-github commented Feb 16, 2017

Copy link
Copy Markdown
Contributor Author

rebased

btw, will be without internet connection from this afternoon, until Sunday morning

also, @italoacasas I don't quite understand why I needed to rebase, the changes all cherry-pick clean I think, based on the fact that the rebase was completely conflict free

sam-github commented Feb 16, 2017

Copy link
Copy Markdown
Contributor Author

@italoacasas which means I can help with anything for the next 5 hours, but not after

italoacasas commented Feb 16, 2017 •
edited
Loading

Copy link
Copy Markdown

Moving this to staging right now

italoacasas commented Feb 16, 2017

Copy link
Copy Markdown

Landed in v7-staging. Thanks for the help @sam-github

sam-github added lts-watch-v6.x semver-minor PRs that contain new features and should be released in the next minor version. and removed lts-watch-v4.x labels Feb 27, 2017

sam-github commented Feb 27, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@nodejs/lts I request acceptance for 6.x, it isn't meaningful for 4.x, which still respects OPENSSL_CONF

It doesn't land clean on 6.x, I am backporting.

EDIT: backported: #11583

sam-github mentioned this pull request Feb 27, 2017
Closed
4 tasks
sam-github deleted the backport-pr-11051-11255-11006 branch April 17, 2017 20:36
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bnoordhuis bnoordhuis approved these changes

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. i18n-api Issues and PRs related to the i18n implementation. process Issues and PRs related to the process subsystem. 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.

6 participants

Back | FazBrowse Home | New Git URL