Skip to content

Commit

Permalink
fix(cli): cross-account asset publishing doesn't work without bootstr…
Browse files Browse the repository at this point in the history
…ap stack (#31876)

If the bootstrap stack can't be found, it can't be validated. We used to fail closed, but that just means that cross-account publishing is broken.

Instead, we have to fail open.

This is not the only protection mechanism we have, so the local check is more of a bonus.

Fixes #31866.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
rix0rrr authored Oct 24, 2024
1 parent d884c7c commit 427bf63
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 4 deletions.
14 changes: 10 additions & 4 deletions packages/aws-cdk/lib/api/util/checks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,19 @@ export async function determineAllowCrossAccountAssetPublishing(sdk: ISDK, custo
return true;
}

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

Expand Down
9 changes: 9 additions & 0 deletions packages/aws-cdk/test/api/util/checks.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,15 @@ describe('determineAllowCrossAccountAssetPublishing', () => {
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);
expect(result).toBe(true);
});

it('should return false for other scenarios', async () => {
AWSMock.mock('CloudFormation', 'describeStacks', (_params: any, callback: Function) => {
callback(null, {
Expand Down

0 comments on commit 427bf63

Please sign in to comment.