aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.61k stars 3.91k forks source link

aws-secretsmanager: Malformed IAM Policy Resource for CodeBuildAction referencing secret name #15875

Open BrettHoutz opened 3 years ago

BrettHoutz commented 3 years ago

In the Python app below a Secret is created and then referenced in the environment configuration for a CodeBuildAction. Trying to deploy this results in a MalformedPolicyDocument error. Sure enough, this is output in cdk.out/Build.template.json in the resource BuildProjectRoleDefaultPolicy:

{
  "Action": "secretsmanager:GetSecretValue",
  "Effect": "Allow",
  "Resource": "${Token[Fn"
}

It appears something went wrong in referencing the secret name. When i switch to the secret ARN instead, though, it works fine. According to the documentation, either the secret name or ARN should work.

Reproduction Steps

Run cdk synth for this Python app:

#!/usr/bin/env python3
from aws_cdk import \
    aws_codebuild as cb, \
    aws_secretsmanager as secrets, \
    aws_codepipeline_actions as actions, \
    aws_codepipeline as cp, \
    core as cdk

class MyPipelineStack(cdk.Stack):

    def __init__(self, scope: cdk.Construct, construct_id: str, **kwargs) -> None:
        super().__init__(scope, construct_id, **kwargs)

        pipeline = cp.Pipeline(self, 'MyPipeline')

        source_stage = pipeline.add_stage(stage_name='Source')
        build_stage = pipeline.add_stage(stage_name='Build')

        source_artifact = cp.Artifact(f'SourceArtifact')
        source_action = actions.CodeStarConnectionsSourceAction(
            action_name='SourceGitHub',
            connection_arn='arn:aws:codestar-connections:us-east-1:0000000000:connection/00000000-0000-0000-0000-000000000000',
            output=source_artifact,
            owner='BrettHoutz',
            repo='myrepo',
        )
        source_stage.add_action(source_action)

        access_token_secret = secrets.Secret(self, 'AccessToken')
        project = cb.PipelineProject(self, 'BuildProject')
        build_action = actions.CodeBuildAction(
            action_name='Build',
            input=source_artifact,
            project=project,
            environment_variables={
                'ACCESS_TOKEN': {'type': cb.BuildEnvironmentVariableType.SECRETS_MANAGER, 'value': access_token_secret.secret_name},
            }
        )

        build_stage.add_action(build_action)

app = cdk.App()
MyPipelineStack(app, 'Build')
app.synth()

What did you expect to happen?

Synthesize a stack with a valid policy

What actually happened?

Policy has an invalid Resource field

Environment


This is :bug: Bug Report

rix0rrr commented 3 years ago

Looks like the CodeBuildAction is doing string processing on the string token, thereby messing it up.

njlynch commented 3 years ago

Hmm..

I tried to reproduce this and was unable to. The result I got was as expected:

            {
              "Action": "secretsmanager:GetSecretValue",
              "Effect": "Allow",
              "Resource": {
                "Fn::Select": [
                  6,
                  {
                    "Fn::Split": [
                      ":",
                      {
                        "Ref": "SecretA720EF05"
                      }
                    ]
                  }
                ]
              }
            },

This was with a minimal example; I'll try using the above repro as well and see if there's something there throwing it off.

(Edit -- Yup, reproing with the above example. Hmm....)

njlynch commented 3 years ago

It appears with "@aws-cdk/aws-secretsmanager:parseOwnedSecretName": false, in cdk.json, things work (mostly) as expected. With the flag set to true (the new default, and the better behavior overall), we get this error.

