Fix v2 signature verification

lib/middleware/authentication.js

@@ -7,7 +7,11 @@ const querystring = require('querystring');
 const AWSAccount = require('../models/account');
 const S3Error = require('../models/error');
 const { RESPONSE_HEADERS } = require('./response-header-override');
-const { parseDate, parseISO8601String } = require('../utils');
+const {
+  encodeURIComponentRFC3986,
+  parseDate,
+  parseISO8601String,
+} = require('../utils');
 
 const SUBRESOURCES = {
   acl: 1,
@@ -51,13 +55,9 @@ module.exports = () =>
       return next();
     }
 
-    const query = mapKeys(ctx.query, (value, key) => key.toLowerCase());
     const amzQueryHeaders = pickBy(
-      query,
-      (value, key) =>
-        key.startsWith('x-amz-') &&
-        // x-amz-website-redirect-location isn't a canonical header
-        key !== 'x-amz-website-redirect-location',
+      mapKeys(ctx.query, (value, key) => key.toLowerCase()),
+      (value, key) => key.startsWith('x-amz-'),
     );
     // x-amz-* values specified in query params take precedence over those in the headers
     Object.assign(ctx.headers, amzQueryHeaders);
@@ -85,32 +85,49 @@ module.exports = () =>
       );
     }
 
-    const canonicalizedAmzHeaders = Object.keys(ctx.headers)
+    let canonicalizedResource = ctx.mountPath || '';
+    if (ctx.params.bucket) {
+      // the following behavior is derived from the behavior of the JS aws-sdk
+      if (ctx.state.vhost) {
+        canonicalizedResource = '/' + ctx.params.bucket + canonicalizedResource;
+      } else {
+        canonicalizedResource += '/' + ctx.params.bucket;
+      }
+      if (ctx.params.key) {
+        canonicalizedResource += '/' + ctx.params.key;
+      }
+    } else {
+      canonicalizedResource += '/';
+    }
+    canonicalizedResource = canonicalizedResource
+      .split('/')
+      .map(encodeURIComponentRFC3986)
+      .join('/');
+
+    const canonicalizedQueryString = Object.entries(ctx.query)
       .filter(
-        headerName =>
-          headerName.startsWith('x-amz-') &&
-          headerName !== 'x-amz-website-redirect-location',
+        ([param]) =>
+          SUBRESOURCES[param] ||
+          RESPONSE_HEADERS[param] ||
+          (mechanisms.queryV4 && param.startsWith('x-amz-')),
+      )
+      .map(([param, value]) =>
+        // v2 signing doesn't encode values in the signature calculation
+        mechanisms.queryV2
+          ? [param, value].join('=')
+          : querystring.stringify({ [param]: value }),
       )
       .sort()
+      .join('&')
+      .replace(/=(&|$)/g, ''); // remove trailing = for empty params
+
+    const canonicalizedAmzHeaders = Object.keys(ctx.headers)
+      .filter(headerName => headerName.startsWith('x-amz-'))
+      .sort()
       .map(
         headerName =>
           `${headerName}:${ctx.get(headerName).replace(/ +/g, ' ')}`,
       );
-    const canonicalizedQueryString = Object.keys(query)
-      .filter(
-        paramName =>
-          SUBRESOURCES[paramName] ||
-          RESPONSE_HEADERS[paramName] ||
-          (mechanisms.queryV4 && paramName.startsWith('x-amz-')),
-      )
-      .map(paramName =>
-        // v2 signing doesn't encode values in the signature calculation
-        mechanisms.queryV2
-          ? [paramName, query[paramName]].join('=')
-          : querystring.stringify({ [paramName]: query[paramName] }),
-      )
-      .sort()
-      .join('&');
 
     const canonicalRequest = {
       method:
@@ -120,10 +137,7 @@ module.exports = () =>
       contentMD5: ctx.get('Content-MD5'),
       contentType: ctx.get('Content-Type'),
       timestamp: undefined,
-      // the following behavior is derived from the behavior of the JS aws-sdk
-      uri: ctx.state.vhost
-        ? `/${ctx.params.bucket}${ctx.mountPath || ''}/${ctx.params.key}`
-        : `${ctx.mountPath || ''}/${ctx.params.bucket}/${ctx.params.key}`,
+      uri: canonicalizedResource,
       querystring: canonicalizedQueryString,
       amzHeaders: canonicalizedAmzHeaders,
     };
