Skip to content
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

Authentication not retrieved / prompted upon redirect #3025

Closed
ashmckenzie opened this issue May 25, 2018 · 4 comments
Closed

Authentication not retrieved / prompted upon redirect #3025

ashmckenzie opened this issue May 25, 2018 · 4 comments

Comments

@ashmckenzie
Copy link

@ashmckenzie ashmckenzie commented May 25, 2018

Hello from GitLab :)

I've observed an issue where a HTTP 302 redirect is detected (https://github.com/git-lfs/git-lfs/blob/master/lfsapi/client.go#L372) but the new HTTP request results in a 401 because authentication is not attempted.

In my scenario, I'm attempting a git lfs -r secondary lock|unlock where the secondary remote is defined as:

secondary	http://localhost:3002/repo.git (fetch)
secondary	http://localhost:3002/repo.git (push)

GitLab is running at http://localhost:3002 and is configured as a secondary (https://docs.gitlab.com/ee/administration/geo/replication/#overview) and is therefore not able to accept DB writes. We have the necessary HTTP redirect logic implemented to redirect to the primary (http://localhost:3001) and is working as expected for git, but we're seeing some issues with git lfs:

git lfs lock -r secondary attempt:

$ GIT_TRACE=1 git lfs lock -r secondary 1527141413.lfs
15:35:32.637108 git.c:576               trace: exec: git-lfs lock -r secondary 1527141413.lfs
15:35:32.637938 run-command.c:640       trace: run_command: git-lfs lock -r secondary 1527141413.lfs
15:35:32.657015 trace git-lfs: exec: git 'version'
15:35:32.670150 trace git-lfs: exec: git '-c' 'filter.lfs.smudge=' '-c' 'filter.lfs.clean=' '-c' 'filter.lfs.process=' '-c' 'filter.lfs.required=false' 'rev-parse' 'HEAD' '--symbolic-full-name' 'HEAD'
15:35:32.685016 trace git-lfs: exec: git 'config' '-l'
15:35:32.694227 trace git-lfs: creds: git credential fill ("http", "localhost:3002", "")
Username for 'http://localhost:3002': root
Password for 'http://root@localhost:3002':
15:35:39.610322 trace git-lfs: Filled credentials for http://localhost:3002/repo.git
15:35:39.687054 trace git-lfs: HTTP: POST http://localhost:3002/repo.git/info/lfs/locks
15:35:43.231598 trace git-lfs: HTTP: 302
15:35:43.231624 trace git-lfs: api: redirect POST http://localhost:3002/repo.git/info/lfs/locks to http://localhost:3001/repo.git/info/lfs/locks
15:35:43.231635 trace git-lfs: HTTP: POST http://localhost:3001/repo.git/info/lfs/locks
15:35:46.261984 trace git-lfs: HTTP: 401
15:35:46.262086 trace git-lfs: api: http response indicates "basic" authentication. Resubmitting...
15:35:46.325415 trace git-lfs: creds: git credential fill ("http", "localhost:3002", "")
Username for 'http://localhost:3002': ^C

15:35:49.250757 trace git-lfs: filepathfilter: rewrite ".git" as "**/.git/**"
15:35:49.250879 trace git-lfs: filepathfilter: rewrite "**/.git" as "**/.git"
15:35:49.250995 trace git-lfs: credential fill error: 'git credential fill' error: signal: interrupt

15:35:49.251043 trace git-lfs: filepathfilter: rejecting "tmp" via []
15:35:49.251075 trace git-lfs: filepathfilter: accepting "tmp"
Lock failed: Git credentials for http://localhost:3002/repo.git not found:
credential fill errors:
'git credential fill' error: signal: interrupt

If however I make the following change (admittedly with a hardcoded remote of secondary), it works as expected. It even correctly approves the credentials for both secondary and primary so subsequent calls do not prompt for auth:

Diff:

$ git diff
diff --git a/lfsapi/client.go b/lfsapi/client.go
index 222a9a5c..92e5fd48 100644
--- a/lfsapi/client.go
+++ b/lfsapi/client.go
@@ -231,7 +231,8 @@ func (c *Client) doWithRedirects(cli *http.Client, req *http.Request, via []*htt
                return res, err
        }

