Fix: Streaming v4 auth extra chunk

circle.yml

@@ -25,7 +25,7 @@ dependencies:
     - wget http://launchpadlibrarian.net/222422124/s3cmd_1.6.0-2_all.deb
     - sudo dpkg -i s3cmd*.deb
     # fog and ruby testing dependencies
-    - gem install fog
+    - gem install fog-aws
     - gem install mime-types
     - gem install rspec
     - gem install json

lib/auth/streamingV4/V4Transform.js

@@ -49,6 +49,8 @@ export default class V4Transform extends Transform {
         this.currentData = undefined;
         this.dataCursor = 0;
         this.currentMetadata = [];
+        this.lastPieceDone = false;
+        this.lastChunk = false;
     }
 
     /**
@@ -95,6 +97,7 @@ export default class V4Transform extends Transform {
         }
 
         const splitMeta = fullMetadata.toString().split(';');
+        this.log.trace('parsed full metadata for chunk', { splitMeta });
         if (splitMeta.length !== 2) {
             this.log.trace('chunk body did not contain correct ' +
             'metadata format');
@@ -114,21 +117,23 @@ export default class V4Transform extends Transform {
         }
         chunkSig = chunkSig.replace('chunk-signature=', '');
         this.currentSignature = chunkSig;
-        this.seekingDataSize = dataSize;
-        this.currentData = Buffer.alloc(dataSize);
         this.haveMetadata = true;
-        // start slice at lineBreak plus 2 to remove line break at end of
-        // metadata piece since length of '\r\n' is 2
-        let unparsedChunk = remainingPlusStoredMetadata
-            .slice(lineBreakIndex + 2);
-        // remove extra line break at end of metadata piece
-        // (beginning of data piece) if any
-        if (unparsedChunk.indexOf('\r\n') === 0) {
-            unparsedChunk = unparsedChunk.slice(2);
+        if (dataSize === 0) {
+            this.lastChunk = true;
+            return {
+                completeMetadata: true,
+            };
         }
+        // + 2 to get \r\n at end
+        this.seekingDataSize = dataSize + 2;
+        this.currentData = Buffer.alloc(dataSize);
+
         return {
             completeMetadata: true,
-            unparsedChunk,
+            // start slice at lineBreak plus 2 to remove line break at end of
+            // metadata piece since length of '\r\n' is 2
+            unparsedChunk: remainingPlusStoredMetadata
+                .slice(lineBreakIndex + 2),
         };
     }
 
@@ -162,7 +167,8 @@ export default class V4Transform extends Transform {
         };
         return vault.authenticateV4Request(vaultParams, null, err => {
             if (err) {
-                this.log.trace('err from vault', { error: err });
+                this.log.trace('err from vault on streaming v4 auth',
+                    { error: err, paramsSentToVault: vaultParams.data });
                 return done(err);
             }
             return done();
@@ -187,6 +193,13 @@ export default class V4Transform extends Transform {
         // signature + \r\n + chunk-data + \r\n
         // Last transfer-encoding chunk will have size 0 and no chunk-data.
 
+        if (this.lastPieceDone) {
+            const slice = chunk.slice(0, 10);
+            this.log.trace('received chunk after end.' +
+            'See first 10 bytes of chunk',
+            { chunk: slice.toString() });
+            return callback();
+        }
         let unparsedChunk = chunk;
         let chunkLeftToEvaluate = true;
         return async.whilst(
@@ -195,6 +208,8 @@ export default class V4Transform extends Transform {
             // async function
             done => {
                 if (!this.haveMetadata) {
+                    this.log.trace('do not have metadata so calling ' +
+                    '_parseMetadata');
                     // need to parse our metadata
                     const parsedMetadataResults =
                         this._parseMetadata(unparsedChunk);
@@ -210,12 +225,14 @@ export default class V4Transform extends Transform {
                     // without metadata piece
                     unparsedChunk = parsedMetadataResults.unparsedChunk;
                 }
-                if (this.seekingDataSize === 0) {
+                if (this.lastChunk) {
+                    this.log.trace('authenticating final chunk with no data');
                     return this._authenticate(null, err => {
                         if (err) {
                             return done(err);
                         }
                         chunkLeftToEvaluate = false;
+                        this.lastPieceDone = true;
                         return done();
                     });
                 }
@@ -227,9 +244,10 @@ export default class V4Transform extends Transform {
                     chunkLeftToEvaluate = false;
                     return done();
                 }
-                // parse just the next data piece
+                // parse just the next data piece without \r\n at the end
+                // (therefore, minus 2)
                 const nextDataPiece =
-                    unparsedChunk.slice(0, this.seekingDataSize);
+                    unparsedChunk.slice(0, this.seekingDataSize - 2);
                 // add parsed data piece to other currentData pieces
                 // so that this.currentData is the full data piece
                 nextDataPiece.copy(this.currentData, this.dataCursor);
@@ -238,7 +256,7 @@ export default class V4Transform extends Transform {
                         return done(err);
                     }
                     unparsedChunk =
-                        unparsedChunk.slice(this.seekingDataSize + 1);
+                        unparsedChunk.slice(this.seekingDataSize);
                     this.push(this.currentData);
                     this.haveMetadata = false;
                     this.seekingDataSize = -1;

tests/functional/fog/tests.rb

@@ -1,4 +1,4 @@
-require "fog"
+require "fog/aws"
 require "digest/md5"
 require "json"
 require "securerandom"

tests/functional/jaws/src/test/java/com/scality/StreamV4AuthTest.java

@@ -7,6 +7,7 @@
 import java.util.Arrays;
 import java.util.Collection;
 import java.io.RandomAccessFile;
+import java.security.SecureRandom;
 
 import com.amazonaws.SDKGlobalConfiguration;
 import org.junit.Assert;
@@ -131,15 +132,24 @@ public void testStreamV4Auth() throws Exception {
         Assert.assertEquals(object.getObjectMetadata().getETag(), md5);
     }
 
-
     /**
     * Creates a temporary file
     * @param {Integer} fileSize - file size in bytes
     * @return A newly created temporary file
     * @throws Exception
     */
     private static File createSampleFile(Integer fileSize) throws Exception {
+        String alph = "\r\nABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
+        SecureRandom random = new SecureRandom();
+        Integer randomLimit = Math.min(Math.round(fileSize/10), 10000);
+        StringBuilder myStringBuilder = new StringBuilder(randomLimit);
+        for(int i = 0; i < randomLimit; i++)
+              myStringBuilder.append(alph.charAt(random.nextInt(alph.length())));
+        String myString = myStringBuilder.toString();
         RandomAccessFile file = new RandomAccessFile(fileName, "rw");
+        file.writeUTF("\r\nlet's add some \r\n data with \r\n\rn\rn\rn\r\n\r\n\n");
+        file.writeUTF(myString);
+        file.writeUTF("\r\nadd\r\nmore\r\nlines\r\n");
         file.setLength(fileSize);
         file.close();
         File myFile = new File(fileName);

tests/unit/auth/V4Transform.js

@@ -0,0 +1,63 @@
+import assert from 'assert';
+import { Readable } from 'stream';
+
+import V4Transform from '../../../lib/auth/streamingV4/V4Transform';
+import { DummyRequestLogger } from '../helpers';
+
+const log = new DummyRequestLogger();
+const streamingV4Params = { accessKey: 'accessKey1',
+  signatureFromRequest: '2b8637632a997e06ee7b6c85d7' +
+    '147d2025e8f04d4374f4d7d7320de1618c7509',
+  region: 'us-east-1',
+  scopeDate: '20170516',
+  timestamp: '20170516T204738Z',
+  credentialScope: '20170516/us-east-1/s3/aws4_request' };
+
+class AuthMe extends Readable {
+    constructor(chunks) {
+        super();
+        this._parts = chunks;
+        this._index = 0;
+    }
+
+    _read() {
+        this.push(this._parts[this._index]);
+        this._index++;
+    }
+
+}
+
+describe('V4Transform class', () => {
+    it('should authenticate successfully', done => {
+        const v4Transform = new V4Transform(streamingV4Params, log, err => {
+            assert.strictEqual(err, null);
+        });
+        const chunks = [new Buffer('8;chunk-signature=' +
+        '51d2511f7c6887907dff20474d8db67d557e5f515a6fa6a8466bb12f8833bcca\r\n' +
+        'contents\r\n'), new Buffer('0;chunk-signature=' +
+        'c0eac24b7ce72141ec077df9753db4cc8b7991491806689da0395c8bd0231e48\r\n'),
+        null];
+        const authMe = new AuthMe(chunks);
+        authMe.pipe(v4Transform);
+        v4Transform.on('finish', () => {
+            done();
+        });
+    });
+
+    it('should ignore data sent after final chunk', done => {
+        const v4Transform = new V4Transform(streamingV4Params, log, err => {
+            assert.strictEqual(err, null);
+            done();
+        });
+        const chunks = [new Buffer('8;chunk-signature=' +
+        '51d2511f7c6887907dff20474d8db67d557e5f515a6fa6a8466bb12f8833bcca\r\n' +
+        'contents\r\n'), new Buffer('0;chunk-signature=' +
+        'c0eac24b7ce72141ec077df9753db4cc8b7991491806689da0395c8bd0231e48\r\n'),
+        new Buffer('\r\n'), null];
+        const authMe = new AuthMe(chunks);
+        authMe.pipe(v4Transform);
+        v4Transform.on('finish', () => {
+            done();
+        });
+    });
+});