Add rule I3042 to check for hardcoded partitions, account IDs, and regions in an ARN

[kddejong]

Issue #, if available:
replace #1601, #1532, #1533

Description of changes:

• add rule I3042 to check for hardcoded partitions, account IDs, and regions in an ARN

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[atkinsonm]

I support this rule. Parameters or pseudo-parameters are almost always better than hard-coding partitions, account IDs, and regions in an ARN. Hardcoded ARNs makes templates less-reusable and add reliability/deployment risks. Suggest that this be categorized as a warning rather than an error since hardcoded ARNs are still syntactically valid. I see that this is in the "informational" category, which is fine with me.

[gruebel]

From my point of view, this is a tricky one. I can see the reason behind adding, but tt will show a lot of false positives for cross account and cross region references. Also cross stack references will trigger this rule, because using ImportValue has its pro and cons.

[PatMyron]

From my point of view, this is a tricky one. I can see the reason behind adding, but tt will show a lot of false positives for cross account and cross region references

Agreed, I'd prefer this only start with hardcoded partitions because that's the only one that doesn't have valid use cases since cross partition references are not possible

[kddejong]

The error level is currently informational so this wouldn't error by default.

Additionally all we care is that its not hard coded. So if it is referring to a parameter, import value, find in map it will pass. Ideally it would be to the pseudo parameters that AWS provides but the provided example of cross account/region is a valid use case a lot of people use.

To Pat's point the only one I think shouldn't be a parameter or map is probably the partition.

[kddejong]

I've changed this to be configurable. By default it will check AWS Partition only. From there you can configure the AWS region and the AccountId checks.

[bgardner-noggin]

So this latest update just caused our pipelines to fail. We will be adding the configuration to ignore this rule by default. We use jinja with parameter files to render our templates and a number of properties have cross account references.

I can see this rule being useful when a substitution is in effect eg

AcmCertificateArn: !Sub 'arn:aws:acm:${AWS::Region}:${AWS::AccountId}:certificate/${SomeParameter}'

but not for the following case

AcmCertificateArn: 'arn:aws:acm:us-east-1:1234567890:certificate/xxxxxxxxxx-xxxxxxxxxx-xxxxxx-xxxxxx'

[kddejong]

@bgardner-noggin looking into it. This rule is also configurable but your configuration would result in the same as ignoring the rules.

I agree on the Fn::Sub portion of it. I may make a quick tweak on that and I apologize for the inconvenience.

[benbridts]

I agree on the Fn::Sub portion of it. I may make a quick tweak on that and I apologize for the inconvenience.

Are you saying that we don't want to flag completely hardcoded ARNS as informational? I agree with that for the current implementation that only flags the partition, I'm not sure I agree in general. Once there is consensus about what is and isn't hardcoded, I think it could be nice to have a check that indicates that an ARN is hardcoded.

Although I can think of some more failure cases:

• SAR apps
• Lambda Layers
• AWS account ids in bucket policies, like for ALB logging
• maybe an organization has a "well-known" account id in trust policies

Counter to the counter: some of those could use at least an AWS::Region

[kddejong]

Yea, I'm still going back and forth on it and I'm looking for some feedback on this. As I went back and looked at some of the scenarios I kind of wanted to leave it as it is but I think there are scenarios as you mention where a hard coded ARN is legit. The one debate I am having is switching this rule to be checking a Fn::Sub then we could discuss a hard coded ARN without a sub as a different rule.

Also the region and acocuntId are configurable so they can be enabled. The thought was there are valid reasons to hard code those (example: known account inside an organization) but we wanted to allow people to enable those ones if they wanted (as the original rule writer was looking to do).