search

mapbox / node-pre-gyp

Node.js tool for easy binary deployment of C++ addons
BSD 3-Clause "New" or "Revised" License
1.11k stars 261 forks source link

Unable to use proxy authentication #572

Closed feuxfollets1013 closed 3 years ago

feuxfollets1013 commented 3 years ago

Current Behavior

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

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

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. https://github.com/mapbox/node-pre-gyp/blob/f9b39484f17955d83cdab42c178a600467fe96bd/lib/install.js#L63-L75

springmeyer commented 3 years ago

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 commented 3 years ago

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.

bmacnaughton commented 3 years ago

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

bmacnaughton commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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

bmacnaughton commented 3 years ago

@feuxfollets1013 @soulchild - please give this branch a try if you can - https://github.com/mapbox/node-pre-gyp/pull/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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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. https://github.com/mapbox/node-pre-gyp/blob/a0100abca39b53184a28fd7c93789e66eb136c1d/lib/install.js#L67-L72

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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. :+1: