Upgrade Authentication to use AWS Signature Version 4 #50

estadtherr · 2016-10-04T22:08:15Z

This pull request upgrades the authentication logic from version 2 to version 4 of the AWS Signature algorithm. Since more information is needed in the bucket context to compute the signature (namely the region), this is a non-backward-compatible change to the API. Consequently, I bumped the library major version from 2 to 3. I chose to support only version 4 instead of trying to support both V2 and V4, since it made the implementation more intuitive, and V4 is supported everywhere according to this note in the AWS documentation:

Note
Amazon S3 supports Signature Version 4, a protocol for authenticating inbound API requests to AWS services, in all AWS regions. At this time, AWS regions created before January 30, 2014 will continue to support the previous protocol, Signature Version 2. Any new regions after January 30, 2014 will support only Signature Version 4 and therefore all requests to those regions must be made with Signature Version 4.

As an added bonus, adding region specification to the API should make the library function much better for buckets that reside somewhere other than the default 'us-east-1' region.

Mac OS builds use the SHA/HMAC algorithms built into libSystem. Linux builds use the SHA/HMAC algorithms in libssl/libcrypto (part of OpenSSL). I am not familiar with mingw or windows, so that build may or may not need additional work. The SHA1 implementation has been removed, along with everything else that only applied to the older signature method.

This change also fixes the Mac OS compilation error referenced in #47, in a way that remains compatible with Linux builds.

…n/signature logic

… script

…PI change

… headers

bosim · 2016-12-13T22:32:44Z

@bji do you know which of the two pull request you are going to merge in?

We are relying on the way of setting the region as this pull request implements.

bji · 2016-12-13T23:04:32Z

Do you have a preference? This one looks better to me, do you agree?

estadtherr · 2016-12-14T16:18:01Z

I'm obviously biased, and would prefer to see this PR merged, but I can at least tell you the reasons I proceeded with a fresh implementation rather than use the other PR:

This PR removes support for protocol V2, which simplifies the code and, since Amazon supports V4 everywhere, is compatible with every region/environment (and I have tested it in multiple environments)
This PR is a more complete implementation (e.g. by supporting single-resource query strings)
The region specification in this PR is more amenable to API integration within other applications. The other PR follows the "s3" command-line utility convention of using an environment variable to specify region, which makes the API behavior inconsistent.
This PR has a more native Mac OS implementation using encryption libraries already available on the platform instead of adding an OpenSSL dependency.
This PR corrects some other minor issues discovered along the way, such as the fix for Fix OSX build error due to uint64_t format string incompatibility #47, and straightening out some confusion between sub-resources and query-strings in upload/copy operations.

Let me know if you have questions after performing a review!

bji · 2016-12-14T16:42:54Z

Thank you for the excellent write-up!

bji · 2016-12-14T16:44:47Z

GNUmakefile.osx

@@ -102,6 +102,7 @@ endif

 ifndef LIBXML2_LIBS
    LIBXML2_LIBS := $(shell xml2-config --libs)
+    LIBXML2_LIBS := $(filter-out -L$(shell xcrun --show-sdk-path)/usr/lib, $(LIBXML2_LIBS))


Can you explain this line please? I don't even have the xcrun command on my Fedora installation. What is this supposed to do?

Oh I just noticed that this is in the osx version of the Makefile, so I guess it's an osx specific command. Still curious about what this change is supposed to do.

