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.1.0i #22318

Closed
wants to merge 4 commits into from
Closed

Upgrade to OpenSSL-1.1.0i #22318

wants to merge 4 commits into from

Conversation

shigeki
Copy link
Contributor

@shigeki shigeki commented Aug 14, 2018

This upgrades OpenSSL-1.1.0i.

test-crypto-scrypt.js and test-tls-passphrase.js were failed due to the changes of
openssl/openssl@45ae18b and openssl/openssl@36d2517. This PR includes the fix of the tests in 0c8bc990aec71111b9f8e3879444c1bd31065015.

Fixes: #22187

@nodejs/crypto

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/561/pipeline

@nodejs-github-bot nodejs-github-bot added the openssl Issues and PRs related to the OpenSSL dependency. label Aug 14, 2018
@Trott
Copy link
Member

Trott commented Aug 14, 2018

@nodejs/security-wg

@shigeki
Copy link
Contributor Author

shigeki commented Aug 14, 2018

node-test-linux-linked-openssl110 would be failed in test-tls-passphrase and test-crypto-scrypt because the CI server still has openssl-1.1.0h.

@shigeki
deps: upgrade openssl sources to 1.1.0i
8315458 
This updates all sources in deps/openssl/openssl with openssl-1.1.0i.
@shigeki
deps: add s390 asm rules for OpenSSL-1.1.0
7ea3581 
This is a floating patch against OpenSSL-1.1.0 to generate asm files
with Makefile rules and it is to be submitted to the upstream.

Fixes: nodejs#4270
PR-URL: nodejs#19794
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@shigeki
deps: update archs files for OpenSSL-1.1.0i
415d6f9 
`cd deps/openssl/config; make` updates all archs dependant files.
@shigeki
test: fix error messages for OpenSSL-1.1.0i
5143132 
After upgradeing OpenSSL-1.1.0i, two tests are failed due to changes
of error messages.

Ref: openssl/openssl@45ae18b
Ref: openssl/openssl@36d2517
@Trott
Copy link
Member

Trott commented Aug 14, 2018

node-test-linux-linked-openssl110 would be failed in test-tls-passphrase and test-crypto-scryptbecause the CI server still has openssl-1.1.0h.

IIRC, shared-lib OpenSSL is observed particularly closely by @danbev? If so, maybe they have a standard way that they skip tests if the linked OpenSSL is outdated? Or maybe @build-infra needs to update that Jenkins host?

@maclover7
Copy link
Contributor

@shigeki @Trott To update the OpenSSL versions in CI, these lines need to changed to point to the new version. I can take a look in a little bit

@shigeki
Copy link
Contributor Author

shigeki commented Aug 15, 2018

@maclover7 Thanks. https://github.com/nodejs/build/blob/63c2e2691a8fbf55fbb02466e4e55f98d551c22f/ansible/roles/docker/templates/ubuntu1604_sharedlibs.Dockerfile.j2#L36-L42 also needs to be updated for #22320 (comment).

rvagg pushed a commit that referenced this pull request Aug 15, 2018
@shigeki @rvagg
deps: upgrade openssl sources to 1.1.0i
337f6b3 
This updates all sources in deps/openssl/openssl with openssl-1.1.0i.

PR-URL: #22318
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
rvagg pushed a commit that referenced this pull request Aug 15, 2018
@shigeki @rvagg
deps: update archs files for OpenSSL-1.1.0i
1ce3887 
`cd deps/openssl/config; make` updates all archs dependant files.

PR-URL: #22318
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
rvagg pushed a commit that referenced this pull request Aug 15, 2018
@shigeki @rvagg
test: fix error messages for OpenSSL-1.1.0i
1ed03b0 
After upgradeing OpenSSL-1.1.0i, two tests are failed due to changes
of error messages.

Ref: openssl/openssl@45ae18b
Ref: openssl/openssl@36d2517
PR-URL: #22318
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
@rvagg rvagg mentioned this pull request Aug 15, 2018
Merged
@danbev
Copy link
Contributor

danbev commented Aug 15, 2018

IIRC, shared-lib OpenSSL is observed particularly closely by @danbev? If so, maybe they have a standard way that they skip tests if the linked OpenSSL is outdated?

We switched from using a shared OpenSSL to the one shipped with node a few months ago since our main deployment environment is a container. So we are no longer building with a shared OpenSSL and not discovering issues like we used to.

@joyeecheung joyeecheung mentioned this pull request Aug 15, 2018
Closed
@Trott Trott mentioned this pull request Aug 15, 2018
Closed
@joyeecheung joyeecheung mentioned this pull request Aug 15, 2018
Closed
2 tasks
@refack refack mentioned this pull request Aug 15, 2018
Closed
rvagg pushed a commit that referenced this pull request Aug 16, 2018
@shigeki @rvagg
deps: upgrade openssl sources to 1.1.0i
6090e1f 
This updates all sources in deps/openssl/openssl with openssl-1.1.0i.

PR-URL: #22318
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
rvagg pushed a commit that referenced this pull request Aug 16, 2018
@shigeki @rvagg
deps: update archs files for OpenSSL-1.1.0i
eaa0ad9 
`cd deps/openssl/config; make` updates all archs dependant files.

PR-URL: #22318
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
rvagg pushed a commit that referenced this pull request Aug 16, 2018
@shigeki @rvagg
test: fix error messages for OpenSSL-1.1.0i
19246de 
After upgradeing OpenSSL-1.1.0i, two tests are failed due to changes
of error messages.

Ref: openssl/openssl@45ae18b
Ref: openssl/openssl@36d2517
PR-URL: #22318
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
@Trott
Copy link
Member

Trott commented Aug 16, 2018

This is in the 10.9.0 release that just went out, right? Should we fast-track landing it on master since we don't usually have stuff in releases that isn't in master as far as I know?

@Trott Trott added the fast-track PRs that do not need to wait for 48 hours to land. label Aug 16, 2018
@Trott
Copy link
Member

Trott commented Aug 16, 2018

👍 here to approve fast-tracking. This qualifies on the "unbreak CI" principle. The sharedlib host will fail CI until this lands, as I understand it. @refack

@refack
Copy link
Contributor

refack commented Aug 16, 2018

Also after this lands, everyone will also need to rebase their PRs.

@refack
Copy link
Contributor

refack commented Aug 16, 2018

Resume build: https://ci.nodejs.org/job/node-test-commit/20572/
(+ arm-fanned): https://ci.nodejs.org/job/node-test-commit-arm-fanned/2933/

@maclover7
Copy link
Contributor

Looks like this has already landed on master via 32902d0...19246de?

@Trott
Copy link
Member

Trott commented Aug 16, 2018

Ah, landed twenty minutes ago. This should be closed, yes?

This was referenced Aug 16, 2018
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
This was referenced Aug 17, 2018
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tniessen tniessen tniessen approved these changes

@Trott Trott Trott approved these changes

@rvagg rvagg rvagg approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
fast-track PRs that do not need to wait for 48 hours to land. 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.

OpenSSL Releases 14th August 2018
10 participants
@shigeki @nodejs-github-bot @Trott @maclover7 @danbev @refack @jasnell @rvagg @tniessen @joyeecheung