Skip to content

Commit 46471d4

Browse files
BourgoisMickaelBourgoisMickael
committed
Merge remote-tracking branch 'origin/improvement/CLDSRV-621-backport-bucketownerid' into w/8.8/improvement/CLDSRV-621-backport-bucketownerid
2 parents 19e13d4 + 612b824 commit 46471d4

11 files changed

Lines changed: 317 additions & 3 deletions

‎constants.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,7 @@ const constants = {
221221
'versionId',
222222
'isNull',
223223
'isDeleteMarker',
224+
'bucketOwnerId',
224225
],
225226
unsupportedSignatureChecksums: new Set([
226227
'STREAMING-AWS4-HMAC-SHA256-PAYLOAD-TRAILER',

‎lib/api/apiUtils/object/createAndStoreObject.js‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,10 @@ function createAndStoreObject(bucketName, bucketMD, objectKey, objMD, authInfo,
193193
metadataStoreParams.oldReplayId = objMD.uploadId;
194194
}
195195

196+
if (authInfo.getCanonicalID() !== bucketMD.getOwner()) {
197+
metadataStoreParams.bucketOwnerId = bucketMD.getOwner();
198+
}
199+
196200
/* eslint-disable camelcase */
197201
const dontSkipBackend = externalBackends;
198202
/* eslint-enable camelcase */

‎lib/api/completeMultipartUpload.js‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -382,6 +382,11 @@ function completeMultipartUpload(authInfo, request, log, callback) {
382382
};
383383
setSSEHeaders(responseHeaders, serverSideEncryption, kmsKey);
384384
}
385+
386+
if (authInfo.getCanonicalID() !== destBucket.getOwner()) {
387+
metaStoreParams.bucketOwnerId = destBucket.getOwner();
388+
}
389+
385390
// if x-scal-s3-version-id header is specified, we overwrite the object/version metadata.
386391
if (isPutVersion) {
387392
const options = overwritingVersioning(objMD, metaStoreParams);

‎lib/api/objectCopy.js‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -191,6 +191,11 @@ function _prepMetadata(request, sourceObjMD, headers, sourceIsDestination,
191191
if (!storeMetadataParams.contentType) {
192192
storeMetadataParams.contentType = sourceObjMD['content-type'];
193193
}
194+
195+
if (authInfo.getCanonicalID() !== destBucketMD.getOwner()) {
196+
storeMetadataParams.bucketOwnerId = destBucketMD.getOwner();
197+
}
198+
194199
return { storeMetadataParams, sourceLocationConstraintName,
195200
backendInfoDest: backendInfoObjDest.backendInfo };
196201
}

‎lib/api/objectPutCopyPart.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -350,6 +350,7 @@ function objectPutCopyPart(authInfo, request, sourceBucket,
350350
splitter: constants.splitter,
351351
lastModified,
352352
overheadField: constants.overheadField,
353+
ownerId: destBucketMD.getOwner(),
353354
};
354355
return services.metadataStorePart(mpuBucketName,
355356
locations, metaStoreParams, log, err => {

‎lib/services.js‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ const services = {
110110
dataStoreName, creationTime, retentionMode, retentionDate,
111111
legalHold, originOp, updateMicroVersionId, archive, oldReplayId,
112112
deleteNullKey, amzStorageClass, overheadField, needOplogUpdate,
113-
restoredEtag } = params;
113+
restoredEtag, bucketOwnerId } = params;
114114
log.trace('storing object in metadata');
115115
assert.strictEqual(typeof bucketName, 'string');
116116
const md = new ObjectMD();
@@ -275,6 +275,10 @@ const services = {
275275
md.setAcl(params.acl);
276276
}
277277

278+
if (bucketOwnerId) {
279+
md.setBucketOwnerId(bucketOwnerId);
280+
}
281+
278282
log.trace('object metadata', { omVal: md.getValue() });
279283
// If this is not the completion of a multipart upload or
280284
// the creation of a delete marker, parse the headers to
@@ -765,7 +769,7 @@ const services = {
765769
metadataStorePart(mpuBucketName, partLocations,
766770
metaStoreParams, log, cb) {
767771
assert.strictEqual(typeof mpuBucketName, 'string');
768-
const { partNumber, contentMD5, size, uploadId, lastModified, splitter, overheadField }
772+
const { partNumber, contentMD5, size, uploadId, lastModified, splitter, overheadField, ownerId }
769773
= metaStoreParams;
770774
const dateModified = typeof lastModified === 'string' ?
771775
lastModified : new Date().toJSON();
@@ -780,6 +784,7 @@ const services = {
780784
'last-modified': dateModified,
781785
'content-md5': contentMD5,
782786
'content-length': size,
787+
'owner-id': ownerId,
783788
};
784789

785790
const params = {};

‎tests/unit/DummyRequest.js‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,11 @@ class DummyRequest extends http.IncomingMessage {
2626
}
2727
this.push(null);
2828
}
29+
30+
_destroy(err, cb) {
31+
// this is a no-op
32+
cb();
33+
}
2934
}
3035

3136
module.exports = DummyRequest;

‎tests/unit/api/multipartUpload.js‎

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ const log = new DummyRequestLogger();
3636
const splitter = constants.splitter;
3737
const canonicalID = 'accessKey1';
3838
const authInfo = makeAuthInfo(canonicalID);
39+
const authInfoOtherAcc = makeAuthInfo('accessKey2');
3940
const namespace = 'default';
4041
const bucketName = 'bucketname';
4142
const lockedBucket = 'objectlockenabledbucket';
@@ -2559,9 +2560,14 @@ describe('complete mpu with bucket policy', () => {
25592560

25602561
beforeEach(done => {
25612562
cleanup();
2563+
sinon.spy(metadataswitch, 'putObjectMD');
25622564
bucketPut(authInfo, bucketPutRequest, log, done);
25632565
});
25642566

2567+
afterEach(() => {
2568+
sinon.restore();
2569+
});
2570+
25652571
it('should complete with a deny on unrelated object as non root', done => {
25662572
const bucketPutPolicyRequest = getPolicyRequest({
25672573
Version: '2012-10-17',
@@ -2628,4 +2634,94 @@ describe('complete mpu with bucket policy', () => {
26282634
done();
26292635
});
26302636
});
2637+
2638+
it('should set bucketOwnerId if requester is not destination bucket owner', done => {
2639+
const partBody = Buffer.from('I am a part\n', 'utf8');
2640+
const bucketPutPolicyRequest = getPolicyRequest({
2641+
Version: '2012-10-17',
2642+
Statement: [
2643+
{
2644+
Effect: 'Allow',
2645+
Principal: { AWS: `arn:aws:iam::${authInfoOtherAcc.shortid}:root` },
2646+
Action: ['s3:*'],
2647+
Resource: `arn:aws:s3:::${bucketName}/*`,
2648+
},
2649+
],
2650+
});
2651+
async.waterfall([
2652+
next => bucketPutPolicy(authInfo, bucketPutPolicyRequest, log, next),
2653+
(corsHeaders, next) => initiateMultipartUpload(authInfoOtherAcc,
2654+
initiateReqFixed, log, next),
2655+
(result, corsHeaders, next) => parseString(result, next),
2656+
],
2657+
(err, json) => {
2658+
// Need to build request in here since do not have uploadId
2659+
// until here
2660+
assert.ifError(err);
2661+
const testUploadId =
2662+
json.InitiateMultipartUploadResult.UploadId[0];
2663+
const md5Hash = crypto.createHash('md5').update(partBody);
2664+
const calculatedHash = md5Hash.digest('hex');
2665+
const partRequest = new DummyRequest(Object.assign({
2666+
bucketName,
2667+
namespace,
2668+
objectKey,
2669+
headers: { host: `${bucketName}.s3.amazonaws.com` },
2670+
url: `/${objectKey}?partNumber=1&uploadId=${testUploadId}`,
2671+
query: {
2672+
partNumber: '1',
2673+
uploadId: testUploadId,
2674+
},
2675+
// Note that the body of the post set in the request here does
2676+
// not really matter in this test.
2677+
// The put is not going through the route so the md5 is being
2678+
// calculated above and manually being set in the request below.
2679+
// What is being tested is that the calculatedHash being sent
2680+
// to the API for the part is stored and then used to
2681+
// calculate the final ETag upon completion
2682+
// of the multipart upload.
2683+
calculatedHash,
2684+
socket: {
2685+
remoteAddress: '1.1.1.1',
2686+
},
2687+
}, requestFix), partBody);
2688+
objectPutPart(authInfoOtherAcc, partRequest, undefined, log, err => {
2689+
assert.ifError(err);
2690+
const completeBody = '<CompleteMultipartUpload>' +
2691+
'<Part>' +
2692+
'<PartNumber>1</PartNumber>' +
2693+
`<ETag>"${calculatedHash}"</ETag>` +
2694+
'</Part>' +
2695+
'</CompleteMultipartUpload>';
2696+
const completeRequest = new DummyRequest(Object.assign({
2697+
bucketName,
2698+
namespace,
2699+
objectKey,
2700+
parsedHost: 's3.amazonaws.com',
2701+
url: `/${objectKey}?uploadId=${testUploadId}`,
2702+
headers: { host: `${bucketName}.s3.amazonaws.com` },
2703+
query: { uploadId: testUploadId },
2704+
post: completeBody,
2705+
actionImplicitDenies: false,
2706+
socket: {
2707+
remoteAddress: '1.1.1.1',
2708+
},
2709+
}, requestFix));
2710+
completeMultipartUpload(authInfoOtherAcc,
2711+
completeRequest, log, err => {
2712+
assert.ifError(err);
2713+
sinon.assert.calledWith(
2714+
metadataswitch.putObjectMD.lastCall,
2715+
bucketName,
2716+
objectKey,
2717+
sinon.match({ bucketOwnerId: authInfo.canonicalId }),
2718+
sinon.match.any,
2719+
sinon.match.any,
2720+
sinon.match.any
2721+
);
2722+
done();
2723+
});
2724+
});
2725+
});
2726+
});
26312727
});

‎tests/unit/api/objectCopy.js‎

Lines changed: 95 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ const sinon = require('sinon');
55

66
const { bucketPut } = require('../../../lib/api/bucketPut');
77
const bucketPutVersioning = require('../../../lib/api/bucketPutVersioning');
8+
const bucketPutPolicy = require('../../../lib/api/bucketPutPolicy');
89
const objectPut = require('../../../lib/api/objectPut');
910
const objectCopy = require('../../../lib/api/objectCopy');
1011
const DummyRequest = require('../DummyRequest');
@@ -42,6 +43,7 @@ function _createObjectCopyRequest(destBucketName) {
4243
objectKey,
4344
headers: {},
4445
url: `/${destBucketName}/${objectKey}`,
46+
socket: {},
4547
};
4648
return new DummyRequest(params);
4749
}
@@ -65,6 +67,7 @@ describe('objectCopy with versioning', () => {
6567

6668
before(done => {
6769
cleanup();
70+
sinon.spy(metadata, 'putObjectMD');
6871
async.series([
6972
callback => bucketPut(authInfo, putDestBucketRequest, log,
7073
callback),
@@ -93,7 +96,10 @@ describe('objectCopy with versioning', () => {
9396
});
9497
});
9598

96-
after(() => cleanup());
99+
after(() => {
100+
metadata.putObjectMD.restore();
101+
cleanup();
102+
});
97103

98104
it('should delete null version when creating new null version, ' +
99105
'even when null version is not the latest version', done => {
@@ -122,6 +128,94 @@ describe('objectCopy with versioning', () => {
122128
});
123129
});
124130
});
131+
132+
it('should not set bucketOwnerId if requesting account owns dest bucket', done => {
133+
const testObjectCopyRequest = _createObjectCopyRequest(destBucketName);
134+
objectCopy(authInfo, testObjectCopyRequest, sourceBucketName, objectKey,
135+
undefined, log, err => {
136+
assert.ifError(err);
137+
sinon.assert.calledWith(
138+
metadata.putObjectMD.lastCall,
139+
destBucketName,
140+
objectKey,
141+
sinon.match({ _data: { bucketOwnerId: sinon.match.typeOf('undefined') } }),
142+
sinon.match.any,
143+
sinon.match.any,
144+
sinon.match.any
145+
);
146+
done();
147+
});
148+
});
149+
150+
// TODO: S3C-9965
151+
// Skipped because the policy is not checked correctly
152+
// When source bucket policy is checked destination arn is used
153+
it.skip('should set bucketOwnerId if requesting account differs from dest bucket owner', done => {
154+
const authInfo2 = makeAuthInfo('accessKey2');
155+
const testObjectCopyRequest = _createObjectCopyRequest(destBucketName);
156+
const testPutSrcPolicyRequest = new DummyRequest({
157+
bucketName: sourceBucketName,
158+
namespace,
159+
headers: { host: `${sourceBucketName}.s3.amazonaws.com` },
160+
url: '/',
161+
socket: {},
162+
post: JSON.stringify({
163+
Version: '2012-10-17',
164+
Statement: [
165+
{
166+
Sid: 'AllowCrossAccountRead',
167+
Effect: 'Allow',
168+
Principal: { AWS: `arn:aws:iam::${authInfo2.shortid}:root` },
169+
Action: ['s3:GetObject'],
170+
Resource: [
171+
`arn:aws:s3:::${sourceBucketName}/*`,
172+
],
173+
},
174+
],
175+
}),
176+
});
177+
const testPutDestPolicyRequest = new DummyRequest({
178+
bucketName: destBucketName,
179+
namespace,
180+
headers: { host: `${destBucketName}.s3.amazonaws.com` },
181+
url: '/',
182+
socket: {},
183+
post: JSON.stringify({
184+
Version: '2012-10-17',
185+
Statement: [
186+
{
187+
Sid: 'AllowCrossAccountWrite',
188+
Effect: 'Allow',
189+
Principal: { AWS: `arn:aws:iam::${authInfo2.shortid}:root` },
190+
Action: ['s3:PutObject'],
191+
Resource: [
192+
`arn:aws:s3:::${destBucketName}/*`,
193+
],
194+
},
195+
],
196+
}),
197+
});
198+
bucketPutPolicy(authInfo, testPutSrcPolicyRequest, log, err => {
199+
assert.ifError(err);
200+
bucketPutPolicy(authInfo, testPutDestPolicyRequest, log, err => {
201+
assert.ifError(err);
202+
objectCopy(authInfo2, testObjectCopyRequest, sourceBucketName, objectKey,
203+
undefined, log, err => {
204+
sinon.assert.calledWith(
205+
metadata.putObjectMD.lastCall,
206+
destBucketName,
207+
objectKey,
208+
sinon.match({ _data: { bucketOwnerId: authInfo.canonicalID } }),
209+
sinon.match.any,
210+
sinon.match.any,
211+
sinon.match.any
212+
);
213+
assert.ifError(err);
214+
done();
215+
});
216+
});
217+
});
218+
});
125219
});
126220

127221
describe('non-versioned objectCopy', () => {

‎tests/unit/api/objectCopyPart.js‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,4 +137,21 @@ describe('objectCopyPart', () => {
137137
done();
138138
});
139139
});
140+
141+
it('should set owner-id to the canonicalId of the dest bucket owner', done => {
142+
const testObjectCopyRequest = _createObjectCopyPartRequest(destBucketName, uploadId);
143+
objectPutCopyPart(authInfo, testObjectCopyRequest, sourceBucketName, objectKey, undefined, log, err => {
144+
assert.ifError(err);
145+
sinon.assert.calledWith(
146+
metadataswitch.putObjectMD.lastCall,
147+
sinon.match.string, // MPU shadow bucket
148+
`${uploadId}..|..00001`,
149+
sinon.match({ 'owner-id': authInfo.canonicalID }),
150+
sinon.match.any,
151+
sinon.match.any,
152+
sinon.match.any
153+
);
154+
done();
155+
});
156+
});
140157
});

0 commit comments

Comments
 (0)