smarty-archives / go-aws-auth

[DEPRECATED] Signs requests to Amazon Web Services (AWS) using IAM roles or signed signature versions 2, 3, and 4. Supports S3 and STS.
https://github.com/smartystreets/go-aws-auth/issues/49
Other
216 stars 71 forks source link

Adding security token handling #6

Closed drocamor closed 10 years ago

drocamor commented 10 years ago

Hi @mholt,

This is not really ready to merge, but I would like some feedback.

I have added the ability to get STS tokens into v4 requests, and this is working. I even added a test for it. I didn't add this into the other requests yet because it was not totally clear in the docs.

I am not super happy about adding the SecurityToken field to the Credentials type. This means you have to add blank items in when you initialize a Credentials struct. This seems tedious. What do you think?

Thanks, Dave

mholt commented 10 years ago

That's looking pretty good. I just pulled it down and it all checks out indeed.

You may be right about keeping the token in the Credentials object... it is kind of strange that not all the fields may be used. Pre-1.0, I'm thinking it's probably fine, and maybe by the 1.0 mark we'll decide on a better credential storage. To avoid the empty string, you could use named parameter initialization:

creds = &Credentials{
    AccessKeyID:     "KEYID",
    SecretAccessKey: "SECRETKEY",
}

It is more verbose, but some people prefer that I guess.

As for which requests to insert it with, let's do that incrementally, or as an option... in other words, let the user of the package decide. Anyway, as you added a test, and the tests are passing, I'd be OK with merging it in when you're ready.

drocamor commented 10 years ago

Cool. I have used the named parameter initialization and updated the readme. I am going to add token support to v2 requests (because that is documented). I know it is possible with s3 and v3 requests, but I may need to do some more digging. I should have some time this weekend.

mholt commented 10 years ago

Great; I'll finalize this merge today or tomorrow.

Internally at work we are looking into adding support for signed S3 URLs which allow you to download resources from S3 with just a URL. Hopefully we'll get around to that pretty soon. I do look forward to seeing what you find out!

drocamor commented 10 years ago

Hi @mholt !

With a bit more work I have v2, v3, and S3 signatures working with STS. So this is pretty much STS anywhere. I'll start on a new PR that will handle IAM role authentication next.

-Dave

mholt commented 10 years ago

That would be awesome! This week at work I plan on implementing those signed S3 URLs (to GET a resource from S3 without needing to send headers and stuff; all you need is the URL). This looks great; I'm merging it in.