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.

Sign up for GitHub

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

Jump to bottom

Add CI OIDC access to new bors branches #355

Merged
merged 4 commits into from
Mar 9, 2024
Merged

Add CI OIDC access to new bors branches #355

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1fd1db9
Bring rustc-ci in sync with latest terraform
Mark-Simulacrum Oct 22, 2023
2160c87
Add access for new bors to PUT into artifacts bucket
Mark-Simulacrum Oct 22, 2023
2f7aefc
Limit write access to specific prefix
Mark-Simulacrum Nov 19, 2023
ac858b7
Revert "Limit write access to specific prefix"
Mark-Simulacrum Mar 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 34 additions & 29 deletions terraform/rustc-ci/.terraform.lock.hcl
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion terraform/rustc-ci/_terraform.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ terraform {
required_providers {
aws = {
source = "hashicorp/aws"
version = "~> 3.59"
version = "~> 4.67"
}
}

Expand Down
12 changes: 8 additions & 4 deletions terraform/rustc-ci/environments.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@ module "public" {
aws.east1 = aws.east1
}

iam_prefix = "ci--rust-lang--rust"
repo = "rust-lang-ci/rust"
iam_prefix = "ci--rust-lang--rust"
repo = "rust-lang-ci/rust"
source_repo = "rust-lang/rust"

caches_bucket = "rust-lang-ci-sccache2"
caches_domain = "ci-caches.rust-lang.org"
Expand All @@ -17,6 +18,7 @@ module "public" {

delete_caches_after_days = 90
delete_artifacts_after_days = 168
response_policy_id = data.terraform_remote_state.shared.outputs.mdbook_response_policy
}

module "security" {
Expand All @@ -26,12 +28,14 @@ module "security" {
aws.east1 = aws.east1
}

iam_prefix = "ci--rust-lang-ci--rsec"
repo = "rust-lang-ci/rsec"
iam_prefix = "ci--rust-lang-ci--rsec"
repo = "rust-lang-ci/rsec"
source_repo = "rust-lang-ci/rsec"

caches_bucket = "rust-lang-security-ci-caches"
artifacts_bucket = "rust-lang-security-ci-artifacts"

delete_caches_after_days = 30
delete_artifacts_after_days = 90
response_policy_id = data.terraform_remote_state.shared.outputs.mdbook_response_policy
}
12 changes: 11 additions & 1 deletion terraform/rustc-ci/impl/_terraform.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ terraform {
required_providers {
aws = {
source = "hashicorp/aws"
version = "~> 3.59"
version = "~> 4.67"
configuration_aliases = [aws.east1]
}
}
Expand Down Expand Up @@ -55,3 +55,13 @@ variable "repo" {
description = "GitHub repository to authorize"
type = string
}

variable "source_repo" {
description = "GitHub repository to authorize for roles"
type = string
}

variable "response_policy_id" {
description = "CDN response policy"
type = string
}
65 changes: 65 additions & 0 deletions terraform/rustc-ci/impl/artifacts.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ module "artifacts_cdn" {

domain_name = var.artifacts_domain
origin_domain_name = aws_s3_bucket.artifacts.bucket_regional_domain_name
response_policy_id = var.response_policy_id
}

data "aws_s3_bucket" "inventories" {
Expand All @@ -148,3 +149,67 @@ resource "aws_s3_bucket_inventory" "artifacts" {
}
}
}

resource "aws_iam_role" "try_builds" {
name = "${var.iam_prefix}--try-role"

assume_role_policy = jsonencode({
Version = "2012-10-17"
Statement = [
{
Effect = "Allow"
Action = "sts:AssumeRoleWithWebIdentity"
Principal = {
Federated = "arn:aws:iam::890664054962:oidc-provider/token.actions.githubusercontent.com"
Mark-Simulacrum marked this conversation as resolved.
Show resolved Hide resolved
}
Condition = {
StringEquals = {
"token.actions.githubusercontent.com:sub" = "repo:${var.repo}:ref:refs/heads/automation/bors/try"
}
}
}
]
})

inline_policy {
name = "put-objects"
policy = jsonencode({
Version = "2012-10-17"
Statement = [
{
Sid = "ArtifactsBucketWrite"
Effect = "Allow"
Resource = [
"${aws_s3_bucket.artifacts.arn}/rustc-builds-try",
"${aws_s3_bucket.artifacts.arn}/rustc-builds-try/*",
"${aws_s3_bucket.artifacts.arn}/rustc-builds-try-alt",
"${aws_s3_bucket.artifacts.arn}/rustc-builds-try-alt/*",
Comment on lines +183 to +186
Copy link
Member

Choose a reason for hiding this comment

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

This is an improvement over the current situation, but it'd be nice to scope these by the commit hash (${aws_s3_bucket.artifacts.arn}/rustc-builds-try-alt/${sha}/*). As far as I'm aware it might not be possible to do that in IAM directly (the commit hash is in the GitHub Actions OIDC claim, but it might not be possible to access it from IAM), but at that point I'd prefer having a lambda function validating the JWT and issuing a properly scoped policy.

Also, the *-less variant shouldn't be needed.

Copy link
Member

@pietroalbini pietroalbini Oct 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

For more context on why this would be helpful: right now a job can override artifacts from other jobs, so in theory a try build (triggered by a trusted reviewer) could override the artifacts of a previously merged commit / try build, while if we scope things by commit hash a build would only be able to write in their scratch space.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I looked into this a bit and it seems like the only way to do this is custom code, we can expose the SHA into the sub claim with a custom claim but I don't think we can parse it out from there.

The Lambda function sounds plausible, we do somehow similar for sync-team kickoff... it would mean custom code in this relatively sensitive area though. I guess the risk would mostly be that we just don't get it's benefits though - should be easy to avoid further escalation.

Copy link
Member Author

Choose a reason for hiding this comment

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

My inclination I think is to not block on the further scope reduction at this time, I'm happy to work on it but I want to get new bors treating unblocked quickly.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I looked into this a bit and it seems like the only way to do this is custom code, we can expose the SHA into the sub claim with a custom claim but I don't think we can parse it out from there.

Yeah, I remember a while back reading somewhere that it was possible to pass arbitrary custom claims into IAM policies if they had specific keys in the JWT token, but I can't seem to find that piece of docs anymore. It'd need work on the GitHub side to change those claims anyway, so it wouldn't be something we could rely on in a timely manner.

The Lambda function sounds plausible, we do somehow similar for sync-team kickoff... it would mean custom code in this relatively sensitive area though. I guess the risk would mostly be that we just don't get it's benefits though - should be easy to avoid further escalation.

Indeed. A way to limit the damage would be to have the function call be authenticated with OIDC too, so the most damage would be a job overriding another job's artifacts (i.e. the status quo) compared to third parties obtaining tokens, but still, it'd be sensitive code.

My inclination I think is to not block on the further scope reduction at this time, I'm happy to work on it but I want to get new bors treating unblocked quickly.

I think it's fine to land this now to unblock the new bors (it uploads to a different directory than current bors anyway, so no risk of overriding), but it'd be nice to have it in place in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I remember a while back reading somewhere that it was possible to pass arbitrary custom claims into IAM policies if they had specific keys in the JWT token, but I can't seem to find that piece of docs anymore. It'd need work on the GitHub side to change those claims anyway, so it wouldn't be something we could rely on in a timely manner.

Yeah, I'm not quite sure if the GitHub side has exactly the claim we'd need. https://token.actions.githubusercontent.com/.well-known/openid-configuration has a listing but I don't know if the head_ref for example is useful or just the branch name.

https://docs.github.com/en/enterprise-cloud@latest/actions/deployment/security-hardening-your-deployments/about-security-hardening-with-openid-connect#understanding-the-oidc-token lists a "sha": "example-sha" but it's not documented and not clear whether example-sha would be a full commit hash or what.

Copy link
Member

@pietroalbini pietroalbini Oct 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think workflow_sha would be the claim we'd be interested in, as it'd be the hash of the commit containing the workflow we're executing.

Also, I found the AWS feature I was thinking of: Session Tags. If the proper tag was passed in the JWT, then we could have ${iam:ResourceTag/workflow_sha} in the IAM policy I think. But that's not applicable for us because GHA doesn't put the claims under the https://aws.amazon.com/tags key.

Edit: the sha in the docs might be legacy, before they split workflow_sha and job_workflow_sha due to the introduction of reusable workflows.

Copy link
Member

Choose a reason for hiding this comment

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

Actually @Mark-Simulacrum found this out looking at that issue https://awsteele.com/blog/2023/10/25/aws-role-session-tags-for-github-actions.html.

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting! I did see some references to Cognito, we should try to figure out how complicated/pricey it'll be.

]
Action = [
"s3:GetObject",
"s3:DeleteObject",
"s3:PutObject",
"s3:PutObjectAcl",
]
},
{
Sid = "ArtifactsBucketList"
Effect = "Allow"
Resource = "${aws_s3_bucket.artifacts.arn}"
Action = [
"s3:ListBucket",
],
},
{
Sid = "HeadBuckets",
Effect = "Allow",
Resource = "*"
Action = [
"s3:HeadBucket",
"s3:GetBucketLocation",
],
},
]
})
}
}
1 change: 1 addition & 0 deletions terraform/rustc-ci/impl/caches.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,4 +116,5 @@ module "caches_cdn" {

domain_name = var.caches_domain
origin_domain_name = aws_s3_bucket.caches.bucket_regional_domain_name
response_policy_id = var.response_policy_id
}
Loading