Enable extension-specific proxy settings #5650

peterguy · 2024-09-20T22:03:21Z

Support extension-specific proxy settings (don't rely on http(s)_proxy).

Support using a unix domain socket as a proxy.

Add a CA cert setting to enable using proxies with self-signed or not-public certificates.

Test plan

To test locally:

Install socat and mitmproxy (brew install socat mitmproxy)
Switch to this branch: git switch peterguy/vscode-add-proxy-settings
In one terminal, start up socat to listen on a UDS and send TCP connections to mitm

socat -d -d unix-listen:${HOME}/mitm-proxy.sock,fork tcp:localhost:8080

In another terminal, launch mitmproxy. I like to use mitmweb because it opens a web view of the traffic that is friendly.

mitmweb

Add the following to VSCode's settings.json (It's at ~/Library/Application Support/Code/User/settings.json for me; you can also get to it by opening the Command Palette (Cmd-Shift-P), typing "settings" to filter, then selecting "Open User Settings (JSON)".

"http.proxySupport": "on",
"cody.proxy.path": "~/mitm-proxy.sock",
"cody.proxy.cacert": "~/.mitmproxy/mitmproxy-ca-cert.pem"

In addition to specifying proxy settings for the Cody extension, it also toggles VSCode's proxy support from override to on. If it's in the default override mode, it won't use the proxy agents we set up. I'd like to avoid this requirement, but until we figure out how to do that (probably with more "monkey patching", to quote @RXminuS 😄 ), we have to toggle the proxy support to either on or fallback.
5. Launch the Cody extension via the Run And Debug pane (Cmd-Shift-D), by clicking the green "play" icon at the top of the pane, when "Launch VSCode Extension (Desktop, recommended)" is selected (it's the default).
6. Note the outputs of socat and mitmproxy in the two terminals, and if you launched mitmweb, see the requests in the browser. If you are using mitmweb you can use the web interface to see the types of requests being made.
7. Execute a completion. I used Document Code as a simple triggerable one. If you launched mitmweb and are using a non-local LLM model (not Ollama), you should see calls to the completions endpoint in the mitm web page.
8. Optional: change the proxy settings to use mitmproxy directly. Remove the proxyPath setting and add proxyHost and proxyPort:

"cody.proxy.host": "localhost",
"cody.proxy.port": 8080

Then verify again that the extension is still operative. You should still see activity in the terminal that's running mitmproxy (or mitmweb), and the mitmweb web page should show more traffic, but there should be no activity in the socat terminal.

RXminuS · 2024-09-26T20:24:48Z

vscode/src/configuration.ts

+    function expandTilde(path?: string): string | undefined {
+        if (path?.startsWith('~/')) {
+            const homeDir = process.env.HOME || process.env.USERPROFILE
+            if (homeDir) {
+                return `${homeDir}/${path.slice(2)}`
+            }
+        }
+        return path
+    }


Can't this just be a path.resolve?

path.resolve doesn't understand ~/, but I could probably use os.homedir() instead of relying on the environment variables, and maybe also handle %USERPROFILE%\ as well for Windows.

RXminuS · 2024-09-26T20:27:03Z

vscode/src/configuration.ts

+                if (!fs.statSync(path).isSocket()) {
+                    throw new Error('Not a socket')
+                }
+                fs.accessSync(path, fs.constants.R_OK | fs.constants.W_OK)


I'd be very careful with this and double check it platform specific docs. For instance on windows there's all sorts of EACCESS edge cases if the file or folder is busy.

From memory we are already importing extra-fs which handles this or I think the fs/promises avoids some of it.

Good point. Switched to using fs-extra, and looked into issues with Windows and file locks. Looks like stat and access aren't affected by busy files.

RXminuS · 2024-09-26T20:29:33Z

vscode/src/configuration.ts

+    }
+
+    function readProxyCACert(): string | undefined {
+        const path = expandTilde(config.get<string>(CONFIG_KEY.proxyCacert))


We should be moving to the new observable config so it automatically re-calculates on change without reload.

Could be some some initialization timing problems that might come up with reading config as fixed value.

@sqs already did that. I double-checked locally, and the custom agent is re-created when settings.json changes.

RXminuS · 2024-09-26T20:30:11Z

vscode/src/configuration.ts

+            }
+            try {
+                return fs.readFileSync(path, { encoding: 'utf-8' })
+            } catch (error) {


Can you make sure to add a logerror too? Notifications don't show in logs if someone asks for support

Good point! Added them.

... and took them away again. Adding logError caused the code to be called repeatedly. Did I do something wrong? Posted in Slack.

RXminuS · 2024-09-26T20:34:34Z

vscode/src/configuration.ts

    const vsCodeConfig = vscode.workspace.getConfiguration()

    return {
        proxy: vsCodeConfig.get<string>('http.proxy'),
+        proxyHost: config.get<string>(CONFIG_KEY.proxyHost),


Should we validate what it says in description that values need to be set together?

Alternatively, the config is full json schema compatible and it still gets flattened down. so you can instead of separate strings ask for a union of different config objects so that they are always set together.

Good idea. I re-worked the schema to enforce setting them together. Doesn't really enforce anything, but it does show the user when a value is missing. I added an error message display when the config is read to help out.

RXminuS · 2024-09-26T20:41:43Z

vscode/src/fetch.node.ts

+                ...(proxyPort ? { port: proxyPort } : null),
+                ...(proxyPath ? { socketPath: proxyPath } : null),
+                keepAlive: true,
+                keepAliveMsecs: 60000,


🙈 ... oh keepAlive; the "golden bullet" that keeps claiming friendly fire. But that's for a later day 😅

Just for now, I don't think Keep Alive works this way with socks proxy agent. So there might be significant latency overhead.

Also I think EventSource has problems with this and reconnection / server closed connections. Which given how socks proxy agent operates might give issues. Not quite sure But just so you're aware.

ok; I was just making sure to copy the settings that are being used in the proxy agents if the global proxy url is set.

Should we not include keepAlive settings in this proxy agent? They're included in the existing http(s) proxy agents, so I thought they should be included in this one.

arafatkatze · 2024-09-30T06:47:48Z

@peterguy @RXminuS This is an interesting new addition to the proxy space but I wanted to add that its entirely possible certain edge cases be not covered by this.

We would also test all the cases with this google sheet just to extra sure that this actually works. I think using Fiddler Mitm etc is fine but we have sourcegraph based proxies too so using those could also be helpful just to be extra certain.

Aside from what rik already pointed out about the keep alive stuff. We also had a long long history of changes around this as mentioned in this PR . We did use proxy agent before but then caused more issues so we reverted it although perhaps it might be okay to use it just for your case coz pnpm update-agent-recordings worked fine.

You can read the whole history here and if the PR still concedes I am happy to do a deep dive by manually running all the cases etc. There's other issues also with the proxy stuff like the ability to load local windows certs and self signed certs loading being specifically an issue(You are very likely fully aware of all of these).

I am sure you have probably covered many to almost all of these but maybe not ALL of these and that's my concern(Unless you do in-fact cover all of the cases atleast for now).

So a question for you both(Rik and Peter):

If we were to use ProxyAgent library mainly for the proxy stuff then what if we face the same issues as before?
Would it be feasible to wait for Rik's solution here for the stripe thing also?

peterguy · 2024-10-03T04:46:46Z

@arafatkatze this affects VSCode. Do you have a setup where you can test on Windows? I'm working on getting a dev environment set up in Windows, but if you have a setup already, it'll be faster to test.

arafatkatze · 2024-10-03T05:22:36Z

@peterguy When I tested proxy stuff on Windows I would just make the VSIX File(in my macbook environment) by


cd vscode 
CODY_RELEASE_TYPE=stable pnpm release:dry-run

This would make a cody.vsix file in cody/vscode/dist directory and then I would take the file to windows and install cody from that vsix file in Windows to test stuff. So basically I didn't have to deal with any of the dev setup in windows.

I assume you do already have a windows machine so that should help out with the rest and you probably don't have to worry about the licensing stuff etc?
Also as for the real testing I only just used [this website](https://devproxy.sgdev.org/) for proxies so that would cover the cases?

IS that something that would work for you? Or are you perhaps looking at a more complex setup with some sort of packet capture etc?

peterguy · 2024-10-04T00:41:18Z

vscode/src/configuration.ts

+    const proxyConfig = config.get<{
+        server?: string
+        path?: string
+        cacert?: string
+    } | null>(CONFIG_KEY.proxy, null)
+
+    function resolveHomedir(filePath: string | null | undefined): string | undefined {
+        for (const homeDir of ['~/', '%USERPROFILE%\\']) {
+            if (filePath?.startsWith(homeDir)) {
+                return `${os.homedir()}${path.sep}${filePath.slice(homeDir.length)}`
+            }
+        }
+        return filePath ? filePath : undefined
+    }
+


Left this in here because it seems to comply with "some sanitization". Can move to validateProxySettings if there's too much clutter in here.

peterguy · 2024-10-04T00:41:59Z

vscode/src/extension.node.ts

-            return subscriptionDisposable(
-                resolvedConfig.subscribe(config => setCustomAgent(config.configuration))
-            )
+            return subscriptionDisposable(proxySettings.subscribe(setCustomAgent))


Need to use the validated proxy settings here, so need to use proxySettings.

peterguy · 2024-10-04T00:46:46Z

vscode/src/fetch.node.ts

-    const customAgent = setCustomAgent(getConfiguration())
+    const customAgent = setCustomAgent(
+        validateProxySettings({
+            configuration: getConfiguration(),
+            auth: {} as AuthCredentials,
+            clientState: {} as ClientState,
+        })
+    )


Had to manually validate the proxy settings, because I don't know how to get around using getConfiguration() here. initializeNetworkAgent is called when the extension is starting up, from vscode/src/extension.node.ts => activate(). Would like to use a subscription, but not sure how to do that, or if it can be done.

peterguy · 2024-10-04T00:49:07Z

vscode/src/main.ts

+
+    proxySettings.subscribe(setCustomAgent)
+


Is this how we envision using the config subscription? I'm not sure that getCustomAgent really conforms to the subscription approach, but I'm not familiar enough with either to make major changes.

proxySettings is exported from the new configuration-proxy.ts file because it's used in multiple places.

- add CA cert proxy setting to support self-signed certificates in the proxy - include keepalive and other settings in the new proxy agent

…uration reading tools already in place. - Rename the settings to match the Cody pattern and to conform to the camel case requirements.

…'s home directory.

… self-validating (to some extent) - memoize the verifying of the proxy path file and the reading of the proxy cacert file - memoize the check of the proxy host and port to cut down on repeated error messages to the user - remove logError on config errors because including it caused the code to be called many times over - combine the global agent CA list with the CA from the settings to ensure that the http(s) client doesn't lack required CA certs.

- move the proxy validation code into its own file - use a config subscriber to validate and disseminate the proxy settings - manually validate the proxy settings in initializeNetworkAgent because it's called when the extension starts up and uses getConfiguration() - run pnpm update-agent-recordings

…es when building for the browser.

peterguy · 2024-11-06T00:56:44Z

Moved to #5883

peterguy requested a review from RXminuS September 25, 2024 01:12

peterguy changed the title ~~Port proxy settings from Search extension~~ Enable extension-specific proxy settings Sep 25, 2024

peterguy self-assigned this Sep 25, 2024

peterguy marked this pull request as ready for review September 25, 2024 02:50

peterguy force-pushed the peterguy/vscode-add-proxy-settings branch from 3e39ab6 to 48ca172 Compare September 26, 2024 19:35

RXminuS reviewed Sep 26, 2024

View reviewed changes

peterguy force-pushed the peterguy/vscode-add-proxy-settings branch from a4331f3 to 9758ccf Compare October 2, 2024 01:16

peterguy commented Oct 4, 2024

View reviewed changes

peterguy added 11 commits October 4, 2024 07:46

port settings from search extension

3b5e0a6

- change proxy settings from "sourcegraph" to "cody"

f4485a4

- add CA cert proxy setting to support self-signed certificates in the proxy - include keepalive and other settings in the new proxy agent

- Move configuration reading into configuration.ts and use the config…

049cfba

…uration reading tools already in place. - Rename the settings to match the Cody pattern and to conform to the camel case requirements.

clean up error reporting

5ffe93e

mollify tests

b5d5274

refactor

ccdccc5

Tighten up the process to resolve paths that are relative to the user…

35c1822

…'s home directory.

Add logError messages

c9b1438

Add back in the combining of the CA certs

3214257

peterguy force-pushed the peterguy/vscode-add-proxy-settings branch from c3432cf to 3214257 Compare October 4, 2024 15:03

peterguy added 2 commits October 4, 2024 11:40

Move the proxy settings observable from main.ts because of build issu…

c67d159

…es when building for the browser.

Starting to override VSCode's request mechanism

39547f6

peterguy closed this Nov 6, 2024

peterguy deleted the peterguy/vscode-add-proxy-settings branch November 6, 2024 00:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable extension-specific proxy settings #5650

Enable extension-specific proxy settings #5650

peterguy commented Sep 20, 2024 •

edited

Loading

RXminuS Sep 26, 2024

peterguy Sep 26, 2024

RXminuS Sep 26, 2024

peterguy Sep 27, 2024

RXminuS Sep 26, 2024

peterguy Oct 2, 2024

RXminuS Sep 26, 2024

peterguy Oct 2, 2024

peterguy Oct 3, 2024 •

edited

Loading

RXminuS Sep 26, 2024

peterguy Oct 2, 2024

RXminuS Sep 26, 2024

peterguy Sep 26, 2024

peterguy Oct 3, 2024 •

edited

Loading

arafatkatze commented Sep 30, 2024

peterguy commented Oct 3, 2024

arafatkatze commented Oct 3, 2024

peterguy Oct 4, 2024

peterguy Oct 4, 2024

peterguy Oct 4, 2024

peterguy Oct 4, 2024

peterguy commented Nov 6, 2024

Enable extension-specific proxy settings #5650

Enable extension-specific proxy settings #5650

Conversation

peterguy commented Sep 20, 2024 • edited Loading

Test plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterguy Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterguy Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

arafatkatze commented Sep 30, 2024

peterguy commented Oct 3, 2024

arafatkatze commented Oct 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterguy commented Nov 6, 2024

peterguy commented Sep 20, 2024 •

edited

Loading

peterguy Oct 3, 2024 •

edited

Loading

peterguy Oct 3, 2024 •

edited

Loading