CLDSRV-898: reject CompleteMPU if explicit checksum but checksum missing in part metadata

Author: leif-scality

lib/api/completeMultipartUpload.js

@@ -126,26 +126,44 @@ function validatePerPartChecksums(jsonList, storedParts, mpuSplitter, mpuChecksu
 
 /**
  * Compute the final-object checksum for a CompleteMultipartUpload from the
- * stored MPU configuration and per-part checksums. Returns null when the MPU
- * has no checksum configured, when any part is missing its stored
- * ChecksumValue (defensive — should not happen in steady state), or when the
- * compute primitive itself reports an error. Errors are logged; the caller
- * proceeds without a final-object checksum so the response simply omits the
- * checksum fields rather than failing the whole CompleteMPU.
+ * stored MPU configuration and per-part checksums.
+ *
+ * Returns `{ result, error }`:
+ *   - Success: `{ result: { algorithm, type, value }, error: null }`
+ *   - No MPU checksum configured: `{ result: null, error: null }`
+ *   - "Should-not-happen" branch (missing stored ChecksumValue, unknown
+ *     checksumType, compute primitive error): behavior depends on whether
+ *     the client opted in to checksums —
+ *       - Default MPU (`checksumIsDefault === true`): the client never
+ *         asked for a checksum; soft-fail with `{ result: null, error: null }`
+ *         so the response simply omits the checksum field.
+ *       - Explicit MPU (`checksumIsDefault === false`): the client asked
+ *         for a checksum at CreateMPU; silently dropping it would violate
+ *         the contract. Return `{ result: null, error: <InternalError> }`
+ *         for the caller to fail the CompleteMPU.
  *
  * @param {array} storedParts - parts from services.getMPUparts (carry ChecksumValue)
  * @param {array} filteredPartList - validated, ordered subset matching jsonList
  * @param {object} storedMetadata - MPU overview metadata
  * @param {string} mpuSplitter - splitter used in part keys
  * @param {string} uploadId - for log context
  * @param {object} log - werelogs logger
- * @returns {object|null} { algorithm, type, value } or null
+ * @returns {Promise<{ result: ({algorithm, type, value}|null), error: (ArsenalError|null) }>}
  */
 async function computeFinalChecksum(storedParts, filteredPartList, storedMetadata, mpuSplitter, uploadId, log) {
     const algorithm = storedMetadata.checksumAlgorithm;
     const type = storedMetadata.checksumType;
     if (!algorithm || !type) {
-        return null;
+        return { result: null, error: null };
+    }
+    const isDefault = !!storedMetadata.checksumIsDefault;
+
+    function failOrSkip(message, extra) {
+        log.error(message, { uploadId, algorithm, type, ...extra });
+        if (isDefault) {
+            return { result: null, error: null };
+        }
+        return { result: null, error: errorInstances.InternalError.customizeDescription(message) };
     }
 
     const storedByKey = new Map();
@@ -164,41 +182,29 @@ async function computeFinalChecksum(storedParts, filteredPartList, storedMetadat
     }
 
     if (missingPartNumbers.length > 0) {
-        log.error('one or more MPU parts missing checksum value; ' + 'skipping final-object checksum computation', {
-            uploadId,
-            algorithm,
-            type,
-            missingPartNumbers,
-        });
-        return null;
+        return failOrSkip('one or more MPU parts missing checksum value', { missingPartNumbers });
     }
 
-    let result;
+    let computed;
     if (type === 'COMPOSITE') {
-        result = await computeCompositeMPUChecksum(
+        computed = await computeCompositeMPUChecksum(
             algorithm,
             partInputs.map(p => p.value),
         );
     } else if (type === 'FULL_OBJECT') {
-        result = computeFullObjectMPUChecksum(algorithm, partInputs);
+        computed = computeFullObjectMPUChecksum(algorithm, partInputs);
     } else {
-        log.error('unknown MPU checksumType; skipping final-object checksum computation', {
-            uploadId,
-            checksumType: type,
-        });
-        return null;
+        return failOrSkip('unknown MPU checksumType', { checksumType: type });
     }
 
-    if (result.error) {
-        log.error('final-object checksum computation failed', {
-            uploadId,
-            checksumErrorCode: result.error.code,
-            checksumErrorDetails: result.error.details,
+    if (computed.error) {
+        return failOrSkip('final-object checksum computation failed', {
+            checksumErrorCode: computed.error.code,
+            checksumErrorDetails: computed.error.details,
         });
-        return null;
     }
 
-    return { algorithm, type, value: result.checksum };
+    return { result: { algorithm, type, value: computed.checksum }, error: null };
 }
 
 /*
@@ -620,9 +626,16 @@ function completeMultipartUpload(authInfo, request, log, callback) {
                     uploadId,
                     log,
                 ).then(
-                    fc => {
+                    ({ result, error }) => {
+                        if (error) {
+                            log.error('failing CompleteMPU due to final-object checksum error', {
+                                uploadId,
+                                error,
+                            });
+                            return callNext(error, destBucket);
+                        }
                         try {
-                            finalChecksum = fc;
+                            finalChecksum = result;
                             const checksumErr = validateCompleteMultipartUploadChecksum(request.headers, finalChecksum);
                             if (checksumErr) {
                                 log.debug('x-amz-checksum-<algo> header validation failed on CompleteMPU', {

tests/unit/api/multipartUpload.js

@@ -4201,13 +4201,23 @@ describe('computeFinalChecksum', () => {
         }));
     }
 
-    it('should return null when MPU has no checksumAlgorithm', async () => {
+    function assertSoftNull(got) {
+        assert.deepStrictEqual(got, { result: null, error: null });
+    }
+
+    function assertInternalError(got) {
+        assert.strictEqual(got.result, null);
+        assert(got.error, 'expected an error on the result');
+        assert.strictEqual(got.error.is.InternalError, true);
+    }
+
+    it('should return { result: null, error: null } when MPU has no checksumAlgorithm', async () => {
         const stored = [makeStoredPart(1, { algorithm: 'sha256', value: SAMPLE_DIGESTS.sha256[0] })];
         const got = await computeFinalChecksum(stored, partListFromStored(stored), {}, splitter, uploadId, log);
-        assert.strictEqual(got, null);
+        assertSoftNull(got);
     });
 
-    it('should return null when MPU has no checksumType', async () => {
+    it('should return { result: null, error: null } when MPU has no checksumType', async () => {
         const stored = [makeStoredPart(1, { algorithm: 'sha256', value: SAMPLE_DIGESTS.sha256[0] })];
         const got = await computeFinalChecksum(
             stored,
@@ -4217,7 +4227,7 @@ describe('computeFinalChecksum', () => {
             uploadId,
             log,
         );
-        assert.strictEqual(got, null);
+        assertSoftNull(got);
     });
 
     it('should return COMPOSITE checksum with -N suffix for SHA256 MPU', async () => {
@@ -4227,25 +4237,26 @@ describe('computeFinalChecksum', () => {
             makeStoredPart(2, { algorithm: 'sha256', value: d2 }),
             makeStoredPart(3, { algorithm: 'sha256', value: d3 }),
         ];
-        const got = await computeFinalChecksum(
+        const { result, error } = await computeFinalChecksum(
             stored,
             partListFromStored(stored),
             { checksumAlgorithm: 'sha256', checksumType: 'COMPOSITE' },
             splitter,
             uploadId,
             log,
         );
-        assert(got);
-        assert.strictEqual(got.algorithm, 'sha256');
-        assert.strictEqual(got.type, 'COMPOSITE');
-        assert(got.value.endsWith('-3'), `expected -N suffix, got ${got.value}`);
+        assert.strictEqual(error, null);
+        assert(result);
+        assert.strictEqual(result.algorithm, 'sha256');
+        assert.strictEqual(result.type, 'COMPOSITE');
+        assert(result.value.endsWith('-3'), `expected -N suffix, got ${result.value}`);
         // computeCompositeMPUChecksum's deterministic output for these
         // exact placeholder digests:
         const expected = crypto
             .createHash('sha256')
             .update(Buffer.concat([d1, d2, d3].map(x => Buffer.from(x, 'base64'))))
             .digest('base64');
-        assert.strictEqual(got.value, `${expected}-3`);
+        assert.strictEqual(result.value, `${expected}-3`);
     });
 
     ['sha1', 'crc32', 'crc32c'].forEach(algo => {
@@ -4255,18 +4266,19 @@ describe('computeFinalChecksum', () => {
                 makeStoredPart(1, { algorithm: algo, value: d1 }),
                 makeStoredPart(2, { algorithm: algo, value: d2 }),
             ];
-            const got = await computeFinalChecksum(
+            const { result, error } = await computeFinalChecksum(
                 stored,
                 partListFromStored(stored),
                 { checksumAlgorithm: algo, checksumType: 'COMPOSITE' },
                 splitter,
                 uploadId,
                 log,
             );
-            assert(got);
-            assert.strictEqual(got.algorithm, algo);
-            assert.strictEqual(got.type, 'COMPOSITE');
-            assert(got.value.endsWith('-2'));
+            assert.strictEqual(error, null);
+            assert(result);
+            assert.strictEqual(result.algorithm, algo);
+            assert.strictEqual(result.type, 'COMPOSITE');
+            assert(result.value.endsWith('-2'));
         });
     });
 
@@ -4299,23 +4311,47 @@ describe('computeFinalChecksum', () => {
                 },
             },
         ];
-        const got = await computeFinalChecksum(
+        const { result, error } = await computeFinalChecksum(
             stored,
             partListFromStored(stored),
             { checksumAlgorithm: 'crc64nvme', checksumType: 'FULL_OBJECT' },
             splitter,
             uploadId,
             log,
         );
-        assert(got);
-        assert.strictEqual(got.algorithm, 'crc64nvme');
-        assert.strictEqual(got.type, 'FULL_OBJECT');
-        assert(!got.value.includes('-'), `FULL_OBJECT should have no -N suffix, got ${got.value}`);
+        assert.strictEqual(error, null);
+        assert(result);
+        assert.strictEqual(result.algorithm, 'crc64nvme');
+        assert.strictEqual(result.type, 'FULL_OBJECT');
+        assert(!result.value.includes('-'), `FULL_OBJECT should have no -N suffix, got ${result.value}`);
         const expected = await algorithms.crc64nvme.digest(Buffer.concat([a, b]));
-        assert.strictEqual(got.value, expected);
+        assert.strictEqual(result.value, expected);
+    });
+
+    // Soft-null (`{ result: null, error: null }`) is intentional only for
+    // default MPUs — the client didn't opt in to a checksum, so missing it
+    // on the response is graceful degradation. Explicit MPUs return
+    // `{ result: null, error: InternalError }` because silently dropping
+    // a checksum the client asked for would violate the CreateMPU contract.
+
+    it('should soft-null when a default-MPU part is missing ChecksumValue', async () => {
+        const stored = [
+            makeStoredPart(1, { algorithm: 'crc64nvme', value: SAMPLE_DIGESTS.crc64nvme[0] }),
+            makeStoredPart(2, null),
+            makeStoredPart(3, { algorithm: 'crc64nvme', value: SAMPLE_DIGESTS.crc64nvme[1] }),
+        ];
+        const got = await computeFinalChecksum(
+            stored,
+            partListFromStored(stored),
+            { checksumAlgorithm: 'crc64nvme', checksumType: 'FULL_OBJECT', checksumIsDefault: true },
+            splitter,
+            uploadId,
+            log,
+        );
+        assertSoftNull(got);
     });
 
-    it('should return null and log when a part is missing ChecksumValue', async () => {
+    it('should return InternalError when an explicit-MPU part is missing ChecksumValue', async () => {
         const stored = [
             makeStoredPart(1, { algorithm: 'sha256', value: SAMPLE_DIGESTS.sha256[0] }),
             makeStoredPart(2, null),
@@ -4324,42 +4360,55 @@ describe('computeFinalChecksum', () => {
         const got = await computeFinalChecksum(
             stored,
             partListFromStored(stored),
-            { checksumAlgorithm: 'sha256', checksumType: 'COMPOSITE' },
+            { checksumAlgorithm: 'sha256', checksumType: 'COMPOSITE', checksumIsDefault: false },
+            splitter,
+            uploadId,
+            log,
+        );
+        assertInternalError(got);
+    });
+
+    it('should soft-null when checksumType is unknown on a default MPU', async () => {
+        const stored = [makeStoredPart(1, { algorithm: 'crc64nvme', value: SAMPLE_DIGESTS.crc64nvme[0] })];
+        const got = await computeFinalChecksum(
+            stored,
+            partListFromStored(stored),
+            { checksumAlgorithm: 'crc64nvme', checksumType: 'WEIRD', checksumIsDefault: true },
             splitter,
             uploadId,
             log,
         );
-        assert.strictEqual(got, null);
+        assertSoftNull(got);
     });
 
-    it('should return null when checksumType is unknown', async () => {
+    it('should return InternalError when checksumType is unknown on an explicit MPU', async () => {
         const stored = [makeStoredPart(1, { algorithm: 'sha256', value: SAMPLE_DIGESTS.sha256[0] })];
         const got = await computeFinalChecksum(
             stored,
             partListFromStored(stored),
-            { checksumAlgorithm: 'sha256', checksumType: 'WEIRD' },
+            { checksumAlgorithm: 'sha256', checksumType: 'WEIRD', checksumIsDefault: false },
             splitter,
             uploadId,
             log,
         );
-        assert.strictEqual(got, null);
+        assertInternalError(got);
     });
 
-    it(
-        'should return null when underlying compute reports an error ' + '(crc64nvme COMPOSITE is not allowed)',
-        async () => {
-            const stored = [makeStoredPart(1, { algorithm: 'crc64nvme', value: SAMPLE_DIGESTS.crc64nvme[0] })];
-            const got = await computeFinalChecksum(
-                stored,
-                partListFromStored(stored),
-                { checksumAlgorithm: 'crc64nvme', checksumType: 'COMPOSITE' },
-                splitter,
-                uploadId,
-                log,
-            );
-            assert.strictEqual(got, null);
-        },
-    );
+    it('should return InternalError when underlying compute reports an error on an explicit MPU', async () => {
+        // crc64nvme + COMPOSITE is not allowed by computeCompositeMPUChecksum.
+        // Reaching here on an explicit MPU means upstream validation failed,
+        // which is exactly the kind of internal-state bug we want to surface.
+        const stored = [makeStoredPart(1, { algorithm: 'crc64nvme', value: SAMPLE_DIGESTS.crc64nvme[0] })];
+        const got = await computeFinalChecksum(
+            stored,
+            partListFromStored(stored),
+            { checksumAlgorithm: 'crc64nvme', checksumType: 'COMPOSITE', checksumIsDefault: false },
+            splitter,
+            uploadId,
+            log,
+        );
+        assertInternalError(got);
+    });
 
     it('should compute over filteredPartList (subset), not all storedParts', async () => {
         const [d1, d2, d3] = [SAMPLE_DIGESTS.sha256[0], SAMPLE_DIGESTS.sha256[1], SAMPLE_DIGESTS.sha256[0]];
@@ -4375,21 +4424,22 @@ describe('computeFinalChecksum', () => {
             size: s.value.Size,
             locations: s.value.partLocations,
         }));
-        const got = await computeFinalChecksum(
+        const { result, error } = await computeFinalChecksum(
             stored,
             filtered,
             { checksumAlgorithm: 'sha256', checksumType: 'COMPOSITE' },
             splitter,
             uploadId,
             log,
         );
-        assert(got);
-        assert(got.value.endsWith('-2'), `should reflect 2 completed parts, got ${got.value}`);
+        assert.strictEqual(error, null);
+        assert(result);
+        assert(result.value.endsWith('-2'), `should reflect 2 completed parts, got ${result.value}`);
         const expected = crypto
             .createHash('sha256')
             .update(Buffer.concat([d1, d3].map(x => Buffer.from(x, 'base64'))))
             .digest('base64');
-        assert.strictEqual(got.value, `${expected}-2`);
+        assert.strictEqual(result.value, `${expected}-2`);
     });
 });