CLDSRV-674: check x-amz-tagging-count permissions in bucket policies

Author: leif-scality

lib/api/api.js

@@ -152,8 +152,6 @@ const api = {
                     log.trace('get object authorization denial from Vault');
                     return errors.AccessDenied;
                 }
-                // TODO add support for returnTagCount in the bucket policy
-                // checks
                 isImplicitDeny[authResults[0].action] = authResults[0].isImplicit;
                 // second item checks s3:GetObject(Version)Tagging action
                 if (!authResults[1].isAllowed) {
@@ -281,6 +279,9 @@ const api = {
                     sourceObject, sourceVersionId, log, callback);
             }
             if (apiMethod === 'objectGet') {
+                // remove objectGetTagging/objectGetTaggingVersion from apiMethods, these were added by
+                // prepareRequestContexts to determine the value of returnTagCount.
+                request.apiMethods = request.apiMethods.filter(methodName => !methodName.includes('Tagging'));
                 return this[apiMethod](userInfo, request, returnTagCount, log, callback);
             }
             return this[apiMethod](userInfo, request, log, callback);

lib/api/apiUtils/authorization/permissionChecks.js

@@ -14,6 +14,7 @@ const {
 const publicReadBuckets = process.env.ALLOW_PUBLIC_READ_BUCKETS
     ? process.env.ALLOW_PUBLIC_READ_BUCKETS.split(',') : [];
 
+// WARNING: enum order matters DO NOT change.
 const checkPrincipalResult = Object.freeze({
     KO: 0,
     CROSS_ACCOUNT_OK: 1,
@@ -288,7 +289,7 @@ function _isRootUser(arn) {
         return false;
     }
 
-    // Vault returns the following arn when the account makes requests 'arn:aws:iam::295412255825:/master/',
+    // Vault returns the following arn when the account makes requests 'arn:aws:iam::123456789012:/accountName/',
     // with an empty resource type ('user/' prefix missing).
     const arns = arn.split(':');
     if (arns.length < 6) {
@@ -312,7 +313,7 @@ function _isRootUser(arn) {
  * @return {checkPrincipalResult} OK if it is not cross-account, CROSS_ACCOUNT_OK otherwise.
  */
 function _checkCrossAccount(requesterARN, requesterCanonicalID, bucketOwnerCanonicalID) {
-    // Vault returns ARNs like 'arn:aws:iam::123456789012:/master/' for root accounts
+    // Vault returns ARNs like 'arn:aws:iam::123456789012:/accountName/' for root accounts
     // with an empty resource type (missing 'user/' prefix)
     if (!_isRootUser(requesterARN)) {
         return bucketOwnerCanonicalID === requesterCanonicalID ?
@@ -347,7 +348,7 @@ function _checkPrincipalAWS(principal, requesterARN, requesterCanonicalID, bucke
         return _checkCrossAccount(requesterARN, requesterCanonicalID, bucketOwnerCanonicalID);
     }
 
-    if (principal.endsWith('root') && _getAccountId(principal) === _getAccountId(requesterARN)) {
+    if (principal.endsWith(':root') && _getAccountId(principal) === _getAccountId(requesterARN)) {
         return _checkCrossAccount(requesterARN, requesterCanonicalID, bucketOwnerCanonicalID);
     }
 
@@ -409,7 +410,7 @@ function _checkPrincipals(canonicalID, arn, principal, bucketOwnerCanonicalID) {
     return checkPrincipalResult.KO;
 }
 
-// checkBucketPolicy FSM.
+// checkBucketPolicy Finite State Machine.
 // ┌───────────────────────────┐
 // │                           ▼
 // │                         ┌───────┐

lib/api/objectGet.js

@@ -51,6 +51,7 @@ function objectGet(authInfo, request, returnTagCount, log, callback) {
         getDeleteMarker: true,
         requestType: request.apiMethods || 'objectGet',
         request,
+        returnTagCount,
     };
 
     return standardMetadataValidateBucketAndObj(mdValParams, request.actionImplicitDenies, log,
@@ -97,7 +98,8 @@ function objectGet(authInfo, request, returnTagCount, log, callback) {
             return callback(headerValResult.error, null, corsHeaders);
         }
         const responseMetaHeaders = collectResponseHeaders(objMD,
-            corsHeaders, verCfg, returnTagCount);
+            corsHeaders, verCfg,
+            returnTagCount && objMD.returnTagCount); // IAM and Bucket policy should both authorize tagging.
 
         setExpirationHeaders(responseMetaHeaders, {
             lifecycleConfig: bucket.getLifecycleConfiguration(),

lib/metadata/metadataUtils.js

@@ -230,13 +230,33 @@ function standardMetadataValidateBucketAndObj(params, actionImplicitDenies, log,
             return next(null, bucket, objMD);
         },
         (bucket, objMD, next) => {
+            const objMetadata = objMD;
             const canonicalID = authInfo.getCanonicalID();
-            if (!isObjAuthorized(bucket, objMD, requestType, canonicalID, authInfo, log, request,
+            if (!isObjAuthorized(bucket, objMetadata, requestType, canonicalID, authInfo, log, request,
                 actionImplicitDenies)) {
                 log.debug('access denied for user on object', { requestType });
                 return next(errors.AccessDenied, bucket);
             }
-            return next(null, bucket, objMD);
+
+            if (!objMetadata) {
+                return next(null, bucket, objMetadata);
+            }
+
+            let returnTagCount = false;
+            if (params.returnTagCount) {
+                // If returnTagCount is true we know that Vault authorized the request so it is not an implicitDeny.
+                const implicitDeny = false;
+                if (requestType.some(r => r === 'objectGet')) {
+                    returnTagCount = isObjAuthorized(bucket, objMetadata, ['objectGetTagging'], canonicalID, authInfo,
+                        log, request, implicitDeny);
+                } else if (requestType.some(r => r === 'objectGetVersion')) {
+                    returnTagCount = isObjAuthorized(bucket, objMetadata, ['objectGetTaggingVersion'],
+                        canonicalID, authInfo, log, request, implicitDeny);
+                }
+
+                objMetadata.returnTagCount = returnTagCount;
+            }
+            return next(null, bucket, objMetadata);
         },
     ], (err, bucket, objMD) => {
         if (err) {

tests/unit/api/apiUtils/permissionChecks.js

@@ -267,7 +267,7 @@ describe('checkBucketPolicy Principal logic', () => {
             },
             requestType: 'bucketGet',
             canonicalID: '79a59df900b949e55d96a1e698fbacedfd6e09d98eacf8f8d5218e7cd47ef2be',
-            arn: 'arn:aws:iam::123456789012:/master/',
+            arn: 'arn:aws:iam::123456789012:/accountName/',
             bucketOwner: 'xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx',
             log: stubLog,
             request: null,
@@ -296,7 +296,7 @@ describe('checkBucketPolicy Principal logic', () => {
             },
             requestType: 'bucketGet',
             canonicalID: '79a59df900b949e55d96a1e698fbacedfd6e09d98eacf8f8d5218e7cd47ef2be',
-            arn: 'arn:aws:iam::123456789012:/master/',
+            arn: 'arn:aws:iam::123456789012:/accountName/',
             bucketOwner: 'xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx',
             log: stubLog,
             request: null,