Skip to content

Commit

Permalink
fix(core): stack.urlSuffix is no longer scoped (#4011)
Browse files Browse the repository at this point in the history
`stack.urlSuffix` used to be a scoped Token. By the use of roles defined
in another stack (using a `ServicePrincipal` which uses a `urlSuffix`
token), this could lead to unintentional stack references.

Two changes here:

* URL Suffix (seems to) only change when Partition changes. Since
  Partition is unscoped (cross-partition references won't work anyway),
  we might as well make URL Suffix unscoped too.
* `ServicePrincipalToken` should not have used the stack's `urlSuffix`,
  but constructed an unscoped `URL_SUFFIX` itself, since it was
  never intended to potentially create a cross-stack reference. It
  couldn't have, since it doesn't know where it is being defined,
  it just knows where it's being used.

Technically, the second change isn't necessary anymore after we
apply the first, but I made both anyway since the bug is still
resolved even if find out we need roll back the first change
because of a future region build.

Fixes #3970.
  • Loading branch information
rix0rrr authored and mergify[bot] committed Sep 13, 2019
1 parent f63bf6f commit 82e08bc
Show file tree
Hide file tree
Showing 4 changed files with 74 additions and 3 deletions.
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-iam/lib/principals.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import cdk = require('@aws-cdk/core');
import { captureStackTrace, Stack } from '@aws-cdk/core';
import { Aws, captureStackTrace, Stack } from '@aws-cdk/core';
import { Default, RegionInfo } from '@aws-cdk/region-info';
import { PolicyStatement } from './policy-statement';
import { mergePrincipal } from './util';
Expand Down Expand Up @@ -340,7 +340,7 @@ class ServicePrincipalToken implements cdk.IResolvable {
public resolve(ctx: cdk.IResolveContext) {
const region = this.opts.region || Stack.of(ctx.scope).region;
const fact = RegionInfo.get(region).servicePrincipal(this.service);
return fact || Default.servicePrincipal(this.service, region, Stack.of(ctx.scope).urlSuffix);
return fact || Default.servicePrincipal(this.service, region, Aws.URL_SUFFIX);
}

public toString() {
Expand Down
51 changes: 51 additions & 0 deletions packages/@aws-cdk/aws-iam/test/principals.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import '@aws-cdk/assert/jest';
import { App, CfnOutput, Stack } from '@aws-cdk/core';
import iam = require('../lib');

test('use of cross-stack role reference does not lead to URLSuffix being exported', () => {
// GIVEN
const app = new App();
const first = new Stack(app, 'First');
const second = new Stack(app, 'Second');

// WHEN
const role = new iam.Role(first, 'Role', {
assumedBy: new iam.ServicePrincipal('s3.amazonaws.com')
});

new CfnOutput(second, 'Output', {
value: role.roleArn
});

// THEN
app.synth();

expect(first).toMatchTemplate({
Resources: {
Role1ABCC5F0: {
Type: "AWS::IAM::Role",
Properties: {
AssumeRolePolicyDocument: {
Statement: [
{
Action: "sts:AssumeRole",
Effect: "Allow",
Principal: { Service: "s3.amazonaws.com" }
}
],
Version: "2012-10-17"
}
}
}
},
Outputs: {
ExportsOutputFnGetAttRole1ABCC5F0ArnB4C0B73E: {
Value: { "Fn::GetAtt": [ "Role1ABCC5F0", "Arn" ] },
Export: {
Name: "First:ExportsOutputFnGetAttRole1ABCC5F0ArnB4C0B73E"
}
}
}
}
);
});
3 changes: 2 additions & 1 deletion packages/@aws-cdk/core/lib/stack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,8 @@ export class Stack extends Construct implements ITaggable {
* The Amazon domain suffix for the region in which this stack is defined
*/
public get urlSuffix(): string {
return new ScopedAws(this).urlSuffix;
// Since URL Suffix always follows partition, it is unscoped like partition is.
return Aws.URL_SUFFIX;
}

/**
Expand Down
19 changes: 19 additions & 0 deletions packages/@aws-cdk/core/test/test.stack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,25 @@ export = {
test.done();
},

'urlSuffix does not imply a stack dependency'(test: Test) {
// GIVEN
const app = new App();
const first = new Stack(app, 'First');
const second = new Stack(app, 'Second');

// WHEN
new CfnOutput(second, 'Output', {
value: first.urlSuffix
});

// THEN
app.synth();

test.equal(second.dependencies.length, 0);

test.done();
},

'stack with region supplied via props returns literal value'(test: Test) {
// GIVEN
const app = new App();
Expand Down

0 comments on commit 82e08bc

Please sign in to comment.