Skip to content

Commit 139dab7

Browse files
zedkippzedkipp
authored
feat(cli): optionally store session token in OS keyring (#20256)
 This change implements optional secure storage of the CLI token using the operating system keyring for Windows, with groundwork laid for macOS in a future change. Previously, the Coder CLI stored authentication tokens in plaintext configuration files, which posed a security risk because users' tokens are stored unencrypted and can be easily accessed by other processes or users with file system access. The keyring is opt-in to preserve compatibility with applications (like the JetBrains Toolbox plugin, VS code plugin, etc). Users can opt into keyring use with a new `--use-keyring` flag. The secure storage is platform dependent. Windows Credential Manager API is used on Windows. The session token continues to be stored in plain text on macOS and Linux. macOS is omitted for now while we figure out the best path forward for compatibility with apps like Coder Desktop. https://www.notion.so/coderhq/CLI-Session-Token-in-OS-Keyring-293d579be592808b8b7fd235304e50d5 #19403
1 parent d306a2d commit 139dab7

File tree

17 files changed

+1383
-15
lines changed

17 files changed

+1383
-15
lines changed

‎cli/keyring_test.go‎

Lines changed: 355 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,355 @@
1+
package cli_test
2+
3+
import (
4+
"bytes"
5+
"net/url"
6+
"os"
7+
"path"
8+
"runtime"
9+
"testing"
10+
11+
"github.com/stretchr/testify/assert"
12+
"github.com/stretchr/testify/require"
13+
14+
"github.com/coder/coder/v2/cli"
15+
"github.com/coder/coder/v2/cli/clitest"
16+
"github.com/coder/coder/v2/coderd/coderdtest"
17+
"github.com/coder/coder/v2/pty/ptytest"
18+
)
19+
20+
// mockKeyring is a mock sessionstore.Backend implementation.
21+
type mockKeyring struct {
22+
credentials map[string]string // service name -> credential
23+
}
24+
25+
const mockServiceName = "mock-service-name"
26+
27+
func newMockKeyring() *mockKeyring {
28+
return &mockKeyring{credentials: make(map[string]string)}
29+
}
30+
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+
}
38+
39+
func (m *mockKeyring) Write(_ *url.URL, token string) error {
40+
m.credentials[mockServiceName] = token
41+
return nil
42+
}
43+
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
51+
}
52+
53+
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.
56+
t.Parallel()
57+
58+
t.Run("Login", func(t *testing.T) {
59+
t.Parallel()
60+
61+
// Create a test server
62+
client := coderdtest.New(t, nil)
63+
coderdtest.CreateFirstUser(t, client)
64+
65+
// Create a pty for interactive prompts
66+
pty := ptytest.New(t)
67+
68+
// Create CLI invocation with --use-keyring flag
69+
inv, cfg := clitest.New(t,
70+
"login",
71+
"--force-tty",
72+
"--use-keyring",
73+
"--no-open",
74+
client.URL.String(),
75+
)
76+
inv.Stdin = pty.Input()
77+
inv.Stdout = pty.Output()
78+
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
86+
87+
// Run login in background
88+
doneChan := make(chan struct{})
89+
go func() {
90+
defer close(doneChan)
91+
err := inv.Run()
92+
assert.NoError(t, err)
93+
}()
94+
95+
// Provide the token when prompted
96+
pty.ExpectMatch("Paste your token here:")
97+
pty.WriteLine(client.SessionToken())
98+
pty.ExpectMatch("Welcome to Coder")
99+
<-doneChan
100+
101+
// Verify that session file was NOT created (using keyring instead)
102+
sessionFile := path.Join(string(cfg), "session")
103+
_, err = os.Stat(sessionFile)
104+
require.True(t, os.IsNotExist(err), "session file should not exist when using keyring")
105+
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")
109+
require.Equal(t, client.SessionToken(), cred, "stored token should match login token")
110+
})
111+
112+
t.Run("Logout", func(t *testing.T) {
113+
t.Parallel()
114+
115+
// Create a test server
116+
client := coderdtest.New(t, nil)
117+
coderdtest.CreateFirstUser(t, client)
118+
119+
// Create a pty for interactive prompts
120+
pty := ptytest.New(t)
121+
122+
// First, login with --use-keyring
123+
loginInv, cfg := clitest.New(t,
124+
"login",
125+
"--force-tty",
126+
"--use-keyring",
127+
"--no-open",
128+
client.URL.String(),
129+
)
130+
loginInv.Stdin = pty.Input()
131+
loginInv.Stdout = pty.Output()
132+
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
140+
141+
doneChan := make(chan struct{})
142+
go func() {
143+
defer close(doneChan)
144+
err := loginInv.Run()
145+
assert.NoError(t, err)
146+
}()
147+
148+
pty.ExpectMatch("Paste your token here:")
149+
pty.WriteLine(client.SessionToken())
150+
pty.ExpectMatch("Welcome to Coder")
151+
<-doneChan
152+
153+
// Verify credential exists in mock keyring
154+
cred, err := mockBackend.Read(nil)
155+
require.NoError(t, err, "read credential should succeed before logout")
156+
require.NotEmpty(t, cred, "credential should exist after logout")
157+
158+
// Now run logout with --use-keyring
159+
logoutInv, _ := clitest.New(t,
160+
"logout",
161+
"--use-keyring",
162+
"--yes",
163+
"--global-config", string(cfg),
164+
)
165+
166+
// Inject the same mock backend
167+
var logoutRoot cli.RootCmd
168+
logoutCmd, err := logoutRoot.Command(logoutRoot.AGPL())
169+
require.NoError(t, err)
170+
logoutRoot.WithSessionStorageBackend(mockBackend)
171+
logoutInv.Command = logoutCmd
172+
173+
var logoutOut bytes.Buffer
174+
logoutInv.Stdout = &logoutOut
175+
176+
err = logoutInv.Run()
177+
require.NoError(t, err, "logout should succeed")
178+
179+
// Verify the credential was deleted from mock keyring
180+
_, err = mockBackend.Read(nil)
181+
require.ErrorIs(t, err, os.ErrNotExist, "credential should be deleted from keyring after logout")
182+
})
183+
184+
t.Run("OmitFlag", func(t *testing.T) {
185+
t.Parallel()
186+
187+
// Create a test server
188+
client := coderdtest.New(t, nil)
189+
coderdtest.CreateFirstUser(t, client)
190+
191+
// Create a pty for interactive prompts
192+
pty := ptytest.New(t)
193+
194+
// --use-keyring flag omitted (should use file-based storage)
195+
inv, cfg := clitest.New(t,
196+
"login",
197+
"--force-tty",
198+
"--no-open",
199+
client.URL.String(),
200+
)
201+
inv.Stdin = pty.Input()
202+
inv.Stdout = pty.Output()
203+
204+
doneChan := make(chan struct{})
205+
go func() {
206+
defer close(doneChan)
207+
err := inv.Run()
208+
assert.NoError(t, err)
209+
}()
210+
211+
pty.ExpectMatch("Paste your token here:")
212+
pty.WriteLine(client.SessionToken())
213+
pty.ExpectMatch("Welcome to Coder")
214+
<-doneChan
215+
216+
// Verify that session file WAS created (not using keyring)
217+
sessionFile := path.Join(string(cfg), "session")
218+
_, err := os.Stat(sessionFile)
219+
require.NoError(t, err, "session file should exist when NOT using --use-keyring")
220+
221+
// Read and verify the token from file
222+
content, err := os.ReadFile(sessionFile)
223+
require.NoError(t, err, "should be able to read session file")
224+
require.Equal(t, client.SessionToken(), string(content), "file should contain the session token")
225+
})
226+
227+
t.Run("EnvironmentVariable", func(t *testing.T) {
228+
t.Parallel()
229+
230+
// Create a test server
231+
client := coderdtest.New(t, nil)
232+
coderdtest.CreateFirstUser(t, client)
233+
234+
// Create a pty for interactive prompts
235+
pty := ptytest.New(t)
236+
237+
// Login using CODER_USE_KEYRING environment variable instead of flag
238+
inv, cfg := clitest.New(t,
239+
"login",
240+
"--force-tty",
241+
"--no-open",
242+
client.URL.String(),
243+
)
244+
inv.Stdin = pty.Input()
245+
inv.Stdout = pty.Output()
246+
inv.Environ.Set("CODER_USE_KEYRING", "true")
247+
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+
256+
doneChan := make(chan struct{})
257+
go func() {
258+
defer close(doneChan)
259+
err := inv.Run()
260+
assert.NoError(t, err)
261+
}()
262+
263+
pty.ExpectMatch("Paste your token here:")
264+
pty.WriteLine(client.SessionToken())
265+
pty.ExpectMatch("Welcome to Coder")
266+
<-doneChan
267+
268+
// Verify that session file was NOT created (using keyring via env var)
269+
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")
272+
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)
277+
})
278+
}
279+
280+
func TestUseKeyringUnsupportedOS(t *testing.T) {
281+
// Verify that trying to use --use-keyring on an unsupported operating system produces
282+
// a helpful error message.
283+
t.Parallel()
284+
285+
// Skip on Windows since the keyring is actually supported.
286+
if runtime.GOOS == "windows" {
287+
t.Skip("Skipping unsupported OS test on Windows where keyring is supported")
288+
}
289+
290+
const expMessage = "keyring storage is not supported on this operating system; remove the --use-keyring flag"
291+
292+
t.Run("LoginWithUnsupportedKeyring", func(t *testing.T) {
293+
t.Parallel()
294+
295+
client := coderdtest.New(t, nil)
296+
coderdtest.CreateFirstUser(t, client)
297+
298+
// Try to login with --use-keyring on an unsupported OS
299+
inv, _ := clitest.New(t,
300+
"login",
301+
"--use-keyring",
302+
client.URL.String(),
303+
)
304+
305+
// The error should occur immediately, before any prompts
306+
loginErr := inv.Run()
307+
308+
// Verify we got an error about unsupported OS
309+
require.Error(t, loginErr)
310+
require.Contains(t, loginErr.Error(), expMessage)
311+
})
312+
313+
t.Run("LogoutWithUnsupportedKeyring", func(t *testing.T) {
314+
t.Parallel()
315+
316+
client := coderdtest.New(t, nil)
317+
coderdtest.CreateFirstUser(t, client)
318+
pty := ptytest.New(t)
319+
320+
// First login without keyring to create a session
321+
loginInv, cfg := clitest.New(t,
322+
"login",
323+
"--force-tty",
324+
"--no-open",
325+
client.URL.String(),
326+
)
327+
loginInv.Stdin = pty.Input()
328+
loginInv.Stdout = pty.Output()
329+
330+
doneChan := make(chan struct{})
331+
go func() {
332+
defer close(doneChan)
333+
err := loginInv.Run()
334+
assert.NoError(t, err)
335+
}()
336+
337+
pty.ExpectMatch("Paste your token here:")
338+
pty.WriteLine(client.SessionToken())
339+
pty.ExpectMatch("Welcome to Coder")
340+
<-doneChan
341+
342+
// Now try to logout with --use-keyring on an unsupported OS
343+
logoutInv, _ := clitest.New(t,
344+
"logout",
345+
"--use-keyring",
346+
"--yes",
347+
"--global-config", string(cfg),
348+
)
349+
350+
err := logoutInv.Run()
351+
// Verify we got an error about unsupported OS
352+
require.Error(t, err)
353+
require.Contains(t, err.Error(), expMessage)
354+
})
355+
}

0 commit comments

Comments
 (0)