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

Version signing broken on master #84

Closed
eldondevcg opened this issue Dec 17, 2015 · 6 comments
Closed

Version signing broken on master #84

eldondevcg opened this issue Dec 17, 2015 · 6 comments
Labels

Comments

@eldondevcg
Copy link

It looks like to me, on master, versioning is currently broken:

vagrant@vagrant-ubuntu-trusty-64:/vagrant$ eval "$(gimme 1.5.1)"
go version go1.5.1 linux/amd64
vagrant@vagrant-ubuntu-trusty-64:/vagrant$ git branch
* master
vagrant@vagrant-ubuntu-trusty-64:/vagrant$ go test -run Version
--- FAIL: TestGetVersion (1.00s)
    s3gof3r_test.go:419: version id: rNBjRePSIbXqSyIA6tZ3J6yXLpO2bAPR
    s3gof3r_test.go:423: 403: "The request signature we calculated does not match the signature you provided. Check your key and signing method."
FAIL
exit status 1
FAIL    _/vagrant   4.167s
vagrant@vagrant-ubuntu-trusty-64:/vagrant$ git checkout v0.5.0
Note: checking out 'v0.5.0'.
HEAD is now at 31603a0... version to 0.5.0
vagrant@vagrant-ubuntu-trusty-64:/vagrant$ go test -run Version
PASS
ok      _/vagrant   2.023s
vagrant@vagrant-ubuntu-trusty-64:/vagrant$ 

Looks like the bucket Travis is using is not versioned, so that test is currently being skipped.

@rlmcpherson
Copy link
Owner

Verified that this is broken on master. The url changes broke the current method of embedding the versionID url parameter in the path. I'm considering how to best fix this still. Parsing versionId from the path is not the cleanest.

@rlmcpherson
Copy link
Owner

Looking at this again, the issue with embedding the versionID in the path argument is that S3 paths can contain special url characters like # parsing the path as a url is problematic and was causing #68, for instance. The path is a string representing the path of the object in the bucket and does not represent a url. Therefore it is not required to be url-encoded.

I'd like to fix this elegantly as part of a more-comprehensive api change to s3gof3r including breaking changes.

For now I see two possibilities:

  1. add an api method specific to versioning with a version parameter.
  2. remove versioning support

@eldondevcg
Copy link
Author

Yeah, clearly for our purposes (2) does not work that well. I was planning on filing another bug today (though I hoped to make it a pull request, or at least provide a demonstrative testcase) where it would appear that 0.5.0 is not successful in uploading keys containing colons. Wonder if that also relates to #68 .

@eldondevcg
Copy link
Author

I could look at the implementation of (1) unless you've got a start on it.

@rlmcpherson
Copy link
Owner

@eldondevcg A third alternative is to specifically handle the versionID query parameter but not url query parameters in general. I'm working on a pr now.

@rlmcpherson
Copy link
Owner

It's not pretty, but this fixes versioning in my tests. @eldondevcg can you try testing it as well?

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

No branches or pull requests

2 participants