search

skypilot-org / skypilot

SkyPilot: Run AI and batch jobs on any infra (Kubernetes or 12+ clouds). Get unified execution, cost savings, and high GPU availability via a simple interface.
https://skypilot.readthedocs.io
Apache License 2.0
6.73k stars 498 forks source link

[Core][AWS] Allow the config to set IAM roles for resources being launched by AWS #3487

Closed JGSweets closed 5 months ago

JGSweets commented 6 months ago

Skypilot forces all of its created resources to utilize skypilot-[v1] IAM role.

The features requested:

  1. Allow specification of the IAM role
  2. Allow specification of which resources get which IAM role.

Reasoning:

concretevitamin commented 6 months ago

Thanks for filing this @JGSweets! We'd love to understand more how you/your team plan to deploy SkyPilot. Is it possible to share some info? Feel free to talk with the dev team on Slack too: https://slack.skypilot.co/

Michaelvll commented 6 months ago

Thanks for the issue @JGSweets! Just a few questions.

In order to follow naming conventions, users would need to be able to set the name of the IAM role

The IAM role only needs to be created once by the admin, and all the user under the same account could share the IAM role. Do you think this is too demanding to the user?

not all resources created need the extensive permissions allowed by the default IAM role.

We have a page for the admin to create an IAM role that has the minimal permission to use SkyPilot. https://skypilot.readthedocs.io/en/latest/cloud-setup/cloud-permissions/aws.html#minimal-permissions Do you mean the permissions mentioned here are still too much?

JGSweets commented 6 months ago

The IAM role only needs to be created once by the admin, and all the user under the same account could share the IAM role. Do you think this is too demanding to the user?

I'm suggesting that the naming of the IAM in this case is hardcoded in the DEFAULT_SKYPILOT_INSTANCE_PROFILE Which also suggests this could change in a PR since it is v1 / controlled by other parameters. It's change would be a breaking change. Allowing users to set the IAM role also allows them to ensure that it is static. Also for security, being able to alter the value to something different than a hard coded value would be helpful.

Do you mean the permissions mentioned here are still too much?

I've been in environments where this would be too permissive. This could be curtailed down to only allowing the setting of specific tags and only allowing alteration of EC2s with those tags. ETC.

However, limiting the role of controller vs replicas is helpful to limit the opportunity for an attacker if they were to have access to a replica vs a controller which has control over other EC2s or other access permissions.

JGSweets commented 6 months ago

@concretevitamin @Michaelvll Thanks for the quick response. I also added a PR which I'm happy to modify or take feedback on to fit your all's vision for skypilot here: https://github.com/skypilot-org/skypilot/pull/3488

I have one for security groups as well which will address https://github.com/skypilot-org/skypilot/issues/3473. However, it isn't as clean an update as the IAM roles which is why I made this first.

Thanks all!