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

chore: remove use of deprecated ServicePrincipal Mapping #30832

Merged
merged 9 commits into from
Jul 12, 2024

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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this service principal should have the region in it, i.e. states.us-west-1.amazonaws.com. This could be right since the principal is sans region some of the time, but looking through the regional maps we had, a majority of the regions are included in the principal.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or at least it should not be static if it can be either. The test should be environment agnostic and be a mapping, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only context in which any region should be included is in opt in regions when the permissions are cross region. This is not that case.

},
},
],
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
Loading