Skip to content

Commit 8781380

Browse files
leif-scalityleif-scality
committed
Merge remote-tracking branch 'origin/w/8.8/bugfix/CLDSRV-674' into w/9.0/bugfix/CLDSRV-674 
2 parents eb292e2 + 6e48546 commit 8781380

File tree

7 files changed

+1021
-124
lines changed

7 files changed

+1021
-124
lines changed

‎lib/api/api.js‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -160,8 +160,6 @@ const api = {
160160
log.trace('get object authorization denial from Vault');
161161
return errors.AccessDenied;
162162
}
163-
// TODO add support for returnTagCount in the bucket policy
164-
// checks
165163
isImplicitDeny[authResults[0].action] = authResults[0].isImplicit;
166164
// second item checks s3:GetObject(Version)Tagging action
167165
if (!authResults[1].isAllowed) {
@@ -300,6 +298,9 @@ const api = {
300298
sourceObject, sourceVersionId, log, methodCallback);
301299
}
302300
if (apiMethod === 'objectGet') {
301+
// remove objectGetTagging/objectGetTaggingVersion from apiMethods, these were added by
302+
// prepareRequestContexts to determine the value of returnTagCount.
303+
request.apiMethods = request.apiMethods.filter(methodName => !methodName.includes('Tagging'));
303304
return this[apiMethod](userInfo, request, returnTagCount, log, callback);
304305
}
305306
return this[apiMethod](userInfo, request, log, methodCallback);

‎lib/api/apiUtils/authorization/permissionChecks.js‎

Lines changed: 195 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,20 @@ function isRequesterNonAccountUser(authInfo) {
3838
return authInfo.isRequesterAnIAMUser() || isRequesterASessionUser(authInfo);
3939
}
4040

41+
// WARNING: enum order matters DO NOT change.
42+
const checkPrincipalResult = Object.freeze({
43+
KO: 0,
44+
CROSS_ACCOUNT_OK: 1,
45+
OK: 2,
46+
});
47+
48+
const checkBucketPolicyResult = Object.freeze({
49+
DEFAULT_DENY: 0,
50+
EXPLICIT_DENY: 1,
51+
ALLOW: 2,
52+
CROSS_ACCOUNT_ALLOW: 3,
53+
});
54+
4155
/**
4256
* Checks the access control for a given bucket based on the request type and user's canonical ID.
4357
*
@@ -141,7 +155,7 @@ function checkBucketAcls(bucket, requestType, canonicalID, mainApiCall) {
141155
// authorization check should just return true so can move on to check
142156
// rights at the object level.
143157
return (requestTypeParsed === 'objectPutACL' || requestTypeParsed === 'objectGetACL'
144-
|| requestTypeParsed === 'objectGet' || requestTypeParsed === 'objectHead');
158+
|| requestTypeParsed === 'objectGet' || requestTypeParsed === 'objectHead');
145159
}
146160

147161
function checkObjectAcls(bucket, objectMD, requestType, canonicalID, requesterIsNotUser,
@@ -278,51 +292,165 @@ function _isAccountId(principal) {
278292
return (principal.length === 12 && /^\d+$/.test(principal));
279293
}
280294

281-
function _checkPrincipal(requester, principal) {
282-
if (principal === '*') {
283-
return true;
295+
/**
296+
* Checks if the ARN represents a root user account
297+
* @param {string} arn - The ARN to check
298+
* @returns {boolean} True if root user, false otherwise
299+
*/
300+
function _isRootUser(arn) {
301+
if (!arn) {
302+
return false;
284303
}
285-
// User in unauthenticated (anonymous request)
286-
if (requester === undefined) {
304+
305+
// Vault returns the following arn when the account makes requests 'arn:aws:iam::123456789012:/accountName/',
306+
// with an empty resource type ('user/' prefix missing).
307+
const arns = arn.split(':');
308+
if (arns.length < 6) {
287309
return false;
288310
}
289-
if (principal === requester) {
311+
312+
const resource = arns[arns.length - 1];
313+
314+
// If we start with '/' is because we have a empty resource type so we know it is a root account.
315+
if (resource.startsWith('/')) {
290316
return true;
291317
}
292-
if (_isAccountId(principal)) {
293-
return _getAccountId(requester) === principal;
318+
319+
return false;
320+
}
321+
322+
/** _evaluateCrossAccount - checks if it is a cross-account request.
323+
* @param {string} requesterARN - requester ARN
324+
* @param {string} requesterCanonicalID - requester canonical ID
325+
* @param {string} bucketOwnerCanonicalID - bucket owner canonical ID
326+
* @return {checkPrincipalResult} OK if it is not cross-account, CROSS_ACCOUNT_OK otherwise.
327+
*/
328+
function _checkCrossAccount(requesterARN, requesterCanonicalID, bucketOwnerCanonicalID) {
329+
// Vault returns ARNs like 'arn:aws:iam::123456789012:/accountName/' for root accounts
330+
// with an empty resource type (missing 'user/' prefix)
331+
if (!_isRootUser(requesterARN)) {
332+
return bucketOwnerCanonicalID === requesterCanonicalID ?
333+
checkPrincipalResult.OK : checkPrincipalResult.CROSS_ACCOUNT_OK;
294334
}
295-
if (principal.endsWith('root')) {
296-
return _getAccountId(requester) === _getAccountId(principal);
335+
336+
return checkPrincipalResult.OK;
337+
}
338+
339+
function _checkPrincipalWildcard(requestARN, requesterCanonicalID, bucketOwnerCanonicalID) {
340+
if (requestARN === undefined) { // User in unauthenticated (anonymous request)
341+
return checkPrincipalResult.OK;
297342
}
298-
return false;
343+
344+
return _checkCrossAccount(requestARN, requesterCanonicalID, bucketOwnerCanonicalID);
299345
}
300346

301-
function _checkPrincipals(canonicalID, arn, principal) {
347+
function _checkPrincipalAWS(principal, requesterARN, requesterCanonicalID, bucketOwnerCanonicalID) {
302348
if (principal === '*') {
303-
return true;
349+
return _checkPrincipalWildcard(requesterARN, requesterCanonicalID, bucketOwnerCanonicalID);
304350
}
305-
if (principal.CanonicalUser) {
306-
if (Array.isArray(principal.CanonicalUser)) {
307-
return principal.CanonicalUser.some(p => _checkPrincipal(canonicalID, p));
351+
352+
if (requesterARN === undefined) { // User in unauthenticated (anonymous request)
353+
return checkPrincipalResult.KO;
354+
}
355+
356+
if (principal === requesterARN) {
357+
return _checkCrossAccount(requesterARN, requesterCanonicalID, bucketOwnerCanonicalID);
358+
}
359+
360+
if (_isAccountId(principal) && principal === _getAccountId(requesterARN)) {
361+
return _checkCrossAccount(requesterARN, requesterCanonicalID, bucketOwnerCanonicalID);
362+
}
363+
364+
if (principal.endsWith(':root') && _getAccountId(principal) === _getAccountId(requesterARN)) {
365+
return _checkCrossAccount(requesterARN, requesterCanonicalID, bucketOwnerCanonicalID);
366+
}
367+
368+
return checkPrincipalResult.KO;
369+
}
370+
371+
function _checkPrincipalCanonicalUser(principal, requesterARN, requesterCanonicalID, bucketOwnerCanonicalID) {
372+
if (principal === '*') {
373+
return _checkPrincipalWildcard(requesterARN, requesterCanonicalID, bucketOwnerCanonicalID);
374+
}
375+
376+
if (requesterARN === undefined) { // User in unauthenticated (anonymous request)
377+
return checkPrincipalResult.KO;
378+
}
379+
380+
if (principal === requesterCanonicalID) {
381+
return _checkCrossAccount(requesterARN, requesterCanonicalID, bucketOwnerCanonicalID);
382+
}
383+
384+
return checkPrincipalResult.KO;
385+
}
386+
387+
function _findBestPrincipalMatch(principalArray, checkFunc) {
388+
let bestMatch = checkPrincipalResult.KO;
389+
if (!principalArray) {
390+
return bestMatch;
391+
}
392+
393+
const principals = Array.isArray(principalArray) ? principalArray : [principalArray];
394+
395+
for (const p of principals) {
396+
const result = checkFunc(p);
397+
if (result === checkPrincipalResult.OK) {
398+
return checkPrincipalResult.OK; // Highest permission, can exit early
308399
}
309-
return _checkPrincipal(canonicalID, principal.CanonicalUser);
400+
if (result > bestMatch) {
401+
bestMatch = result;
402+
}
403+
}
404+
405+
return bestMatch;
406+
}
407+
408+
function _checkPrincipals(canonicalID, arn, principal, bucketOwnerCanonicalID) {
409+
if (principal === '*') {
410+
return _checkPrincipalWildcard(arn, canonicalID, bucketOwnerCanonicalID);
411+
}
412+
413+
if (principal.CanonicalUser) {
414+
return _findBestPrincipalMatch(principal.CanonicalUser,
415+
p => _checkPrincipalCanonicalUser(p, arn, canonicalID, bucketOwnerCanonicalID));
310416
}
417+
311418
if (principal.AWS) {
312-
if (Array.isArray(principal.AWS)) {
313-
return principal.AWS.some(p => _checkPrincipal(arn, p));
314-
}
315-
return _checkPrincipal(arn, principal.AWS);
419+
return _findBestPrincipalMatch(principal.AWS,
420+
p => _checkPrincipalAWS(p, arn, canonicalID, bucketOwnerCanonicalID));
316421
}
317-
return false;
422+
423+
return checkPrincipalResult.KO;
318424
}
319425

426+
// checkBucketPolicy Finite State Machine.
427+
// ┌───────────────────────────┐
428+
// │ ▼
429+
// │ ┌───────┐
430+
// │ ┌────────►│ ALLOW ├──────────────┐
431+
// │ ┌─────┐ │ └──┬────┘ │
432+
// │ │START│ │ │ │
433+
// │ └──┬──┘ │ │ │
434+
// │ │ │ │ │
435+
// │ ▼ │ ▼ ▼
436+
// │┌──────────────┤ ┌────┐ ┌─────┐
437+
// ││ DEFAULT_DENY ├─────────►│DENY├────────────►│ END │
438+
// │└──────┬───────┤ └────┘ └─────┘
439+
// │ │ │ ▲ ▲ ▲
440+
// │ │ │ │ │ │
441+
// │ │ │ │ │ │
442+
// │ │ │ ┌───┴───────────┐ │ │
443+
// │ │ └────────►│ CROSS_ACCOUNT ├──────┘ │
444+
// │ │ └┬──────────────┘ │
445+
// └───────┼──────────────────┘ │
446+
// └──────────────────────────────────────────┘
447+
//
320448
function checkBucketPolicy(policy, requestType, canonicalID, arn, bucketOwner, log, request, actionImplicitDenies) {
321-
let permission = 'defaultDeny';
449+
let permission = checkBucketPolicyResult.DEFAULT_DENY;
322450
// if requester is user within bucket owner account, actions should be
323451
// allowed unless explicitly denied (assumes allowed by IAM policy)
324452
if (bucketOwner === canonicalID && actionImplicitDenies[requestType] === false) {
325-
permission = 'allow';
453+
permission = checkBucketPolicyResult.ALLOW;
326454
}
327455
let copiedStatement = JSON.parse(JSON.stringify(policy.Statement));
328456

@@ -336,19 +464,42 @@ function checkBucketPolicy(policy, requestType, canonicalID, arn, bucketOwner, l
336464

337465
while (copiedStatement.length > 0) {
338466
const s = copiedStatement[0];
339-
340-
const principalMatch = _checkPrincipals(canonicalID, arn, s.Principal);
467+
const principalMatch = _checkPrincipals(canonicalID, arn, s.Principal, bucketOwner);
341468
const actionMatch = _checkBucketPolicyActions(requestType, s.Action, log);
342469
const resourceMatch = _checkBucketPolicyResources(requestContext, s.Resource, log);
343470
const conditionsMatch = _checkBucketPolicyConditions(requestContext, s.Condition, log);
344471

345-
if (principalMatch && actionMatch && resourceMatch && conditionsMatch && s.Effect === 'Deny') {
346-
// explicit deny trumps any allows, so return immediately
347-
return 'explicitDeny';
348-
}
349-
if (principalMatch && actionMatch && resourceMatch && conditionsMatch && s.Effect === 'Allow') {
350-
permission = 'allow';
472+
const ok = principalMatch === checkPrincipalResult.OK && actionMatch && resourceMatch && conditionsMatch;
473+
const okCross = principalMatch === checkPrincipalResult.CROSS_ACCOUNT_OK
474+
&& actionMatch && resourceMatch && conditionsMatch;
475+
switch (permission) {
476+
case checkBucketPolicyResult.DEFAULT_DENY:
477+
if ((ok || okCross) && s.Effect === 'Deny') {
478+
return checkBucketPolicyResult.EXPLICIT_DENY;
479+
} else if (ok && s.Effect === 'Allow') {
480+
permission = checkBucketPolicyResult.ALLOW;
481+
} else if (okCross && s.Effect === 'Allow') {
482+
permission = checkBucketPolicyResult.CROSS_ACCOUNT_ALLOW;
483+
}
484+
break;
485+
case checkBucketPolicyResult.EXPLICIT_DENY:
486+
return checkBucketPolicyResult.EXPLICIT_DENY;
487+
case checkBucketPolicyResult.ALLOW:
488+
if ((ok || okCross) && s.Effect === 'Deny') {
489+
return checkBucketPolicyResult.EXPLICIT_DENY;
490+
}
491+
break;
492+
case checkBucketPolicyResult.CROSS_ACCOUNT_ALLOW:
493+
if ((ok || okCross) && s.Effect === 'Deny') {
494+
return checkBucketPolicyResult.EXPLICIT_DENY;
495+
} else if (ok && s.Effect === 'Allow') {
496+
permission = checkBucketPolicyResult.ALLOW;
497+
}
498+
break;
499+
default: // Needed for the linter, should be unreachable.
500+
break;
351501
}
502+
352503
copiedStatement = copiedStatement.splice(1);
353504
}
354505
return permission;
@@ -364,9 +515,13 @@ function processBucketPolicy(requestType, bucket, canonicalID, arn, bucketOwner,
364515
const bucketPolicyPermission = checkBucketPolicy(bucketPolicy, requestType, canonicalID, arn,
365516
bucketOwner, log, request, actionImplicitDenies);
366517

367-
if (bucketPolicyPermission === 'explicitDeny') {
518+
if (bucketPolicyPermission === checkBucketPolicyResult.EXPLICIT_DENY) {
368519
processedResult = false;
369-
} else if (bucketPolicyPermission === 'allow') {
520+
} else if (bucketPolicyPermission === checkBucketPolicyResult.ALLOW) {
521+
processedResult = true;
522+
} else if (bucketPolicyPermission === checkBucketPolicyResult.CROSS_ACCOUNT_ALLOW
523+
&& actionImplicitDenies[requestType] === false) {
524+
// If the bucket policy is cross account, only return true if Vault also returned an explicit allow.
370525
processedResult = true;
371526
} else {
372527
processedResult = actionImplicitDenies[requestType] === false && aclPermission;
@@ -428,7 +583,7 @@ function evaluateBucketPolicyWithIAM(bucket, requestTypesInput, canonicalID, aut
428583
arn = authInfo.getArn();
429584
}
430585
return processBucketPolicy(_requestType, bucket, canonicalID, arn, bucket.getOwner(), log,
431-
request, true, results, actionImplicitDenies);
586+
request, true, results, actionImplicitDenies);
432587
});
433588
}
434589

@@ -479,8 +634,8 @@ function isObjAuthorized(bucket, objectMD, requestTypesInput, canonicalID, authI
479634
// - account is the bucket owner
480635
// - requester is account, not user
481636
if (bucketOwnerActions.includes(parsedMethodName)
482-
&& (bucketOwner === canonicalID)
483-
&& requesterIsNotUser) {
637+
&& (bucketOwner === canonicalID)
638+
&& requesterIsNotUser) {
484639
results[_requestType] = actionImplicitDenies[_requestType] === false;
485640
return results[_requestType];
486641
}
@@ -640,4 +795,6 @@ module.exports = {
640795
validatePolicyConditions,
641796
isLifecycleSession,
642797
evaluateBucketPolicyWithIAM,
798+
checkBucketPolicy,
799+
checkBucketPolicyResult,
643800
};

‎lib/api/objectGet.js‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ function objectGet(authInfo, request, returnTagCount, log, callback) {
7070
getDeleteMarker: true,
7171
requestType: request.apiMethods || 'objectGet',
7272
request,
73+
returnTagCount,
7374
};
7475

7576
return standardMetadataValidateBucketAndObj(mdValParams, request.actionImplicitDenies, log,
@@ -123,7 +124,8 @@ function objectGet(authInfo, request, returnTagCount, log, callback) {
123124
return callback(headerValResult.error, null, corsHeaders);
124125
}
125126
const responseMetaHeaders = collectResponseHeaders(objMD,
126-
corsHeaders, verCfg, returnTagCount);
127+
corsHeaders, verCfg,
128+
returnTagCount && objMD.returnTagCount); // IAM and Bucket policy should both authorize tagging.
127129

128130
setExpirationHeaders(responseMetaHeaders, {
129131
lifecycleConfig: bucket.getLifecycleConfiguration(),

‎lib/metadata/metadataUtils.js‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -230,13 +230,33 @@ function standardMetadataValidateBucketAndObj(params, actionImplicitDenies, log,
230230
return next(null, bucket, objMD);
231231
},
232232
(bucket, objMD, next) => {
233+
const objMetadata = objMD;
233234
const canonicalID = authInfo.getCanonicalID();
234-
if (!isObjAuthorized(bucket, objMD, requestType, canonicalID, authInfo, log, request,
235+
if (!isObjAuthorized(bucket, objMetadata, requestType, canonicalID, authInfo, log, request,
235236
actionImplicitDenies)) {
236237
log.debug('access denied for user on object', { requestType });
237238
return next(errors.AccessDenied, bucket);
238239
}
239-
return next(null, bucket, objMD);
240+
241+
if (!objMetadata) {
242+
return next(null, bucket, objMetadata);
243+
}
244+
245+
let returnTagCount = false;
246+
if (params.returnTagCount) {
247+
// If returnTagCount is true we know that Vault authorized the request so it is not an implicitDeny.
248+
const implicitDeny = false;
249+
if (requestType.some(r => r === 'objectGet')) {
250+
returnTagCount = isObjAuthorized(bucket, objMetadata, ['objectGetTagging'], canonicalID, authInfo,
251+
log, request, implicitDeny);
252+
} else if (requestType.some(r => r === 'objectGetVersion')) {
253+
returnTagCount = isObjAuthorized(bucket, objMetadata, ['objectGetTaggingVersion'],
254+
canonicalID, authInfo, log, request, implicitDeny);
255+
}
256+
257+
objMetadata.returnTagCount = returnTagCount;
258+
}
259+
return next(null, bucket, objMetadata);
240260
},
241261
(bucket, objMD, next) => {
242262
const needQuotaCheck = requestType => requestType.some(type => actionNeedQuotaCheck[type] ||

0 commit comments

Comments
 (0)