feat(auth): add --no-keyring flag to address keyring access issues (#643) #653
Conversation
Addresses issue UpCloudLtd#643 where users on WSL and ARM experience keyring access issues. Changes: - Add --no-keyring global flag to disable system keyring integration - Support no-keyring configuration option in upctl.yaml - When --no-keyring is set, save tokens to config file instead of keyring - Skip credentials.Parse() when keyring is disabled to avoid access attempts - Update account login command to save tokens to config when keyring disabled This allows users experiencing keyring issues to still use token authentication by storing tokens in the configuration file instead.
When keyring operations fail, provide clear hints about alternative options: - Using --no-keyring flag - Setting no-keyring in config file - Using environment variables This helps users quickly resolve keyring access issues on WSL, ARM, and other environments with problematic keyring support.
| flags.BoolVar( | ||
| &conf.GlobalFlags.NoKeyring, "no-keyring", false, | ||
| "Disable system keyring integration for credential storage.", | ||
| ) |
There was a problem hiding this comment.
Comment from #643 to make it visible for other reviewers:
There should be no need for
--no-keyringflag: keyring has least priority when parsing the credentials, so with--no-keyringuser 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-keyringflag the keyring access failed error message could be clarified.For
login --with-token, the--no-keyringflag 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.
| if noKeyring { | ||
| // Get config file path from flag if specified | ||
| configFile, _ := s.Cobra().Root().PersistentFlags().GetString("config") | ||
| msg := "Saving token to configuration file (keyring disabled)." |
There was a problem hiding this comment.
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) 🤔
There was a problem hiding this comment.
Should we open a separate issue for this? Such a separate place should also have a config option to alter it.
| noKeyring = v.GetBool("no-keyring") | ||
| } | ||
| } | ||
| if noKeyring { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Replaced by #654 after feedback. |
2 participants
Summary
Implements
--no-keyringflag to address issue #643 where users on WSL and ARM systems experience keyring access failures.Changes
--no-keyringglobal flag to disable system keyring integrationno-keyringconfiguration option inupctl.yamlaccount logincommand to save tokens to config file when keyring is disabledcredentials.Parse()calls when keyring is disabled to avoid access attempts--no-keyringwhen keyring operations failMotivation
Users on WSL, ARM, and other environments often encounter keyring errors like:
This PR allows them to bypass the keyring entirely and use configuration file storage instead.
Usage
# Use --no-keyring flag upctl account login --with-token --no-keyringOr set in config file:
Works with all commands
Testing
--no-keyringflag saves token to config fileno-keyring: trueworksFor #643