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.

[shigeki]

@ChALkeR Oops. Yes, some of number in the first column are duplicated.
The uniq number of the packages is 2551 so that it is 0.675% in ratio if the number of total is 242,054. Is that right?

ohtsu@omb:Downloads$ awk -F : '{print $1;}' grep.2016-01-28.NODE_TLS_REJECT_UNAUTHORIZED.sorted.txt |sort |uniq -c |sort -nr |wc
     515    1545   27020
ohtsu@omb:Downloads$ awk -F : '{print $1;}' grep.2016-01-28.rejectUnauthorized.sorted.txt |sort |uniq -c |sort -nr |wc
    2098    6297  115957
ohtsu@omb:Downloads$ awk -F : '{print $1;}' grep.2016-01-28.* |sort |uniq -c |sort -nr |wc    2551    7656  139748

[ChALkeR]

@shigeki No, the number of packages is 1634 (note that there are false positives and false negatives).
2551 is the number of unique matched files in those packages.

The total number of packages at the moment of the dataset build was 227866.

[shigeki]

@ChALkeR Okay, thanks.
The ratio of affected packages is 1,634/242,054=0.675%
Here is a list of top 10 downloaded files. But its sum of number of downloads is less than 1% in total per month.

     1  11440560 request-2.69.0.tgz
     2  3442171 ws-1.0.1.tgz
     3  2419380 socket.io-client-1.4.5.tgz
     4  2086802 http-proxy-1.13.0.tgz
     5  1723493 engine.io-client-1.6.8.tgz
     6  1371175 mongodb-2.1.4.tgz
     7  1291810 aws-sdk-2.2.32.tgz
     8  1067936 xmlhttprequest-ssl-1.5.2.tgz
     9  1061476 node-sass-3.4.2.tgz
    10  924359  infinity-agent-2.0.3.tgz

It is smaller than that of I thought before.
Probably rejectUnauthorized might be used in internal. But we cannot estimate it.
With this number, I think we can change its name with soft deprecation.

@indutny @bnoordhuis Any thoughts?

[ChALkeR]

@shigeki Note that downloads count is not equal to usage, and that a lot of modules use those modules as dependencies and so on. Directly affecting packages that form about 1% of the total downloads is very significant.

Btw, it's 1.2%, 37532450/3080995738 (numbers taken from the same date when those stats were built).

[ChALkeR]

@shigeki

I think that:

1. NODE_TLS_REJECT_UNAUTHORIZED deserves a warning, and it's alternative name also deserves a warning — because there are many places where such an env var could be set unnoticed. This has nothing to do with deprecation. (tls: deprecate NODE_TLS_REJECT_AUTHORIZED #5318 misses it).

2. NODE_TLS_REJECT_UNAUTHORIZED should be both soft deprecated and hard deprecated, but this would need only a slightly rephrased warning from p.1 (tls: deprecate NODE_TLS_REJECT_AUTHORIZED #5318 has the warning text good, I assume). An underprecated alternative is required, perhaps.

   Or should we make it a runtime flag instead of an env var? If yes, then this will also solve p.1.

3. rejectUnauthorized should be soft deprecated only (just in the docs) for at least one major release, maybe two — will need to recheck the usage then. An underprecated alternative is required, of course.

[mikemaccana]

Since request is so popular I investigated it manually: confirmed, it allows untrusted certificates by default:

strictSSL - If true, requires SSL certificates be valid.

See https://www.npmjs.com/package/request#tlsssl-protocol

[jwalton]

@mikemaccana request only doesn't use strictSSL if it is explicitly set to false, so by default request will reject bad SSL certificates.

[allens]

Please can you just leave this variable as it is? What's the benefit of changing it?

I honestly have enough headaches getting tools such as Git and NPM to work behind corporate firewalls without people going round renaming variables unexpectedly.

I have to set NODE_TLS_REJECT_UNAUTHORIZED=0 before doing an NPM install of a project I'm working on. This is because openlayers is a dependency and the post install step uses request.js to pull down closure-util. NODE_TLS_REJECT_UNAUTHORIZED is currently the only way I know of making this work without a certificate error.

[mikemaccana]

@allens The benefit is knowing you're actually downloading closure-util rather than having some network device at your conference wrap it in a little bit of malware.

[jwalton]

@allens A better solution would be to figure out why the certificate for closure-util is invalid in the first place. Are you behind a corporate MITM device like Cisco Ironports that is serving you a bad cert? If so, you probably have already lost all security as many of these devices are misconfigured to accept bad certificates on your behalf, but your best option would be to add the corporate CA to your trusted certificate store, using the 'ca' option. Check out http://www.thedreaming.org/2016/09/27/nodejs-ssl/ for some other SSL troubleshooting tips.

[allens]

@mikemaccana I'm all for adding the warning if that's what you mean. I expect most people know exactly what the risks are anyway.

@jwalton I know what the better solution is, it's just not practical for me at the moment. I should have been more specific when I said "corporate firewall". A corporate device injecting a fake CA cert is exactly the problem. Thanks for the link but I don't know how to solve my issue with that information Isn't that specific to running my own node app? I'm one step removed - it's an NPM post-install script which is the problem.

If I could wave a magic wand and not have to do this I would. I know what I'm doing is a horrible hack but please don't remove the option. What's the point of making life difficult for other developers? As for renaming the variable that just seems like pointless meddling.

Thanks

[jwalton]

@allens Ahh, sorry, I misunderstood. But, I think you might be in luck. This PR was merged to node in October which I think will solve your issues: https://github.com/nodejs/node/pull/8334/files. In short, --use-openssl-ca and setting SSL_CERT_DIR or SSL_CERT_FILE should solve this for you in a secure fashion. I am currently not behind an evil firewall™ though, so I can't verify.

[Antony74]

@jwalton Can you clarify how to get npm to automatically pass the --use-openssl-ca parameter to Node when it runs (post-install) scripts, please.

Shame there isn't a way to get the certificates from the Operating System. These would already have been pre-doctored by the little-brother-in-the-middle SSL scanning technology, thus eliminating the need for additional configuration.

[jwalton]

I think --use-openssl-ca should, by default, fetch from the operating system on most systems (more than you ever wanted to know about this). Also, the docs state that --use-openssl-ca may be the default - it's configured at build time - so you may just have to upgrade to node v7.5.0 or higher and 🎉 .

But, if that's not working for you, npm is just a node.js script, so you can do something like this on MaxOS/Linux:

node --use-openssl-ca "$(which npm)" install promise-breaker

Or something roughly equivalent on Windows.

[Antony74]

Sorry no, it didn't find the certificate it was looking for, so it can't have been looking in the Windows Certificate Store (which is obviously set up fine or I wouldn't be able to access the https website where we're having this discussion).

I was using this https example

const https = require('https');

https.get('https://encrypted.google.com/', (res) => {
  console.log('statusCode:', res.statusCode);
  console.log('headers:', res.headers);

  res.on('data', (d) => {
    process.stdout.write(d);
  });

}).on('error', (e) => {
  console.error(e);
});

[sam-github]

--use-openssl-ca doesn't do anything useful on Windows, it certainly doesn't use the windows cert store, and Windows doesn't come with an OpenSSL style cert store (Linux does).

https://nodejs.org/api/cli.html#cli_node_extra_ca_certs_file is probably what you want

Can you clarify how to get npm to automatically pass the --use-openssl-ca parameter to Node when it runs (post-install) scripts, please.

You can't, that's the problem with CLI options, but when #12648 or #12677 land you will be able to use NODE_OPTIONS to pass CLI options to child processes via env var.

[Antony74]

Thanks. And that's why we'll be continuing to use NODE_TLS_REJECT_UNAUTHORIZED for the moment.

[sam-github]

@Antony74 Sorry, I don't understand your "that's why" comment. I proposed a more secure alternative, did you see https://nodejs.org/api/cli.html#cli_node_extra_ca_certs_file? Was its applicability not clear? Perhaps I misunderstand your problem, but it allows you to recursively add your proxy's cert to node's builtin CA certs, using environment variables, so your proxy is trusted, including for outbound http requests made by npm package install scripts, and it can be used instead of globally disabling all security.

@mikemaccana How will changing the env var name make people more secure? They'll just use the new env var. I think what we should do is document the env var. Right now, its existence is being passed around via stackoverflow and other informal channels. We didn't document it because its so insecure, but by not documenting it, we also no longer have a place in the documentation where we can explain why its such a bad idea, and to explain how there are better alternatives.

/cc @nodejs/security @nodejs/documentation

[Antony74]

@sam-github Oh I see, NODE_EXTRA_CA_CERTS is an env var not a CLI options so we don't have to wait for one of those two issues you mentioned before trying it.

[allens]

Thanks for the update @sam-github. I'm more that happy for NODE_TLS_REJECT_UNAUTHORIZED to be removed once there is a suitable alternative in place. I assume there are no plans to remove it in the current LTS? As well as being behind a corporate SSL filter I am using Windows so at the moment NODE_TLS_REJECT_UNAUTHORIZED is currently the only option for me.

This proposal is about renaming NODE_TLS_REJECT_UNAUTHORIZED though. If it is being removed eventually anyway then I think it would be better just to leave it be - just let the option die out with the current LTS.

[sam-github]

I assume there are no plans to remove it in the current LTS?

I have seen no proposal to do this.

As well as being behind a corporate SSL filter I am using Windows so at the moment NODE_TLS_REJECT_UNAUTHORIZED is currently the only option for me.

`NODE_EXTRA_CA_CERTS works on Windows, if there are use-cases it doesn't work for I would like to understand them.

For renaming NODE_TLS_REJECT_UNAUTHORIZED, I'm -1

[allens]

@sam-github I use the recommended LTS version of node.js. AFAIK NODE_EXTRA_CA_CERTS is only in the latest features release. So for the time being that limits me to NODE_TLS_REJECT_UNAUTHORIZED?

[sam-github]

@allens For the moment, yes, sorry. You can weigh in on #12677, the backport to 6.x

[Trott]

Should this remain open?

[joepie91]

Was a decision reached on this? I haven't seen anything conclusive so far.

[bnoordhuis]

It doesn't seem like there is broad agreement here. If someone wants to open a PR that prints a warning, go ahead, but I'll close this out.

Maybe this reflects my bleak view on human nature in general and programmers in particular but if --unsafe-perm has taught me anything, it's that no matter what you name it, there will always be people that blindly copy/paste SO answers without giving it second thought. You can't teach someone that doesn't want to learn.

[joepie91]

no matter what you name it, there will always be people that blindly copy/paste SO answers without giving it second thought.

While technically true, it doesn't address the purpose of this issue.

The goal isn't to reduce the amount of misuse to zero, as that is clearly not achievable. Instead, the goal is to reduce the misuse as much as possible, and that means clearly signalling to legitimately unaware users that they are doing something dangerous, so that they can take a step back and rectify the issue if they simply weren't aware of the consequences. If at that point they still choose to go ahead, there's not much that can be done about it.

In that context, the current interface is really not sufficient; there's no clear signal in either the name of the setting or its operation that the user is doing something dangerous, and it doesn't look any more dangerous to the average user than any other copypasted answer. Something needs to be changed about that, the question is just what.

Personally, I feel like ChALKeRs suggestions are reasonable.

[bnoordhuis]

@joepie91 Then you should follow up with a pull request. I closed the issue because there has been no movement in the six months since it was filed.

While technically true, it doesn't address the purpose of this issue.

It wasn't meant to. I'm less hopeful than you a name change or warning will effect any real change, though. Only removal will do that but that's going to be a prolonged effort with our deprecation policy.

[ChALkeR]

NODE_TLS_REJECT_UNAUTHORIZED=0 now prints warnings, PR (landed): #21900.