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

Set custom CA file path for yarn berry #5783

Merged
merged 8 commits into from
Sep 29, 2022
Merged

Set custom CA file path for yarn berry #5783

merged 8 commits into from
Sep 29, 2022

Conversation

jurre
Copy link
Member

@jurre jurre commented Sep 22, 2022

In order for yarn berry to pick up our custom CA file path, we can set an environment variable instead of a custom config file.

See https://yarnpkg.com/configuration/yarnrc#caFilePath:

Finally, note that most settings can also be defined through environment variables (at least for the simpler ones; arrays and objects aren't supported yet). To do this, just prefix the names and write them in snake case: YARN_CACHE_FOLDER will set the cache folder (such values will overwrite any that might have been defined in the RC files - use them sparingly).

@jurre jurre requested a review from a team as a code owner September 22, 2022 20:14
@jurre
Copy link
Member Author

jurre commented Sep 22, 2022

Didn't seem to do the trick in staging 🤔

@pavera
Copy link
Contributor

pavera commented Sep 23, 2022

Starting from the following error:

(Dependabot::SharedHelpers::HelperSubprocessFailed)
updater | ERROR <job_467881699> Internal Error: Error when performing the request
updater | <job_467881699>     at ClientRequest.<anonymous> (/usr/lib/node_modules/corepack/dist/corepack.js:16003:20)
updater | <job_467881699>     at ClientRequest.emit (node:events:513:28)
updater | <job_467881699>     at TLSSocket.socketErrorListener (node:_http_client:481:9)
updater | <job_467881699>     at TLSSocket.emit (node:events:513:28)
updater | <job_467881699>     at emitErrorNT (node:internal/streams/destroy:157:8)
updater | <job_467881699>     at emitErrorCloseNT (node:internal/streams/destroy:122:3)
updater | <job_467881699>     at processTicksAndRejections

I've managed to find a resolution for yarn >= 4.0.0-rc.18.

Based on the error I found this corepack issue which led me to setting the NODE_EXTRA_CA_CERTS env var.

Once that was set we started seeing:

ERROR <job_cli> Error processing @braintree/sanitize-url (Dependabot::SharedHelpers::HelperSubprocessFailed)
ERROR <job_cli> ➤ YN0000: ┌ Resolution step
<job_cli> ➤ YN0001: │ RequestError: getaddrinfo EAI_AGAIN registry.yarnpkg.com
<job_cli>     at ClientRequest.<anonymous> (/home/dependabot/.node/corepack/yarn/3.2.1/yarn.js:195:14361)
<job_cli>     at Object.onceWrapper (node:events:628:26)

This is because yarn does not respect HTTP_PROXY or HTTPS_PROXY env vars. I've added yarn config set commands for these to this PR. Alternatively we need to set these in the container env at startup.

Yarn > 4.0.0-rc.18 was then functional, however due to yarnpkg/berry#3684 earlier versions fail with RequestError: tunneling socket could not be established, statusCode=407. I think we will need to find a way to patch yarn to add support for this given the zero install nature of yarn berry.

@pavera pavera requested review from pavera and honeyankit September 23, 2022 20:38
@jurre jurre force-pushed the jurre/yarn-berry-ca-path branch from 5100b69 to 7986ff1 Compare September 26, 2022 07:27
@jurre
Copy link
Member Author

jurre commented Sep 26, 2022

Thanks for figuring that out @pavera 🎉

So it seems that the issue is mainly that proxy auth credentials aren't supported by older versions of yarn berry, but proxy authentication in itself is supported (although via this slightly funky setup instead of just relying on HTTP(S)_PROXY which most programs do). When I say "proxy auth credentials" I refer to the http basic auth headers that our proxy expects and that we expose in the HTTP_PROXY environment variable.

Right now I think we have a couple of options:

  1. We only support yarn berry > 4.0.0-rc.18. This sucks IMO because most people aren't on that version, we should support the widest number of installs and not require people to use a specific version.
  2. We set up yet another proxy without any authentication that forwards requests from yarn to the authenticated proxy. This is a bit cumbersome, but it should work and we'd be able to support all yarn berry releases
  3. We get rid of basic authentication in our proxy. From what I understand, we have strict networking policies in place to ensure that a job VM only has access to its dedicated proxy, no other machine in the network should be able to access it. Anything running inside the VM already has access to the proxy and its credentials as they're exposed as env variables. I think it should be safe to get rid of the basic auth. This would mean having to update this configuration in a couple of places (our runner infra, our actions runner infra, CLI, debugger, some scripts that run proxy and core in tandem).

As far as I'm concerned option 1 is not acceptable, but the other to options are both worth considering. Maybe I'm missing something?

@brrygrdn
Copy link
Contributor

I've read over the linked issues, but I'm not entirely clear on the regression from YARN 1 behaviour where I presume proxy auth worked 🤔

I can see the recommendation of setting it via:

yarn config set proxy http://<username>:<password>@proxy.com:1234

in yarnpkg/berry#3684 (comment)

Is there no way we can set both the Yarn 1 & berry envvars and then use version detection to set the configuration, or will these config variables fight each other in a way I'm not seeing?

@jurre
Copy link
Member Author

jurre commented Sep 26, 2022

@brrygrdn from my understanding, the issue is that before yarnpkg/berry#4243 the <username>:<password> part in the proxy url is ignored by yarn berry, which leads to the RequestError: tunneling socket could not be established, statusCode=407 error.

Since Yarn Berry repo's typically ship with either the version of yarn that they used checked in to the repo, or specify what specific version to use which will then by installed on-the-fly by corepack, we can't rely on simply installing that latest release-candidate, but will instead use whatever version of yarn that is used by the repo (which is good).

@brrygrdn
Copy link
Contributor

Since Yarn Berry repo's typically ship with either the version of yarn that they used checked in to the repo, or specify what specific version to use which will then by installed on-the-fly by corepack, we can't rely on simply installing that latest release-candidate, but will instead use whatever version of yarn that is used by the repo (which is good).

Hmm, could we interrogate the version of Yarn in use and set the envvars accordingly? I'm assuming not since some are being set in the docker image and it might result in his having to wrap every yarn command in a bunch of environment manipulation 🤔

@jurre
Copy link
Member Author

jurre commented Sep 26, 2022

Since Yarn Berry repo's typically ship with either the version of yarn that they used checked in to the repo, or specify what specific version to use which will then by installed on-the-fly by corepack, we can't rely on simply installing that latest release-candidate, but will instead use whatever version of yarn that is used by the repo (which is good).

Hmm, could we interrogate the version of Yarn in use and set the envvars accordingly? I'm assuming not since some are being set in the docker image and it might result in his having to wrap every yarn command in a bunch of environment manipulation 🤔

We can, and we do, but in currently released versions of yarn berry setting these env variables does not work when they contain basic auth, as yarn does not forward the basic auth credentials, so any request made to the proxy fails

Edit: Let me try to clarify. Yarn classic (1) is working fine, no issues there, the new env variables also don't clash. Yarn Berry however, currently does not work at all when the proxy is also running. We can correctly set the configuration to tell yarn what proxy to use (yarn config set httpProx <url>), however yarn does not pick up the basic auth creds from that URL on versions before 4.0.0-rc.18, which results in yarn attempting to make unauthenticated requests to the proxy.

@jakecoffman
Copy link
Member

I wonder if we could monkey patch the auth fix with a plugin?

@jurre
Copy link
Member Author

jurre commented Sep 26, 2022

I wonder if we could monkey patch the auth fix with a plugin?

I briefly looked into it, but it didn't seem like you're able to override that part of yarn? 🤔

@jurre jurre force-pushed the jurre/yarn-berry-ca-path branch from 991c90d to 2cfec5d Compare September 26, 2022 13:51
@jurre
Copy link
Member Author

jurre commented Sep 26, 2022

I've confirmed that the current changes work for both yarn 3 and 4 when removing basic auth from the proxy, I think we can merge these changes and then discuss what to do about the proxy separately, but I think it should be safe to remove the basic auth because we already have 2 other layers of protection:

  • Networking rules ensure only the updater VM/container has access to its dedicated proxy container
  • There is a dedicated CA cert required to talk to the proxy that wouldn't be available to other vms/containers

@mctofu
Copy link
Contributor

mctofu commented Sep 26, 2022

  • There is a dedicated CA cert required to talk to the proxy that wouldn't be available to other vms/containers

The cert is to allow clients to validate/trust https responses from the proxy but wouldn't actually block usage.

  • Networking rules ensure only the updater VM/container has access to its dedicated proxy container

It'd be worth reviewing if the proxy ever listens on its internet facing network. If so, we should shut that off regardless of auth as that could be an issue when running the proxy locally for development.

I agree, besides confirming the proxy only listens on the internal updater network I'm not seeing much risk in removing the proxy auth.

@jurre jurre force-pushed the jurre/yarn-berry-ca-path branch 2 times, most recently from 1f00528 to 0a738d5 Compare September 28, 2022 13:21
@jurre jurre force-pushed the jurre/yarn-berry-ca-path branch from 0a738d5 to 936a32f Compare September 28, 2022 17:52
@jurre jurre merged commit 42425eb into main Sep 29, 2022
@jurre jurre deleted the jurre/yarn-berry-ca-path branch September 29, 2022 10:00
@pavera pavera mentioned this pull request Oct 31, 2022
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.

6 participants