Unable to use proxy authentication

[feuxfollets1013]

Current Behavior

I set auth-proxy address to npm proxy as following and I got invalid proxy error when I built some library.

• npm proxy setting

https-proxy=http://id:pass@example.com:18080/
proxy=http://id:pass@example.com:18080/

• error

node-pre-gyp WARN download ignoring invalid "proxy" config setting: "http://id:pass@example.com:18080/"

Expected Behavior

The build process uses proxy config setting.

Additional Info

In the following code(L63), it looks like auth-proxy and 5 digits port is not taken into account.
I checked whether node-https-proxy-agent(L65) supports auth-proxy, and I guess it supports auth-proxy from this comment.

node-pre-gyp/lib/install.js

Lines 63 to 75 in f9b3948

	const m = proxyUrl.match(/^(?:(https?:)\/\/)?([^:]+)(?::(\d{1,4}))?\/?$/);
	if (m) {
	const ProxyAgent = require(m[1] === 'https:' ? 'https-proxy-agent' : 'http-proxy-agent');
	const protocol = m[1] === 'https:' ? 'https:' : 'http:';
	const host = m[2];
	const port = +(m[3] || (protocol === 'https:' ? 443 : 80));
	const agentOpts = { host, port, protocol };
	agent = new ProxyAgent(agentOpts);
	log.http('download', 'proxy agent configured using: "%o"', agentOpts);
	} else {
	log.warn('download', 'ignoring invalid "proxy" config setting: "%s"', proxyUrl);
	}
	}

[springmeyer]

That code is new in the v1.x version of node-pre-gyp. So I presume this worked for you with node-pre-gyp@v0.12.0? Asking to try to understand if this is truly a regression or not.

/cc @bmacnaughton

[feuxfollets1013]

Thank you for your reply.
I suppose it is regression. I tried building bcrypt @v.5.0.0 and @v.5.0.1.
According to the change log, bcrypt has only changed node-pre-gyp@0.15.0 to @1.0.0, when bcrypt updated from v.5.0.0 to v5.0.1.

The building result is following.

• v.5.0.0: success
• v.5.0.1: fail WARN download ignoring invalid "proxy"

[bmacnaughton]

i will take a look this weekend. @feuxfollets1013 - thank you for the report.

[bmacnaughton]

