From df83dc2d58f242fc368fa62656d410e5fd820f6e Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Tue, 28 Feb 2023 11:04:53 +1100 Subject: [PATCH] Add ability to store tokens in encrypted storage (#7043) --- go.mod | 6 +- go.sum | 13 ++- internal/config/config.go | 33 +++++- pkg/cmd/auth/login/login.go | 46 ++++---- pkg/cmd/auth/login/login_test.go | 163 +++++++++++++++++++-------- pkg/cmd/auth/logout/logout_test.go | 57 ++++++++-- pkg/cmd/auth/refresh/refresh.go | 16 ++- pkg/cmd/auth/refresh/refresh_test.go | 36 +++++- pkg/cmd/auth/shared/login_flow.go | 27 ++--- pkg/cmd/auth/shared/writeable.go | 8 +- pkg/cmd/auth/token/token.go | 13 ++- pkg/cmd/auth/token/token_test.go | 77 ++++++++++++- pkg/liveshare/test/server.go | 9 +- 13 files changed, 386 insertions(+), 118 deletions(-) diff --git a/go.mod b/go.mod index 730410c82..234549355 100644 --- a/go.mod +++ b/go.mod @@ -9,7 +9,7 @@ require ( github.com/cenkalti/backoff/v4 v4.2.0 github.com/charmbracelet/glamour v0.5.1-0.20220727184942-e70ff2d969da github.com/charmbracelet/lipgloss v0.5.0 - github.com/cli/go-gh v1.0.0 + github.com/cli/go-gh v1.2.0 github.com/cli/oauth v1.0.1 github.com/cli/safeexec v1.0.1 github.com/cpuguy83/go-md2man/v2 v2.0.2 @@ -35,6 +35,7 @@ require ( github.com/spf13/cobra v1.6.1 github.com/spf13/pflag v1.0.5 github.com/stretchr/testify v1.7.5 + github.com/zalando/go-keyring v0.2.2 golang.org/x/crypto v0.0.0-20210921155107-089bfa567519 golang.org/x/sync v0.1.0 golang.org/x/term v0.5.0 @@ -46,13 +47,16 @@ require ( require ( github.com/alecthomas/chroma v0.10.0 // indirect + github.com/alessio/shellescape v1.4.1 // indirect github.com/aymerick/douceur v0.2.0 // indirect github.com/cli/browser v1.1.0 // indirect github.com/cli/shurcooL-graphql v0.0.2 // indirect + github.com/danieljoos/wincred v1.1.2 // indirect github.com/davecgh/go-spew v1.1.1 // indirect github.com/dlclark/regexp2 v1.4.0 // indirect github.com/fatih/color v1.7.0 // indirect github.com/gdamore/encoding v1.0.0 // indirect + github.com/godbus/dbus/v5 v5.1.0 // indirect github.com/golang/protobuf v1.5.2 // indirect github.com/gorilla/css v1.0.0 // indirect github.com/hashicorp/errwrap v1.0.0 // indirect diff --git a/go.sum b/go.sum index 6d0fb00a1..61dc0a7a8 100644 --- a/go.sum +++ b/go.sum @@ -41,6 +41,8 @@ github.com/Netflix/go-expect v0.0.0-20220104043353-73e0943537d2 h1:+vx7roKuyA63n github.com/Netflix/go-expect v0.0.0-20220104043353-73e0943537d2/go.mod h1:HBCaDeC1lPdgDeDbhX8XFpy1jqjK0IBG8W5K+xYqA0w= github.com/alecthomas/chroma v0.10.0 h1:7XDcGkCQopCNKjZHfYrNLraA+M7e0fMiJ/Mfikbfjek= github.com/alecthomas/chroma v0.10.0/go.mod h1:jtJATyUxlIORhUOFNA9NZDWGAQ8wpxQQqNSB4rjA/1s= +github.com/alessio/shellescape v1.4.1 h1:V7yhSDDn8LP4lc4jS8pFkt0zCnzVJlG5JXy9BVKJUX0= +github.com/alessio/shellescape v1.4.1/go.mod h1:PZAiSCk0LJaZkiCSkPv8qIobYglO3FPpyFjDCtHLS30= github.com/aymerick/douceur v0.2.0 h1:Mv+mAeH1Q+n9Fr+oyamOlAkUNPWPlA8PPGR0QAaYuPk= github.com/aymerick/douceur v0.2.0/go.mod h1:wlT5vV2O3h55X9m7iVYN0TBM0NH/MmbLnd30/FjWUq4= github.com/briandowns/spinner v1.18.1 h1:yhQmQtM1zsqFsouh09Bk/jCjd50pC3EOGsh28gLVvwY= @@ -60,8 +62,8 @@ github.com/cli/browser v1.1.0 h1:xOZBfkfY9L9vMBgqb1YwRirGu6QFaQ5dP/vXt5ENSOY= github.com/cli/browser v1.1.0/go.mod h1:HKMQAt9t12kov91Mn7RfZxyJQQgWgyS/3SZswlZ5iTI= github.com/cli/crypto v0.0.0-20210929142629-6be313f59b03 h1:3f4uHLfWx4/WlnMPXGai03eoWAI+oGHJwr+5OXfxCr8= github.com/cli/crypto v0.0.0-20210929142629-6be313f59b03/go.mod h1:GvvjBRRGRdwPK5ydBHafDWAxML/pGHZbMvKqRZ5+Abc= -github.com/cli/go-gh v1.0.0 h1:zE1YUAUYqGXNZuICEBeOkIMJ5F50BS0ftvtoWGlsEFI= -github.com/cli/go-gh v1.0.0/go.mod h1:bqxLdCoTZ73BuiPEJx4olcO/XKhVZaFDchFagYRBweE= +github.com/cli/go-gh v1.2.0 h1:LjcdjdQtCWXVg3YTNEuwrHFY/amJzBXy5QjMxnWB/0Q= +github.com/cli/go-gh v1.2.0/go.mod h1:Jxk8X+TCO4Ui/GarwY9tByWm/8zp4jJktzVZNlTW5VM= github.com/cli/oauth v1.0.1 h1:pXnTFl/qUegXHK531Dv0LNjW4mLx626eS42gnzfXJPA= github.com/cli/oauth v1.0.1/go.mod h1:qd/FX8ZBD6n1sVNQO3aIdRxeu5LGw9WhKnYhIIoC2A4= github.com/cli/safeexec v1.0.0/go.mod h1:Z/D4tTN8Vs5gXYHDCbaM1S/anmEDnJb1iW0+EJ5zx3Q= @@ -76,6 +78,8 @@ github.com/cpuguy83/go-md2man/v2 v2.0.2/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46t github.com/creack/pty v1.1.17/go.mod h1:MOBLtS5ELjhRRrroQr9kyvTxUAFNvYEK993ew/Vr4O4= github.com/creack/pty v1.1.18 h1:n56/Zwd5o6whRC5PMGretI4IdRLlmBXYNjScPaBgsbY= github.com/creack/pty v1.1.18/go.mod h1:MOBLtS5ELjhRRrroQr9kyvTxUAFNvYEK993ew/Vr4O4= +github.com/danieljoos/wincred v1.1.2 h1:QLdCxFs1/Yl4zduvBdcHB8goaYk9RARS2SgLLRuAyr0= +github.com/danieljoos/wincred v1.1.2/go.mod h1:GijpziifJoIBfYh+S7BbkdUTU4LfM+QnGqR5Vl2tAx0= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= @@ -96,6 +100,8 @@ github.com/gdamore/tcell/v2 v2.5.4/go.mod h1:dZgRy5v4iMobMEcWNYBtREnDZAT9DYmfqIk github.com/go-gl/glfw v0.0.0-20190409004039-e6da0acd62b1/go.mod h1:vR7hzQXu2zJy9AVAgeJqvqgH9Q5CA+iKCZ2gyEVpxRU= github.com/go-gl/glfw/v3.3/glfw v0.0.0-20191125211704-12ad95a8df72/go.mod h1:tQ2UAYgL5IevRw8kRxooKSPJfGvJ9fJQFa0TUsXzTg8= github.com/go-gl/glfw/v3.3/glfw v0.0.0-20200222043503-6f7a984d4dc4/go.mod h1:tQ2UAYgL5IevRw8kRxooKSPJfGvJ9fJQFa0TUsXzTg8= +github.com/godbus/dbus/v5 v5.1.0 h1:4KLkAxT3aOY8Li4FRJe/KvhoNFFxo0m6fNuFUO8QJUk= +github.com/godbus/dbus/v5 v5.1.0/go.mod h1:xhWf0FNVPg57R7Z0UbKHbJfkEywrmjJnf7w5xrFpKfA= github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b/go.mod h1:SBH7ygxi8pfUlaOkMMuAQtPIUF8ecWP5IEl/CR7VP2Q= github.com/golang/groupcache v0.0.0-20190702054246-869f871628b6/go.mod h1:cIg4eruTrX1D+g88fzRXU5OdNfaM+9IcxsU14FzY7Hc= github.com/golang/groupcache v0.0.0-20191227052852-215e87163ea7/go.mod h1:cIg4eruTrX1D+g88fzRXU5OdNfaM+9IcxsU14FzY7Hc= @@ -265,6 +271,8 @@ github.com/yuin/goldmark v1.4.13 h1:fVcFKWvrslecOb/tg+Cc05dkeYx540o0FuFt3nUVDoE= github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY= github.com/yuin/goldmark-emoji v1.0.1 h1:ctuWEyzGBwiucEqxzwe0SOYDXPAucOrE9NQC18Wa1os= github.com/yuin/goldmark-emoji v1.0.1/go.mod h1:2w1E6FEWLcDQkoTE+7HU6QF1F6SLlNGjRIBbIZQFqkQ= +github.com/zalando/go-keyring v0.2.2 h1:f0xmpYiSrHtSNAVgwip93Cg8tuF45HJM6rHq/A5RI/4= +github.com/zalando/go-keyring v0.2.2/go.mod h1:sI3evg9Wvpw3+n4SqplGSJUMwtDeROfD4nsFz4z9PG0= go.opencensus.io v0.21.0/go.mod h1:mSImk1erAIZhrmZN+AvHh14ztQfjbGwt4TtuofqLduU= go.opencensus.io v0.22.0/go.mod h1:+kGneAE2xo2IficOXnaByMWTGM9T73dGwxeWcUqIpI8= go.opencensus.io v0.22.2/go.mod h1:yxeiOL68Rb0Xd1ddK5vPZ/oVn4vY4Ynel7k9FzqtOIw= @@ -386,6 +394,7 @@ golang.org/x/sys v0.0.0-20210319071255-635bc2c9138d/go.mod h1:h1NjWce9XRLGQEsW7w golang.org/x/sys v0.0.0-20210423082822-04245dca01da/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20210630005230-0f9fa26af87c/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/sys v0.0.0-20210819135213-f52c844e1c1c/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20210831042530-f4d43177bf5e/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20211019181941-9d821ace8654/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20211216021012-1d35b9e2eb4e/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= diff --git a/internal/config/config.go b/internal/config/config.go index 86beec93b..ae4660977 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -6,6 +6,7 @@ import ( ghAuth "github.com/cli/go-gh/pkg/auth" ghConfig "github.com/cli/go-gh/pkg/config" + "github.com/zalando/go-keyring" ) const ( @@ -144,7 +145,15 @@ func (c *AuthConfig) Token(hostname string) (string, string) { if c.tokenOverride != nil { return c.tokenOverride(hostname) } - return ghAuth.TokenForHost(hostname) + token, source := ghAuth.TokenFromEnvOrConfig(hostname) + if token == "" { + var err error + token, err = c.TokenFromKeyring(hostname) + if err == nil { + source = "keyring" + } + } + return token, source } // SetToken will override any token resolution and return the given @@ -155,6 +164,12 @@ func (c *AuthConfig) SetToken(token, source string) { } } +// TokenFromKeyring will retrieve the auth token for the given hostname, +// only searching in encrypted storage. +func (c *AuthConfig) TokenFromKeyring(hostname string) (string, error) { + return keyring.Get(keyringServiceName(hostname), "") +} + // User will retrieve the username for the logged in user at the given hostname. func (c *AuthConfig) User(hostname string) (string, error) { return c.cfg.Get([]string{hosts, hostname, "user"}) @@ -193,8 +208,15 @@ func (c *AuthConfig) DefaultHost() (string, string) { // Login will set user, git protocol, and auth token for the given hostname. // If the encrypt option is specified it will first try to store the auth token // in encrypted storage and will fall back to the plain text config file. -func (c *AuthConfig) Login(hostname, username, token, gitProtocol string, encrypt bool) error { - if token != "" { +func (c *AuthConfig) Login(hostname, username, token, gitProtocol string, secureStorage bool) error { + var setErr error + if secureStorage { + if setErr = keyring.Set(keyringServiceName(hostname), "", token); setErr == nil { + // Clean up the previous oauth_token from the config file. + _ = c.cfg.Remove([]string{hosts, hostname, oauthToken}) + } + } + if !secureStorage || setErr != nil { c.cfg.Set([]string{hosts, hostname, oauthToken}, token) } if username != "" { @@ -213,9 +235,14 @@ func (c *AuthConfig) Logout(hostname string) error { return nil } _ = c.cfg.Remove([]string{hosts, hostname}) + _ = keyring.Delete(keyringServiceName(hostname), "") return ghConfig.Write(c.cfg) } +func keyringServiceName(hostname string) string { + return "gh:" + hostname +} + type AliasConfig struct { cfg *ghConfig.Config } diff --git a/pkg/cmd/auth/login/login.go b/pkg/cmd/auth/login/login.go index a3f80a551..3eda42ae0 100644 --- a/pkg/cmd/auth/login/login.go +++ b/pkg/cmd/auth/login/login.go @@ -30,11 +30,12 @@ type LoginOptions struct { Interactive bool - Hostname string - Scopes []string - Token string - Web bool - GitProtocol string + Hostname string + Scopes []string + Token string + Web bool + GitProtocol string + SecureStorage bool } func NewCmdLogin(f *cmdutil.Factory, runF func(*LoginOptions) error) *cobra.Command { @@ -123,6 +124,7 @@ func NewCmdLogin(f *cmdutil.Factory, runF func(*LoginOptions) error) *cobra.Comm cmd.Flags().BoolVar(&tokenStdin, "with-token", false, "Read token from standard input") cmd.Flags().BoolVarP(&opts.Web, "web", "w", false, "Open a browser to authenticate") cmdutil.StringEnumFlag(cmd, &opts.GitProtocol, "git-protocol", "p", "", []string{"ssh", "https"}, "The protocol to use for git operations") + cmd.Flags().BoolVarP(&opts.SecureStorage, "secure-storage", "", false, "Save authentication credentials in secure credential store") return cmd } @@ -134,6 +136,11 @@ func loginRun(opts *LoginOptions) error { } authCfg := cfg.Authentication() + if opts.SecureStorage { + cs := opts.IO.ColorScheme() + fmt.Fprintf(opts.IO.ErrOut, "%s Using secure storage could break installed extensions\n", cs.WarningIcon()) + } + hostname := opts.Hostname if opts.Interactive && hostname == "" { var err error @@ -158,8 +165,8 @@ func loginRun(opts *LoginOptions) error { if err := shared.HasMinimumScopes(httpClient, hostname, opts.Token); err != nil { return fmt.Errorf("error validating token: %w", err) } - - return authCfg.Login(hostname, "", opts.Token, opts.GitProtocol, false) + // Adding a user key ensures that a nonempty host section gets written to the config file. + return authCfg.Login(hostname, "x-access-token", opts.Token, opts.GitProtocol, opts.SecureStorage) } existingToken, _ := authCfg.Token(hostname) @@ -176,18 +183,19 @@ func loginRun(opts *LoginOptions) error { } return shared.Login(&shared.LoginOptions{ - IO: opts.IO, - Config: authCfg, - HTTPClient: httpClient, - Hostname: hostname, - Interactive: opts.Interactive, - Web: opts.Web, - Scopes: opts.Scopes, - Executable: opts.MainExecutable, - GitProtocol: opts.GitProtocol, - Prompter: opts.Prompter, - GitClient: opts.GitClient, - Browser: opts.Browser, + IO: opts.IO, + Config: authCfg, + HTTPClient: httpClient, + Hostname: hostname, + Interactive: opts.Interactive, + Web: opts.Web, + Scopes: opts.Scopes, + Executable: opts.MainExecutable, + GitProtocol: opts.GitProtocol, + Prompter: opts.Prompter, + GitClient: opts.GitClient, + Browser: opts.Browser, + SecureStorage: opts.SecureStorage, }) } diff --git a/pkg/cmd/auth/login/login_test.go b/pkg/cmd/auth/login/login_test.go index 168df246e..9f95cc9ee 100644 --- a/pkg/cmd/auth/login/login_test.go +++ b/pkg/cmd/auth/login/login_test.go @@ -17,6 +17,7 @@ import ( "github.com/cli/cli/v2/pkg/iostreams" "github.com/google/shlex" "github.com/stretchr/testify/assert" + "github.com/zalando/go-keyring" ) func stubHomeDir(t *testing.T, dir string) { @@ -172,6 +173,23 @@ func Test_NewCmdLogin(t *testing.T) { Interactive: true, }, }, + { + name: "tty secure-storage", + stdinTTY: true, + cli: "--secure-storage", + wants: LoginOptions{ + Interactive: true, + SecureStorage: true, + }, + }, + { + name: "nontty secure-storage", + cli: "--secure-storage", + wants: LoginOptions{ + Hostname: "github.com", + SecureStorage: true, + }, + }, } for _, tt := range tests { @@ -223,13 +241,14 @@ func Test_NewCmdLogin(t *testing.T) { func Test_loginRun_nontty(t *testing.T) { tests := []struct { - name string - opts *LoginOptions - httpStubs func(*httpmock.Registry) - cfgStubs func(*config.ConfigMock) - wantHosts string - wantErr string - wantStderr string + name string + opts *LoginOptions + httpStubs func(*httpmock.Registry) + cfgStubs func(*config.ConfigMock) + wantHosts string + wantErr string + wantStderr string + wantSecureToken string }{ { name: "with token", @@ -240,7 +259,7 @@ func Test_loginRun_nontty(t *testing.T) { httpStubs: func(reg *httpmock.Registry) { reg.Register(httpmock.REST("GET", ""), httpmock.ScopesResponder("repo,read:org")) }, - wantHosts: "github.com:\n oauth_token: abc123\n", + wantHosts: "github.com:\n oauth_token: abc123\n user: x-access-token\n", }, { name: "with token and https git-protocol", @@ -252,7 +271,7 @@ func Test_loginRun_nontty(t *testing.T) { httpStubs: func(reg *httpmock.Registry) { reg.Register(httpmock.REST("GET", ""), httpmock.ScopesResponder("repo,read:org")) }, - wantHosts: "github.com:\n oauth_token: abc123\n git_protocol: https\n", + wantHosts: "github.com:\n oauth_token: abc123\n user: x-access-token\n git_protocol: https\n", }, { name: "with token and non-default host", @@ -263,7 +282,7 @@ func Test_loginRun_nontty(t *testing.T) { httpStubs: func(reg *httpmock.Registry) { reg.Register(httpmock.REST("GET", "api/v3/"), httpmock.ScopesResponder("repo,read:org")) }, - wantHosts: "albert.wesker:\n oauth_token: abc123\n", + wantHosts: "albert.wesker:\n oauth_token: abc123\n user: x-access-token\n", }, { name: "missing repo scope", @@ -296,7 +315,7 @@ func Test_loginRun_nontty(t *testing.T) { httpStubs: func(reg *httpmock.Registry) { reg.Register(httpmock.REST("GET", ""), httpmock.ScopesResponder("repo,admin:org")) }, - wantHosts: "github.com:\n oauth_token: abc456\n", + wantHosts: "github.com:\n oauth_token: abc456\n user: x-access-token\n", }, { name: "github.com token from environment", @@ -336,15 +355,30 @@ func Test_loginRun_nontty(t *testing.T) { To have GitHub CLI store credentials instead, first clear the value from the environment. `), }, + { + name: "with token and secure storage", + opts: &LoginOptions{ + Hostname: "github.com", + Token: "abc123", + SecureStorage: true, + }, + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.REST("GET", ""), httpmock.ScopesResponder("repo,read:org")) + }, + wantHosts: "github.com:\n user: x-access-token\n", + wantSecureToken: "abc123", + wantStderr: "! Using secure storage could break installed extensions\n", + }, } for _, tt := range tests { - ios, _, stdout, stderr := iostreams.Test() - ios.SetStdinTTY(false) - ios.SetStdoutTTY(false) - tt.opts.IO = ios - t.Run(tt.name, func(t *testing.T) { + ios, _, stdout, stderr := iostreams.Test() + ios.SetStdinTTY(false) + ios.SetStdoutTTY(false) + tt.opts.IO = ios + + keyring.MockInit() readConfigs := config.StubWriteConfig(t) cfg := config.NewBlankConfig() if tt.cfgStubs != nil { @@ -355,6 +389,7 @@ func Test_loginRun_nontty(t *testing.T) { } reg := &httpmock.Registry{} + defer reg.Verify(t) tt.opts.HttpClient = func() (*http.Client, error) { return &http.Client{Transport: reg}, nil } @@ -375,11 +410,12 @@ func Test_loginRun_nontty(t *testing.T) { mainBuf := bytes.Buffer{} hostsBuf := bytes.Buffer{} readConfigs(&mainBuf, &hostsBuf) + secureToken, _ := cfg.Authentication().TokenFromKeyring(tt.opts.Hostname) assert.Equal(t, "", stdout.String()) assert.Equal(t, tt.wantStderr, stderr.String()) assert.Equal(t, tt.wantHosts, hostsBuf.String()) - reg.Verify(t) + assert.Equal(t, tt.wantSecureToken, secureToken) }) } } @@ -388,14 +424,15 @@ func Test_loginRun_Survey(t *testing.T) { stubHomeDir(t, t.TempDir()) tests := []struct { - name string - opts *LoginOptions - httpStubs func(*httpmock.Registry) - prompterStubs func(*prompter.PrompterMock) - runStubs func(*run.CommandStubber) - wantHosts string - wantErrOut *regexp.Regexp - cfgStubs func(*config.ConfigMock) + name string + opts *LoginOptions + httpStubs func(*httpmock.Registry) + prompterStubs func(*prompter.PrompterMock) + runStubs func(*run.CommandStubber) + cfgStubs func(*config.ConfigMock) + wantHosts string + wantErrOut *regexp.Regexp + wantSecureToken string }{ { name: "already authenticated", @@ -553,32 +590,62 @@ func Test_loginRun_Survey(t *testing.T) { }, wantErrOut: regexp.MustCompile("Tip: you can generate a Personal Access Token here https://github.com/settings/tokens"), }, - // TODO how to test browser auth? + { + name: "secure storage", + opts: &LoginOptions{ + Hostname: "github.com", + Interactive: true, + SecureStorage: true, + }, + prompterStubs: func(pm *prompter.PrompterMock) { + pm.SelectFunc = func(prompt, _ string, opts []string) (int, error) { + switch prompt { + case "What is your preferred protocol for Git operations?": + return prompter.IndexFor(opts, "HTTPS") + case "How would you like to authenticate GitHub CLI?": + return prompter.IndexFor(opts, "Paste an authentication token") + } + return -1, prompter.NoSuchPromptErr(prompt) + } + }, + runStubs: func(rs *run.CommandStubber) { + rs.Register(`git config credential\.https:/`, 1, "") + rs.Register(`git config credential\.helper`, 1, "") + }, + wantHosts: heredoc.Doc(` + github.com: + user: jillv + git_protocol: https + `), + wantErrOut: regexp.MustCompile("! Using secure storage could break installed extensions"), + wantSecureToken: "def456", + }, } for _, tt := range tests { - if tt.opts == nil { - tt.opts = &LoginOptions{} - } - ios, _, _, stderr := iostreams.Test() - - ios.SetStdinTTY(true) - ios.SetStderrTTY(true) - ios.SetStdoutTTY(true) - - tt.opts.IO = ios - - readConfigs := config.StubWriteConfig(t) - - cfg := config.NewBlankConfig() - if tt.cfgStubs != nil { - tt.cfgStubs(cfg) - } - tt.opts.Config = func() (config.Config, error) { - return cfg, nil - } - t.Run(tt.name, func(t *testing.T) { + if tt.opts == nil { + tt.opts = &LoginOptions{} + } + ios, _, _, stderr := iostreams.Test() + + ios.SetStdinTTY(true) + ios.SetStderrTTY(true) + ios.SetStdoutTTY(true) + + tt.opts.IO = ios + + keyring.MockInit() + readConfigs := config.StubWriteConfig(t) + + cfg := config.NewBlankConfig() + if tt.cfgStubs != nil { + tt.cfgStubs(cfg) + } + tt.opts.Config = func() (config.Config, error) { + return cfg, nil + } + reg := &httpmock.Registry{} tt.opts.HttpClient = func() (*http.Client, error) { return &http.Client{Transport: reg}, nil @@ -620,8 +687,10 @@ func Test_loginRun_Survey(t *testing.T) { mainBuf := bytes.Buffer{} hostsBuf := bytes.Buffer{} readConfigs(&mainBuf, &hostsBuf) + secureToken, _ := cfg.Authentication().TokenFromKeyring(tt.opts.Hostname) assert.Equal(t, tt.wantHosts, hostsBuf.String()) + assert.Equal(t, tt.wantSecureToken, secureToken) if tt.wantErrOut == nil { assert.Equal(t, "", stderr.String()) } else { diff --git a/pkg/cmd/auth/logout/logout_test.go b/pkg/cmd/auth/logout/logout_test.go index 0a5e32f7c..75c215aaa 100644 --- a/pkg/cmd/auth/logout/logout_test.go +++ b/pkg/cmd/auth/logout/logout_test.go @@ -2,6 +2,7 @@ package logout import ( "bytes" + "fmt" "net/http" "regexp" "testing" @@ -13,6 +14,7 @@ import ( "github.com/cli/cli/v2/pkg/iostreams" "github.com/google/shlex" "github.com/stretchr/testify/assert" + "github.com/zalando/go-keyring" ) func Test_NewCmdLogout(t *testing.T) { @@ -96,6 +98,7 @@ func Test_logoutRun_tty(t *testing.T) { opts *LogoutOptions prompterStubs func(*prompter.PrompterMock) cfgHosts []string + secureStorage bool wantHosts string wantErrOut *regexp.Regexp wantErr string @@ -133,14 +136,31 @@ func Test_logoutRun_tty(t *testing.T) { wantHosts: "github.com:\n oauth_token: abc123\n", wantErrOut: regexp.MustCompile(`Logged out of cheryl.mason account 'cybilb'`), }, + { + name: "secure storage", + secureStorage: true, + opts: &LogoutOptions{ + Hostname: "github.com", + }, + cfgHosts: []string{"github.com"}, + wantHosts: "{}\n", + wantErrOut: regexp.MustCompile(`Logged out of github.com account 'cybilb'`), + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + keyring.MockInit() readConfigs := config.StubWriteConfig(t) cfg := config.NewFromString("") for _, hostname := range tt.cfgHosts { - cfg.Set(hostname, "oauth_token", "abc123") + if tt.secureStorage { + cfg.Set(hostname, "user", "monalisa") + _ = keyring.Set(fmt.Sprintf("gh:%s", hostname), "", "abc123") + cfg.Authentication().SetToken("abc123", "keyring") + } else { + cfg.Set(hostname, "oauth_token", "abc123") + } } tt.opts.Config = func() (config.Config, error) { return cfg, nil @@ -183,8 +203,10 @@ func Test_logoutRun_tty(t *testing.T) { mainBuf := bytes.Buffer{} hostsBuf := bytes.Buffer{} readConfigs(&mainBuf, &hostsBuf) + secureToken, _ := cfg.Authentication().TokenFromKeyring(tt.opts.Hostname) assert.Equal(t, tt.wantHosts, hostsBuf.String()) + assert.Equal(t, "", secureToken) reg.Verify(t) }) } @@ -192,12 +214,13 @@ func Test_logoutRun_tty(t *testing.T) { func Test_logoutRun_nontty(t *testing.T) { tests := []struct { - name string - opts *LogoutOptions - cfgHosts []string - wantHosts string - wantErr string - ghtoken string + name string + opts *LogoutOptions + cfgHosts []string + secureStorage bool + ghtoken string + wantHosts string + wantErr string }{ { name: "hostname, one host", @@ -222,14 +245,30 @@ func Test_logoutRun_nontty(t *testing.T) { }, wantErr: `not logged in to any hosts`, }, + { + name: "secure storage", + secureStorage: true, + opts: &LogoutOptions{ + Hostname: "harry.mason", + }, + cfgHosts: []string{"harry.mason"}, + wantHosts: "{}\n", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + keyring.MockInit() readConfigs := config.StubWriteConfig(t) cfg := config.NewFromString("") for _, hostname := range tt.cfgHosts { - cfg.Set(hostname, "oauth_token", "abc123") + if tt.secureStorage { + cfg.Set(hostname, "user", "monalisa") + _ = keyring.Set(fmt.Sprintf("gh:%s", hostname), "", "abc123") + cfg.Authentication().SetToken("abc123", "keyring") + } else { + cfg.Set(hostname, "oauth_token", "abc123") + } } tt.opts.Config = func() (config.Config, error) { return cfg, nil @@ -257,8 +296,10 @@ func Test_logoutRun_nontty(t *testing.T) { mainBuf := bytes.Buffer{} hostsBuf := bytes.Buffer{} readConfigs(&mainBuf, &hostsBuf) + secureToken, _ := cfg.Authentication().TokenFromKeyring(tt.opts.Hostname) assert.Equal(t, tt.wantHosts, hostsBuf.String()) + assert.Equal(t, "", secureToken) reg.Verify(t) }) } diff --git a/pkg/cmd/auth/refresh/refresh.go b/pkg/cmd/auth/refresh/refresh.go index 7b1a9ef76..ce4c5029f 100644 --- a/pkg/cmd/auth/refresh/refresh.go +++ b/pkg/cmd/auth/refresh/refresh.go @@ -26,21 +26,26 @@ type RefreshOptions struct { Hostname string Scopes []string - AuthFlow func(*config.AuthConfig, *iostreams.IOStreams, string, []string, bool) error + AuthFlow func(*config.AuthConfig, *iostreams.IOStreams, string, []string, bool, bool) error - Interactive bool + Interactive bool + SecureStorage bool } func NewCmdRefresh(f *cmdutil.Factory, runF func(*RefreshOptions) error) *cobra.Command { opts := &RefreshOptions{ IO: f.IOStreams, Config: f.Config, - AuthFlow: func(authCfg *config.AuthConfig, io *iostreams.IOStreams, hostname string, scopes []string, interactive bool) error { + AuthFlow: func(authCfg *config.AuthConfig, io *iostreams.IOStreams, hostname string, scopes []string, interactive, secureStorage bool) error { + if secureStorage { + cs := io.ColorScheme() + fmt.Fprintf(io.ErrOut, "%s Using secure storage could break installed extensions", cs.WarningIcon()) + } token, username, err := authflow.AuthFlow(hostname, io, "", scopes, interactive, f.Browser) if err != nil { return err } - return authCfg.Login(hostname, username, token, "", false) + return authCfg.Login(hostname, username, token, "", secureStorage) }, HttpClient: &http.Client{}, GitClient: f.GitClient, @@ -80,6 +85,7 @@ func NewCmdRefresh(f *cmdutil.Factory, runF func(*RefreshOptions) error) *cobra. cmd.Flags().StringVarP(&opts.Hostname, "hostname", "h", "", "The GitHub host to use for authentication") cmd.Flags().StringSliceVarP(&opts.Scopes, "scopes", "s", nil, "Additional authentication scopes for gh to have") + cmd.Flags().BoolVarP(&opts.SecureStorage, "secure-storage", "", false, "Save authentication credentials in secure credential store") return cmd } @@ -152,7 +158,7 @@ func refreshRun(opts *RefreshOptions) error { additionalScopes = append(additionalScopes, credentialFlow.Scopes()...) } - if err := opts.AuthFlow(authCfg, opts.IO, hostname, append(opts.Scopes, additionalScopes...), opts.Interactive); err != nil { + if err := opts.AuthFlow(authCfg, opts.IO, hostname, append(opts.Scopes, additionalScopes...), opts.Interactive, opts.SecureStorage); err != nil { return err } diff --git a/pkg/cmd/auth/refresh/refresh_test.go b/pkg/cmd/auth/refresh/refresh_test.go index 89903ad0d..863bc0b7c 100644 --- a/pkg/cmd/auth/refresh/refresh_test.go +++ b/pkg/cmd/auth/refresh/refresh_test.go @@ -85,6 +85,14 @@ func Test_NewCmdRefresh(t *testing.T) { Scopes: []string{"repo:invite", "read:public_key"}, }, }, + { + name: "secure storage", + tty: true, + cli: "--secure-storage", + wants: RefreshOptions{ + SecureStorage: true, + }, + }, } for _, tt := range tests { @@ -126,8 +134,10 @@ func Test_NewCmdRefresh(t *testing.T) { } type authArgs struct { - hostname string - scopes []string + hostname string + scopes []string + interactive bool + secureStorage bool } func Test_refreshRun(t *testing.T) { @@ -230,17 +240,33 @@ func Test_refreshRun(t *testing.T) { scopes: []string{"repo:invite", "public_key:read", "delete_repo", "codespace"}, }, }, + { + name: "secure storage", + cfgHosts: []string{ + "obed.morton", + }, + opts: &RefreshOptions{ + Hostname: "obed.morton", + SecureStorage: true, + }, + wantAuthArgs: authArgs{ + hostname: "obed.morton", + scopes: nil, + secureStorage: true, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { aa := authArgs{} - tt.opts.AuthFlow = func(_ *config.AuthConfig, _ *iostreams.IOStreams, hostname string, scopes []string, interactive bool) error { + tt.opts.AuthFlow = func(_ *config.AuthConfig, _ *iostreams.IOStreams, hostname string, scopes []string, interactive, secureStorage bool) error { aa.hostname = hostname aa.scopes = scopes + aa.interactive = interactive + aa.secureStorage = secureStorage return nil } - _ = config.StubWriteConfig(t) cfg := config.NewFromString("") for _, hostname := range tt.cfgHosts { cfg.Set(hostname, "oauth_token", "abc123") @@ -291,6 +317,8 @@ func Test_refreshRun(t *testing.T) { assert.Equal(t, tt.wantAuthArgs.hostname, aa.hostname) assert.Equal(t, tt.wantAuthArgs.scopes, aa.scopes) + assert.Equal(t, tt.wantAuthArgs.interactive, aa.interactive) + assert.Equal(t, tt.wantAuthArgs.secureStorage, aa.secureStorage) }) } } diff --git a/pkg/cmd/auth/shared/login_flow.go b/pkg/cmd/auth/shared/login_flow.go index 4f96eada3..0201af155 100644 --- a/pkg/cmd/auth/shared/login_flow.go +++ b/pkg/cmd/auth/shared/login_flow.go @@ -24,18 +24,19 @@ type iconfig interface { } type LoginOptions struct { - IO *iostreams.IOStreams - Config iconfig - HTTPClient *http.Client - GitClient *git.Client - Hostname string - Interactive bool - Web bool - Scopes []string - Executable string - GitProtocol string - Prompter Prompt - Browser browser.Browser + IO *iostreams.IOStreams + Config iconfig + HTTPClient *http.Client + GitClient *git.Client + Hostname string + Interactive bool + Web bool + Scopes []string + Executable string + GitProtocol string + Prompter Prompt + Browser browser.Browser + SecureStorage bool sshContext ssh.Context } @@ -186,7 +187,7 @@ func Login(opts *LoginOptions) error { fmt.Fprintf(opts.IO.ErrOut, "%s Configured git protocol\n", cs.SuccessIcon()) } - if err := cfg.Login(hostname, username, authToken, gitProtocol, false); err != nil { + if err := cfg.Login(hostname, username, authToken, gitProtocol, opts.SecureStorage); err != nil { return err } diff --git a/pkg/cmd/auth/shared/writeable.go b/pkg/cmd/auth/shared/writeable.go index 2de17054b..cf8e678e4 100644 --- a/pkg/cmd/auth/shared/writeable.go +++ b/pkg/cmd/auth/shared/writeable.go @@ -1,14 +1,12 @@ package shared import ( - "github.com/cli/cli/v2/internal/config" -) + "strings" -const ( - oauthToken = "oauth_token" + "github.com/cli/cli/v2/internal/config" ) func AuthTokenWriteable(authCfg *config.AuthConfig, hostname string) (string, bool) { token, src := authCfg.Token(hostname) - return src, (token == "" || src == oauthToken) + return src, (token == "" || !strings.HasSuffix(src, "_TOKEN")) } diff --git a/pkg/cmd/auth/token/token.go b/pkg/cmd/auth/token/token.go index c28d42d26..093c6b4f3 100644 --- a/pkg/cmd/auth/token/token.go +++ b/pkg/cmd/auth/token/token.go @@ -14,7 +14,8 @@ type TokenOptions struct { IO *iostreams.IOStreams Config func() (config.Config, error) - Hostname string + Hostname string + SecureStorage bool } func NewCmdToken(f *cmdutil.Factory, runF func(*TokenOptions) error) *cobra.Command { @@ -37,6 +38,8 @@ func NewCmdToken(f *cmdutil.Factory, runF func(*TokenOptions) error) *cobra.Comm } cmd.Flags().StringVarP(&opts.Hostname, "hostname", "h", "", "The hostname of the GitHub instance authenticated with") + cmd.Flags().BoolVarP(&opts.SecureStorage, "secure-storage", "", false, "Search only secure credential store for authentication token") + _ = cmd.Flags().MarkHidden("secure-storeage") return cmd } @@ -51,8 +54,14 @@ func tokenRun(opts *TokenOptions) error { if err != nil { return err } + authCfg := cfg.Authentication() - val, _ := cfg.AuthToken(hostname) + var val string + if opts.SecureStorage { + val, _ = authCfg.TokenFromKeyring(hostname) + } else { + val, _ = authCfg.Token(hostname) + } if val == "" { return fmt.Errorf("no oauth token") } diff --git a/pkg/cmd/auth/token/token_test.go b/pkg/cmd/auth/token/token_test.go index 22d13a9a5..463c073b2 100644 --- a/pkg/cmd/auth/token/token_test.go +++ b/pkg/cmd/auth/token/token_test.go @@ -9,6 +9,7 @@ import ( "github.com/cli/cli/v2/pkg/iostreams" "github.com/google/shlex" "github.com/stretchr/testify/assert" + "github.com/zalando/go-keyring" ) func TestNewCmdToken(t *testing.T) { @@ -34,6 +35,11 @@ func TestNewCmdToken(t *testing.T) { input: "-h github.mycompany.com", output: TokenOptions{Hostname: "github.mycompany.com"}, }, + { + name: "with secure-storage", + input: "--secure-storage", + output: TokenOptions{SecureStorage: true}, + }, } for _, tt := range tests { @@ -71,11 +77,12 @@ func TestNewCmdToken(t *testing.T) { assert.NoError(t, err) assert.Equal(t, tt.output.Hostname, cmdOpts.Hostname) + assert.Equal(t, tt.output.SecureStorage, cmdOpts.SecureStorage) }) } } -func Test_tokenRun(t *testing.T) { +func TestTokenRun(t *testing.T) { tests := []struct { name string opts TokenOptions @@ -121,17 +128,77 @@ func Test_tokenRun(t *testing.T) { } for _, tt := range tests { - ios, _, stdout, _ := iostreams.Test() - tt.opts.IO = ios - t.Run(tt.name, func(t *testing.T) { + ios, _, stdout, _ := iostreams.Test() + tt.opts.IO = ios + err := tokenRun(&tt.opts) + if tt.wantErr { + assert.Error(t, err) + assert.EqualError(t, err, tt.wantErrMsg) + return + } + assert.NoError(t, err) + assert.Equal(t, tt.wantStdout, stdout.String()) + }) + } +} + +func TestTokenRunSecureStorage(t *testing.T) { + tests := []struct { + name string + opts TokenOptions + wantStdout string + wantErr bool + wantErrMsg string + }{ + { + name: "token", + opts: TokenOptions{ + Config: func() (config.Config, error) { + cfg := config.NewBlankConfig() + _ = keyring.Set("gh:github.com", "", "gho_ABCDEFG") + return cfg, nil + }, + }, + wantStdout: "gho_ABCDEFG\n", + }, + { + name: "token by hostname", + opts: TokenOptions{ + Config: func() (config.Config, error) { + cfg := config.NewBlankConfig() + _ = keyring.Set("gh:mycompany.com", "", "gho_1234567") + return cfg, nil + }, + Hostname: "mycompany.com", + }, + wantStdout: "gho_1234567\n", + }, + { + name: "no token", + opts: TokenOptions{ + Config: func() (config.Config, error) { + cfg := config.NewBlankConfig() + return cfg, nil + }, + }, + wantErr: true, + wantErrMsg: "no oauth token", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + keyring.MockInit() + ios, _, stdout, _ := iostreams.Test() + tt.opts.IO = ios + tt.opts.SecureStorage = true err := tokenRun(&tt.opts) if tt.wantErr { assert.Error(t, err) assert.EqualError(t, err, tt.wantErrMsg) return } - assert.NoError(t, err) assert.Equal(t, tt.wantStdout, stdout.String()) }) diff --git a/pkg/liveshare/test/server.go b/pkg/liveshare/test/server.go index 5dd4e56aa..0b4f6a7ba 100644 --- a/pkg/liveshare/test/server.go +++ b/pkg/liveshare/test/server.go @@ -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 + if r.WantReply { + if err := r.Reply(true, nil); err != nil { sendError(errc, fmt.Errorf("error replying to channel request: %w", err)) return } } - if strings.HasPrefix(req.Type, "stream-transport") { + if strings.HasPrefix(r.Type, "stream-transport") { go func() { - if err := forwardStream(ctx, server, req.Type, channel); err != nil { + if err := forwardStream(ctx, server, r.Type, channel); err != nil { sendError(errc, fmt.Errorf("failed to forward stream: %w", err)) } }()