crypto: fail early when loading crypto module w/o openssl by jasnell · Pull Request #5611 · nodejs/node

jasnell · 2016-03-08T23:46:59Z

Pull Request check-list

Please make sure to review and check all of these items:

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to [CONTRIBUTING.md][0]?
If this change fixes a bug (or a performance problem), is a regression
test (or a benchmark) included?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)?

Affected core subsystem(s)

crypto, tls, https, process, src

Description of change

Updated
Fail early in require('crypto'), require('tls'), require('https'), etc when crypto is not available
(rather than depending on an internal try/catch).

Add documentation for detecting when crypto is not available.

(this previously had a process.noCrypto flag that was removed)

evanlucas · 2016-03-08T23:48:23Z

what about something like process.hasCrypto? Using process.noCrypto seems a little odd to me

vkurchatkin · 2016-03-08T23:49:59Z

the only way to determine if crypto is enabled is to either attempt to require the 'crypto' module

It seems to be perfectly fine

jasnell · 2016-03-08T23:50:32Z

The typical check is to see if crypto is disabled. Having it as process.hasCrypto would require developers to negate in the typical case... e.g. if (!process.hasCrypto)

jasnell · 2016-03-08T23:57:02Z

@vkurchatkin ... sure, it works, but it also requires two try/catch blocks (one outside crypto.js, one inside) and a call to process.binding to determine if crypto to present... which is silly because node already knows that crypto isn't available when it is compiled. This alternative is more efficient, consolidates the checking, and makes it easier on developers.

vkurchatkin · 2016-03-09T00:09:30Z

@jasnell I just don't like adding more and more properties to process. We can change internal implementation to throw immediately on require, but I think we shouldn't add a flag.

In most cases if script requires crypto and node is compiled without OpenSSL, failing is the only option. In other cases (which should be rare, in my opinion) try catch is good enough.

jasnell · 2016-03-09T00:28:06Z

I just don't like adding more and more properties to process

Out of curiosity... how come? Would it be more palatable if the flag was _ prefixed and left undocumented?

Other than adding the flag, this PR does modify the internal implementation to throw immediately on require eliminating the internal try/catch.

jasnell · 2016-03-09T00:29:55Z

Btw, see https://github.com/npm/npm/blob/master/lib/adduser.js#L7-L20 for an example of the kind of "is crypto enabled" check this is intended to address.

vkurchatkin · 2016-03-09T00:33:51Z

how come? Would it be more palatable if the flag was _ prefixed and left undocumented

no, it's much worse. The problem is that we use process for all kinds of unrelated things, both public and private. The only reason for this is that there is no other simple way to expose something to js without thinking too much. Because of this process is really littered.

Other than adding the flag, this PR does modify the internal implementation to throw immediately on require eliminating the internal try/catch.

This seems reasonable to me

rvagg · 2016-03-09T00:39:59Z

wouldn't !!process.versions.openssl do this same thing?

jasnell · 2016-03-09T00:41:57Z

@rvagg +1 ... was just going to suggest the same thing. The only reason I had added the process.noCrypto was to make the check slightly easier.

jasnell · 2016-03-09T00:44:17Z

@vkurchatkin @rvagg: ok, I'll pull the process.noCrypto flag and will keep everything else.

jasnell · 2016-03-09T00:55:57Z

@vkurchatkin @rvagg ... ok, new commit added. PTAL

jasnell · 2016-03-09T00:59:52Z

Thinking about it further, I think this may actually be semver-major because it changes the error reported when doing require('https') and others when openssl is not available.

rvagg · 2016-03-09T02:25:15Z

empty line above code blocks is standard isn't it?

rvagg · 2016-03-09T02:28:51Z

lgtm sans two minor formatting nits in doc, I'm not sold on semver-minor mainly since no-crypto mode isn't really something we "officially" support, although I don't imagine there's any great hurry to get this in so I'll leave that call to you.

jasnell · 2016-03-09T02:54:29Z

Nope, no hurry at all. I'll let this one sit for a bit. I suspect you're right tho... it's entirely possible this could land as a patch without any real impact.

bnoordhuis · 2016-03-09T09:53:38Z

I'm -1. The try/catch approach is still the most future-proof. What if we switch to another crypto library some day?

jasnell · 2016-03-10T17:14:39Z

@bnoordhuis ... not sure how switching to another crypto library would impact this. Wouldn't we still know that it's not available without having to do the internal try/catch?

jasnell · 2016-03-10T17:15:57Z

... and even if we did switch to a different library, the assertCrypto utility method could simply be modified to check for any crypto as opposed to specifically for process.versions.openssl

jasnell · 2016-03-19T16:52:35Z

