Skip to content

Commit 45b2057

Browse files
mergify[bot]mergify[bot]
authored
Merge branch 'main' into scheduler-unit-test-lambda-version
2 parents dcefa28 + 8d06824 commit 45b2057

2 files changed

Lines changed: 62 additions & 28 deletions

File tree

‎packages/aws-cdk/lib/api/util/checks.ts‎

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -18,13 +18,19 @@ export async function determineAllowCrossAccountAssetPublishing(sdk: ISDK, custo
1818
return true;
1919
}
2020

21-
// other scenarios are highly irregular and potentially dangerous so we prevent it by
22-
// instructing cdk-assets to detect foreign bucket ownership and reject.
21+
// If there is a staging bucket AND the bootstrap version is old, then we want to protect
22+
// against accidental cross-account publishing.
2323
return false;
2424
} catch (e) {
25+
// You would think we would need to fail closed here, but the reality is
26+
// that we get here if we couldn't find the bootstrap stack: that is
27+
// completely valid, and many large organizations may have their own method
28+
// of creating bootstrap resources. If they do, there's nothing for us to validate,
29+
// but we can't use that as a reason to disallow cross-account publishing. We'll just
30+
// have to trust they did their due diligence. So we fail open.
2531
debug(`Error determining cross account asset publishing: ${e}`);
26-
debug('Defaulting to disallowing cross account asset publishing');
27-
return false;
32+
debug('Defaulting to allowing cross account asset publishing');
33+
return true;
2834
}
2935
}
3036

@@ -54,15 +60,16 @@ export async function getBootstrapStackInfo(sdk: ISDK, stackName: string): Promi
5460
throw new Error(`Invalid BootstrapVersion value: ${versionOutput.OutputValue}`);
5561
}
5662

57-
// try to get bucketname from the logical resource id
58-
let bucketName: string | undefined;
59-
const resourcesResponse = await cfn.describeStackResources({ StackName: stackName }).promise();
60-
const bucketResource = resourcesResponse.StackResources?.find(resource =>
61-
resource.ResourceType === 'AWS::S3::Bucket',
62-
);
63-
bucketName = bucketResource?.PhysicalResourceId;
64-
65-
let hasStagingBucket = !!bucketName;
63+
// try to get bucketname from the logical resource id. If there is no
64+
// bucketname, or the value doesn't look like an S3 bucket name, we assume
65+
// the bucket doesn't exist (this is for the case where a template customizer did
66+
// not dare to remove the Output, but put a dummy value there like '' or '-' or '***').
67+
//
68+
// We would have preferred to look at the stack resources here, but
69+
// unfortunately the deploy role doesn't have permissions call DescribeStackResources.
70+
const bucketName = stack.Outputs?.find(output => output.OutputKey === 'BucketName')?.OutputValue;
71+
// Must begin and end with letter or number.
72+
const hasStagingBucket = !!(bucketName && bucketName.match(/^[a-z0-9]/) && bucketName.match(/[a-z0-9]$/));
6673

6774
return {
6875
hasStagingBucket,

‎packages/aws-cdk/test/api/util/checks.test.ts‎

Lines changed: 42 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,20 @@ describe('determineAllowCrossAccountAssetPublishing', () => {
2525
});
2626
});
2727

