Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for config file option 'cafile' #736

Merged
merged 1 commit into from
Oct 15, 2016
Merged

Add support for config file option 'cafile' #736

merged 1 commit into from
Oct 15, 2016

Conversation

chlunde
Copy link
Contributor

@chlunde chlunde commented Oct 11, 2016

Summary

With a private registry using a company CA issued certificate, yarn fails because nodejs does not use the OS SSL/TLS trust store (at least on Linux). The same issue affects registries using self signed certificates and might also apply for the public registries in companies using SSL/TLS intercepting proxies.

This is change adds support for the cafile-option, which should point to a file path containing one or more PEM files concatenated together, like in npm.

I've chosen cafile and not the ca option, because it is simpler to point to the operating system TLS trust store, which often will already have the correct CA certificates for corporate computers.

This does not add support for disabling strict-ssl, because that will open for MITM attacks.

References #606, #631

Test plan

_Integration test_
I've added a test which sets up a web server with a self signed certificate and a trust store containing a normal CA and the self signed certificate, and verifies that a connection set up using RequestManager can perform a GET-request from that server.

_Manual test_
With default config an no cafile set, I get following stack trace:

~/yarn/bin/yarn install
yarn install v0.15.1
[1/4] Resolving packages...
[2/4] Fetching packages...
error unable to verify the first certificate
    at Error (native)
    at TLSSocket.<anonymous> (_tls_wrap.js:1017:38)
    at emitNone (events.js:67:13)
    at TLSSocket.emit (events.js:166:7)
    at TLSSocket._init.ssl.onclienthello.ssl.oncertcb.TLSSocket._finishInit (_tls_wrap.js:582:8)
    at TLSWrap.ssl.onclienthello.ssl.oncertcb.ssl.onnewsession.ssl.onhandshakedone (_tls_wrap.js:424:38)
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.

With cafile=/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem in .npmrc or cafile /etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem in .yarnrc installation succeeds

@cpojer
Copy link
Contributor

cpojer commented Oct 12, 2016

@chlunde Thanks! Do you mind adding an integration test for this feature?

@sindresorhus
Copy link

Wouldn't it be better to make Yarn use the OS TLS trust store instead of adding an option?

@donaldpipowitch
Copy link
Contributor

I'm not sure if this the same error, but... on our custom registry we need to set I also get error unable to verify the first certificate on macOS..npmrc supports a config strict-ssl=false in this case.

@chlunde
Copy link
Contributor Author

chlunde commented Oct 12, 2016

@sindresorhus Having nodejs, not Yarn, use the OS trust store by default would be a good thing in the long run. There's some information related to that issue here: https://www.happyassassin.net/2015/01/12/a-note-about-ssltls-trusted-certificate-stores-and-platforms/

But I think a change like this would be required anyway if someone uses a self-signed certificate or cannot add the registry CA to the default trust store for some other reason.

@sindresorhus
Copy link

There might not be any way around this, true. Just want to recommend being careful about adding options. It's usually better if it just works. Now users must know to set the cafile option instead of it just relying on the OS trust store like other apps. And you don't want Yarn to end up with almost 100 options, like npm.

@@ -118,6 +123,12 @@ export default class RequestManager {
if (opts.httpsProxy != null) {
this.httpsProxy = opts.httpsProxy;
}

if (opts.cafile != null && opts.cafile != '') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome! Would you mind adding a short comment here explaining what this does and how it works?

@chlunde
Copy link
Contributor Author

chlunde commented Oct 12, 2016

@kittens @cpojer Thanks for the comments, I've updated the PR with integration tests. I added support for comments between the certificates (like used by https://curl.haxx.se/docs/caextract.html), and no longer throw an exception if the file is missing, because yarn config set cafile "correctpath" was not working when the current cafile was missing.

Issues:

  • tests broken since add cache dir command to output path to cache directory #880 (comment)
  • is it OK to hardcode port 3443 for the tests?
  • is there a better way to prevent caching the test file? I added nocache as a query parameter and look for that in the fixture request handler
  • importing http-proxy fails, is this an issue with my code or jest-resolve?
import httpProxy from 'http-proxy';
console.dir(httpProxy);

fails with

    TypeError: Cannot set property 'createProxy' of undefined

      at Object.<anonymous> (node_modules/http-proxy/lib/http-proxy.js:28:30)
      at Object.<anonymous> (node_modules/http-proxy/index.js:13:18)
      at _load_httpProxy (__tests__/fetchers.js:27:1636)
      at Object.<anonymous> (__tests__/fetchers.js:29:55)

@chlunde
Copy link
Contributor Author

chlunde commented Oct 14, 2016

I've updated the PR to address my own comments, except for the "nocache" query parameter in the mock which I've kept. The branch cafile-old in my repo contains the original integration test.

Add support for local registries with TLS/SSL certificates issued by
private CAs certificates or self-signed certificates.

References #606, #631
@sebmck sebmck merged commit 7a18523 into yarnpkg:master Oct 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants