From 6e3604a4518008fa8785830aff59619eeb0e8eb7 Mon Sep 17 00:00:00 2001 From: Kyle Herock Date: Sat, 6 Jun 2020 03:44:11 -0400 Subject: [PATCH 1/8] make entire test suite run with v2 signatures --- test/helpers.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/helpers.js b/test/helpers.js index a6cfc0be..de156246 100644 --- a/test/helpers.js +++ b/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 }; From 08af9bd9b50eddcc03b767ac97b4c5ec1dfd0813 Mon Sep 17 00:00:00 2001 From: Kyle Herock Date: Sat, 6 Jun 2020 03:45:50 -0400 Subject: [PATCH 2/8] do not use timestamp in v2 stringToSign calculation --- lib/middleware/authentication.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/middleware/authentication.js b/lib/middleware/authentication.js index ddbd6df4..15d388a7 100644 --- a/lib/middleware/authentication.js +++ b/lib/middleware/authentication.js @@ -134,7 +134,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); From 5d523c0c7e76ad47b82edf52b7962d98f856d7c3 Mon Sep 17 00:00:00 2001 From: Kyle Herock Date: Sat, 6 Jun 2020 03:47:09 -0400 Subject: [PATCH 3/8] trim trailing = in canonicalized querystring --- lib/middleware/authentication.js | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/lib/middleware/authentication.js b/lib/middleware/authentication.js index 15d388a7..1c4c71dd 100644 --- a/lib/middleware/authentication.js +++ b/lib/middleware/authentication.js @@ -96,21 +96,22 @@ module.exports = () => headerName => `${headerName}:${ctx.get(headerName).replace(/ +/g, ' ')}`, ); - const canonicalizedQueryString = Object.keys(query) + const canonicalizedQueryString = Object.entries(query) .filter( - paramName => - SUBRESOURCES[paramName] || - RESPONSE_HEADERS[paramName] || - (mechanisms.queryV4 && paramName.startsWith('x-amz-')), + ([param]) => + SUBRESOURCES[param] || + RESPONSE_HEADERS[param] || + (mechanisms.queryV4 && param.startsWith('x-amz-')), ) - .map(paramName => + .map(([param, value]) => // v2 signing doesn't encode values in the signature calculation mechanisms.queryV2 - ? [paramName, query[paramName]].join('=') - : querystring.stringify({ [paramName]: query[paramName] }), + ? [param, value].join('=') + : querystring.stringify({ [param]: value }), ) .sort() - .join('&'); + .join('&') + .replace(/=(&|$)/g, ''); // remove trailing = for empty params const canonicalRequest = { method: From 62420aec50505fb1e2cfa3bb80e28aeb7077aa09 Mon Sep 17 00:00:00 2001 From: Kyle Herock Date: Sat, 6 Jun 2020 04:16:15 -0400 Subject: [PATCH 4/8] fix canonicalized resource calculation for bucket subresources --- lib/middleware/authentication.js | 43 +++++++++++++++++++++----------- lib/routes.js | 2 +- 2 files changed, 29 insertions(+), 16 deletions(-) diff --git a/lib/middleware/authentication.js b/lib/middleware/authentication.js index 1c4c71dd..06b29a78 100644 --- a/lib/middleware/authentication.js +++ b/lib/middleware/authentication.js @@ -85,17 +85,21 @@ module.exports = () => ); } - const canonicalizedAmzHeaders = Object.keys(ctx.headers) - .filter( - headerName => - headerName.startsWith('x-amz-') && - headerName !== 'x-amz-website-redirect-location', - ) - .sort() - .map( - headerName => - `${headerName}:${ctx.get(headerName).replace(/ +/g, ' ')}`, - ); + 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 += '/'; + } + const canonicalizedQueryString = Object.entries(query) .filter( ([param]) => @@ -113,6 +117,18 @@ module.exports = () => .join('&') .replace(/=(&|$)/g, ''); // remove trailing = for empty params + const canonicalizedAmzHeaders = Object.keys(ctx.headers) + .filter( + headerName => + headerName.startsWith('x-amz-') && + headerName !== 'x-amz-website-redirect-location', + ) + .sort() + .map( + headerName => + `${headerName}:${ctx.get(headerName).replace(/ +/g, ' ')}`, + ); + const canonicalRequest = { method: ctx.method === 'OPTIONS' @@ -121,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, }; diff --git a/lib/routes.js b/lib/routes.js index 48b2fa17..a96d7dc7 100644 --- a/lib/routes.js +++ b/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(); From 3f07ca7e51fe48671237ce53149dba3d36c96e5b Mon Sep 17 00:00:00 2001 From: Kyle Herock Date: Sun, 7 Jun 2020 02:06:44 -0400 Subject: [PATCH 5/8] use correct RFC3986 escaping for canonicalizing resource URI --- lib/middleware/authentication.js | 10 +++++++++- lib/utils.js | 18 ++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/lib/middleware/authentication.js b/lib/middleware/authentication.js index 06b29a78..e69116af 100644 --- a/lib/middleware/authentication.js +++ b/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, @@ -99,6 +103,10 @@ module.exports = () => } else { canonicalizedResource += '/'; } + canonicalizedResource = canonicalizedResource + .split('/') + .map(encodeURIComponentRFC3986) + .join('/'); const canonicalizedQueryString = Object.entries(query) .filter( diff --git a/lib/utils.js b/lib/utils.js index 1361600e..ebb29106 100644 --- a/lib/utils.js +++ b/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); From 865f4a08722e2d58beca7f53a4f55d8d89e872f7 Mon Sep 17 00:00:00 2001 From: Kyle Herock Date: Sun, 7 Jun 2020 02:30:38 -0400 Subject: [PATCH 6/8] fix signature verification of camelCased subresource URIs --- lib/middleware/authentication.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/middleware/authentication.js b/lib/middleware/authentication.js index e69116af..aa522319 100644 --- a/lib/middleware/authentication.js +++ b/lib/middleware/authentication.js @@ -55,9 +55,8 @@ module.exports = () => return next(); } - const query = mapKeys(ctx.query, (value, key) => key.toLowerCase()); const amzQueryHeaders = pickBy( - query, + mapKeys(ctx.query, (value, key) => key.toLowerCase()), (value, key) => key.startsWith('x-amz-') && // x-amz-website-redirect-location isn't a canonical header @@ -108,7 +107,7 @@ module.exports = () => .map(encodeURIComponentRFC3986) .join('/'); - const canonicalizedQueryString = Object.entries(query) + const canonicalizedQueryString = Object.entries(ctx.query) .filter( ([param]) => SUBRESOURCES[param] || From a8546892c320b562228f788b0f8031c05477e0c2 Mon Sep 17 00:00:00 2001 From: Kyle Herock Date: Sun, 7 Jun 2020 02:31:17 -0400 Subject: [PATCH 7/8] include x-amz-website-redirect-location as canonical header --- lib/middleware/authentication.js | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/lib/middleware/authentication.js b/lib/middleware/authentication.js index aa522319..ceac7b89 100644 --- a/lib/middleware/authentication.js +++ b/lib/middleware/authentication.js @@ -57,10 +57,7 @@ module.exports = () => const amzQueryHeaders = pickBy( mapKeys(ctx.query, (value, key) => key.toLowerCase()), - (value, key) => - key.startsWith('x-amz-') && - // x-amz-website-redirect-location isn't a canonical header - key !== 'x-amz-website-redirect-location', + (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); @@ -125,11 +122,7 @@ module.exports = () => .replace(/=(&|$)/g, ''); // remove trailing = for empty params const canonicalizedAmzHeaders = Object.keys(ctx.headers) - .filter( - headerName => - headerName.startsWith('x-amz-') && - headerName !== 'x-amz-website-redirect-location', - ) + .filter(headerName => headerName.startsWith('x-amz-')) .sort() .map( headerName => From 7e38a25b360d3b50afffb69afa82fc490dba3e11 Mon Sep 17 00:00:00 2001 From: Kyle Herock Date: Sun, 7 Jun 2020 03:13:18 -0400 Subject: [PATCH 8/8] additional test cleanup --- test/controllers/object.spec.js | 57 +++++++++++---------------------- 1 file changed, 19 insertions(+), 38 deletions(-) diff --git a/test/controllers/object.spec.js b/test/controllers/object.spec.js index 98368da2..a76d39af 100644 --- a/test/controllers/object.spec.js +++ b/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) {