search

npm / npm

This repository is moving to: https://github.com/npm/cli
http://npm.community
17.53k stars 3.02k forks source link

npm@5.0.3: When proxied, `strict-ssl=false` or custom CAfile isn’t working #16868

Closed colinrotherham closed 7 years ago

colinrotherham commented 7 years ago

I'm opening this issue because:

What's going wrong?

Running npm install on npm@4.6.1 works no problem Running npm install on npm@5.0.3 throws UNABLE_TO_VERIFY_LEAF_SIGNATURE

We're using an internal registry via npm config set registry which is behind a corporate proxy. The registry is protected using a self-signed root CA certificate.

We know our custom registry has an invalid SSL certificate. Setting strict-ssl=false should workaround this.

The corporate root CA is trusted locally (Windows and Mac keychain etc) but for npm we add a custom certificate bundle via npm config set cafile … and strict-ssl=false as a workaround.

How can the CLI team reproduce the problem?

Try running npm install to a custom registry with an invalid SSL certificate. To avoid any errors, ensure strict-ssl=false is set.

It works on npm@4.6.1 but throws an error on npm@5.0.3.

Included is my npm-debug.log (sensitive info replaced): https://gist.github.com/colinrotherham/ce6d21ba35271e356f9255a5c12c3d94

Thanks

supporting information:

colinrotherham commented 7 years ago

As mentioned in the linked gist above, our .npmrc file contains:

strict-ssl=false
cafile=/path/to/certificates/cacert.pem

Under npm@4.6.1 this allowed our self-signed root CA certificate to be accepted. Under npm@5.0.1 this throws an UNABLE_TO_VERIFY_LEAF_SIGNATURE error.

jimivdw commented 7 years ago

It appears there is an issue with the propagation of these options all the way through to tls. I managed to work around it (for now) by setting the NODE_TLS_REJECT_UNAUTHORIZED environment variable to 0, but I still would say this needs to be fixed.

colinrotherham commented 7 years ago

Thanks @jimivdw, forgot all about that one. I've used it in the past.

My gist does show lots of unable to verify the first certificate errors which seems to suggest our custom CA root certificate isn't accepted anymore, or is unknown to npm@5.0.1.

I'd expect:

  1. Our custom cafile should suppress the error (i.e. it should trust the custom CA root)
  2. Otherwise, strict-ssl=false should ignore all SSL errors anyway

We'd prefer only 1) for security but both were needed under npm@4.6.1.

zkat commented 7 years ago

This may not be getting passed down to pacote. The chain of configs looks something like this: npm/lib/config/pacote.js -> pacote/lib/util/opt-check.js -> pacote/lib/fetchers/registry/fetch.js -> make-fetch-happen/agent.js

See if you can trace that, and I'll totally take a PR if I don't get around to this before someone else does.

colinrotherham commented 7 years ago

Thanks @zkat, appreciate the help.

Let me take a look tomorrow and see what our setup suggests.

zkat commented 7 years ago

@colinrotherham thank you! I'm sure it'll be really obvious as soon as you trace it 👍

colinrotherham commented 7 years ago

Hi @zkat,

Suggested changes:

  1. The ca key missing from function pacoteOpts

    Add to const opts in npm/lib/config/pacote.js — It's available via npm.config.get('ca')

  2. The ca key also missing from function regFetch

    See 2nd argument for fetch() in pacote/lib/fetchers/registry/fetch.js — It'll now be available via opts.ca

  3. A line for this.ca is missing from function PacoteOptions e.g. Add this.ca = opts.ca

I'm starting a pull request now.

But, there's one other issue. Currently I need to comment out these lines in make-fetch-happen/agent.js for everything to work again:

  // if (pxuri) {
  //   const proxy = getProxy(pxuri, opts, isHttps)
  //   AGENT_CACHE.set(key, proxy)
  //   return proxy
  // }

Any ideas before I take a look at that too?

Thanks

rally25rs commented 7 years ago

The behavior of strict-ssl is inconsistent when items are in the cache vs not cached...

With packages previously installed and cached, then allow-strict=false is added to .npmrc you get a warning:

$ cat .npmrc
proxy=http://127.0.0.1:8080
https-proxy=http://127.0.0.1:8080
strict-ssl=false

$ rm -rf node_modules/

$ npm i --prefer-online
npm WARN registry Unexpected warning for https://registry.npmjs.org/: Miscellaneous Warning SELF_SIGNED_CERT_IN_CHAIN: request to https://registry.npmjs.org/fsevents/-/fsevents-1.1.1.tgz failed, reason: self signed certificate in certificate chain
   ...rest of output omitted...
added 1317 packages in 28.015s

But of you clear the cache, it fails:

$ cat .npmrc
proxy=http://127.0.0.1:8080
https-proxy=http://127.0.0.1:8080
strict-ssl=false

$ npm cache clear --force
npm WARN using --force I sure hope you know what you are doing.

$ rm -rf node_modules/

$ npm i
npm ERR! code SELF_SIGNED_CERT_IN_CHAIN
npm ERR! errno SELF_SIGNED_CERT_IN_CHAIN
npm ERR! request to https://registry.npmjs.org/fsevents/-/fsevents-1.1.1.tgz failed, reason: self signed certificate in certificate chain

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/me/.npm/_logs/2017-06-02T14_28_02_289Z-debug.log

edit:

I think what happens in the first case with the cache in place is that it tries to contact the registry, fails because of the self-signed cert and prints the warning, then falls back to using the cache (behaves the same as being offline after the initial failure). I believe that explains the seemingly "inconsistent" behavior. Looks like just a matter of the setting not being passed down.

