Feature request: support ca and rejectUnauthorized for HTTPS proxy server

[mbargiel]

The ca and rejectUnauthorized properties from the request options are correctly passed to the upstream HTTPS endpoints but it's currently not possible to set those for the proxy server itself when sending the HTTP CONNECT request. This means it's impossible to use this implementation with an HTTPS proxy server unless that server uses a certificate from a well-known certificate authority.

I can think of two simple options to support these options:

1. Pass the ca and rejectUnauthorized properties from the createConnection options parameter to the https.request function, through the requestOptions.
2. Support additional, non-standard properties to allow separately configuring the proxy server connection and the upstream connection. Ideas: 2.1. options.proxyCa and options.proxyRejectUnauthorized 2.2. options.proxy.ca and options.proxy.rejectUnauthorized 2.3. options.proxyAgentOptions.ca and options.proxyAgentOptions.rejectUnauthorized
3. Support passing an agent for the proxy connection: options.proxyAgent

Personally, I dislike option 1 because it's inflexible. Any other option seems good to me.

I can provide a PR for this, based on which one is your preference.

[ckcr4lyf]

Btw, for the time being you could also use NODE_EXTRA_CA_CERTS option and pass your proxy cert to it, which will make the underlying implementation trust it.

But I like your idea number 2.

[delvedor]

Hello! You are right, there is no option to do what you need. I agree tho that you can easily solve this via the NODE_EXTRA_CA_CERTS env variable, which should be the best way to address this issue. Does this work for you?

[mbargiel]

Hi @delvedor @ckcr4lyf ! Thanks for the suggestion. I already knew about this option but it's unfortunately not something that should ever be used in production because it makes your code vulnerable to Man-in-the-Middle attacks for every request. For instance, if I want to connect to a proxy without validating its certificate (or rather, with a custom CA vertificate), I most certainly do not want to disable certificate validation for the upstream call itself and all unproxied calls. 😨

Since you think there's value in this, I'll look at opening a PR shortly. We can discuss about the preferred making in the PR 😄

[ckcr4lyf]

It will not disable validation (that option it TLS_REJECT_UNAUTHORIZED), rather it will trust a custom certificate. As long as you make sure the cert is only for that specific domain, I think it's an appropriate use case.

[mbargiel]

It will not disable validation (that option it TLS_REJECT_UNAUTHORIZED), rather it will trust a custom certificate. As long as you make sure the cert is only for that specific domain, I think it's an appropriate use case.

Oh geez yes, I read too fast :sweat_smile: My bad!

You're right, NODE_EXTRA_CA_CERTS is an option under some circumstances but not mine. It assumes we know the extra certs upfront because it's only heeded by Node at startup. If you set it later, from within the process, it does nothing.

[ckcr4lyf]

That is true, yeah. I can certainly imagine a use case wherein you provision a proxy with a self signed cert generated after the node process to be using it starts, so a way to pass in a proxy cert at runtime would be beneficial.

[delvedor]

Oks, let's add an option to address this. Given that this situation could happen again, but with different options, I would create a top-level option named proxyRequestOptions, where you can configure everything you need. In this way, we don't risk adding too many new options. The main issue I see with a single proxyRequestOptions is that you could override the internal options defaults, which might be hard to debug. We would need a small validation layer that ensures that users cannot override the internal configuration to solve this.

Would you like to send a pull request?

[mbargiel]

Yes, I'll do it.

I'm not too sure what you mean regarding the internal options defaults and the validation layer - could you please clarify what you have in mind?

[delvedor]

https://github.com/delvedor/hpagent/blob/cc90c2b60a0b946574993d5598d93229ce452f72/index.js#L17-L26

[mbargiel]

Gotcha!

One more thing: as I was getting a PR prepared, I noticed the tests had a lot of duplication, and in some cases, there were tests missing in some combinations of http-http, http-https, etc. (ie. present in some but not all combinations). I have a branch where I played with reducing the duplication by parameterizing tests and I got that working pretty simply without losing any clarity. Would you be interested in seeing this in a PR? (Sneak peak: https://github.com/mbargiel/hpagent/compare/main..feature/parameterize-tests)

[mbargiel]

Another question: I'm not certain how to validate my implementation. I would normally use a mocking library like sinon to detect that e.g. http.request was called with the right custom options and without overriding the internal options defaults but you're not using any in this project. I'm not sure if there's an other client-side event I could rely on, before or while issuing the remote connection, to detect whether custom proxy request options were correctly passed. Do have any suggestion?

For now, what I could do was with request and connect events on the server and proxy sides to check the headers that had been passed, but I'd like to validate actual connection-related options. (Sneak peak: https://github.com/mbargiel/hpagent/compare/main..feature/proxy-agent-options - which, by the way, makes https://github.com/delvedor/hpagent/pull/8 obsolete as it supports passing custom headers to the CONNECT proxy in a much cleaner way)

[arossert]

Any estimation of when this is going to be implemented?

[mbargiel]

@arossert I don't know, I opened a PR for this 3 months ago but it seems that @delvedor didn't see it, didn't have time or might have forgotten about it 😅 I'm also interested in getting this landed before we can use it in production code.

[delvedor]

Closed in #72.