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

Node 4.2: Unable to run Node in non-FIPS mode if compiled with FIPS support #3819

Closed
lordjabez opened this issue Nov 13, 2015 · 10 comments

Comments

Projects
None yet
6 participants
@lordjabez
Copy link

commented Nov 13, 2015

As currently implemented, when Node is compiled with FIPS support (./configure fips), there is no way to disable engaging FIPS mode during execution. This means that several functions that rely on non-FIPS approved algorithms (e.g. md5 hashing) will fail, as will any code that depends on them (most obviously, npm).

What seems needed to me is a way to explicitly enable or disable FIPS operation each time node is invoked. The way this is done with the openssl CLI is via the OPENSSL_FIPS environment variable.

It is straightforward to add a similar capability to Node. A pull request with a suggested implementation is forthcoming.

@jasnell

This comment has been minimized.

Copy link
Member

commented Nov 13, 2015

/cc @mhdawson

@mhdawson

This comment has been minimized.

Copy link
Member

commented Nov 14, 2015

@lordjabez in what cases are you seeing npm fail with FIPS turned on ?

I agree an option to turn on /off would likely be useful @stefanmb

@stefanmb

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2015

@lordjabez @mhdawson

At first look MD5 dependencies in npm are not well justified. I'm trying to fix them.

@lordjabez

This comment has been minimized.

Copy link
Author

commented Nov 14, 2015

Certainly if npm can be made to run with FIPS mode active, that's a good thing. But in any case I agree with @mhdawson that some form of runtime switch is needed to make a FIPS validated Node practical.

I'm pushing my company pretty hard to move our entire tech stack to Node, and running FIPS validated is a key part of that.

@stefanmb

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2015

Another complication is that the test cases will become much more complex:

(1) Turn on FIPS.
(2) Test crypto, including test failures due to FIPS incompatible crypto.
(3) Turn off FIPS.
(4) Test crypto, include test success with FIPS incompatible crypto.
(5) Repeat a number of times.

@stefanmb

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2015

@lordjabez

I had better luck with npm and FIPS after the following patches:
npm/write-file-atomic#7
npm/unique-slug#1
npm/fs-write-stream-atomic#6

I'll see if I can get them accepted. There is still the issue regarding native modules #3815 which will have to be addressed.

@jelmd

This comment has been minimized.

Copy link

commented Nov 15, 2015

Not sure, whether this applies to 4.x as well, however, I had to fix v5.0.0 too because it never called FIPS_mode() (and if compiled with -DFIPS_NODE it calls fips_mode_set(1) again and again w/o need, which in turn causes annoying messages)! Anyway, see http://iws.cs.uni-magdeburg.de/~elkner/tmp/node5/ssl.patch and look for 'FIPS_mode()' (2 places) - the major problem.

BTW: I think the major issue here is, that it is assumed, that a FIPS capable lib is always used with FIPS_mode set to 1. This is wrong! Actually one doesn't need a non-FIPS capable OpenSSL lib at all, if the application provides a mechanism, to switch it on on demand (e.g. node --fips), because basically only than the non-FIPS compliant stuff gets switched off.

@mhdawson

This comment has been minimized.

Copy link
Member

commented Nov 16, 2015

@stefanb I could be wrong but I didn't think you could turn it on/off, more that you have to set it one way or the other before any crypto is used.

@stefanmb

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2016

I have made PR #5181 to resolve outstanding issues here.

@mhdawson mhdawson closed this in 7c48cb5 Feb 25, 2016

@mhdawson

This comment has been minimized.

Copy link
Member

commented Feb 25, 2016

Closing since change is now in master. Since the change is semver major it won't go into 4.X or 5.X.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.