28-
AWSMock.mock('CloudFormation', 'describeStackResources', (_params: any, callback: Function) => {
29-
callback(null, { StackResources: [] });
28+
const result = await determineAllowCrossAccountAssetPublishing(mockSDK);
29+
expect(result).toBe(true);
30+
});
31+
32+
it.each(['', '-', '*', '---'])('should return true when the bucket output does not look like a real bucket', async (notABucketName) => {
33+
AWSMock.mock('CloudFormation', 'describeStacks', (_params: any, callback: Function) => {
34+
callback(null, {
35+
Stacks: [{
36+
Outputs: [
37+
{ OutputKey: 'BootstrapVersion', OutputValue: '1' },
38+
{ OutputKey: 'BucketName', OutputValue: notABucketName },
39+
],
40+
}],
41+
});
3042
});
3143

3244
const result = await determineAllowCrossAccountAssetPublishing(mockSDK);
@@ -37,13 +49,30 @@ describe('determineAllowCrossAccountAssetPublishing', () => {
3749
AWSMock.mock('CloudFormation', 'describeStacks', (_params: any, callback: Function) => {
3850
callback(null, {
3951
Stacks: [{
40-
Outputs: [{ OutputKey: 'BootstrapVersion', OutputValue: '21' }],
52+
Outputs: [
53+
{ OutputKey: 'BootstrapVersion', OutputValue: '21' },
54+
{ OutputKey: 'BucketName', OutputValue: 'some-bucket' },
55+
],
4156
}],
4257
});
4358
});
4459

45-
AWSMock.mock('CloudFormation', 'describeStackResources', (_params: any, callback: Function) => {
46-
callback(null, { StackResources: [{ ResourceType: 'AWS::S3::Bucket', PhysicalResourceId: 'some-bucket' }] });
60+
const result = await determineAllowCrossAccountAssetPublishing(mockSDK);
61+
expect(result).toBe(true);
62+
});
63+
64+
it('should return true if looking up the bootstrap stack fails', async () => {
65+
AWSMock.mock('CloudFormation', 'describeStacks', (_params: any, callback: Function) => {
66+
callback(new Error('Could not read bootstrap stack'));
67+
});
68+
69+
const result = await determineAllowCrossAccountAssetPublishing(mockSDK);
70+
expect(result).toBe(true);
71+
});
72+
73+
it('should return true if looking up the bootstrap stack fails', async () => {
74+
AWSMock.mock('CloudFormation', 'describeStacks', (_params: any, callback: Function) => {
75+
callback(new Error('Could not read bootstrap stack'));
4776
});
4877

4978
const result = await determineAllowCrossAccountAssetPublishing(mockSDK);
@@ -54,15 +83,14 @@ describe('determineAllowCrossAccountAssetPublishing', () => {
5483
AWSMock.mock('CloudFormation', 'describeStacks', (_params: any, callback: Function) => {
5584
callback(null, {
5685
Stacks: [{
57-
Outputs: [{ OutputKey: 'BootstrapVersion', OutputValue: '20' }],
86+
Outputs: [
87+
{ OutputKey: 'BootstrapVersion', OutputValue: '20' },
88+
{ OutputKey: 'BucketName', OutputValue: 'some-bucket' },
89+
],
5890
}],
5991
});
6092
});
6193

62-
AWSMock.mock('CloudFormation', 'describeStackResources', (_params: any, callback: Function) => {
63-
callback(null, { StackResources: [{ ResourceType: 'AWS::S3::Bucket', PhysicalResourceId: 'some-bucket' }] });
64-
});
65-
6694
const result = await determineAllowCrossAccountAssetPublishing(mockSDK);
6795
expect(result).toBe(false);
6896
});
@@ -85,15 +113,14 @@ describe('getBootstrapStackInfo', () => {
85113
AWSMock.mock('CloudFormation', 'describeStacks', (_params: any, callback: Function) => {
86114
callback(null, {
87115
Stacks: [{
88-
Outputs: [{ OutputKey: 'BootstrapVersion', OutputValue: '21' }],
116+
Outputs: [
117+
{ OutputKey: 'BootstrapVersion', OutputValue: '21' },
118+
{ OutputKey: 'BucketName', OutputValue: 'some-bucket' },
119+
],
89120
}],
90121
});
91122
});
92123

93-
AWSMock.mock('CloudFormation', 'describeStackResources', (_params: any, callback: Function) => {
94-
callback(null, { StackResources: [{ ResourceType: 'AWS::S3::Bucket', PhysicalResourceId: 'some-bucket' }] });
95-
});
96-
97124
const result = await getBootstrapStackInfo(mockSDK, 'CDKToolkit');
98125
expect(result).toEqual({
99126
hasStagingBucket: true,

0 commit comments

Comments
 (0)