Added proxy auth headers

[cemremengu]

Continuing #810 since I am currently bitten by this and have to move around node_modules 😄

@cjbj Anything else I need to sign & send? I didn't test this btw but assume that it's working given the previous conversations were towards merging it.

[cjbj]

@cemremengu before we can do anything with this, the OCA must be signed & accepted.

[cemremengu]

@cjbj Done

[cjbj]

@cemremengu that's good news. And don't forget the signoff on the PR - see CONTRIBUTING. As you probably saw, I don't have ready access to a proxy that needs authentication, so it would be good to get a viable PR from someone.

[cemremengu]

Sure, the commit message should have the signature under it as

Signed-off-by: Cemre MENGU <cemre[dot]mengu[at]ttgint[dot]com>

I will make sure to add it for next commits as well. Let me know, once they get my email and anything else is needed

[cemremengu]

@cjbj They rejected it and I sent an updated version same day. It's been 3-4 days haven't heard since. Could you please notify them if possible?

[cjbj]

@cemremengu they batch them up. I'll ping them.

[cjbj]

@cemremengu your OCA was approved - excellent!

What about doing it this way?:

-function getFileReadStreamByProxy(hostname, path, proxyHostname, proxyPort) {
+function getFileReadStreamByProxy(hostname, path, proxyHostname, proxyPort, auth) {
   return new Promise((resolve, reject) => {
     packageUtil.trace('In getFileReadStreamByProxy', hostname, path, proxyHostname, proxyPort);
 
+    let headers = {
+      'host': hostname + ':' + PORT
+    }
+    if (auth) {
+      headers['Proxy-Authorization'] = 'Basic ' + new Buffer(auth).toString('base64');
+    }
+
     // Open a proxy tunnel
     const req = http.request({
       host: proxyHostname,
       port: proxyPort,
       method: 'CONNECT',
       path: hostname + ':' + PORT,
-      headers: {
-        'host': hostname + ':' + PORT,
-      }
+      headers: headers
     });
 
     req.on('error', reject);

And also passing auth in the proxy redirection code:

@@ -212,7 +218,7 @@ function getFileReadStreamByProxy(hostname, path, proxyHostname, proxyPort) {
         proxyHostname = redirectUrl.hostname;
         proxyPort  = redirectUrl.port;
 
-        return getFileReadStreamByProxy(hostname, path, proxyHostname, proxyPort);
+        return getFileReadStreamByProxy(hostname, path, proxyHostname, proxyPort, auth);

[cemremengu]

This may sound dumb but how do I test this locally without npm package ? 😄

npm --proxy http://username:password@cacheaddress.com.br:80 install <what?>

[cjbj]

@cemremengu probably just create the build package with 'cd package; make npmpackage'
But watch out for the hacky way that oracledbinstall.js gets overwritten - you will want to comment out the '$(GIT) checkout oracledbinstall.js' command in Makefile. Then run 'npm --proxy http://username:password@cacheaddress.com.br:80 install /path/to/oracledb-2.3.0.tgz'

[cemremengu]

I will try but the problem is that I am on windows 🤔

Anyways, I will find a unix machine somehwere to build it if it does not work 😄

[cjbj]

@cemremengu the same steps are in MAKEPKG.BAT - just comment out the bits that are not related to creating oracledb-2.3.0.tgz. Effectively you need to run 'npm pack' with the package/package.json moved up to the top level dir. Something like

xcopy /Y "package.json" "../package.json"
npm pack

[cemremengu]

After some fiddling I am getting the error

Error: HTTPS request for https://github.com/oracle/node-oracledb/releases/download/v2.3.0-dev/oracledb-v2.3.0-dev-node-v57-win32-x64.gz failed with code 404

Looks like it is putting a -dev for package name where it shouldn't.

OK, needed to modify package.json to strip that

[cemremengu]

@cjbj Success! One thing is that I expected it to get the proxy from the npm --proxy arg but it is strictly looking for one of the

  let proxy = process.env.https_proxy ||
    process.env.HTTPS_PROXY ||
    process.env.http_proxy ||
    process.env.HTTP_PROXY ||
    process.env.all_proxy ||
    process.env.ALL_PROXY;

is there a way to get rid of this and instead propagate the npm proxy arg to the install script?

[cjbj]

I was thinking about that today when I helped someone else. The only way I know of right now is run npm config get https-proxy from oracledbinstall.js and use that value.

[cemremengu]

Strange...It fails on some machines getting a 403 from github (https) CDN

And exec is painfully slow on some machines too..Still trying other ways, any ideas are appreciated

[cjbj]

@cemremengu npm has some quirks regarding proxies and firewalls, e.g npm/npm#18567. I don't know if you are hitting this.

[cemremengu]

Yes it looks like it is this one and its very random. It also sometimes returns 407 although I provide credentials.

Another thing is execSync('npm config get something') takes couple of minutes to complete on some VMs for some reason. Async version is same too.

Similar to npm/npm#14458

[cjbj]

@cemremengu it's a can of worms.

[Getz85]

Any news about this PR? I can't install oracledb anymore because none of the different installations proposal work:

• proxy error (auth refused because username and password are ignored) with standard npm install
• ssh error with npm install oracle/node-oracledb.git#v2.3.0 (while this worked few months ago...)

npm ERR! Error while executing:
npm ERR! D:\Programmes\Git\cmd\git.EXE ls-remote -h -t ssh://git@github.com/oracle/node-oracledb.git
npm ERR!
npm ERR! Unable to open connection:
npm ERR! Host does not existfatal: Could not read from remote repository.

• 403 error with npm install https://github.com/oracle/node-oracledb/releases/download/v2.3.0/oracledb-src-2.3.0.tgz

[cjbj]

@Getz85 I don't have an authenticated proxy to test with, so you could try this PR and let us know if it works for you. You'd need to build the package as I described above, and use that with 'npm install'.

[cjbj]

@Getz85 and you can always install with https://oracle.github.io/node-oracledb/INSTALL.html#manualextraction or https://oracle.github.io/node-oracledb/INSTALL.html#github

[cemremengu]

I have tested this on 3 different machines (VM) for 2 of them it worked perfectly with npm --(https)proxy option. The other one was an old windows VM which was very slow when calling npm config get.

So as an emergency option (borrowing from the old way) I added a fallback env variable to get the proxy from, in order not to block users installing the package (which eventually worked for the old VM as well).

As described above there can be random proxy auth errors but that's on NPM I guess ¯\_(ツ)_/¯

@cjbj Let me know what you think and if you want to me to change the name of the env variable.

[cjbj]

@cemremengu good work.

Because there are already so many proxy environment variables, I would use those instead of introducing a new one. Basically:

• check existing https_proxy etc environment variables
• if none set, then call npm config get https-proxy
• if that is not set, then call npm config get proxy
• do the no_proxy logic

If npm config get is slow, I'd avoid calling get proxy if get https-proxy already returned a value.

[cjbj]

I'm happy from visual inspection, but let's see if @dmcghan has any comments. :)

[cemremengu]

ping @dmcghan :)

[dmcghan]

Looks good to me overall.

[dmcghan]

Seems like we are favoring proxy settings from env over npm config. Is that standard? Should it be reversed?

[cjbj]

The ordering was chosen as a way to overcome the slow perf issues of npm config get without needing an 'emergency option', see #919 (comment)

[cemremengu]

To explain further:
a) In order not to break current setups
b) Prevent blocking the installation due to any issues with npm config get (though I think it should work fine most of the time)

[cemremengu]

@cjbj One more thing; should we add something like
packageUtil.log(`Using proxy ${proxyConfig.hostname}`); on line 207 or somewhere else to inform the user? The downside is that it will print multiple times like:

oracledb Beginning installation
oracledb Verifying installation
oracledb Continuing installation
oracledb Using proxy xxx.xx.xx.xxx
oracledb Using proxy xxx.xx.xx.xxx
oracledb Oracledb downloaded
oracledb Verifying installation
oracledb Using proxy xxx.xx.xx.xxx
oracledb Using proxy xxx.xx.xx.xxx
oracledb Binary SHA matches SHA in SHASUMS256.txt

Otherwise LGTM I think? 😄

[cjbj]

@cemremengu I would not display the proxy details by default. For a start, I think we can probably cut down on what is displayed by default to reduce visual overload. And second, it's better not to display info we don't really need to avoid users over-sharing site info when they open issues. The details are already logged with packageUtil.trace(), if anyone needs them.

I'm just waiting for a final ack from @dmcghan and then I'll merge to our internal repo so that the change will come out with the next push to GitHub.

[dmcghan]

Yup, looks good to me. I'd probably have reversed the lookup to use NPM first, but that's always something that can be revisited in the future if needed.

[cjbj]

Thanks @dmcghan

[cjbj]

@cemremengu This fix is now on master ready for the next release. Thank you.