feat(aws-codepipeline): support for pipeline action’s service role #1449
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rsmogura
requested review from
RomainMuller,
skinny85 and
a team
as code owners
eladb
approved these changes
Dec 30, 2018
| * | ||
| * @see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-codepipeline-pipeline-stages-actions.html | ||
| */ | ||
| actionRole?: iam.IRole; |
There was a problem hiding this comment.
PipelineCloudFormationDeployActionProps uses role attribute to describe role assumed by Cloud Formation. The ref. arch. actually suggest usage of both roles for cross-account deployments.
If we would rename this attribute, one on CloudFormation should be renamed too, which would occur in a backward incompatible change.
There was a problem hiding this comment.
okay, let's leave this actionRole.
There was a problem hiding this comment.
I wonder whether it's actually worth it to change the attribute in PipelineCloudFormationDeployActionProps to something like cloudFormationRole, and name this new one role. The reason is that this one is much more generic than the CFN Actions one, so the name role there does not seem entirely appropriate.
@eladb what do you think?
There was a problem hiding this comment.
@skinny85 not a bad idea. Maybe something like deploymentRole? (it is a deploy action)
There was a problem hiding this comment.
deploymentRole sounds a little ambiguous to me, but sure. @rsmogura can you make this change? Thanks!
There was a problem hiding this comment.
I renamed CF role to deploymentRole and PipelineDeployStackAction in app-delivery, changed addToRolePolicy method names to addToDeploymentRolePolicy.
I do not update action's service role permissions when added to pipeline. I think as feature is for advanced use-cases role or artifacts can come from other stack(s).
I tested this change with simple pipeline and setting action's role on source action, requires granting s3 and codecommit actions by developer.
There was a problem hiding this comment.
@skinny85 - I don't think of deploymentRole as so ambiguous in that context... Thats the way at least I think of it there... And it maps nicely with the user intention... So that's good. As I mentioned in a separate note, deployRole might be another option (I have no strong feelings either way).
|
Thanks for the PR @rsmogura! The one thing I would change is to add this new |
rsmogura
force-pushed
the
action-role
branch
4 times, most recently
from
ffe88e7 to
3dd7836
Compare
|
I think it's a good idea; moved |
skinny85
suggested changes
Jan 3, 2019
rsmogura
force-pushed
the
action-role
branch
2 times, most recently
from
2c1229d to
2fc2239
Compare
skinny85
reviewed
Jan 7, 2019
|
@RomainMuller do you have any objections about renaming the |
rsmogura
force-pushed
the
action-role
branch
3 times, most recently
from
aab545d to
ebbcd65
Compare
|
@skinny85 - no major objection with the rename. I wonder if |
RomainMuller
approved these changes
Jan 8, 2019
|
I've found that project build action is using |
Can you link to the code? |
My bad - I took a look at |
skinny85
reviewed
Jan 10, 2019
skinny85
reviewed
Jan 10, 2019
packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-cfn.expected.json
Outdated
Show resolved
Hide resolved
rsmogura
force-pushed
the
action-role
branch
2 times, most recently
from
d92dc04 to
96b1628
Compare
rsmogura
pushed a commit
to rsmogura/aws-cdk
that referenced
this pull request
Jan 12, 2019
…orkflow) Use custom KMS keys, which are passed or generated by CDK in order to encrypt artifacts stored in S3. Customers would be able to pass own configuration of artifacts store - including S3 Bucker and KMS Encryption Key. Set of changes: * add KMS key & alias (with statically generated name) to scaffold stack; * reference scaffold KMS key in ArtifactStore resource; * scaffold stack artifact names derive name from pipeline name (or other identifier) to prevent collision with other projects. This change is mainly motivated to enable cross-account deployments. The work should enable to construct stacks like described in reference architecture https://github.com/awslabs/aws-refarch-cross-account-pipeline In such a case foreign role have to have access to stored artifacts in S3 buckets. However in order to achieve this two things must be achieved * foreign account (role) has to be whitelisted on artifacts' bucket policy; and * foreign account (role) has to be whitelisted on KMS keys used to encrypt artifacts in above bucket (which is impossible to do with AWS managed one). In order to enable above cross-region scaffold stacks will contain KMS keys. As KMS key can't be directly reference in cross-region / cross-account the alias with known name is generated, in similar way as it happens for S3 buckets. Together with aws#1449 customers should be able to create on their own roles, and import those in pipeline stack. When Some parts of this workflow could be automated, however even without it users would be enabled to create cross-account pipelines. * Create stack for deployments in foreign account * create *jump* role which would be set on action (aws#1449) this role would mainly allow passing Cloud Formation deployment role; * grant *jump* role read / write (as required) permissions to S3 buckets and KMS keys (right now whole foreign account is whitelisted) * create deployment role; * In pipeline account * import both roles (require role ARNs to be known) * set imported *jump* role on Cloud Formation actions; * set imported deployment role as Cloud Formation role
rsmogura
pushed a commit
to rsmogura/aws-cdk
that referenced
this pull request
Jan 12, 2019
…orkflow) Use custom KMS keys, which are passed or generated by CDK in order to encrypt artifacts stored in S3. Customers would be able to pass own configuration of artifacts store - including S3 Bucker and KMS Encryption Key. Set of changes: * add KMS key & alias (with statically generated name) to scaffold stack; * reference scaffold KMS key in ArtifactStore resource; * scaffold stack artifact names derive name from pipeline name (or other identifier) to prevent collision with other projects. This change is mainly motivated to enable cross-account deployments. The work should enable to construct stacks like described in reference architecture https://github.com/awslabs/aws-refarch-cross-account-pipeline In such a case foreign role have to have access to stored artifacts in S3 buckets. However in order to achieve this two things must be achieved * foreign account (role) has to be whitelisted on artifacts' bucket policy; and * foreign account (role) has to be whitelisted on KMS keys used to encrypt artifacts in above bucket (which is impossible to do with AWS managed one). In order to enable above cross-region scaffold stacks will contain KMS keys. As KMS key can't be directly reference in cross-region / cross-account the alias with known name is generated, in similar way as it happens for S3 buckets. Together with aws#1449 customers should be able to create on their own roles, and import those in pipeline stack. When Some parts of this workflow could be automated, however even without it users would be enabled to create cross-account pipelines. * Create stack for deployments in foreign account * create *jump* role which would be set on action (aws#1449) this role would mainly allow passing Cloud Formation deployment role; * grant *jump* role read / write (as required) permissions to S3 buckets and KMS keys (right now whole foreign account is whitelisted) * create deployment role; * In pipeline account * import both roles (require role ARNs to be known) * set imported *jump* role on Cloud Formation actions; * set imported deployment role as Cloud Formation role
rsmogura
pushed a commit
to rsmogura/aws-cdk
that referenced
this pull request
Jan 14, 2019
…orkflow) Use custom KMS keys, which are passed or generated by CDK in order to encrypt artifacts stored in S3. Customers would be able to pass own configuration of artifacts store - including S3 Bucker and KMS Encryption Key. Set of changes: * add KMS key & alias (with statically generated name) to scaffold stack; * reference scaffold KMS key in ArtifactStore resource; * scaffold stack artifact names derive name from pipeline name (or other identifier) to prevent collision with other projects. This change is mainly motivated to enable cross-account deployments. The work should enable to construct stacks like described in reference architecture https://github.com/awslabs/aws-refarch-cross-account-pipeline In such a case foreign role have to have access to stored artifacts in S3 buckets. However in order to achieve this two things must be achieved * foreign account (role) has to be whitelisted on artifacts' bucket policy; and * foreign account (role) has to be whitelisted on KMS keys used to encrypt artifacts in above bucket (which is impossible to do with AWS managed one). In order to enable above cross-region scaffold stacks will contain KMS keys. As KMS key can't be directly reference in cross-region / cross-account the alias with known name is generated, in similar way as it happens for S3 buckets. Together with aws#1449 customers should be able to create on their own roles, and import those in pipeline stack. When Some parts of this workflow could be automated, however even without it users would be enabled to create cross-account pipelines. * Create stack for deployments in foreign account * create *jump* role which would be set on action (aws#1449) this role would mainly allow passing Cloud Formation deployment role; * grant *jump* role read / write (as required) permissions to S3 buckets and KMS keys (right now whole foreign account is whitelisted) * create deployment role; * In pipeline account * import both roles (require role ARNs to be known) * set imported *jump* role on Cloud Formation actions; * set imported deployment role as Cloud Formation role
rsmogura
pushed a commit
to rsmogura/aws-cdk
that referenced
this pull request
Jan 14, 2019
Support for cross-account deployments, and cross-account artifacts replication. The work should enable to construct stacks simillar to one described in reference architecture https://github.com/awslabs/aws-refarch-cross-account-pipeline In such a case foreign role have to have access to stored artifacts in S3 buckets. However in order to achieve this two things must be achieved * foreign account (role) has to be whitelisted on artifacts' bucket policy; and * foreign account (role) has to be whitelisted on KMS keys used to encrypt artifacts in above bucket (which is impossible to do with AWS managed one). Because names of buckets and KMS key aliases have to be known upfront to pass those in cross region fashion between underlying Cloud Formation templates, those are derived from pipeline name (similarly as it's right for scaffold buckets). As KMS keys doesn't have name the alias to key is used and it's passed as configuration to pipeline artifact store, so replicated artifacts will be encrypted with those keys. Pipeline tries to whitelist foreign accounts on artifacts stores (buckets and KMS keys), by adding foreign account root principal to resource policy. Add account attribute to action, and treat it in similar way as it's right now for region. Introduced new constructs `ArtifactsStore` and `ImportedArtifactsStore` to encapsulate artifact store (S3 bucket and KMS key), and simplify managing of stores. When action is attached to pipeline, check if it's from foreign account, check if artifact store exists for this region and account. If it doesn't than create new scaffold stack. Each artifact store created by pipeline is considered pipeline managed. If artifact store is pipeline managed whitelist foreign account (add appropriate resource policy to S3 bucket and KMS key). When granting permissions to pipeline's artifact bucket (`grantBucketRead/Write`), store those n list and grant when pipeline is rendered. It's driven by two factors: * actions may ask (i.e. project build one) for permission before all stores has been created; * in case of cross region / account calls, such calls have to be performed only on imported store to add grants in IAM principal policy, and not in resource policy (scaffold stacks are deployed before actual role can be deployed) Changes to S3 allowing grant to `ArnPrincipal` (would updated only resource policy) Together with aws#1449 customers should be able to create on their own roles, and import those in pipeline stack. When Some parts of this workflow could be automated, however even without it users would be enabled to create cross-account pipelines. * Create stack for deployments in foreign account * create *jump* role which would be set on action (aws#1449) this role would mainly allow passing Cloud Formation deployment role; * grant *jump* role read / write (as required) permissions to S3 buckets and KMS keys (right now whole foreign account is whitelisted) * create deployment role; * In pipeline account * import both roles (require role ARNs to be known) * set imported *jump* role on Cloud Formation actions; * set imported deployment role as Cloud Formation role * user may be required to get encryption key for pipeline's artifact store and pass it to Project Build action, as Project Build by default encrypts output with `aws/s3` key (even if default key is set on bucket) In many places `kms:*` actions are allowed (those should be narrowed to only those required for encryption, decryption and alias dereference). Eventually, use tags and conditional statements to widen access to KMS keys; Right now all KMS keys are granted for principals (in order to allow principal access to particular key it's UUID has to be known, alias can't be used in-place)
rsmogura
pushed a commit
to rsmogura/aws-cdk
that referenced
this pull request
Jan 14, 2019
Support for cross-account deployments, and cross-account artifacts replication. The work should enable to construct stacks simillar to one described in reference architecture https://github.com/awslabs/aws-refarch-cross-account-pipeline In such a case foreign role have to have access to stored artifacts in S3 buckets. However in order to achieve this two things must be achieved * foreign account (role) has to be whitelisted on artifacts' bucket policy; and * foreign account (role) has to be whitelisted on KMS keys used to encrypt artifacts in above bucket (which is impossible to do with AWS managed one). Because names of buckets and KMS key aliases have to be known upfront to pass those in cross region fashion between underlying Cloud Formation templates, those are derived from pipeline name (similarly as it's right for scaffold buckets). As KMS keys doesn't have name the alias to key is used and it's passed as configuration to pipeline artifact store, so replicated artifacts will be encrypted with those keys. Pipeline tries to whitelist foreign accounts on artifacts stores (buckets and KMS keys), by adding foreign account root principal to resource policy. Add account attribute to action, and treat it in similar way as it's right now for region. Introduced new constructs `ArtifactsStore` and `ImportedArtifactsStore` to encapsulate artifact store (S3 bucket and KMS key), and simplify managing of stores. When action is attached to pipeline, check if it's from foreign account, check if artifact store exists for this region and account. If it doesn't than create new scaffold stack. Each artifact store created by pipeline is considered pipeline managed. If artifact store is pipeline managed whitelist foreign account (add appropriate resource policy to S3 bucket and KMS key). When granting permissions to pipeline's artifact bucket (`grantBucketRead/Write`), store those n list and grant when pipeline is rendered. It's driven by two factors: * actions may ask (i.e. project build one) for permission before all stores has been created; * in case of cross region / account calls, such calls have to be performed only on imported store to add grants in IAM principal policy, and not in resource policy (scaffold stacks are deployed before actual role can be deployed) Changes to S3 allowing grant to `ArnPrincipal` (would updated only resource policy) Together with aws#1449 customers should be able to create on their own roles, and import those in pipeline stack. When Some parts of this workflow could be automated, however even without it users would be enabled to create cross-account pipelines. * Create stack for deployments in foreign account * create *jump* role which would be set on action (aws#1449) this role would mainly allow passing Cloud Formation deployment role; * grant *jump* role read / write (as required) permissions to S3 buckets and KMS keys (right now whole foreign account is whitelisted) * create deployment role; * In pipeline account * import both roles (require role ARNs to be known) * set imported *jump* role on Cloud Formation actions; * set imported deployment role as Cloud Formation role * user may be required to get encryption key for pipeline's artifact store and pass it to Project Build action, as Project Build by default encrypts output with `aws/s3` key (even if default key is set on bucket) In many places `kms:*` actions are allowed (those should be narrowed to only those required for encryption, decryption and alias dereference). Eventually, use tags and conditional statements to widen access to KMS keys; Right now all KMS keys are granted for principals (in order to allow principal access to particular key it's UUID has to be known, alias can't be used in-place)
|
What do we do with this PR? @skinny85 ? |
|
@rix0rrr yep, I'm on this one, no worries |
skinny85
approved these changes
Jan 22, 2019
packages/@aws-cdk/aws-codepipeline/test/test.cloudformation-pipeline-actions.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-codepipeline/test/test.cloudformation-pipeline-actions.ts
Outdated
Show resolved
Hide resolved
|
I will try to sit on it today or tomorrow, unfortunately today I had a lot of project work. |
added 7 commits
January 22, 2019 21:59
In realation to aws#49 The action’s service roles is a role which will be assumed by pipeline during execution of this action. The pipeline action’s service role can be used to perform more advanced configuration, when i.e. elevation of permissions is required, or when fine grained access control may be required. https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-codepipeline-pipeline-stages-actions.html This commit is motivated by enabling cross-account deployments, for which service role will be used as jump role to assume one used by Cloud Formation in target account.
…le attribute Renamed * `PipelineCloudFormationDeployActionProps.role`; * `PipelineCloudFormationDeployAction.role`; * `PipelineDeployStackActionProps.role` to `deploymentRole` as `role` could cause a lot of ambiguity with more generic action’s roles. BREAKING CHANGE: `PipelineCloudFormationDeployActionProps.role`, `PipelineCloudFormationDeployAction.role`, `PipelineDeployStackActionProps.role` renamed to `deploymentRole`
Renames previously introduced actionRole to role, as it’s more generic name. This commit has been extracted, as during development the existing `role` attribute present on some classes has been renamed to `deploymentRole`. Splitting change to two separate commits helped coding bugs related to name clash. BREAKING CHANGE: together with previous commits `role` has been refactored to `deploymentRole`
* use spread operator for lambda invoke action; * revert `integ.pipeline-cfn.expected.json`
RIght now we want this role to be present only on Cloud Formation actions.
* reverted changes to `aws-codecommit/test/test.codecommit.ts`; * removed `aws-codedeploy/test/test.pipeline-action.ts`; * removed `runOrder` from `test/test.cloudformation-pipeline-actions.ts`; * removed comma after `"RunOrder": 8`; * fix idents in `test/test.cloudformation-pipeline-actions.ts`.
|
Thanks @rsmogura! Merged. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The action’s service roles is a role which will be assumed
by pipeline during execution of this action.
The pipeline action’s service role can be used to perform more
advanced configuration, when i.e. elevation of permissions
is required, or when fine grained access control may be required.
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-codepipeline-pipeline-stages-actions.html
This commit is motivated by enabling cross-account deployments,
for which service role’s will be used as jump role to assume
one used by Cloud Formation in target account.
BREAKING CHANGE:
PipelineCloudFormationDeployActionProps.role,PipelineCloudFormationDeployAction.role,PipelineDeployStackActionProps.rolerenamed todeploymentRolePull Request Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.