Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions cli/keyring_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
6 changes: 6 additions & 0 deletions cli/sessionstore/sessionstore.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
105 changes: 105 additions & 0 deletions cli/sessionstore/sessionstore_darwin.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
//go:build darwin

package sessionstore

import (
"encoding/base64"
"fmt"
"io"
"os"
"os/exec"
"regexp"
"strings"
)

const (
// fixedUsername is the fixed username used for all keychain entries.
// Since our interface only uses service names, we use a constant username.
fixedUsername = "coder-login-credentials"

execPathKeychain = "/usr/bin/security"
notFoundStr = "could not be found"
Copy link
Member

Choose a reason for hiding this comment

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

I see it's what Zalando does, and it's cosmetic but this seems broken. Wouldn't the error string be localized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did a little digging on this and it appears the security binary isn't localized. I tried setting a different locale and it made no difference. No issues reported for the Github CLI about this (uses the zalando package under the hood).

)

// 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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ethanndickson what are your thoughts on keeping this base64 encoding/decoding? It's borrowed from https://github.com/zalando/go-keyring/. I don't think we explicitly need it given our token format.

Copy link
Member

@ethanndickson ethanndickson Nov 11, 2025

Choose a reason for hiding this comment

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

I'm pretty sure we have a bunch of users who populate Coder login credentials from a script. It doesn't seem impossible for users to accidentally add extra newlines or something if they populate this new format, so I think we should keep it.

password := base64.StdEncoding.EncodeToString([]byte(credential))

cmd := exec.Command(execPathKeychain, "-i")
Copy link
Member

Choose a reason for hiding this comment

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

My first thought is that we should be calling exec.CommandContext with inv.Context, but these processes should all die immediately so it's probably fine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think nominally this should be fine. I did see the github CLI has wrapped keyring operations with a timeout, but I am inclined to hold off on adding any timeouts/wrappers https://github.com/cli/cli/blob/85d6766c621a9d67e0d353a12bb388c40ceca2a1/internal/keyring/keyring.go#L2

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
}
34 changes: 34 additions & 0 deletions cli/sessionstore/sessionstore_darwin_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
//go:build darwin

package sessionstore_test

import (
"encoding/base64"
"os/exec"
"testing"
)

const (
execPathKeychain = "/usr/bin/security"
fixedUsername = "coder-login-credentials"
)

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]
}
4 changes: 1 addition & 3 deletions cli/sessionstore/sessionstore_other.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
//go:build !windows
//go:build !windows && !darwin

package sessionstore

const defaultServiceName = "not-implemented"

type operatingSystemKeyring struct{}

func (operatingSystemKeyring) Set(_, _ string) error {
Expand Down
10 changes: 10 additions & 0 deletions cli/sessionstore/sessionstore_other_test.go
Original file line number Diff line number Diff line change
@@ -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
}
70 changes: 68 additions & 2 deletions cli/sessionstore/sessionstore_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package sessionstore_test

import (
"encoding/json"
"errors"
"fmt"
"net/url"
Expand All @@ -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"`
}
Copy link
Contributor Author

@zedkipp zedkipp Oct 31, 2025

Choose a reason for hiding this comment

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

@ethanndickson this is essentially what gets stored as a JSON blob (b64 encoded - see my other comment). Looks like this in my keychain. Any problems/concerns for Coder Desktop on macOS, in terms of future session token sharing?
Screenshot 2025-10-31 at 4 27 17 PM

Copy link
Member

Choose a reason for hiding this comment

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

Nope, looks good to me. For posterity, I discovered the contents of Access Control are more or less irrelevant in this case, as the user will always be prompted when another app (i.e. Coder Desktop) tries to read from the keychain item.

Comment on lines +20 to +23
Copy link
Member

Choose a reason for hiding this comment

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

Same here, re: making these tests internal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The linter testpackage we have enabled asks for these separate test packages. The approach seems pretty reasonable for these keyring tests.


// 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 {
Expand All @@ -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,
Expand Down Expand Up @@ -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) {
Expand Down
6 changes: 0 additions & 6 deletions cli/sessionstore/sessionstore_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Expand Down
75 changes: 11 additions & 64 deletions cli/sessionstore/sessionstore_windows_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand All @@ -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")

Expand All @@ -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)
}
Loading