Skip to content

feat(auth): add --no-keyring flag to address keyring access issues (#643) #653

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mgajda wants to merge 3 commits into from
Closed

feat(auth): add --no-keyring flag to address keyring access issues (#643) #653

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ece4336
feat(auth): add --no-keyring flag to skip keyring access
mgajda Dec 31, 2025
75ff2af
feat(auth): improve keyring error messages with helpful hints
mgajda Dec 31, 2025
1273d77
fix: apply go fmt formatting
mgajda Dec 31, 2025
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
43 changes: 36 additions & 7 deletions internal/commands/account/login.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/UpCloudLtd/upcloud-cli/v3/internal/config"
"github.com/UpCloudLtd/upcloud-cli/v3/internal/output"
"github.com/spf13/pflag"
"github.com/spf13/viper"
)

// LoginCommand creates the "account login" command
Expand Down Expand Up @@ -57,14 +58,42 @@ func (s *loginCommand) executeWithToken(exec commands.Executor) (output.Output,
return nil, fmt.Errorf("failed to read token from standard input: %w", err)
}

msg := "Saving provided token to the system keyring."
exec.PushProgressStarted(msg)
err = config.SaveTokenToKeyring(strings.TrimSpace(token))
if err != nil {
return commands.HandleError(exec, msg, err)
}
token = strings.TrimSpace(token)

exec.PushProgressSuccess(msg)
// Check if keyring is disabled via global flag or config
// Note: We check the flag directly because the config isn't loaded for offline commands
noKeyring, _ := s.Cobra().Root().PersistentFlags().GetBool("no-keyring")
// Also check if it was set in config via viper (if available)
if !noKeyring && s.Cobra().Root().PersistentFlags().Changed("config") {
// Config file was specified, check if no-keyring is set there
v := viper.New()
configFile, _ := s.Cobra().Root().PersistentFlags().GetString("config")
if configFile != "" {
v.SetConfigFile(configFile)
v.SetConfigType("yaml")
_ = v.ReadInConfig()
noKeyring = v.GetBool("no-keyring")
}
}
if noKeyring {
Copy link
Member

@kangasta kangasta Dec 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Instead of checking for --no-keyring we could write to a config file if keyring access failed because of a specific error. In that case an warning should be printed.

Copy link
Contributor Author

@mgajda mgajda Dec 31, 2025

Choose a reason for hiding this comment

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

In case keyring is available the token is put in there unless --no-keyring option is present. The PR also fixes an example when --config option was ignored.

// Get config file path from flag if specified
configFile, _ := s.Cobra().Root().PersistentFlags().GetString("config")
msg := "Saving token to configuration file (keyring disabled)."
Copy link
Member

@kangasta kangasta Dec 31, 2025

Choose a reason for hiding this comment

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

We use the token saved with upctl in our other tooling as well (see credentials helper), so this should be saved in more generic location (e.g. UpCloud level config instead of UpCloud CLI level) 🤔

Copy link
Contributor Author

@mgajda mgajda Dec 31, 2025

Choose a reason for hiding this comment

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

Should we open a separate issue for this? Such a separate place should also have a config option to alter it.

exec.PushProgressStarted(msg)
err = config.SaveTokenToConfigFile(token, configFile)
if err != nil {
return commands.HandleError(exec, msg, err)
}
exec.PushProgressSuccess(msg)
} else {
msg := "Saving provided token to the system keyring."
exec.PushProgressStarted(msg)
err = config.SaveTokenToKeyring(token)
if err != nil {
return commands.HandleError(exec, msg, err)
}
exec.PushProgressSuccess(msg)
}

return output.None{}, nil
}
94 changes: 84 additions & 10 deletions internal/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ type GlobalFlags struct {
OutputFormat string `valid:"in(human|json|yaml)"`
NoColours OptionalBoolean
ForceColours OptionalBoolean
NoKeyring bool `valid:"-"`
}

// Config holds the configuration for running upctl
Expand Down Expand Up @@ -98,15 +99,26 @@ 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)
// Check for no-keyring setting from both flag and config file
noKeyring := s.GlobalFlags.NoKeyring || v.GetBool("no-keyring")

