ssm: service principals are incorrect for all regions since ap-east-1

[egriffith]

🐛 Bug Report

The Issue

iam.ServicePrincipal (https://docs.aws.amazon.com/cdk/api/latest/python/aws_cdk.aws_iam/ServicePrincipal.html) offers a "region" parameter. Presumably this parameter is so you can say something like ssm.ap-us-east-1.amazonaws.com to be inclusive of regional-only service endpoints.

Such as the ones called out on this page: https://docs.aws.amazon.com/systems-manager/latest/userguide/sysman-inventory-datasync.html#systems-manager-inventory-resource-data-sync-AWS-Organizations

"Note

The Asia Pacific Region came online in April 25, 2019. If you create a resource data sync for an AWS Region that came online since the Asia Pacific (Hong Kong) Region (ap-east-1) or later, then you must enter a Region-specific service principal entry in the SSMBucketDelivery section. The following example includes a Region-specific service principal entry for ssm.ap-east-1.amazonaws.com."

However

Here is a snippet of CDK Python that results in some odd behavior:

        kmsKey = kms.Key(
            self,
            "S3-KMSKey",
        )

        kmsKey.add_to_resource_policy(
            statement=iam.PolicyStatement(
                sid="ssm-access-policy",
                conditions=[],
                effect=iam.Effect.ALLOW,
                actions=["kms:GenerateDataKey"],
                principals=[
                    iam.ServicePrincipal(service="ssm.amazonaws.com"),
                    iam.ServicePrincipal(
                        service="ssm.amazonaws.com", region="ap-east-1"
                    ),
                    iam.ServicePrincipal(service="ssm", region="ap-east-1"),
                ],
                resources=[kmsKey.key_arn],
            )
        )

That snippet uses ServicePrincipal three different ways, two of which should result in regional endpoints, and yet none of them do.

That snippet spits out something that looks like...

Resources:
  S3KMSKey26947010:
    Type: AWS::KMS::Key
    Properties:
      KeyPolicy:
        Statement:
          - Action: kms:*
            Effect: Allow
            Principal:
              AWS:
                Fn::Join:
                  - ""
                  - - "arn:"
                    - Ref: AWS::Partition
                    - ":iam::"
                    - Ref: AWS::AccountId
                    - :root
            Resource: "*"
          - Action: kms:GenerateDataKey
            Effect: Allow
            Principal:
              Service:
                - ssm.amazonaws.com
                - ssm.amazonaws.com
                - ssm.amazonaws.com
            Resource:
              Fn::GetAtt:
                - S3KMSKey26947010
                - Arn
            Sid: ssm-access-policy
        Version: "2012-10-17"
    UpdateReplacePolicy: Retain
    DeletionPolicy: Retain
    Metadata:
      aws:cdk:path: SsmInventoryAthenaStack/S3-KMSKey/Resource

Notice that none of the Principal -> Service definitions have 'ap-east-1' in their URLs.

This might be related to these two issues:
#2622
#2999

Where CDK was exclusively crafting regional endpoints

Environment

• CDK CLI Version: 2.0.0-rc.17 (build fb5dc58)
• Module Version: 2.0.0rc17 (I think, pulled from python's Pipfile.lock for aws-cdk-libs)
• Node.js Version: v14.17.1
• OS: OSX
• Language: Python

[rix0rrr]

region= is a way to override the default region, but it does not control whether the region is actually rendered. You should almost never need to supply it, because the correct value is taken from the containing Stack by default.

Whether or not the region is rendered is controlled here, in the region-info package:

aws-cdk/packages/@aws-cdk/region-info/lib/default.ts

Line 23 in 67b4921

public static servicePrincipal(service: string, region: string, urlSuffix: string): string {

Looks like we are missing the fact that SSM service principals need to be regionalized for all regions since 2019.

Until this is fixed, you can override the fact database: https://github.com/aws/aws-cdk/tree/v1.125.0/packages/%40aws-cdk/region-info#supplying-new-or-missing-information

[njlynch]

This should be a fairly straight-forward fix (adding ssm) to the list of region-specific principals here:

aws-cdk/packages/@aws-cdk/region-info/lib/default.ts

Lines 83 to 85 in 67b4921

	// Services with a regional principal
	case 'states':
	return `${service}.${region}.amazonaws.com`;

The "gotcha" will just be testing & verifying that the regional service principals also work for the older regions (e.g., us-east-1, eu-west-1). The linked documentation above isn't explicit about this, so a manual test to verify seems in order.

Contributions welcome!

[github-actions]

⚠️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.

[mickael-caro-sonarsource]

@njlynch @rix0rrr this change doesn't work with eu-central-1, CloudFormation complains about a malformed document.

[stephenwiebe]

@njlynch @rix0rrr This appears to have broken our stack in us-east-1, after updating to 1.134.0 getting:

Invalid principal in policy: "SERVICE":"ssm.us-east-1.amazonaws.com" (Service: AmazonIdentityManagement; Status Code: 400; Error Code: MalformedPolicyDocument; Request ID: 86fd6c33-2099-4887-aff4-efff716bc7ea; Proxy: null)

This is the relevant IAM role I'm creating with the SSM service principal:

const ssmServiceRole = new iam.Role(this, 'SSMServiceRole', {
      assumedBy: new iam.ServicePrincipal('ssm.amazonaws.com'),
      managedPolicies: [iam.ManagedPolicy.fromAwsManagedPolicyName('AmazonSSMManagedInstanceCore')],
      roleName: 'SSMServiceRole'
    });

[adhorn]

Also running into the issue.

        // SSMA Role for SSMA Documents faults
        const ssmarole = new iam.Role(this, 'ssma-role', {
            assumedBy: new iam.CompositePrincipal(
                new iam.ServicePrincipal('iam.amazonaws.com'),
                new iam.ServicePrincipal('ssm.amazonaws.com')
            )
        });

        // ssmarole.assumeRolePolicy?.addStatements(
        //     new iam.PolicyStatement({
        //       effect: iam.Effect.ALLOW,
        //       principals: [new iam.ServicePrincipal('ssm.amazonaws.com')],
        //       actions: ['sts:AssumeRole'],
        //     }),
        //   );

Results to:

│ + │ ${FisRole/ssma-role.Arn} │ Allow  │ sts:AssumeRole │ Service:iam.amazonaws.com
│ + │ ${FisRole/ssma-role.Arn} │ Allow  │ sts:AssumeRole │ Service:ssm.${AWS::Region}.amazonaws.com               

Which obviously fails

5:54:10 PM | UPDATE_FAILED        | AWS::IAM::Role     | ssmarole20460B02
Invalid principal in policy: "SERVICE":"ssm.us-east-1.amazonaws.com" (Service: AmazonIdentityManagement; Status Code: 400; Error Code: MalformedPolicyDocument; Request ID: 07c942
b5-8982-4867-974c-239f3bb48dff; Proxy: null)

[rix0rrr]

Reopening since this seems unresolved.

It's probably not that ALL ssm ServicePrincipals need to be regionalized, just those for "new" regions.

[rix0rrr]

Also fun: there is apparently no way to write this service principal in an environment-agnostic way.

• It must be ssm.amazonaws.com for "old" regions
• It must be ssm.$REGION.amazonaws.com for "new" regions

There does not seem to be a single string that will work in both, and { Fn::FindInMap } cannot do defaults.

[rix0rrr]

On further scouring of the docs, a CfnCondition combined with a Fn.if might do it. We would render something like:

Conditions:
  NoRegionNecessary:
    Fn::Or: 
      - Fn::Equals: [{ Ref: 'AWS::Region }, 'us-east-1']
      - Fn::Equals: [{ Ref: 'AWS::Region }, 'us-east-2']
      - Fn::Equals: [{ Ref: 'AWS::Region }, 'eu-west-1']
      ...

Resources:
  ... 
  Principal:
    Service: { Fn::If: [NoRegionNecessary, 'ssm.amazonaws.com', 'ssm.$REGION.amazonaws.com'] } 

It wouldn't be pretty in the JSON, but who looks at that anyway eh? 😇

[adhorn]

Workaround:

const ssmaAsgRole = new iam.Role(this, 'ssma-asg-role', {
        assumedBy: new iam.CompositePrincipal(
        new iam.ServicePrincipal('iam.amazonaws.com'),
        new iam.ServicePrincipal('ssm.amazonaws.com')
       )
});
const ssmaAsgRoleAsCfn = ssmaAsgRole.node.defaultChild as iam.CfnRole;
ssmaAsgRoleAsCfn.addOverride('Properties.AssumeRolePolicyDocument.Statement.0.Principal.Service', ['ssm.amazonaws.com', 'iam.amazonaws.com']);

[github-actions]

⚠️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.