-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat(cli): add macOS support for session token keyring storage #20613
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,105 @@ | ||
| //go:build darwin | ||
|
|
||
| package sessionstore | ||
|
|
||
| import ( | ||
| "encoding/base64" | ||
| "fmt" | ||
| "io" | ||
| "os" | ||
| "os/exec" | ||
| "regexp" | ||
| "strings" | ||
| ) | ||
|
|
||
| const ( | ||
| // fixedUsername is the fixed username used for all keychain entries. | ||
| // Since our interface only uses service names, we use a constant username. | ||
| fixedUsername = "coder-login-credentials" | ||
|
|
||
| execPathKeychain = "/usr/bin/security" | ||
| notFoundStr = "could not be found" | ||
| ) | ||
|
|
||
| // operatingSystemKeyring implements keyringProvider for macOS. | ||
| // It is largely adapted from the zalando/go-keyring package. | ||
| type operatingSystemKeyring struct{} | ||
|
|
||
| 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 | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| password := base64.StdEncoding.EncodeToString([]byte(credential)) | ||
|
|
||
| cmd := exec.Command(execPathKeychain, "-i") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My first thought is that we should be calling
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| stdIn, err := cmd.StdinPipe() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err = cmd.Start(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| command := fmt.Sprintf("add-generic-password -U -s %s -a %s -w %s\n", | ||
| shellEscape(service), | ||
| shellEscape(fixedUsername), | ||
| shellEscape(password)) | ||
| if len(command) > 4096 { | ||
| return ErrSetDataTooBig | ||
| } | ||
|
|
||
| if _, err := io.WriteString(stdIn, command); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err = stdIn.Close(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return cmd.Wait() | ||
| } | ||
|
|
||
| func (operatingSystemKeyring) Get(service string) ([]byte, error) { | ||
| out, err := exec.Command( | ||
| execPathKeychain, | ||
| "find-generic-password", | ||
| "-s", service, | ||
| "-wa", fixedUsername).CombinedOutput() | ||
| if err != nil { | ||
| if strings.Contains(string(out), notFoundStr) { | ||
| return nil, os.ErrNotExist | ||
| } | ||
| return nil, err | ||
| } | ||
|
|
||
| trimStr := strings.TrimSpace(string(out)) | ||
| return base64.StdEncoding.DecodeString(trimStr) | ||
| } | ||
|
|
||
| func (operatingSystemKeyring) Delete(service string) error { | ||
| out, err := exec.Command( | ||
| execPathKeychain, | ||
| "delete-generic-password", | ||
| "-s", service, | ||
| "-a", fixedUsername).CombinedOutput() | ||
| if strings.Contains(string(out), notFoundStr) { | ||
| return os.ErrNotExist | ||
| } | ||
| return err | ||
| } | ||
|
|
||
| // shellEscape returns a shell-escaped version of the string s. | ||
| // This is adapted from github.com/zalando/go-keyring/internal/shellescape. | ||
| func shellEscape(s string) string { | ||
| if len(s) == 0 { | ||
| return "''" | ||
| } | ||
|
|
||
| pattern := regexp.MustCompile(`[^\w@%+=:,./-]`) | ||
| if pattern.MatchString(s) { | ||
| return "'" + strings.ReplaceAll(s, "'", "'\"'\"'") + "'" | ||
| } | ||
|
|
||
| return s | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| //go:build darwin | ||
|
|
||
| package sessionstore_test | ||
|
|
||
| import ( | ||
| "encoding/base64" | ||
| "os/exec" | ||
| "testing" | ||
| ) | ||
|
|
||
| const ( | ||
| execPathKeychain = "/usr/bin/security" | ||
| fixedUsername = "coder-login-credentials" | ||
| ) | ||
|
|
||
| func readRawKeychainCredential(t *testing.T, service string) []byte { | ||
| t.Helper() | ||
|
|
||
| out, err := exec.Command( | ||
| execPathKeychain, | ||
| "find-generic-password", | ||
| "-s", service, | ||
| "-wa", fixedUsername).CombinedOutput() | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| dst := make([]byte, base64.StdEncoding.DecodedLen(len(out))) | ||
| n, err := base64.StdEncoding.Decode(dst, out) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| return dst[:n] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| //go:build !windows && !darwin | ||
|
|
||
| package sessionstore_test | ||
|
|
||
| import "testing" | ||
|
|
||
| func readRawKeychainCredential(t *testing.T, _ string) []byte { | ||
| t.Fatal("not implemented") | ||
| return nil | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| package sessionstore_test | ||
|
|
||
| import ( | ||
| "encoding/json" | ||
| "errors" | ||
| "fmt" | ||
| "net/url" | ||
|
|
@@ -16,6 +17,11 @@ import ( | |
| "github.com/coder/coder/v2/cli/sessionstore" | ||
| ) | ||
|
|
||
| type storedCredentials map[string]struct { | ||
| CoderURL string `json:"coder_url"` | ||
| APIToken string `json:"api_token"` | ||
| } | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope, looks good to me. For posterity, I discovered the contents of
Comment on lines
+20
to
+23
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, re: making these tests internal
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The linter |
||
|
|
||
| // Generate a test service name for use with the OS keyring. It uses a combination | ||
| // of the test name and a nanosecond timestamp to prevent collisions. | ||
| func keyringTestServiceName(t *testing.T) string { | ||
|
|
@@ -26,8 +32,8 @@ func keyringTestServiceName(t *testing.T) string { | |
| func TestKeyring(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| if runtime.GOOS != "windows" { | ||
| t.Skip("linux and darwin are not supported yet") | ||
| if runtime.GOOS != "windows" && runtime.GOOS != "darwin" { | ||
| t.Skip("linux is not supported yet") | ||
| } | ||
|
|
||
| // This test exercises use of the operating system keyring. As a result, | ||
|
|
@@ -199,6 +205,66 @@ func TestKeyring(t *testing.T) { | |
| err = backend.Delete(srvURL2) | ||
| require.NoError(t, err) | ||
| }) | ||
|
|
||
| t.Run("StorageFormat", func(t *testing.T) { | ||
| t.Parallel() | ||
| // The storage format must remain consistent to ensure we don't break | ||
| // compatibility with other Coder related applications that may read | ||
| // or decode the same credential. | ||
|
|
||
| const testURL1 = "http://127.0.0.1:1337" | ||
| srv1URL, err := url.Parse(testURL1) | ||
| require.NoError(t, err) | ||
|
|
||
| const testURL2 = "http://127.0.0.1:1338" | ||
| srv2URL, err := url.Parse(testURL2) | ||
| require.NoError(t, err) | ||
|
|
||
| serviceName := keyringTestServiceName(t) | ||
| backend := sessionstore.NewKeyringWithService(serviceName) | ||
| t.Cleanup(func() { | ||
| _ = backend.Delete(srv1URL) | ||
| _ = backend.Delete(srv2URL) | ||
| }) | ||
|
|
||
| // Write token for server 1 | ||
| const token1 = "token-server-1" | ||
| err = backend.Write(srv1URL, token1) | ||
| require.NoError(t, err) | ||
|
|
||
| // Write token for server 2 (should NOT overwrite server 1's token) | ||
| const token2 = "token-server-2" | ||
| err = backend.Write(srv2URL, token2) | ||
| require.NoError(t, err) | ||
|
|
||
| // Verify both credentials are stored in the raw format and can | ||
| // be extracted through the Backend API. | ||
| rawCredential := readRawKeychainCredential(t, serviceName) | ||
|
|
||
| storedCreds := make(storedCredentials) | ||
| err = json.Unmarshal(rawCredential, &storedCreds) | ||
| require.NoError(t, err, "unmarshalling stored credentials") | ||
|
|
||
| // Both credentials should exist | ||
| require.Len(t, storedCreds, 2) | ||
| require.Equal(t, token1, storedCreds[srv1URL.Host].APIToken) | ||
| require.Equal(t, token2, storedCreds[srv2URL.Host].APIToken) | ||
|
|
||
| // Read individual credentials | ||
| token, err := backend.Read(srv1URL) | ||
| require.NoError(t, err) | ||
| require.Equal(t, token1, token) | ||
|
|
||
| token, err = backend.Read(srv2URL) | ||
| require.NoError(t, err) | ||
| require.Equal(t, token2, token) | ||
|
|
||
| // Cleanup | ||
| err = backend.Delete(srv1URL) | ||
| require.NoError(t, err) | ||
| err = backend.Delete(srv2URL) | ||
| require.NoError(t, err) | ||
| }) | ||
| } | ||
|
|
||
| func TestFile(t *testing.T) { | ||
|
|
||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).