// Only attempt to parse credentials with keyring if no-keyring is not set
if !noKeyring {
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)
} else {
// Log keyring error but don't fail - credentials might be provided differently
// Wrap the error with helpful hints for debugging
wrappedErr := WrapKeyringError(err)
logger.Debug("Failed to parse credentials from keyring", "error", wrappedErr)
}
}

v.Set("config", v.ConfigFileUsed())
Expand Down Expand Up @@ -144,6 +156,11 @@ func (s *Config) GetString(key string) string {
return s.viper.GetString(key)
}

// GetBool is a convenience method of getting a configuration value in the current namespace as a boolean
func (s *Config) GetBool(key string) bool {
return s.viper.GetBool(key)
}

// FlagByKey returns pflag.Flag associated with a key in config
func (s *Config) FlagByKey(key string) *pflag.Flag {
if s.flagSet == nil {
Expand Down Expand Up @@ -242,7 +259,64 @@ func GetVersion() string {
}

func SaveTokenToKeyring(token string) error {
return keyring.Set(credentials.KeyringServiceName, credentials.KeyringTokenUser, token)
err := keyring.Set(credentials.KeyringServiceName, credentials.KeyringTokenUser, token)
if err != nil {
return WrapKeyringError(err)
}
return nil
}

// WrapKeyringError adds helpful hints to keyring errors
func WrapKeyringError(err error) error {
if err == nil {
return nil
}

// Check for common keyring error patterns
errStr := err.Error()
if strings.Contains(errStr, "failed to unlock") ||
strings.Contains(errStr, "keyring") ||
strings.Contains(errStr, "secret") ||
strings.Contains(errStr, "dbus") ||
strings.Contains(errStr, "collection") {
return fmt.Errorf("%w\n\nHint: If you're experiencing keyring issues, you can:\n 1. Use the --no-keyring flag to save credentials to config file instead\n 2. Set 'no-keyring: true' in your upctl.yaml config file\n 3. Use environment variables UPCLOUD_TOKEN for authentication", err)
}

return err
}

func SaveTokenToConfigFile(token string, configFile string) error {
// Read existing config or create new one
v := viper.New()
v.SetConfigType("yaml")

if configFile != "" {
// Use the specified config file
v.SetConfigFile(configFile)
} else {
// Use default locations
v.SetConfigName("upctl")
v.AddConfigPath(xdg.ConfigHome)
v.AddConfigPath("$HOME/.config")
}

// Try to read existing config (ignore errors if not found)
_ = v.ReadInConfig()

// Set the token
v.Set("token", token)

// Determine config file path for writing
if configFile == "" {
configFile = v.ConfigFileUsed()
if configFile == "" {
// Config file doesn't exist, create it in XDG config home
configFile = filepath.Join(xdg.ConfigHome, "upctl.yaml")
}
}

// Write the config file
return v.WriteConfigAs(configFile)
}

func getVersion() string {
Expand Down
4 changes: 4 additions & 0 deletions internal/core/core.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,10 @@ func BuildRootCmd(conf *config.Config) cobra.Command {
&conf.GlobalFlags.Debug, "debug", false,
"Print out more verbose debug logs.",
)
flags.BoolVar(
&conf.GlobalFlags.NoKeyring, "no-keyring", false,
"Disable system keyring integration for credential storage.",
)
Comment on lines +105 to +108
Copy link
Member

@kangasta kangasta Dec 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Comment from #643 to make it visible for other reviewers:

There should be no need for --no-keyring flag: keyring has least priority when parsing the credentials, so with --no-keyring user would just get a slightly different error message. I.e., if credentials are defined in environment variables, the keyring should not be accessed. Maybe instead of adding --no-keyring flag the keyring access failed error message could be clarified.

For login --with-token, the --no-keyring flag would not make sense as we do not currently have any alternative methods for saving the token. One alternative would be to save the token to upctl configuration file, but in that case the parameter should be about allowing this less secure fallback.

flags.DurationVarP(
&conf.GlobalFlags.ClientTimeout, "client-timeout", "t",
0,
Expand Down
Loading