7.x backport: crypto: support OPENSSL_CONF again (and its dependencies) #11345
Conversation
|
/to @italoacasas @bnoordhuis |
|
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? |
|
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. |
ac72c3e to
2f1ce29
Compare
|
I assume v7.x-staging got rebased, this PR now has 110 commits. Could you rebase @sam-github ? |
|
This is my bad, I had to force push to remove some commits with specs issues. |
|
@italoacasas I think that's pretty much unavoidable with the number of commits you're juggling. |
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>
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>
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>
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>
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>
4f98413 to
a3bf4a2
Compare
|
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 |
|
@italoacasas which means I can help with anything for the next 5 hours, but not after |
|
Moving this to |
|
Landed in |
|
@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 |
Backport of:
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)