Skip to content

Commit bdb2a67

Browse files
zedkippzedkipp
committed
feat(cli)!: enable keyring usage by default
Make keyring usage for session token storage on by default for supported platforms (Windows and macOS), with the ability to opt-out via --use-keyring=false. This change will be a breaking change for any users depending on the session token being stored on disk, though users can restore file usage via the flag above.
1 parent ddcc841 commit bdb2a67

File tree

9 files changed

+151
-103
lines changed

9 files changed

+151
-103
lines changed

‎cli/clitest/clitest.go‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -66,11 +66,13 @@ func NewWithCommand(
6666
Named("cli")
6767
i := &serpent.Invocation{
6868
Command: cmd,
69-
Args: append([]string{"--global-config", string(configDir)}, args...),
70-
Stdin: io.LimitReader(nil, 0),
71-
Stdout: (&logWriter{prefix: "stdout", log: logger}),
72-
Stderr: (&logWriter{prefix: "stderr", log: logger}),
73-
Logger: logger,
69+
// Keyring usage is disabled here because many existing tests expect the session token
70+
// to be stored on disk.
71+
Args: append([]string{"--global-config", string(configDir), "--use-keyring=false"}, args...),
72+
Stdin: io.LimitReader(nil, 0),
73+
Stdout: (&logWriter{prefix: "stdout", log: logger}),
74+
Stderr: (&logWriter{prefix: "stderr", log: logger}),
75+
Logger: logger,
7476
}
7577
t.Logf("invoking command: %s %s", cmd.Name(), strings.Join(i.Args, " "))
7678

‎cli/keyring_test.go‎

Lines changed: 120 additions & 79 deletions
Original file line numberDiff line numberDiff line change
@@ -2,87 +2,94 @@ package cli_test
22

33
import (
44
"bytes"
5+
"crypto/rand"
6+
"encoding/binary"
7+
"fmt"
58
"net/url"
69
"os"
710
"path"
811
"runtime"
912
"testing"
13+
"time"
1014

1115
"github.com/stretchr/testify/assert"
1216
"github.com/stretchr/testify/require"
1317

1418
"github.com/coder/coder/v2/cli"
1519
"github.com/coder/coder/v2/cli/clitest"
20+
"github.com/coder/coder/v2/cli/sessionstore"
1621
"github.com/coder/coder/v2/coderd/coderdtest"
1722
"github.com/coder/coder/v2/pty/ptytest"
23+
"github.com/coder/serpent"
1824
)
1925

20-
// mockKeyring is a mock sessionstore.Backend implementation.
21-
type mockKeyring struct {
22-
credentials map[string]string // service name -> credential
26+
// keyringTestServiceName generates a unique service name for keyring tests
27+
// using the test name and a nanosecond timestamp to prevent collisions.
28+
func keyringTestServiceName(t *testing.T) string {
29+
t.Helper()
30+
var n uint32
31+
err := binary.Read(rand.Reader, binary.BigEndian, &n)
32+
if err != nil {
33+
t.Fatal(err)
34+
}
35+
return fmt.Sprintf("%s_%v_%d", t.Name(), time.Now().UnixNano(), n)
2336
}
2437

25-
const mockServiceName = "mock-service-name"
38+
// instrumentKeyring sets up the CLI invocation to use the actual OS keyring
39+
// with a unique test service name to allow test parallelization. It returns
40+
// the backend and URL for verification of keyring contents in tests.
41+
func instrumentKeyring(t *testing.T, inv *serpent.Invocation, serverURL string) (sessionstore.Backend, *url.URL) {
42+
t.Helper()
2643

27-
func newMockKeyring() *mockKeyring {
28-
return &mockKeyring{credentials: make(map[string]string)}
29-
}
44+
serviceName := keyringTestServiceName(t)
45+
backend := sessionstore.NewKeyringWithService(serviceName)
3046

31-
func (m *mockKeyring) Read(_ *url.URL) (string, error) {
32-
cred, ok := m.credentials[mockServiceName]
33-
if !ok {
34-
return "", os.ErrNotExist
35-
}
36-
return cred, nil
37-
}
47+
srvURL, err := url.Parse(serverURL)
48+
require.NoError(t, err)
3849

39-
func (m *mockKeyring) Write(_ *url.URL, token string) error {
40-
m.credentials[mockServiceName] = token
41-
return nil
42-
}
50+
t.Cleanup(func() {
51+
_ = backend.Delete(srvURL)
52+
})
4353

44-
func (m *mockKeyring) Delete(_ *url.URL) error {
45-
_, ok := m.credentials[mockServiceName]
46-
if !ok {
47-
return os.ErrNotExist
48-
}
49-
delete(m.credentials, mockServiceName)
50-
return nil
54+
var root cli.RootCmd
55+
cmd, err := root.Command(root.AGPL())
56+
require.NoError(t, err)
57+
root.WithSessionStorageBackend(backend)
58+
inv.Command = cmd
59+
60+
return backend, srvURL
5161
}
5262

5363
func TestUseKeyring(t *testing.T) {
54-
// Verify that the --use-keyring flag opts into using a keyring backend for
55-
// storing session tokens instead of plain text files.
64+
// Verify that the --use-keyring flag default opts into using a keyring backend
65+
// for storing session tokens instead of plain text files.
5666
t.Parallel()
5767

5868
t.Run("Login", func(t *testing.T) {
5969
t.Parallel()
6070

71+
if runtime.GOOS != "windows" && runtime.GOOS != "darwin" {
72+
t.Skip("keyring is not supported on this OS")
73+
}
74+
6175
// Create a test server
6276
client := coderdtest.New(t, nil)
6377
coderdtest.CreateFirstUser(t, client)
6478

6579
// Create a pty for interactive prompts
6680
pty := ptytest.New(t)
6781

68-
// Create CLI invocation with --use-keyring flag
82+
// Create CLI invocation which defaults to using the keyring
6983
inv, cfg := clitest.New(t,
7084
"login",
7185
"--force-tty",
72-
"--use-keyring",
7386
"--no-open",
7487
client.URL.String(),
7588
)
7689
inv.Stdin = pty.Input()
7790
inv.Stdout = pty.Output()
7891

79-
// Inject the mock backend before running the command
80-
var root cli.RootCmd
81-
cmd, err := root.Command(root.AGPL())
82-
require.NoError(t, err)
83-
mockBackend := newMockKeyring()
84-
root.WithSessionStorageBackend(mockBackend)
85-
inv.Command = cmd
92+
backend, srvURL := instrumentKeyring(t, inv, client.URL.String())
8693

8794
// Run login in background
8895
doneChan := make(chan struct{})
@@ -100,43 +107,40 @@ func TestUseKeyring(t *testing.T) {
100107

101108
// Verify that session file was NOT created (using keyring instead)
102109
sessionFile := path.Join(string(cfg), "session")
103-
_, err = os.Stat(sessionFile)
110+
_, err := os.Stat(sessionFile)
104111
require.True(t, os.IsNotExist(err), "session file should not exist when using keyring")
105112

106-
// Verify that the credential IS stored in mock keyring
107-
cred, err := mockBackend.Read(nil)
108-
require.NoError(t, err, "credential should be stored in mock keyring")
113+
// Verify that the credential IS stored in OS keyring
114+
cred, err := backend.Read(srvURL)
115+
require.NoError(t, err, "credential should be stored in OS keyring")
109116
require.Equal(t, client.SessionToken(), cred, "stored token should match login token")
110117
})
111118

112119
t.Run("Logout", func(t *testing.T) {
113120
t.Parallel()
114121

122+
if runtime.GOOS != "windows" && runtime.GOOS != "darwin" {
123+
t.Skip("keyring is not supported on this OS")
124+
}
125+
115126
// Create a test server
116127
client := coderdtest.New(t, nil)
117128
coderdtest.CreateFirstUser(t, client)
118129

119130
// Create a pty for interactive prompts
120131
pty := ptytest.New(t)
121132

122-
// First, login with --use-keyring
133+
// First, login with the keyring (default)
123134
loginInv, cfg := clitest.New(t,
124135
"login",
125136
"--force-tty",
126-
"--use-keyring",
127137
"--no-open",
128138
client.URL.String(),
129139
)
130140
loginInv.Stdin = pty.Input()
131141
loginInv.Stdout = pty.Output()
132142

133-
// Inject the mock backend
134-
var loginRoot cli.RootCmd
135-
loginCmd, err := loginRoot.Command(loginRoot.AGPL())
136-
require.NoError(t, err)
137-
mockBackend := newMockKeyring()
138-
loginRoot.WithSessionStorageBackend(mockBackend)
139-
loginInv.Command = loginCmd
143+
backend, srvURL := instrumentKeyring(t, loginInv, client.URL.String())
140144

141145
doneChan := make(chan struct{})
142146
go func() {
@@ -150,24 +154,23 @@ func TestUseKeyring(t *testing.T) {
150154
pty.ExpectMatch("Welcome to Coder")
151155
<-doneChan
152156

153-
// Verify credential exists in mock keyring
154-
cred, err := mockBackend.Read(nil)
157+
// Verify credential exists in OS keyring
158+
cred, err := backend.Read(srvURL)
155159
require.NoError(t, err, "read credential should succeed before logout")
156-
require.NotEmpty(t, cred, "credential should exist after logout")
160+
require.NotEmpty(t, cred, "credential should exist before logout")
157161

158-
// Now run logout with --use-keyring
162+
// Now logout
159163
logoutInv, _ := clitest.New(t,
160164
"logout",
161-
"--use-keyring",
162165
"--yes",
163166
"--global-config", string(cfg),
164167
)
165168

166-
// Inject the same mock backend
169+
// Instrument logout with the same backend
167170
var logoutRoot cli.RootCmd
168171
logoutCmd, err := logoutRoot.Command(logoutRoot.AGPL())
169172
require.NoError(t, err)
170-
logoutRoot.WithSessionStorageBackend(mockBackend)
173+
logoutRoot.WithSessionStorageBackend(backend)
171174
logoutInv.Command = logoutCmd
172175

173176
var logoutOut bytes.Buffer
@@ -176,22 +179,25 @@ func TestUseKeyring(t *testing.T) {
176179
err = logoutInv.Run()
177180
require.NoError(t, err, "logout should succeed")
178181

179-
// Verify the credential was deleted from mock keyring
180-
_, err = mockBackend.Read(nil)
182+
// Verify the credential was deleted from OS keyring
183+
_, err = backend.Read(srvURL)
181184
require.ErrorIs(t, err, os.ErrNotExist, "credential should be deleted from keyring after logout")
182185
})
183186

184-
t.Run("OmitFlag", func(t *testing.T) {
187+
t.Run("DefaultFileStorage", func(t *testing.T) {
185188
t.Parallel()
186189

190+
if runtime.GOOS != "linux" {
191+
t.Skip("file storage is the default for Linux")
192+
}
193+
187194
// Create a test server
188195
client := coderdtest.New(t, nil)
189196
coderdtest.CreateFirstUser(t, client)
190197

191198
// Create a pty for interactive prompts
192199
pty := ptytest.New(t)
193200

194-
// --use-keyring flag omitted (should use file-based storage)
195201
inv, cfg := clitest.New(t,
196202
"login",
197203
"--force-tty",
@@ -216,7 +222,7 @@ func TestUseKeyring(t *testing.T) {
216222
// Verify that session file WAS created (not using keyring)
217223
sessionFile := path.Join(string(cfg), "session")
218224
_, err := os.Stat(sessionFile)
219-
require.NoError(t, err, "session file should exist when NOT using --use-keyring")
225+
require.NoError(t, err, "session file should exist when NOT using --use-keyring on Linux")
220226

221227
// Read and verify the token from file
222228
content, err := os.ReadFile(sessionFile)
@@ -234,7 +240,8 @@ func TestUseKeyring(t *testing.T) {
234240
// Create a pty for interactive prompts
235241
pty := ptytest.New(t)
236242

237-
// Login using CODER_USE_KEYRING environment variable instead of flag
243+
// Login using CODER_USE_KEYRING environment variable set to disable keyring usage,
244+
// which should have the same behavior on all platforms.
238245
inv, cfg := clitest.New(t,
239246
"login",
240247
"--force-tty",
@@ -243,15 +250,49 @@ func TestUseKeyring(t *testing.T) {
243250
)
244251
inv.Stdin = pty.Input()
245252
inv.Stdout = pty.Output()
246-
inv.Environ.Set("CODER_USE_KEYRING", "true")
253+
inv.Environ.Set("CODER_USE_KEYRING", "false")
247254

248-
// Inject the mock backend
249-
var root cli.RootCmd
250-
cmd, err := root.Command(root.AGPL())
251-
require.NoError(t, err)
252-
mockBackend := newMockKeyring()
253-
root.WithSessionStorageBackend(mockBackend)
254-
inv.Command = cmd
255+
doneChan := make(chan struct{})
256+
go func() {
257+
defer close(doneChan)
258+
err := inv.Run()
259+
assert.NoError(t, err)
260+
}()
261+
262+
pty.ExpectMatch("Paste your token here:")
263+
pty.WriteLine(client.SessionToken())
264+
pty.ExpectMatch("Welcome to Coder")
265+
<-doneChan
266+
267+
// Verify that session file WAS created (not using keyring)
268+
sessionFile := path.Join(string(cfg), "session")
269+
_, err := os.Stat(sessionFile)
270+
require.NoError(t, err, "session file should exist when CODER_USE_KEYRING set to false")
271+
272+
// Read and verify the token from file
273+
content, err := os.ReadFile(sessionFile)
274+
require.NoError(t, err, "should be able to read session file")
275+
require.Equal(t, client.SessionToken(), string(content), "file should contain the session token")
276+
})
277+
278+
t.Run("DisableKeyringWithFlag", func(t *testing.T) {
279+
t.Parallel()
280+
281+
client := coderdtest.New(t, nil)
282+
coderdtest.CreateFirstUser(t, client)
283+
pty := ptytest.New(t)
284+
285+
// Login with --use-keyring=false to explicitly disable keyring usage, which
286+
// should have the same behavior on all platforms.
287+
inv, cfg := clitest.New(t,
288+
"login",
289+
"--use-keyring=false",
290+
"--force-tty",
291+
"--no-open",
292+
client.URL.String(),
293+
)
294+
inv.Stdin = pty.Input()
295+
inv.Stdout = pty.Output()
255296

256297
doneChan := make(chan struct{})
257298
go func() {
@@ -265,15 +306,15 @@ func TestUseKeyring(t *testing.T) {
265306
pty.ExpectMatch("Welcome to Coder")
266307
<-doneChan
267308

268-
// Verify that session file was NOT created (using keyring via env var)
309+
// Verify that session file WAS created (not using keyring)
269310
sessionFile := path.Join(string(cfg), "session")
270-
_, err = os.Stat(sessionFile)
271-
require.True(t, os.IsNotExist(err), "session file should not exist when using keyring via env var")
311+
_, err := os.Stat(sessionFile)
312+
require.NoError(t, err, "session file should exist when --use-keyring=false is specified")
272313

273-
// Verify credential is in mock keyring
274-
cred, err := mockBackend.Read(nil)
275-
require.NoError(t, err, "credential should be stored in keyring when CODER_USE_KEYRING=true")
276-
require.NotEmpty(t, cred)
314+
// Read and verify the token from file
315+
content, err := os.ReadFile(sessionFile)
316+
require.NoError(t, err, "should be able to read session file")
317+
require.Equal(t, client.SessionToken(), string(content), "file should contain the session token")
277318
})
278319
}
279320

@@ -287,7 +328,7 @@ func TestUseKeyringUnsupportedOS(t *testing.T) {
287328
t.Skipf("Skipping unsupported OS test on %s where keyring is supported", runtime.GOOS)
288329
}
289330

290-
const expMessage = "keyring storage is not supported on this operating system; remove the --use-keyring flag"
331+
const expMessage = "keyring storage is not supported on this operating system; omit --use-keyring to use file-based storage"
291332

292333
t.Run("LoginWithUnsupportedKeyring", func(t *testing.T) {
293334
t.Parallel()
@@ -317,7 +358,7 @@ func TestUseKeyringUnsupportedOS(t *testing.T) {
317358
coderdtest.CreateFirstUser(t, client)
318359
pty := ptytest.New(t)
319360

320-
// First login without keyring to create a session
361+
// First login without keyring to create a session (default behavior)
321362
loginInv, cfg := clitest.New(t,
322363
"login",
323364
"--force-tty",

‎cli/login.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -154,9 +154,9 @@ func (r *RootCmd) login() *serpent.Command {
154154
cmd := &serpent.Command{
155155
Use: "login [<url>]",
156156
Short: "Authenticate with Coder deployment",
157-
Long: "By default, the session token is stored in a plain text file. Use the " +
158-
"--use-keyring flag or set CODER_USE_KEYRING=true to store the token in " +
159-
"the operating system keyring instead.",
157+
Long: "By default, the session token is stored in the operating system keyring on " +
158+
"macOS and Windows and a plain text file on Linux. Use the --use-keyring flag " +
159+
"or CODER_USE_KEYRING environment variable to change the storage mechanism.",
160160
Middleware: serpent.RequireRangeArgs(0, 1),
161161
Handler: func(inv *serpent.Invocation) error {
162162
ctx := inv.Context()

0 commit comments

Comments
 (0)