From 855a6599d6201b42be3553606ab8b5cfdea2de94 Mon Sep 17 00:00:00 2001 From: Bill Sager Date: Fri, 31 Jan 2025 10:56:26 -0800 Subject: [PATCH 1/2] Add VSCode extension configuration to disable KeyChain storage for credentials --- .vscode/launch.json | 2 +- cmd/publisher/commands/ui.go | 8 +++-- extensions/vscode/package.json | 5 +++ extensions/vscode/src/commands.ts | 12 ++++++- extensions/vscode/src/extension.ts | 14 ++++++-- extensions/vscode/src/servers.ts | 11 ++++-- extensions/vscode/src/services.ts | 5 ++- internal/credentials/credentials.go | 17 ++++++--- internal/credentials/credentials_test.go | 38 ++++++++++++++++++++- internal/logging/loggingtest/mock_logger.go | 5 +++ 10 files changed, 103 insertions(+), 14 deletions(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index 451731d4b..900317bde 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -10,7 +10,7 @@ "request": "launch", "mode": "auto", "program": "${workspaceFolder}/cmd/publisher/main.go", - "args": ["ui", "--listen=localhost:9001", "-vv"], + "args": ["ui", "--listen=localhost:9001", "-vv", "--use-keychain=false"], "cwd": "${workspaceFolder}/test/sample-content" } ] diff --git a/cmd/publisher/commands/ui.go b/cmd/publisher/commands/ui.go index 129bb1d89..816d76b39 100644 --- a/cmd/publisher/commands/ui.go +++ b/cmd/publisher/commands/ui.go @@ -4,6 +4,7 @@ package commands import ( "github.com/posit-dev/publisher/internal/cli_types" + "github.com/posit-dev/publisher/internal/credentials" "github.com/posit-dev/publisher/internal/events" "github.com/posit-dev/publisher/internal/services/api" "github.com/posit-dev/publisher/internal/util" @@ -11,8 +12,9 @@ import ( ) type UICmd struct { - Path util.Path `help:"Sets the current working directory for the agent." arg:"" default:"."` - Listen string `help:"Network address to listen on." placeholder:"HOST[:PORT]" default:"localhost:0"` + Path util.Path `help:"Sets the current working directory for the agent." arg:"" default:"."` + Listen string `help:"Network address to listen on." placeholder:"HOST[:PORT]" default:"localhost:0"` + UseKeychain bool `help:"Use Keychain services to store/manage credentials." default:"true"` } func (cmd *UICmd) Run(args *cli_types.CommonArgs, ctx *cli_types.CLIContext) error { @@ -31,6 +33,8 @@ func (cmd *UICmd) Run(args *cli_types.CommonArgs, ctx *cli_types.CLIContext) err return err } + credentials.UseKeychain = cmd.UseKeychain + // Auto-initialize if needed. This will be replaced by an API call from the UI // for better error handling and startup performance. svc := api.NewService( diff --git a/extensions/vscode/package.json b/extensions/vscode/package.json index 2960d80f0..de5d731b8 100644 --- a/extensions/vscode/package.json +++ b/extensions/vscode/package.json @@ -41,6 +41,11 @@ "markdownDescription": "Verify TLS certificates for connections. Only **disable** this setting if you experience certificate verification errors and the Posit Connect server is using a self-signed, expired, or otherwise misconfigured certificate.", "type": "boolean", "default": true + }, + "positPublisher.useKeyChainCredentialStorage": { + "markdownDescription": "Enable Key Chain storage when retrieving and storing credentials. On systems which do not support a Key Chain facility, this configuration option is ignored. When the Key Chain facility is not used, a YAML file is used instead. (`~/.connect_credentials`). After changing this value, you must restart the extension to have the setting go into effect.", + "type": "boolean", + "default": true } } } diff --git a/extensions/vscode/src/commands.ts b/extensions/vscode/src/commands.ts index a37777ae9..95b39c502 100644 --- a/extensions/vscode/src/commands.ts +++ b/extensions/vscode/src/commands.ts @@ -11,10 +11,20 @@ export const create = async ( context: ExtensionContext, path: string, port: number, + useKeyChain: boolean, subcommand: string = "ui", ): Promise<[string, string[]]> => { const executable = await getExecutableBinary(context); - return [executable, [subcommand, "-vv", `--listen=${HOST}:${port}`, path]]; + return [ + executable, + [ + subcommand, + "-vv", + `--listen=${HOST}:${port}`, + `--use-keychain=${useKeyChain}`, + path, + ], + ]; }; const getExecutableBinary = (context: ExtensionContext): string => { diff --git a/extensions/vscode/src/extension.ts b/extensions/vscode/src/extension.ts index 412d444a7..661d61481 100644 --- a/extensions/vscode/src/extension.ts +++ b/extensions/vscode/src/extension.ts @@ -66,7 +66,9 @@ export async function activate(context: ExtensionContext) { const stream = new EventStream(port); context.subscriptions.push(stream); - service = new Service(context, port, useExternalAgent); + const useKeyChain = extensionSettings.useKeyChainCredentialStorage(); + + service = new Service(context, port, useExternalAgent, useKeyChain); service.start(); const watchers = new WatcherManager(); @@ -119,10 +121,18 @@ export async function deactivate() { export const extensionSettings = { verifyCertificates(): boolean { - // set value from extension configuration - defaults to true + // get value from extension configuration - defaults to true const configuration = workspace.getConfiguration("positPublisher"); const value: boolean | undefined = configuration.get("verifyCertificates"); return value !== undefined ? value : true; }, + useKeyChainCredentialStorage(): boolean { + // get value from extension configuration - defaults to true + const configuration = workspace.getConfiguration("positPublisher"); + const value: boolean | undefined = configuration.get( + "useKeyChainCredentialStorage", + ); + return value !== undefined ? value : true; + }, }; diff --git a/extensions/vscode/src/servers.ts b/extensions/vscode/src/servers.ts index d4144021b..c012e1a3a 100644 --- a/extensions/vscode/src/servers.ts +++ b/extensions/vscode/src/servers.ts @@ -13,12 +13,14 @@ import * as workspaces from "src/workspaces"; export class Server implements Disposable { readonly port: number; readonly outputChannel: OutputChannel; + readonly useKeyChain: boolean; process: ChildProcessWithoutNullStreams | undefined = undefined; - constructor(port: number) { + constructor(port: number, useKeyChain: boolean) { this.port = port; this.outputChannel = window.createOutputChannel(`Posit Publisher`); + this.useKeyChain = useKeyChain; } /** @@ -38,7 +40,12 @@ export class Server implements Disposable { // todo - make this configurable const path = workspaces.path(); // Create command to send to terminal stdin - const [command, args] = await commands.create(context, path!, this.port); + const [command, args] = await commands.create( + context, + path!, + this.port, + this.useKeyChain, + ); // Spawn child process this.process = spawn(command, args); // Handle error output diff --git a/extensions/vscode/src/services.ts b/extensions/vscode/src/services.ts index 2942a0140..86252ab18 100644 --- a/extensions/vscode/src/services.ts +++ b/extensions/vscode/src/services.ts @@ -11,17 +11,20 @@ export class Service implements Disposable { private server: Server; private agentURL: string; private useExternalAgent: boolean; + private useKeyChain: boolean; constructor( context: ExtensionContext, port: number, useExternalAgent: boolean, + useKeyChain: boolean, ) { this.context = context; this.useExternalAgent = useExternalAgent; + this.useKeyChain = useKeyChain; this.agentURL = `http://${HOST}:${port}/api`; - this.server = new Server(port); + this.server = new Server(port, this.useKeyChain); initApi(this.isUp(), this.agentURL); } diff --git a/internal/credentials/credentials.go b/internal/credentials/credentials.go index 164f1c690..592fd1a01 100644 --- a/internal/credentials/credentials.go +++ b/internal/credentials/credentials.go @@ -66,6 +66,8 @@ type CredentialRecord struct { type CredentialTable = map[string]CredentialRecord +var UseKeychain bool = true + // ToCredential converts a CredentialRecord to a Credential based on its version. func (cr *CredentialRecord) ToCredential() (*Credential, error) { switch cr.Version { @@ -93,12 +95,19 @@ type CredentialsService interface { // The main credentials service constructor that determines if the system's keyring is available to be used, // if not, returns a file based credentials service. func NewCredentialsService(log logging.Logger) (CredentialsService, error) { - krService := NewKeyringCredentialsService(log) - if krService.IsSupported() { - return krService, nil + var fcService *fileCredentialsService = nil + + if UseKeychain { + krService := NewKeyringCredentialsService(log) + if krService.IsSupported() { + log.Debug("Using keychain for credential storage.") + return krService, nil + } + log.Debug("Fallback to file managed credentials service due to unavailable system keyring.") + } else { + log.Debug("Configuration has disabled keychain credentials. Using file managed credentials instead.") } - log.Debug("Fallback to file managed credentials service due to unavailable system keyring") fcService, err := NewFileCredentialsService(log) if err != nil { return fcService, err diff --git a/internal/credentials/credentials_test.go b/internal/credentials/credentials_test.go index 6e35e68f9..2d778895d 100644 --- a/internal/credentials/credentials_test.go +++ b/internal/credentials/credentials_test.go @@ -103,6 +103,11 @@ func (s *CredentialsServiceTestSuite) TestCredentialRecord_VersionErr() { func (s *CredentialsServiceTestSuite) TestNewCredentialsService_KeyringOK() { keyring.MockInit() + + // Don't set credentials.UseKeychain to any value, let it default + + s.log.On("Debug", "Using keychain for credential storage.").Return() + credservice, err := NewCredentialsService(s.log) s.NoError(err) s.Implements((*CredentialsService)(nil), credservice) @@ -118,7 +123,38 @@ func (s *CredentialsServiceTestSuite) TestNewCredentialsService_KeyringErrFallba keyring.MockInitWithError(keyringErr) s.log.On("Debug", "System keyring service is not available", "error", "failed to load credentials: this is a teapot, unsupported system").Return() - s.log.On("Debug", "Fallback to file managed credentials service due to unavailable system keyring").Return() + s.log.On("Debug", "Fallback to file managed credentials service due to unavailable system keyring.").Return() + + credservice, err := NewCredentialsService(s.log) + s.NoError(err) + s.Implements((*CredentialsService)(nil), credservice) +} + +func (s *CredentialsServiceTestSuite) TestNewCredentialsService_WithUseKeyringTrue() { + keyring.MockInit() + + // set credentials.UseKeychain to true + UseKeychain = true + + s.log.On("Debug", "Using keychain for credential storage.").Return() + + credservice, err := NewCredentialsService(s.log) + s.NoError(err) + s.Implements((*CredentialsService)(nil), credservice) +} + +func (s *CredentialsServiceTestSuite) TestNewCredentialsService_WithUseKeyringFalse() { + // Use an in memory filesystem for this test + // avoiding to manipulate users ~/.connect-credentials + fsys = afero.NewMemMapFs() + defer func() { fsys = afero.NewOsFs() }() + + keyring.MockInit() + + s.log.On("Debug", "Configuration has disabled keychain credentials. Using file managed credentials instead.").Return() + + // set credentials.UseKeychain to true + UseKeychain = false credservice, err := NewCredentialsService(s.log) s.NoError(err) diff --git a/internal/logging/loggingtest/mock_logger.go b/internal/logging/loggingtest/mock_logger.go index 17eae6ad4..8a3db436a 100644 --- a/internal/logging/loggingtest/mock_logger.go +++ b/internal/logging/loggingtest/mock_logger.go @@ -42,3 +42,8 @@ func (m *MockLogger) WithArgs(args ...any) logging.Logger { mockArgs := m.Called(args...) return mockArgs.Get(0).(logging.Logger) } + +func (m *MockLogger) Debug(msg string, args ...any) { + mockArgs := append([]any{msg}, args...) + m.Called(mockArgs...) +} From d495cdcaca3348200cba59ff9ffc93600ae97911 Mon Sep 17 00:00:00 2001 From: Bill Sager Date: Mon, 10 Feb 2025 08:33:50 -0800 Subject: [PATCH 2/2] Update extensions/vscode/package.json Co-authored-by: Jordan Jensen --- extensions/vscode/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/vscode/package.json b/extensions/vscode/package.json index de5d731b8..200f55d47 100644 --- a/extensions/vscode/package.json +++ b/extensions/vscode/package.json @@ -43,7 +43,7 @@ "default": true }, "positPublisher.useKeyChainCredentialStorage": { - "markdownDescription": "Enable Key Chain storage when retrieving and storing credentials. On systems which do not support a Key Chain facility, this configuration option is ignored. When the Key Chain facility is not used, a YAML file is used instead. (`~/.connect_credentials`). After changing this value, you must restart the extension to have the setting go into effect.", + "markdownDescription": "Controls whether Posit Publisher will use the system keychain to store credentials. When disabled, or on a system without a keychain, `~/.connect-credentials` is used. After changing this setting, you must restart VS Code for it to go into effect.", "type": "boolean", "default": true }