Skip to content

Commit

Permalink
chore: remove use of deprecated ServicePrincipal Mapping (#30832)
Browse files Browse the repository at this point in the history
They have now been standardized for a few years. We did not initially remove the old mappings out of caution and because we were unsure that the changes has made it to all regions yet. It is long past that happening at this point.

Because we never removed this or marked it as deprecated, we still have a not insignificant amount of customers who believe the individual mapping is necessary and cut tickets because it is not up-to-date.

### Issue # (if applicable)

Closes #<issue number here>.

### Reason for this change



### Description of changes



### Description of how you validated changes



### Checklist
- [ ] My code adheres to the [CONTRIBUTING GUIDE](https:/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https:/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
TheRealAmazonKendra authored Jul 12, 2024
1 parent 7ee8783 commit 4af3685
Show file tree
Hide file tree
Showing 24 changed files with 536 additions and 1,274 deletions.

Large diffs are not rendered by default.

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,7 @@ exports[`stepfunctions should grant pipe role invoke access 1`] = `
"Action": "sts:AssumeRole",
"Effect": "Allow",
"Principal": {
"Service": {
"Fn::FindInMap": [
"ServiceprincipalMap",
{
"Ref": "AWS::Region",
},
"states",
],
},
"Service": "states.amazonaws.com",
},
},
],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,7 @@ exports[`step-function should grant pipe role push access (StartAsyncExecution)
"Action": "sts:AssumeRole",
"Effect": "Allow",
"Principal": {
"Service": {
"Fn::FindInMap": [
"ServiceprincipalMap",
{
"Ref": "AWS::Region",
},
"states",
],
},
"Service": "states.amazonaws.com",
},
},
],
Expand Down Expand Up @@ -74,15 +66,7 @@ exports[`step-function should grant pipe role push access (StartAsyncExecution)
"Action": "sts:AssumeRole",
"Effect": "Allow",
"Principal": {
"Service": {
"Fn::FindInMap": [
"ServiceprincipalMap",
{
"Ref": "AWS::Region",
},
"states",
],
},
"Service": "states.amazonaws.com",
},
},
],
Expand Down Expand Up @@ -121,15 +105,7 @@ exports[`step-function should grant pipe role push access (StartSyncExecution) w
"Action": "sts:AssumeRole",
"Effect": "Allow",
"Principal": {
"Service": {
"Fn::FindInMap": [
"ServiceprincipalMap",
{
"Ref": "AWS::Region",
},
"states",
],
},
"Service": "states.amazonaws.com",
},
},
],
Expand Down
22 changes: 2 additions & 20 deletions packages/@aws-cdk/cx-api/FEATURE_FLAGS.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ Flags come in three types:
| [@aws-cdk/core:enablePartitionLiterals](#aws-cdkcoreenablepartitionliterals) | Make ARNs concrete if AWS partition is known | 2.38.0 | (fix) |
| [@aws-cdk/aws-ecs:disableExplicitDeploymentControllerForCircuitBreaker](#aws-cdkaws-ecsdisableexplicitdeploymentcontrollerforcircuitbreaker) | Avoid setting the "ECS" deployment controller when adding a circuit breaker | 2.51.0 | (fix) |
| [@aws-cdk/aws-events:eventsTargetQueueSameAccount](#aws-cdkaws-eventseventstargetqueuesameaccount) | Event Rules may only push to encrypted SQS queues in the same account | 2.51.0 | (fix) |
| [@aws-cdk/aws-iam:standardizedServicePrincipals](#aws-cdkaws-iamstandardizedserviceprincipals) | Use standardized (global) service principals everywhere | 2.51.0 | (fix) |
| [@aws-cdk/aws-iam:importedRoleStackSafeDefaultPolicyName](#aws-cdkaws-iamimportedrolestacksafedefaultpolicyname) | Enable this feature to by default create default policy names for imported roles that depend on the stack the role is in. | 2.60.0 | (fix) |
| [@aws-cdk/aws-s3:serverAccessLogsUseBucketPolicy](#aws-cdkaws-s3serveraccesslogsusebucketpolicy) | Use S3 Bucket Policy instead of ACLs for Server Access Logging | 2.60.0 | (fix) |
| [@aws-cdk/customresources:installLatestAwsSdkDefault](#aws-cdkcustomresourcesinstalllatestawssdkdefault) | Whether to install the latest SDK by default in AwsCustomResource | 2.60.0 | (default) |
Expand Down Expand Up @@ -72,7 +71,7 @@ Flags come in three types:
| [@aws-cdk/pipelines:reduceAssetRoleTrustScope](#aws-cdkpipelinesreduceassetroletrustscope) | Remove the root account principal from PipelineAssetsFileRole trust policy | 2.141.0 | (default) |
| [@aws-cdk/aws-ecs:removeDefaultDeploymentAlarm](#aws-cdkaws-ecsremovedefaultdeploymentalarm) | When enabled, remove default deployment alarm settings | 2.143.0 | (default) |
| [@aws-cdk/custom-resources:logApiResponseDataPropertyTrueDefault](#aws-cdkcustom-resourceslogapiresponsedatapropertytruedefault) | When enabled, the custom resource used for `AwsCustomResource` will configure the `logApiResponseData` property as true by default | 2.145.0 | (fix) |
| [@aws-cdk/aws-stepfunctions-tasks:ecsReduceRunTaskPermissions](#aws-cdkaws-stepfunctions-tasksecsreduceruntaskpermissions) | When enabled, IAM Policy created to run tasks won't include the task definition ARN, only the revision ARN. | V2NEXT | (fix) |
| [@aws-cdk/aws-stepfunctions-tasks:ecsReduceRunTaskPermissions](#aws-cdkaws-stepfunctions-tasksecsreduceruntaskpermissions) | When enabled, IAM Policy created to run tasks won't include the task definition ARN, only the revision ARN. | 2.148.0 | (fix) |

<!-- END table -->

Expand Down Expand Up @@ -101,7 +100,6 @@ The following json shows the current recommended set of flags, as `cdk init` wou
"@aws-cdk/aws-apigateway:disableCloudWatchRole": true,
"@aws-cdk/core:enablePartitionLiterals": true,
"@aws-cdk/aws-events:eventsTargetQueueSameAccount": true,
"@aws-cdk/aws-iam:standardizedServicePrincipals": true,
"@aws-cdk/aws-ecs:disableExplicitDeploymentControllerForCircuitBreaker": true,
"@aws-cdk/aws-iam:importedRoleStackSafeDefaultPolicyName": true,
"@aws-cdk/aws-s3:serverAccessLogsUseBucketPolicy": true,
Expand Down Expand Up @@ -748,22 +746,6 @@ always apply, regardless of the value of this flag.
| 2.51.0 | `false` | `true` |


### @aws-cdk/aws-iam:standardizedServicePrincipals

*Use standardized (global) service principals everywhere* (fix)

We used to maintain a database of exceptions to Service Principal names in various regions. This database
is no longer necessary: all service principals names have been standardized to their global form (`SERVICE.amazonaws.com`).

This flag disables use of that exceptions database and always uses the global service principal.


| Since | Default | Recommended |
| ----- | ----- | ----- |
| (not in v1) | | |
| 2.51.0 | `false` | `true` |


### @aws-cdk/aws-iam:importedRoleStackSafeDefaultPolicyName

*Enable this feature to by default create default policy names for imported roles that depend on the stack the role is in.* (fix)
Expand Down Expand Up @@ -1370,7 +1352,7 @@ for more details.
| Since | Default | Recommended |
| ----- | ----- | ----- |
| (not in v1) | | |
| V2NEXT | `false` | `true` |
| 2.148.0 | `false` | `true` |


<!-- END details -->
Original file line number Diff line number Diff line change
Expand Up @@ -140,15 +140,7 @@ describe('CodeDeploy ECS DeploymentGroup', () => {
Action: 'sts:AssumeRole',
Effect: 'Allow',
Principal: {
Service: {
'Fn::FindInMap': [
'ServiceprincipalMap',
{
Ref: 'AWS::Region',
},
'codedeploy',
],
},
Service: 'codedeploy.amazonaws.com',
},
}],
Version: '2012-10-17',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,15 +94,7 @@ describe('CodeDeploy Lambda DeploymentGroup', () => {
Action: 'sts:AssumeRole',
Effect: 'Allow',
Principal: {
Service: {
'Fn::FindInMap': [
'ServiceprincipalMap',
{
Ref: 'AWS::Region',
},
'codedeploy',
],
},
Service: 'codedeploy.amazonaws.com',
},
}],
Version: '2012-10-17',
Expand Down
13 changes: 10 additions & 3 deletions packages/aws-cdk-lib/aws-ec2/lib/vpc-endpoint-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { Construct } from 'constructs';
import { CfnVPCEndpointService, CfnVPCEndpointServicePermissions } from './ec2.generated';
import { ArnPrincipal } from '../../aws-iam';
import { Aws, Fn, IResource, Resource, Stack, Token } from '../../core';
import { Default, RegionInfo } from '../../region-info';
import { RegionInfo } from '../../region-info';

/**
* A load balancer that can host a VPC Endpoint Service
Expand Down Expand Up @@ -46,6 +46,13 @@ export interface IVpcEndpointService extends IResource {
*/
export class VpcEndpointService extends Resource implements IVpcEndpointService {

/**
* The default value for a VPC Endpoint Service name prefix, useful if you do
* not have a synthesize-time region literal available (all you have is
* `{ "Ref": "AWS::Region" }`)
*/
public static readonly DEFAULT_PREFIX = 'com.amazonaws.vpce';

/**
* One or more network load balancers to host the service.
* @attribute
Expand Down Expand Up @@ -119,8 +126,8 @@ export class VpcEndpointService extends Resource implements IVpcEndpointService

const { region } = Stack.of(this);
const serviceNamePrefix = !Token.isUnresolved(region) ?
(RegionInfo.get(region).vpcEndpointServiceNamePrefix ?? Default.VPC_ENDPOINT_SERVICE_NAME_PREFIX) :
Default.VPC_ENDPOINT_SERVICE_NAME_PREFIX;
(RegionInfo.get(region).vpcEndpointServiceNamePrefix ?? VpcEndpointService.DEFAULT_PREFIX) :
VpcEndpointService.DEFAULT_PREFIX;

this.vpcEndpointServiceName = Fn.join('.', [serviceNamePrefix, Aws.REGION, this.vpcEndpointServiceId]);
if (this.allowedPrincipals.length > 0) {
Expand Down
47 changes: 16 additions & 31 deletions packages/aws-cdk-lib/aws-iam/lib/principals.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,7 @@ import { defaultAddPrincipalToAssumeRole } from './private/assume-role-policy';
import { LITERAL_STRING_KEY, mergePrincipal } from './private/util';
import { ISamlProvider } from './saml-provider';
import * as cdk from '../../core';
import * as cxapi from '../../cx-api';
import { Default, FactName, RegionInfo } from '../../region-info';
import { RegionInfo } from '../../region-info';

/**
* Any object that has an associated principal that a permission can be granted to
Expand Down Expand Up @@ -541,11 +540,13 @@ export class ServicePrincipal extends PrincipalBase {
* These days all service principal names are standardized, and they are all
* of the form `<servicename>.amazonaws.com`.
*
* If the feature flag `@aws-cdk/aws-iam:standardizedServicePrincipals` is set, this
* method will always return its input. If this feature flag is not set, this
* method will perform the legacy behavior, which appends the region-specific
* domain suffix for some select services (for example, it would append `.cn`
* to some service principal names).
* To avoid breaking changes, handling is provided for services added with the formats below,
* however, no additional handling will be added for new regions or partitions.
* - s3
* - s3.amazonaws.com
* - s3.amazonaws.com.cn
* - s3.c2s.ic.gov
* - s3.sc2s.sgov.gov
*
* @example
* const principalName = iam.ServicePrincipal.servicePrincipalName('ec2.amazonaws.com');
Expand Down Expand Up @@ -942,44 +943,28 @@ class ServicePrincipalToken implements cdk.IResolvable {
}

public resolve(ctx: cdk.IResolveContext) {
return cdk.FeatureFlags.of(ctx.scope).isEnabled(cxapi.IAM_STANDARDIZED_SERVICE_PRINCIPALS)
? this.newStandardizedBehavior(ctx)
: this.legacyBehavior(ctx);

// The correct behavior is to always use the global service principal
return this.newStandardizedBehavior(ctx);
}

/**
* Return the global (original) service principal, and a second one if region is given and points to an opt-in region
*/
private newStandardizedBehavior(ctx: cdk.IResolveContext) {
const stack = cdk.Stack.of(ctx.scope);

// If the user had previously set the feature flag to `false` we would allow them to provide only the service name instead of the
// entire service principal. We can't break them so now everyone gets to do it!
const match = this.service.match(/^([^.]+)(?:(?:\.amazonaws\.com(?:\.cn)?)|(?:\.c2s\.ic\.gov)|(?:\.sc2s\.sgov\.gov))?$/);
const service = match ? `${match[1]}.amazonaws.com` : this.service;
if (
this.opts.region &&
!cdk.Token.isUnresolved(this.opts.region) &&
stack.region !== this.opts.region &&
RegionInfo.get(this.opts.region).isOptInRegion
) {
return this.service.replace(/\.amazonaws\.com$/, `.${this.opts.region}.amazonaws.com`);
}
return this.service;
}

/**
* Do a single lookup
*/
private legacyBehavior(ctx: cdk.IResolveContext) {
if (this.opts.region) {
// Special case, handle it separately to not break legacy behavior.
return RegionInfo.get(this.opts.region).servicePrincipal(this.service) ??
Default.servicePrincipal(this.service, this.opts.region, cdk.Aws.URL_SUFFIX);
return service.replace(/\.amazonaws\.com$/, `.${this.opts.region}.amazonaws.com`);
}

const stack = cdk.Stack.of(ctx.scope);
return stack.regionalFact(
FactName.servicePrincipal(this.service),
Default.servicePrincipal(this.service, stack.region, cdk.Aws.URL_SUFFIX),
);
return service;
}

public toString() {
Expand Down
17 changes: 1 addition & 16 deletions packages/aws-cdk-lib/aws-iam/test/policy-document.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { testDeprecated } from '@aws-cdk/cdk-build-tools';
import { Template } from '../../assertions';
import { Lazy, Stack, Token } from '../../core';
import {
Expand Down Expand Up @@ -464,21 +463,7 @@ describe('IAM policy document', () => {
expect(stack.resolve(s.toStatementJson())).toEqual({
Effect: 'Allow',
Action: 'test:Action',
Principal: { Service: 'codedeploy.cn-north-1.amazonaws.com.cn' },
});
});

// Deprecated: 'region' parameter to ServicePrincipal shouldn't be used.
testDeprecated('regional service principals resolve appropriately (with user-set region)', () => {
const stack = new Stack(undefined, undefined, { env: { region: 'cn-northeast-1' } });
const s = new PolicyStatement();
s.addActions('test:Action');
s.addServicePrincipal('codedeploy.amazonaws.com', { region: 'cn-north-1' });

expect(stack.resolve(s.toStatementJson())).toEqual({
Effect: 'Allow',
Action: 'test:Action',
Principal: { Service: 'codedeploy.cn-north-1.amazonaws.com.cn' },
Principal: { Service: 'codedeploy.amazonaws.com' },
});
});

Expand Down
24 changes: 3 additions & 21 deletions packages/aws-cdk-lib/aws-iam/test/principals.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -364,29 +364,13 @@ describe('deprecated ServicePrincipal behavior', () => {
const afSouthStack = new Stack(undefined, undefined, { env: { region: 'af-south-1' } });
const principalName = iam.ServicePrincipal.servicePrincipalName('states.amazonaws.com');

expect(usEastStack.resolve(principalName)).toEqual('states.us-east-1.amazonaws.com');
expect(afSouthStack.resolve(principalName)).toEqual('states.af-south-1.amazonaws.com');
expect(usEastStack.resolve(principalName)).toEqual('states.amazonaws.com');
expect(afSouthStack.resolve(principalName)).toEqual('states.amazonaws.com');
});

test('Passing non-string as accountId parameter in AccountPrincipal constructor should throw error', () => {
expect(() => new iam.AccountPrincipal(1234)).toThrowError('accountId should be of type string');
});

test('ServicePrincipal in agnostic stack generates lookup table', () => {
// GIVEN
const stack = new Stack();

// WHEN
new iam.Role(stack, 'Role', {
assumedBy: new iam.ServicePrincipal('states.amazonaws.com'),
});

// THEN
const template = Template.fromStack(stack);
const mappings = template.findMappings('ServiceprincipalMap');
expect(mappings.ServiceprincipalMap['af-south-1']?.states).toEqual('states.af-south-1.amazonaws.com');
expect(mappings.ServiceprincipalMap['us-east-1']?.states).toEqual('states.us-east-1.amazonaws.com');
});
});

describe('standardized Service Principal behavior', () => {
Expand All @@ -396,9 +380,7 @@ describe('standardized Service Principal behavior', () => {

let app: App;
beforeEach(() => {
app = new App({
postCliContext: { [cxapi.IAM_STANDARDIZED_SERVICE_PRINCIPALS]: true },
});
app = new App();
});

test('no more regional service principals by default', () => {
Expand Down
18 changes: 2 additions & 16 deletions packages/aws-cdk-lib/aws-logs-destinations/test/kinesis.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,7 @@ test('stream can be subscription destination', () => {
Action: 'sts:AssumeRole',
Effect: 'Allow',
Principal: {
Service: {
'Fn::Join': ['', [
'logs.',
{ Ref: 'AWS::Region' },
'.',
{ Ref: 'AWS::URLSuffix' },
]],
},
Service: 'logs.amazonaws.com',
},
}],
},
Expand Down Expand Up @@ -102,14 +95,7 @@ test('stream can be subscription destination twice, without duplicating permissi
Action: 'sts:AssumeRole',
Effect: 'Allow',
Principal: {
Service: {
'Fn::Join': ['', [
'logs.',
{ Ref: 'AWS::Region' },
'.',
{ Ref: 'AWS::URLSuffix' },
]],
},
Service: 'logs.amazonaws.com',
},
}],
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,16 +88,7 @@ describe('state machine', () => {
Action: 'sts:AssumeRole',
Effect: 'Allow',
Principal: {
Service: {
'Fn::Join': [
'',
[
'states.',
stack.resolve(stack.region),
'.amazonaws.com',
],
],
},
Service: 'states.amazonaws.com',
},
},
],
Expand Down
Loading

0 comments on commit 4af3685

Please sign in to comment.