DownloadReleaseAsset fails.

[vsmart]

Hi,

We are trying to a download a release jar with DownloadReleaseAsset.
It fails on this:

<?xml version="1.0" encoding="UTF-8"?>
<Error><Code>InvalidArgument</Code><Message>Only one auth mechanism allowed; only the X-Amz-Algorithm query parameter, Signature query string parameter or the Authorization header should be specified</Message><ArgumentName>Authorization</ArgumentName><ArgumentValue>Bearer{token}</ArgumentValue><RequestId> {req-id} </RequestId><HostId> {host} </HostId></Error>

Could you point us in the right direction?
Thank you.

[willnorris]

Could you provide a snippet of the exact call you're making?

[vito]

Seeing this too. It only seems to happen when using a client that has a token configured. Perhaps it's propagating the Authorization header through to the S3 redirect, which AWS seems to reject?

For me it doesn't technically fail, it just downloads the aforementioned XML instead of the actual contents of the artifact, since it doesn't check the response status code:

go-github/github/repos_releases.go

Lines 230 to 235 in 84fc804

	resp, err := s.client.client.Do(req)
	if err != nil {
	return nil, err
	}
	return resp.Body, nil

Here's our code that exhibits the problem:

https://github.com/concourse/github-release-resource/blob/91b8f79018718c8256faa29ddc43aed87191e053/github.go

...if this client is constructed with a token configured in Source, we run into this issue.

[xoebus]

I've made a little package that allows a per-host http.RoundTripper. We're using this to get around this at the moment. i.e. api.github.com uses an OAuth Transport and then anything else uses the default transport.

[gmlewis]

To keep this simple, we are thinking about breaking the github.DownloadReleaseAsset API and additionally returning a URL, like this:

// DownloadReleaseAsset downloads a release asset.
//...
// If a redirect is returned, the redirect URL will be returned as a string instead
// of the io.ReadCloser. Exactly one of io.ReadCloser and url string will be zero.
//...
func (s *RepositoriesService) DownloadReleaseAsset(owner, repo string, id int) (io.ReadCloser, string, error) {

If a redirect to AWS occurs, then the redirect will be canceled and the redirect Location will be returned in the string and the io.ReadCloser will be nil. At this point, the user would need to perform the http.Get on the returned URL using a transport without the GitHub authentication.

We are also thinking about adding a mutex to protect the http.Client's CheckRedirect func to temporarily disable the redirect.

Comments on this plan are welcome.

[dmitshur]

One minor suggestion (if you choose to go with the DownloadReleaseAsset plan), I think the documentation would be better with named returned values, specifically to document the string. Something like this:

// DownloadReleaseAsset downloads a release asset.
//...
// If a redirect is returned, redirectURL will be returned as a string instead
// of the io.ReadCloser. Exactly one of io.ReadCloser and redirectURL will be zero.
//...
func (s *RepositoriesService) DownloadReleaseAsset(owner, repo string, id int) (rc io.ReadCloser, redirectURL string, err error) {

[willnorris]

Agreed that named return values would be helpful here.

@shurcooL, Glenn and I talked through a number of different options yesterday, and this was basically the "least bad" that we could come up with. But we're certainly open to other ideas. I'd love to be able to follow the redirect and and return an io.ReadCloser for the Amazon-hosted file, but I can't come up with a clean way to enable that.

What we need is an http.RoundTripper that doesn't add auth headers. We can't simply use a zero-value http.Transport because that won't work in many environments such as App Engine and Google's production environment. That's why we alway require that the user of go-github provide the underlying http.Client to use.

So we have the http.RoundTripper that is on the http.Client that the user provides, but that is almost certainly going to be a transport that adds auth headers... either from golang.org/x/oauth2, the older goauth2 library, or the BasicAuthTransport or UnauthenticatedRateLimitedTransport provided by go-github. We could do a type switch to pull out the underlying RoundTripper for these four types, but that will of course only work if it's one of these four. And it creates a new compile-time dependency on these oauth libraries, which I've successfully avoided thus far.

We looked at having the caller provide a "clean" http.RoundTripper to the DownloadReleaseAsset method, but that didn't feel as clean as simply returning the URL and letting the caller fetch the resource directly.

[vito]

For what it's worth, the package @xoebus linked (https://github.com/xoebus/statham) handles this pretty cleanly and doesn't require any changes to go-github itself. We've started using it to fix our use case and it doesn't really feel like a "workaround". It's a tiny bit of code that I was surprised to not already find somewhere else given how often *http.Clients are injected for oauth stuff.

[willnorris]

@vito: yeah, I saw that as well and thought it was pretty neat. But I also don't want to rely on users always using that. Perhaps we could attempt to use the provided client, and only if Amazon returns an error do we bail and just return the redirectURL to the caller.

[dmitshur]

@shurcooL, Glenn and I talked through a number of different options yesterday, and this was basically the "least bad" that we could come up with. But we're certainly open to other ideas.

I like returning redirectURL. It's simple and fits well in the scope of the library. I doubt it's worth it to try to do more, given the unpredictable nature of that URL (as I understand it) and the fact it effectively needs its own *http.Client.