Skip to content

Commit e401787

Browse files
zedkippzedkipp
committed
feat(cli): store session token in OS keyring with file fallback
This change implements secure storage of the CLI token using the operating system keyring with a fallback to plaintext file storage for Windows and macOS. 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 secure storage is platform dependent. The security command is used on macOS. Windows Credential Manager API is used on Windows. The session token continues to be stored in plain text on Linux.
1 parent 9f3b2cd commit e401787

File tree

11 files changed

+410
-17
lines changed

11 files changed

+410
-17
lines changed

‎cli/clitest/clitest.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"cdr.dev/slog/sloggers/slogtest"
2222
"github.com/coder/coder/v2/cli"
2323
"github.com/coder/coder/v2/cli/config"
24+
"github.com/coder/coder/v2/cli/sessionstore"
2425
"github.com/coder/coder/v2/codersdk"
2526
"github.com/coder/coder/v2/provisioner/echo"
2627
"github.com/coder/coder/v2/testutil"
@@ -32,6 +33,10 @@ import (
3233
func New(t testing.TB, args ...string) (*serpent.Invocation, config.Root) {
3334
var root cli.RootCmd
3435

36+
// Always store the session token in a file to simplify tests that aren't
37+
// explicitly exercising or need to depend on the operating system keyring.
38+
root.WithSessionStorageBackend(sessionstore.File{})
39+
3540
cmd, err := root.Command(root.AGPL())
3641
require.NoError(t, err)
3742

‎cli/login.go‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919
"github.com/coder/pretty"
2020

2121
"github.com/coder/coder/v2/cli/cliui"
22+
"github.com/coder/coder/v2/cli/sessionstore"
2223
"github.com/coder/coder/v2/coderd/userpassword"
2324
"github.com/coder/coder/v2/codersdk"
2425
"github.com/coder/serpent"
@@ -115,9 +116,12 @@ func (r *RootCmd) loginWithPassword(
115116

116117
sessionToken := resp.SessionToken
117118
config := r.createConfig()
118-
err = config.Session().Write(sessionToken)
119-
if err != nil {
120-
return xerrors.Errorf("write session token: %w", err)
119+
location, werr := r.tokenBackend.Write(config, client.URL, sessionToken)
120+
if werr != nil {
121+
return xerrors.Errorf("write session token: %w", werr)
122+
}
123+
if r.tokenBackend.PreferredLocation() == sessionstore.LocationKeyring && location == sessionstore.LocationFile {
124+
cliui.Warn(inv.Stderr, "⚠️ Token stored in PLAIN TEXT because keyring access failed.")
121125
}
122126

123127
client.SetSessionToken(sessionToken)
@@ -149,8 +153,12 @@ func (r *RootCmd) login() *serpent.Command {
149153
useTokenForSession bool
150154
)
151155
cmd := &serpent.Command{
152-
Use: "login [<url>]",
153-
Short: "Authenticate with Coder deployment",
156+
Use: "login [<url>]",
157+
Short: "Authenticate with Coder deployment",
158+
Long: "Stores the session token in the operating system keyring, with a fallback " +
159+
"to plain text if the keyring is not available. The security command is used " +
160+
"on macOS. Windows Credential Manager API is used on Windows. The session " +
161+
"is only stored in plain text on Linux.",
154162
Middleware: serpent.RequireRangeArgs(0, 1),
155163
Handler: func(inv *serpent.Invocation) error {
156164
ctx := inv.Context()
@@ -394,10 +402,13 @@ func (r *RootCmd) login() *serpent.Command {
394402
}
395403

396404
config := r.createConfig()
397-
err = config.Session().Write(sessionToken)
405+
location, err := r.tokenBackend.Write(config, client.URL, sessionToken)
398406
if err != nil {
399407
return xerrors.Errorf("write session token: %w", err)
400408
}
409+
if r.tokenBackend.PreferredLocation() == sessionstore.LocationKeyring && location == sessionstore.LocationFile {
410+
cliui.Warn(inv.Stderr, "⚠️ Token stored in PLAIN TEXT because keyring access failed.")
411+
}
401412
err = config.URL().Write(serverURL.String())
402413
if err != nil {
403414
return xerrors.Errorf("write server url: %w", err)

‎cli/logout.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -46,11 +46,11 @@ func (r *RootCmd) logout() *serpent.Command {
4646
errors = append(errors, xerrors.Errorf("remove URL file: %w", err))
4747
}
4848

49-
err = config.Session().Delete()
49+
err = r.tokenBackend.Delete(config, client.URL)
5050
// Only throw error if the session configuration file is present,
5151
// otherwise the user is already logged out, and we proceed
52-
if err != nil && !os.IsNotExist(err) {
53-
errors = append(errors, xerrors.Errorf("remove session file: %w", err))
52+
if err != nil && !xerrors.Is(err, os.ErrNotExist) {
53+
errors = append(errors, xerrors.Errorf("remove session token: %w", err))
5454
}
5555

5656
err = config.Organization().Delete()

‎cli/root.go‎

Lines changed: 32 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ import (
3737
"github.com/coder/coder/v2/cli/cliui"
3838
"github.com/coder/coder/v2/cli/config"
3939
"github.com/coder/coder/v2/cli/gitauth"
40+
"github.com/coder/coder/v2/cli/sessionstore"
4041
"github.com/coder/coder/v2/cli/telemetry"
4142
"github.com/coder/coder/v2/codersdk"
4243
"github.com/coder/coder/v2/codersdk/agentsdk"
@@ -382,6 +383,14 @@ func (r *RootCmd) Command(subcommands []*serpent.Command) (*serpent.Command, err
382383
if r.clientURL == nil {
383384
r.clientURL = new(url.URL)
384385
}
386+
if r.tokenBackend == nil {
387+
switch runtime.GOOS {
388+
case "windows", "darwin":
389+
r.tokenBackend = sessionstore.NewKeyringWithFileFallback()
390+
default:
391+
r.tokenBackend = sessionstore.File{}
392+
}
393+
}
385394

386395
globalGroup := &serpent.Group{
387396
Name: "Global",
@@ -508,6 +517,7 @@ func (r *RootCmd) Command(subcommands []*serpent.Command) (*serpent.Command, err
508517
type RootCmd struct {
509518
clientURL *url.URL
510519
token string
520+
tokenBackend sessionstore.Backend
511521
globalConfig string
512522
header []string
513523
headerCommand string
@@ -549,14 +559,20 @@ func (r *RootCmd) InitClient(inv *serpent.Invocation) (*codersdk.Client, error)
549559
return nil, err
550560
}
551561
}
552-
// Read the token stored on disk.
553562
if r.token == "" {
554-
r.token, err = conf.Session().Read()
563+
tok, location, err := r.tokenBackend.Read(conf, r.clientURL)
555564
// Even if there isn't a token, we don't care.
556565
// Some API routes can be unauthenticated.
557-
if err != nil && !os.IsNotExist(err) {
566+
if err != nil && !xerrors.Is(err, os.ErrNotExist) {
558567
return nil, err
559568
}
569+
if tok != "" {
570+
r.token = tok
571+
}
572+
if r.tokenBackend.PreferredLocation() == sessionstore.LocationKeyring && location == sessionstore.LocationFile {
573+
_, _ = fmt.Fprintln(inv.Stderr, pretty.Sprint(cliui.DefaultStyles.Warn,
574+
"⚠️ Token read from PLAIN TEXT. Consider logging in again to use keyring storage."))
575+
}
560576
}
561577

562578
// Configure HTTP client with transport wrappers
@@ -588,7 +604,6 @@ func (r *RootCmd) InitClient(inv *serpent.Invocation) (*codersdk.Client, error)
588604
// This allows commands to run without requiring authentication, but still use auth if available.
589605
func (r *RootCmd) TryInitClient(inv *serpent.Invocation) (*codersdk.Client, error) {
590606
conf := r.createConfig()
591-
var err error
592607
// Read the client URL stored on disk.
593608
if r.clientURL == nil || r.clientURL.String() == "" {
594609
rawURL, err := conf.URL().Read()
@@ -605,14 +620,20 @@ func (r *RootCmd) TryInitClient(inv *serpent.Invocation) (*codersdk.Client, erro
605620
}
606621
}
607622
}
608-
// Read the token stored on disk.
609623
if r.token == "" {
610-
r.token, err = conf.Session().Read()
624+
tok, location, err := r.tokenBackend.Read(conf, r.clientURL)
611625
// Even if there isn't a token, we don't care.
612626
// Some API routes can be unauthenticated.
613-
if err != nil && !os.IsNotExist(err) {
627+
if err != nil && !xerrors.Is(err, os.ErrNotExist) {
614628
return nil, err
615629
}
630+
if tok != "" {
631+
r.token = tok
632+
}
633+
if r.tokenBackend.PreferredLocation() == sessionstore.LocationKeyring && location == sessionstore.LocationFile {
634+
_, _ = fmt.Fprintln(inv.Stderr, pretty.Sprint(cliui.DefaultStyles.Warn,
635+
"⚠️ Token read from PLAIN TEXT. Login again to use keyring storage."))
636+
}
616637
}
617638

618639
// Only configure the client if we have a URL
@@ -688,6 +709,10 @@ func (r *RootCmd) createUnauthenticatedClient(ctx context.Context, serverURL *ur
688709
return client, nil
689710
}
690711

712+
func (r *RootCmd) WithSessionStorageBackend(backend sessionstore.Backend) {
713+
r.tokenBackend = backend
714+
}
715+
691716
type AgentAuth struct {
692717
// Agent Client config
693718
agentToken string

‎cli/sessionstore/sessionstore.go‎

Lines changed: 166 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,166 @@
1+
// Package sessionstore provides CLI session token storage mechanisms.
2+
package sessionstore
3+
4+
import (
5+
"errors"
6+
"net/url"
7+
"os"
8+
"strings"
9+
10+
"github.com/zalando/go-keyring"
11+
12+
"github.com/coder/coder/v2/cli/config"
13+
)
14+
15+
// Backend is a storage backend for session tokens. It's similar to keyring.Keyring
16+
// but intended to allow more flexible types and avoid global behaviors in the keyring
17+
// package (e.g. using the keyring package mock modifies a package global).
18+
type Backend interface {
19+
// Read returns the session token and where it is stored for the given server URL,
20+
// or an error, if any.
21+
Read(conf config.Root, serverURL *url.URL) (string, Location, error)
22+
// Write stores the session token for the given server URL. It returns where the
23+
// token was stored or an error, if any.
24+
Write(conf config.Root, serverURL *url.URL, token string) (Location, error)
25+
// Delete removes any stored session token or an error, if any. It will return
26+
// os.ErrNotExist error if no token exists to delete.
27+
Delete(conf config.Root, serverURL *url.URL) error
28+
// PreferredLocation returns the preferred token storage Location of this Backend.
29+
PreferredLocation() Location
30+
}
31+
32+
// keyringProvider is an interface alias to prevent leaking the go-keyring package
33+
// outside this package.
34+
type keyringProvider = keyring.Keyring
35+
36+
type operatingSystemKeyring struct{}
37+
38+
func (operatingSystemKeyring) Set(service, user, password string) error {
39+
return keyring.Set(service, user, password)
40+
}
41+
42+
func (operatingSystemKeyring) Get(service, user string) (string, error) {
43+
return keyring.Get(service, user)
44+
}
45+
46+
func (operatingSystemKeyring) Delete(service, user string) error {
47+
return keyring.Delete(service, user)
48+
}
49+
50+
const (
51+
servicePrefix = "coder-cli:"
52+
keyringAccount = "session"
53+
)
54+
55+
// Location represents where a session token is stored.
56+
type Location string
57+
58+
const (
59+
LocationNone Location = "none"
60+
LocationFile Location = "file"
61+
LocationKeyring Location = "keyring"
62+
)
63+
64+
func serviceName(u *url.URL) string {
65+
if u == nil || u.Host == "" {
66+
return servicePrefix + "default"
67+
}
68+
host := strings.TrimSpace(strings.ToLower(u.Host))
69+
return servicePrefix + host
70+
}
71+
72+
// KeyringWithFallback is a Backend that prefers keyring storage and falls back to file
73+
// storage if the operating system keyring is unavailable. Happy path usage of this
74+
// type should start with NewKeyringWithFileFallback.
75+
type KeyringWithFallback struct {
76+
keyringProvider keyringProvider
77+
}
78+
79+
func NewKeyringWithFileFallback() KeyringWithFallback {
80+
return NewKeyringProviderWithFileFallback(operatingSystemKeyring{})
81+
}
82+
83+
func NewKeyringProviderWithFileFallback(provider keyringProvider) KeyringWithFallback {
84+
return KeyringWithFallback{keyringProvider: provider}
85+
}
86+
87+
// Read prefers reading the token from the operating system keyring and falls back
88+
// to file storage if the operating system keyring is unavailable.
89+
func (o KeyringWithFallback) Read(conf config.Root, serverURL *url.URL) (string, Location, error) {
90+
svc := serviceName(serverURL)
91+
tok, err := o.keyringProvider.Get(svc, keyringAccount)
92+
if err == nil && tok != "" {
93+
return tok, LocationKeyring, nil
94+
}
95+
// Fallback to file storage.
96+
return File{}.Read(conf, serverURL)
97+
}
98+
99+
// Write prefers storing the token in the operating system keyring and falls back
100+
// to file storage if the keyring operation fails.
101+
func (o KeyringWithFallback) Write(conf config.Root, serverURL *url.URL, token string) (Location, error) {
102+
svc := serviceName(serverURL)
103+
err := o.keyringProvider.Set(svc, keyringAccount, token)
104+
if err == nil {
105+
// Best effort: remove plaintext file if it exists.
106+
_ = conf.Session().Delete()
107+
return LocationKeyring, nil
108+
}
109+
// Fallback to file storage.
110+
return File{}.Write(conf, serverURL, token)
111+
}
112+
113+
// Delete removes any stored session token from both the keyring and file.
114+
func (o KeyringWithFallback) Delete(conf config.Root, serverURL *url.URL) error {
115+
svc := serviceName(serverURL)
116+
117+
keyringErr := o.keyringProvider.Delete(svc, keyringAccount)
118+
if keyringErr != nil {
119+
if errors.Is(keyringErr, keyring.ErrNotFound) {
120+
// Make the error handling for keyring/file backends uniform for the caller.
121+
keyringErr = os.ErrNotExist
122+
}
123+
}
124+
125+
fileErr := File{}.Delete(conf, serverURL)
126+
switch {
127+
case fileErr != nil && keyringErr == nil:
128+
// Happy path when using the keyring. We could drill down into fileErr, but for
129+
// simplicity we will assume the error is because use of the keyring means the
130+
// file didn't exist.
131+
return nil
132+
case fileErr == nil && keyringErr != nil:
133+
// Deleted the file because of a problem with the OS keyring. The file may have
134+
// been created as a result of falling back to disk, or it may have already
135+
// existed.
136+
return nil
137+
default:
138+
return errors.Join(keyringErr, fileErr)
139+
}
140+
}
141+
142+
func (KeyringWithFallback) PreferredLocation() Location { return LocationKeyring }
143+
144+
// File is a Backend that exclusively stores the session token in a file on disk.
145+
type File struct{}
146+
147+
func (File) Read(conf config.Root, _ *url.URL) (string, Location, error) {
148+
tok, err := conf.Session().Read()
149+
if err != nil {
150+
return "", LocationNone, err
151+
}
152+
return tok, LocationFile, nil
153+
}
154+
155+
func (File) Write(conf config.Root, _ *url.URL, token string) (Location, error) {
156+
if err := conf.Session().Write(token); err != nil {
157+
return LocationNone, err
158+
}
159+
return LocationFile, nil
160+
}
161+
162+
func (File) Delete(conf config.Root, _ *url.URL) error {
163+
return conf.Session().Delete()
164+
}
165+
166+
func (File) PreferredLocation() Location { return LocationFile }

0 commit comments

Comments
 (0)