@bnoordhuis .. updated! squashed. PTAL!

bnoordhuis · 2016-03-19T18:23:23Z

Is there still a reason to set this property?

jasnell · 2016-03-21T19:17:35Z

@bnoordhuis ... PTAL

bnoordhuis · 2016-03-23T11:21:11Z

LGTM

jasnell · 2016-03-23T16:00:13Z

One final CI before landing (although we don't actually run CI with openssl off): https://ci.nodejs.org/job/node-test-pull-request/2044/

jasnell · 2016-03-23T17:02:49Z

CI had a couple of red tests that I believe are unrelated. Just to be safe, running again: https://ci.nodejs.org/job/node-test-pull-request/2045/

Fail early in require('crypto'), require('tls'), require('https'), etc when crypto is not available (rather than depending on an internal try/catch). Add documentation for detecting when crypto is not available.

jasnell · 2016-03-23T17:33:45Z

CI is green. One unrelated failure.

Fail early in require('crypto'), require('tls'), require('https'), etc when crypto is not available (rather than depending on an internal try/catch). Add documentation for detecting when crypto is not available. PR-URL: #5611 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

jasnell · 2016-03-23T17:35:44Z

Landed in f429fe1

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * OS X * MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7 [#6402](#6402). * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).

jasnell added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-minor PRs that contain new features and should be released in the next minor version. labels Mar 9, 2016

jasnell changed the title ~~src: add process.noCrypto flag~~crypto: fail early when loading crypto module w/o opensslMar 9, 2016

rvagg reviewed Mar 9, 2016
View reviewed changes

Comment threaddoc/api/crypto.markdown Outdated
rvaggMar 9, 2016
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
empty line above code blocks is standard isn't it?

jasnell removed the process Issues and PRs related to the process subsystem. label Mar 9, 2016

bnoordhuis reviewed Mar 19, 2016
View reviewed changes

Comment threadlib/internal/util.js Outdated
bnoordhuisMar 19, 2016
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there still a reason to set this property?

jasnell force-pushed the no-openssl-flag branch from 57eb355 to b408b10Compare March 21, 2016 19:15

crypto: fail early when loading crypto without openssl
d1fbbdf
Fail early in require('crypto'), require('tls'), require('https'), etc when crypto is not available (rather than depending on an internal try/catch). Add documentation for detecting when crypto is not available.

jasnell force-pushed the no-openssl-flag branch from b408b10 to d1fbbdfCompare March 23, 2016 17:33

jasnell closed this Mar 23, 2016

jasnell mentioned this pull request Apr 19, 2016
What is new in v6? #6264
Closed

jasnell mentioned this pull request Apr 26, 2016
2016-04-26, Version 6.0.0 (Current) Release #6383
Closed

mihailik mentioned this pull request Oct 11, 2017
Handle absent 'crypto' as per nodejs docs microsoft/TypeScript#19100
Closed

muxator mentioned this pull request Mar 31, 2020
SQL injection attempts killls Etherpad lite ether/etherpad#3502
Closed

Uh oh!

Uh oh!

Conversation

jasnell commented Mar 8, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

Uh oh!

evanlucas commented Mar 8, 2016

Uh oh!

vkurchatkin commented Mar 8, 2016

Uh oh!

jasnell commented Mar 8, 2016

Uh oh!

jasnell commented Mar 8, 2016

Uh oh!

vkurchatkin commented Mar 9, 2016

Uh oh!

jasnell commented Mar 9, 2016

Uh oh!

jasnell commented Mar 9, 2016

Uh oh!

vkurchatkin commented Mar 9, 2016

Uh oh!

rvagg commented Mar 9, 2016

Uh oh!

jasnell commented Mar 9, 2016

Uh oh!

jasnell commented Mar 9, 2016

Uh oh!

jasnell commented Mar 9, 2016

Uh oh!

jasnell commented Mar 9, 2016

Uh oh!

rvaggMar 9, 2016

Choose a reason for hiding this comment

Uh oh!

rvagg commented Mar 9, 2016

Uh oh!

jasnell commented Mar 9, 2016

Uh oh!

bnoordhuis commented Mar 9, 2016

Uh oh!

jasnell commented Mar 10, 2016

Uh oh!

jasnell commented Mar 10, 2016

Uh oh!

jasnell commented Mar 19, 2016

Uh oh!

bnoordhuisMar 19, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented Mar 21, 2016

Uh oh!

bnoordhuis commented Mar 23, 2016

Uh oh!

jasnell commented Mar 23, 2016

Uh oh!

jasnell commented Mar 23, 2016

Uh oh!

jasnell commented Mar 23, 2016

Uh oh!

jasnell commented Mar 23, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants