Copilot commented on code in PR #16265:
URL: https://github.com/apache/grails-core/pull/16265#discussion_r3885164324
##########
grails-forge/infrastructure/shared.yaml:
##########
@@ -246,10 +250,24 @@ Resources:
- Action:
- s3:GetObject
- s3:GetObjectVersion
+ - s3:GetObjectAcl
- s3:PutObject
+ - s3:PutObjectAcl
+ - s3:DeleteObject
Review Comment:
The GitHub deploy role now has `s3:*Acl` and `s3:DeleteObject` on the
artifact/Elastic Beanstalk buckets, but the deploy workflow only performs `aws
s3 cp` uploads (no ACL changes or deletions). If not strictly required,
removing these permissions reduces the impact of a compromised deploy token.
##########
grails-forge/infrastructure/shared.yaml:
##########
@@ -225,6 +225,10 @@ Resources:
GitHubDeployRole:
Type: AWS::IAM::Role
Properties:
+ ManagedPolicyArns:
+ - Fn::Sub:
arn:${AWS::Partition}:iam::aws:policy/AdministratorAccess-AWSElasticBeanstalk
+ - Fn::Sub:
arn:${AWS::Partition}:iam::aws:policy/AWSElasticBeanstalkWebTier
+ - Fn::Sub:
arn:${AWS::Partition}:iam::aws:policy/AWSElasticBeanstalkManagedUpdatesCustomerRolePolicy
Review Comment:
Attaching the AWS managed policy `AdministratorAccess-AWSElasticBeanstalk`
to the GitHub OIDC deploy role grants very broad permissions and increases
blast radius if the role is ever abused. Prefer least-privilege by removing
this managed policy and granting only the specific actions needed for the
workflow via the inline policy.
##########
.github/workflows/forge-deploy-aws.yml:
##########
@@ -163,9 +163,18 @@ jobs:
wait_for_ready_green() {
local operation="$1"
local expected_version="$2"
- local status health current_version attempt
+ local status health current_version attempt event
+ local start_iso
+ start_iso="$(date -u +%Y-%m-%dT%H:%M:%SZ)"
Review Comment:
`wait_for_ready_green` sets `start_iso` when the wait begins, so Elastic
Beanstalk ERROR events emitted immediately after `update-environment` starts
could be missed, undermining the "fail fast on EB errors" behavior. Allow the
start time to be injected (e.g., via `START_ISO`) so it can be set just before
triggering the deployment.
This issue also appears on line 202 of the same file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
