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

fix: do not proxy traffic to sockets #60

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tommyknows
Copy link

This commit implements a proxy-exception for traffic targeting local sockets instead of a "host:port" combination. If such traffic would be forwarded through the proxy, the socket destination info would be lost, while a "localhost:80" destination would be added. This means that the proxy cannot handle such requests correctly either way, which makes sending traffic to the proxy pointless.

Because of that, if the agent receives traffic destined for a socket, it will simply use the fallback agent to handle that traffic.

Fixes #59.

This commit implements a proxy-exception for traffic targeting local
sockets instead of a "host:port" combination. If such traffic would be
forwarded through the proxy, the socket destination info would be lost,
while a "localhost:80" destination would be added. This means that the
proxy cannot handle such requests correctly either way, which makes
sending traffic to the proxy pointless.

Because of that, if the agent receives traffic destined for a socket, it
will simply use the fallback agent to handle that traffic.
tommyknows added a commit to snyk/cli that referenced this pull request Oct 19, 2022
For CLIv2, all traffic is proxied through a local server that the CLIv2
spawns. This is achieved by updating the `http.globalAgent` and
`http.request` and `http.get` functions through the `global-agent`
library.

However, this causes issues with HTTP traffic that is supposed to go to
local sockets. For example, `snyk container` talks to the
`/var/run/docker.sock` socket for doing some image operations (if the
socket exists). When the http agent and functions are replaced, this
causes traffic to the socket to be routed to / through the proxy, and
because at that point it is "proper" HTTP traffic, the destination is
not the socket anymore but `localhost:80` (the fallback).

The proxy has no way of handling this correctly, as all the information
of the socket is lost.

To fix this, there would have been two possibilities:
- Make it possible to use custom agents. Currently, even if a user
  creates a custom agent and passes it to `http.request` or `http.get`,
  this custom agent will not be used. This is because the `global-agent`
  library *enforces* the usage of the proxy-agent. There is a flag to
  change this behaviour (`forceGLobalAgent`), but we do not want to
  use this as traffic should *always* go through the proxy.
- Fix the `global-agent` library to not use the proxy-agent if traffic
  is supposed to go to sockets.

A [PR](gajus/global-agent#60) has been raised on
the library, but the repository seems to be unmaintaned. Because we need
this fix to continue with the app vulnerability enablement, this commit
adds a `postinstall` hook that patches the library in the `node-modules`
folder through a `git apply` command.

With the applied patch, traffic to the Docker socket (and all other
sockets) will not be rerouted through the proxy anymore.
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.

Do not proxy traffic to sockets
1 participant