Store authentication token in encrypted keyring

[mislav]

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 is stored in the system keyring (encrypted storage) instead:

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

I've 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 or gh auth refresh, 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 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.

Fixes #449
Depends on cli/go-gh#107

[reegnz]

Any reason why you are not using https://github.com/99designs/keyring besides the cgo problem? The zalando one forks a security command on MacOS, which is not a secure practice really.

I have to grant access to the security cli for the github auth token access, and security can then be invoked with any other shell script after that, losing control of who I grant access to those creds.
The 99designs lib does not have this problem, as it uses native API-s, so MacOS would prompt me to grant access to gh only.

Using the security cli tool directly opens up people's hosts to malicious shell scripts also being able to use the security cli tool and gaining access to the credentials, partially defeating the purpose of storing those secrets in the keychain.

[reegnz]

Checking a bit with the cgo with 99designs, it's only required for macos. None of the other arch needs cgo. With macos builds I think it makes sense to build on MacOS though, as this security feature is more important than being able to build the mac binary on linux, as you're throwing away the security advantage of keychain in the process.

[samcoe]

@reegnz I believe the CGO is the main reason Mislav chose not to use the 99design/keyring package. I am not sure I am following how shelling out to security opens up people's hosts to malicious shell scripts though, could you elaborate on that? It seems to me that security is a tool that already exists on MacOS so gh utilizing it wouldn't open up any security holes that do not already exist.

[reegnz]

On macos when a program accesses a secret in the keychain it prompts you to grant access to the caller application to the given secret. It prompts for your OS password. For convenience you can allow access to that specific secret permanently so the program can access the secret withput havong to type the password on every access.

If you use the security cli, you're allowing access to the security binary, not gh, and this leads to weakened security, as now I cannot restrict access to the secret to only gh having passwordless access.

This leads to a bad pattern, as now access to the secret is not scoped to your app. The more apps shell out to security the more access you need to grant that single tool.

This leads to security having broad access to secrets of unrelated use-cases, and instead of having a clean control in the form of an access matrix of what program accessing which secrets, you now grant access to a single binary no matter the use-case.

If you use the security cli just for github cli this is not a pronlem, but it perpetuates a pattern that weakens the security on macos.

[samcoe]

@reegnz Thanks for explaining, I understand your concern now. I think it is definitely a valid concern and have taken it back to the team to discuss further.

[reegnz]

TBQH even using security cli in this flawed way is still much better than plaintext files, so don't let it block rolling out this change completely. I just wanted to highlight that it doesn't fit correctly with the MacOS security model. Eventually improving the implementation to use the native API-s instead of shelling out to security would be ideal.

[samcoe]

@reegnz We agree that the security approach is a bit flawed and could be improved, but is better than plaintext files. We also decided that we do not have the bandwidth at the moment to use a library that requires CGO. Our release process would need a major overhaul to support this library and we are trying to address this security concern in a timely manner as it has been outstanding since the start of this project. We hope that in the future we can improve our implementation and use the native API's instead as you suggested.

[reegnz]

Thank you, most important thing is awareness that there's some points of improvement that could/should be made at a future date.

[samcoe]

Closing in favor of #7043