From ad2b9ef704baefa9164a4fff5dcfb855935f3617 Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Thu, 23 Mar 2017 13:44:51 -0600 Subject: [PATCH 01/11] lfsapi: extract sshResolver interface --- lfsapi/client.go | 2 +- lfsapi/lfsapi.go | 3 +++ lfsapi/ssh.go | 26 +++++++++++++++++--------- 3 files changed, 21 insertions(+), 10 deletions(-) diff --git a/lfsapi/client.go b/lfsapi/client.go index 9ae2a506..da220186 100644 --- a/lfsapi/client.go +++ b/lfsapi/client.go @@ -18,7 +18,7 @@ var UserAgent = "git-lfs" const MediaType = "application/vnd.git-lfs+json; charset=utf-8" func (c *Client) NewRequest(method string, e Endpoint, suffix string, body interface{}) (*http.Request, error) { - sshRes, err := c.resolveSSHEndpoint(e, method) + sshRes, err := c.sshResolver.Resolve(e, method) if err != nil { tracerx.Printf("ssh: %s failed, error: %s, message: %s", e.SshUserAndHost, err.Error(), sshRes.Message, diff --git a/lfsapi/lfsapi.go b/lfsapi/lfsapi.go index ac083c70..e5f37923 100644 --- a/lfsapi/lfsapi.go +++ b/lfsapi/lfsapi.go @@ -41,6 +41,8 @@ type Client struct { hostClients map[string]*http.Client clientMu sync.Mutex + sshResolver sshResolver + ntlmSessions map[string]ntlm.ClientSession ntlmMu sync.Mutex @@ -89,6 +91,7 @@ func NewClient(osEnv Env, gitEnv Env) (*Client, error) { NoProxy: noProxy, gitEnv: gitEnv, osEnv: osEnv, + sshResolver: &sshAuthClient{os: osEnv}, } return c, nil diff --git a/lfsapi/ssh.go b/lfsapi/ssh.go index e3cf80f1..185692ee 100644 --- a/lfsapi/ssh.go +++ b/lfsapi/ssh.go @@ -12,13 +12,28 @@ import ( "github.com/rubyist/tracerx" ) -func (c *Client) resolveSSHEndpoint(e Endpoint, method string) (sshAuthResponse, error) { +type sshResolver interface { + Resolve(Endpoint, string) (sshAuthResponse, error) +} + +type sshAuthResponse struct { + Message string `json:"-"` + Href string `json:"href"` + Header map[string]string `json:"header"` + ExpiresAt string `json:"expires_at"` +} + +type sshAuthClient struct { + os Env +} + +func (c *sshAuthClient) Resolve(e Endpoint, method string) (sshAuthResponse, error) { res := sshAuthResponse{} if len(e.SshUserAndHost) == 0 { return res, nil } - exe, args := sshGetLFSExeAndArgs(c.osEnv, e, method) + exe, args := sshGetLFSExeAndArgs(c.os, e, method) cmd := exec.Command(exe, args...) // Save stdout and stderr in separate buffers @@ -42,13 +57,6 @@ func (c *Client) resolveSSHEndpoint(e Endpoint, method string) (sshAuthResponse, return res, err } -type sshAuthResponse struct { - Message string `json:"-"` - Href string `json:"href"` - Header map[string]string `json:"header"` - ExpiresAt string `json:"expires_at"` -} - func sshGetLFSExeAndArgs(osEnv Env, e Endpoint, method string) (string, []string) { operation := endpointOperation(e, method) tracerx.Printf("ssh: %s git-lfs-authenticate %s %s", From 34e8c01baa831a5eb9e9bce180dff02cff736a5d Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Thu, 23 Mar 2017 13:48:52 -0600 Subject: [PATCH 02/11] lfsapi: export SSHResolver and property --- lfsapi/client.go | 2 +- lfsapi/lfsapi.go | 16 +++++++++------- lfsapi/ssh.go | 2 +- 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/lfsapi/client.go b/lfsapi/client.go index da220186..ed718350 100644 --- a/lfsapi/client.go +++ b/lfsapi/client.go @@ -18,7 +18,7 @@ var UserAgent = "git-lfs" const MediaType = "application/vnd.git-lfs+json; charset=utf-8" func (c *Client) NewRequest(method string, e Endpoint, suffix string, body interface{}) (*http.Request, error) { - sshRes, err := c.sshResolver.Resolve(e, method) + sshRes, err := c.SSH.Resolve(e, method) if err != nil { tracerx.Printf("ssh: %s failed, error: %s, message: %s", e.SshUserAndHost, err.Error(), sshRes.Message, diff --git a/lfsapi/lfsapi.go b/lfsapi/lfsapi.go index e5f37923..bf33f858 100644 --- a/lfsapi/lfsapi.go +++ b/lfsapi/lfsapi.go @@ -22,6 +22,7 @@ var ( type Client struct { Endpoints EndpointFinder Credentials CredentialHelper + SSH SSHResolver Netrc NetrcFinder DialTimeout int @@ -41,8 +42,6 @@ type Client struct { hostClients map[string]*http.Client clientMu sync.Mutex - sshResolver sshResolver - ntlmSessions map[string]ntlm.ClientSession ntlmMu sync.Mutex @@ -72,11 +71,15 @@ func NewClient(osEnv Env, gitEnv Env) (*Client, error) { httpsProxy, httpProxy, noProxy := getProxyServers(osEnv, gitEnv) + creds := &commandCredentialHelper{ + SkipPrompt: !osEnv.Bool("GIT_TERMINAL_PROMPT", true), + } + sshResolver := &sshAuthClient{os: osEnv} + c := &Client{ - Endpoints: NewEndpointFinder(gitEnv), - Credentials: &commandCredentialHelper{ - SkipPrompt: !osEnv.Bool("GIT_TERMINAL_PROMPT", true), - }, + Endpoints: NewEndpointFinder(gitEnv), + Credentials: creds, + SSH: sshResolver, Netrc: netrc, DialTimeout: gitEnv.Int("lfs.dialtimeout", 0), KeepaliveTimeout: gitEnv.Int("lfs.keepalive", 0), @@ -91,7 +94,6 @@ func NewClient(osEnv Env, gitEnv Env) (*Client, error) { NoProxy: noProxy, gitEnv: gitEnv, osEnv: osEnv, - sshResolver: &sshAuthClient{os: osEnv}, } return c, nil diff --git a/lfsapi/ssh.go b/lfsapi/ssh.go index 185692ee..c055cd47 100644 --- a/lfsapi/ssh.go +++ b/lfsapi/ssh.go @@ -12,7 +12,7 @@ import ( "github.com/rubyist/tracerx" ) -type sshResolver interface { +type SSHResolver interface { Resolve(Endpoint, string) (sshAuthResponse, error) } From 9b5007975572375f0659b759618fe34bdb513282 Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Thu, 23 Mar 2017 13:58:15 -0600 Subject: [PATCH 03/11] spike a quick cache without expiration --- lfsapi/lfsapi.go | 2 +- lfsapi/ssh.go | 25 +++++++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/lfsapi/lfsapi.go b/lfsapi/lfsapi.go index bf33f858..d225d9aa 100644 --- a/lfsapi/lfsapi.go +++ b/lfsapi/lfsapi.go @@ -74,7 +74,7 @@ func NewClient(osEnv Env, gitEnv Env) (*Client, error) { creds := &commandCredentialHelper{ SkipPrompt: !osEnv.Bool("GIT_TERMINAL_PROMPT", true), } - sshResolver := &sshAuthClient{os: osEnv} + sshResolver := withSSHCache(&sshAuthClient{os: osEnv}) c := &Client{ Endpoints: NewEndpointFinder(gitEnv), diff --git a/lfsapi/ssh.go b/lfsapi/ssh.go index c055cd47..37fb98a3 100644 --- a/lfsapi/ssh.go +++ b/lfsapi/ssh.go @@ -16,6 +16,31 @@ type SSHResolver interface { Resolve(Endpoint, string) (sshAuthResponse, error) } +func withSSHCache(ssh SSHResolver) SSHResolver { + return &sshCache{ + endpoints: make(map[string]*sshAuthResponse), + ssh: ssh, + } +} + +type sshCache struct { + endpoints map[string]*sshAuthResponse + ssh SSHResolver +} + +func (c *sshCache) Resolve(e Endpoint, method string) (sshAuthResponse, error) { + key := strings.Join([]string{e.SshUserAndHost, e.SshPort, e.SshPath, method}, "//") + if res, ok := c.endpoints[key]; ok { + return *res, nil + } + + res, err := c.ssh.Resolve(e, method) + if err == nil { + c.endpoints[key] = &res + } + return res, err +} + type sshAuthResponse struct { Message string `json:"-"` Href string `json:"href"` From e32b267b4a81da3112aafdff5e72fe08ee84139e Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Fri, 24 Mar 2017 11:27:44 -0600 Subject: [PATCH 04/11] Skip ssh caching for non ssh remotes --- lfsapi/ssh.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lfsapi/ssh.go b/lfsapi/ssh.go index 37fb98a3..69709088 100644 --- a/lfsapi/ssh.go +++ b/lfsapi/ssh.go @@ -29,8 +29,14 @@ type sshCache struct { } func (c *sshCache) Resolve(e Endpoint, method string) (sshAuthResponse, error) { + if len(e.SshUserAndHost) == 0 { + return sshAuthResponse{}, nil + } + key := strings.Join([]string{e.SshUserAndHost, e.SshPort, e.SshPath, method}, "//") if res, ok := c.endpoints[key]; ok { + tracerx.Printf("ssh cache: %s git-lfs-authenticate %s %s", + e.SshUserAndHost, e.SshPath, endpointOperation(e, method)) return *res, nil } From 9868de85592378f77dddb81ab581ab95c5ca2392 Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Fri, 24 Mar 2017 11:27:52 -0600 Subject: [PATCH 05/11] Add credential caching --- lfsapi/creds.go | 53 ++++++++++++++++++++++++++++++++++++++++++++++++ lfsapi/lfsapi.go | 4 ++-- 2 files changed, 55 insertions(+), 2 deletions(-) diff --git a/lfsapi/creds.go b/lfsapi/creds.go index b0b2e604..39a2e92a 100644 --- a/lfsapi/creds.go +++ b/lfsapi/creds.go @@ -5,6 +5,8 @@ import ( "fmt" "os/exec" "strings" + + "github.com/rubyist/tracerx" ) type CredentialHelper interface { @@ -28,11 +30,62 @@ func bufferCreds(c Creds) *bytes.Buffer { return buf } +func withCredentialCache(helper CredentialHelper) CredentialHelper { + return &credentialCacher{ + creds: make(map[string]Creds), + helper: helper, + } +} + +type credentialCacher struct { + creds map[string]Creds + helper CredentialHelper +} + +func credCacheKey(creds Creds) string { + parts := []string{ + creds["protocol"], + creds["host"], + creds["path"], + } + return strings.Join(parts, "//") +} + +func (c *credentialCacher) Fill(creds Creds) (Creds, error) { + key := credCacheKey(creds) + if cache, ok := c.creds[key]; ok { + tracerx.Printf("creds: git credential cache (%q, %q, %q)", + creds["protocol"], creds["host"], creds["path"]) + return cache, nil + } + + creds, err := c.helper.Fill(creds) + if err == nil && len(creds["username"]) > 0 && len(creds["password"]) > 0 { + c.creds[key] = creds + } + return creds, err +} + +func (c *credentialCacher) Reject(creds Creds) error { + delete(c.creds, credCacheKey(creds)) + return c.helper.Reject(creds) +} + +func (c *credentialCacher) Approve(creds Creds) error { + err := c.helper.Approve(creds) + if err == nil { + c.creds[credCacheKey(creds)] = creds + } + return err +} + type commandCredentialHelper struct { SkipPrompt bool } func (h *commandCredentialHelper) Fill(creds Creds) (Creds, error) { + tracerx.Printf("creds: git credential fill (%q, %q, %q)", + creds["protocol"], creds["host"], creds["path"]) return h.exec("fill", creds) } diff --git a/lfsapi/lfsapi.go b/lfsapi/lfsapi.go index d225d9aa..3a44f664 100644 --- a/lfsapi/lfsapi.go +++ b/lfsapi/lfsapi.go @@ -71,9 +71,9 @@ func NewClient(osEnv Env, gitEnv Env) (*Client, error) { httpsProxy, httpProxy, noProxy := getProxyServers(osEnv, gitEnv) - creds := &commandCredentialHelper{ + creds := withCredentialCache(&commandCredentialHelper{ SkipPrompt: !osEnv.Bool("GIT_TERMINAL_PROMPT", true), - } + }) sshResolver := withSSHCache(&sshAuthClient{os: osEnv}) c := &Client{ From 09642bced76d8edf7203a6e3955cc53845c2d8ce Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Fri, 24 Mar 2017 11:37:23 -0600 Subject: [PATCH 06/11] Only cache credentials with `lfs.cachecredentials` in 2.0.2 --- lfsapi/lfsapi.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/lfsapi/lfsapi.go b/lfsapi/lfsapi.go index 3a44f664..4d25fc2b 100644 --- a/lfsapi/lfsapi.go +++ b/lfsapi/lfsapi.go @@ -71,10 +71,15 @@ func NewClient(osEnv Env, gitEnv Env) (*Client, error) { httpsProxy, httpProxy, noProxy := getProxyServers(osEnv, gitEnv) - creds := withCredentialCache(&commandCredentialHelper{ + var creds CredentialHelper = &commandCredentialHelper{ SkipPrompt: !osEnv.Bool("GIT_TERMINAL_PROMPT", true), - }) - sshResolver := withSSHCache(&sshAuthClient{os: osEnv}) + } + var sshResolver SSHResolver = &sshAuthClient{os: osEnv} + + if gitEnv.Bool("lfs.cachecredentials", false) { + creds = withCredentialCache(creds) + sshResolver = withSSHCache(sshResolver) + } c := &Client{ Endpoints: NewEndpointFinder(gitEnv), From 333d3c55a7d210f2f3bc0ffd5229916a3cbf24b2 Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Fri, 24 Mar 2017 12:23:32 -0600 Subject: [PATCH 07/11] lfsapi: add credential cacher test --- lfsapi/creds_test.go | 268 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 268 insertions(+) create mode 100644 lfsapi/creds_test.go diff --git a/lfsapi/creds_test.go b/lfsapi/creds_test.go new file mode 100644 index 00000000..b0bc01ce --- /dev/null +++ b/lfsapi/creds_test.go @@ -0,0 +1,268 @@ +package lfsapi + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// test that cache satisfies Fill() without looking at creds +func TestCredsCacheFillFromCache(t *testing.T) { + creds := newFakeCreds() + cache := withCredentialCache(creds).(*credentialCacher) + cache.creds["http//lfs.test//foo/bar"] = Creds{ + "protocol": "http", + "host": "lfs.test", + "path": "foo/bar", + "username": "u", + "password": "p", + } + + filled, err := cache.Fill(Creds{ + "protocol": "http", + "host": "lfs.test", + "path": "foo/bar", + }) + assert.Nil(t, err) + require.NotNil(t, filled) + assert.Equal(t, "u", filled["username"]) + assert.Equal(t, "p", filled["password"]) + + assert.Equal(t, 1, len(cache.creds)) + cached, ok := cache.creds["http//lfs.test//foo/bar"] + assert.True(t, ok) + assert.Equal(t, "u", cached["username"]) + assert.Equal(t, "p", cached["password"]) +} + +// test that cache caches Fill() value from creds +func TestCredsCacheFillFromValidHelperFill(t *testing.T) { + creds := newFakeCreds() + cache := withCredentialCache(creds).(*credentialCacher) + + creds.list = append(creds.list, Creds{ + "protocol": "http", + "host": "lfs.test", + "path": "foo/bar", + "username": "u", + "password": "p", + }) + + assert.Equal(t, 0, len(cache.creds)) + + filled, err := cache.Fill(Creds{ + "protocol": "http", + "host": "lfs.test", + "path": "foo/bar", + }) + assert.Nil(t, err) + require.NotNil(t, filled) + assert.Equal(t, "u", filled["username"]) + assert.Equal(t, "p", filled["password"]) + + assert.Equal(t, 1, len(cache.creds)) + cached, ok := cache.creds["http//lfs.test//foo/bar"] + assert.True(t, ok) + assert.Equal(t, "u", cached["username"]) + assert.Equal(t, "p", cached["password"]) + + creds.list = make([]Creds, 0) + filled2, err := cache.Fill(Creds{ + "protocol": "http", + "host": "lfs.test", + "path": "foo/bar", + }) + assert.Nil(t, err) + require.NotNil(t, filled2) + assert.Equal(t, "u", filled2["username"]) + assert.Equal(t, "p", filled2["password"]) +} + +// test that cache ignores Fill() value from creds with missing username+password +func TestCredsCacheFillFromInvalidHelperFill(t *testing.T) { + creds := newFakeCreds() + cache := withCredentialCache(creds).(*credentialCacher) + + creds.list = append(creds.list, Creds{ + "protocol": "http", + "host": "lfs.test", + "path": "foo/bar", + "username": "no-password", + }) + + assert.Equal(t, 0, len(cache.creds)) + + filled, err := cache.Fill(Creds{ + "protocol": "http", + "host": "lfs.test", + "path": "foo/bar", + "username": "u", + "password": "p", + }) + assert.Nil(t, err) + require.NotNil(t, filled) + assert.Equal(t, "no-password", filled["username"]) + assert.Equal(t, "", filled["password"]) + + assert.Equal(t, 0, len(cache.creds)) +} + +// test that cache ignores Fill() value from creds with error +func TestCredsCacheFillFromErroringHelperFill(t *testing.T) { + creds := newFakeCreds() + cache := withCredentialCache(&erroringCreds{creds}).(*credentialCacher) + + creds.list = append(creds.list, Creds{ + "protocol": "http", + "host": "lfs.test", + "path": "foo/bar", + "username": "u", + "password": "p", + }) + + assert.Equal(t, 0, len(cache.creds)) + + filled, err := cache.Fill(Creds{ + "protocol": "http", + "host": "lfs.test", + "path": "foo/bar", + }) + assert.NotNil(t, err) + require.NotNil(t, filled) + assert.Equal(t, "u", filled["username"]) + assert.Equal(t, "p", filled["password"]) + + assert.Equal(t, 0, len(cache.creds)) +} + +func TestCredsCacheRejectWithoutError(t *testing.T) { + creds := newFakeCreds() + cache := withCredentialCache(creds).(*credentialCacher) + + cache.creds["http//lfs.test//foo/bar"] = Creds{ + "protocol": "http", + "host": "lfs.test", + "path": "foo/bar", + "username": "u", + "password": "p", + } + + err := cache.Reject(Creds{ + "protocol": "http", + "host": "lfs.test", + "path": "foo/bar", + }) + assert.Nil(t, err) + assert.Equal(t, 0, len(cache.creds)) +} + +func TestCredsCacheRejectWithError(t *testing.T) { + creds := newFakeCreds() + cache := withCredentialCache(&erroringCreds{creds}).(*credentialCacher) + + cache.creds["http//lfs.test//foo/bar"] = Creds{ + "protocol": "http", + "host": "lfs.test", + "path": "foo/bar", + "username": "u", + "password": "p", + } + + err := cache.Reject(Creds{ + "protocol": "http", + "host": "lfs.test", + "path": "foo/bar", + }) + assert.NotNil(t, err) + assert.Equal(t, 0, len(cache.creds)) +} + +func TestCredsCacheApproveWithoutError(t *testing.T) { + creds := newFakeCreds() + cache := withCredentialCache(creds).(*credentialCacher) + + assert.Equal(t, 0, len(cache.creds)) + + err := cache.Approve(Creds{ + "protocol": "http", + "host": "lfs.test", + "path": "foo/bar", + "username": "U", + "password": "P", + }) + assert.Nil(t, err) + assert.Equal(t, 1, len(cache.creds)) + cached, ok := cache.creds["http//lfs.test//foo/bar"] + assert.True(t, ok) + assert.Equal(t, "U", cached["username"]) + assert.Equal(t, "P", cached["password"]) +} + +func TestCredsCacheApproveWithError(t *testing.T) { + creds := newFakeCreds() + cache := withCredentialCache(&erroringCreds{creds}).(*credentialCacher) + + assert.Equal(t, 0, len(cache.creds)) + + err := cache.Approve(Creds{ + "protocol": "http", + "host": "lfs.test", + "path": "foo/bar", + "username": "u", + "password": "p", + }) + assert.NotNil(t, err) + assert.Equal(t, 0, len(cache.creds)) +} + +func newFakeCreds() *fakeCreds { + return &fakeCreds{list: make([]Creds, 0)} +} + +type erroringCreds struct { + helper CredentialHelper +} + +func (e *erroringCreds) Fill(creds Creds) (Creds, error) { + c, _ := e.helper.Fill(creds) + return c, errors.New("fill error") +} + +func (e *erroringCreds) Reject(creds Creds) error { + e.helper.Reject(creds) + return errors.New("reject error") +} + +func (e *erroringCreds) Approve(creds Creds) error { + e.helper.Approve(creds) + return errors.New("approve error") +} + +type fakeCreds struct { + list []Creds +} + +func credsMatch(c1, c2 Creds) bool { + return c1["protocol"] == c2["protocol"] && + c1["host"] == c2["host"] && + c1["path"] == c2["path"] +} + +func (f *fakeCreds) Fill(creds Creds) (Creds, error) { + for _, saved := range f.list { + if credsMatch(creds, saved) { + return saved, nil + } + } + return creds, nil +} + +func (f *fakeCreds) Reject(creds Creds) error { + return nil +} + +func (f *fakeCreds) Approve(creds Creds) error { + return nil +} From b77a01cc343656c1c8e556370b10802f9a9c6fce Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Fri, 24 Mar 2017 12:23:43 -0600 Subject: [PATCH 08/11] lfsapi: add ssh resolver cache test --- lfsapi/ssh_test.go | 91 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/lfsapi/ssh_test.go b/lfsapi/ssh_test.go index ec01bf9a..dcbca7f3 100644 --- a/lfsapi/ssh_test.go +++ b/lfsapi/ssh_test.go @@ -1,6 +1,7 @@ package lfsapi import ( + "errors" "path/filepath" "testing" @@ -8,6 +9,96 @@ import ( "github.com/stretchr/testify/require" ) +func TestSSHCacheResolveFromCache(t *testing.T) { + ssh := newFakeResolver() + cache := withSSHCache(ssh).(*sshCache) + cache.endpoints["userandhost//1//path//post"] = &sshAuthResponse{ + Href: "cache", + } + + e := Endpoint{ + SshUserAndHost: "userandhost", + SshPort: "1", + SshPath: "path", + } + + res, err := cache.Resolve(e, "post") + assert.Nil(t, err) + assert.Equal(t, "cache", res.Href) +} + +func TestSSHCacheResolveWithoutError(t *testing.T) { + ssh := newFakeResolver() + cache := withSSHCache(ssh).(*sshCache) + + assert.Equal(t, 0, len(cache.endpoints)) + + ssh.responses["userandhost"] = sshAuthResponse{Href: "real"} + + e := Endpoint{ + SshUserAndHost: "userandhost", + SshPort: "1", + SshPath: "path", + } + + res, err := cache.Resolve(e, "post") + assert.Nil(t, err) + assert.Equal(t, "real", res.Href) + + assert.Equal(t, 1, len(cache.endpoints)) + cacheres, ok := cache.endpoints["userandhost//1//path//post"] + assert.True(t, ok) + assert.NotNil(t, cacheres) + assert.Equal(t, "real", cacheres.Href) + + delete(ssh.responses, "userandhost") + res2, err := cache.Resolve(e, "post") + assert.Nil(t, err) + assert.Equal(t, "real", res2.Href) +} + +func TestSSHCacheResolveWithError(t *testing.T) { + ssh := newFakeResolver() + cache := withSSHCache(ssh).(*sshCache) + + assert.Equal(t, 0, len(cache.endpoints)) + + ssh.responses["userandhost"] = sshAuthResponse{Message: "resolve error", Href: "real"} + + e := Endpoint{ + SshUserAndHost: "userandhost", + SshPort: "1", + SshPath: "path", + } + + res, err := cache.Resolve(e, "post") + assert.NotNil(t, err) + assert.Equal(t, "real", res.Href) + + assert.Equal(t, 0, len(cache.endpoints)) + delete(ssh.responses, "userandhost") + res2, err := cache.Resolve(e, "post") + assert.Nil(t, err) + assert.Equal(t, "", res2.Href) +} + +func newFakeResolver() *fakeResolver { + return &fakeResolver{responses: make(map[string]sshAuthResponse)} +} + +type fakeResolver struct { + responses map[string]sshAuthResponse +} + +func (r *fakeResolver) Resolve(e Endpoint, method string) (sshAuthResponse, error) { + res := r.responses[e.SshUserAndHost] + var err error + if len(res.Message) > 0 { + err = errors.New(res.Message) + } + return res, err +} + func TestSSHGetLFSExeAndArgs(t *testing.T) { cli, err := NewClient(TestEnv(map[string]string{}), nil) require.Nil(t, err) From 6fb9b2cd916d1d38f75bb0032123985340e7d705 Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Fri, 24 Mar 2017 12:37:00 -0600 Subject: [PATCH 09/11] lfsapi: respect ssh response ExpiresAt in the ssh cache --- lfsapi/ssh.go | 5 +++-- lfsapi/ssh_test.go | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 2 deletions(-) diff --git a/lfsapi/ssh.go b/lfsapi/ssh.go index 69709088..bd5c8ca1 100644 --- a/lfsapi/ssh.go +++ b/lfsapi/ssh.go @@ -7,6 +7,7 @@ import ( "os/exec" "path/filepath" "strings" + "time" "github.com/git-lfs/git-lfs/tools" "github.com/rubyist/tracerx" @@ -34,7 +35,7 @@ func (c *sshCache) Resolve(e Endpoint, method string) (sshAuthResponse, error) { } key := strings.Join([]string{e.SshUserAndHost, e.SshPort, e.SshPath, method}, "//") - if res, ok := c.endpoints[key]; ok { + if res, ok := c.endpoints[key]; ok && (res.ExpiresAt.IsZero() || res.ExpiresAt.After(time.Now().Add(5*time.Second))) { tracerx.Printf("ssh cache: %s git-lfs-authenticate %s %s", e.SshUserAndHost, e.SshPath, endpointOperation(e, method)) return *res, nil @@ -51,7 +52,7 @@ type sshAuthResponse struct { Message string `json:"-"` Href string `json:"href"` Header map[string]string `json:"header"` - ExpiresAt string `json:"expires_at"` + ExpiresAt time.Time `json:"expires_at"` } type sshAuthClient struct { diff --git a/lfsapi/ssh_test.go b/lfsapi/ssh_test.go index dcbca7f3..1d0e4e1e 100644 --- a/lfsapi/ssh_test.go +++ b/lfsapi/ssh_test.go @@ -4,6 +4,7 @@ import ( "errors" "path/filepath" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -15,6 +16,7 @@ func TestSSHCacheResolveFromCache(t *testing.T) { cache.endpoints["userandhost//1//path//post"] = &sshAuthResponse{ Href: "cache", } + ssh.responses["userandhost"] = sshAuthResponse{Href: "real"} e := Endpoint{ SshUserAndHost: "userandhost", @@ -27,6 +29,46 @@ func TestSSHCacheResolveFromCache(t *testing.T) { assert.Equal(t, "cache", res.Href) } +func TestSSHCacheResolveFromCacheWithFutureExpiresAt(t *testing.T) { + ssh := newFakeResolver() + cache := withSSHCache(ssh).(*sshCache) + cache.endpoints["userandhost//1//path//post"] = &sshAuthResponse{ + Href: "cache", + ExpiresAt: time.Now().Add(time.Duration(1) * time.Hour), + } + ssh.responses["userandhost"] = sshAuthResponse{Href: "real"} + + e := Endpoint{ + SshUserAndHost: "userandhost", + SshPort: "1", + SshPath: "path", + } + + res, err := cache.Resolve(e, "post") + assert.Nil(t, err) + assert.Equal(t, "cache", res.Href) +} + +func TestSSHCacheResolveFromCacheWithPastExpiresAt(t *testing.T) { + ssh := newFakeResolver() + cache := withSSHCache(ssh).(*sshCache) + cache.endpoints["userandhost//1//path//post"] = &sshAuthResponse{ + Href: "cache", + ExpiresAt: time.Now().Add(time.Duration(-1) * time.Hour), + } + ssh.responses["userandhost"] = sshAuthResponse{Href: "real"} + + e := Endpoint{ + SshUserAndHost: "userandhost", + SshPort: "1", + SshPath: "path", + } + + res, err := cache.Resolve(e, "post") + assert.Nil(t, err) + assert.Equal(t, "real", res.Href) +} + func TestSSHCacheResolveWithoutError(t *testing.T) { ssh := newFakeResolver() cache := withSSHCache(ssh).(*sshCache) From 1ffb075f2b8e6a58b4c9d0644bbd2cebf74d8b13 Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Fri, 24 Mar 2017 14:34:39 -0600 Subject: [PATCH 10/11] docs/man: explain the lfs.cachecredentials option --- docs/man/git-lfs-config.5.ronn | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/man/git-lfs-config.5.ronn b/docs/man/git-lfs-config.5.ronn index 131bb84b..c9db3a14 100644 --- a/docs/man/git-lfs-config.5.ronn +++ b/docs/man/git-lfs-config.5.ronn @@ -45,6 +45,11 @@ be scoped inside the configuration for a remote. Sets the maximum time, in seconds, for the HTTP client to maintain keepalive connections. Default: 30 minutes. +* `lfs.cachecredentials` + + Enables in-memory SSH and Git Credential caching for a single 'git lfs' + command. Default: false. This will default to true in v2.1.0. + ### Transfer (upload / download) settings These settings control how the upload and download of LFS content occurs. From 5b74bb93be41a5f1e9422e19d234c6329946732e Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Fri, 24 Mar 2017 14:36:12 -0600 Subject: [PATCH 11/11] lfsapi: better time comparison --- lfsapi/ssh.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lfsapi/ssh.go b/lfsapi/ssh.go index bd5c8ca1..25e3a5b0 100644 --- a/lfsapi/ssh.go +++ b/lfsapi/ssh.go @@ -35,7 +35,7 @@ func (c *sshCache) Resolve(e Endpoint, method string) (sshAuthResponse, error) { } key := strings.Join([]string{e.SshUserAndHost, e.SshPort, e.SshPath, method}, "//") - if res, ok := c.endpoints[key]; ok && (res.ExpiresAt.IsZero() || res.ExpiresAt.After(time.Now().Add(5*time.Second))) { + if res, ok := c.endpoints[key]; ok && (res.ExpiresAt.IsZero() || time.Until(res.ExpiresAt) > 5*time.Second) { tracerx.Printf("ssh cache: %s git-lfs-authenticate %s %s", e.SshUserAndHost, e.SshPath, endpointOperation(e, method)) return *res, nil