search

drone-plugins / drone-s3

Drone plugin for publishing artifacts to Amazon S3
http://plugins.drone.io/drone-plugins/drone-s3
Apache License 2.0
36 stars 64 forks source link

Allow for the s3 plugin to use the ec2 instance auth #19

Closed josmo closed 8 years ago

josmo commented 8 years ago

Changing to not setting the auth on the constructor if not key is provided. Using the Setenv (unless there's a better suggestion). This will allow using the ec2 perms to be used to upload to s3

tboerger commented 8 years ago

If we really do it like that we should have the same behavior for all aws based plugins

josmo commented 8 years ago

@tboerger Agree on making it the same way and totally down on however that is, but setting the auth empty doesn't allow for the ec2 instance profile to work. If there's a better or alternative way I'm down to change it :)

tboerger commented 8 years ago

But that change would also apply for ami roles or not? For other aws plugins we already had requests to avoid a hard dependency for the provided secrets

josmo commented 8 years ago

I believe so. The recommended was is http://docs.aws.amazon.com/AWSJavaScriptSDK/guide/node-configuring.html. by passing it in the constructor it forces the hardcoded way, even if it's empty (I think, can totally stand to be corrected but it seemed like that's the way it's working for me at least).

josmo commented 8 years ago

@bradrydzewski you were right it works by just removing it so it's not needed. Thanks for catching that!

bradrydzewski commented 8 years ago

let's re-submit this based on above feedback. Closing so it doesn't get accidentally merged since plugins auto-upgrade.

tboerger commented 8 years ago

As far as I know the value of https://github.com/josmo/drone-s3/blob/master/main.go#L27-L31 gets populated by the option or the env variables PLUGIN_ACCESS_KEY, AWS_ACCESS_KEY_ID. But this won't work in the other direction, it won't set the env variables, it just consumes them. So we should set some sane default of those variables are set and not drop the entire authentication.

Beside that there is still an open pull request which adds the use_iam flag to ignore the secrets in favor of the assigned iam role: https://github.com/drone-plugins/drone-s3/pull/12

tboerger commented 8 years ago

Maybe like that? https://github.com/drone-plugins/drone-ecr/pull/16#issuecomment-226696026

bradrydzewski commented 8 years ago

I just wanted to add a note here that I included in the ECR plugin issue ..

I just want to point out the possible security vulnerability here. This means ANY build running on your server will have access to your infrastructure via iam. This included a malicious pull request. I would not recommend using iam with a public github repository.

Typically this is not an issue because you have to sign the yaml and the passwords are not provided to the container if the signature doesn't match. In this case there are no passwords, and therefore, the step can run without a signature.