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

Upgrade to openssl-1.0.2p in Node-v8 #22320

Closed
wants to merge 7 commits into from
Closed

Upgrade to openssl-1.0.2p in Node-v8 #22320

wants to merge 7 commits into from

Conversation

shigeki
Copy link
Contributor

@shigeki shigeki commented Aug 14, 2018

This upgrades OpenSSL-1.0.2p. It has no updates of asm files.

test-tls-passphrase.js was failed due to the changes of openssl/openssl@18026c0. This PR includes the fix of the test in 0ec9c32cf9bb99d8ff0effc15de6afb319ebce16.

Fixes: #22187

@nodejs/security-wg

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

@shigeki shigeki added test Issues and PRs related to the tests. openssl Issues and PRs related to the OpenSSL dependency. labels Aug 14, 2018
@nodejs-github-bot
Copy link
Collaborator

@shigeki build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/562/pipeline

@nodejs-github-bot nodejs-github-bot added openssl Issues and PRs related to the OpenSSL dependency. v8.x labels Aug 14, 2018
@shigeki
Copy link
Contributor Author

shigeki commented Aug 14, 2018

node-test-linux-linked-openssl102 would be failed in test-tls-passphrase because the CI server still has openssl-1.0.2o. Others test failures seems to be not related to this PR.

@shigeki shigeki mentioned this pull request Aug 15, 2018
Closed
4 tasks
@rvagg
Copy link
Member

rvagg commented Aug 15, 2018

@shigeki I was trying to pull this on to v8.x cause we're not releasing everything that's on v8.x-staging but there's conflicts in ecs_ossl.c (and maybe others) because of the patches we were going to float. I would prefer to ditch those now that we have the final version.

Currently I'm messing around with squashing them in to the 1.0.2p commit you added here but I'm not confident that we have precisely the right source in the end. Are you around and can you do this against v8.x instead?

@shigeki shigeki changed the base branch from v8.x-staging to v8.x August 15, 2018 03:35
@shigeki shigeki changed the base branch from v8.x to v8.x-staging August 15, 2018 03:35
@shigeki
Copy link
Contributor Author

shigeki commented Aug 15, 2018

@rvagg Okay, I will change this against v8.x branch.

shigeki and others added 7 commits August 15, 2018 12:38
@shigeki
deps: upgrade openssl sources to 1.0.2p
0da98b9 
This replaces all sources of openssl-1.0.2p.tar.gz into
deps/openssl/openssl
@shigeki
deps: copy all openssl header files to include dir
fba2f54 
All symlink files in `deps/openssl/openssl/include/openssl/`
are removed and replaced with real header files to avoid
issues on Windows. Two files of opensslconf.h in crypto and
include dir are replaced to refer config/opensslconf.h.
@indutny @shigeki
deps: fix openssl assembly error on ia32 win32
2e2066f 
`x86masm.pl` was mistakenly using .486 instruction set, why `cpuid` (and
perhaps others) are requiring .686 .

Fixes: nodejs#589
PR-URL: nodejs#1389
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
@shigeki
deps: fix asm build error of openssl in x86_win32
65178a8 
See
https://mta.openssl.org/pipermail/openssl-dev/2015-February/000651.html

iojs needs to stop using masm and move to nasm or yasm on Win32.

Fixes: nodejs#589
PR-URL: nodejs#1389
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@shigeki
openssl: fix keypress requirement in apps on win32
412d796 
Reapply b910613 .

Fixes: nodejs#589
PR-URL: nodejs#1389
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@shigeki
deps: add -no_rand_screen to openssl s_client
d581739 
In openssl s_client on Windows, RAND_screen() is invoked to initialize
random state but it takes several seconds in each connection.
This added -no_rand_screen to openssl s_client on Windows to skip
RAND_screen() and gets a better performance in the unit test of
test-tls-server-verify.
Do not enable this except to use in the unit test.

Fixes: nodejs#1461
PR-URL: nodejs#1836
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@shigeki
test: fix error messages for OpenSSL-1.0.2p
127eff6 
After upgradeing OpenSSL-1.0.2p, test-tls-passphrase.js was failed
due to change of error messages.

Ref: openssl/openssl@18026c0
@shigeki shigeki changed the base branch from v8.x-staging to v8.x August 15, 2018 03:41
@shigeki
Copy link
Contributor Author

shigeki commented Aug 15, 2018

@rvagg Done. Could you try CI?

@shigeki
Copy link
Contributor Author

shigeki commented Aug 15, 2018

I found that test-tls-server-verify.js was failed because 0c9760d is missed in v8.x branch. Others seem to be good.

@rvagg
Copy link
Member

rvagg commented Aug 15, 2018

Yep, we have 0c9760d included in this release already so it's all sorted. CI is good for this and the other commits queued up for release. The only red is on v6 for a GYP addon compile failure on Windows for vcbt2015 which I don't believe is anything to do with our release. Thanks @shigeki!

@joyeecheung joyeecheung mentioned this pull request Aug 15, 2018
Closed
This was referenced Aug 16, 2018
Closed
Closed
@rvagg
Copy link
Member

rvagg commented Aug 16, 2018

landed in 6.x and 8.x and published in 6.14.4, 8.11.4, thanks @shigeki!

@rvagg rvagg closed this Aug 16, 2018
@beevelop beevelop mentioned this pull request Aug 16, 2018
Closed
@beevelop beevelop mentioned this pull request Aug 16, 2018
Closed
@joyeecheung joyeecheung mentioned this pull request Aug 17, 2018
Closed
@MylesBorins MylesBorins mentioned this pull request Aug 17, 2018
Merged
@joyeecheung joyeecheung mentioned this pull request Aug 17, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rvagg rvagg rvagg approved these changes

Assignees
No one assigned
Labels
openssl Issues and PRs related to the OpenSSL dependency. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@shigeki @nodejs-github-bot @rvagg @indutny