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.

Sign up for GitHub

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

Jump to bottom

Windows: make win-ca also work with the agent #4618

Merged
merged 1 commit into from
Jun 19, 2024
Merged

Windows: make win-ca also work with the agent #4618

merged 1 commit into from
Jun 19, 2024

Conversation

olafurpg
Copy link
Member

@olafurpg olafurpg commented Jun 19, 2024 •
edited
Loading

In the PR #4598, we fixed a bug related to how we installed self-signed certificates on Windows. This PR only fixed the issue for VS Code. This PR additionally fixes the problem for the agent. For the agent, we use a slightly different extensionUri. This PR adds logic to manually copy the roots.exe file over to the directory we use as extensionUri with the agent. The benefit of solving this problem like this is that it works automatically for all agent clients, avoiding the need to do custom solutions for individual clients like JetBrains.

Test plan

Manually confirmed that we are no longer printing out an error when running the tests on Windows.

@olafurpg olafurpg requested a review from a team June 19, 2024 08:20
@github-actions GitHub Actions
Copy link

‼️ Hey @sourcegraph/cody-security, please review this PR carefully as it introduces the usage of an unsafe_ function or abuses PromptString.

@olafurpg olafurpg force-pushed the olafurpg/cody-2467-make-requirewin-caexe-work-on-jetbrains branch 2 times, most recently from 7f61d0a to 1b1642e Compare June 19, 2024 08:23
@olafurpg
Windows: make win-ca also work with the agent
7a0cce6 
In the PR #4598, we fixed a bug
related to how we installed self-signed certificates on Windows. This PR
only fixed the issue for VS Code. This PR additionally fixes the problem
for the agent. For the agent, we use a slightly different
`extensionUri`. This PR adds logic to manually copy the `roots.exe` file
over to the directory we use as `extensionUri` with the agent.
@olafurpg olafurpg force-pushed the olafurpg/cody-2467-make-requirewin-caexe-work-on-jetbrains branch from 1b1642e to 7a0cce6 Compare June 19, 2024 08:26
@olafurpg olafurpg enabled auto-merge (squash) June 19, 2024 08:26
olafurpg
olafurpg commented Jun 19, 2024
View reviewed changes
vscode/scripts/copy-win-ca-roots.ts
import { copyFileSync } from 'node:fs'
import path from 'node:path'

copyFileSync(
Copy link
Member Author

Choose a reason for hiding this comment

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

Written as a script so that it works consistently on macOS/Windows

@olafurpg
Copy link
Member Author

From the unit test logs on Windows in the main branch, we see the error related to installing certificates https://github.com/sourcegraph/cody/actions/runs/9578259706/job/26408138136

Error installing Windows certs <ref *1> Error: spawnSync c:\Users\runneradmin\AppData\Roaming\Cody-nodejs\Config\dist\win-ca-roots.exe ENOENT
    at Object.spawnSync (node:internal/child_process:1124:20)
    at spawnSync (node:child_process:876:24)
    at Object.execFileSync (node:child_process:919:15)
    at Object.run2 (D:\a\cody\cody\node_modules\.pnpm\win-ca@3.5.1\node_modules\win-ca\lib\fallback.js:46:26)
    at api (D:\a\cody\cody\node_modules\.pnpm\win-ca@3.5.1\node_modules\win-ca\lib\index.js:60:10)
    at registerLocalCertificates (D:\a\cody\cody\vscode\src\certs.js:[35](https://github.com/sourcegraph/cody/actions/runs/9578259706/job/26408138136#step:9:36):9)
    at initializeNetworkAgent (D:\a\cody\cody\vscode\src\fetch.node.ts:97:5)

The logs for this PR no longer show this error https://github.com/sourcegraph/cody/actions/runs/9578695458/job/26409501979?pr=4618#step:9:31

pkukielka
pkukielka reviewed Jun 19, 2024
View reviewed changes
agent/src/agent.ts
export async function initializeVscodeExtension(
workspaceRoot: vscode.Uri,
extensionActivate: ExtensionActivate,
extensionClient: ExtensionClient
): Promise<void> {
const paths = envPaths('Cody')
const extensionPath = paths.config
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why instead we do not simply set extensionPath to __dirname?
It seems then we could completely avoid having copyWinCaRootsBinary, and it would be more correct also as that is path where extension files exists from the agent perspective, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not opposed to that change but we should make it separate from this PR. If we do this, then we should consider cleaning up the old directory, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

we do all sorts of stuff in this directory, like downloading symf/bfg, etc. It's a bigger scope change than what I have appetite to deal with right now

pkukielka
pkukielka approved these changes Jun 19, 2024
View reviewed changes
Copy link
Contributor

@pkukielka pkukielka left a comment

Choose a reason for hiding this comment

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

LGTM

@olafurpg olafurpg merged commit 62ac4e8 into main Jun 19, 2024
19 checks passed
@olafurpg olafurpg deleted the olafurpg/cody-2467-make-requirewin-caexe-work-on-jetbrains branch June 19, 2024 08:46
@pkukielka pkukielka mentioned this pull request Jun 19, 2024
Merged
pkukielka added a commit to sourcegraph/jetbrains that referenced this pull request Jun 20, 2024
@pkukielka
Make sure win-ca-roots.exe is included in the plugin build (#1792)
f48f089 
## Changes

Follow up to the sourcegraph/cody#4618

## Test plan

```
➜ CODY_DIR=/Users/pkukielka/Work/sourcegraph/cody ./gradlew :buildPlugin

➜ unzip -l build/distributions/Sourcegraph-6.0-localbuild.zip | grep exe
 69252760  06-19-2024 16:09   Sourcegraph/agent/node-win-x64.exe
    82944  06-19-2024 16:09   Sourcegraph/agent/win-ca-roots.exe
```
steveyegge pushed a commit to sourcegraph/jetbrains that referenced this pull request Jun 25, 2024
@pkukielka @steveyegge
Make sure win-ca-roots.exe is included in the plugin build (#1792)
5fe4166 
## Changes

Follow up to the sourcegraph/cody#4618

## Test plan

```
➜ CODY_DIR=/Users/pkukielka/Work/sourcegraph/cody ./gradlew :buildPlugin

➜ unzip -l build/distributions/Sourcegraph-6.0-localbuild.zip | grep exe
 69252760  06-19-2024 16:09   Sourcegraph/agent/node-win-x64.exe
    82944  06-19-2024 16:09   Sourcegraph/agent/win-ca-roots.exe
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pkukielka pkukielka pkukielka approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@olafurpg @pkukielka