From 8d06824298d80b18c6b0143a9ac38b79ea5d6253 Mon Sep 17 00:00:00 2001 From: Rico Hermans Date: Thu, 24 Oct 2024 13:20:50 +0200 Subject: [PATCH] fix(cli): deploy-role is not authorized to perform DescribeStackResources (#31878) The cross-account asset uploading detection check required that the `deploy-role` could call `DescribeStackResources` -- which it can't. Instead, rely on parsing the `Outputs` of `DescribeStacks`. This is equivalent for the built-in stack, and relies on stack customizers to have removed the Output or put a dummy value there that does not look like a stack name (like `''`, `'-'` or `'*'`). It's not *as* good, but probably good enough. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- packages/aws-cdk/lib/api/util/checks.ts | 19 ++++---- packages/aws-cdk/test/api/util/checks.test.ts | 48 +++++++++++++------ 2 files changed, 43 insertions(+), 24 deletions(-) diff --git a/packages/aws-cdk/lib/api/util/checks.ts b/packages/aws-cdk/lib/api/util/checks.ts index a094156ba74fb..96fe2a3bc549f 100644 --- a/packages/aws-cdk/lib/api/util/checks.ts +++ b/packages/aws-cdk/lib/api/util/checks.ts @@ -60,15 +60,16 @@ export async function getBootstrapStackInfo(sdk: ISDK, stackName: string): Promi throw new Error(`Invalid BootstrapVersion value: ${versionOutput.OutputValue}`); } - // try to get bucketname from the logical resource id - let bucketName: string | undefined; - const resourcesResponse = await cfn.describeStackResources({ StackName: stackName }).promise(); - const bucketResource = resourcesResponse.StackResources?.find(resource => - resource.ResourceType === 'AWS::S3::Bucket', - ); - bucketName = bucketResource?.PhysicalResourceId; - - let hasStagingBucket = !!bucketName; + // try to get bucketname from the logical resource id. If there is no + // bucketname, or the value doesn't look like an S3 bucket name, we assume + // the bucket doesn't exist (this is for the case where a template customizer did + // not dare to remove the Output, but put a dummy value there like '' or '-' or '***'). + // + // We would have preferred to look at the stack resources here, but + // unfortunately the deploy role doesn't have permissions call DescribeStackResources. + const bucketName = stack.Outputs?.find(output => output.OutputKey === 'BucketName')?.OutputValue; + // Must begin and end with letter or number. + const hasStagingBucket = !!(bucketName && bucketName.match(/^[a-z0-9]/) && bucketName.match(/[a-z0-9]$/)); return { hasStagingBucket, diff --git a/packages/aws-cdk/test/api/util/checks.test.ts b/packages/aws-cdk/test/api/util/checks.test.ts index 660577a8f6aec..697cbced9254b 100644 --- a/packages/aws-cdk/test/api/util/checks.test.ts +++ b/packages/aws-cdk/test/api/util/checks.test.ts @@ -25,8 +25,20 @@ describe('determineAllowCrossAccountAssetPublishing', () => { }); }); - AWSMock.mock('CloudFormation', 'describeStackResources', (_params: any, callback: Function) => { - callback(null, { StackResources: [] }); + const result = await determineAllowCrossAccountAssetPublishing(mockSDK); + expect(result).toBe(true); + }); + + it.each(['', '-', '*', '---'])('should return true when the bucket output does not look like a real bucket', async (notABucketName) => { + AWSMock.mock('CloudFormation', 'describeStacks', (_params: any, callback: Function) => { + callback(null, { + Stacks: [{ + Outputs: [ + { OutputKey: 'BootstrapVersion', OutputValue: '1' }, + { OutputKey: 'BucketName', OutputValue: notABucketName }, + ], + }], + }); }); const result = await determineAllowCrossAccountAssetPublishing(mockSDK); @@ -37,13 +49,21 @@ describe('determineAllowCrossAccountAssetPublishing', () => { AWSMock.mock('CloudFormation', 'describeStacks', (_params: any, callback: Function) => { callback(null, { Stacks: [{ - Outputs: [{ OutputKey: 'BootstrapVersion', OutputValue: '21' }], + Outputs: [ + { OutputKey: 'BootstrapVersion', OutputValue: '21' }, + { OutputKey: 'BucketName', OutputValue: 'some-bucket' }, + ], }], }); }); - AWSMock.mock('CloudFormation', 'describeStackResources', (_params: any, callback: Function) => { - callback(null, { StackResources: [{ ResourceType: 'AWS::S3::Bucket', PhysicalResourceId: 'some-bucket' }] }); + const result = await determineAllowCrossAccountAssetPublishing(mockSDK); + expect(result).toBe(true); + }); + + it('should return true if looking up the bootstrap stack fails', async () => { + AWSMock.mock('CloudFormation', 'describeStacks', (_params: any, callback: Function) => { + callback(new Error('Could not read bootstrap stack')); }); const result = await determineAllowCrossAccountAssetPublishing(mockSDK); @@ -63,15 +83,14 @@ describe('determineAllowCrossAccountAssetPublishing', () => { AWSMock.mock('CloudFormation', 'describeStacks', (_params: any, callback: Function) => { callback(null, { Stacks: [{ - Outputs: [{ OutputKey: 'BootstrapVersion', OutputValue: '20' }], + Outputs: [ + { OutputKey: 'BootstrapVersion', OutputValue: '20' }, + { OutputKey: 'BucketName', OutputValue: 'some-bucket' }, + ], }], }); }); - AWSMock.mock('CloudFormation', 'describeStackResources', (_params: any, callback: Function) => { - callback(null, { StackResources: [{ ResourceType: 'AWS::S3::Bucket', PhysicalResourceId: 'some-bucket' }] }); - }); - const result = await determineAllowCrossAccountAssetPublishing(mockSDK); expect(result).toBe(false); }); @@ -94,15 +113,14 @@ describe('getBootstrapStackInfo', () => { AWSMock.mock('CloudFormation', 'describeStacks', (_params: any, callback: Function) => { callback(null, { Stacks: [{ - Outputs: [{ OutputKey: 'BootstrapVersion', OutputValue: '21' }], + Outputs: [ + { OutputKey: 'BootstrapVersion', OutputValue: '21' }, + { OutputKey: 'BucketName', OutputValue: 'some-bucket' }, + ], }], }); }); - AWSMock.mock('CloudFormation', 'describeStackResources', (_params: any, callback: Function) => { - callback(null, { StackResources: [{ ResourceType: 'AWS::S3::Bucket', PhysicalResourceId: 'some-bucket' }] }); - }); - const result = await getBootstrapStackInfo(mockSDK, 'CDKToolkit'); expect(result).toEqual({ hasStagingBucket: true,