SSM Module: String Parameter assumes parameter will have a '/' at the front of the string.

[adamjkeller]

Describe the bug

When creating a string parameter in SSM parameter store, CDK makes the assumption that a '/' will be placed in front of the parameter. This is not a requirement of SSM parameter store; hence, when creating a parameter without a '/' in the front of the string and granting access from another resource to this parameter, cdk will provide an ARN that is invalid. This isn't immediately noticable as the IAM policy will still get created, but one would have to dig to figure out why the requested access is not working as expected.

See here:

https://github.com/awslabs/aws-cdk/blob/master/packages/%40aws-cdk/aws-ssm/lib/parameter.ts#L125

To Reproduce

cdk deploy

app.py:

#!/usr/bin/env python3

from aws_cdk import (
    aws_lambda,
    aws_ssm,
    cdk
)


class LambdaTestSSMParam(cdk.Stack):
    def __init__(self, app: cdk.App, id: str) -> None:
        super().__init__(app, id)

        string_param = aws_ssm.StringParameter(
            self, "StringParameterWithoutSlash",
            name="NO_SLASH_STRING_PARAM",
            string_value="test"
        )

        # If you want to see the function actually fail due to lack of permissions
        lambda_code = """
#!/usr/bin/env python3
def lambda_handler(event, context):
    import boto3 
    client = boto3.client('ssm')
    return client.get_parameter(
        Name='{}',
        WithDecryption=False
    )
""".format(string_param.parameter_name)

        lambda_function = aws_lambda.Function(
            self, "BasicLambda",
            code=aws_lambda.InlineCode(lambda_code),
            handler="index.lambda_handler",
            timeout=30,
            runtime=aws_lambda.Runtime.PYTHON37,
        )

        string_param.grant_read(lambda_function)


app = cdk.App()
LambdaTestSSMParam(app, "LambdaCronExample")
app.run()

IAM policy that is created:

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Action": [
                "ssm:DescribeParameters",
                "ssm:GetParameter",
                "ssm:GetParameterHistory"
            ],
            "Resource": "arn:aws:ssm:us-west-2:580961807929:parameterNO_SLASH_STRING_PARAM",
            "Effect": "Allow"
        }
    ]
}

Expected behavior

IAM Policy :

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Action": [
                "ssm:DescribeParameters",
                "ssm:GetParameter",
                "ssm:GetParameterHistory"
            ],
            "Resource": "arn:aws:ssm:us-west-2:580961807929:parameter/NO_SLASH_STRING_PARAM",
            "Effect": "Allow"
        }
    ]
}

Version:

• Mac OSX 10.13.6
• Python 3.7.3
• CDK Version: 0.33.0 (build 50d71bf)

[rileylyman]

PR #1726, which introduced this error, modified some code in arn.ts In particular, if you look at the changes the first commit made to lines 29-31 of that file, originally we had

if (sep !== '/' && sep !== ':') {
  throw new Error('resourcePathSep may only be ":" or "/"');
}

but the change made it so sep === '' was also allowed. I think this was under the assumption that resourceName would begin with a /, like you said. So maybe the fix to this problem that makes the most sense is to only accept sep === '' if resourceName.startsWith('/'). But now this would cause your example to fail since the string parameter name does not start with /.

In PR #1726, they say "the SSM parameter name (resourceName portion of it's ARN) is required to start with a /." In this case, your example failing would be the correct behavior. But I have not been able to find anything else that confirms this, so otherwise we could just change the ParameterBase implementation such that it sets sep = '/' if parameterName does not start with /.

[rileylyman]

Looking through the unit tests further in test.parameter.ts, it actually seems like we expect no seperator between resource and resourceName to behave this way (see this test). Can anyone weigh in on this?

For instance, in the linked test they expect the correct ARN to be parameterMyParamName instead of parameter/MyParamName.

[made2591]

I just made a parameter without "/": I also supposed it was required but actually is not. If it's ok for everyone, maybe I can jump on this - maybe as suggested by @rileylyman

[RomainMuller]

@made2591 - if you're willing to try to fix this, it sounds like the current understanding of the issue is sane. There is one trick however: if you create a parameter without a leading / in the name, SSM Parameter Store will append it for you (for the exact same ARN would be used for the parameter with a leading /).

So I guess the fix probably should be to completely ban leading slashes (beware - there could be unresolved tokens!) and just compose the ARN with the / separator.

[made2591]

Hi @RomainMuller and thank you for the suggestion!

I don't know if I got it right: I supposed to just force the parameter name to start without / by introducing in the arnForParameterName function (source) a check like this:

if (parameterName.charAt(0) === "/") {
    throw new Error(`The supplied parameterName (${parameterName}) already starts by default with '/': you cannot insert '/'.`);
}

and then change the value of the sep property in the subsequent call to be exactly /. Then, the formatArn function (source) will take care of calling the Arn.format function with the specified separator, and cover the use case without changing the way arn is built. Am I missing something?

Also: is that possible that parameterName will be unresolved in SSM package (and if so, why)?

[jonnyyu]

Just a note, the auto generated parameter_name doesn't begin with '/' either.
please fix the auto generated parameter_name too.

ssm_param = ssm.StringParameter(self, '/myparam', string_value='some value')

[gordonmleigh]

edit: removed unnecessary grumbling :)

@RomainMuller should not have modified the general arnFromComponents function to support the case for a single kind of resource, and he should not have been allowed to by @eladb who approved the PR. The arnForParameterName function should have been modified to normalise the parameter going in. Like so:

function arnForParameterName(scope: IConstruct, parameterName: string): string {
  return Stack.of(scope).formatArn({
    service: 'ssm',
    resource: 'parameter',
    sep: '/',
    resourceName: parameterName.replace(/^\//,''),
  });
}

If the parameter name is supposed to have a slash in front of it, normalise that to have a slash in front of it.

And yes @rileylyman is correct there is an incorrectly passing test for this, just to compound the issue:

aws-cdk/packages/@aws-cdk/aws-ssm/test/test.parameter.ts

Lines 116 to 127 in 6f22446

	test.deepEqual(stack.resolve(param.parameterArn), {
	'Fn::Join': ['', [
	'arn:',
	{ Ref: 'AWS::Partition' },
	':ssm:',
	{ Ref: 'AWS::Region' },
	':',
	{ Ref: 'AWS::AccountId' },
	':parameter',
	{ Ref: 'Parameter9E1B4FBA' }
	]]
	});

[gordonmleigh]

I am happy to contribute the fix to this, if no-one else is doing one right now. First I need to understand, why is the parameter name assumed to (or supposed to) have a leading slash?