lfsapi: re-authenticate HTTP redirects when needed#3028
Merged
Conversation
ashmckenzie
reviewed
May 25, 2018
| } | ||
|
|
||
| func (c *Client) doWithCreds(req *http.Request, credHelper CredentialHelper, creds Creds, credsURL *url.URL, access Access) (*http.Response, error) { | ||
| func (c *Client) doWithCreds(req *http.Request, credHelper CredentialHelper, creds Creds, credsURL *url.URL, access Access, via []*http.Request) (*http.Response, error) { |
|
Looks good! I started to prepare a fix but found myself putting Challenging one to test! |
Contributor
Author
Thanks for the reminder. We test package |
Contributor
Author
|
Windows failures are unrelated. |
|
Thanks for fixing this @ttaylorr |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request teaches package
lfsapito re-authenticate HTTP requests when the response indicates a redirection.When an HTTP request is sent through package
lfsapi, there are a few paths that we can take down intodo()anddoWithRedirects(). The one that we're concerned with involves one of two outcomes:We send an HTTP request that is responded to with a 301 or 302 code. If we're within our retry limit, we construct a new request sent to the new host/path, and copy over the headers from the old request [1]. Notably, we do not copy the
Authorizationheader iff the new host differs from the old one, as a security concern. We retry the request.Same setup as (1), except we have exceeded our retry count. We complain and fail [2].
In case (1) this security measure has the opportunity to fail when the redirected host differs from the requested host and the redirected host requires authentication. Because we recur early on
doWithRedirects(), and notDoWithAuth(), we don't re-authenticate the request, and instead devolve into a case like #3025.But, recurring through
DoWithAuth()would throw away our statevia, allowing authenticated request to flow through different pairs of hosts infinitely. That's no good.Instead, let's try this:
do()to takevia []*http.Request, the list of requests that have lead up to this one (redirects), ornilif there are no such requests.remoteassociated with a given request, or""if no remote matches, or we don't care which remote is associated.doWithAuthand override thevialist such that we can re-authenticate requests across different hosts.One tricky thing in 5257e58 is that we don't actually know whether the original request was designated to be authenticated or not. An additional tricky thing is that we might never authenticate the original request, and instead load from
http.[$url.]extraHeaders, such thatAuthorizationheader is present, but Git LFS didn't put it there.In all of those cases, we reduce the problem to checking whether the original request contained an
Authorizationheader.Closes: #3025.
/cc @git-lfs/core #3025