-
Notifications
You must be signed in to change notification settings - Fork 5
🔒 Improve AWS Cloud IAM policy security #839
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
base: master
Are you sure you want to change the base?
Conversation
- Scope IAM roles and instance profiles to TowerForge* prefix - Add resource-level restrictions for EC2 operations - Split launch policy into granular statements with conditions - Restrict instance operations to TowerForge-* tagged resources 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
✅ Deploy Preview for seqera-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Thanks, @kenibrewer! Adding Georgi for a dev review and then good to merge. |
georgi-seqera
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At a glance looks good to me, but I've not really been involved in the AWS Cloud CE work, so I'll defer to @stefanoboriero who is more familiar with the resource management of the AWS Cloud CE
|
Do the same changes need to be made for Enterprise docs too? Or are the pages linked? |
stefanoboriero
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main reason why I didn't scope them down by prefix in the docs is that the prefix is actually configurable by Enterprise users (see TOWER_FORGE_PREFIX in https://docs.seqera.io/platform-enterprise/enterprise/configuration/overview), and I wanted to be as generic as possible. If we want to add this to the docs, we should be clear that if you override the default forge prefix this permissions cannot be copy-pasted.
| "arn:aws:ec2:*:*:instance/*", | ||
| "arn:aws:ec2:*:*:volume/*", | ||
| "arn:aws:ec2:*:*:network-interface/*", | ||
| "arn:aws:ec2:*:*:subnet/*", | ||
| "arn:aws:ec2:*:*:security-group/*", | ||
| "arn:aws:ec2:*:*:key-pair/*", | ||
| "arn:aws:ec2:*:*:image/*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really any different than using *? Actions are anyhow not applicable to all resource types, so defining individual resource types still with a star permission doesn't seem to change much the scope of the actual permissions.
For example, with the current config I can't use ec2:RunInstances on a Kinesis stream, because the action already carries the scope of what resource type it can be taken against.
On the other hand I get this might have a placebo effect if someone reads it through without giving it much thought, it seems more scoped.
| "Sid": "AwsCloudLaunchInstances", | ||
| "Effect": "Allow", | ||
| "Action": [ | ||
| "ec2:DescribeInstances", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you test this? AWS Docs say that using the ec2:ResourceTag/Name condition key is not supported for action ec2:DescribeInstances. See https://docs.aws.amazon.com/service-authorization/latest/reference/list_amazonec2.html#amazonec2-actions-as-permissions.
| "Action": [ | ||
| "logs:GetLogEvents" | ||
| ], | ||
| "Resource": "arn:aws:logs:*:*:log-group:*:log-stream:*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above, this does not seem to move the needle much in terms of actual permission scopes. However here we could be more specific, since we know the exact log group we're interested into, which is tower/nextflow-run
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. Please implement.
|
I don't have the bandwidth to bring this PR to completion. Can someone else work on this and ping me for a review so I can ensure it meets pre-sales needs? Addressing the specific feedback. It is VERY IMPORTANT in my view to scope things using the To address the |
I'll pick this up and ping you for review |
|
@stefanoboriero @justinegeffen Can we get this re-reviewed and merged? |
Signed-off-by: Justine Geffen <justinegeffen@users.noreply.github.com>
|
@stefanoboriero, if you have bandwidth to do another review of this PR that would be super helpful. :) |
|
Could we please get this merged? It's been open for almost three months. Thanks! |
|
@robnewman @kenibrewer I believe this is superseded by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#941 and #943 are updating the AWS Batch CE policy, this PR is updating the AWS Cloud CE policy
I agree with the comments from Stefano, many changes are just aestethic, they aren't limiting or improving the policy.
In #941 and #943 I added a copy-pasteable policy with wide-open permissions for AWS Batch CEs, and I then commented each section in it to reduce the scope of each section according to the needs of each customer. I can replicate it for AWS Cloud CEs too.
I think we should mirror these changes (with the caveat of the TOWER_FORGE_PREFIX env var) in the Enterprise docs too.
| ### Platform credentials | ||
|
|
||
| To create and launch pipelines or Studio sessions with this compute environment type, you must attach Seqera credentials for the cloud provider. Some permissions are mandatory for the compute environment to be created and function correctly; others are optional and used to pre-fill options in Platform. | ||
| To create and launch pipelines or Studio sessions with this compute environment type, you must attach Seqera credentials for the cloud provider. Some permissions are mandatory for the compute environment to be created and function correctly; others are optional and used to pre-fill options in Platform. If you use `TOWER_FORGE_PREFIX` you can customize the policy with find and replace. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| To create and launch pipelines or Studio sessions with this compute environment type, you must attach Seqera credentials for the cloud provider. Some permissions are mandatory for the compute environment to be created and function correctly; others are optional and used to pre-fill options in Platform. If you use `TOWER_FORGE_PREFIX` you can customize the policy with find and replace. | |
| To create and launch pipelines or Studio sessions with this compute environment type, you must attach Seqera credentials for the cloud provider. Some permissions are mandatory for the compute environment to be created and function correctly; others are optional and used to pre-fill options in Platform. |
Seqera Cloud users can't customize the forge prefix, Enterprise users can. On Cloud it's set to the default TowerForge-.
This comment should probably be moved to the enterprise version of the policy
| "arn:aws:iam::*:role/TowerForge*", | ||
| "arn:aws:iam::*:instance-profile/TowerForge*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "arn:aws:iam::*:role/TowerForge*", | |
| "arn:aws:iam::*:instance-profile/TowerForge*" | |
| "arn:aws:iam::*:role/TowerForge-*", | |
| "arn:aws:iam::*:instance-profile/TowerForge-*" |
| "arn:aws:iam::*:role/TowerForge*", | ||
| "arn:aws:iam::*:instance-profile/TowerForge*" | ||
| ] | ||
| }, | ||
| { | ||
| "Sid": "AwsCloudCreatePassRole", | ||
| "Effect": "Allow", | ||
| "Action": [ | ||
| "iam:PassRole" | ||
| ], | ||
| "Resource": "arn:aws:iam::*:role/TowerForge*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "arn:aws:iam::*:role/TowerForge*", | |
| "arn:aws:iam::*:instance-profile/TowerForge*" | |
| ] | |
| }, | |
| { | |
| "Sid": "AwsCloudCreatePassRole", | |
| "Effect": "Allow", | |
| "Action": [ | |
| "iam:PassRole" | |
| ], | |
| "Resource": "arn:aws:iam::*:role/TowerForge*" | |
| "arn:aws:iam::*:role/TowerForge-*", | |
| "arn:aws:iam::*:instance-profile/TowerForge-*" | |
| ] | |
| }, | |
| { | |
| "Sid": "AwsCloudCreatePassRole", | |
| "Effect": "Allow", | |
| "Action": [ | |
| "iam:PassRole" | |
| ], | |
| "Resource": "arn:aws:iam::*:role/TowerForge-*" |
Summary
TowerForge*prefix instead of wildcard resourcesTowerForge-*instance namingTest plan
🤖 Generated with Claude Code