@@ -134,7 +148,6 @@ module.exports = () =>
     if (mechanisms.header) {
       const result = parseHeader(ctx.headers);
       ({ signature, signatureProvided } = result);
-      canonicalRequest.timestamp = result.timestamp;
     } else if (mechanisms.queryV2) {
       const result = parseQueryV2(ctx.query, ctx.headers);
       ({ signature, signatureProvided } = result);

lib/routes.js

@@ -231,7 +231,7 @@ router
 
 // append trailing slash to key when applicable
 router.param('key', (key, ctx, next) => {
-  if (ctx.path.endsWith('/')) {
+  if (key && ctx.path.endsWith('/')) {
     ctx.params.key = key + '/';
   }
   return next();

lib/utils.js

@@ -55,6 +55,24 @@ exports.concatStreams = function(streams) {
   return passThrough;
 };
 
+/**
+ * URI-encodes a string according to RFC 3986. This is what AWS uses for
+ * S3 resource URIs.
+ *
+ * @param {string} string
+ */
+exports.encodeURIComponentRFC3986 = function(string) {
+  return encodeURIComponent(string).replace(
+    /[!'()*]/g,
+    ch =>
+      '%' +
+      ch
+        .charCodeAt(0)
+        .toString(16)
+        .toUpperCase(),
+  );
+};
+
 exports.getXmlRootTag = function(xml) {
   const traversal = xmlParser.getTraversalObj(xml.toString());
   const [[root]] = Object.values(traversal.child);

test/controllers/object.spec.js

@@ -1,6 +1,5 @@
 'use strict';
 
-const AWS = require('aws-sdk');
 const { expect } = require('chai');
 const express = require('express');
 const FormData = require('form-data');
@@ -14,8 +13,6 @@ const request = require('request-promise-native').defaults({
 });
 const { URL, URLSearchParams } = require('url');
 
-const S3rver = require('../..');
-
 const { createServerAndClient, generateTestObjects } = require('../helpers');
 
 describe('Operations on Objects', () => {
@@ -849,35 +846,19 @@ describe('Operations on Objects', () => {
 
     it('stores an object in a bucket after all objects are deleted', async function() {
       const bucket = 'foobars';
-
-      const server = new S3rver();
-      const { port } = await server.run();
-      const s3Client = new AWS.S3({
-        accessKeyId: 'S3RVER',
-        secretAccessKey: 'S3RVER',
-        endpoint: `http://localhost:${port}`,
-        sslEnabled: false,
-        s3ForcePathStyle: true,
-      });
-      try {
-        await s3Client.createBucket({ Bucket: bucket }).promise();
-        await s3Client
-          .putObject({ Bucket: bucket, Key: 'foo.txt', Body: 'Hello!' })
-          .promise();
-        await s3Client
-          .deleteObject({ Bucket: bucket, Key: 'foo.txt' })
-          .promise();
-        await s3Client
-          .putObject({ Bucket: bucket, Key: 'foo2.txt', Body: 'Hello2!' })
-          .promise();
-      } finally {
-        await server.close();
-      }
+      await s3Client.createBucket({ Bucket: bucket }).promise();
+      await s3Client
+        .putObject({ Bucket: bucket, Key: 'foo.txt', Body: 'Hello!' })
+        .promise();
+      await s3Client.deleteObject({ Bucket: bucket, Key: 'foo.txt' }).promise();
+      await s3Client
+        .putObject({ Bucket: bucket, Key: 'foo2.txt', Body: 'Hello2!' })
+        .promise();
     });
   });
 
   describe('PUT Object - Copy', () => {
-    it('copys an image object into another bucket', async function() {
+    it('copies an image object into another bucket', async function() {
       const srcKey = 'image';
       const destKey = 'image/jamie';
 
@@ -895,7 +876,7 @@ describe('Operations on Objects', () => {
         .copyObject({
           Bucket: 'bucket-b',
           Key: destKey,
-          CopySource: '/' + 'bucket-a' + '/' + srcKey,
+          CopySource: '/bucket-a/' + srcKey,
         })
         .promise();
       expect(copyResult.ETag).to.equal(data.ETag);
@@ -909,7 +890,7 @@ describe('Operations on Objects', () => {
       expect(object.ETag).to.equal(data.ETag);
     });
 
-    it('copys an image object into another bucket including its metadata', async function() {
+    it('copies an image object into another bucket including its metadata', async function() {
       const srcKey = 'image';
       const destKey = 'image/jamie';
 
@@ -931,7 +912,7 @@ describe('Operations on Objects', () => {
           Bucket: 'bucket-b',
           Key: destKey,
           // MetadataDirective is implied to be COPY
-          CopySource: '/' + 'bucket-a' + '/' + srcKey,
+          CopySource: '/bucket-a/' + srcKey,
         })
         .promise();
       const object = await s3Client
@@ -942,7 +923,7 @@ describe('Operations on Objects', () => {
       expect(object.ETag).to.equal(data.ETag);
     });
 
-    it('copys an object using spaces/unicode chars in keys', async function() {
+    it('copies an object using spaces/unicode chars in keys', async function() {
       const srcKey = 'awesome 驚くばかり.jpg';
       const destKey = 'new 新しい.jpg';
 
@@ -960,14 +941,14 @@ describe('Operations on Objects', () => {
         .copyObject({
           Bucket: 'bucket-a',
           Key: destKey,
-          CopySource: '/' + 'bucket-a' + '/' + encodeURI(srcKey),
+          CopySource: '/bucket-a/' + encodeURI(srcKey),
         })
         .promise();
       expect(copyResult.ETag).to.equal(data.ETag);
       expect(moment(copyResult.LastModified).isValid()).to.be.true;
     });
 
-    it('copys an image object into another bucket and update its metadata', async function() {
+    it('copies an image object into another bucket and update its metadata', async function() {
       const srcKey = 'image';
       const destKey = 'image/jamie';
 
@@ -984,7 +965,7 @@ describe('Operations on Objects', () => {
         .copyObject({
           Bucket: 'bucket-b',
           Key: destKey,
-          CopySource: '/' + 'bucket-a' + '/' + srcKey,
+          CopySource: '/bucket-a/' + srcKey,
           MetadataDirective: 'REPLACE',
           Metadata: {
             someKey: 'value',
@@ -1016,7 +997,7 @@ describe('Operations on Objects', () => {
         .copyObject({
           Bucket: 'bucket-b',
           Key: destKey,
-          CopySource: '/' + 'bucket-a' + '/' + srcKey,
+          CopySource: '/bucket-a/' + srcKey,
           MetadataDirective: 'REPLACE',
           Metadata: {
             someKey: 'value',
@@ -1049,7 +1030,7 @@ describe('Operations on Objects', () => {
           .copyObject({
             Bucket: 'bucket-a',
             Key: key,
-            CopySource: '/' + 'bucket-a' + '/' + key,
+            CopySource: '/bucket-a/' + key,
             Metadata: {
               someKey: 'value',
             },
@@ -1069,7 +1050,7 @@ describe('Operations on Objects', () => {
           .copyObject({
             Bucket: 'bucket-b',
             Key: 'image/jamie',
-            CopySource: '/' + 'bucket-a' + '/doesnotexist',
+            CopySource: '/bucket-a/doesnotexist',
           })
           .promise();
       } catch (err) {

test/helpers.js

@@ -50,6 +50,7 @@ exports.createServerAndClient = async function createServerAndClient(options) {
     endpoint: `http://localhost:${port}`,
     sslEnabled: false,
     s3ForcePathStyle: true,
+    signatureVersion: 'v2',
   });
 
   return { s3rver, s3Client };