feat(cli): add macOS support for session token keyring storage #20613
Conversation
| func (operatingSystemKeyring) Set(service, credential string) error { | ||
| // if the added secret has multiple lines or some non ascii, | ||
| // macOS will hex encode it on return. To avoid getting garbage, we | ||
| // encode all passwords |
There was a problem hiding this comment.
@ethanndickson what are your thoughts on keeping this base64 encoding/decoding? It's borrowed from https://github.com/zalando/go-keyring/. I don't think we explicitly need it given our token format.
There was a problem hiding this comment.
I'm pretty sure we have a bunch of users who populate Coder login credentials from a script. It doesn't seem impossible for users to accidentally add extra newlines or something if they populate this new format, so I think we should keep it.
| type storedCredentials map[string]struct { | ||
| CoderURL string `json:"coder_url"` | ||
| APIToken string `json:"api_token"` | ||
| } |
There was a problem hiding this comment.
@ethanndickson this is essentially what gets stored as a JSON blob (b64 encoded - see my other comment). Looks like this in my keychain. Any problems/concerns for Coder Desktop on macOS, in terms of future session token sharing?
There was a problem hiding this comment.
Nope, looks good to me. For posterity, I discovered the contents of Access Control are more or less irrelevant in this case, as the user will always be prompted when another app (i.e. Coder Desktop) tries to read from the keychain item.
Add support for storing the CLI session token in the OS keyring on macOS when the --use-keyring flag is provided.
af850f4 to
dc96f84
Compare
| fixedUsername = "coder-session-token" | ||
|
|
||
| execPathKeychain = "/usr/bin/security" | ||
| notFoundStr = "could not be found" |
There was a problem hiding this comment.
I see it's what Zalando does, and it's cosmetic but this seems broken. Wouldn't the error string be localized?
There was a problem hiding this comment.
I did a little digging on this and it appears the security binary isn't localized. I tried setting a different locale and it made no difference. No issues reported for the Github CLI about this (uses the zalando package under the hood).
|
|
||
| // fixedUsername is the fixed username used for all keychain entries. | ||
| // Since our interface only uses service names, we use a constant username. | ||
| fixedUsername = "coder-session-token" |
There was a problem hiding this comment.
Given that it's not just the session token, should we maybe call this coder-login-credentials?
| // encode all passwords | ||
| password := base64.StdEncoding.EncodeToString([]byte(credential)) | ||
|
|
||
| cmd := exec.Command(execPathKeychain, "-i") |
There was a problem hiding this comment.
My first thought is that we should be calling exec.CommandContext with inv.Context, but these processes should all die immediately so it's probably fine?
There was a problem hiding this comment.
I think nominally this should be fine. I did see the github CLI has wrapped keyring operations with a timeout, but I am inclined to hold off on adding any timeouts/wrappers https://github.com/cli/cli/blob/85d6766c621a9d67e0d353a12bb388c40ceca2a1/internal/keyring/keyring.go#L2
| const ( | ||
| execPathKeychain = "/usr/bin/security" | ||
| fixedUsername = "coder-session-token" | ||
| ) |
There was a problem hiding this comment.
Why not make these tests internal, so we don't have to redefine these?
There was a problem hiding this comment.
This is primarily driven by other other keyring tests in the sessionstore_test package which import an os specific readRawKeychainCredential function.
| type storedCredentials map[string]struct { | ||
| CoderURL string `json:"coder_url"` | ||
| APIToken string `json:"api_token"` | ||
| } |
There was a problem hiding this comment.
Same here, re: making these tests internal
There was a problem hiding this comment.
The linter testpackage we have enabled asks for these separate test packages. The approach seems pretty reasonable for these keyring tests.
| type storedCredentials map[string]struct { | ||
| CoderURL string `json:"coder_url"` | ||
| APIToken string `json:"api_token"` | ||
| } |
There was a problem hiding this comment.
Nope, looks good to me. For posterity, I discovered the contents of Access Control are more or less irrelevant in this case, as the user will always be prompted when another app (i.e. Coder Desktop) tries to read from the keychain item.
Add support for storing the CLI session token in the OS keyring on macOS when the --use-keyring flag is provided.
#19403
https://www.notion.so/coderhq/CLI-Session-Token-in-OS-Keyring-293d579be592808b8b7fd235304e50d5