From 20db0f0d5a0198e63d30026e07de612d8785c55a Mon Sep 17 00:00:00 2001 From: Zach Kipp Date: Tue, 25 Nov 2025 23:56:19 -0700 Subject: [PATCH 1/4] fix(cli): don't default to keyring when --global-config set This fixes a regression that caused the VS code extension to be unable to authenticate after making keyring usage on by default. This is because the VS code extension assumes the CLI will always use the session token stored on disk, specifically in the directory specified by --global-config. This fix makes keyring usage enabled when the --global-config directory is not set. This is a bit wonky but necessary to allow the extension to continue working without modification and without backwards compat concerns. In the future we should modify these extensions to either access the credential in the keyring (like Coder Desktop) or some other approach that doesn't rely on the session token being stored on disk. --- cli/root.go | 16 ++++++++++++---- cli/testdata/coder_--help.golden | 6 +++--- docs/reference/cli/index.md | 2 +- enterprise/cli/testdata/coder_--help.golden | 6 +++--- 4 files changed, 19 insertions(+), 11 deletions(-) diff --git a/cli/root.go b/cli/root.go index 2ec035d64ab6d..9d1fcf2e13c7b 100644 --- a/cli/root.go +++ b/cli/root.go @@ -21,6 +21,7 @@ import ( "strings" "sync" "syscall" + "testing" "text/tabwriter" "time" @@ -483,9 +484,9 @@ func (r *RootCmd) Command(subcommands []*serpent.Command) (*serpent.Command, err Flag: varUseKeyring, Env: envUseKeyring, Description: "Store and retrieve session tokens using the operating system " + - "keyring. Enabled by default. If the keyring is not supported on the " + - "current platform, file-based storage is used automatically. Set to " + - "false to force file-based storage.", + "keyring. Enabled by default when --global-config is not set. If the " + + "keyring is not supported on the current platform, file-based storage is " + + "used automatically. Set to false to force file-based storage.", Default: "true", Value: serpent.BoolOf(&r.useKeyring), Group: globalGroup, @@ -721,8 +722,15 @@ func (r *RootCmd) createUnauthenticatedClient(ctx context.Context, serverURL *ur // flag. func (r *RootCmd) ensureTokenBackend() sessionstore.Backend { if r.tokenBackend == nil { + // Checking for the --global-config directory being set is a bit wonky but necessary + // to allow extensions that invoke the CLi with this flag (e.g. VS code) to continue + // working without modification. In the future we should modify these extensions to + // either access the credential in the keyring (like Coder Desktop) or some other + // approach that doesn't rely on the session token being stored on disk. We set the + // global config directory in most CLI tests, so we need to skip this check for tests. + assumeExtensionInUse := r.globalConfig != config.DefaultDir() && !testing.Testing() keyringSupported := runtime.GOOS == "windows" || runtime.GOOS == "darwin" - if r.useKeyring && keyringSupported { + if r.useKeyring && !assumeExtensionInUse && keyringSupported { serviceName := sessionstore.DefaultServiceName if r.keyringServiceName != "" { serviceName = r.keyringServiceName diff --git a/cli/testdata/coder_--help.golden b/cli/testdata/coder_--help.golden index 0057fdc15d226..8ddcc34cc6bdd 100644 --- a/cli/testdata/coder_--help.golden +++ b/cli/testdata/coder_--help.golden @@ -111,9 +111,9 @@ variables or flags. --use-keyring bool, $CODER_USE_KEYRING (default: true) Store and retrieve session tokens using the operating system keyring. - Enabled by default. If the keyring is not supported on the current - platform, file-based storage is used automatically. Set to false to - force file-based storage. + Enabled by default when --global-config is not set. If the keyring is + not supported on the current platform, file-based storage is used + automatically. Set to false to force file-based storage. -v, --verbose bool, $CODER_VERBOSE Enable verbose output. diff --git a/docs/reference/cli/index.md b/docs/reference/cli/index.md index e22e2deeeca82..2d3627112a90a 100644 --- a/docs/reference/cli/index.md +++ b/docs/reference/cli/index.md @@ -179,7 +179,7 @@ Disable network telemetry. Network telemetry is collected when connecting to wor | Environment | $CODER_USE_KEYRING | | Default | true | -Store and retrieve session tokens using the operating system keyring. Enabled by default. If the keyring is not supported on the current platform, file-based storage is used automatically. Set to false to force file-based storage. +Store and retrieve session tokens using the operating system keyring. Enabled by default when --global-config is not set. If the keyring is not supported on the current platform, file-based storage is used automatically. Set to false to force file-based storage. ### --global-config diff --git a/enterprise/cli/testdata/coder_--help.golden b/enterprise/cli/testdata/coder_--help.golden index 41ed3209687d8..ef171fcbd9cee 100644 --- a/enterprise/cli/testdata/coder_--help.golden +++ b/enterprise/cli/testdata/coder_--help.golden @@ -70,9 +70,9 @@ variables or flags. --use-keyring bool, $CODER_USE_KEYRING (default: true) Store and retrieve session tokens using the operating system keyring. - Enabled by default. If the keyring is not supported on the current - platform, file-based storage is used automatically. Set to false to - force file-based storage. + Enabled by default when --global-config is not set. If the keyring is + not supported on the current platform, file-based storage is used + automatically. Set to false to force file-based storage. -v, --verbose bool, $CODER_VERBOSE Enable verbose output. From 8c173bdc1d4c2f9a447e2d2880f31e9cd86195b5 Mon Sep 17 00:00:00 2001 From: Zach Kipp Date: Wed, 26 Nov 2025 00:18:52 -0700 Subject: [PATCH 2/4] fix: comment typo --- cli/root.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/root.go b/cli/root.go index 9d1fcf2e13c7b..7827dfa99f965 100644 --- a/cli/root.go +++ b/cli/root.go @@ -723,7 +723,7 @@ func (r *RootCmd) createUnauthenticatedClient(ctx context.Context, serverURL *ur func (r *RootCmd) ensureTokenBackend() sessionstore.Backend { if r.tokenBackend == nil { // Checking for the --global-config directory being set is a bit wonky but necessary - // to allow extensions that invoke the CLi with this flag (e.g. VS code) to continue + // to allow extensions that invoke the CLI with this flag (e.g. VS code) to continue // working without modification. In the future we should modify these extensions to // either access the credential in the keyring (like Coder Desktop) or some other // approach that doesn't rely on the session token being stored on disk. We set the From 785982daa8b4f973edc3f745ec043dc00f3c9442 Mon Sep 17 00:00:00 2001 From: Zach Kipp Date: Wed, 26 Nov 2025 00:36:59 -0700 Subject: [PATCH 3/4] clarify --use-keyring ignored when --global-config set --- cli/root.go | 6 +++--- cli/testdata/coder_--help.golden | 7 ++++--- docs/reference/cli/index.md | 2 +- enterprise/cli/testdata/coder_--help.golden | 7 ++++--- 4 files changed, 12 insertions(+), 10 deletions(-) diff --git a/cli/root.go b/cli/root.go index 7827dfa99f965..0bd2c964bb038 100644 --- a/cli/root.go +++ b/cli/root.go @@ -484,9 +484,9 @@ func (r *RootCmd) Command(subcommands []*serpent.Command) (*serpent.Command, err Flag: varUseKeyring, Env: envUseKeyring, Description: "Store and retrieve session tokens using the operating system " + - "keyring. Enabled by default when --global-config is not set. If the " + - "keyring is not supported on the current platform, file-based storage is " + - "used automatically. Set to false to force file-based storage.", + "keyring. This flag is ignored and file-based storage is used when " + + "--global-config is set or keyring usage is not supported on the current " + + "platform. Set to false to force file-based storage on supported platforms.", Default: "true", Value: serpent.BoolOf(&r.useKeyring), Group: globalGroup, diff --git a/cli/testdata/coder_--help.golden b/cli/testdata/coder_--help.golden index 8ddcc34cc6bdd..ab13e2af71e0f 100644 --- a/cli/testdata/coder_--help.golden +++ b/cli/testdata/coder_--help.golden @@ -111,9 +111,10 @@ variables or flags. --use-keyring bool, $CODER_USE_KEYRING (default: true) Store and retrieve session tokens using the operating system keyring. - Enabled by default when --global-config is not set. If the keyring is - not supported on the current platform, file-based storage is used - automatically. Set to false to force file-based storage. + This flag is ignored and file-based storage is used when + --global-config is set or keyring usage is not supported on the + current platform. Set to false to force file-based storage on + supported platforms. -v, --verbose bool, $CODER_VERBOSE Enable verbose output. diff --git a/docs/reference/cli/index.md b/docs/reference/cli/index.md index 2d3627112a90a..b26ec94a7f80d 100644 --- a/docs/reference/cli/index.md +++ b/docs/reference/cli/index.md @@ -179,7 +179,7 @@ Disable network telemetry. Network telemetry is collected when connecting to wor | Environment | $CODER_USE_KEYRING | | Default | true | -Store and retrieve session tokens using the operating system keyring. Enabled by default when --global-config is not set. If the keyring is not supported on the current platform, file-based storage is used automatically. Set to false to force file-based storage. +Store and retrieve session tokens using the operating system keyring. This flag is ignored and file-based storage is used when --global-config is set or keyring usage is not supported on the current platform. Set to false to force file-based storage on supported platforms. ### --global-config diff --git a/enterprise/cli/testdata/coder_--help.golden b/enterprise/cli/testdata/coder_--help.golden index ef171fcbd9cee..e199e8cc27d4d 100644 --- a/enterprise/cli/testdata/coder_--help.golden +++ b/enterprise/cli/testdata/coder_--help.golden @@ -70,9 +70,10 @@ variables or flags. --use-keyring bool, $CODER_USE_KEYRING (default: true) Store and retrieve session tokens using the operating system keyring. - Enabled by default when --global-config is not set. If the keyring is - not supported on the current platform, file-based storage is used - automatically. Set to false to force file-based storage. + This flag is ignored and file-based storage is used when + --global-config is set or keyring usage is not supported on the + current platform. Set to false to force file-based storage on + supported platforms. -v, --verbose bool, $CODER_VERBOSE Enable verbose output. From c21ebd23c9b5616208a923cf494859783811037d Mon Sep 17 00:00:00 2001 From: Zach Kipp Date: Wed, 26 Nov 2025 01:11:34 -0700 Subject: [PATCH 4/4] remove testing.Testing() usage and add test coverage --- cli/clitest/clitest.go | 8 ++++---- cli/keyring_test.go | 2 ++ cli/root.go | 24 +++++++++++++++--------- 3 files changed, 21 insertions(+), 13 deletions(-) diff --git a/cli/clitest/clitest.go b/cli/clitest/clitest.go index 20db312101814..3e506a26b6d59 100644 --- a/cli/clitest/clitest.go +++ b/cli/clitest/clitest.go @@ -61,10 +61,10 @@ func NewWithCommand( t testing.TB, cmd *serpent.Command, args ...string, ) (*serpent.Invocation, config.Root) { configDir := config.Root(t.TempDir()) - // Keyring usage is disabled here because many existing tests expect the session token - // to be stored on disk and is not properly instrumented for parallel testing against - // the actual operating system keyring. - invArgs := append([]string{"--global-config", string(configDir), "--use-keyring=false"}, args...) + // Keyring usage is disabled here when --global-config is set because many existing + // tests expect the session token to be stored on disk and is not properly instrumented + // for parallel testing against the actual operating system keyring. + invArgs := append([]string{"--global-config", string(configDir)}, args...) return setupInvocation(t, cmd, invArgs...), configDir } diff --git a/cli/keyring_test.go b/cli/keyring_test.go index 27b7c12d53cb0..7cb190845a31b 100644 --- a/cli/keyring_test.go +++ b/cli/keyring_test.go @@ -54,6 +54,7 @@ func setupKeyringTestEnv(t *testing.T, clientURL string, args ...string) keyring serviceName := keyringTestServiceName(t) root.WithKeyringServiceName(serviceName) + root.UseKeyringWithGlobalConfig() inv, cfg := clitest.NewWithDefaultKeyringCommand(t, cmd, args...) @@ -169,6 +170,7 @@ func TestUseKeyring(t *testing.T) { logoutCmd, err := logoutRoot.Command(logoutRoot.AGPL()) require.NoError(t, err) logoutRoot.WithKeyringServiceName(env.serviceName) + logoutRoot.UseKeyringWithGlobalConfig() logoutInv, _ := clitest.NewWithDefaultKeyringCommand(t, logoutCmd, "logout", diff --git a/cli/root.go b/cli/root.go index 0bd2c964bb038..7f0561e180ff2 100644 --- a/cli/root.go +++ b/cli/root.go @@ -21,7 +21,6 @@ import ( "strings" "sync" "syscall" - "testing" "text/tabwriter" "time" @@ -537,11 +536,12 @@ type RootCmd struct { disableDirect bool debugHTTP bool - disableNetworkTelemetry bool - noVersionCheck bool - noFeatureWarning bool - useKeyring bool - keyringServiceName string + disableNetworkTelemetry bool + noVersionCheck bool + noFeatureWarning bool + useKeyring bool + keyringServiceName string + useKeyringWithGlobalConfig bool } // InitClient creates and configures a new client with authentication, telemetry, @@ -726,9 +726,8 @@ func (r *RootCmd) ensureTokenBackend() sessionstore.Backend { // to allow extensions that invoke the CLI with this flag (e.g. VS code) to continue // working without modification. In the future we should modify these extensions to // either access the credential in the keyring (like Coder Desktop) or some other - // approach that doesn't rely on the session token being stored on disk. We set the - // global config directory in most CLI tests, so we need to skip this check for tests. - assumeExtensionInUse := r.globalConfig != config.DefaultDir() && !testing.Testing() + // approach that doesn't rely on the session token being stored on disk. + assumeExtensionInUse := r.globalConfig != config.DefaultDir() && !r.useKeyringWithGlobalConfig keyringSupported := runtime.GOOS == "windows" || runtime.GOOS == "darwin" if r.useKeyring && !assumeExtensionInUse && keyringSupported { serviceName := sessionstore.DefaultServiceName @@ -750,6 +749,13 @@ func (r *RootCmd) WithKeyringServiceName(serviceName string) { r.keyringServiceName = serviceName } +// UseKeyringWithGlobalConfig enables the use of the keyring storage backend +// when the --global-config directory is set. This is only intended as an override +// for tests, which require specifying the global config directory for test isolation. +func (r *RootCmd) UseKeyringWithGlobalConfig() { + r.useKeyringWithGlobalConfig = true +} + type AgentAuth struct { // Agent Client config agentToken string