Skip to content

Commit

Permalink
fix(cli): asset uploads fail if Object Lock is enabled on access buck…
Browse files Browse the repository at this point in the history
…et (#31937)

Object Lock requires passing an object checksum. By default, SDKv2 only calculates MD5 checksums.

We used to turn off checksums altogether and rely on SigV4 checksums to produce a workable setup for both FIPS and non-FIPS users, but in case of Object Lock this doesn't work: we must definitely have an S3 content checksum, and the the SigV4 checksum alone is not good enough.

Since SDKv2 only supports MD5 checksums, we now only disable checksums for FIPS environments.

The unfortunate result is that Object Lock will not work in a FIPS environment, but there's no way around that for now.

When we migrate to SDKv3, which can be configured to checksum using SHA256, Object Lock + FIPS will work again.

Relates to #31926

(This PR also adds tests for the PluginHost because otherwise the build fails due to coverage requirements)

----

*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 Nov 1, 2024
1 parent 4e715b8 commit ab1e91d
Show file tree
Hide file tree
Showing 8 changed files with 171 additions and 18 deletions.
3 changes: 2 additions & 1 deletion packages/@aws-cdk-testing/cli-integ/lib/aws.ts
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ export class AwsClients {
}
}

public async emptyBucket(bucketName: string) {
public async emptyBucket(bucketName: string, options?: { bypassGovernance?: boolean }) {
const objects = await this.s3.send(
new ListObjectVersionsCommand({
Bucket: bucketName,
Expand All @@ -154,6 +154,7 @@ export class AwsClients {
Objects: deletes,
Quiet: false,
},
BypassGovernanceRetention: options?.bypassGovernance ? true : undefined,
}),
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
PutRolePolicyCommand,
} from '@aws-sdk/client-iam';
import { InvokeCommand } from '@aws-sdk/client-lambda';
import { PutObjectLockConfigurationCommand } from '@aws-sdk/client-s3';
import { CreateTopicCommand, DeleteTopicCommand } from '@aws-sdk/client-sns';
import { AssumeRoleCommand, GetCallerIdentityCommand } from '@aws-sdk/client-sts';
import {
Expand Down Expand Up @@ -1318,6 +1319,43 @@ integTest(
}),
);

integTest('deploy stack with Lambda Asset to Object Lock-enabled asset bucket', withoutBootstrap(async (fixture) => {
// Bootstrapping with custom toolkit stack name and qualifier
const qualifier = fixture.qualifier;
const toolkitStackName = fixture.bootstrapStackName;
await fixture.cdkBootstrapModern({
verbose: true,
toolkitStackName: toolkitStackName,
qualifier: qualifier,
});

const bucketName = `cdk-${qualifier}-assets-${await fixture.aws.account()}-${fixture.aws.region}`;
await fixture.aws.s3.send(new PutObjectLockConfigurationCommand({
Bucket: bucketName,
ObjectLockConfiguration: {
ObjectLockEnabled: 'Enabled',
Rule: {
DefaultRetention: {
Days: 1,
Mode: 'GOVERNANCE',
},
},
},
}));

// Deploy a stack that definitely contains a file asset
await fixture.cdkDeploy('lambda', {
options: [
'--toolkit-stack-name', toolkitStackName,
'--context', `@aws-cdk/core:bootstrapQualifier=${qualifier}`,
],
});

// THEN - should not fail. Now clean the bucket with governance bypass: a regular delete
// operation will fail.
await fixture.aws.emptyBucket(bucketName, { bypassGovernance: true });
}));

integTest(
'cdk ls',
withDefaultFixture(async (fixture) => {
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/integ-runner/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@
"@aws-cdk/cloud-assembly-schema": "^38.0.1",
"@aws-cdk/cloudformation-diff": "0.0.0",
"@aws-cdk/cx-api": "0.0.0",
"cdk-assets": "^2.155.17",
"cdk-assets": "^2.155.20",
"@aws-cdk/aws-service-spec": "^0.1.30",
"@aws-cdk/cdk-cli-wrapper": "0.0.0",
"aws-cdk": "0.0.0",
Expand Down
19 changes: 16 additions & 3 deletions packages/aws-cdk/lib/api/aws-auth/sdk.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as crypto from 'crypto';
import * as AWS from 'aws-sdk';
import type { ConfigurationOptions } from 'aws-sdk/lib/config-base';
import { debug, trace } from './_env';
Expand Down Expand Up @@ -188,14 +189,26 @@ export class SDK implements ISDK {
}: S3ClientOptions = {}): AWS.S3 {
const config = { ...this.config };

if (!apiRequiresMd5Checksum) {
if (crypto.getFips() && apiRequiresMd5Checksum) {
// This should disappear for SDKv3; in SDKv3, we can always force the client to use SHA256 checksums
throw new Error('This operation requires MD5 for integrity purposes; unfortunately, it therefore is not available in FIPS enabled environments.');
}

if (crypto.getFips()) {
// In FIPS enabled environments, the MD5 algorithm is not available for use in crypto module.
// However by default the S3 client is using an MD5 checksum for content integrity checking.
// While this usage is technically allowed in FIPS (MD5 is only prohibited for cryptographic use),
// in practice it is just easier to use an allowed checksum mechanism.
// We are disabling the S3 content checksums, and are re-enabling the regular SigV4 body signing.
// SigV4 uses SHA256 for their content checksum. This configuration matches the default behavior
// of the AWS SDKv3 and is a safe choice for all users, except in the above APIs.
// SigV4 uses SHA256 for their content checksum.
//
// As far as we know, this configuration will work for most APIs except:
// - DeleteObjects (note the plural)
// - PutObject to a bucket with Object Lock enabled.
//
// These APIs refuse to work without a content checksum at the S3 level (a SigV4 checksum is not
// good enough). There is no way to get those to work with SHA256 in the SDKv2, but this limitation
// will be alleviated once we migrate to SDKv3.
config.s3DisableBodySigning = false;
config.computeChecksums = false;
}
Expand Down
12 changes: 9 additions & 3 deletions packages/aws-cdk/lib/api/plugin/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@ import { error } from './_env';
import { ContextProviderPlugin, isContextProviderPlugin } from './context-provider-plugin';
import { CredentialProviderSource } from './credential-provider-source';

export let TESTING = false;

export function markTesting() {
TESTING = true;
}

/**
* The basic contract for plug-ins to adhere to::
*
Expand Down Expand Up @@ -52,7 +58,7 @@ export class PluginHost {
public readonly contextProviderPlugins: Record<string, ContextProviderPlugin> = {};

constructor() {
if (PluginHost.instance && PluginHost.instance !== this) {
if (!TESTING && PluginHost.instance && PluginHost.instance !== this) {
throw new Error('New instances of PluginHost must not be built. Use PluginHost.instance instead!');
}
}
Expand All @@ -71,10 +77,10 @@ export class PluginHost {
error(`Module ${chalk.green(moduleSpec)} is not a valid plug-in, or has an unsupported version.`);
throw new Error(`Module ${moduleSpec} does not define a valid plug-in.`);
}
if (plugin.init) { plugin.init(PluginHost.instance); }
if (plugin.init) { plugin.init(this); }
} catch (e: any) {
error(`Unable to load ${chalk.green(moduleSpec)}: ${e.stack}`);
throw new Error(`Unable to load plug-in: ${moduleSpec}`);
throw new Error(`Unable to load plug-in: ${moduleSpec}: ${e}`);
}

function isPlugin(x: any): x is Plugin {
Expand Down
2 changes: 1 addition & 1 deletion packages/aws-cdk/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@
"archiver": "^5.3.2",
"aws-sdk": "^2.1691.0",
"camelcase": "^6.3.0",
"cdk-assets": "^2.155.17",
"cdk-assets": "^2.155.20",
"cdk-from-cfn": "^0.162.0",
"chalk": "^4",
"chokidar": "^3.6.0",
Expand Down
95 changes: 95 additions & 0 deletions packages/aws-cdk/test/api/plugin/plugin-host.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
import { ContextProviderPlugin } from '../../../lib/api/plugin/context-provider-plugin';
import { CredentialProviderSource } from '../../../lib/api/plugin/credential-provider-source';
import { PluginHost, markTesting } from '../../../lib/api/plugin/plugin';

markTesting();

beforeEach(() => {
jest.resetModules();
});

const THE_PLUGIN = 'the-plugin';

test('load a plugin using the PluginHost', () => {
const host = new PluginHost();

jest.mock(THE_PLUGIN, () => {
return {
version: '1',
init() {
},
};
}, { virtual: true });

host.load(THE_PLUGIN);
});

test('fail to load a plugin using the PluginHost', () => {
const host = new PluginHost();

// This is not a plugin
jest.mock(THE_PLUGIN, () => {
return {};
}, { virtual: true });

expect(() => host.load(THE_PLUGIN)).toThrow(/Unable to load plug-in/);
});

test('plugin that registers a Credential Provider', () => {
const host = new PluginHost();

jest.mock(THE_PLUGIN, () => {
return {
version: '1',
init(h: PluginHost) {
h.registerCredentialProviderSource({
canProvideCredentials() { return Promise.resolve(false); },
name: 'test',
isAvailable() { return Promise.resolve(false); },
getProvider() { return Promise.reject('Dont call me'); },
} satisfies CredentialProviderSource);

},
};
}, { virtual: true });

host.load(THE_PLUGIN);

expect(host.credentialProviderSources).toHaveLength(1);
});

test('plugin that registers a Context Provider', () => {
const host = new PluginHost();

jest.mock(THE_PLUGIN, () => {
return {
version: '1',
init(h: PluginHost) {
h.registerContextProviderAlpha('name', {
getValue(_args: Record<string, any>) {
return Promise.resolve('asdf');
},
} satisfies ContextProviderPlugin);
},
};
}, { virtual: true });

host.load(THE_PLUGIN);

expect(Object.keys(host.contextProviderPlugins)).toHaveLength(1);
});

test('plugin that registers an invalid Context Provider throws', () => {
const host = new PluginHost();

jest.mock(THE_PLUGIN, () => {
return {
version: '1',
init(h: PluginHost) {
h.registerContextProviderAlpha('name', {} as any);
},
};
}, { virtual: true });

expect(() => host.load(THE_PLUGIN)).toThrow(/does not look like a ContextProviderPlugin/);
});
18 changes: 9 additions & 9 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,10 @@
jsonschema "^1.4.1"
semver "^7.6.3"

"@aws-cdk/cx-api@^2.163.1":
version "2.163.1"
resolved "https://registry.npmjs.org/@aws-cdk/cx-api/-/cx-api-2.163.1.tgz#ef55da9f471c963d877b23d3201ca4560d656b2e"
integrity sha512-0bVL/pX0UcliCdXVcgtLVL3W5EHAp4RgW7JN3prz1dIOmLZzZ30DW0qWSc0D0EVE3rVG6RVgfIiuFBFK6WFZ+w==
"@aws-cdk/cx-api@^2.164.1":
version "2.164.1"
resolved "https://registry.npmjs.org/@aws-cdk/cx-api/-/cx-api-2.164.1.tgz#dce8eaede6b9ec95c4a69f7acbe486b499c32516"
integrity sha512-VwYDcI8b5KYS2VptkIAm75yK1SwLAClFnlyH0Ea5dI3YJrIYtvxW930nhppxmwPihbMJa4Z0sxic7EBTt4ZaBQ==
dependencies:
semver "^7.6.3"

Expand Down Expand Up @@ -6646,13 +6646,13 @@ [email protected], case@^1.6.3:
resolved "https://registry.npmjs.org/case/-/case-1.6.3.tgz#0a4386e3e9825351ca2e6216c60467ff5f1ea1c9"
integrity sha512-mzDSXIPaFwVDvZAHqZ9VlbyF4yyXRuX6IvB06WvPYkqJVO24kX1PPhv9bfpKNFZyxYFmmgo03HUiD8iklmJYRQ==

cdk-assets@^2.155.17:
version "2.155.17"
resolved "https://registry.npmjs.org/cdk-assets/-/cdk-assets-2.155.17.tgz#d6c285d0279aec8226b45577a151e6dd32a12fa5"
integrity sha512-+hJlYYlsPHhPCeMC/V3pMyrjz5K8p9SQdC50qMg6a8/w/3w0WY1ZixyKGtpJfFB11C3Ubb04l2miieaAH00CIA==
cdk-assets@^2.155.20:
version "2.155.20"
resolved "https://registry.npmjs.org/cdk-assets/-/cdk-assets-2.155.20.tgz#a7a380f820001d2087d0dce802eac4c71a688100"
integrity sha512-NXU7RCJsPecQbRVkQ6iPyOV3jDEojENaxWs9956pYddY5Pq0onSibXItivavQC74i0YZdyWDdlH6RcLPzFQhPQ==
dependencies:
"@aws-cdk/cloud-assembly-schema" "^38.0.1"
"@aws-cdk/cx-api" "^2.163.1"
"@aws-cdk/cx-api" "^2.164.1"
archiver "^5.3.2"
aws-sdk "^2.1691.0"
glob "^7.2.3"
Expand Down

0 comments on commit ab1e91d

Please sign in to comment.