Skip to content

Conversation

@andreikop
Copy link
Contributor

I don't know why Transfer-Encoding: is appended, but it seems like the header is silently ignored by curl.
However when S3 auth v4 is used, this header is included to a list of signed headers and breaks authorization. At list for minio.
This PR together with #50 allows to use libs3 with minio server implementation.

My master branch contains both this PR and #50 and works with minio

Empty header is not sent by curl, but gets included to the list of signed
headers and breaks authorization for minio. And probably for other
server implementations too.
@bji
Copy link
Owner

bji commented Nov 3, 2016

Thank you!

@bji bji merged commit ebd0789 into bji:master Nov 3, 2016
@bosim
Copy link

bosim commented Nov 4, 2016

It seems to break AWS S3.
Got ErrorNotImplemented
A header you provided implies functionality that is not implemented
Header: Transfer-Encoding

@bji
Copy link
Owner

bji commented Nov 4, 2016

OK thanks.

@bji
Copy link
Owner

bji commented Nov 4, 2016

Sorry so bosim, you're saying that having Transfer-Encoding breaks functionality? So this change that I merged removed Transfer-Encoding, therefore I think you are saying that this change should be kept, and not reverted, correct?

@bosim
Copy link

bosim commented Nov 4, 2016

Removing Transfer-Encoding gives the error I mentioned above, so basically this commit breaks AWS compatibility. I can connect fine to miniio using PR #50 with the Transfer-Encoding present (just tried).

@bji
Copy link
Owner

bji commented Nov 4, 2016

OK thanks for the clarification I will revert the change.

@bji
Copy link
Owner

bji commented Nov 4, 2016

I have reverted the change. I have to admit that I don't really actively develop libs3 anymore and am not even doing basic checks when I merge the pull requests, just trusting that the requester has done due diligence. I do look at the code to make sure that it looks sane and that I can't easily detect any attempts to put backdoors or security violations in.

I think that andreikop's change may or may not be necessary after I merge in the v4 authentication pull requests, which I have not done because they are much more significant and I feel like I have to do much more due diligence with them, which I haven't found the time/energy for yet.

@andreikop
Copy link
Contributor Author

@bosim, which libcurl version do you use?
Could you provide dumps of:

  • succesfull request without the fix to Amazon S3
  • succesfull request without the fix to minio with V4 auth
  • failed request to Amazon S3
    It is easier to record dumps with Wireshark, than press right button, chose Follow -> TCP stream and copy-paste the text

@bosim
Copy link

bosim commented Nov 6, 2016

@andreikop The machine I used for testing was openSUSE LEAP 42.2, i.e. libcurl version 7.37.0-13.1

I will try to provide the dumps tomorrow.

@bosim
Copy link

bosim commented Nov 7, 2016

@andreikop Could you maybe test #50 with the recent version of miniio to see if stuff works out for you?

@andreikop
Copy link
Contributor Author

@bosim, at first I tried to use #50, than added this patch because libs3 sends wrong authorization.
My master contains #50 + my patch

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants