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

Rm aws keys verification to allow for iam roles #38

Merged
merged 3 commits into from Feb 11, 2019

Conversation

fernandrone
Copy link
Contributor

Remove the validation of aws credentials, since this was blocking the use of IAM roles for authentication.

@fernandrone
Copy link
Contributor Author

I saw that a drone signed yaml was required when using iam roles on https://github.com/drone-plugins/drone-s3 for security reasons. However, since there's no drone sign anymore, I'm wondering if this solution is acceptable.

Remove the validation of aws credentials, since this was blocking
the use of IAM roles for authentication.
@tonglil
Copy link
Member

tonglil commented Jan 24, 2018

To echo my post in discourse:

IMO, I think the yaml should explicitly say iam_roles: true # default false before it tries anything using IAM roles.

By default AWS plugins should require the access key and secret key.

@tonglil tonglil requested review from tboerger and nlf Jan 24, 2018
@tboerger
Copy link
Contributor

Sounds reasonable, I think we should sync that with drone-s3

@fernandrone
Copy link
Contributor Author

@tboerger I have a PR here as well: drone-plugins/drone-s3#41
@tonglil thanks, I replied here: https://discourse.drone.io/t/aws-plugins-using-iam-roles-no-aws-keys/1598/3

tl;dr:

I’m afraid this [iam_roles: true] might actually confuse users.

The default behavior for all official AWS tools I know of is to try multiple different authentication methods. So users familiar with the AWS environment will likely expect drone plugins to adopt the same behavior.

@fernandrone
Copy link
Contributor Author

fernandrone commented Jan 24, 2018

By the way, I haven't really looked into it, but have you considered merging drone-s3/drone-s3-sync into a single plugin, with maybe some sort of "sync: true" parameter? It seems that a good chunk of the code and parameters are the same anyway.

@tonglil
Copy link
Member

tonglil commented Jan 24, 2018

I think the default AWS behavior works if you assume a single tenant or private environment.

Seeing as Drone is a service, I think it typically serves a multi-tenant situation that single tenant, and should comr with safe defaults.
Even in one company, each team might have separate AWS projects.

For example, if Travis-CI's backing infra is AWS, should it's customers be able to assume the instance roles of Travis-CI?

@tonglil
Copy link
Member

tonglil commented Jan 24, 2018

Think about it this way: Drone jobs do not know what platform the agent is running on unless they are told explicitly, otherwise the assumption is based on a leaking abstraction.

Someone running agents in mixed providers will get different experiences.

@fernandrone
Copy link
Contributor Author

For example, if Travis-CI's backing infra is aws, should it's customers be able to assume the instance roles of Travis-CI?

Well, I see your point but the thing is, there's nothing stopping you from running a generic step with an aws cli image, which will automatically assume the roles of the instance anyway. So it's more of an issue of anyone running the aws cli or an aws sdk on Drone would have different experiences on different environments.

Of course, you could argue that this is a bad implementation, and that Drone "official" plugins should "fix" it by adopting a different, "safer" behavior.

Personally, I see plugins such as drone-s3 as pretty much a convenient wrapper of the aws API, so I expect it to work just like most aws tools. But then again your mileage may vary.

@tboerger
Copy link
Contributor

I have to say I tend more into the direction of fbcbarbosa, even if I'm not an aws user. If I remember correctly we already had discussions in that direction and said we would do an automatic consume of the rules.

Even if you don't want to do that, you are still able to drop an iam role or not?

@fernandrone
Copy link
Contributor Author

Hi @tboerger, not sure what you mean by "drop an iam role", could you clarify?

@tboerger
Copy link
Contributor

I mean to define a more restrictive role

@fernandrone
Copy link
Contributor Author

fernandrone commented Jan 26, 2018

Well, if you provide aws credentials through environment variables (AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY), these will be given priority.

So you could provide credentials with less access than the IAM role. But if you don't provide any, and you don't provide credentials through any other means (such as sharing a credentials file at ~/.aws/credentials), you'll fallback to the IAM role.

Not sure if that's an use case though. I mean, IAM roles are attached to an EC2 instance directly. By default they have none, so to give an EC2 instance permission to assume a role is a conscious decision by whomever is maintaining the infrastructure, and any process running on that machine will be able to assume that role through the OS.

So to associate an IAM role to an instance and then choose not to use it is an odd decision; you'd better of not giving the instance a role in the first place.

We're using it because we have dozens of private repositories on Drone that deploy to services such as S3 and Elastic Beanstalk, so maintaining credentials for all these repositories is just too much work (specially without global secrets).

There's more info here: Using an IAM Role to Grant Permissions to Applications Running on Amazon EC2 Instances

Not sure if I answered your questions though.

@tboerger
Copy link
Contributor

You don't need to convince me, I already said that I'm fine without the iam attribute.

@acazacu
Copy link

acazacu commented Nov 19, 2018

Did CI fail on running the checks?

I ran into this issue as well and this PR would have come in handy.

@tboerger
Copy link
Contributor

Let's see if it builds correctly...

@djenriquez
Copy link

Hey guys, any progress with this? Figured the holidays would have slowed things down a bit 😄 .

@tboerger tboerger merged commit 2942dd3 into drone-plugins:master Feb 11, 2019
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.

None yet

6 participants