Propose NODE_TLS_REJECT_UNAUTHORIZED be renamed

[mikemaccana]

Happy to send a PR, but wanted to talk first:

I recently noticed there's a bunch of people trying to connect to untrusted sites using requests, superagent, etc. A bunch of answers to those questions are just

process.env.NODE_TLS_REJECT_UNAUTHORIZED = "0"

Which is scary. There's two things that could be improved here:

• Make it explicit that the option is insecure
• Having a Boolean for a 'reject' option creates a weird double negative: you have to think you're rejecting unauthorised is false, and what that means (the answer being: you're accepting untrusted certs)

I propose:

process.env.NODE_TLS_REJECT_UNAUTHORIZED = "0"

Be replaced with a more explicit, inverse option:

process.env.NODE_TLS_ACCEPT_UNTRUSTED_CERTIFICATES_THIS_IS_INSECURE = "1"

Or something similar. Any thoughts?

[rvagg]

/cc @nodejs/crypto

@mikemaccana thanks for opening this issue. Although I personally prefer the suggestion over at ladjs/superagent#205 for THIS_IS_TOTALLY_INSECURE_AND_WILL_EAT_BABIES, I think that removing the NODE_TLS_REJECT_UNAUTHORIZED option might be too big a leap. We could deprecate it and introduce a new name but maybe the easiest path would be to simply print a deprecation-style warning at startup explaining the potential insecurity implications of turning something like this on globally. I'll defer to @nodejs/crypto on this but it seems reasonable to me to warn when you're setting a global that impacts the entire runtime when it's likely that users are turning it on to solve a single problem in their application.

[mikemaccana]

[take 2 since I didn't parse this the first time around]

Thanks @rvagg. There's a couple of options:

• Print a warning on startup if NODE_TLS_REJECT_UNAUTHORIZED is set to 0
• Replace the option with something clearer, with the standard 'print a nice message saying this will be deprecated soon and show people the new option'.

Or some combination of both

[shigeki]

I agree that the word of rejectUnauthorized is not intuitive and I sometimes lost the name.

The name of process.env.NODE_TLS_REJECT_UNAUTHORIZED has one benefit that it is consistent with the name of option.rejectUnauthorized. Only renaming of NODE_TLS_REJECT_UNAUTHORIZED'looses its association and I think it is not a good idea.

Looking at github, it shows that https://github.com/search?l=javascript&q=rejectUnAuthorized&type=Code&utf8=%E2%9C%93 is much larger than https://github.com/search?l=javascript&q=NODE_TLS_REJECT_UNAUTHORIZED&type=Code&utf8=%E2%9C%93

We can only make soft deprecation for both of them and it would be a hard work. I wonder if it is worth while to go.

I have no objection to show a warning due to environment settings related to the security.

[ChALkeR]

@shigeki

option.rejectUnauthorized

Perhaps is also not the ideal name for that option, given that changing it allows insecure connections.

We can only make soft deprecation for both of them and it would be a hard work. I wonder if it is worth while to go.

I will prepare some npm package stats (though it would be iteresting who actually uses that outside of tests), but I think that soft (documentation-only) deprecation would be good.

I have no objection to show a warning due to environment settings related to the security.

+1 for the warning from me.

[shigeki]

I will prepare some npm package stats (though it would be iteresting who actually uses that outside of tests), but I think that soft (documentation-only) deprecation would be good.

That's good to know it and give us a great help to make decision.

[ChALkeR]

The preliminary results do not look good, even NODE_TLS_REJECT_UNAUTHORIZED has huge enough usage, and a signifcant part of that is outside tests.

A few examples:

• https://github.com/happner/happn/blob/master/lib/client/base.js#L131-L133
• https://github.com/germanrcuriel/jira-cmd/blob/master/lib/config.js#L4
• https://github.com/flesch/hipchat-notify.js/blob/master/index.js#L44
• https://github.com/switer/http-route-proxy/blob/master/index.js#L130
• https://github.com/jemershaw/node-mcafee/blob/master/src/index.js#L13

etc.

[mikemaccana]

Thanks @ChALkeR. Also agreed, options should be updated to: options.acceptUntrustedCertificatesThisIsInsecure or whatever else.

[dcposch]

Made a PR just to see what deprecating NODE_TLS_REJECT_UNAUTHORIZED would look like.

• Looks pretty doable
• Additionally deprecating options.rejectUnauthorized would be more invasive

[ChALkeR]

Sorry, forgot about the greps.
Here they are: rejectUnauthorized, NODE_TLS_REJECT_UNAUTHORIZED.

Note that the grep was done only in js/ts/coffeescript sources, and the env var (NODE_TLS_REJECT_UNAUTHORIZED) could be set somewhere else.

[shigeki]

@ChALkeR Thanks. I guess the first column is the id of package. How many is total? Is it corresponded the number of total package number of 242,054 shown in https://www.npmjs.com/ ?

[ChALkeR]

@shigeki The first column is downloads/month, as usual =).

That data is also a month old, but it should give the right impression.