ladjs / superagent

Ajax for Node.js and browsers (JS HTTP client). Maintained for @forwardemail, @ladjs, @spamscanner, @breejs, @cabinjs, and @lassjs.
https://ladjs.github.io/superagent/
MIT License
16.59k stars 1.33k forks source link

How to ignore ssl certificates #926

Closed rkostrab closed 5 years ago

rkostrab commented 8 years ago

Hello, I know it's insecure, but I need to ignore ssl certificates while requesting. How it is possible?

Thanks

kornelski commented 8 years ago

We don't have such option currently. There have been some proposals for exposing tls options #681, but nobody has implemented it well yet.

rkostrab commented 8 years ago

Can you give me any hint how to implement that? I may make PR.

kornelski commented 8 years ago

As discussed in #681, a .tls({}) function that takes an object and only allows options that are necessary (which AFAIK would be key, cert and something to disable security).

As for disabling security, see https://github.com/visionmedia/superagent/pull/832

denishowe commented 8 years ago

Temporary hack:

process.env['NODE_TLS_REJECT_UNAUTHORIZED'] = 0;

kornelski commented 8 years ago

No, please don't make all of node's requests insecure.

Please either use letsencrypt, or set your own ca using .ca() call.

sebs commented 8 years ago

awesome: This discussion goers for multiple tickets now. What about you let the tester decide how to test and not be to anal about everything. Thing is: You give me the choice: Test all without ssl or deactivate it for ALL. I want my ssl stuff tested

krisdahl commented 8 years ago

This is silly. We're forced to choosing between completely disabling certificate authentication across the entire node process, or not using SSL at all.

It should be allowed to be disabled on a per-request basis, just like you could do on any other browser, http utility or request library (Firefox, Chrome, curl, wget, IE, etc.)

Not everyone can create their own CA, or has control of the server (which may be totally internal even, and on a trusted network).

We all understand the risks of a MITM attack, but it isn't always that big of a deal. After-all, superagent will work with HTTP requests as well as HTTPS, right? By the logic of rejecting all self-signed certificates, should't superagent also just reject any non HTTPS requests as well?

cyjake commented 7 years ago

Regarding the .tls() or .ca() method, where is the documentation? I can't find it at http://visionmedia.github.io/superagent/

kornelski commented 7 years ago

We pass these to the agent, so details are documented here: https://nodejs.org/api/https.html#https_https_request_options_callback

it'd be great if someone made a PR adding reference to these to superagent's docs.

andineck commented 7 years ago

I don't get it. node's own https.request has got the option to allow rejectUnauthorized. curl has --insecure and other tools have their option to disable the ssl check per request.

I really don't think that the world becomes a safer place with rejecting the need to reject unauthorized ssl certificates.

kornelski commented 7 years ago

I strongly believe that Node's own rejectUnauthorized is a mistake, which should be contained, and not propagated further.

I've written about it before, but mainly:

If you want to make connections without any real security or privacy, then own it, and use HTTP.

denishowe commented 7 years ago

Pornel, please (re)read Kris's comment.

kornelski commented 7 years ago

I see it as "We need to remove a spider. We could have just set doNotBurnDownTheBedroom = false, but we're forced to set DONT_BURN_DOWN_THE_HOUSE=0 setting instead!"

I'm not opposed to solving the problem. I'm opposed to solving it in the misguided way that makes it look easy, and has more serious consequences than it seems.

If you'd like to solve the problem, I'm happy to accept solutions that are less likely to be a footgun. For example:

For the current superagent version, if you're connecting to some API, I'd suggest making base URL of that API configurable:

const base = prod ? 'https://api.example.com' : 'http://localhost/api';
…
request.post(`${base}/do/something`);

so that in dev you can use HTTP, and HTTPS in prod. This is no more secure than rejectUnauthorized = false, but it does not give false sense of security.

It may be worthwhile to improve .agent() to make setting base URL easier.

kornelski commented 7 years ago

After-all, superagent will work with HTTP requests

I'm happy to add an option to superagent that completely blocks HTTP. I know it may seem radical: we've had HTTP forever, and HTTPS has been such a pain. However, things have changed rapidly and HTTPS-only world is already a reality for new client-side web applications and many macOS/iOS apps.

In web apps you need to be HTTPS-only to be able to use modern browser features, and that blocks all HTTP requests. Apple has deprecated HTTP. For real, system-wide. It's blocked it by default for all new macOS and iOS apps (you can add exceptions for now).

So allowing HTTP by default in superagent is pretty lax in that context.

julien-f commented 7 years ago

@pornel I understand your point very well, but unfortunately I do not control my clients environment in which my code is executed.

So my choices currently are:

What I want is to allow my clients to explicitly disable certificate validation if they need to and understand the risks.

kornelski commented 7 years ago

@julien-f I hope you'd join me in pushing for proper HTTPS everywhere, and nudge your clients to fix their HTTPS instead of disabling it.

julien-f commented 7 years ago

@pornel indeed I want to promote proper use of HTTPS, but I have to support the self-signed certificates of my clients (adding the CA for all servers is not an option either) 😕

stefanotorresi commented 7 years ago

@pornel are you open to reconsider this? Every http client that I know of has a way to disable SSL verification on a per-request basis, which is a completely valid use case for self-signed certificates, and the existence of LetsEncrypt doesn't mean that these should no longer exist.