This is actually a workaround for an Xcode error on Mac OS 10.11. See this Stack Overflow post for more information. I just built it now successfully without the workaround on Mac OS 10.12 (to which I've upgraded since I worked on this PR), so it may not be needed unless someone is building on 10.11. We could remove it and let people discover the workaround if they're on 10.11, or keep the workaround as a comment with a note about 10.11. What's your preference?

My preference is to keep the workaround but add comments to the makefile so that it's not mysterious.

Well anyway, you've already done the lion's share of the work, I'll just take the commit and add the comments myself. Thanks.

bji · 2016-12-14T16:48:27Z

inc/util.h

-// base64 encode bytes.  The output buffer must have at least
-// ((4 * (inLen + 1)) / 3) bytes in it.  Returns the number of bytes written
-// to [out].
-int base64Encode(const unsigned char *in, int inLen, char *out);


Curious -- why did you remove the signatures of these funtions from this header file?

The base64Encode() function was only used by the implementation of version 2 of the signature algorithm. All of the hashing/encoding in version 4 is done with HMAC/SHA256 routines, so the base64 implementation was no longer needed.

Does that mean that these functions can be removed from util.c as well?

I removed them from util.c in the same commit (584bf48).

Hm, sorry, somehow I didn't see that, and I looked over the diff several times. Must have missed it.

Ah, it was hidden behind a 'show diff' link. Sorry I am not used to github's change inspection system.

yeah, that's caught me before, too! I think it does that with diffs larger than some threshold.

bji · 2016-12-14T18:05:55Z

Thank you again!

bji · 2016-12-14T18:25:18Z

On 12/14/16 10:16, Eric Stadtherr wrote: My pleasure! Would you like another PR adding a comment about the Mac OS Makefile workaround? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#50 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABDjMbnRzPBgQUuLLPsGbOfFYV2pPjjfks5rIDKNgaJpZM4KONsO>.

Thanks for the offer, but I already did it, it's in a subsequent commit. Long lost in the annals of history are the reasons why I didn't use openssl versions of the SHA1 or base64encode functions or whatever else was custom added to util.c. I vaguely recall getting cross-platform versions of those libraries that worked well was a pain in 2008 so I just decided to reduce the dependencies and go with custom code. I have my fingers crossed that the removal of those functions in preference to additional external dependencies is going to work well for all platforms ... Thanks, Bryan

bji · 2016-12-14T18:27:08Z

On 12/14/16 10:16, Eric Stadtherr wrote: My pleasure! Would you like another PR adding a comment about the Mac OS Makefile workaround? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#50 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABDjMbnRzPBgQUuLLPsGbOfFYV2pPjjfks5rIDKNgaJpZM4KONsO>.

Actually looking more closely at the change, I guess I missed the fact that the mingw makefile probably doesn't work anymore, as it doesn't include any of the ssl/crypt libs. Or maybe it does work -- did you try it? To be honest I haven't even tried to build the mingw version in about 7 years so it was very likely broken already anyway. Bryan

estadtherr · 2016-12-14T18:32:08Z

Unfortunately I don't have anything resembling a Windows environment, so I wasn't able to build or test the mingw version. Sorry, I tried to point that out in the PR description, but that fact was buried a bit in the description of the crypto libraries that I used on Mac OS and Linux.

How would you like to handle the mingw version with respect to this change?

bji · 2016-12-14T18:50:25Z

On 12/14/16 10:32, Eric Stadtherr wrote: Unfortunately I don't have anything resembling a Windows environment, so I wasn't able to build or test the mingw version. Sorry, I tried to point that out in the PR description, but that fact was buried a bit in the description of the crypto libraries that I used on Mac OS and Linux. How would you like to handle the mingw version with respect to this change? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#50 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABDjMZro92te-jGXTX88c1jpyKNfvP2wks5rIDYogaJpZM4KONsO>.

I think I'd like to punt on the issue. If someone comes along who really still cares about the mingw version, then they can get it to work. Thanks, Bryan

estadtherr · 2016-12-14T19:11:21Z

I think I'd like to punt on the issue. If someone comes along who really still cares about the mingw version, then they can get it to work.

Sounds like a plan - open source philosophy at work!

codemedic · 2018-03-22T21:29:12Z

Hey Bryan ( @bji ).. what is the state of master branch?
I was after the feature added by this PR #50.
A lot seem to have changed since 2.0; i.e no chance of applying it as a patch over v2.0.

Any chance of creating a release?

estadtherr · 2018-03-22T23:15:19Z

@codemedic - this PR was merged to master in this commit: 86cbc59
I know some Linux distributions have picked up version 3.0 (and even version 4.0) since then.

codemedic · 2018-03-23T09:26:32Z

Thanks @estadtherr .. are these releases tagged or branched?
I couldn't find any, from the repo.

estadtherr · 2018-03-29T22:01:46Z

I suspect the OS distribution maintainers grabbed an untagged commit, and did their own packaging and testing from there.

Eric Stadtherr added 13 commits September 29, 2016 14:27

got v4 auth working on Mac OS

da1f0fb

adapted v4 signature construction for Linux

1b714e0

add auth. region to relevant API calls

34d11b2

add openssl libs to makefile, fix test code on Linux

e67a3f6

support the generate_query_string operation with the new authorizatio…

0aedf1f

…n/signature logic

fix query parameter handling in canonicalization; fix echoing in test…

226bc75

… script

remove obsolete hash functions, update library major version due to A…

584bf48

…PI change

fix expected output for ACL with new identifier format

e466cc7

missed some whitespace in previous commit

c3ed9e5

fix uninitialized memory issue

20785ed

increase length of auth. header to account for requests with many amz…

02f519d

… headers

clean up test code

0feed6b

remove temporary test drivers from makefiles

68d30d6

estadtherr mentioned this pull request Oct 6, 2016

Addition of Amazon S3 tile store module openstreetmap/mod_tile#143

Open

fix the logic that determines the hostname to use in HTTP headers

61f6565

andreikop mentioned this pull request Oct 27, 2016

Fix minio compatibility. Do not append empty Transfer-Encoding header. #51

Merged

Merge branch 'master' into aws_auth_v4_pr

8a3b971

bji reviewed Dec 14, 2016

View reviewed changes

bji merged commit 86cbc59 into bji:master Dec 14, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade Authentication to use AWS Signature Version 4 #50

Upgrade Authentication to use AWS Signature Version 4 #50

estadtherr commented Oct 4, 2016 •

edited

Loading

bosim commented Dec 13, 2016

bji commented Dec 13, 2016

estadtherr commented Dec 14, 2016

bji commented Dec 14, 2016

bji Dec 14, 2016

bji Dec 14, 2016

estadtherr Dec 14, 2016

bji Dec 14, 2016

bji Dec 14, 2016

bji Dec 14, 2016

estadtherr Dec 14, 2016 •

edited

Loading

bji Dec 14, 2016

estadtherr Dec 14, 2016

bji Dec 14, 2016

bji Dec 14, 2016

estadtherr Dec 14, 2016

bji commented Dec 14, 2016

bji commented Dec 14, 2016 via email

bji commented Dec 14, 2016 via email

estadtherr commented Dec 14, 2016

bji commented Dec 14, 2016 via email

estadtherr commented Dec 14, 2016

codemedic commented Mar 22, 2018

estadtherr commented Mar 22, 2018

codemedic commented Mar 23, 2018

estadtherr commented Mar 29, 2018

Upgrade Authentication to use AWS Signature Version 4 #50

Upgrade Authentication to use AWS Signature Version 4 #50

Conversation

estadtherr commented Oct 4, 2016 • edited Loading

bosim commented Dec 13, 2016

bji commented Dec 13, 2016

estadtherr commented Dec 14, 2016

bji commented Dec 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estadtherr Dec 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bji commented Dec 14, 2016

bji commented Dec 14, 2016 via email

bji commented Dec 14, 2016 via email

estadtherr commented Dec 14, 2016

bji commented Dec 14, 2016 via email

estadtherr commented Dec 14, 2016

codemedic commented Mar 22, 2018

estadtherr commented Mar 22, 2018

codemedic commented Mar 23, 2018

estadtherr commented Mar 29, 2018

estadtherr commented Oct 4, 2016 •

edited

Loading

estadtherr Dec 14, 2016 •

edited

Loading