Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
52 changes: 48 additions & 4 deletions internal/clierrors/missing_credentials.go
Original file line number Diff line number Diff line change
@@ -1,18 +1,62 @@
package clierrors

import "fmt"
import (
"strings"
)

var _ ClientError = MissingCredentialsError{}

type MissingCredentialsError struct {
ConfigFile string
ServiceName string
ConfigFile string
ServiceName string
KeyringError error // New field to track keyring issues
}

func (err MissingCredentialsError) ErrorCode() int {
return MissingCredentials
}

func (err MissingCredentialsError) Error() string {
return fmt.Sprintf("user credentials not found, these must be set in config file (%s) or via environment variables or in a keyring (%s)", err.ConfigFile, err.ServiceName)
var msg strings.Builder

msg.WriteString("Authentication credentials not found.\n\n")

// Check if this is due to keyring issues
if err.KeyringError != nil && isKeyringError(err.KeyringError) {
msg.WriteString("System keyring is not accessible. ")
msg.WriteString("You can:\n")
msg.WriteString(" 1. Save token to file: upctl account login --with-token --save-to-file\n")
msg.WriteString(" 2. Set environment variable: export UPCLOUD_TOKEN=your-token\n")
msg.WriteString(" 3. Add to config file: " + err.ConfigFile + "\n\n")
msg.WriteString("Original error: " + err.KeyringError.Error())
} else {
msg.WriteString("Please configure authentication using one of these methods:\n")
msg.WriteString(" 1. Login with token: upctl account login --with-token\n")
msg.WriteString(" 2. Set environment variable: export UPCLOUD_TOKEN=your-token\n")
msg.WriteString(" 3. Add to config file: " + err.ConfigFile + "\n")

if err.ServiceName != "" {
msg.WriteString("\nNote: System keyring service '" + err.ServiceName + "' may not be available.\n")
msg.WriteString("Use --save-to-file flag with login command if keyring fails.")
}
}

return msg.String()
}

func isKeyringError(err error) bool {
if err == nil {
return false
}
errStr := strings.ToLower(err.Error())
keyringIndicators := []string{
"keyring", "secret", "dbus", "collection",
"unlock", "gnome-keyring", "kwallet",
}
for _, indicator := range keyringIndicators {
if strings.Contains(errStr, indicator) {
return true
}
}
return false
}
66 changes: 60 additions & 6 deletions internal/commands/account/login.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import (
"bufio"
"fmt"
"os"
"strings"

"github.com/UpCloudLtd/upcloud-cli/v3/internal/commands"
Expand All @@ -16,22 +17,26 @@
return &loginCommand{
BaseCommand: commands.New(
"login",
"Configure an authentication token to the system keyring (EXPERIMENTAL) ",
"Configure an authentication token to the system keyring or credentials file",
"upctl account login --with-token",
"upctl account login --with-token --save-to-file",
),
}
}

type loginCommand struct {
*commands.BaseCommand

withToken config.OptionalBoolean
withToken config.OptionalBoolean
saveToFile bool
}

// InitCommand implements Command.InitCommand
func (s *loginCommand) InitCommand() {
fs := &pflag.FlagSet{}
config.AddToggleFlag(fs, &s.withToken, "with-token", false, "Read token from standard input.")
fs.BoolVar(&s.saveToFile, "save-to-file", false,
"Save token to credentials file (~/.config/upcloud/credentials) instead of keyring when keyring is unavailable")
Comment on lines +38 to +39
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's look into how similar tools handle this situation and find out if there are some common practices 👀

For example, GitHub CLI, which does authentication very nicely, seems to (based on documentation) fallback to saving the credentials to configuration file, but not sure if this requires using --insecure-storage flag or is the default when keyring access fails (gh auth login docs) 🤔

Copy link
Copy Markdown
Contributor Author

@mgajda mgajda Jan 2, 2026

Choose a reason for hiding this comment

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

I took example of AWS CLI, which uses a separate credentials and configuration files. This allows all to read configuration, but only the user can read credentials.

Saving without explicit override means defaulting to potentially insecure behaviour, which should be discouraged.

s.AddFlags(fs)

// Require the with-token flag until we support using browser to authenticate.
Expand All @@ -56,15 +61,64 @@
if err != nil {
return nil, fmt.Errorf("failed to read token from standard input: %w", err)
}
token = strings.TrimSpace(token)

msg := "Saving provided token to the system keyring."
exec.PushProgressStarted(msg)
err = config.SaveTokenToKeyring(strings.TrimSpace(token))
if err != nil {
// Try keyring first (unless explicitly using file)
if !s.saveToFile {
msg := "Saving provided token to the system keyring."
exec.PushProgressStarted(msg)
err = config.SaveTokenToKeyring(token)

if err == nil {
exec.PushProgressSuccess(msg)
return output.None{}, nil
}

// Keyring failed
if config.IsKeyringError(err) {
var errMsg strings.Builder
errMsg.WriteString("System keyring is not accessible.\n\n")

// Provide system-specific hints
if os.Getenv("WSL_DISTRO_NAME") != "" {

Check failure on line 83 in internal/commands/account/login.go

View workflow job for this annotation

GitHub Actions / golangci-lint

ifElseChain: rewrite if-else to switch statement (gocritic)
errMsg.WriteString(" (WSL detected - keyring typically doesn't work in WSL)\n")
} else if os.Getenv("SSH_CONNECTION") != "" {
errMsg.WriteString(" (SSH session detected - keyring may not be available)\n")
} else if os.Getenv("DISPLAY") == "" && os.Getenv("WAYLAND_DISPLAY") == "" {
errMsg.WriteString(" (No display detected - keyring requires GUI or D-Bus)\n")
}

errMsg.WriteString("\nTo save your token, you can:\n")
errMsg.WriteString(" 1. Retry with file storage:\n")
errMsg.WriteString(" upctl account login --with-token --save-to-file\n\n")
errMsg.WriteString(" 2. Set environment variable:\n")
errMsg.WriteString(" export UPCLOUD_TOKEN=" + token + "\n\n")
errMsg.WriteString(" 3. Manually add to config file (~/.config/upctl.yaml):\n")
errMsg.WriteString(" token: " + token + "\n\n")
errMsg.WriteString("For security, tokens are not automatically saved to files without --save-to-file flag.")

return commands.HandleError(exec, msg, fmt.Errorf("%s\n\nOriginal error: %w", errMsg.String(), err))
}

// Non-keyring error
return commands.HandleError(exec, msg, err)
}

// User explicitly requested file storage
msg := "Saving token to credentials file."
exec.PushProgressStarted(msg)

credPath, saveErr := config.SaveTokenToCredentialsFile(token)
if saveErr != nil {
errMsg := fmt.Sprintf("Failed to save token to file: %v\n\nYou can still use environment variable:\n export UPCLOUD_TOKEN=%s", saveErr, token)
return commands.HandleError(exec, msg, fmt.Errorf(errMsg))

Check failure on line 114 in internal/commands/account/login.go

View workflow job for this annotation

GitHub Actions / golangci-lint

SA1006: printf-style function with dynamic format string and no further arguments should use print-style function instead (staticcheck)

Check failure on line 114 in internal/commands/account/login.go

View workflow job for this annotation

GitHub Actions / golangci-lint

printf: non-constant format string in call to fmt.Errorf (govet)

Check failure on line 114 in internal/commands/account/login.go

View workflow job for this annotation

GitHub Actions / Run unit-tests (macos-latest)

non-constant format string in call to fmt.Errorf

Check failure on line 114 in internal/commands/account/login.go

View workflow job for this annotation

GitHub Actions / Run unit-tests (ubuntu-latest)

non-constant format string in call to fmt.Errorf
}

exec.PushProgressSuccess(msg)
fmt.Fprintf(s.Cobra().OutOrStderr(), "\nToken saved to: %s\n", credPath)
fmt.Fprintf(s.Cobra().OutOrStderr(), "File permissions: 0600 (read/write for owner only)\n\n")
fmt.Fprintf(s.Cobra().OutOrStderr(), "This credentials file can be shared by other UpCloud tools.\n")
fmt.Fprintf(s.Cobra().OutOrStderr(), "You can now use: upctl account show\n")

return output.None{}, nil
}
64 changes: 49 additions & 15 deletions internal/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package config
import (
"context"
"fmt"
"os"
"path/filepath"
"regexp"
"runtime/debug"
Expand Down Expand Up @@ -65,11 +66,12 @@ type GlobalFlags struct {

// Config holds the configuration for running upctl
type Config struct {
viper *viper.Viper
flagSet *pflag.FlagSet
cancel context.CancelFunc
context context.Context //nolint: containedctx // This is where the top-level context is stored
GlobalFlags GlobalFlags
viper *viper.Viper
flagSet *pflag.FlagSet
cancel context.CancelFunc
context context.Context //nolint: containedctx // This is where the top-level context is stored
GlobalFlags GlobalFlags
keyringError error // Track keyring access errors for better error messages
}

// Load loads config and sets up service
Expand Down Expand Up @@ -98,15 +100,43 @@ func (s *Config) Load() error {
}
}

creds, err := credentials.Parse(credentials.Credentials{
Username: v.GetString("username"),
Password: v.GetString("password"),
Token: v.GetString("token"),
})
if err == nil {
v.Set("username", creds.Username)
v.Set("password", creds.Password)
v.Set("token", creds.Token)
// Try to load from credentials file first (before trying keyring)
fileCreds, fileErr := LoadCredentialsFile()
if fileErr != nil && !os.IsNotExist(fileErr) {
logger.Debug("Could not load credentials file", "error", fileErr)
} else if fileCreds.Token != "" || (fileCreds.Username != "" && fileCreds.Password != "") {
// Merge file credentials (config/env takes precedence)
if v.GetString("token") == "" && fileCreds.Token != "" {
v.Set("token", fileCreds.Token)
logger.Debug("Using token from credentials file")
}
if v.GetString("username") == "" && fileCreds.Username != "" {
v.Set("username", fileCreds.Username)
}
if v.GetString("password") == "" && fileCreds.Password != "" {
v.Set("password", fileCreds.Password)
}
}

// Only try keyring if we don't have credentials yet
needsKeyring := v.GetString("token") == "" &&
(v.GetString("username") == "" || v.GetString("password") == "")

if needsKeyring {
creds, err := credentials.Parse(credentials.Credentials{
Username: v.GetString("username"),
Password: v.GetString("password"),
Token: v.GetString("token"),
})
if err != nil {
// Store the keyring error for later use in error messages
s.keyringError = err
logger.Debug("Keyring access failed", "error", err)
} else {
v.Set("username", creds.Username)
v.Set("password", creds.Password)
v.Set("token", creds.Token)
}
}

v.Set("config", v.ConfigFileUsed())
Expand Down Expand Up @@ -213,7 +243,11 @@ func (s *Config) CreateService() (internal.AllServices, error) {
if s.GetString("config") != "" {
configDetails = fmt.Sprintf("used %s", s.GetString("config"))
}
return nil, clierrors.MissingCredentialsError{ConfigFile: configDetails, ServiceName: credentials.KeyringServiceName}
return nil, clierrors.MissingCredentialsError{
ConfigFile: configDetails,
ServiceName: credentials.KeyringServiceName,
KeyringError: s.keyringError,
}
}

configs := []client.ConfigFn{
Expand Down
Loading
Loading