diff --git a/config.json b/config.json index 6846ead615..502b9915dc 100644 --- a/config.json +++ b/config.json @@ -153,7 +153,8 @@ "bucketPutPolicy": 20480 }, "integrityChecks": { - "enabled": true + "enabled": true, + "storeCompositeChecksums": true }, "serverAccessLogs": { "mode": "DISABLED", diff --git a/lib/Config.js b/lib/Config.js index f6ec35a79c..d283300050 100644 --- a/lib/Config.js +++ b/lib/Config.js @@ -605,36 +605,35 @@ function parseServerAccessLogs(config) { * so CompleteMultipartUpload cannot compose a final checksum for that object: * it completes the upload without one and logs an error naming the parts. * + * `storeCompositeChecksums` if false COMPOSITE checksums are not stored in the final object metadata + * and also no per part checksum is stored. + * * @param {object} config - raw parsed config file contents - * @return {{enabled: boolean}} the parsed integrityChecks section + * @return {{enabled: boolean, storeCompositeChecksums: boolean}} the parsed integrityChecks section */ function parseIntegrityChecks(config) { - const res = { enabled: true }; - - if (config && config.integrityChecks) { - assert( - typeof config.integrityChecks === 'object' && !Array.isArray(config.integrityChecks), - 'bad config: integrityChecks must be an object', - ); + const joiSchema = joi.object({ + enabled: joi.boolean().strict().default(true), + storeCompositeChecksums: joi.boolean().strict().default(true), + }); + const section = (config && config.integrityChecks) || {}; - if ('enabled' in config.integrityChecks) { - assert( - typeof config.integrityChecks.enabled === 'boolean', - 'bad config: integrityChecks.enabled must be a boolean', - ); - res.enabled = config.integrityChecks.enabled; - } - } + // stripUnknown: legacy per-API keys (CLDSRV-943) may still be in deployed + // configs, and joi rejects unknown keys by default. Ignore, do not reject. + const res = joi.attempt(section, joiSchema, 'bad config', { stripUnknown: true }); - if (process.env.S3_INTEGRITY_CHECKS_ENABLED !== undefined) { - assert( - ['true', 'false'].includes(process.env.S3_INTEGRITY_CHECKS_ENABLED), - "bad config: S3_INTEGRITY_CHECKS_ENABLED must be 'true' or 'false'", - ); - res.enabled = process.env.S3_INTEGRITY_CHECKS_ENABLED === 'true'; - } + const envSchema = joi + .object({ + S3_INTEGRITY_CHECKS_ENABLED: joi.boolean().default(res.enabled), + S3_INTEGRITY_CHECKS_STORE_COMPOSITE_CHECKSUMS: joi.boolean().default(res.storeCompositeChecksums), + }) + .unknown(true); + const env = joi.attempt(process.env, envSchema, 'bad config'); - return res; + return { + enabled: env.S3_INTEGRITY_CHECKS_ENABLED, + storeCompositeChecksums: env.S3_INTEGRITY_CHECKS_STORE_COMPOSITE_CHECKSUMS, + }; } /** diff --git a/tests/unit/Config.js b/tests/unit/Config.js index a03508a187..28b3e90a8e 100644 --- a/tests/unit/Config.js +++ b/tests/unit/Config.js @@ -21,7 +21,7 @@ describe('Config', () => { const defaultConfig = JSON.parse(fs.readFileSync('config.json'), { encoding: 'utf-8' }); const envToRestore = []; - const setEnv = (key, value) => { + const recordEnv = key => { if (key in process.env) { const v = process.env[key]; envToRestore.push(() => { @@ -32,8 +32,15 @@ describe('Config', () => { delete process.env[key]; }); } + }; + const setEnv = (key, value) => { + recordEnv(key); process.env[key] = value; }; + const deleteEnv = key => { + recordEnv(key); + delete process.env[key]; + }; beforeEach(() => { envToRestore.length = 0; @@ -931,23 +938,33 @@ describe('Config', () => { }); describe('parse integrity checks', () => { - afterEach(() => { - delete process.env.S3_INTEGRITY_CHECKS_ENABLED; + // CI exports S3_INTEGRITY_CHECKS_ENABLED for some jobs, so clear both + // vars rather than assert around whatever the environment inherited. + // The suite-level afterEach puts them back. + beforeEach(() => { + deleteEnv('S3_INTEGRITY_CHECKS_ENABLED'); + deleteEnv('S3_INTEGRITY_CHECKS_STORE_COMPOSITE_CHECKSUMS'); }); it('should default to enabled when not configured', () => { - assert.deepStrictEqual(parseIntegrityChecks(null), { enabled: true }); - assert.deepStrictEqual(parseIntegrityChecks({}), { enabled: true }); + assert.deepStrictEqual(parseIntegrityChecks(null), { enabled: true, storeCompositeChecksums: true }); + assert.deepStrictEqual(parseIntegrityChecks({}), { enabled: true, storeCompositeChecksums: true }); }); it('should read the configured value', () => { - assert.deepStrictEqual(parseIntegrityChecks({ integrityChecks: { enabled: false } }), { enabled: false }); - assert.deepStrictEqual(parseIntegrityChecks({ integrityChecks: { enabled: true } }), { enabled: true }); + assert.deepStrictEqual(parseIntegrityChecks({ integrityChecks: { enabled: false } }), { + enabled: false, + storeCompositeChecksums: true, + }); + assert.deepStrictEqual(parseIntegrityChecks({ integrityChecks: { enabled: true } }), { + enabled: true, + storeCompositeChecksums: true, + }); }); it('should throw if integrityChecks is not an object', () => { - assert.throws(() => parseIntegrityChecks({ integrityChecks: 'yes' }), /must be an object/); - assert.throws(() => parseIntegrityChecks({ integrityChecks: [true] }), /must be an object/); + assert.throws(() => parseIntegrityChecks({ integrityChecks: 'yes' }), /must be of type object/); + assert.throws(() => parseIntegrityChecks({ integrityChecks: [true] }), /must be of type object/); }); it('should throw if enabled is not a boolean', () => { @@ -955,21 +972,99 @@ describe('Config', () => { assert.throws(() => parseIntegrityChecks({ integrityChecks: { enabled: 0 } }), /must be a boolean/); }); + it('should ignore unknown keys left over from older config files', () => { + assert.deepStrictEqual( + parseIntegrityChecks({ integrityChecks: { objectPutRetention: true, bucketPutACL: false } }), + { enabled: true, storeCompositeChecksums: true }, + ); + assert.deepStrictEqual( + parseIntegrityChecks({ integrityChecks: { enabled: false, objectPutRetention: true } }), + { enabled: false, storeCompositeChecksums: true }, + ); + }); + it('should let S3_INTEGRITY_CHECKS_ENABLED override the config file', () => { - process.env.S3_INTEGRITY_CHECKS_ENABLED = 'false'; - assert.deepStrictEqual(parseIntegrityChecks({ integrityChecks: { enabled: true } }), { enabled: false }); - process.env.S3_INTEGRITY_CHECKS_ENABLED = 'true'; - assert.deepStrictEqual(parseIntegrityChecks({ integrityChecks: { enabled: false } }), { enabled: true }); + setEnv('S3_INTEGRITY_CHECKS_ENABLED', 'false'); + assert.deepStrictEqual(parseIntegrityChecks({ integrityChecks: { enabled: true } }), { + enabled: false, + storeCompositeChecksums: true, + }); + setEnv('S3_INTEGRITY_CHECKS_ENABLED', 'true'); + assert.deepStrictEqual(parseIntegrityChecks({ integrityChecks: { enabled: false } }), { + enabled: true, + storeCompositeChecksums: true, + }); }); it('should throw on a non-boolean S3_INTEGRITY_CHECKS_ENABLED', () => { - process.env.S3_INTEGRITY_CHECKS_ENABLED = 'nope'; - assert.throws(() => parseIntegrityChecks(null), /S3_INTEGRITY_CHECKS_ENABLED/); + setEnv('S3_INTEGRITY_CHECKS_ENABLED', 'nope'); + assert.throws(() => parseIntegrityChecks(null), /"S3_INTEGRITY_CHECKS_ENABLED" must be a boolean/); + setEnv('S3_INTEGRITY_CHECKS_ENABLED', '1'); + assert.throws(() => parseIntegrityChecks(null), /"S3_INTEGRITY_CHECKS_ENABLED" must be a boolean/); + }); + + it('should default storeCompositeChecksums to true when not configured', () => { + assert.strictEqual(parseIntegrityChecks(null).storeCompositeChecksums, true); + assert.strictEqual(parseIntegrityChecks({ integrityChecks: {} }).storeCompositeChecksums, true); + }); + + it('should read the configured storeCompositeChecksums value', () => { + assert.deepStrictEqual(parseIntegrityChecks({ integrityChecks: { storeCompositeChecksums: false } }), { + enabled: true, + storeCompositeChecksums: false, + }); + assert.deepStrictEqual(parseIntegrityChecks({ integrityChecks: { storeCompositeChecksums: true } }), { + enabled: true, + storeCompositeChecksums: true, + }); + }); + + it('should parse enabled and storeCompositeChecksums independently', () => { + assert.deepStrictEqual( + parseIntegrityChecks({ integrityChecks: { enabled: false, storeCompositeChecksums: true } }), + { enabled: false, storeCompositeChecksums: true }, + ); + assert.deepStrictEqual( + parseIntegrityChecks({ integrityChecks: { enabled: true, storeCompositeChecksums: false } }), + { enabled: true, storeCompositeChecksums: false }, + ); + }); + + it('should throw if storeCompositeChecksums is not a boolean', () => { + assert.throws( + () => parseIntegrityChecks({ integrityChecks: { storeCompositeChecksums: 'false' } }), + /"storeCompositeChecksums" must be a boolean/, + ); + assert.throws( + () => parseIntegrityChecks({ integrityChecks: { storeCompositeChecksums: 0 } }), + /"storeCompositeChecksums" must be a boolean/, + ); + }); + + it('should let S3_INTEGRITY_CHECKS_STORE_COMPOSITE_CHECKSUMS override the config file', () => { + setEnv('S3_INTEGRITY_CHECKS_STORE_COMPOSITE_CHECKSUMS', 'false'); + assert.deepStrictEqual(parseIntegrityChecks({ integrityChecks: { storeCompositeChecksums: true } }), { + enabled: true, + storeCompositeChecksums: false, + }); + setEnv('S3_INTEGRITY_CHECKS_STORE_COMPOSITE_CHECKSUMS', 'true'); + assert.deepStrictEqual(parseIntegrityChecks({ integrityChecks: { storeCompositeChecksums: false } }), { + enabled: true, + storeCompositeChecksums: true, + }); + }); + + it('should throw on a non-boolean S3_INTEGRITY_CHECKS_STORE_COMPOSITE_CHECKSUMS', () => { + setEnv('S3_INTEGRITY_CHECKS_STORE_COMPOSITE_CHECKSUMS', 'nope'); + assert.throws( + () => parseIntegrityChecks(null), + /"S3_INTEGRITY_CHECKS_STORE_COMPOSITE_CHECKSUMS" must be a boolean/, + ); }); it('should expose integrityChecks on the config object', () => { const config = new ConfigObject(); - assert.deepStrictEqual(config.integrityChecks, { enabled: true }); + assert.deepStrictEqual(config.integrityChecks, { enabled: true, storeCompositeChecksums: true }); }); });