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

Keyring v3 #7043

Merged
merged 1 commit into from
Feb 28, 2023
Merged

Keyring v3 #7043

merged 1 commit into from
Feb 28, 2023

Conversation

samcoe
Copy link
Contributor

@samcoe samcoe commented Feb 26, 2023

Previously, upon completing gh auth login, GitHub CLI was storing the GitHub API authentication token in its ~/.config/gh/hosts.yml file:

github.com:
    user: mislav
    oauth_token: <REDACTED>
    git_protocol: https

This introduces a change where the token can now be stored in the system keyring (encrypted storage) instead:

  • Keychain on macOS
  • GNOME Keyring on Linux (Secret Service dbus interface)
  • Wincred on Windows

We have chosen the zalando/go-keyring library for this because it's pure Go, i.e. does not require cgo to be enabled. Currently we disable cgo in our precompiled binaries to increase portability.

If none of the storage providers were found, or the store operation failed, the token will be written to the config file as before to ensure as little disruption to our users as possible should something go wrong with the new approach.

After this change, after completing gh auth login --secure-storage or gh auth refresh --secure-storage, the config file will look like this:

github.com:
    user: mislav
    git_protocol: https

Note the omission of the oauth_token key. Instead, the key is saved in encrypted storage. Note that depending on the OS and circumstances, gh communicating with encrypted store might spawn a system dialog to approve access. The recommended setting is to “always allow”.

Those who use environment variables (e.g. GH_TOKEN) to authenticate gh will not be affected, as in the case of environment variables, the token is neither read nor stored in the config file.

⚠️ This feature is currently opt-in using the --secure-storage flag but the intention is that this will be the default and only option in the near future. The reason for this intermediate step is to decrease the risk to extensions which is outlined below.

⚠️ This feature has an obvious benefit (more secure token storage), but a big downside: all Go extensions that were relying on API clients from go-gh to make API requests will be broken until they bump the go-gh version and make a new release. This is because the go-gh library often used in Go extensions reads directly from YAML config files, and after users have migrated to encrypted storage, the oauth_token key will not be found anymore.

Supersedes: #7023
Depends on: #7033
Fixes #449

@samcoe samcoe self-assigned this Feb 26, 2023
@samcoe samcoe changed the base branch from trunk to keyring-v2 February 26, 2023 05:03
@samcoe samcoe mentioned this pull request Feb 27, 2023
@samcoe samcoe marked this pull request as ready for review February 27, 2023 00:31
@samcoe samcoe requested review from a team as code owners February 27, 2023 00:31
@samcoe samcoe requested review from vilmibm and removed request for a team February 27, 2023 00:31
@cliAutomation cliAutomation added this to Needs review 🤔 in The GitHub CLI Feb 27, 2023
@@ -242,16 +242,17 @@ func handleRequests(ctx context.Context, server *Server, channel ssh.Channel, re
errc := make(chan error, 1)
go func() {
for req := range reqs {
if req.WantReply {
if err := req.Reply(true, nil); err != nil {
r := req
Copy link
Contributor Author

@samcoe samcoe Feb 27, 2023

Choose a reason for hiding this comment

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

This is an unrelated change but was giving a diagnostics lint warning that we were using req in the function call to forwardStream. Using the loop iteration variable in function calls like that is a common source of Go bugs.

Copy link
Contributor

@vilmibm vilmibm left a comment

Choose a reason for hiding this comment

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

thanks for taking this work over 🙏

pkg/cmd/auth/login/login_test.go Outdated Show resolved Hide resolved
The GitHub CLI automation moved this from Needs review 🤔 to Needs to be merged 🎉 Feb 27, 2023
Base automatically changed from keyring-v2 to trunk February 27, 2023 23:51
@samcoe samcoe enabled auto-merge (squash) February 27, 2023 23:54
@samcoe samcoe merged commit df83dc2 into trunk Feb 28, 2023
The GitHub CLI automation moved this from Needs to be merged 🎉 to Pending Release 🥚 Feb 28, 2023
@samcoe samcoe deleted the keyring-v3 branch February 28, 2023 00:04
@Esiakha

This comment was marked as spam.

@Esiakha

This comment was marked as spam.

rwjblue added a commit to rwjblue/test-github-device-flow-auth that referenced this pull request Feb 8, 2024
This isn't *great*, but it does significantly improve local developer
ergonomics.

Prior to this change, things worked just fine (WRT the keychain parts),
but the ergonomics kinda sucked. This is because, by default, only the
application that writes the keychain entry is allowed to read from it.
So what would happen during development is that you'd compile, go
through the oauth device flow then realize you have to fix some bug, fix
the bug, then try to run the application again: 💥 macOS password
prompt.

The changes here in this commit work around the issue by using
`security` (the macOS built-in CLI for handling this stuff). That
results in the keychain entry always being read/written by a stable
binary so debug builds/re-builds don't have issue.

The downside here, is that "anyone" that can execute `security` can read
the password. For what it's worth, this is exactly what `gh` does. See
some references below:

- cli/cli#7043
- https://github.com/zalando/go-keyring
- cli/cli#7123
- cli/cli#7023 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
The GitHub CLI
  
Done 💤
Development

Successfully merging this pull request may close these issues.

feat: Support for storing OAuth token in encrypted keychain
4 participants