CLDSRV-674: handle cross-account bucket policies

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,20 @@ 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,
+    OK: 2,
+});
+
+const checkBucketPolicyResult = Object.freeze({
+    DEFAULT_DENY: 0,
+    EXPLICIT_DENY: 1,
+    ALLOW: 2,
+    CROSS_ACCOUNT_ALLOW: 3,
+});
+
 /**
  * Checks the access control for a given bucket based on the request type and user's canonical ID.
  *
@@ -117,7 +131,7 @@ function checkBucketAcls(bucket, requestType, canonicalID, mainApiCall) {
     // authorization check should just return true so can move on to check
     // rights at the object level.
     return (requestTypeParsed === 'objectPutACL' || requestTypeParsed === 'objectGetACL'
-    || requestTypeParsed === 'objectGet' || requestTypeParsed === 'objectHead');
+        || requestTypeParsed === 'objectGet' || requestTypeParsed === 'objectHead');
 }
 
 function checkObjectAcls(bucket, objectMD, requestType, canonicalID, requesterIsNotUser,
@@ -265,67 +279,206 @@ function _isAccountId(principal) {
     return (principal.length === 12 && /^\d+$/.test(principal));
 }
 
-function _checkPrincipal(requester, principal) {
-    if (principal === '*') {
-        return true;
+/**
+ * Checks if the ARN represents a root user account
+ * @param {string} arn - The ARN to check
+ * @returns {boolean} True if root user, false otherwise
+ */
+function _isRootUser(arn) {
+    if (!arn) {
+        return false;
     }
-    // User in unauthenticated (anonymous request)
-    if (requester === undefined) {
+
+    // 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) {
         return false;
     }
-    if (principal === requester) {
+
+    const resource = arns[arns.length - 1];
+
+    // If we start with '/' is because we have a empty resource type so we know it is a root account.
+    if (resource.startsWith('/')) {
         return true;
     }
-    if (_isAccountId(principal)) {
-        return _getAccountId(requester) === principal;
+
+    return false;
+}
+
+/** _evaluateCrossAccount - checks if it is a cross-account request.
+ * @param {string} requesterARN - requester ARN
+ * @param {string} requesterCanonicalID - requester canonical ID
+ * @param {string} bucketOwnerCanonicalID - bucket owner canonical ID
+ * @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:/accountName/' for root accounts
+    // with an empty resource type (missing 'user/' prefix)
+    if (!_isRootUser(requesterARN)) {
+        return bucketOwnerCanonicalID === requesterCanonicalID ?
+            checkPrincipalResult.OK : checkPrincipalResult.CROSS_ACCOUNT_OK;
     }
-    if (principal.endsWith('root')) {
-        return _getAccountId(requester) === _getAccountId(principal);
+
+    return checkPrincipalResult.OK;
+}
+
+function _checkPrincipalWildcard(requestARN, requesterCanonicalID, bucketOwnerCanonicalID) {
+    if (requestARN === undefined) { // User in unauthenticated (anonymous request)
+        return checkPrincipalResult.OK;
     }
-    return false;
+
+    return _checkCrossAccount(requestARN, requesterCanonicalID, bucketOwnerCanonicalID);
 }
 
-function _checkPrincipals(canonicalID, arn, principal) {
+function _checkPrincipalAWS(principal, requesterARN, requesterCanonicalID, bucketOwnerCanonicalID) {
     if (principal === '*') {
-        return true;
+        return _checkPrincipalWildcard(requesterARN, requesterCanonicalID, bucketOwnerCanonicalID);
     }
-    if (principal.CanonicalUser) {
-        if (Array.isArray(principal.CanonicalUser)) {
-            return principal.CanonicalUser.some(p => _checkPrincipal(canonicalID, p));
+
+    if (requesterARN === undefined) { // User in unauthenticated (anonymous request)
+        return checkPrincipalResult.KO;
+    }
+
+    if (principal === requesterARN) {
+        return _checkCrossAccount(requesterARN, requesterCanonicalID, bucketOwnerCanonicalID);
+    }
+
+    if (_isAccountId(principal) && principal === _getAccountId(requesterARN)) {
+        return _checkCrossAccount(requesterARN, requesterCanonicalID, bucketOwnerCanonicalID);
+    }
+
+    if (principal.endsWith(':root') && _getAccountId(principal) === _getAccountId(requesterARN)) {
+        return _checkCrossAccount(requesterARN, requesterCanonicalID, bucketOwnerCanonicalID);
+    }
+
+    return checkPrincipalResult.KO;
+}
+
+function _checkPrincipalCanonicalUser(principal, requesterARN, requesterCanonicalID, bucketOwnerCanonicalID) {
+    if (principal === '*') {
+        return _checkPrincipalWildcard(requesterARN, requesterCanonicalID, bucketOwnerCanonicalID);
+    }
+
+    if (requesterARN === undefined) { // User in unauthenticated (anonymous request)
+        return checkPrincipalResult.KO;
+    }
+
+    if (principal === requesterCanonicalID) {
+        return _checkCrossAccount(requesterARN, requesterCanonicalID, bucketOwnerCanonicalID);
+    }
+
+    return checkPrincipalResult.KO;
+}
+
+function _findBestPrincipalMatch(principalArray, checkFunc) {
+    let bestMatch = checkPrincipalResult.KO;
+    if (!principalArray) {
+        return bestMatch;
+    }
+
+    const principals = Array.isArray(principalArray) ? principalArray : [principalArray];
+
+    // eslint-disable-next-line no-restricted-syntax
+    for (const p of principals) {
+        const result = checkFunc(p);
+        if (result === checkPrincipalResult.OK) {
+            return checkPrincipalResult.OK; // Highest permission, can exit early
+        }
+        if (result > bestMatch) {
+            bestMatch = result;
         }
-        return _checkPrincipal(canonicalID, principal.CanonicalUser);
     }
+
+    return bestMatch;
+}
+
+function _checkPrincipals(canonicalID, arn, principal, bucketOwnerCanonicalID) {
+    if (principal === '*') {
+        return _checkPrincipalWildcard(arn, canonicalID, bucketOwnerCanonicalID);
+    }
+
+    if (principal.CanonicalUser) {
+        return _findBestPrincipalMatch(principal.CanonicalUser,
+            p => _checkPrincipalCanonicalUser(p, arn, canonicalID, bucketOwnerCanonicalID));
+    }
+
     if (principal.AWS) {
-        if (Array.isArray(principal.AWS)) {
-            return principal.AWS.some(p => _checkPrincipal(arn, p));
-        }
-        return _checkPrincipal(arn, principal.AWS);
+        return _findBestPrincipalMatch(principal.AWS,
+            p => _checkPrincipalAWS(p, arn, canonicalID, bucketOwnerCanonicalID));
     }
-    return false;
+
+    return checkPrincipalResult.KO;
 }
 
+// checkBucketPolicy Finite State Machine.
+// ┌───────────────────────────┐
+// │                           ▼
+// │                         ┌───────┐
+// │               ┌────────►│ ALLOW ├──────────────┐
+// │ ┌─────┐       │         └──┬────┘              │
+// │ │START│       │            │                   │
+// │ └──┬──┘       │            │                   │
+// │    │          │            │                   │
+// │    ▼          │            ▼                   ▼
+// │┌──────────────┤          ┌────┐             ┌─────┐
+// ││ DEFAULT_DENY ├─────────►│DENY├────────────►│ END │
+// │└──────┬───────┤          └────┘             └─────┘
+// │       │       │             ▲                  ▲ ▲
+// │       │       │             │                  │ │
+// │       │       │             │                  │ │
+// │       │       │         ┌───┴───────────┐      │ │
+// │       │       └────────►│ CROSS_ACCOUNT ├──────┘ │
+// │       │                 └┬──────────────┘        │
+// └───────┼──────────────────┘                       │
+//         └──────────────────────────────────────────┘
+//
 function checkBucketPolicy(policy, requestType, canonicalID, arn, bucketOwner, log, request, actionImplicitDenies) {
-    let permission = 'defaultDeny';
+    let permission = checkBucketPolicyResult.DEFAULT_DENY;
     // if requester is user within bucket owner account, actions should be
     // allowed unless explicitly denied (assumes allowed by IAM policy)
     if (bucketOwner === canonicalID && actionImplicitDenies[requestType] === false) {
-        permission = 'allow';
+        permission = checkBucketPolicyResult.ALLOW;
     }
     let copiedStatement = JSON.parse(JSON.stringify(policy.Statement));
     while (copiedStatement.length > 0) {
         const s = copiedStatement[0];
-        const principalMatch = _checkPrincipals(canonicalID, arn, s.Principal);
+        const principalMatch = _checkPrincipals(canonicalID, arn, s.Principal, bucketOwner);
         const actionMatch = _checkBucketPolicyActions(requestType, s.Action, log);
         const resourceMatch = _checkBucketPolicyResources(request, s.Resource, log);
         const conditionsMatch = _checkBucketPolicyConditions(request, s.Condition, log);
 
-        if (principalMatch && actionMatch && resourceMatch && conditionsMatch && s.Effect === 'Deny') {
-            // explicit deny trumps any allows, so return immediately
-            return 'explicitDeny';
-        }
-        if (principalMatch && actionMatch && resourceMatch && conditionsMatch && s.Effect === 'Allow') {
-            permission = 'allow';
+        const ok = principalMatch === checkPrincipalResult.OK && actionMatch && resourceMatch && conditionsMatch;
+        const okCross = principalMatch === checkPrincipalResult.CROSS_ACCOUNT_OK
+            && actionMatch && resourceMatch && conditionsMatch;
+        switch (permission) {
+        case checkBucketPolicyResult.DEFAULT_DENY:
+            if ((ok || okCross) && s.Effect === 'Deny') {
+                return checkBucketPolicyResult.EXPLICIT_DENY;
+            } else if (ok && s.Effect === 'Allow') {
+                permission = checkBucketPolicyResult.ALLOW;
+            } else if (okCross && s.Effect === 'Allow') {
+                permission = checkBucketPolicyResult.CROSS_ACCOUNT_ALLOW;
+            }
+            break;
+        case checkBucketPolicyResult.EXPLICIT_DENY:
+            return checkBucketPolicyResult.EXPLICIT_DENY;
+        case checkBucketPolicyResult.ALLOW:
+            if ((ok || okCross) && s.Effect === 'Deny') {
+                return checkBucketPolicyResult.EXPLICIT_DENY;
+            }
+            break;
+        case checkBucketPolicyResult.CROSS_ACCOUNT_ALLOW:
+            if ((ok || okCross) && s.Effect === 'Deny') {
+                return checkBucketPolicyResult.EXPLICIT_DENY;
+            } else if (ok && s.Effect === 'Allow') {
+                permission = checkBucketPolicyResult.ALLOW;
+            }
+            break;
+        default: // Needed for the linter, should be unreachable.
+            break;
         }
+
         copiedStatement = copiedStatement.splice(1);
     }
     return permission;
@@ -341,9 +494,13 @@ function processBucketPolicy(requestType, bucket, canonicalID, arn, bucketOwner,
         const bucketPolicyPermission = checkBucketPolicy(bucketPolicy, requestType, canonicalID, arn,
             bucketOwner, log, request, actionImplicitDenies);
 
-        if (bucketPolicyPermission === 'explicitDeny') {
+        if (bucketPolicyPermission === checkBucketPolicyResult.EXPLICIT_DENY) {
             processedResult = false;
-        } else if (bucketPolicyPermission === 'allow') {
+        } else if (bucketPolicyPermission === checkBucketPolicyResult.ALLOW) {
+            processedResult = true;
+        } else if (bucketPolicyPermission === checkBucketPolicyResult.CROSS_ACCOUNT_ALLOW
+            && actionImplicitDenies[requestType] === false) {
+            // If the bucket policy is cross account, only return true if Vault also returned an explicit allow.
             processedResult = true;
         } else {
             processedResult = actionImplicitDenies[requestType] === false && aclPermission;
@@ -405,7 +562,7 @@ function evaluateBucketPolicyWithIAM(bucket, requestTypesInput, canonicalID, aut
             arn = authInfo.getArn();
         }
         return processBucketPolicy(_requestType, bucket, canonicalID, arn, bucket.getOwner(), log,
-        request, true, results, actionImplicitDenies);
+            request, true, results, actionImplicitDenies);
     });
 }
 
@@ -456,8 +613,8 @@ function isObjAuthorized(bucket, objectMD, requestTypesInput, canonicalID, authI
         // - account is the bucket owner
         // - requester is account, not user
         if (bucketOwnerActions.includes(parsedMethodName)
-        && (bucketOwner === canonicalID)
-        && requesterIsNotUser) {
+            && (bucketOwner === canonicalID)
+            && requesterIsNotUser) {
             results[_requestType] = actionImplicitDenies[_requestType] === false;
             return results[_requestType];
         }
@@ -613,4 +770,6 @@ module.exports = {
     validatePolicyConditions,
     isLifecycleSession,
     evaluateBucketPolicyWithIAM,
+    checkBucketPolicy,
+    checkBucketPolicyResult,
 };

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) {