Skip to content
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.

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

Trying to retrieve token from Keyring causes an error on WSL2 #39

Closed
s3i7h opened this issue Apr 13, 2024 · 5 comments · Fixed by #51
Closed

Trying to retrieve token from Keyring causes an error on WSL2 #39

s3i7h opened this issue Apr 13, 2024 · 5 comments · Fixed by #51
Assignees

Comments

@s3i7h
Copy link

s3i7h commented Apr 13, 2024

Although WSL provides a lot of features available in Linux, dbus is not well-supported (microsoft/WSL#8842) and causes the following error:

ERROR Secure storage error: Secret Service returned an error: no secret service provider or dbus session found

and even if you work-around it (by logging into wsl with wsl.exe -u root -- su <USER> ) and created the proper dbus, there will be another error:

ERROR Secure storage error: Secret Service returned an error: zbus error: org.freedesktop.DBus.Error.ServiceUnknown: The name org.freedesktop.secrets was not provided by any .service files

Currently gh-config's implementation returns Err when Keyring returns an error as self.retrieve_token_secure(hostname)?, but I would personally like gh-config to fall back to retrieving tokens naively from yaml files. I'm not sure how to properly handle this error so I'd appreciate thoughts over this issue. Maybe returning Ok(None) when Keyring fails connecting to dbus or fails to find a service?

gh-config-rs/src/lib.rs

Lines 172 to 184 in a8157c0

/// Retrieves a token from the secure storage or insecure storage.
/// User interaction may be required to unlock the keychain, depending on the OS.
/// If any token found for the hostname, returns None.
#[allow(deprecated)]
pub fn retrieve_token(&self, hostname: &str) -> Result<Option<String>, Error> {
Ok(self.retrieve_token_secure(hostname)?.or_else(|| {
self.get(hostname)
.and_then(|h| match h.oauth_token.is_empty() {
true => None,
_ => Some(h.oauth_token.to_owned()),
})
}))
}

@siketyan
Copy link
Owner

I think ghr should also support the fallback to non-secure storage if the official gh CLI support it. I am going to de-deprecate retrive_token in gh-config-rs then switch to use it in ghr.

@s3i7h
Copy link
Author

s3i7h commented Apr 14, 2024

After taking a look at:
https://github.com/cli/cli/blob/trunk/pkg/cmd/auth/token/token.go
https://github.com/cli/cli/blob/trunk/internal/config/config.go
https://github.com/cli/go-gh/blob/trunk/pkg/auth/auth.go
(go-gh is the internal library cli/cli uses to retrieve auth tokens naively from env/yaml)

I rethinked about how to retrieve a token and use it to communicate with github. gh-cli delegates how to retrieve auth token naively to go-gh, and tries to retrieve token from secure storage last.

Acknowledging the above, I believe the concise way to handle tokens should be:

gh-config (aims to be consistent with cli/cli and cli/go-gh): (go-gh and cli both ignores any error occurred while searching tokens)

  1. from env (GH_ENTERPRISE_TOKEN, GITHUB_ENTERPRIZE_TOKEN, GH_TOKEN, GITUB_TOKEN)
  2. from yaml
  3. from secret-service
  4. from gh auth token (backup strategy for changes in gh auth architecture)
    (there's a different proposal to support per user token but I'll make it a different issue)

tools using this library (e.g. ghr):

  1. from env (TOOL_TOKEN, GITHUB_TOKEN)
  2. from config in plain text
  3. from config via arbitrary command e.g. gh auth token, security find-generic-password (this is an example feature that the tool may provide)
  4. from any means the tool can access on it's own
  5. from gh-config
  6. ...other ways the tool may prioritize lower than gh-config

and use the first available token

I think this guideline might be helpful to make decisions on future development of this library and tools that use this library.

@s3i7h
Copy link
Author

s3i7h commented Apr 14, 2024

Regarding the above, will there be a problem if I made a PR to support this?

@siketyan
Copy link
Owner

siketyan commented May 6, 2024

@s3i7h Sorry for the late response. I prepared a PR to add support of the env variables in retrieve_token or dedicated retrieve_token_from_env. Could you please review the PR to confirm the changes will resolve this issue?

@s3i7h
Copy link
Author

s3i7h commented May 9, 2024

sure thing! I'll get to it tonight. Thank you for the implementation

siketyan added a commit that referenced this issue May 25, 2024
feat!: Revert deprecating retrieve_token, adding support of environment variables
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants