Skip to content

Commit d3076cc

Browse files
tcarmettcarmet
committed
Merge remote-tracking branch 'origin/w/9.4/bugfix/CLDSRV-1039-versioned-copy-source-authorization' into w/9.5/bugfix/CLDSRV-1039-versioned-copy-source-authorization
# Conflicts: # lib/api/apiUtils/quotas/quotaUtils.js
2 parents b0ec588 + f3646bd commit d3076cc

8 files changed

Lines changed: 515 additions & 5 deletions

File tree

‎lib/api/apiUtils/quotas/quotaUtils.js‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
const async = require('async');
22
const { errors } = require('@scality/arsenal');
33
const monitoring = require('../../../utilities/monitoringHandler');
4-
const { actionNeedQuotaCheckCopy, actionNeedQuotaCheck, actionWithDataDeletion } = require('@scality/arsenal').policies;
4+
const { actionNeedQuotaCheck, actionWithDataDeletion } = require('@scality/arsenal').policies;
55
const { config } = require('../../../Config');
66
const QuotaService = require('../../../utilization/instance');
77

@@ -265,8 +265,12 @@ function validateQuotas(request, bucket, account, apiNames, apiMethod, inflight,
265265
// meaning the source object, containing the current bytes,
266266
// is checked second. This logic handles these APIs calls by
267267
// ensuring the bytes are positives (i.e., not an object
268-
// replacement).
269-
if (actionNeedQuotaCheckCopy(apiName, apiMethod)) {
268+
// replacement). The source is checked with objectGetVersion
269+
// instead of objectGet when the copy specifies its version.
270+
const isCopySource =
271+
(apiName === 'objectGet' || apiName === 'objectGetVersion') &&
272+
(apiMethod === 'objectCopy' || apiMethod === 'objectPutCopyPart');
273+
if (isCopySource) {
270274
// eslint-disable-next-line no-param-reassign
271275
inflight = Math.abs(inflight);
272276
} else if (!actionNeedQuotaCheck[apiName] && !actionWithDataDeletion[apiName]) {

‎lib/api/objectCopy.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -464,7 +464,8 @@ function objectCopy(authInfo, request, sourceBucket, sourceObject, sourceVersion
464464
objectKey: sourceObject,
465465
versionId: sourceVersionId,
466466
getDeleteMarker: true,
467-
requestType: 'objectGet',
467+
// Same action as the one authorized by Vault for the source
468+
requestType: sourceVersionId ? 'objectGetVersion' : 'objectGet',
468469
/**
469470
* Authorization will first check the target object, with an objectPut
470471
* action. But in this context, the source object metadata is still

‎lib/api/objectPutCopyPart.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,8 @@ function objectPutCopyPart(authInfo, request, sourceBucket, sourceObject, reqVer
146146
objectKey: sourceObject,
147147
versionId: reqVersionId,
148148
getDeleteMarker: true,
149-
requestType: 'objectGet',
149+
// Same action as the one authorized by Vault for the source
150+
requestType: reqVersionId ? 'objectGetVersion' : 'objectGet',
150151
/**
151152
* Authorization will first check the target object, with an objectPut
152153
* action. But in this context, the source object metadata is still

‎tests/functional/aws-node-sdk/lib/utility/iam-util.js‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
const { S3Client } = require('@aws-sdk/client-s3');
2+
const {
3+
IAMClient,
4+
CreateUserCommand,
5+
CreateAccessKeyCommand,
6+
DeleteAccessKeyCommand,
7+
DeleteUserCommand,
8+
CreatePolicyCommand,
9+
AttachUserPolicyCommand,
10+
DetachUserPolicyCommand,
11+
DeletePolicyCommand,
12+
} = require('@aws-sdk/client-iam');
13+
const { v4: uuid } = require('uuid');
14+
15+
const getConfig = require('../../test/support/config');
16+
const { config } = require('../../../../../lib/Config');
17+
18+
const vaultHost = config.vaultd?.host || 'localhost';
19+
20+
function makeIamClient(profile) {
21+
const iamConfig = getConfig(profile, { region: 'us-east-1' });
22+
iamConfig.endpoint = `http://${vaultHost}:8600`; // define outside of getConfig for Integration
23+
return new IAMClient(iamConfig);
24+
}
25+
26+
// Creates an IAM user with one managed policy, returns its S3 client and a
27+
// function removing the user
28+
async function createUserWithPolicy(iamClient, statements) {
29+
const name = `test-user-${uuid()}`;
30+
const policyResponse = await iamClient.send(
31+
new CreatePolicyCommand({
32+
PolicyName: name,
33+
PolicyDocument: JSON.stringify({ Version: '2012-10-17', Statement: statements }),
34+
}),
35+
);
36+
const policyArn = policyResponse.Policy.Arn;
37+
await iamClient.send(new CreateUserCommand({ UserName: name }));
38+
await iamClient.send(new AttachUserPolicyCommand({ UserName: name, PolicyArn: policyArn }));
39+
const accessKeyResponse = await iamClient.send(new CreateAccessKeyCommand({ UserName: name }));
40+
const { AccessKeyId, SecretAccessKey } = accessKeyResponse.AccessKey;
41+
const s3Client = new S3Client(
42+
getConfig('default', {
43+
credentials: {
44+
accessKeyId: AccessKeyId,
45+
secretAccessKey: SecretAccessKey,
46+
},
47+
}),
48+
);
49+
async function deleteUser() {
50+
await iamClient.send(new DeleteAccessKeyCommand({ UserName: name, AccessKeyId }));
51+
await iamClient.send(new DetachUserPolicyCommand({ UserName: name, PolicyArn: policyArn }));
52+
await iamClient.send(new DeletePolicyCommand({ PolicyArn: policyArn }));
53+
await iamClient.send(new DeleteUserCommand({ UserName: name }));
54+
}
55+
return { s3Client, deleteUser };
56+
}
57+
58+
module.exports = {
59+
makeIamClient,
60+
createUserWithPolicy,
61+
};

‎tests/functional/aws-node-sdk/test/object/copySourceVersionAuthorization.js‎

Lines changed: 226 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,226 @@
1+
const assert = require('assert');
2+
const {
3+
PutObjectCommand,
4+
GetObjectCommand,
5+
CopyObjectCommand,
6+
CreateMultipartUploadCommand,
7+
UploadPartCopyCommand,
8+
AbortMultipartUploadCommand,
9+
PutBucketPolicyCommand,
10+
DeleteBucketPolicyCommand,
11+
PutBucketVersioningCommand,
12+
} = require('@aws-sdk/client-s3');
13+
const { v4: uuid } = require('uuid');
14+
15+
const BucketUtility = require('../../lib/utility/bucket-util');
16+
const { makeIamClient, createUserWithPolicy } = require('../../lib/utility/iam-util');
17+
const { config } = require('../../../../../lib/Config');
18+
19+
// this test needs a real vault to create IAM users
20+
const isVaultScality = config.backends.auth !== 'mem';
21+
22+
const describeWithVault = isVaultScality ? describe : describe.skip;
23+
24+
describeWithVault('Copy of a specific source version', () => {
25+
const secretKey = 'secret';
26+
27+
async function assertAccessDenied(promise) {
28+
await assert.rejects(promise, { name: 'AccessDenied' });
29+
}
30+
31+
describe('in the same account, without IAM permission to read the source', () => {
32+
const ownBucketName = `copy-source-own-${uuid()}`;
33+
const bucketUtil = new BucketUtility('default');
34+
const iamClient = makeIamClient('default');
35+
const mpuKey = 'mine/mpu-copy';
36+
let user;
37+
let uploadId;
38+
39+
before(async () => {
40+
await bucketUtil.createOne(ownBucketName);
41+
await bucketUtil.s3.send(
42+
new PutObjectCommand({
43+
Bucket: ownBucketName,
44+
Key: secretKey,
45+
Body: 'secret content',
46+
}),
47+
);
48+
user = await createUserWithPolicy(iamClient, [
49+
{
50+
Effect: 'Allow',
51+
Action: ['s3:GetObject', 's3:PutObject'],
52+
Resource: `arn:aws:s3:::${ownBucketName}/mine/*`,
53+
},
54+
]);
55+
const mpuResponse = await user.s3Client.send(
56+
new CreateMultipartUploadCommand({
57+
Bucket: ownBucketName,
58+
Key: mpuKey,
59+
}),
60+
);
61+
uploadId = mpuResponse.UploadId;
62+
});
63+
64+
after(async () => {
65+
if (uploadId) {
66+
await bucketUtil.s3.send(
67+
new AbortMultipartUploadCommand({
68+
Bucket: ownBucketName,
69+
Key: mpuKey,
70+
UploadId: uploadId,
71+
}),
72+
);
73+
}
74+
await bucketUtil.empty(ownBucketName);
75+
await bucketUtil.deleteOne(ownBucketName);
76+
if (user) {
77+
await user.deleteUser();
78+
}
79+
});
80+
81+
it('should deny CopyObject from the latest version', async () => {
82+
await assertAccessDenied(
83+
user.s3Client.send(
84+
new CopyObjectCommand({
85+
Bucket: ownBucketName,
86+
Key: 'mine/copy',
87+
CopySource: `${ownBucketName}/${secretKey}`,
88+
}),
89+
),
90+
);
91+
});
92+
93+
it('should deny CopyObject from the null version', async () => {
94+
await assertAccessDenied(
95+
user.s3Client.send(
96+
new CopyObjectCommand({
97+
Bucket: ownBucketName,
98+
Key: 'mine/copy',
99+
CopySource: `${ownBucketName}/${secretKey}?versionId=null`,
100+
}),
101+
),
102+
);
103+
});
104+
105+
it('should deny UploadPartCopy from the null version', async () => {
106+
await assertAccessDenied(
107+
user.s3Client.send(
108+
new UploadPartCopyCommand({
109+
Bucket: ownBucketName,
110+
Key: mpuKey,
111+
UploadId: uploadId,
112+
PartNumber: 1,
113+
CopySource: `${ownBucketName}/${secretKey}?versionId=null`,
114+
}),
115+
),
116+
);
117+
});
118+
});
119+
120+
describe('with a bucket policy denying s3:GetObjectVersion', () => {
121+
const policyBucketName = `copy-source-policy-${uuid()}`;
122+
const bucketUtil = new BucketUtility('default');
123+
const iamClient = makeIamClient('default');
124+
const latestContent = 'latest content';
125+
let user;
126+
let oldVersionId;
127+
128+
before(async () => {
129+
await bucketUtil.createOne(policyBucketName);
130+
await bucketUtil.s3.send(
131+
new PutBucketVersioningCommand({
132+
Bucket: policyBucketName,
133+
VersioningConfiguration: { Status: 'Enabled' },
134+
}),
135+
);
136+
const oldPut = await bucketUtil.s3.send(
137+
new PutObjectCommand({
138+
Bucket: policyBucketName,
139+
Key: secretKey,
140+
Body: 'old content',
141+
}),
142+
);
143+
oldVersionId = oldPut.VersionId;
144+
await bucketUtil.s3.send(
145+
new PutObjectCommand({
146+
Bucket: policyBucketName,
147+
Key: secretKey,
148+
Body: latestContent,
149+
}),
150+
);
151+
await bucketUtil.s3.send(
152+
new PutBucketPolicyCommand({
153+
Bucket: policyBucketName,
154+
Policy: JSON.stringify({
155+
Version: '2012-10-17',
156+
Statement: [
157+
{
158+
Effect: 'Deny',
159+
Principal: '*',
160+
Action: 's3:GetObjectVersion',
161+
Resource: `arn:aws:s3:::${policyBucketName}/*`,
162+
},
163+
],
164+
}),
165+
}),
166+
);
167+
user = await createUserWithPolicy(iamClient, [
168+
{
169+
Effect: 'Allow',
170+
Action: ['s3:GetObject', 's3:GetObjectVersion', 's3:PutObject'],
171+
Resource: `arn:aws:s3:::${policyBucketName}/*`,
172+
},
173+
]);
174+
});
175+
176+
after(async () => {
177+
await bucketUtil.s3.send(new DeleteBucketPolicyCommand({ Bucket: policyBucketName }));
178+
await bucketUtil.empty(policyBucketName);
179+
await bucketUtil.deleteOne(policyBucketName);
180+
if (user) {
181+
await user.deleteUser();
182+
}
183+
});
184+
185+
it('should deny GetObject of an older version', async () => {
186+
await assertAccessDenied(
187+
user.s3Client.send(
188+
new GetObjectCommand({
189+
Bucket: policyBucketName,
190+
Key: secretKey,
191+
VersionId: oldVersionId,
192+
}),
193+
),
194+
);
195+
});
196+
197+
it('should deny CopyObject from an older version', async () => {
198+
await assertAccessDenied(
199+
user.s3Client.send(
200+
new CopyObjectCommand({
201+
Bucket: policyBucketName,
202+
Key: 'leak',
203+
CopySource: `${policyBucketName}/${secretKey}?versionId=${oldVersionId}`,
204+
}),
205+
),
206+
);
207+
});
208+
209+
it('should allow CopyObject from the latest version', async () => {
210+
await user.s3Client.send(
211+
new CopyObjectCommand({
212+
Bucket: policyBucketName,
213+
Key: 'latest-copy',
214+
CopySource: `${policyBucketName}/${secretKey}`,
215+
}),
216+
);
217+
const getResponse = await user.s3Client.send(
218+
new GetObjectCommand({
219+
Bucket: policyBucketName,
220+
Key: 'latest-copy',
221+
}),
222+
);
223+
assert.strictEqual(await getResponse.Body.transformToString(), latestContent);
224+
});
225+
});
226+
});

‎tests/unit/api/apiUtils/quotas/quotaUtils.js‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,19 @@ describe('validateQuotas (buckets)', () => {
114114
});
115115
});
116116

117+
['objectCopy', 'objectPutCopyPart'].forEach(apiMethod => {
118+
it(`should return errors.QuotaExceeded if quota is exceeded by a ${apiMethod} of a source version`, done => {
119+
getLatestMetricsStub.resolves({ bytesTotal: BigInt(150) });
120+
121+
validateQuotas(request, mockBucket, {}, ['objectGetVersion'], apiMethod, 1, false, mockLog, err => {
122+
assert(err, 'expected the quota to be evaluated');
123+
assert.strictEqual(err.is.QuotaExceeded, true);
124+
assert.strictEqual(getLatestMetricsStub.callCount, 1);
125+
done();
126+
});
127+
});
128+
});
129+
117130
it('should not return QuotaExceeded if quotas are exceeded but operation is creating a delete marker', done => {
118131
const result1 = {
119132
bytesTotal: BigInt(150),

0 commit comments

Comments
 (0)