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

pathogen-repo-build: Support assuming an AWS role via GitHub Actions' OIDC provider #53

Merged
merged 1 commit into from
Feb 5, 2024

Conversation

tsibley
Copy link
Member

@tsibley tsibley commented Sep 14, 2023

The default role has a pretty limited set of permissions right now: basically only those required to submit and interact with AWS Batch jobs, but not, for example, permissions to upload to our production S3 buckets. The permissions will probably need to be extended in the future as repos/builds opt into this workflow and this role assumption. The role's trust policy will also need to be extended to allow other repos to assume it.

While the role is currently manually managed, it should really be managed by a Terraform configuration (but one separate from the current nextstrain.org Terraform configurations).

Related-to: https://github.com/nextstrain/private/issues/22

Checklist

  • Checks pass

@tsibley tsibley marked this pull request as ready for review October 11, 2023 23:04
@tsibley tsibley requested a review from a team October 11, 2023 23:04
@joverlee521 joverlee521 self-requested a review October 12, 2023 20:16
Copy link
Contributor

@joverlee521 joverlee521 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes here look reasonable to me!

My main questions are about the permissions and management of the GitHubActionsRoleNextstrainBatchJobs role.

The default role has a pretty limited set of permissions right now: basically only those required to submit and interact with AWS Batch jobs, but not, for example, permissions to upload to our production S3 buckets.

Is the plan to manage upload permissions with another AWS Batch job role (as you mentioned previously). So the temp credentials here will allow the deployment and running of the Batch job, but permissions for AWS interactions within the build runtime will be managed by a separate role.

The role's trust policy will also need to be extended to allow other repos to assume it.

Is the plan to maintain a list of allowed repos in the role's trust policy and we would add repos to it as we add more pathogen workflows?

.github/workflows/pathogen-repo-build.yaml Show resolved Hide resolved
@tsibley
Copy link
Member Author

tsibley commented Oct 13, 2023

moved to https://github.com/nextstrain/private/issues/96

Is the plan to manage upload permissions with another AWS Batch job role (as you mentioned previously). So the temp credentials here will allow the deployment and running of the Batch job, but permissions for AWS interactions within the build runtime will be managed by a separate role.

Unclear right now. A few options I see:

  1. Extend the GitHubActionsRoleNextstrainBatchJobs role to allow uploading/downloading to S3 buckets used for pathogen builds (e.g. nextstrain-data, nextstrain-data-private, nextstrain-ncov-private, nextstrain-staging). Ensure the temporary credentials from role assumption are passed into the AWS Batch job via nextstrain build, as they are now.
  2. Create a new role (or several) that allows the permissions above and have this workflow arrange to pass those temporary credentials into the Batch job (e.g. via nextstrain build + using output-credentials: true to configure-aws-credentials). This is a minor privilege separation and probably not worth it.
  3. Update the default Batch job role (NextstrainJobsRole) to allow the permissions we need and stop passing credentials into the job via nextstrain build.
  4. Create a new role (or several) that can be assumed by the Batch job itself and arrange via nextstrain build to have it assumed as part of the job submission.

Is the plan to maintain a list of allowed repos in the role's trust policy and we would add repos to it as we add more pathogen workflows?

I'd probably allow all existing pathogen repos initially, even if they're not using role assumption right now, and then yeah, add more as we need. I'd think of this as "enabling" a repo to use our AWS Batch infra.

@tsibley
Copy link
Member Author

tsibley commented Oct 13, 2023

moved to https://github.com/nextstrain/private/issues/95

To choose amongst options, I'd think to consider:

  • Review permissions we currently grant via individual IAM users/policies for each pathogen that's automated
  • Decide if the union of those is palatable to grant widely to pathogen repos and any one with commit access to them
  • Decide if the union of those is palatable to grant to any one who can submit AWS Batch jobs in our group (review this)
  • Determine if the maximum session lifetime of 12 hours for an assumed role via GitHub OIDC is sufficient for all our pathogen builds or not

Option 1 is tempting because it's ~transparent to pathogen builds and easy. But it maybe runs into session lifetime issues if we're passing those assumed temporary credentials into the Batch job.

Options 3 and 4 are tempting because they're also ~transparent and easy and won't run into session lifetime issues. However, it means being ok allowing everyone in the lab who can submit jobs to make all the changes our automated builds need to do.

… OIDC provider

The default role has a pretty limited set of permissions right now:
basically only those required to submit and interact with AWS Batch
jobs, but not, for example, permissions to upload to our production S3
buckets.  The permissions will probably need to be extended in the
future as repos/builds opt into this workflow and this role assumption.
The role's trust policy will also need to be extended to allow other
repos to assume it.

While the role is currently manually managed, it should really be
managed by a Terraform configuration (but maybe one separate from the
current nextstrain.org Terraform configurations?).

Related-to: <nextstrain/private#22>
@tsibley tsibley force-pushed the trs/pathogen-repo-build/aws-assume-role branch from 4e26422 to fbff47b Compare January 22, 2024 22:25
@tsibley
Copy link
Member Author

tsibley commented Feb 3, 2024

This PR shouldn't have any functional change for our current usage, as I believe all callers provide explicit credentials themselves so the new role won't be assumed. As such, I'm inclined to merge it sooner than later to not leave it dangling. Further work to come will extend the role's permissions (or otherwise provide for additional permissions) and ensure they're passed in one way or another to the runtime.

@tsibley
Copy link
Member Author

tsibley commented Feb 5, 2024

While the role is currently manually managed, it should really be managed by a Terraform configuration (but one separate from the current nextstrain.org Terraform configurations).

Will be part of nextstrain/infra@main...dev.

@tsibley tsibley merged commit eaaa3c4 into master Feb 5, 2024
14 of 19 checks passed
@tsibley tsibley deleted the trs/pathogen-repo-build/aws-assume-role branch February 5, 2024 22:25
@tsibley tsibley mentioned this pull request Feb 5, 2024
1 task
tsibley added a commit to nextstrain/infra that referenced this pull request Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

3 participants