CLI: replace keytar with custom solution #4750

olafurpg · 2024-07-02T12:24:21Z

Previously, we used Keytar to write/read/delete secrets. This was problematic for two reasons:

Keytar has been unmaintained since 2022
Keytar required native Node modules that meant cody-agent was crashing on macOS when using a release that was built on Linux (in CI)

This PR addresses both problems by replacing keytar with a custom solution that shells out to different secret managers (security on macOS, secret-tool on Linux, and powershell on
Windows). See comment in secrets.ts for a more detailed reasoning why
we went with this approach.

Test plan

Manually tested on macOS/Linux/Windows.

olafurpg · 2024-07-02T13:18:24Z

TODO: report helpful error messages when missing dependencies

Linux:

sudo apt install libsecret-tools
sudo apt install gnome-keyring

Windows with Admin Powershell

Install-Module -Name CredentialManager

eseliger · 2024-07-02T17:09:31Z

agent/src/cli/auth/secrets.ts

+class MacOSKeychain extends KeychainOperations {
+    async readSecret(): Promise<string> {
+        const { stdout } = await execAsync(
+            `security find-generic-password -s "${this.service()}" -a "${this.account.username}" -w`


this seems prone for command injection, is there some safe shellquoting package on NPM like Gos shellquote?

you could use the spawn APIs where you pass in the args array

@olafurpg This might be worth fixing, since we have security-sensitive customers, and I don't think this is a PLG-only feature.

eseliger · 2024-07-02T17:10:26Z

agent/src/cli/auth/secrets.ts

+
+    async writeSecret(secret: string): Promise<void> {
+        await execAsync(
+            `echo "${secret}" | secret-tool store --label="${this.service()}" service '${this.service()}' account ${


Is secret-tool expected to exist in most distros? I assume server-environments (VSCode remote SSH for example) won't have this?

secret-tool isn't usually installed, but the libsecret stuff via dbus is normally. I think as long as we are graceful here it should be fine. Personally for me I prefer not having tools like this, and rather manage it myself via envvars on my linux desktop. EG see this problem I had trying out cody-emacs where the agent wanted to store secrets, even though cody-emacs was responsible for the secret. sourcegraph/emacs-cody#30 (comment)

I was going to follow up separately to report helpful error messages if the dependencies are missing #4750 (comment)

@keegancsmith auth is an optional feature, you can still use environment variables or explicit --access-token to manage secrets. From personal experience, I find it incredibly convenient when tools like gh just work consistently on macOS/Windows/Linux regardless of how or where you started the terminal. Storing secrets as environment variables has lots of problems. For example, any program you run has access to SRC_ACCESS_TOKEN without doing any additional work. With a secret manager, you need to at least know the server endpoint and username to read the correct secret (which you can do by reading the plaintext user-settings.json file, but it's still a bit more effort).

My primary motivation for all this effort is to increase onboarding and retention. I want the cli tool to be super easy to get started with, and my personal experience is that I stick most easily with cli tools that manage secrets for me (example: gh)

I 100% agree, these tools should have a slick way to manage this stuff. But for example in automation and people who like to make like difficult like myself its nice to be able to have escape hatches. EG for emacs-cody I'd rather have auth be handled in the normal emacs way (if configured).

steveyegge

Quoting the shell args somehow does seem like a good thing to add. I won't stand in the way of merging, though, since keytar needs to die die die.

steveyegge · 2024-07-03T04:52:34Z

agent/src/cli/auth/secrets.ts

+class MacOSKeychain extends KeychainOperations {
+    async readSecret(): Promise<string> {
+        const { stdout } = await execAsync(
+            `security find-generic-password -s "${this.service()}" -a "${this.account.username}" -w`


@olafurpg This might be worth fixing, since we have security-sensitive customers, and I don't think this is a PLG-only feature.

github-actions · 2024-07-03T13:23:36Z

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

olafurpg · 2024-07-03T13:30:27Z

Thank you for the review! I've addressed the feedback on shell quoting, the solution is now using spawn instead of exec. I will submit a request in #discuss-security to additionally review this functionality after the release is out (we won't make public noise about this feature until after security review is done)

olafurpg · 2024-07-03T13:52:42Z

CI failures are unrelated, posted about it here https://sourcegraph.slack.com/archives/C05AGQYD528/p1720014583901319

eseliger

One remaining question about command injection in the powershell scripts, otherwise LGTM

eseliger · 2024-07-03T14:17:46Z

agent/src/cli/auth/secrets.ts

+class MacOSKeychain extends KeychainOperations {
+    installationInstructions = ''
+    async readSecret(): Promise<string> {
+        return await this.spawnAsync('security', [


much nicer than a shell script, thanks!

Thank you for raising the concern! This is much cleaner and easier to reason about and maintain

I would probably have used spawn in the first iteration, it was using exec because that's what Cody generated on the first shot 😅

eseliger · 2024-07-03T14:19:18Z

agent/src/cli/auth/secrets.ts

+        return `${this.service()}:${this.account.username}`
+    }
+    async readSecret(): Promise<string> {
+        const powershellCommand = `(Get-StoredCredential -Target "${this.target()}").GetNetworkCredential().Password`


this is prone to injection I believe, as it's using string concatenation again

I pushed a change to escape all " in the target. This is a low-risk injection vector because the target is a combination of the server endpoint and username. Still, worth escaping out of principle

eseliger · 2024-07-03T14:22:14Z

agent/src/cli/auth/secrets.ts

    try {
-        await keytar.deletePassword(codyServiceName, account)
+        execSync(`which ${command}`, { stdio: 'ignore' })


Non-blocking but might be nice to reduce the dependency on a shell that has which - https://github.com/otiai10/lookpath/blob/main/src/index.ts seems to be an implementation of Gos exec.LookPath for node.

I thought about it, but we only do this on Linux so it's not a problem. On macOS, security is pre-installed and on Windows the user needs to install CredentialManager with the pre-installed powershell.

pkukielka · 2024-07-03T14:24:40Z

agent/src/cli/auth/secrets.ts

+// modules for each supported OS. The current implementation shells out to the
+// `security` command on macOS, `CredentialManager` on Windows, and `secret-tool` on
+// Linux. This is an o)ptional feature. Users can always avoid using this functionality
+// by setting the `CODY_ACCESS_TOKEN` environment variable or passing the `--access-token`


Cody clients should also be able to pass the token as part of the initialize call.
It was semi-broken though, I was fixing it today by the way of looking tat the telemetry issue:
https://github.com/sourcegraph/cody/pull/4766/files#diff-85c89382076aeb49c8645dd41e6422552fba7bbf25575701c1a9253027164a76

NICE! I've had a gut feeling we are doing some broken things like that. Glad we are fixing it <3

pkukielka

LGTM

Previously, we used Keytar to write/read/delete secrets. This was problematic for two reasons: - Keytar has been unmaintained since 2022 - Keytar required native Node modules that meant `cody-agent` was crashing on macOS when using a release that was built on Linux (in CI) This PR addresses both problems by replacing keytar with a custom solution that shells out to different secret managers (`security` on macOS, `secret-tool` on Linux, and `powershell` on Windows). See comment in `secrets.ts` for a more detailed reasoning why we went with this approach.

Only tested on macOS for now, will test on Linux/Windows once I push the commit.

olafurpg · 2024-07-03T14:35:04Z

Rebase on top of main to fix CI #4768

olafurpg · 2024-07-03T14:43:19Z

Thank you everybody for the review! 🙇🏻 I will cut a release and get a second round of review from the security team CODY-2738 : Complete security review on Cody cli auth secret management

olafurpg force-pushed the olafurpg-cody-2708-fix-npm-release-again branch 2 times, most recently from ee83417 to bb81f51 Compare July 2, 2024 13:16

olafurpg requested review from a team July 2, 2024 17:06

eseliger reviewed Jul 2, 2024

View reviewed changes

steveyegge reviewed Jul 3, 2024

View reviewed changes

olafurpg force-pushed the olafurpg-cody-2708-fix-npm-release-again branch from 547e48d to a918dde Compare July 3, 2024 13:28

olafurpg requested review from keegancsmith, steveyegge and eseliger July 3, 2024 13:52

eseliger reviewed Jul 3, 2024

View reviewed changes

pkukielka reviewed Jul 3, 2024

View reviewed changes

pkukielka approved these changes Jul 3, 2024

View reviewed changes

olafurpg force-pushed the olafurpg-cody-2708-fix-npm-release-again branch from afafd1e to 21a5b26 Compare July 3, 2024 14:31

olafurpg added 10 commits July 3, 2024 16:31

Get this working on Windows

30a493b

Fix implementation on Linux

7c69ebf

Address review feedback.

1e25879

Only tested on macOS for now, will test on Linux/Windows once I push the commit.

Fix credentials in Linux + error handling

379b6ba

Fix build error

adfc18a

Fix Windows, again, with nicer instructions this time

fad3e91

Minor code cleanup

37a8fbc

Clarify comment on custom secret storage solutio

a08d0ec

Avoid injection

aa27404

olafurpg force-pushed the olafurpg-cody-2708-fix-npm-release-again branch from 21a5b26 to aa27404 Compare July 3, 2024 14:32

olafurpg merged commit d6e931a into main Jul 3, 2024
19 checks passed

olafurpg deleted the olafurpg-cody-2708-fix-npm-release-again branch July 3, 2024 14:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLI: replace keytar with custom solution #4750

CLI: replace keytar with custom solution #4750

olafurpg commented Jul 2, 2024

olafurpg commented Jul 2, 2024

eseliger Jul 2, 2024

keegancsmith Jul 2, 2024

steveyegge Jul 3, 2024

eseliger Jul 2, 2024

keegancsmith Jul 2, 2024

olafurpg Jul 3, 2024

olafurpg Jul 3, 2024

olafurpg Jul 3, 2024

keegancsmith Jul 5, 2024

steveyegge left a comment

steveyegge Jul 3, 2024

github-actions bot commented Jul 3, 2024

olafurpg commented Jul 3, 2024

olafurpg commented Jul 3, 2024

eseliger left a comment

eseliger Jul 3, 2024

olafurpg Jul 3, 2024

olafurpg Jul 3, 2024 •

edited

Loading

eseliger Jul 3, 2024

olafurpg Jul 3, 2024

eseliger Jul 3, 2024

olafurpg Jul 3, 2024

pkukielka Jul 3, 2024

olafurpg Jul 3, 2024

pkukielka left a comment

olafurpg commented Jul 3, 2024

olafurpg commented Jul 3, 2024

CLI: replace keytar with custom solution #4750

CLI: replace keytar with custom solution #4750

Conversation

olafurpg commented Jul 2, 2024

Test plan

olafurpg commented Jul 2, 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

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

steveyegge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 3, 2024

olafurpg commented Jul 3, 2024

olafurpg commented Jul 3, 2024

eseliger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olafurpg Jul 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkukielka left a comment

Choose a reason for hiding this comment

olafurpg commented Jul 3, 2024

olafurpg commented Jul 3, 2024

olafurpg Jul 3, 2024 •

edited

Loading