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

Fix wrong update of externalId for pivotRole #591

Merged
merged 11 commits into from
Jul 25, 2023

Conversation

dlpzx
Copy link
Contributor

@dlpzx dlpzx commented Jul 20, 2023

Feature or Bugfix

  • Bugfix

Detail

Fixes #589 by:

  • using CDK constructs to check the existence of an externalID in Secrets Manager
  • using boto3 calls using the CDK look up role in the deployment accounts to find an externalID in the Systems Manager Parameter Store

Manual operations needed ONLY if upgrading. Fresh deployments are unaffected

In the first run the CodePipeline will fail in the CDK Synth stage if no additional changes are done:

botocore.exceptions.ClientError: An error occurred (AccessDenied) when calling the AssumeRole operation: User: arn:aws:sts::111111111111:assumed-role/SOME ROLE/... is not authorized to perform: sts:AssumeRole on resource: arn:aws:iam::222222222222:role/cdk-hnb659fds-lookup-role-22222222222-eu-west-1

CodeBuild needs additional permissions to assume the IAM role in the CDK Synth stage. Since we cannot update this CodeBuild stage without running it, the permissions need to be added manually.

Upgrading from V1.6.0 to v1.6.1

The role that we need to update is a role named <PREFIX>-<GITBRANCH>-codebuild-baseline-role. It will say it in the error message in the CodeBuild logs

  1. Go to the IAM role (<PREFIX>-<GITBRANCH>-codebuild-baseline-role) and click on Add permissions > Create inline policy
image 2. Update the policy, use the JSON and copy the policy below: image

The policy of the Codebuild execution role need to include the following:

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Sid": "VisualEditor0",
            "Effect": "Allow",
            "Action": "sts:AssumeRole",
            "Resource": "arn:aws:iam::*:role/cdk-hnb659fds-lookup-role*"
        }
    ]
}

After the pipeline has successfully run, go back to the IAM role and remove the manually added policy. The policy is now added as part of infrastructure as code.
image

Upgrading from <V1.6.0 to v1.6.1

The error points at a different role some. A role created by CDK that looks like the following in the CodeBuild logs:

botocore.exceptions.ClientError: An error occurred (AccessDenied) when calling the AssumeRole operation: User: arn:aws:sts:::111111111111:assumed-role/dataall-sbx8-cicd-stack-dataallsbx8cdkpipelinePipe-HMXY7D9OX4FM/AWSCodeBuild-30c50765-4529-4d20-99ce-88f82139a82c is not authorized to perform: sts:AssumeRole on resource: arn:aws:iam::22222222222:role/cdk-hnb659fds-lookup-role-22222222222-eu-west-1

We find the role and update it as we explained in the "Upgrading from V1.6.0 to v1.6.1" section.
image

Once that is done, retry the CodeBuild Synth stage. In this case you do NOT need to cleanup the manually added policies as this role will be deleted.

Tests

[X] Tested by merging this branch into a deployment initially in V1.6.0 -> SSM parameter value unmodified
[X] Tested by merging this branch into a deployment initially in V1.5.6 -> Secret value copied to SSM parameter unmodified
[X] Tested by deploying this branch on a fresh account

Relates

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@dlpzx dlpzx requested a review from noah-paige July 20, 2023 12:59
@dlpzx dlpzx marked this pull request as ready for review July 20, 2023 13:33
@dlpzx
Copy link
Contributor Author

dlpzx commented Jul 21, 2023

We have 2 approaches, I have tried them both.

1. use boto3 calls to the infra accounts from the CICD pipeline

It is the current code in the PR. It assumes the CDK look-up-role in the infra accounts.

  • CONS: CodeBuild needs additional permissions to assume the IAM role. And as it happened with ec2:DescribePrefixList there is no way of updating the pipeline without that permission
  • PROS: It works well

2. use CDK look-up resources from the infra account while the stack is deployed

We tried several CDK constructs for this.

  • CONS: Either they did not work or they needed CDK synth with infra account credentials. --> which I am not sure how is going to work with a fresh deployment.
  • CONS: bad handling of SSM and Secrets together - exceptions in CDK are not as usable as in normal python
  • PROS: no additional permissions granted

In conclusion, because of how unstable the solution with CDK constructs is (I am specially worried about the user experience for fresh deployments) I am more inclined to use boto3. What do you think? @noah-paige

@noah-paige
Copy link
Contributor

I agree with opting for boto3 at this point since the CDK Construct methods seem to not work as we would like

The main issue I see with Option 1 is in my testing the default cdk look up role does not have the required permissions to get the secret either

@dlpzx
Copy link
Contributor Author

dlpzx commented Jul 21, 2023

I checked the permission on the role and it has "secretsmanager:Describe*", damm. Let me review that again

@noah-paige
Copy link
Contributor

Yeah but it has kms:Decrypt Deny and does not have secretsmanager:GetSecretValue

@noah-paige
Copy link
Contributor

What if we use boto3 for SSM and CDK Constructs to get secret... I am testing this approach and I think it works as expected

Will still have to add permissions to assume role in Synth step

@dlpzx dlpzx requested review from nikpodsh and dbalintx July 24, 2023 09:58
@dlpzx dlpzx requested a review from dbalintx July 25, 2023 06:13
@dlpzx dlpzx merged commit f3baf14 into main Jul 25, 2023
@dlpzx dlpzx deleted the bugfix/lookup-externalId-wrong-account branch August 11, 2023 12:24
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.

Replaced externalId in V1.6.0
3 participants