I wanted to use SuperAgent in a CI setup for e2e tests, and using actual SSL certificates doesn't even make sense in such an environment.

IMHO you're not making anything more secure with your decision, because we're not talking about defaults here. You're enforcing an arbitrary limitation that makes this library unusable in development and testing environments.

kornelski commented 7 years ago

@stefanotorresi For testing in controlled environment that may be useful indeed, but I'm concerned about a for-testing-only option being accidentally left enabled in production. How would you design that option to prevent accidental misuse?

So far that's still what I'd recommend: https://github.com/visionmedia/superagent/issues/926#issuecomment-296444305

stefanotorresi commented 7 years ago

Using the host field or unspecific environment variables (like a generic and arbitrary development mode) to change the behaviour of the client is far from what I'd expect as a consumer, and I'd strongly advise against it anyway because this line of thought involves dangerous assumptions and is a slippery slope.

If I were to choose, I'd apply the so called principle of least astonishment and just follow the common practice among HTTP clients in the wild.

Node options have already been pointed out, so here are a few examples from other ecosystems:

I could continue and list every major programming language and every major HTTP tool ever made, to be honest; my point is that this is not an isolated opinion of few, it's the industry de-facto standard.

To be completely frank, I find a bit naive to think that providing an opt-in insecure mode is a bad practice.
It's not; providing insecure defaults is.

IMHO any other approach would result to be unexpected at best, and totally wonky at worst.

Just my 2 cents, though.

kornelski commented 7 years ago

I appreciate you're bringing data to this discussion. Options to disable security are common indeed, but misuse of these options is also common. There are numerous CVEs for this vulnerable major bank apps, vulnerable mail clients, etc, etc, etc. In non-browser software misuse of these options is a widespread problem.

So I think the status quo needs to be improved. I'm OK to have such option in superagent, but only if it's clearly labelled as dangerous and hard to accidentally leave on in production. Could we focus the discussion on achieving these goals?

Most of APIs you quote IMHO fall short (though having insecure in the name is a good dierction). For example CURLOPT_SSL_VERIFY* is famous for being a footgun. In PHP-related QAs it's easy to find comments such as "if you get an error about invalid hostname, just set CURLOPT_SSL_VERIFYHOST to 0".

Unfortunately, completely broken and useless SSL appears to be working just as well as secure one. It's not a bug you can notice, so I think it really needs extra help to prevent such mistakes.

People tend to go for the quickest "fix", so I'd like to make secure connections easier than insecure ones.

How about pinning specific certificate(s)?

Something vaguely similar to SSH's host key verification. You could specifically list certain certificates as trusted. That would allow self-signed and "3rd party failed to maintain TLS and need two weeks to fix it" cases, but will not trust just any random certs, so it still protects against MITM.

Are there use-cases it won't solve?

What would syntax for this be? Can somebody help with the implementation?

stefanotorresi commented 7 years ago

By all means, using a custom Certificate Authority key pair can be a way to verify only a subset of certificates, even self signed ones, and as you can see from the examples above, this is something that goes very well along with the ability to turn verification off entirely.

But hey, I still don't agree with your "it's a footgun" argument. Everything can be a footgun, even forgetting debugging enabled can become problem, but that's not a reason to make it a PITA to enable. In fact, I'd argue that a well designed API should rather not expose a function, rather than make it very inconvenient just to protect people from their own mistakes. Safe defaults, that's all I'd expect, and I also expect people to know what their doing when they turn safeties off.

Then again, it's your library, so I'll leave you at that. ;)

pimterry commented 6 years ago

How about pinning specific certificate(s)?

This is notably similar to Chrome's approach: https://codereview.webrtc.org/2753123002/.

They recently added an ignore-certificate-errors-spki-list flag, which lets you specify specific certificates that should not be validated, as part of moving towards removing their current ignore-certificate-errors option entirely

fedika commented 5 years ago

So...

Is there any options to skip/ignore/ ssl certificates?

niftylettuce commented 5 years ago

PR welcome, see https://github.com/visionmedia/superagent/issues/926#issuecomment-337416938

chrisvoo commented 5 years ago

I agree with the previous comment, voluntary disabling the opportunity to ignore certs errors is not a good choice. Just an example: I need to test a payment form of an insurance company in their staging area. They opted to use a self-signed certificate or maybe the certificate used in production but for another domain and port. It's not important, it's a test of something I don't have the power to do anything.

glintik commented 5 years ago

Repo authors, what a problem to commit this PR to the master? Many users want it. Security is a private deal, if user wants to make insecure requests, let's give him to decide.

pimterry commented 5 years ago

@glintik what do you think about the solution proposed in https://github.com/visionmedia/superagent/issues/926#issuecomment-337416938 of certificate pinning, i.e. adding an API that allows extra certificates to be trusted? Would that work for you?

glintik commented 5 years ago

@glintik what do you think about the solution proposed in #926 (comment) of certificate pinning, i.e. adding an API that allows extra certificates to be trusted? Would that work for you?

Let's do developers to decide, what to choose - cert pinning or just disable chain checking at all. This possibility exists in many languages and frameworks, I know, just because we can not forecast all possible use cases. In dev environment I prefer to disable cert checking at all, no any risk here.

niftylettuce commented 5 years ago

PR welcome, I am locking this thread.

niftylettuce commented 4 years ago

v5.1.1 released that resolves this issue