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

aws_ssm: When creating or consuming an SSM Parameter w/ a simple name, the arn attribute of Parameter/IParameter construct contains an extra slash after 'parameter' in the arn. #28778

Closed
mttwise opened this issue Jan 19, 2024 · 5 comments · Fixed by #30653
Assignees
Labels
@aws-cdk/aws-ssm Related to AWS Systems Manager bug This issue is a bug. effort/medium Medium work item – several days of effort p2

Comments

@mttwise
Copy link

mttwise commented Jan 19, 2024

Describe the bug

Scenario 1: I am creating an ssm parameter in CDK. I have to use simple_name=True because i need to use a token for the prefix. I pass the parameter path to an IAM policy using the parameter method parameter_arn .

my_parameter = aws_ssm.StringParameter(
            construct,
            "ssm-my-parameter",
            string_value="SOME_VALUE",
            description="Some Description",
            parameter_name=f"/{ssm_prefix}/path/for/my/parameter",
            simple_name=True,
        )

After creating the StringParameter, the parameter_arn property is used in an IAM role to grant access to read the parameter:

#Inside of IAM role
inline_policies={
    "ssm-policy": aws_iam.PolicyDocument(
        statements=[
            aws_iam.PolicyStatement(
                effect=aws_iam.Effect.ALLOW,
                actions=["ssm:GetParameter"],
                resources=[my_parameter.parameter_arn],
           ),
      ]
}

The resulting policy in the synth'd template.json looks like this:

"Policies": [
    {
        "PolicyDocument": {
            "Statement": [
                {
                    "Action": [
                        "ssm:GetParameter",
                        "ssm:PutParameter"
                    ],
                    "Effect": "Allow",
                    "Resource": {
                        "Fn::Join": [
                            "",
                            [
                                "arn:",
                                {
                                    "Ref": "AWS::Partition"
                                },
                                ":ssm:",
                                {
                                    "Ref": "AWS::Region"
                                },
                                ":",
                                {
                                    "Ref": "AWS::AccountId"
                                },
                                ":parameter/",
                                {
                                    "Ref": "MyParameter"
                                }
                            ]
                        ]
                    }
                }
            ],
            "Version": "2012-10-17"
        },
        "PolicyName": "ssm-policy"
    }
]

Note that within the join, the parameter string has a / so it gets concatenated with the Ref MyParameter (which contains a / already)
The resultant ARN gets an extra slash so it looks like this:

arn:aws:ssm:us-east-1:1111111111111:parameter//prefix/for/parameter/path/for/my/parameter


Scenario 2: I want to dynamically consume an existing SSM Parameter at deploy time of my synth'd CFN template.

Example: Use the value of a CfnParameter in the path of an SSM String Parameter lookup:

id_property = CfnParameter(
    Stack.of(self),
    "IdProperty",
    description="User input specifying id of deployment",
    default="value",
    type="String",
).value_as_string

my_parameter = aws_ssm.StringParameter.from_string_parameter_attributes(
    self,
    "ssm-param",
    parameter_name=f"/prefix/{id_property}/path/to/my/parameter"
    simple_name=True,
    force_dynamic_reference=True,
)

In this scenario, two differing results are observed depending on how the IParameter is used.

the resulting CloudFormation Template gets a resolve string that contains a double / if the FormatArn function is used or if the IParameter is passed to another construct such as an ECS Task for use in Secrets.

When using parameter.string_value, the correct resolve string is observed:

"Value": {
    "Fn::Join": [
        "",
        [
            "{{resolve:ssm:/prefix/",
            {
                "Ref": "IdProperty"
            },
            "/path/to/my/parameter}}"
        ]
    ]
}

This effectively looks like this:
{{resolve:ssm://prefix/some-id/path/to/my/parameter}}

However, when the IParameter is passed to the task_definition.add_container function, a second slash is observed in the resultant secret property in CFN.

task_definition.add_container(
...
secrets={
    "MY_SECRET": aws_ecs.Secret.from_ssm_parameter(my_parameter),
}
...

Note that the second slash is being inserted during synth.

The resultant CFN Template resolve looks like this:

...
    "Secrets": [
    {
        "Name": "BACKSTAGE_NAME",
        "ValueFrom": {
            "Fn::Join": [
                "",
                [
                    "arn:aws:ssm:us-east-1:11111111111:parameter//prefix/",
                    {
                         "Ref": "IdProperty"
                    },
                    "/path/to/my/parameter"
               ]
           ]
       }
    }
]

There are two issues here, the first is that the account and region are being hardcoded despite the string being dynamic. The second is that there is an extra slash after parameter in the arn.
In addition, the biggest issue is that there is a difference in behavior between the resolve string fromstring_value and how the ARN is generated.

Expected Behavior

When using SSM Parameter/IParameter constructs with simple_name=True, the construct should not add an additional / before the ARN/resolve strings.

Current Behavior

Included in bug description.

Reproduction Steps

Included in bug description

Possible Solution

For backwards compatibility, there could be a new additional property used in conjunction with simple_name=True to allow setting the construct to not add the slash.

Another solution could be removing the validation that there are no tokens in SSM parameters when not using the 'simple_name' parameter

Additional Information/Context

No response

CDK CLI Version

2.117.0

Framework Version

No response

Node.js Version

v18.17.1

OS

OSX Sonoma

Language

Python

Language Version

3.10.11

Other information

No response

@mttwise mttwise added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jan 19, 2024
@github-actions github-actions bot added the @aws-cdk/aws-ssm Related to AWS Systems Manager label Jan 19, 2024
@pahud
Copy link
Contributor

pahud commented Jan 22, 2024

Thank you for the detailed report and proposed solutions. I am making it a p1 as it's not easy to work it around but we welcome any PRs from the community as well.

@pahud pahud added p1 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Jan 22, 2024
@aaythapa aaythapa self-assigned this Feb 8, 2024
@aaythapa aaythapa removed their assignment Feb 29, 2024
@pahud pahud added p2 and removed p1 labels Jun 11, 2024
@GavinZZ GavinZZ self-assigned this Jun 24, 2024
@GavinZZ
Copy link
Contributor

GavinZZ commented Jun 24, 2024

Hi @mttwise, thanks for the thorough and detail descriptions. It really helps my investigation a lot easier. I can confirm that I am able to reproduce the issues. However, I do not think this is a bug in the code but a usage issue. Here is my reasoning:

In your first use case, you need to use a token for the prefix of the string parameter. However, based on the looking of your parameter name, f"/{ssm_prefix}/path/for/my/parameter", it does not satisfy the requirement of being a simple name. By description, simple name indicates if the parameter name is a simple name (i.e. does not include "/" separators). So setting simple_name: false should solve the problem.

For the second use case, I think it's the same issue where this does not satisfy the requirement of being a simple name, yet you gave simple name. Setting the value to false should fix both problems you described, i.e. the arn should not have double / and parameter name and string value behaviors should stay as current.

That being said, what I can offer is to add a WARNING check to the code and to express a warning (non-blocking) whenever an incorrect usage of simple name.

Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

1 similar comment
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@mttwise
Copy link
Author

mttwise commented Jul 1, 2024

@GavinZZ Thanks for the detailed info, I'm testing out setting simple_name to false and using in conjunction with force_dynamic_reference for reads w/ tokens. I'll update and let you know if the issue is resolved now.

For a little more context, my original design included using a full token for the parameter_name, but later started using "/{token_after_slash}" which most likely is the reason why setting simple_name="False" will now work.

I think this remains an issue if you are using a fully qualified token name for the entire SSM param name since the cdk synth will throw an error complaining about the lack of '/' prefix in this case when simple_name=False

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ssm Related to AWS Systems Manager bug This issue is a bug. effort/medium Medium work item – several days of effort p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants