Fix test regression for OpenSSL compiled with FIPS #23871

AdamMajer · 2018-10-25T11:27:27Z

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

addaleax · 2018-10-25T11:34:22Z

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.

AdamMajer · 2018-10-25T11:47:12Z

Sorry, had to change commit message to follow guidelines

refack · 2018-10-25T16:10:54Z

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)

richardlau · 2018-10-25T16:26:10Z

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).

mhdawson · 2018-10-25T16:28:45Z

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

mhdawson

LGTM

sam-github · 2018-10-25T17:38:26Z

@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?

AdamMajer · 2018-10-25T18:34:42Z

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.

lpinca · 2018-10-25T19:15:15Z

test/parallel/test-process-versions.js

@@ -34,7 +34,7 @@ assert(/^\d+\.\d+\.\d+(?:\.\d+)?-node\.\d+(?: \(candidate\))?$/
 assert(/^\d+$/.test(process.versions.modules));

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


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.

targos · 2018-10-28T14:12:50Z

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 pending requests for changes, 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix test regression for OpenSSL compiled with FIPS #23871

Fix test regression for OpenSSL compiled with FIPS #23871

AdamMajer commented Oct 25, 2018

addaleax commented Oct 25, 2018

AdamMajer commented Oct 25, 2018

refack commented Oct 25, 2018 •

edited

Loading

richardlau commented Oct 25, 2018

mhdawson commented Oct 25, 2018

mhdawson left a comment

sam-github commented Oct 25, 2018

AdamMajer commented Oct 25, 2018 via email

lpinca Oct 25, 2018

AdamMajer Oct 26, 2018

targos commented Oct 28, 2018

	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 #23871

Fix test regression for OpenSSL compiled with FIPS #23871

Conversation

AdamMajer commented Oct 25, 2018

Checklist

addaleax commented Oct 25, 2018

AdamMajer commented Oct 25, 2018

refack commented Oct 25, 2018 • edited Loading

richardlau commented Oct 25, 2018

mhdawson commented Oct 25, 2018

mhdawson left a comment

Choose a reason for hiding this comment

sam-github commented Oct 25, 2018

AdamMajer commented Oct 25, 2018 via email

lpinca Oct 25, 2018

Choose a reason for hiding this comment

AdamMajer Oct 26, 2018

Choose a reason for hiding this comment

targos commented Oct 28, 2018

refack commented Oct 25, 2018 •

edited

Loading