CLDSRV-848: return InvalidDigest if Content-MD5 is invalid

Author: leif-scality

lib/api/api.js

@@ -312,6 +312,7 @@ const api = {
                             return next(null, userInfo, authorizationResults, streamingV4Params, infos);
                         })
                         .catch(error => {
+                            log.error('error validating checksums', { error });
                             next(error);
                         });
                 });

lib/api/apiUtils/integrity/validateChecksums.js

@@ -7,6 +7,7 @@ const { config } = require('../../../Config');
 
 const ChecksumError = Object.freeze({
     MD5Mismatch: 'MD5Mismatch',
+    MD5Invalid: 'MD5Invalid',
     XAmzMismatch: 'XAmzMismatch',
     MissingChecksum: 'MissingChecksum',
     AlgoNotSupported: 'AlgoNotSupported',
@@ -158,7 +159,14 @@ async function validateChecksumsNoChunking(headers, body) {
 
     let md5Present = false;
     if ('content-md5' in headers) {
-        // TODO: check if the content-md5 is valid base64
+        if (typeof headers['content-md5'] !== 'string') {
+            return { error: ChecksumError.MD5Invalid, details: { expected: headers['content-md5'] } };
+        }
+
+        if (headers['content-md5'].length !== 24) {
+            return { error: ChecksumError.MD5Invalid, details: { expected: headers['content-md5'] } };
+        }
+
         const md5 = crypto.createHash('md5').update(body).digest('base64');
         if (md5 !== headers['content-md5']) {
             return { error: ChecksumError.MD5Mismatch, details: { calculated: md5, expected: headers['content-md5'] } };
@@ -182,7 +190,9 @@ async function defaultValidationFunc(request, body, log) {
         return null;
     }
 
-    log.debug('failed checksum validation', { method: request.apiMethod }, err);
+    if (err.error !== ChecksumError.MissingChecksum) {
+        log.debug('failed checksum validation', { method: request.apiMethod }, err);
+    }
 
     switch (err.error) {
         case ChecksumError.MissingChecksum:
@@ -214,6 +224,8 @@ async function defaultValidationFunc(request, body, log) {
             return ArsenalErrors.InvalidRequest.customizeDescription(
                 `Value for x-amz-checksum-${err.details.algorithm} header is invalid.`
             );
+        case ChecksumError.MD5Invalid:
+            return ArsenalErrors.InvalidDigest;
         default:
             return ArsenalErrors.BadDigest;
     }

tests/unit/api/api.js

@@ -138,7 +138,7 @@ describe('api.callApiMethod', () => {
             it(`should return BadDigest for ${method} when bad MD5 checksum is provided`, done => {
                 const body = '<xml></xml>';
                 const headers = {
-                    'content-md5': 'badchecksum123=', // Invalid MD5
+                    'content-md5': '1B2M2Y8AsgTpgAmY7PhCfg==', // Wrong MD5
                     'content-length': body.length.toString()
                 };
 
@@ -160,6 +160,32 @@ describe('api.callApiMethod', () => {
             });
         });
 
+        methodsWithChecksumValidation.forEach(method => {
+            it(`should return InvalidDigest for ${method} when invalid MD5 checksum is provided`, done => {
+                const body = '<xml></xml>';
+                const headers = {
+                    'content-md5': 'x', // Invalid MD5
+                    'content-length': body.length.toString()
+                };
+                
+                const requestWithBody = new DummyRequest({
+                    headers,
+                    query: {},
+                    socket: { remoteAddress: '127.0.0.1', destroy: sandbox.stub() }
+                }, body);
+                
+                sandbox.stub(api, method).callsFake(() => {
+                    done(new Error(`${method} was called despite bad checksum`));
+                });
+
+                api.callApiMethod(method, requestWithBody, response, log, err => {
+                    assert(err, `Expected error for ${method} with bad checksum`);
+                    assert(err.is.InvalidDigest, `Expected InvalidDigest error for ${method}, got: ${err.code}`);
+                    done();
+                });
+            });
+        });
+
         methodsWithChecksumValidation.forEach(method => {
             it(`should succeed for ${method} when correct MD5 checksum is provided`, done => {
                 const body = '<xml></xml>';

tests/unit/api/apiUtils/integrity/validateChecksums.js

@@ -24,7 +24,7 @@ describe('validateChecksumsNoChunking MD5', () => {
     describe('with MD5 mismatch', () => {
         it('should return MD5Mismatch error when checksums do not match', async () => {
             const body = 'Hello, World!';
-            const wrongMd5 = 'wrongchecksum123=';
+            const wrongMd5 = '1B2M2Y8AsgTpgAmY7PhCfg==';
             const expectedMd5 = crypto.createHash('md5').update(body, 'utf8').digest('base64');
             const headers = {
                 'content-md5': wrongMd5
@@ -56,45 +56,39 @@ describe('validateChecksumsNoChunking MD5', () => {
             assert.strictEqual(result.details, null);
         });
 
-        it('should return MD5Mismatch error when content-md5 header is undefined', async () => {
+        it('should return MD5Invalid error when content-md5 header is undefined', async () => {
             const body = 'Hello, World!';
             const headers = {
                 'content-type': 'application/json',
                 'content-md5': undefined
             };
-            const calculatedMD5 = crypto.createHash('md5').update(body, 'utf8').digest('base64');
 
             const result = await validateChecksumsNoChunking(headers, body);
-            assert.strictEqual(result.error, ChecksumError.MD5Mismatch);
-            assert.strictEqual(result.details.calculated, calculatedMD5);
+            assert.strictEqual(result.error, ChecksumError.MD5Invalid);
             assert.strictEqual(result.details.expected, undefined);
         });
 
-        it('should return MD5Mismatch error when content-md5 header is null', async () => {
+        it('should return MD5Invalid error when content-md5 header is null', async () => {
             const body = 'Hello, World!';
             const headers = {
                 'content-type': 'application/json',
                 'content-md5': null
             };
-            const calculatedMD5 = crypto.createHash('md5').update(body, 'utf8').digest('base64');
 
             const result = await validateChecksumsNoChunking(headers, body);
-            assert.strictEqual(result.error, ChecksumError.MD5Mismatch);
-            assert.strictEqual(result.details.calculated, calculatedMD5);
+            assert.strictEqual(result.error, ChecksumError.MD5Invalid);
             assert.strictEqual(result.details.expected, null);
         });
 
-        it('should return MD5Mismatch error when content-md5 header is empty string', async () => {
+        it('should return MD5Invalid error when content-md5 header is empty string', async () => {
             const body = 'Hello, World!';
             const headers = {
                 'content-type': 'application/json',
                 'content-md5': ''
             };
-            const calculatedMD5 = crypto.createHash('md5').update(body, 'utf8').digest('base64');
 
             const result = await validateChecksumsNoChunking(headers, body);
-            assert.strictEqual(result.error, ChecksumError.MD5Mismatch);
-            assert.strictEqual(result.details.calculated, calculatedMD5);
+            assert.strictEqual(result.error, ChecksumError.MD5Invalid);
             assert.strictEqual(result.details.expected, '');
         });
     });
@@ -318,7 +312,7 @@ describe('validateMethodChecksumNoChunking', () => {
                 config.integrityChecks[method] = true;
 
                 const body = 'Hello, World!';
-                const wrongMd5 = 'wrongchecksum123=';
+                const wrongMd5 = '1B2M2Y8AsgTpgAmY7PhCfg==';
                 const request = {
                     apiMethod: method,
                     headers: {
@@ -335,6 +329,29 @@ describe('validateMethodChecksumNoChunking', () => {
         });
     });
 
+    describe('when checksum mismatches', () => {
+        supportedMethods.forEach(method => {
+            it(`should return InvalidDigest error for ${method} when checksum mismatch`, async () => {
+                config.integrityChecks[method] = true;
+
+                const body = 'Hello, World!';
+                const wrongMd5 = 'wrongchecksum123=';
+                const request = {
+                    apiMethod: method,
+                    headers: {
+                        'content-md5': wrongMd5
+                    }
+                };
+                const log = { debug: sandbox.stub() };
+
+                const result = await validateMethodChecksumNoChunking(request, body, log);
+
+                assert.deepStrictEqual(result, ArsenalErrors.InvalidDigest, 'Expected BadDigest error');
+                assert(log.debug.calledOnce);
+            });
+        });
+    });
+
     describe('when no checksum is provided', () => {
         supportedMethods.forEach(method => {
             it(`should return null for ${method} when no checksum is provided`, async () => {