Skip to content

Conversation

@brucehoff
Copy link
Contributor

@brucehoff brucehoff commented Dec 2, 2025

Allow code pipeline roles to access synapse dev KMS key;
Allow Stack-Builder workflow to use AS OIDC integration

@brucehoff brucehoff requested a review from a team as a code owner December 2, 2025 01:37
@brucehoff brucehoff requested a review from xschildw December 2, 2025 01:38
@brucehoff brucehoff changed the title PLFM-9253: Allow code pipeline roles to access synapse dev KMS key PLFM-9253: Changes to support using Code Pipeline with Synapse Dec 2, 2025
{ "Fn::Sub": "arn:aws:iam::${AWS::AccountId}:role/sagebase-github-oidc-sage-bionetworks-it" },
{ "Fn::Sub": "arn:aws:iam::${AWS::AccountId}:role/aws-reserved/sso.amazonaws.com/AWSReservedSSO_Administrator_693a85eb20cd5043" }
]
"AWS": "*"
Copy link
Member

Choose a reason for hiding this comment

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

Why is this being changed to *? Is there something preventing us from maintaining least privilege?

Copy link
Contributor Author

@brucehoff brucehoff Dec 2, 2025

Choose a reason for hiding this comment

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

Good question: In this policy the 'allow' and 'deny' sections have the same list of IAM entities, creating unnecessary duplication. By simplifying the 'allow' to '*' and leaving the 'deny' in place, we get the same level of 'least privilege' while simplifying the policy document. This begs the question, "why not keep the 'allow' and eliminate the 'deny' part of the policy?" The reason is because the 'deny' section allows the use of wildcards and we need to use the pattern, Synapse-Build-*-CodeBuildServiceRole.

}
},
{
"Sid": "Allow root administration of the key",
Copy link
Member

Choose a reason for hiding this comment

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

Nit: This statement is more expansive than just root administration, can you update the description?

{ "Fn::Sub": "arn:aws:iam::${AWS::AccountId}:role/aws-reserved/sso.amazonaws.com/AWSReservedSSO_Administrator_693a85eb20cd5043" }
{ "Fn::Sub": "arn:aws:iam::${AWS::AccountId}:role/aws-reserved/sso.amazonaws.com/AWSReservedSSO_Administrator_693a85eb20cd5043" },
{ "Fn::Sub": "arn:aws:iam::${AWS::AccountId}:role/Synapse-Build-*-CodeBuildServiceRole" },
{ "Fn::Sub": "arn:aws:iam::${AWS::AccountId}:role/Deployment-Pipeline-CodeBuildServiceRole" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Really needed?

@brucehoff brucehoff merged commit 61815c0 into Sage-Bionetworks-IT:master Dec 2, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants