CLDSRV-1021: Evaluate the bucket policy against the copy source

Author: tcarmet

lib/api/apiUtils/authorization/permissionChecks.js

@@ -445,7 +445,8 @@ function _checkPrincipals(canonicalID, arn, principal, bucketOwnerCanonicalID) {
 // └───────┼──────────────────┘                       │
 //         └──────────────────────────────────────────┘
 //
-function checkBucketPolicy(policy, requestType, canonicalID, arn, bucketOwner, log, request, actionImplicitDenies) {
+function checkBucketPolicy(policy, requestType, canonicalID, arn, bucketOwner, log, request, actionImplicitDenies,
+    policyTarget) {
     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)
@@ -454,10 +455,18 @@ function checkBucketPolicy(policy, requestType, canonicalID, arn, bucketOwner, l
     }
     let copiedStatement = JSON.parse(JSON.stringify(policy.Statement));
 
+    // The policy target, when set, is the resource being authorized when it
+    // differs from the one the request targets (e.g. the source of a copy)
+    let bucketName = request?.bucketName;
+    let objectKey = request?.objectKey;
+    if (policyTarget) {
+        bucketName = policyTarget.bucketName;
+        objectKey = policyTarget.objectKey;
+    }
     const ip = request ? requestUtils.getClientIp(request, config) : undefined;
     const isSecure = request ? requestUtils.getHttpProtocolSecurity(request, config) : undefined;
     const requestContext = request ? new RequestContext(request.headers, request.query,
-        request.bucketName, request.objectKey, ip,
+        bucketName, objectKey, ip,
         isSecure, request.resourceType, 's3', null, null,
         null, null, null, null, null, null, null, null, null,
         request.objectLockRetentionDays) : undefined;
@@ -506,7 +515,7 @@ function checkBucketPolicy(policy, requestType, canonicalID, arn, bucketOwner, l
 }
 
 function processBucketPolicy(requestType, bucket, canonicalID, arn, bucketOwner, log,
-    request, aclPermission, results, actionImplicitDenies) {
+    request, aclPermission, results, actionImplicitDenies, policyTarget) {
     const bucketPolicy = bucket.getBucketPolicy();
     let processedResult = results[requestType];
     let aclRequired = false;
@@ -515,7 +524,7 @@ function processBucketPolicy(requestType, bucket, canonicalID, arn, bucketOwner,
         aclRequired = true;
     } else {
         const bucketPolicyPermission = checkBucketPolicy(bucketPolicy, requestType, canonicalID, arn,
-            bucketOwner, log, request, actionImplicitDenies);
+            bucketOwner, log, request, actionImplicitDenies, policyTarget);
 
         if (bucketPolicyPermission === checkBucketPolicyResult.EXPLICIT_DENY) {
             processedResult = false;
@@ -534,7 +543,7 @@ function processBucketPolicy(requestType, bucket, canonicalID, arn, bucketOwner,
 }
 
 function isBucketAuthorized(bucket, requestTypesInput, canonicalID, authInfo, log, request,
-    actionImplicitDeniesInput = {}, isWebsite = false) {
+    actionImplicitDeniesInput = {}, isWebsite = false, policyTarget = undefined) {
     const requestTypes = Array.isArray(requestTypesInput) ? requestTypesInput : [requestTypesInput];
     const actionImplicitDenies = !actionImplicitDeniesInput ? {} : actionImplicitDeniesInput;
     const mainApiCall = requestTypes[0];
@@ -568,7 +577,7 @@ function isBucketAuthorized(bucket, requestTypesInput, canonicalID, authInfo, lo
             actionImplicitDenies.objectGet = actionImplicitDenies.objectGet || false;
         }
         const { allowed, aclRequired } = processBucketPolicy(_requestType, bucket, canonicalID, arn,
-            bucket.getOwner(), log, request, aclPermission, results, actionImplicitDenies);
+            bucket.getOwner(), log, request, aclPermission, results, actionImplicitDenies, policyTarget);
         if (aclRequired && request?.serverAccessLog) {
             // eslint-disable-next-line no-param-reassign
             request.serverAccessLog.aclRequired = 'Yes';
@@ -597,7 +606,7 @@ function evaluateBucketPolicyWithIAM(bucket, requestTypesInput, canonicalID, aut
 }
 
 function isObjAuthorized(bucket, objectMD, requestTypesInput, canonicalID, authInfo, log, request,
-    actionImplicitDeniesInput = {}, isWebsite = false) {
+    actionImplicitDeniesInput = {}, isWebsite = false, policyTarget = undefined) {
     const requestTypes = Array.isArray(requestTypesInput) ? requestTypesInput : [requestTypesInput];
     const actionImplicitDenies = !actionImplicitDeniesInput ? {} : actionImplicitDeniesInput;
     const results = {};
@@ -617,7 +626,7 @@ function isObjAuthorized(bucket, objectMD, requestTypesInput, canonicalID, authI
                 permission = 'objectPut';
             }
             results[_requestType] = isBucketAuthorized(bucket, permission, canonicalID, authInfo, log, request,
-                actionImplicitDenies, isWebsite);
+                actionImplicitDenies, isWebsite, policyTarget);
             // User is already authorized on the bucket for FULL_CONTROL or WRITE or
             // bucket has canned ACL public-read-write
             if ((parsedMethodName === 'objectPut' || parsedMethodName === 'objectDelete')
@@ -651,7 +660,7 @@ function isObjAuthorized(bucket, objectMD, requestTypesInput, canonicalID, authI
         const aclPermission = checkObjectAcls(bucket, objectMD, parsedMethodName,
             canonicalID, requesterIsNotUser, isUserUnauthenticated, mainApiCall);
         const { allowed, aclRequired } = processBucketPolicy(_requestType, bucket, canonicalID, arn, bucketOwner,
-            log, request, aclPermission, results, actionImplicitDenies);
+            log, request, aclPermission, results, actionImplicitDenies, policyTarget);
         if (aclRequired && request?.serverAccessLog) {
             // eslint-disable-next-line no-param-reassign
             request.serverAccessLog.aclRequired = 'Yes';

lib/api/objectCopy.js

@@ -243,6 +243,9 @@ function objectCopy(authInfo, request, sourceBucket, sourceObject, sourceVersion
          */
         checkQuota: true,
         request,
+        // The request targets the destination, the bucket policy must be
+        // evaluated against the source
+        policyTarget: { bucketName: sourceBucket, objectKey: sourceObject },
     };
     const valPutParams = {
         authInfo,

lib/api/objectPutCopyPart.js

@@ -56,6 +56,9 @@ function objectPutCopyPart(authInfo, request, sourceBucket, sourceObject, reqVer
          */
         checkQuota: true,
         request,
+        // The request targets the destination, the bucket policy must be
+        // evaluated against the source
+        policyTarget: { bucketName: sourceBucket, objectKey: sourceObject },
     };
 
     const partNumber = Number.parseInt(request.query.partNumber, 10);

lib/metadata/metadataUtils.js

@@ -202,6 +202,8 @@ function metadataGetObjects(bucketName, objectsKeys, log, cb) {
  * @param {string} params.requestType - type of request
  * @param {string} [params.preciseRequestType] - precise type of request
  * @param {object} params.request - http request object
+ * @param {object} [params.policyTarget] - resource to evaluate the bucket
+ *   policy against when it differs from the request target
  *  @param {RequestLogger} log - request logger
  * @param {object} actionImplicitDenies - identity authorization results
  * @return {ArsenalError|null} returns a validation error, or null if validation OK
@@ -212,7 +214,7 @@ function metadataGetObjects(bucketName, objectsKeys, log, cb) {
  * - AccessDenied: bucket is not authorized
  */
 function validateBucket(bucket, params, log, actionImplicitDenies = {}) {
-    const { authInfo, preciseRequestType, request } = params;
+    const { authInfo, preciseRequestType, request, policyTarget } = params;
     let requestType = params.requestType;
     if (bucketShield(bucket, requestType)) {
         log.debug('bucket is shielded from request', {
@@ -249,6 +251,8 @@ function validateBucket(bucket, params, log, actionImplicitDenies = {}) {
             log,
             request,
             actionImplicitDenies,
+            false,
+            policyTarget,
         )
     ) {
         log.debug('access denied for user on bucket', { requestType });
@@ -349,13 +353,18 @@ function checkRateLimitIfNeeded(request, authInfo, bucketMD, log, callback) {
  * @param {object} [params.serverAccessLogOptions] - options for server access logging
  * @param {boolean} [params.serverAccessLogOptions.copySource] - if true, stores source bucket
  *   information in request.sourceServerAccessLog for copy operations
+ * @param {object} [params.policyTarget] - resource to evaluate the bucket policy against
+ *   when it differs from the request target, e.g. the source of a copy
+ * @param {string} params.policyTarget.bucketName - name of bucket
+ * @param {string} params.policyTarget.objectKey - name of object
  * @param {boolean} actionImplicitDenies - identity authorization results
  * @param {RequestLogger} log - request logger
  * @param {function} callback - callback
  * @return {undefined} - and call callback with params err, bucket md
  */
 function standardMetadataValidateBucketAndObj(params, actionImplicitDenies, log, callback) {
-    const { authInfo, bucketName, objectKey, versionId, getDeleteMarker, request, withVersionId } = params;
+    const { authInfo, bucketName, objectKey, versionId, getDeleteMarker, request, withVersionId, policyTarget } =
+        params;
     let requestType = params.requestType;
     if (!Array.isArray(requestType)) {
         requestType = [requestType];
@@ -433,6 +442,8 @@ function standardMetadataValidateBucketAndObj(params, actionImplicitDenies, log,
                         log,
                         request,
                         actionImplicitDenies,
+                        false,
+                        policyTarget,
                     )
                 ) {
                     log.debug('access denied for user on object', { requestType });

tests/unit/api/objectCopy.js

@@ -152,10 +152,7 @@ describe('objectCopy with versioning', () => {
         });
     });
 
-    // TODO: S3C-9965
-    // Skipped because the policy is not checked correctly
-    // When source bucket policy is checked destination arn is used
-    it.skip('should set bucketOwnerId if requesting account differs from dest bucket owner', done => {
+    it('should set bucketOwnerId if requesting account differs from dest bucket owner', done => {
         const authInfo2 = makeAuthInfo('accessKey2');
         const testObjectCopyRequest = _createObjectCopyRequest(destBucketName);
         const testPutSrcPolicyRequest = new DummyRequest({