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

LFS 2.0 doesn't include basic auth when doing a verify. #2016

Closed
sgoo opened this Issue Mar 9, 2017 · 5 comments

Comments

Projects
None yet
5 participants
@sgoo

sgoo commented Mar 9, 2017

In 1.5.6 and earlier when doing the verify action the LFS client would include a Authorization: Basic header with basic auth from the git credential manager.

However since 2.0.0 it no longer includes this information.

A bisect shows the change was introduced in e1b4563 which looks unrelated to such a change.
Was this an intentional change or is this a bug? The spec isn't very clear, but does say that the client uses basic auth.

@ttaylorr

This comment has been minimized.

Member

ttaylorr commented Mar 9, 2017

@sgoo thanks for opening this, and nice find. I can't remember where we landed on this when working on the lfsapi package, so I'm going to defer to @technoweenie.

@technoweenie I vaguely recall you suggesting that we not include header Authorization header in the verify requests. Is this correct?

@sgoo

This comment has been minimized.

sgoo commented Mar 9, 2017

I was reading the spec some more and what we are doing doesn't really fit with the spec here I guess.

For upload requests we are returning a both a upload and verify action.

The upload request goes to a seperate service that has its own authentication and we include that as a Authorization header in our response.
The verify requests comes back to the same service that processed the batch request. So we expect LFS to attach the same auth it used for the batch request.

Reading batch.md a bit more and looking into the authenticated option. Given that it applies to all actions equally it doesn't really fit with what we are doing here?

I'm still not really sure what the correct behaviour should be here.

@sinbad

This comment has been minimized.

Contributor

sinbad commented Mar 9, 2017

Looking at the code this got lost in the API refactor because the previous behaviour was:

func VerifyUpload(cfg *config.Configuration, obj *ObjectResource) error {
        // .....
	res, err := DoRequest(req, true)

Where the second argument to DoRequest was useCreds. It was explicitly set to true before which meant that if the first response was an auth error, the creds were added.

I think you could make the argument that creds shouldn't be filled if the authenticated response of the original batch call was true, but in this case it's not supplied which according to the spec means it's false, meaning that git credentials will be looked up for the URLs. This is a bit of a funny case because the upload URL actually does include an Authorization header, but the verify doesn't. But, given that the API allowing separately provided URLs means they can be on different systems I don't think this is necessarily incorrect.

@technoweenie

This comment has been minimized.

Member

technoweenie commented Mar 9, 2017

If the verify url has no authenticated property and no Authorization header, it probably should be checking git credentials.

@basmevissen

This comment has been minimized.

basmevissen commented Apr 3, 2017

Ok, this seems fine in Git-LFS 2.0.2 now. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment