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.

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

Fix test regression for OpenSSL compiled with FIPS #23871

Closed
wants to merge 1 commit into from

Conversation

AdamMajer
Copy link
Contributor

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 25, 2018
@addaleax 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
@addaleax
Copy link
Member

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
Copy link
Contributor Author

Sorry, had to change commit message to follow guidelines

@refack 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
@refack
Copy link
Contributor

refack commented Oct 25, 2018

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
Copy link
Member

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
Copy link
Member

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

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sam-github
Copy link
Contributor

@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
Copy link
Contributor Author

AdamMajer commented Oct 25, 2018 via email

@@ -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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 targos removed the fast-track PRs that do not need to wait for 48 hours to land. label Oct 28, 2018
@targos
Copy link
Member

targos commented Oct 28, 2018

Landed in 0a23538

@targos targos closed this Oct 28, 2018
targos pushed a commit that referenced this pull request Oct 28, 2018
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>
targos pushed a commit that referenced this pull request Oct 28, 2018
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>
targos pushed a commit that referenced this pull request Nov 1, 2018
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>
MylesBorins pushed a commit that referenced this pull request Nov 26, 2018
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>
MylesBorins pushed a commit that referenced this pull request Nov 26, 2018
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>
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
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>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
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>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
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>
@codebytere codebytere mentioned this pull request Nov 29, 2018
@BethGriggs BethGriggs mentioned this pull request Dec 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. openssl Issues and PRs related to the OpenSSL dependency. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.