-       return c.doWithRedirects(cli, redirectedReq, via)
+       // return c.doWithRedirects(cli, redirectedReq, via)
+       return c.DoWithAuth("secondary", redirectedReq)
 }

 func (c *Client) httpClient(host string) *http.Client {

Output:

$ GIT_TRACE=1 git lfs lock -r secondary 1527141413.lfs
15:38:56.116704 git.c:576               trace: exec: git-lfs lock -r secondary 1527141413.lfs
15:38:56.117438 run-command.c:640       trace: run_command: git-lfs lock -r secondary 1527141413.lfs
15:38:56.132751 trace git-lfs: exec: git 'version'
15:38:56.145132 trace git-lfs: exec: git '-c' 'filter.lfs.smudge=' '-c' 'filter.lfs.clean=' '-c' 'filter.lfs.process=' '-c' 'filter.lfs.required=false' 'rev-parse' 'HEAD' '--symbolic-full-name' 'HEAD'
15:38:56.158613 trace git-lfs: exec: git 'config' '-l'
15:38:56.166103 trace git-lfs: creds: git credential fill ("http", "localhost:3002", "")
Username for 'http://localhost:3002': root
Password for 'http://root@localhost:3002':
15:39:01.348693 trace git-lfs: Filled credentials for http://localhost:3002/repo.git
15:39:01.423749 trace git-lfs: HTTP: POST http://localhost:3002/repo.git/info/lfs/locks
15:39:01.681113 trace git-lfs: HTTP: 302
15:39:01.681141 trace git-lfs: api: redirect POST http://localhost:3002/repo.git/info/lfs/locks to http://localhost:3001/repo.git/info/lfs/locks
15:39:01.681191 trace git-lfs: creds: git credential fill ("http", "localhost:3001", "")
Username for 'http://localhost:3001': root
Password for 'http://root@localhost:3001':
15:39:04.891269 trace git-lfs: Filled credentials for http://localhost:3001/repo.git/info/lfs/locks
15:39:04.969385 trace git-lfs: HTTP: POST http://localhost:3001/repo.git/info/lfs/locks
15:39:06.364562 trace git-lfs: HTTP: 201
15:39:06.364591 trace git-lfs: creds: git credential approve ("http", "localhost:3001", "")
15:39:06.434513 trace git-lfs: creds: git credential approve ("http", "localhost:3002", "")
15:39:06.507188 trace git-lfs: HTTP: {"lock":{"path":"1527141413.lfs","id":"19","locked_at":"2018-05-25T05:39:06Z","owner":{"name":"Administrator"}}}
Locked 1527141413.lfs
15:39:06.516208 trace git-lfs: filepathfilter: rewrite ".git" as "**/.git/**"
15:39:06.516229 trace git-lfs: filepathfilter: rewrite "**/.git" as "**/.git"
15:39:06.516270 trace git-lfs: filepathfilter: rejecting "tmp" via []
15:39:06.516277 trace git-lfs: filepathfilter: accepting "tmp"

Please help, thanks :)

@ttaylorr
Copy link
Contributor

@ttaylorr ttaylorr commented May 25, 2018

Hi @ashmckenzie, thanks for opening this issue. My reading of this indicates that Git LFS does not re-authenticate subsequent HTTP requests after being redirected. Looking at the implementation in package lfsapi (in particular, lfsapi/auth.go and lfsapi/client.go), I can confirm that this is the case.

I think that the diff you attached is on the right track, but I don't think that it's the entire fix that I'd like to merge. One thing in particular is that doWithRedirects tracks via []*http.Request so that we don't redirect more than three times, whereas DoWithAuth (though it eventually calls doWithRedirects) will in-effect reset this count to 0. I'd like to retain this feature, so I think that it's important that our solution consider that.

I wrote an implementation of this in https://github.com/git-lfs/git-lfs/tree/lfsapi-authenticate-redirects, and I would be grateful if you could run it yourself and let me know whether it works as expected or not. Here are some builds with that change applied:

git-lfs-darwin-386-2.4.0.tar.gz
git-lfs-darwin-amd64-2.4.0.tar.gz
git-lfs-freebsd-386-2.4.0.tar.gz
git-lfs-freebsd-amd64-2.4.0.tar.gz
git-lfs-linux-386-2.4.0.tar.gz
git-lfs-linux-amd64-2.4.0.tar.gz
git-lfs-windows-386-2.4.0.zip
git-lfs-windows-amd64-2.4.0.zip

@ttaylorr
Copy link
Contributor

@ttaylorr ttaylorr commented May 25, 2018

Here's a pull-request that fixes the issue: #3028.

@ashmckenzie
Copy link
Author

@ashmckenzie ashmckenzie commented May 25, 2018

Amazing speedy work @ttaylorr ! Have confirmed in my scenario this fixes the issue I'm seeing 👍 💯

@ashmckenzie
Copy link
Author

@ashmckenzie ashmckenzie commented May 29, 2018

Well done mate, top job! We are very grateful :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants