Fix test regression for OpenSSL compiled with FIPS by AdamMajer · Pull Request #23871 · nodejs/node · GitHub

In commit bff53c5, a check was added for very specific OpenSSL
format. Unfortunately, when OpenSSL is compiled in FIPS mode, this
check fails. Added additional regex to satisfy OpenSSL version
strings in both regular and FIPS modes.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

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

Collaborators, feel free to 👍 this comment to approve fast-tracking.

In commit bff53c5, a check was added for very specific OpenSSL format. Unfortunately, when OpenSSL is compiled in FIPS mode, this check fails. Added additional regex to satisfy OpenSSL version strings in both regular and FIPS modes.

Sorry, had to change commit message to follow guidelines

Two failed platform CI: https://ci.nodejs.org/job/node-test-commit-linux/22673/ + https://ci.nodejs.org/job/node-test-commit-freebsd/21471/

Ok to fast track with two approvals (@refack and @addaleax)

LGTM but I thought there isn't a FIPS module compatible with OpenSSL 1.1.0/1.1.1 yet and (AFAIK) Node.js doesn't compile against OpenSSL 1.0.2 since the update to 1.1.0 (#22025).

We still build/test FIPs on earlier versions so its good to have this in case PR mentioned is backported. We also will want to compile in FIPs in the future

LGTM

@AdamMajer thanks for this.

I don't mind fast-tracking this, its small and correct, but since master doesn't build against FIPS, its not breaking anything ATM, is it?

On 2018-10-25 7:39 p.m., Sam Roberts wrote: @AdamMajer <https://github.com/AdamMajer> thanks for this. I don't mind fast-tracking this, its small and correct, but since master doesn't build against FIPS, its not breaking anything ATM, is it?

It's breaking my build that uses SUSE's OpenSSL library. fast-track is not important for me, as long as the change makes it to Node's tree so I don't have to keep too many external patches around.

Suggested change

      
              assert(/^\d+\.\d+\.\d+[a-z]?(-fips)?$/.test(process.versions.openssl));
          
              assert(/^\d+\.\d+\.\d+[a-z]?(?:-fips)?$/.test(process.versions.openssl));

Yes, could change that but I think it makes it more difficult to read and we are not really saving anything here. So I think for readability sake, I would keep this as-is.

Landed in 0a23538

In commit bff53c5, a check was added for very specific OpenSSL format. Unfortunately, when OpenSSL is compiled in FIPS mode, this check fails. Added additional regex to satisfy OpenSSL version strings in both regular and FIPS modes. PR-URL: #23871 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 25, 2018

addaleax approved these changes Oct 25, 2018

View reviewed changes

addaleax added openssl Issues and PRs related to the OpenSSL dependency. fast-track PRs that do not need to wait for 48 hours to land. labels Oct 25, 2018

test: fix regression when compiled with FIPS

7b8b723

In commit bff53c5, a check was added for very specific OpenSSL format. Unfortunately, when OpenSSL is compiled in FIPS mode, this check fails. Added additional regex to satisfy OpenSSL version strings in both regular and FIPS modes.

AdamMajer force-pushed the fix_fips branch from a1e653a to 7b8b723 Compare October 25, 2018 11:46

cjihrig approved these changes Oct 25, 2018

View reviewed changes

jasnell approved these changes Oct 25, 2018

View reviewed changes

tniessen approved these changes Oct 25, 2018

View reviewed changes

refack approved these changes Oct 25, 2018

View reviewed changes

refack added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Oct 25, 2018

richardlau approved these changes Oct 25, 2018

View reviewed changes

mhdawson approved these changes Oct 25, 2018

View reviewed changes

sam-github approved these changes Oct 25, 2018

View reviewed changes

lpinca approved these changes Oct 25, 2018

View reviewed changes

targos removed the fast-track PRs that do not need to wait for 48 hours to land. label Oct 28, 2018

targos closed this Oct 28, 2018

targos mentioned this pull request Oct 28, 2018

Release proposal: v11.1.0 #23922

Merged

beevelop mentioned this pull request Nov 2, 2018

2018-11-02 Version 11.1.0 (Current) @targos beevelop/docker-android-nodejs#245

Closed

This was referenced Nov 2, 2018

2018-11-02 Version 11.1.0 (Current) @targos beevelop/docker-android-nodejs#246

Closed

2018-11-02 Version 11.1.0 (Current) @targos beevelop/docker-android-nodejs#247

Closed

2018-11-02 Version 11.1.0 (Current) @targos beevelop/docker-android-nodejs#248

Closed

MylesBorins added land-on-v8.x labels Nov 26, 2018

codebytere mentioned this pull request Nov 27, 2018

v10.13.1 proposal #24675

Closed

codebytere mentioned this pull request Nov 29, 2018

v10.14.2 proposal #24727

Merged

BethGriggs mentioned this pull request Dec 4, 2018

v8.14.1 proposal #24832

Merged

Fix test regression for OpenSSL compiled with FIPS by AdamMajer · Pull Request #23871 · nodejs/node · GitHub

Uh oh!

Uh oh!

Fix test regression for OpenSSL compiled with FIPS - #23871

Fix test regression for OpenSSL compiled with FIPS#23871
AdamMajer wants to merge 1 commit into
nodejs:masterfrom
AdamMajer:fix_fips

AdamMajer commented Oct 25, 2018

Uh oh!

addaleax commented Oct 25, 2018

Uh oh!

AdamMajer commented Oct 25, 2018

Uh oh!

refack commented Oct 25, 2018 •
edited
Loading

Uh oh!

Uh oh!

richardlau commented Oct 25, 2018

Uh oh!

mhdawson commented Oct 25, 2018

Uh oh!

mhdawson left a comment

Uh oh!

sam-github commented Oct 25, 2018

Uh oh!

AdamMajer commented Oct 25, 2018 via email

Uh oh!

lpinca Oct 25, 2018

Uh oh!

AdamMajer Oct 26, 2018

Uh oh!

targos commented Oct 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

               if (common.hasCrypto) {
-                assert(/^\d+\.\d+\.\d+[a-z]?$/.test(process.versions.openssl));
+                assert(/^\d+\.\d+\.\d+[a-z]?(-fips)?$/.test(process.versions.openssl));

	assert(/^\d+\.\d+\.\d+[a-z]?(-fips)?$/.test(process.versions.openssl));
	assert(/^\d+\.\d+\.\d+[a-z]?(?:-fips)?$/.test(process.versions.openssl));

Fix test regression for OpenSSL compiled with FIPS by AdamMajer · Pull Request #23871 · nodejs/node · GitHub

Uh oh!

Uh oh!

Fix test regression for OpenSSL compiled with FIPS - #23871

Fix test regression for OpenSSL compiled with FIPS#23871AdamMajer wants to merge 1 commit intonodejs:masternodejs/node:masterfrom AdamMajer:fix_fipsAdamMajer/node:fix_fipsCopy head branch name to clipboard

Conversation

AdamMajer commented Oct 25, 2018

Checklist

Uh oh!

addaleax commented Oct 25, 2018

Uh oh!

AdamMajer commented Oct 25, 2018

Uh oh!

refack commented Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richardlau commented Oct 25, 2018

Uh oh!

mhdawson commented Oct 25, 2018

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

sam-github commented Oct 25, 2018

Uh oh!

AdamMajer commented Oct 25, 2018 via email

Uh oh!

lpinca Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

AdamMajer Oct 26, 2018

Choose a reason for hiding this comment

Uh oh!

targos commented Oct 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

Fix test regression for OpenSSL compiled with FIPS#23871
AdamMajer wants to merge 1 commit into
nodejs:masterfrom
AdamMajer:fix_fips

refack commented Oct 25, 2018 •
edited
Loading