From dc96f84d6551dd1faec759ef214c3cb48956c877 Mon Sep 17 00:00:00 2001 From: Zach Kipp Date: Thu, 30 Oct 2025 14:21:41 -0600 Subject: [PATCH 1/2] feat(cli): add macOS support for session token keyring storage Add support for storing the CLI session token in the OS keyring on macOS when the --use-keyring flag is provided. --- cli/keyring_test.go | 6 +- cli/sessionstore/sessionstore_darwin.go | 109 ++++++++++++++++++ cli/sessionstore/sessionstore_darwin_test.go | 34 ++++++ cli/sessionstore/sessionstore_other.go | 2 +- cli/sessionstore/sessionstore_other_test.go | 10 ++ cli/sessionstore/sessionstore_test.go | 70 ++++++++++- cli/sessionstore/sessionstore_windows_test.go | 75 ++---------- 7 files changed, 236 insertions(+), 70 deletions(-) create mode 100644 cli/sessionstore/sessionstore_darwin.go create mode 100644 cli/sessionstore/sessionstore_darwin_test.go create mode 100644 cli/sessionstore/sessionstore_other_test.go diff --git a/cli/keyring_test.go b/cli/keyring_test.go index 1a4e90895a6eb..646f4ae19a034 100644 --- a/cli/keyring_test.go +++ b/cli/keyring_test.go @@ -282,9 +282,9 @@ func TestUseKeyringUnsupportedOS(t *testing.T) { // a helpful error message. t.Parallel() - // Skip on Windows since the keyring is actually supported. - if runtime.GOOS == "windows" { - t.Skip("Skipping unsupported OS test on Windows where keyring is supported") + // Only run this on an unsupported OS. + if runtime.GOOS == "windows" || runtime.GOOS == "darwin" { + t.Skipf("Skipping unsupported OS test on %s where keyring is supported", runtime.GOOS) } const expMessage = "keyring storage is not supported on this operating system; remove the --use-keyring flag" diff --git a/cli/sessionstore/sessionstore_darwin.go b/cli/sessionstore/sessionstore_darwin.go new file mode 100644 index 0000000000000..6caf3065ea189 --- /dev/null +++ b/cli/sessionstore/sessionstore_darwin.go @@ -0,0 +1,109 @@ +//go:build darwin + +package sessionstore + +import ( + "encoding/base64" + "fmt" + "io" + "os" + "os/exec" + "regexp" + "strings" +) + +const ( + // defaultServiceName is the service name used in the macOS Keychain + // for storing Coder CLI session tokens. + defaultServiceName = "coder-v2-credentials" + + // fixedUsername is the fixed username used for all keychain entries. + // Since our interface only uses service names, we use a constant username. + fixedUsername = "coder-session-token" + + execPathKeychain = "/usr/bin/security" + notFoundStr = "could not be found" +) + +// operatingSystemKeyring implements keyringProvider for macOS. +// It is largely adapted from the zalando/go-keyring package. +type operatingSystemKeyring struct{} + +func (operatingSystemKeyring) Set(service, credential string) error { + // if the added secret has multiple lines or some non ascii, + // macOS will hex encode it on return. To avoid getting garbage, we + // encode all passwords + password := base64.StdEncoding.EncodeToString([]byte(credential)) + + cmd := exec.Command(execPathKeychain, "-i") + stdIn, err := cmd.StdinPipe() + if err != nil { + return err + } + + if err = cmd.Start(); err != nil { + return err + } + + command := fmt.Sprintf("add-generic-password -U -s %s -a %s -w %s\n", + shellEscape(service), + shellEscape(fixedUsername), + shellEscape(password)) + if len(command) > 4096 { + return ErrSetDataTooBig + } + + if _, err := io.WriteString(stdIn, command); err != nil { + return err + } + + if err = stdIn.Close(); err != nil { + return err + } + + return cmd.Wait() +} + +func (operatingSystemKeyring) Get(service string) ([]byte, error) { + out, err := exec.Command( + execPathKeychain, + "find-generic-password", + "-s", service, + "-wa", fixedUsername).CombinedOutput() + if err != nil { + if strings.Contains(string(out), notFoundStr) { + return nil, os.ErrNotExist + } + return nil, err + } + + trimStr := strings.TrimSpace(string(out)) + return base64.StdEncoding.DecodeString(trimStr) +} + +func (operatingSystemKeyring) Delete(service string) error { + out, err := exec.Command( + execPathKeychain, + "delete-generic-password", + "-s", service, + "-a", fixedUsername).CombinedOutput() + if strings.Contains(string(out), notFoundStr) { + return os.ErrNotExist + } + return err +} + +// shellEscape returns a shell-escaped version of the string s. +// This is adapted from github.com/zalando/go-keyring/internal/shellescape. +func shellEscape(s string) string { + if len(s) == 0 { + return "''" + } + + pattern := regexp.MustCompile(`[^\w@%+=:,./-]`) + if pattern.MatchString(s) { + return "'" + strings.ReplaceAll(s, "'", "'\"'\"'") + "'" + } + + return s +} diff --git a/cli/sessionstore/sessionstore_darwin_test.go b/cli/sessionstore/sessionstore_darwin_test.go new file mode 100644 index 0000000000000..029028fa77879 --- /dev/null +++ b/cli/sessionstore/sessionstore_darwin_test.go @@ -0,0 +1,34 @@ +//go:build darwin + +package sessionstore_test + +import ( + "encoding/base64" + "os/exec" + "testing" +) + +const ( + execPathKeychain = "/usr/bin/security" + fixedUsername = "coder-session-token" +) + +func readRawKeychainCredential(t *testing.T, service string) []byte { + t.Helper() + + out, err := exec.Command( + execPathKeychain, + "find-generic-password", + "-s", service, + "-wa", fixedUsername).CombinedOutput() + if err != nil { + t.Fatal(err) + } + + dst := make([]byte, base64.StdEncoding.DecodedLen(len(out))) + n, err := base64.StdEncoding.Decode(dst, out) + if err != nil { + t.Fatal(err) + } + return dst[:n] +} diff --git a/cli/sessionstore/sessionstore_other.go b/cli/sessionstore/sessionstore_other.go index d930790135aca..e2e41ba536f02 100644 --- a/cli/sessionstore/sessionstore_other.go +++ b/cli/sessionstore/sessionstore_other.go @@ -1,4 +1,4 @@ -//go:build !windows +//go:build !windows && !darwin package sessionstore diff --git a/cli/sessionstore/sessionstore_other_test.go b/cli/sessionstore/sessionstore_other_test.go new file mode 100644 index 0000000000000..b924a95d12897 --- /dev/null +++ b/cli/sessionstore/sessionstore_other_test.go @@ -0,0 +1,10 @@ +//go:build !windows && !darwin + +package sessionstore_test + +import "testing" + +func readRawKeychainCredential(t *testing.T, _ string) []byte { + t.Fatal("not implemented") + return nil +} diff --git a/cli/sessionstore/sessionstore_test.go b/cli/sessionstore/sessionstore_test.go index c01a1256eb085..1ecb0279918fd 100644 --- a/cli/sessionstore/sessionstore_test.go +++ b/cli/sessionstore/sessionstore_test.go @@ -1,6 +1,7 @@ package sessionstore_test import ( + "encoding/json" "errors" "fmt" "net/url" @@ -16,6 +17,11 @@ import ( "github.com/coder/coder/v2/cli/sessionstore" ) +type storedCredentials map[string]struct { + CoderURL string `json:"coder_url"` + APIToken string `json:"api_token"` +} + // Generate a test service name for use with the OS keyring. It uses a combination // of the test name and a nanosecond timestamp to prevent collisions. func keyringTestServiceName(t *testing.T) string { @@ -26,8 +32,8 @@ func keyringTestServiceName(t *testing.T) string { func TestKeyring(t *testing.T) { t.Parallel() - if runtime.GOOS != "windows" { - t.Skip("linux and darwin are not supported yet") + if runtime.GOOS != "windows" && runtime.GOOS != "darwin" { + t.Skip("linux is not supported yet") } // This test exercises use of the operating system keyring. As a result, @@ -199,6 +205,66 @@ func TestKeyring(t *testing.T) { err = backend.Delete(srvURL2) require.NoError(t, err) }) + + t.Run("StorageFormat", func(t *testing.T) { + t.Parallel() + // The storage format must remain consistent to ensure we don't break + // compatibility with other Coder related applications that may read + // or decode the same credential. + + const testURL1 = "http://127.0.0.1:1337" + srv1URL, err := url.Parse(testURL1) + require.NoError(t, err) + + const testURL2 = "http://127.0.0.1:1338" + srv2URL, err := url.Parse(testURL2) + require.NoError(t, err) + + serviceName := keyringTestServiceName(t) + backend := sessionstore.NewKeyringWithService(serviceName) + t.Cleanup(func() { + _ = backend.Delete(srv1URL) + _ = backend.Delete(srv2URL) + }) + + // Write token for server 1 + const token1 = "token-server-1" + err = backend.Write(srv1URL, token1) + require.NoError(t, err) + + // Write token for server 2 (should NOT overwrite server 1's token) + const token2 = "token-server-2" + err = backend.Write(srv2URL, token2) + require.NoError(t, err) + + // Verify both credentials are stored in the raw format and can + // be extracted through the Backend API. + rawCredential := readRawKeychainCredential(t, serviceName) + + storedCreds := make(storedCredentials) + err = json.Unmarshal(rawCredential, &storedCreds) + require.NoError(t, err, "unmarshalling stored credentials") + + // Both credentials should exist + require.Len(t, storedCreds, 2) + require.Equal(t, token1, storedCreds[srv1URL.Host].APIToken) + require.Equal(t, token2, storedCreds[srv2URL.Host].APIToken) + + // Read individual credentials + token, err := backend.Read(srv1URL) + require.NoError(t, err) + require.Equal(t, token1, token) + + token, err = backend.Read(srv2URL) + require.NoError(t, err) + require.Equal(t, token2, token) + + // Cleanup + err = backend.Delete(srv1URL) + require.NoError(t, err) + err = backend.Delete(srv2URL) + require.NoError(t, err) + }) } func TestFile(t *testing.T) { diff --git a/cli/sessionstore/sessionstore_windows_test.go b/cli/sessionstore/sessionstore_windows_test.go index 4d90e27f5c132..ef643d3033dba 100644 --- a/cli/sessionstore/sessionstore_windows_test.go +++ b/cli/sessionstore/sessionstore_windows_test.go @@ -14,6 +14,16 @@ import ( "github.com/coder/coder/v2/cli/sessionstore" ) +func readRawKeychainCredential(t *testing.T, serviceName string) []byte { + t.Helper() + + winCred, err := wincred.GetGenericCredential(serviceName) + if err != nil { + t.Fatal(err) + } + return winCred.CredentialBlob +} + func TestWindowsKeyring_WriteReadDelete(t *testing.T) { t.Parallel() @@ -38,10 +48,7 @@ func TestWindowsKeyring_WriteReadDelete(t *testing.T) { winCred, err := wincred.GetGenericCredential(serviceName) require.NoError(t, err, "getting windows credential") - var storedCreds map[string]struct { - CoderURL string `json:"coder_url"` - APIToken string `json:"api_token"` - } + storedCreds := make(storedCredentials) err = json.Unmarshal(winCred.CredentialBlob, &storedCreds) require.NoError(t, err, "unmarshalling stored credentials") @@ -65,63 +72,3 @@ func TestWindowsKeyring_WriteReadDelete(t *testing.T) { _, err = backend.Read(srvURL) require.ErrorIs(t, err, os.ErrNotExist) } - -func TestWindowsKeyring_MultipleServers(t *testing.T) { - t.Parallel() - - const testURL1 = "http://127.0.0.1:1337" - srv1URL, err := url.Parse(testURL1) - require.NoError(t, err) - - const testURL2 = "http://127.0.0.1:1338" - srv2URL, err := url.Parse(testURL2) - require.NoError(t, err) - - serviceName := keyringTestServiceName(t) - backend := sessionstore.NewKeyringWithService(serviceName) - t.Cleanup(func() { - _ = backend.Delete(srv1URL) - _ = backend.Delete(srv2URL) - }) - - // Write token for server 1 - const token1 = "token-server-1" - err = backend.Write(srv1URL, token1) - require.NoError(t, err) - - // Write token for server 2 (should NOT overwrite server 1's token) - const token2 = "token-server-2" - err = backend.Write(srv2URL, token2) - require.NoError(t, err) - - // Verify both credentials are stored in Windows Credential Manager - winCred, err := wincred.GetGenericCredential(serviceName) - require.NoError(t, err, "getting windows credential") - - var storedCreds map[string]struct { - CoderURL string `json:"coder_url"` - APIToken string `json:"api_token"` - } - err = json.Unmarshal(winCred.CredentialBlob, &storedCreds) - require.NoError(t, err, "unmarshalling stored credentials") - - // Both credentials should exist - require.Len(t, storedCreds, 2) - require.Equal(t, token1, storedCreds[srv1URL.Host].APIToken) - require.Equal(t, token2, storedCreds[srv2URL.Host].APIToken) - - // Read individual credentials - token, err := backend.Read(srv1URL) - require.NoError(t, err) - require.Equal(t, token1, token) - - token, err = backend.Read(srv2URL) - require.NoError(t, err) - require.Equal(t, token2, token) - - // Cleanup - err = backend.Delete(srv1URL) - require.NoError(t, err) - err = backend.Delete(srv2URL) - require.NoError(t, err) -} From adb6f5bd25c03b44b5d0f82d08689662310cb7e4 Mon Sep 17 00:00:00 2001 From: Zach Kipp Date: Wed, 12 Nov 2025 10:05:53 -0700 Subject: [PATCH 2/2] fix: adjust macOS keychain username and cleanup consts --- cli/sessionstore/sessionstore.go | 6 ++++++ cli/sessionstore/sessionstore_darwin.go | 6 +----- cli/sessionstore/sessionstore_darwin_test.go | 2 +- cli/sessionstore/sessionstore_other.go | 2 -- cli/sessionstore/sessionstore_windows.go | 6 ------ 5 files changed, 8 insertions(+), 14 deletions(-) diff --git a/cli/sessionstore/sessionstore.go b/cli/sessionstore/sessionstore.go index 8ae14c2786c2f..029e86ad7e99b 100644 --- a/cli/sessionstore/sessionstore.go +++ b/cli/sessionstore/sessionstore.go @@ -46,6 +46,12 @@ var ( ErrNotImplemented = xerrors.New("not implemented") ) +const ( + // defaultServiceName is the service name used in keyrings for storing Coder CLI session + // tokens. + defaultServiceName = "coder-v2-credentials" +) + // keyringProvider represents an operating system keyring. The expectation // is these methods operate on the user/login keyring. type keyringProvider interface { diff --git a/cli/sessionstore/sessionstore_darwin.go b/cli/sessionstore/sessionstore_darwin.go index 6caf3065ea189..be398d42e7049 100644 --- a/cli/sessionstore/sessionstore_darwin.go +++ b/cli/sessionstore/sessionstore_darwin.go @@ -13,13 +13,9 @@ import ( ) const ( - // defaultServiceName is the service name used in the macOS Keychain - // for storing Coder CLI session tokens. - defaultServiceName = "coder-v2-credentials" - // fixedUsername is the fixed username used for all keychain entries. // Since our interface only uses service names, we use a constant username. - fixedUsername = "coder-session-token" + fixedUsername = "coder-login-credentials" execPathKeychain = "/usr/bin/security" notFoundStr = "could not be found" diff --git a/cli/sessionstore/sessionstore_darwin_test.go b/cli/sessionstore/sessionstore_darwin_test.go index 029028fa77879..a90ee12d96cc1 100644 --- a/cli/sessionstore/sessionstore_darwin_test.go +++ b/cli/sessionstore/sessionstore_darwin_test.go @@ -10,7 +10,7 @@ import ( const ( execPathKeychain = "/usr/bin/security" - fixedUsername = "coder-session-token" + fixedUsername = "coder-login-credentials" ) func readRawKeychainCredential(t *testing.T, service string) []byte { diff --git a/cli/sessionstore/sessionstore_other.go b/cli/sessionstore/sessionstore_other.go index e2e41ba536f02..a71458a360c94 100644 --- a/cli/sessionstore/sessionstore_other.go +++ b/cli/sessionstore/sessionstore_other.go @@ -2,8 +2,6 @@ package sessionstore -const defaultServiceName = "not-implemented" - type operatingSystemKeyring struct{} func (operatingSystemKeyring) Set(_, _ string) error { diff --git a/cli/sessionstore/sessionstore_windows.go b/cli/sessionstore/sessionstore_windows.go index 6c48db0154f3f..3dd38c19da31d 100644 --- a/cli/sessionstore/sessionstore_windows.go +++ b/cli/sessionstore/sessionstore_windows.go @@ -10,12 +10,6 @@ import ( "github.com/danieljoos/wincred" ) -const ( - // defaultServiceName is the service name used in the Windows Credential Manager - // for storing Coder CLI session tokens. - defaultServiceName = "coder-v2-credentials" -) - // operatingSystemKeyring implements keyringProvider and uses Windows Credential Manager. // It is largely adapted from the zalando/go-keyring package. type operatingSystemKeyring struct{}