@feuxfollets1013 - can you install `npm install https://github.com/mapbox/node-pre-gyp#proxy-url-work and see if that works for you?

i probably should always have let the agent parse the proxy url; i've changed it to do so.

[feuxfollets1013]

@bmacnaughton - Thank you for your support.
I will run it next Monday.
As far as I can see, the code works because if proxyUrl is malformed, then the ProxyAgent constructer throws an error.

[soulchild]

Maybe related: The switch from needle to node-fetch seems to have broken proxy support for us as well (without proxy authentication). After upgrading bcrypt from 5.0.0 to 5.0.1 our builds are now failing because Python is needed to build from source:

> bcrypt@5.0.1 install node_modules/bcrypt
node-pre-gyp install --fallback-to-build
node-pre-gyp ERR! install request to https://github.com/kelektiv/node.bcrypt.js/releases/download/v5.0.1/bcrypt_lib-v5.0.1-napi-v3-linux-x64-musl.tar.gz failed, reason: connect ECONNREFUSED 140.82.121.3:443 
node-pre-gyp WARN Pre-built binaries not installable for bcrypt@5.0.1 and node@14.16.0 (node-v83 ABI, musl) (falling back to source compile with node-gyp) 
node-pre-gyp WARN Hit error request to https://github.com/kelektiv/node.bcrypt.js/releases/download/v5.0.1/bcrypt_lib-v5.0.1-napi-v3-linux-x64-musl.tar.gz failed, reason: connect ECONNREFUSED 140.82.121.3:443 

In my experience, only the now deprecated request library ever got proxy support right. Every other HTTP library I tried either didn't support proxies at all or failed for edge-cases (e.g. redirects not going through the proxies). 😞

[soulchild]

Just a wild guess, but maybe this has to do with the way the code here decides whether to use https-proxy-agent vs http-proxy-agent. Both our proxy env variables are set to a non-https proxy:

http_proxy=http://our-proxy.company.net:3128
https_proxy=http://our-proxy.company.net:3128

In that case, the code mentioned above decides to use http-proxy-agent whereas https-proxy-agent would be the correct choice because the URL that is to be retrieved is an HTTPS one.

If my thinking is correct, this would mean https-proxy-agent can be used in all cases and http-proxy-agent is not required, because https-proxy-agent is already able to connect to http and https proxies:

This module provides an http.Agent implementation that connects to a specified HTTP or HTTPS proxy server, and can be used with the built-in https module.
https://www.npmjs.com/package/https-proxy-agent

Or am I off track here? 😉

[bmacnaughton]

@soulchild - i will take a look at this sometime this week. thanks for your detailed thoughts.

there was some risk to replacing request (now unsupported and deprecated) with the modern node-fetch, but continuing to use an unsupported and deprecated package wasn't an alternative.

[feuxfollets1013]

@bmacnaughton
I created a project contains only following package.json, ran yarn install and got an error.
The following fork ver. bcrypt uses node-pre-gyp@proxy-url-work.

{
 "name": "proxy-works-bcrypt",
 "version": "1.0.0",
 "main": "index.js",
 "license": "MIT",
 "dependencies": {
   "bcrypt": "https://github.com/feuxfollets1013/node.bcrypt.js.git"
 }
}

This is error details.
yarn.log

[bmacnaughton]

i'm seeing the error with bcrypt - it uses a github remote_path in the binary section of package.json and that redirects to amazon. i believe it is the redirect that is creating the problem. if that doesn't seem right to everyone, please let me know.

on the http/https proxy agent choice - i don't think that is an issue. my understanding might be wrong, but i believe that both handle http and https requests; the difference is whether the traffic between the client and the proxy is encrypted or not.

i will get to it this weekend if i can't find time during the week.

[soulchild]

Well, at least with our proxy (Squid) using http-proxy-agent causes errors whereas with https-proxy-agent I'm correctly getting the redirect to https://github-releases.githubusercontent.com you're mentioning.

Can you give this a try and see if you're getting similar results?

const fetch = require('node-fetch');
const HTTPProxyAgent = require('http-proxy-agent');
const HTTPSProxyAgent = require('https-proxy-agent');

const proxyUrl = 'http://proxy.company.net:3128';
const m = proxyUrl.match(/^(?:(https?:)\/\/)?([^:]+)(?::(\d{1,4}))?\/?$/);
const protocol = m[1] === 'https:' ? 'https:' : 'http:';
const host = m[2];
const port = +(m[3] || (protocol === 'https:' ? 443 : 80));
const agentOpts = { host, port, protocol };

const httpAgent = new HTTPProxyAgent(agentOpts);
const httpsAgent = new HTTPSProxyAgent(agentOpts);

(async() => {
  console.log(`Non-SSL URL w/ httpAgent  : ${(await fetch('http://example.com', { agent: httpAgent })).status}`);
  console.log(`Non-SSL URL w/ httpsAgent : ${(await fetch('http://example.com', { agent: httpsAgent })).status}`);
  console.log(`SSL URL     w/ httpAgent  : ${(await fetch('https://example.com', { agent: httpAgent })).status}`);
  console.log(`SSL URL     w/ httpsAgent : ${(await fetch('https://example.com', { agent: httpsAgent })).status}`);
})().catch(console.error);

Results

Non-SSL URL w/ httpAgent  : 200
Non-SSL URL w/ httpsAgent : 403
SSL URL     w/ httpAgent  : 503
SSL URL     w/ httpsAgent : 200

EDIT: The redirect is also followed correctly when utilizing https-proxy-agent and I'm receiving the bcrypt binary:

const res = await fetch('https://github.com/kelektiv/node.bcrypt.js/releases/download/v5.0.1/bcrypt_lib-v5.0.1-napi-v3-linux-x64-musl.tar.gz', { agent: httpsAgent });
console.log(await res.text());

[feuxfollets1013]

I made a few changes to your code and tried to run it.
In my environment, Non-SSL URL w/ httpsAgent status is 200.

EDIT: The redirect is also followed correctly when utilizing https-proxy-agent and I'm receiving the bcrypt binary:

I was able to download it successfully too.

const fetch = require('node-fetch');
const HTTPProxyAgent = require('http-proxy-agent');
const HTTPSProxyAgent = require('https-proxy-agent');

const proxyUrl = 'http://id:pass@myproxy.com:8080/';
const httpAgent = new HTTPProxyAgent(proxyUrl);
const httpsAgent = new HTTPSProxyAgent(proxyUrl);

const fetchBcrypt = async() => {
  const res = await fetch('https://github.com/kelektiv/node.bcrypt.js/releases/download/v5.0.1/bcrypt_lib-v5.0.1-napi-v3-linux-x64-musl.tar.gz', { agent: httpsAgent });
  console.log(await res.text());
}

(async() => {
  console.log(`Non-SSL URL w/ httpAgent  : ${(await fetch('http://example.com', { agent: httpAgent })).status}`);
  console.log(`Non-SSL URL w/ httpsAgent : ${(await fetch('http://example.com', { agent: httpsAgent })).status}`);
  console.log(`SSL URL     w/ httpAgent  : ${(await fetch('https://example.com', { agent: httpAgent })).status}`);
  console.log(`SSL URL     w/ httpsAgent : ${(await fetch('https://example.com', { agent: httpsAgent })).status}`);
  await fetchBcrypt();
})().catch(console.error);

Results

Non-SSL URL w/ httpAgent  : 200
Non-SSL URL w/ httpsAgent : 200
SSL URL     w/ httpAgent  : 503
SSL URL     w/ httpsAgent : 200

[soulchild]

@feuxfollets1013 Interesting! Don't know why Non-SSL w/ httpsAgent works in your case, but the fact that SSL URLs only work with httpsAgent is a case in point that http-proxy-agent probably isn't needed and is even causing the download to fail, right?

Also, https-proxy-agent seems to be able to parse the proxy URL on its own just fine, so maybe the deconstruction into host, port and protocol done is unnecessary?

[feuxfollets1013]

Also, https-proxy-agent seems to be able to parse the proxy URL on its own just fine, so maybe the deconstruction into host, port and protocol done is unnecessary?

I think we don't need to deconstruct the url, because if we call https-proxy-agent constructor with a string url, the constructor destructs the url using url.parse.
https://github.com/TooTallNate/node-https-proxy-agent/blob/c30a9dc75a272591a80b9b4acef1496f80824f88/src/agent.ts#L31-L41

[bmacnaughton]

@feuxfollets1013 @soulchild - thank you both for your efforts on this. i will make make the change to only use https-proxy-agent in the proxy-url-work branch. that already has removed url.parse() in favor of leaving that to the proxy agent. i will post a message here when done and look forward to hearing from you both. i might be able to get it done tonight or tomorrow but it will be the weekend at the latest (i'm really crunched at work right now). once again, thank you both for digging into this.

[bmacnaughton]

@feuxfollets1013 @soulchild - https://github.com/mapbox/node-pre-gyp/tree/proxy-url-work uses only https-proxy-agent and previously removed url.parse() in favor of letting the proxy agent handle it.

If you get a chance to give it a try that would be great. i plan on adding additional tests this weekend, but it seems to address the issues you've raised.

[bmacnaughton]

@springmeyer - i think this is an improvement at a very minimum. i have just tested this locally running through a squid proxy to install bcrypt (which redirects - it can be seen using the previous version:

bruce@lapux:~/github.com/test-npg-bcrypt/node_modules/bcrypt$ npx node-pre-gyp install --update-binary
npx: installed 66 in 2.987s
node-pre-gyp info it worked if it ends with ok
node-pre-gyp info using node-pre-gyp@0.17.0
node-pre-gyp info using node@14.15.5 | linux | x64
node-pre-gyp WARN Using needle for node-pre-gyp https download 
node-pre-gyp http GET https://github.com/kelektiv/node.bcrypt.js/releases/download/v5.0.1/bcrypt_lib-v5.0.1-napi-v3-linux-x64-glibc.tar.gz
node-pre-gyp WARN download ignoring invalid "proxy" config setting: "localhost:3128"
node-pre-gyp http 302 https://github.com/kelektiv/node.bcrypt.js/releases/download/v5.0.1/bcrypt_lib-v5.0.1-napi-v3-linux-x64-glibc.tar.gz
node-pre-gyp http 200 https://github.com/kelektiv/node.bcrypt.js/releases/download/v5.0.1/bcrypt_lib-v5.0.1-napi-v3-linux-x64-glibc.tar.gz
node-pre-gyp info install unpacking napi-v3/bcrypt_lib.node
node-pre-gyp info tarball done parsing tarball
[bcrypt] Success: "/home/bruce/github.com/test-npg-bcrypt/node_modules/bcrypt/lib/binding/napi-v3/bcrypt_lib.node" is installed via remote
node-pre-gyp info ok

but with the version in this branch the redirect is transparent and the download still succeeds:

bruce@lapux:~/github.com/test-npg-bcrypt/node_modules/bcrypt$ npx node-pre-gyp install --update-binary
node-pre-gyp info it worked if it ends with ok
node-pre-gyp info using node-pre-gyp@1.0.1
node-pre-gyp info using node@14.15.5 | linux | x64
node-pre-gyp http GET https://github.com/kelektiv/node.bcrypt.js/releases/download/v5.0.1/bcrypt_lib-v5.0.1-napi-v3-linux-x64-glibc.tar.gz
node-pre-gyp http download proxy agent configured using: "localhost:3128"
node-pre-gyp info install unpacking napi-v3/bcrypt_lib.node
node-pre-gyp info extracted file count: 1 
[bcrypt] Success: "/home/bruce/github.com/test-npg-bcrypt/node_modules/bcrypt/lib/binding/napi-v3/bcrypt_lib.node" is installed via remote
node-pre-gyp info ok 

@feuxfollets1013 and @soulchild - it would be great if you could install this version as a dependency in an already-downloaded bcrypt and manually execute a command similar to the one i did above: :~/github.com/test-npg-bcrypt/node_modules/bcrypt$ npx node-pre-gyp install --update-binary (modified for your local environment). your verification would be much appreciated.

(you can install with npm install https://github.com/mapbox/node-pre-gyp#proxy-url-work or you can reference a local copy.)

i really appreciate the testing you two put into this. my understanding was that the only difference between the two was whether the connection from client => proxy was http or https. the test showed that my understanding was wrong (in this implementation at the very least) and that just using the https proxy agent is the best solution.

[springmeyer]

Thank you everyone for the great collaboration here! Happy to cut a new release once @bmacnaughton thinks the fixed code is ready.

[bmacnaughton]

@feuxfollets1013 @soulchild - please give this branch a try if you can - #573. I believe that it addresses the issues you've raised.

I added a test using the simple proxy that's part of our test suite and, in the debugger, verified that node-fetch is handling a redirect even though it is transparent to node-pre-gyp, and that the downloaded file unpacks and has the expected contents. i do not test authentication but as that is handled by the agent, don't believe that should be an issue.

i think we should target a release on tuesday or wednesday - let me know if that timeframe is to aggressive for your testing. or, if you just won't have time to test, let me know as well and we can move ahead sooner.

thank you again for the work you've done and information you've provided.

[soulchild]

Sorry for the late reply! This looks good to me:

> https_proxy=http://proxy.company.net:3128 http_proxy=http://proxy.company.net:3128 ../.bin/node-pre-gyp install --update-binary
node-pre-gyp info it worked if it ends with ok
node-pre-gyp info using node-pre-gyp@1.0.1
node-pre-gyp info using node@14.13.1 | darwin | x64
node-pre-gyp http GET https://github.com/kelektiv/node.bcrypt.js/releases/download/v5.0.1/bcrypt_lib-v5.0.1-napi-v3-darwin-x64-unknown.tar.gz
node-pre-gyp http download proxy agent configured using: "http://proxy.company.net:3128"
node-pre-gyp info install unpacking napi-v3/bcrypt_lib.node
node-pre-gyp info extracted file count: 1
[bcrypt] Success: "/Users/.../Desktop/node-pre-gyp-bug/node_modules/bcrypt/lib/binding/napi-v3/bcrypt_lib.node" is installed via remote
node-pre-gyp info ok

Thanks for the work, @bmacnaughton!

[bmacnaughton]

awesome @soulchild. i've just pushed a few changes to the tests only (so they run on node 8). proxy support is really important and we knew we were likely to run into some troubles replacing request but felt like we needed to. the information you provided helped immensely. @springmeyer - let's give @feuxfollets1013 another day to test and then get this released on tuesday, if that works for you.

[feuxfollets1013]

@bmacnaughton - Sorry for the late reply. I got an following error.

node-pre-gyp info it worked if it ends with ok
node-pre-gyp info using node-pre-gyp@1.0.1
node-pre-gyp info using node@14.15.5 | win32 | x64
node-pre-gyp info check checked for "C:\Program Files\Jenkins\workspace\MY_POJECT\node_modules\bcrypt\lib\binding\napi-v3\bcrypt_lib.node" (not found)
node-pre-gyp http GET https://github.com/kelektiv/node.bcrypt.js/releases/download/v5.0.1/bcrypt_lib-v5.0.1-napi-v3-win32-x64-unknown.tar.gz
node-pre-gyp http download proxy agent configured using: "http://id:pass@myproxy.com:8080/"
node-pre-gyp ERR! install request to https://github.com/kelektiv/node.bcrypt.js/releases/download/v5.0.1/bcrypt_lib-v5.0.1-napi-v3-win32-x64-unknown.tar.gz failed, reason: getaddrinfo ENOTFOUND github.com 

I found out the part(L70) that seems to be the cause.

node-pre-gyp/lib/install.js

Lines 67 to 72 in a0100ab

	const options = url.parse(sanitized);
	options.agent = agent;
	fetch(options)
	.then((res) => {
	if (!res.ok) {

I changed to following and it works for me, but I don't understand for detail yet, because I haven't checked node-fetch args at well.
https://github.com/feuxfollets1013/node-pre-gyp/blob/13ea032e189b3efa224923e6623805694ad76a4f/lib/install.js#L67-L70

[bmacnaughton]

i suppose there is no point in parsing the url at all. let me see how all the other tests work.

thanks for testing in advance of a release. it really helps.

[bmacnaughton]

i just pushed a version making your changes - i think it makes perfect sense to let node-fetch handle the parsing. url.parse is deprecated and parsing the url should be the responsibility of the api in most cases. i just tested locally and all seems to work, so i pushed the changes to the branch. if you want to try again, feel free to, but as they are your changes i think everything will work just fine.

[feuxfollets1013]

@bmacnaughton - It works!

i think it makes perfect sense to let node-fetch handle the parsing. url.parse is deprecated and parsing the url should be the responsibility of the api in most cases.

I agree with you, because node-fetch constructor parses a request url at first, so we don't need to parse the url in most cases.

Thank you for your rapidly and great supports!

[springmeyer]

I've released the great work of @bmacnaughton in #573 as @mapbox/node-pre-gyp@1.0.2. Will close this now. Please re-open new tickets if anyone still sees any problems.

[soulchild]

Thanks for the great work guys! I really appreciate that. Just wanted to report back that our CI is happy again as well: It's correctly downloading a pre-built bcrypt binary via our corporate proxy. 👍