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

Support AWS IAM Role credentials #81

Closed
medianotion opened this issue Jun 20, 2019 · 17 comments
Closed

Support AWS IAM Role credentials #81

medianotion opened this issue Jun 20, 2019 · 17 comments

Comments

@medianotion
Copy link

Provide support for AWS IAM Role credentials rather than having to set long term credentials in the sleet.json or reference a user's .aws profile. This would allow Sleet to be used with automated build processes that run under an IAM Role and be more secure.

@emgarten
Copy link
Owner

This sounds like a useful feature to me. Do you know what changes would be needed to make this happen?

@medianotion
Copy link
Author

medianotion commented Jun 20, 2019 via email

@emgarten
Copy link
Owner

Setting profileName in sleet.json is supported today. You can see the change here: https://github.com/emgarten/Sleet/pull/54/files

Does this work for your environment, or do you still need env var support?

@medianotion
Copy link
Author

medianotion commented Jun 20, 2019 via email

@dpryden
Copy link

dpryden commented Aug 30, 2019

I will just add that I'm currently evaluating using Sleet for a project I'm working on, and the lack of support for environment variables makes Sleet a non-starter for me.

In the specific case of a short-term credential (such as what sts:AssumeRole or sts:GetSessionToken returns), you need three pieces of information: the access key id, the access key secret, and the session token. Without the session token, the other two are insufficient to authenticate. The normal way that these get propagated is in environment variables, and the standard Amazon SDK classes already automatically detect the presence of these credentials and use them.

Unfortunately, if a profile is explicitly provided, it overrides the ambient credentials from the environment, which means that there is no way to include the session token, even if it is present. This is especially problematic in a situation where you have multiple Amazon accounts involved, since in a typical configuration, the credentials that can be selected using a profile would not have sufficient permissions to actually interact with an S3 bucket in a different account.

Separately from that, if running on an EC2 instance with an Instance Role assigned, the credentials that you want to use will need to be retrieved from the EC2 metadata API. The Amazon SDK classes already know about this and will fall back to the Instance Profile service on the metadata API endpoint automatically if no other credentials are present.

I believe that the code change required to do this is simple. You just need to allow omitting both the profile and the access key from the sleet.json; if neither are present, use the parameterless constructor of AmazonS3Client() and I believe it will just work. Basically: just remove the code that throws an exception here: https://github.com/emgarten/Sleet/blob/master/src/SleetLib/FileSystem/FileSystemFactory.cs#L129

If you wanted to be more explicit about it, you could also provide a dedicated config value in sleet.json to select this mode; e.g. something like "useInstanceCredentials": true, or "useIAMRole": true (as @medianotion suggested), or something. I'm not sure that that gains you much, though.

I'm happy to try to put together a pull request for this if it would help.

@emgarten
Copy link
Owner

emgarten commented Sep 1, 2019

@dpryden a PR for this would be great! I'm happy to help out, so if you have an issues once you get into it just create a work in progress PR and ping me on it.

I don't understand S3 credentials as well as Azure, so if you can make a change and verify that it works correctly in your EC2 environment it would be a big help for this.

I'm fine with leaving out the additional sleet.json setting for this as long as the user gets a helpful error message when the feed isn't configured properly. I agree that it wouldn't add much, but if the experience from defaulting to AmazonS3Client() is cryptic when it goes to use it later, then I think an explicit opt in is needed.

Also, docs for S3 sleet are here: https://github.com/emgarten/Sleet/blob/master/doc/feed-type-s3.md If you create a PR feel free to add an example of the sleet.json needed to use the feature. S3 seems to be the most popular host for Sleet feeds, so I'm sure others will want to use this feature as well.

@emgarten
Copy link
Owner

emgarten commented Sep 5, 2019

@medianotion have you a chance to look at this?

@medianotion
Copy link
Author

medianotion commented Sep 6, 2019 via email

@emgarten
Copy link
Owner

Fixed by @iainb123 with #90

@emgarten
Copy link
Owner

This change is in https://www.nuget.org/packages/Sleet/3.0.5

@iainb123
Copy link

iainb123 commented Sep 26, 2019 via email

@dpryden
Copy link

dpryden commented Sep 26, 2019

Thanks for putting together that fix, @iainb123. My apologies for not contributing a PR as I had said I could; I've been busy with other things and ultimately our team is not using Sleet for unrelated reasons, so it's hard to justify spending time on it.

One thing I will point out: the fix as currently implemented only will work for an EC2 Instance Profile role, since it requires the EC2 metadata API (169.254.169.254) to be accessible. This does not help for the use case I had in mind, which is an IAM Role assumed across accounts, where the access key, access secret, and session token are all in the environment.

My recommendation is to rip out all of this error handling code and instead just call sts:GetCallerIdentity (I don't know offhand where that is exposed in the C# API, though). If you get a valid caller identity, then you are good to go, regardless of what settings may or may not be present. If you don't get a valid caller identity, then you can try to throw a more helpful error message, although it's probably not helping as much as you might think it is.

@iainb123
Copy link

iainb123 commented Sep 26, 2019 via email

@medianotion
Copy link
Author

medianotion commented Sep 26, 2019 via email

@iainb123
Copy link

iainb123 commented Sep 26, 2019 via email

@medianotion
Copy link
Author

medianotion commented Sep 26, 2019 via email

@emgarten emgarten reopened this Sep 26, 2019
@emgarten
Copy link
Owner

Fixed in #91

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

No branches or pull requests

4 participants