diff --git a/constants.js b/constants.js index 925a8ec6c3..e4d4952852 100644 --- a/constants.js +++ b/constants.js @@ -221,6 +221,7 @@ const constants = { 'versionId', 'isNull', 'isDeleteMarker', + 'bucketOwnerId', ], unsupportedSignatureChecksums: new Set([ 'STREAMING-AWS4-HMAC-SHA256-PAYLOAD-TRAILER', diff --git a/lib/api/apiUtils/object/createAndStoreObject.js b/lib/api/apiUtils/object/createAndStoreObject.js index e7b273d333..2f94851fe4 100644 --- a/lib/api/apiUtils/object/createAndStoreObject.js +++ b/lib/api/apiUtils/object/createAndStoreObject.js @@ -193,6 +193,10 @@ function createAndStoreObject(bucketName, bucketMD, objectKey, objMD, authInfo, metadataStoreParams.oldReplayId = objMD.uploadId; } + if (authInfo.getCanonicalID() !== bucketMD.getOwner()) { + metadataStoreParams.bucketOwnerId = bucketMD.getOwner(); + } + /* eslint-disable camelcase */ const dontSkipBackend = externalBackends; /* eslint-enable camelcase */ diff --git a/lib/api/completeMultipartUpload.js b/lib/api/completeMultipartUpload.js index 5a7f8b5f84..b1be2b8275 100644 --- a/lib/api/completeMultipartUpload.js +++ b/lib/api/completeMultipartUpload.js @@ -382,6 +382,11 @@ function completeMultipartUpload(authInfo, request, log, callback) { }; setSSEHeaders(responseHeaders, serverSideEncryption, kmsKey); } + + if (authInfo.getCanonicalID() !== destBucket.getOwner()) { + metaStoreParams.bucketOwnerId = destBucket.getOwner(); + } + // if x-scal-s3-version-id header is specified, we overwrite the object/version metadata. if (isPutVersion) { const options = overwritingVersioning(objMD, metaStoreParams); diff --git a/lib/api/objectCopy.js b/lib/api/objectCopy.js index e3ae55d595..d948c6e7c9 100644 --- a/lib/api/objectCopy.js +++ b/lib/api/objectCopy.js @@ -191,6 +191,11 @@ function _prepMetadata(request, sourceObjMD, headers, sourceIsDestination, if (!storeMetadataParams.contentType) { storeMetadataParams.contentType = sourceObjMD['content-type']; } + + if (authInfo.getCanonicalID() !== destBucketMD.getOwner()) { + storeMetadataParams.bucketOwnerId = destBucketMD.getOwner(); + } + return { storeMetadataParams, sourceLocationConstraintName, backendInfoDest: backendInfoObjDest.backendInfo }; } diff --git a/lib/api/objectPutCopyPart.js b/lib/api/objectPutCopyPart.js index 851f01b1b4..ed43082252 100644 --- a/lib/api/objectPutCopyPart.js +++ b/lib/api/objectPutCopyPart.js @@ -350,6 +350,7 @@ function objectPutCopyPart(authInfo, request, sourceBucket, splitter: constants.splitter, lastModified, overheadField: constants.overheadField, + ownerId: destBucketMD.getOwner(), }; return services.metadataStorePart(mpuBucketName, locations, metaStoreParams, log, err => { diff --git a/lib/services.js b/lib/services.js index ce11396dcf..a312137e5e 100644 --- a/lib/services.js +++ b/lib/services.js @@ -110,7 +110,7 @@ const services = { dataStoreName, creationTime, retentionMode, retentionDate, legalHold, originOp, updateMicroVersionId, archive, oldReplayId, deleteNullKey, amzStorageClass, overheadField, needOplogUpdate, - restoredEtag } = params; + restoredEtag, bucketOwnerId } = params; log.trace('storing object in metadata'); assert.strictEqual(typeof bucketName, 'string'); const md = new ObjectMD(); @@ -275,6 +275,10 @@ const services = { md.setAcl(params.acl); } + if (bucketOwnerId) { + md.setBucketOwnerId(bucketOwnerId); + } + log.trace('object metadata', { omVal: md.getValue() }); // If this is not the completion of a multipart upload or // the creation of a delete marker, parse the headers to @@ -765,7 +769,7 @@ const services = { metadataStorePart(mpuBucketName, partLocations, metaStoreParams, log, cb) { assert.strictEqual(typeof mpuBucketName, 'string'); - const { partNumber, contentMD5, size, uploadId, lastModified, splitter, overheadField } + const { partNumber, contentMD5, size, uploadId, lastModified, splitter, overheadField, ownerId } = metaStoreParams; const dateModified = typeof lastModified === 'string' ? lastModified : new Date().toJSON(); @@ -780,6 +784,7 @@ const services = { 'last-modified': dateModified, 'content-md5': contentMD5, 'content-length': size, + 'owner-id': ownerId, }; const params = {}; diff --git a/package.json b/package.json index c4ad40744f..dfa8b356ec 100644 --- a/package.json +++ b/package.json @@ -21,7 +21,7 @@ "dependencies": { "@azure/storage-blob": "^12.12.0", "@hapi/joi": "^17.1.0", - "arsenal": "git+https://github.com/scality/arsenal#8.1.158", + "arsenal": "git+https://github.com/scality/arsenal#8.1.159", "async": "~2.5.0", "aws-sdk": "2.905.0", "bucketclient": "scality/bucketclient#8.1.14", diff --git a/tests/unit/DummyRequest.js b/tests/unit/DummyRequest.js index 28b21337eb..6189fb8eb0 100644 --- a/tests/unit/DummyRequest.js +++ b/tests/unit/DummyRequest.js @@ -26,6 +26,11 @@ class DummyRequest extends http.IncomingMessage { } this.push(null); } + + _destroy(err, cb) { + // this is a no-op + cb(); + } } module.exports = DummyRequest; diff --git a/tests/unit/api/multipartUpload.js b/tests/unit/api/multipartUpload.js index 4d4d37e0d1..1e8451f28b 100644 --- a/tests/unit/api/multipartUpload.js +++ b/tests/unit/api/multipartUpload.js @@ -36,6 +36,7 @@ const log = new DummyRequestLogger(); const splitter = constants.splitter; const canonicalID = 'accessKey1'; const authInfo = makeAuthInfo(canonicalID); +const authInfoOtherAcc = makeAuthInfo('accessKey2'); const namespace = 'default'; const bucketName = 'bucketname'; const lockedBucket = 'objectlockenabledbucket'; @@ -2559,9 +2560,14 @@ describe('complete mpu with bucket policy', () => { beforeEach(done => { cleanup(); + sinon.spy(metadataswitch, 'putObjectMD'); bucketPut(authInfo, bucketPutRequest, log, done); }); + afterEach(() => { + sinon.restore(); + }); + it('should complete with a deny on unrelated object as non root', done => { const bucketPutPolicyRequest = getPolicyRequest({ Version: '2012-10-17', @@ -2628,4 +2634,94 @@ describe('complete mpu with bucket policy', () => { done(); }); }); + + it('should set bucketOwnerId if requester is not destination bucket owner', done => { + const partBody = Buffer.from('I am a part\n', 'utf8'); + const bucketPutPolicyRequest = getPolicyRequest({ + Version: '2012-10-17', + Statement: [ + { + Effect: 'Allow', + Principal: { AWS: `arn:aws:iam::${authInfoOtherAcc.shortid}:root` }, + Action: ['s3:*'], + Resource: `arn:aws:s3:::${bucketName}/*`, + }, + ], + }); + async.waterfall([ + next => bucketPutPolicy(authInfo, bucketPutPolicyRequest, log, next), + (corsHeaders, next) => initiateMultipartUpload(authInfoOtherAcc, + initiateReqFixed, log, next), + (result, corsHeaders, next) => parseString(result, next), + ], + (err, json) => { + // Need to build request in here since do not have uploadId + // until here + assert.ifError(err); + const testUploadId = + json.InitiateMultipartUploadResult.UploadId[0]; + const md5Hash = crypto.createHash('md5').update(partBody); + const calculatedHash = md5Hash.digest('hex'); + const partRequest = new DummyRequest(Object.assign({ + bucketName, + namespace, + objectKey, + headers: { host: `${bucketName}.s3.amazonaws.com` }, + url: `/${objectKey}?partNumber=1&uploadId=${testUploadId}`, + query: { + partNumber: '1', + uploadId: testUploadId, + }, + // Note that the body of the post set in the request here does + // not really matter in this test. + // The put is not going through the route so the md5 is being + // calculated above and manually being set in the request below. + // What is being tested is that the calculatedHash being sent + // to the API for the part is stored and then used to + // calculate the final ETag upon completion + // of the multipart upload. + calculatedHash, + socket: { + remoteAddress: '1.1.1.1', + }, + }, requestFix), partBody); + objectPutPart(authInfoOtherAcc, partRequest, undefined, log, err => { + assert.ifError(err); + const completeBody = '' + + '' + + '1' + + `"${calculatedHash}"` + + '' + + ''; + const completeRequest = new DummyRequest(Object.assign({ + bucketName, + namespace, + objectKey, + parsedHost: 's3.amazonaws.com', + url: `/${objectKey}?uploadId=${testUploadId}`, + headers: { host: `${bucketName}.s3.amazonaws.com` }, + query: { uploadId: testUploadId }, + post: completeBody, + actionImplicitDenies: false, + socket: { + remoteAddress: '1.1.1.1', + }, + }, requestFix)); + completeMultipartUpload(authInfoOtherAcc, + completeRequest, log, err => { + assert.ifError(err); + sinon.assert.calledWith( + metadataswitch.putObjectMD.lastCall, + bucketName, + objectKey, + sinon.match({ bucketOwnerId: authInfo.canonicalId }), + sinon.match.any, + sinon.match.any, + sinon.match.any + ); + done(); + }); + }); + }); + }); }); diff --git a/tests/unit/api/objectCopy.js b/tests/unit/api/objectCopy.js index 9e96fd0b33..cb268f746a 100644 --- a/tests/unit/api/objectCopy.js +++ b/tests/unit/api/objectCopy.js @@ -5,6 +5,7 @@ const sinon = require('sinon'); const { bucketPut } = require('../../../lib/api/bucketPut'); const bucketPutVersioning = require('../../../lib/api/bucketPutVersioning'); +const bucketPutPolicy = require('../../../lib/api/bucketPutPolicy'); const objectPut = require('../../../lib/api/objectPut'); const objectCopy = require('../../../lib/api/objectCopy'); const DummyRequest = require('../DummyRequest'); @@ -42,6 +43,7 @@ function _createObjectCopyRequest(destBucketName) { objectKey, headers: {}, url: `/${destBucketName}/${objectKey}`, + socket: {}, }; return new DummyRequest(params); } @@ -65,6 +67,7 @@ describe('objectCopy with versioning', () => { before(done => { cleanup(); + sinon.spy(metadata, 'putObjectMD'); async.series([ callback => bucketPut(authInfo, putDestBucketRequest, log, callback), @@ -93,7 +96,10 @@ describe('objectCopy with versioning', () => { }); }); - after(() => cleanup()); + after(() => { + metadata.putObjectMD.restore(); + cleanup(); + }); it('should delete null version when creating new null version, ' + 'even when null version is not the latest version', done => { @@ -122,6 +128,94 @@ describe('objectCopy with versioning', () => { }); }); }); + + it('should not set bucketOwnerId if requesting account owns dest bucket', done => { + const testObjectCopyRequest = _createObjectCopyRequest(destBucketName); + objectCopy(authInfo, testObjectCopyRequest, sourceBucketName, objectKey, + undefined, log, err => { + assert.ifError(err); + sinon.assert.calledWith( + metadata.putObjectMD.lastCall, + destBucketName, + objectKey, + sinon.match({ _data: { bucketOwnerId: sinon.match.typeOf('undefined') } }), + sinon.match.any, + sinon.match.any, + sinon.match.any + ); + done(); + }); + }); + + // TODO: S3C-9965 + // Skipped because the policy is not checked correctly + // When source bucket policy is checked destination arn is used + it.skip('should set bucketOwnerId if requesting account differs from dest bucket owner', done => { + const authInfo2 = makeAuthInfo('accessKey2'); + const testObjectCopyRequest = _createObjectCopyRequest(destBucketName); + const testPutSrcPolicyRequest = new DummyRequest({ + bucketName: sourceBucketName, + namespace, + headers: { host: `${sourceBucketName}.s3.amazonaws.com` }, + url: '/', + socket: {}, + post: JSON.stringify({ + Version: '2012-10-17', + Statement: [ + { + Sid: 'AllowCrossAccountRead', + Effect: 'Allow', + Principal: { AWS: `arn:aws:iam::${authInfo2.shortid}:root` }, + Action: ['s3:GetObject'], + Resource: [ + `arn:aws:s3:::${sourceBucketName}/*`, + ], + }, + ], + }), + }); + const testPutDestPolicyRequest = new DummyRequest({ + bucketName: destBucketName, + namespace, + headers: { host: `${destBucketName}.s3.amazonaws.com` }, + url: '/', + socket: {}, + post: JSON.stringify({ + Version: '2012-10-17', + Statement: [ + { + Sid: 'AllowCrossAccountWrite', + Effect: 'Allow', + Principal: { AWS: `arn:aws:iam::${authInfo2.shortid}:root` }, + Action: ['s3:PutObject'], + Resource: [ + `arn:aws:s3:::${destBucketName}/*`, + ], + }, + ], + }), + }); + bucketPutPolicy(authInfo, testPutSrcPolicyRequest, log, err => { + assert.ifError(err); + bucketPutPolicy(authInfo, testPutDestPolicyRequest, log, err => { + assert.ifError(err); + objectCopy(authInfo2, testObjectCopyRequest, sourceBucketName, objectKey, + undefined, log, err => { + sinon.assert.calledWith( + metadata.putObjectMD.lastCall, + destBucketName, + objectKey, + sinon.match({ _data: { bucketOwnerId: authInfo.canonicalID } }), + sinon.match.any, + sinon.match.any, + sinon.match.any + ); + assert.ifError(err); + done(); + }); + }); + }); + }); }); describe('non-versioned objectCopy', () => { diff --git a/tests/unit/api/objectCopyPart.js b/tests/unit/api/objectCopyPart.js index 88df569abc..46c95a0452 100644 --- a/tests/unit/api/objectCopyPart.js +++ b/tests/unit/api/objectCopyPart.js @@ -137,4 +137,21 @@ describe('objectCopyPart', () => { done(); }); }); + + it('should set owner-id to the canonicalId of the dest bucket owner', done => { + const testObjectCopyRequest = _createObjectCopyPartRequest(destBucketName, uploadId); + objectPutCopyPart(authInfo, testObjectCopyRequest, sourceBucketName, objectKey, undefined, log, err => { + assert.ifError(err); + sinon.assert.calledWith( + metadataswitch.putObjectMD.lastCall, + sinon.match.string, // MPU shadow bucket + `${uploadId}..|..00001`, + sinon.match({ 'owner-id': authInfo.canonicalID }), + sinon.match.any, + sinon.match.any, + sinon.match.any + ); + done(); + }); + }); }); diff --git a/tests/unit/api/objectPut.js b/tests/unit/api/objectPut.js index ccf6f95bc9..eb964679d5 100644 --- a/tests/unit/api/objectPut.js +++ b/tests/unit/api/objectPut.js @@ -8,6 +8,7 @@ const { bucketPut } = require('../../../lib/api/bucketPut'); const bucketPutObjectLock = require('../../../lib/api/bucketPutObjectLock'); const bucketPutACL = require('../../../lib/api/bucketPutACL'); const bucketPutVersioning = require('../../../lib/api/bucketPutVersioning'); +const bucketPutPolicy = require('../../../lib/api/bucketPutPolicy'); const { parseTagFromQuery } = s3middleware.tagging; const { cleanup, DummyRequestLogger, makeAuthInfo, versioningTestUtils } = require('../helpers'); @@ -666,6 +667,86 @@ describe('objectPut API', () => { }); }); }); + + it('should not set bucketOwnerId if requester owns the bucket', done => { + const testPutObjectRequest = new DummyRequest({ + bucketName, + namespace, + objectKey: objectName, + headers: {}, + url: `/${bucketName}/${objectName}`, + calculatedHash: 'vnR+tLdVF79rPPfF+7YvOg==', + }, postBody); + + bucketPut(authInfo, testPutBucketRequest, log, () => { + objectPut(authInfo, testPutObjectRequest, undefined, log, + err => { + assert.ifError(err); + sinon.assert.calledWith(metadata.putObjectMD.lastCall, + bucketName, + objectName, + sinon.match({ bucketOwnerId: sinon.match.typeOf('undefined') }), + any, + any, + any + ); + done(); + } + ); + }); + }); + + it('should set bucketOwnerId if requester does not own the bucket', done => { + const authInfo2 = makeAuthInfo('accessKey2'); + + const testPutObjectRequest = new DummyRequest({ + bucketName, + namespace, + objectKey: objectName, + headers: {}, + url: `/${bucketName}/${objectName}`, + calculatedHash: 'vnR+tLdVF79rPPfF+7YvOg==', + }, postBody); + + const testPutPolicyRequest = new DummyRequest({ + bucketName, + namespace, + headers: { host: `${bucketName}.s3.amazonaws.com` }, + url: '/', + post: JSON.stringify({ + Version: '2012-10-17', + Statement: [ + { + Sid: 'AllowCrossAccountReadWrite', + Effect: 'Allow', + Principal: { AWS: `arn:aws:iam::${authInfo2.shortid}:root` }, + Action: ['s3:PutObject'], + Resource: [`arn:aws:s3:::${bucketName}/*`], + }, + ], + }), + }); + + bucketPut(authInfo, testPutBucketRequest, log, () => { + bucketPutPolicy(authInfo, testPutPolicyRequest, log, err => { + assert.ifError(err); + objectPut(authInfo, testPutObjectRequest, undefined, log, + err => { + assert.ifError(err); + sinon.assert.calledWith(metadata.putObjectMD.lastCall, + bucketName, + objectName, + sinon.match({ bucketOwnerId: authInfo.canonicalId }), + any, + any, + any + ); + done(); + } + ); + }); + }); + }); }); describe('objectPut API with versioning', () => { diff --git a/yarn.lock b/yarn.lock index 5ad5b1d2d0..91df06ab51 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1222,9 +1222,9 @@ arraybuffer.slice@~0.0.7: optionalDependencies: ioctl "^2.0.2" -"arsenal@git+https://github.com/scality/arsenal#8.1.158": - version "8.1.158" - resolved "git+https://github.com/scality/arsenal#92e4c51bb1691d37f289fa20b694d94801d271e5" +"arsenal@git+https://github.com/scality/arsenal#8.1.159": + version "8.1.159" + resolved "git+https://github.com/scality/arsenal#c1ff3cefdc73602fdbcb73d19edbbf0e8de23e99" dependencies: "@azure/identity" "^3.1.1" "@azure/storage-blob" "^12.12.0"