It looks like this was flagged during the PR, but ultimately dismissed by both parties as a corner case (see https://github.com/aws/aws-cdk/pull/13706#discussion_r602258670).

The issue is with these lines here:

https://github.com/aws/aws-cdk/blob/04b8d400b2653aff4f48709e8b420c6adb996ef5/packages/@aws-cdk/aws-codebuild/lib/project.ts#L912-L915

Unfortunately, the Token in this case of this repro is something like ${Token[Fn::Join.153]}. This gets split on the : and turned into something useless. It appears this logic needs to be made more robust to detect when the entirety of the input is a token and not split in that case (or detect once split if a token has been broken). sigh

skinny85 commented 3 years ago

Unfortunately, the Token in this case of this repro is something like ${Token[Fn::Join.153]}. This gets split on the : and turned into something useless. It appears this logic needs to be made more robust to detect when the entirety of the input is a token and not split in that case (or detect once split if a token has been broken). sigh

I think the problem is that the Token should not include : in its serialized form. Our ARN parsing logic here actually assumes this is the case (note the Tokens won't contain ":", so this won't break them comment). This is the thing that we need to change.

skinny85 commented 3 years ago

It appears with "@aws-cdk/aws-secretsmanager:parseOwnedSecretName": false, in cdk.json, things work (mostly) as expected.

I don't think that's really true - with this flag set to false, we get the following template:

  BuildProjectRoleDefaultPolicy3E9F248C:
    Type: AWS::IAM::Policy
    Properties:
      PolicyDocument:
        Statement:
          - Action: secretsmanager:GetSecretValue
            Effect: Allow
            Resource:
              Fn::Select:
                - 6
                - Fn::Split:
                    - ":"
                    - Ref: AccessToken4AAD9E79

Which is incorrect (it passes the name of the Secret as the Resource of the Policy, while it should be the ARN).

This is because of these lines, in which we assume a Token means we got passed the ARN, not the name, because CodeBuild does not support providing the full name (meaning, with the additional generated random hashes prepended to it) of a Secret, which is what this Fn::Select - Fn::Split expression returns.

So, in summary @BrettHoutz, for a new Secret, going with the ARN is the way to go - when trying to use the Secret's name, even if the IAM Policy was not malformed, you would get an error from CodeBuild.

We should probably update the CodeBuild docs to call out this case explicitly.

skinny85 commented 3 years ago

Hmm. I guess, when "@aws-cdk/aws-secretsmanager:parseOwnedSecretName" is true, the correct (short) name is actually passed to the Project:

  BuildProject097C5DB7:
    Type: AWS::CodeBuild::Project
    Properties:
      Environment:
        EnvironmentVariables:
          - Name: ACCESS_TOKEN
            Type: SECRETS_MANAGER
            Value:
              Fn::Join:
                - "-"
                - - Fn::Select:
                      - 0
                      - Fn::Split:
                          - "-"
                          - Fn::Select:
                              - 6
                              - Fn::Split:
                                  - ":"
                                  - Ref: AccessToken4AAD9E79
                  - Fn::Select:
                      - 1
                      - Fn::Split:
                          - "-"
                          - Fn::Select:
                              - 6
                              - Fn::Split:
                                  - ":"
                                  - Ref: AccessToken4AAD9E79

However, we still can't know that that Token passed to environmentVariables represents a name and not an ARN in the Project code, so there's no way for us to format the ARN for the IAM Policy.

aka47 commented 2 years ago

We also run into this bug.

A workaround that works for us is to use the secretArn instead of the secretName. So in the example instead of access_token_secret.secret_name use access_token_secret.secret_arn ..

ggallotti commented 2 years ago

Same problem here

'DB_PASSWORD': {
                value: secret!.secretValueFromJson('password'),
                type: BuildEnvironmentVariableType.SECRETS_MANAGER
 }

Resource 5GhEKEYIk8^8ICiU-riNs05xW.hSEz must be in ARN format or "*". (Service: AmazonIdentityManagement; Status Code: 400; Error Code: MalformedPolicyDocument; Request ID: f9c08434-8af0-4288-bae9-2883b9b47567; Proxy: null)

mbonig commented 2 years ago

The issue here is that you shouldn't be using .secretValueFromJson in your setup. This will cause CloudFormation to retrieve the value and render it in the configuration, bypassing the benefits of Secrets Manager.

@ggallotti , try something like this instead:

DOCKERHUB_USERNAME: {
  type: BuildEnvironmentVariableType.SECRETS_MANAGER,
  value: `${dockerhubCredentialsSecret.secretArn}:username`,
},
hugowschneider commented 1 year ago

This is happening also in CDK 2.61.1. The proposed flag in cdk.json does not exist in CDK 2. Any solutions on that?

hugowschneider commented 1 year ago

Following up on this.

I have a very similar code to this:

environment_variables={
                'ACCESS_TOKEN': {'type': cb.BuildEnvironmentVariableType.SECRETS_MANAGER, 'value': access_token_secret.secret_name},
            }

and I changed to something like:

environment_variables={
                'ACCESS_TOKEN': {'type': cb.BuildEnvironmentVariableType.SECRETS_MANAGER, 'value': access_token_secret.secret_arn},
            }

Don't ask me why changing from secrect_name to secret_arn makes a difference, but now it works. I hope this issue gets fixed soon

BwL1289 commented 1 year ago

I am currently experiencing this. Is there an update? For others reading, @hugowschneider's solution worked.