colinrotherham commented 7 years ago

Hi @zkat @rally25rs,

So far my debugging has led me to this package: https://github.com/TooTallNate/node-https-proxy-agent

See file https-proxy-agent.js.

With my two pull requests above, the ca option now makes it into make-fetch-happen, but it never makes it into tls.connect(opts) (i.e. it's not in the options).

colinrotherham commented 7 years ago

Found the next part of the issue.

Even if a cafile|ca option is passed into https-proxy-agent (via npm -> pacote -> make-fetch-happen) it's then not passed into tls.connect() when Node calls back.

I've opened https://github.com/TooTallNate/node-https-proxy-agent/pull/30

colinrotherham commented 7 years ago

@zkat I'm worried there's no timeout event support in https-proxy-agent too.

Why? On some npm install runs I'm now getting random hangs.

There was an old pull request a while ago, but was never merged: https://github.com/TooTallNate/node-https-proxy-agent/pull/20

Should npm look at another proxy library?

Thanks for the hard work 😊

zkat commented 7 years ago

@colinrotherham I think @TooTallNate has expressed a strong intent to make sure https-proxy-agent and family are in good condition. I spent some time looking for proxy libraries way back when and found these particular ones to be in the best condition, but I'm always open to changing what make-fetch-happen is using. We already knew going in that proxy stuff was going to be the trickiest part of replacing request -- it's what stopped us from doing it much sooner.

zkat commented 7 years ago

@colinrotherham btw this is amazing, careful work you're doing. Thank you so much! I'll do my best to support this and hopefully we can get all this sorted out. I was hoping we'd be able to find all this during the beta, 'cause I really don't have a full concept of the different ways people use these things, but stuff still slipped through the cracks. 😭

TooTallNate commented 7 years ago

I'm gonna be doing some needed maintenance on my *proxy-agent modules today, including porting that timeout PR to agent-base where it belongs (/cc @colinrotherham), and hopefully figuring out the cleanest fix for this issue as well (I've commented on TooTallNate/node-https-proxy-agent#30).

@zkat Btw, great work on npm5 and all these underlying new pieces!

TooTallNate commented 7 years ago

Also, what is the minimum supported Node.js version for npm5 at this point? It would be nice to drop support for some older versions and start moving to Promise-based APIs.

colinrotherham commented 7 years ago

Thanks @zkat, that's great, good to hear.

@TooTallNate Perfect, that should be everything for our setup. If you take your own direction with https://github.com/TooTallNate/node-https-proxy-agent/pull/30 let me know if I can help test it.

zkat commented 7 years ago

@TooTallNate npm5 supports any maintained Node version. That means down to node@4 right now, until that goes out of maintenance.

colinrotherham commented 7 years ago

Hi @zkat,

Thanks for releasing npm@5.0.3, it's hopefully helping others already.

For our case though we still need to make sure the ca option gets passed into the tls.connect() call inside node-https-proxy-agent.

If I run npm@5.0.3 without https://github.com/TooTallNate/node-https-proxy-agent/issues/30: npm install fails at UNABLE_TO_VERIFY_LEAF_SIGNATURE

If I run npm@5.0.3 with https://github.com/TooTallNate/node-https-proxy-agent/issues/30: npm install largely works but appears to time out for some dependencies.

I can see @TooTallNate is working on https://github.com/TooTallNate/node-agent-base so we'll have to temporarily roll back to npm@4.6.1 for now.

colinrotherham commented 7 years ago

@zkat I can confirm that strict-ssl=false isn't working either under a proxy environment.

I've tweaked my pull request https://github.com/TooTallNate/node-https-proxy-agent/pull/30/

colinrotherham commented 7 years ago

We've moved back on to npm@5.0.3 but with export NODE_TLS_REJECT_UNAUTHORIZED="0" set temporarily until the options can be handled by https://github.com/TooTallNate/node-https-proxy-agent/pull/30

The speed improvements were worth it, it's a good upgrade 😊

colinrotherham commented 7 years ago

Hi @zkat,

I've created a pull request for make-fetch-happen: https://github.com/zkat/make-fetch-happen/pull/32 to pull in the options rewrite from https://github.com/TooTallNate/node-agent-base/commit/1939a2f32ca1b0fc61dbf3a808269c9f35e5c745.

Test results as follows:

  1. Setting strict-ssl=false suppresses UNABLE_TO_VERIFY_LEAF_SIGNATURE Result: 👍 Running npm install works perfectly

  2. Setting custom cafile option suppresses UNABLE_TO_VERIFY_LEAF_SIGNATURE Result: 👍 Running npm install works perfectly

  3. Setting neither still outputs the warning:

npm WARN registry Unexpected warning for https://registry.domain.co.uk/repository/internet-npm/: Miscellaneous Warning UNABLE_TO_VERIFY_LEAF_SIGNATURE: request to https://registry.domain.co.uk/repository/internet-npm/@namespace%2fpackage failed, reason: unable to verify the first certificate

If that looks alright to you, we can close this issue! Thanks again

zkat commented 7 years ago

@colinrotherham Your patch is merged into release-next, and I've pushed out npmc@5.0.4-canary.3 if you want to try something right now and verify that everything works ok for you. Once you confirm that the canary passes, go ahead and close this issue 👍

colinrotherham commented 7 years ago

Thanks @zkat that's great, appreciate your help. I've pulled in the release-next branch and everything's working as expected! 😊

zkat commented 7 years ago

@colinrotherham thank YOU for your patch! I'm glad everything is back in working order for you 💚