Improvement/cldsrv 431 misc api imp deny

.github/workflows/tests.yaml

@@ -227,13 +227,11 @@ jobs:
     - name: Setup CI services
       run: docker-compose up -d
       working-directory: .github/docker
-      # TODO CLDSRV-431 re-enable file backend tests
-      # Note: Disabled here to save time as due to API logic changes only 28 passing, 474 pending and 696 failing
-    # - name: Run file ft tests
-    #   run: |-
-    #     set -o pipefail;
-    #     bash wait_for_local_port.bash 8000 40
-    #     yarn run ft_test | tee /tmp/artifacts/${{ github.job }}/tests.log
+    - name: Run file ft tests
+      run: |-
+        set -o pipefail;
+        bash wait_for_local_port.bash 8000 40
+        yarn run ft_test | tee /tmp/artifacts/${{ github.job }}/tests.log
     - name: Upload logs to artifacts
       uses: scality/action-artifacts@v3
       with:

lib/api/apiUtils/authorization/permissionChecks.js

@@ -273,9 +273,10 @@ function checkBucketPolicy(policy, requestType, canonicalID, arn, bucketOwner, l
     let permission = 'defaultDeny';
     // if requester is user within bucket owner account, actions should be
     // allowed unless explicitly denied (assumes allowed by IAM policy)
-    if (bucketOwner === canonicalID) {
-        permission = 'allow';
-    }
+    // Update: manual testing on the 18/9/2023 found this not to be the case
+    // if (bucketOwner === canonicalID) {
+    //     permission = 'allow';
+    // }
     let copiedStatement = JSON.parse(JSON.stringify(policy.Statement));
     while (copiedStatement.length > 0) {
         const s = copiedStatement[0];
@@ -381,10 +382,10 @@ function isObjAuthorized(bucket, objectMD, requestTypes, canonicalID, authInfo,
         if (!objectMD) {
         // 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') {
-                results[_requestType] = actionImplicitDenies[_requestType] === false;
-                return;
-            }
+            // if (parsedMethodName === 'objectPut' || parsedMethodName === 'objectDelete') {
+            //     results[_requestType] = actionImplicitDenies[_requestType] === false;
+            //     return;
+            // }
             // check bucket has read access
             // 'bucketGet' covers listObjects and listMultipartUploads, bucket read actions
             results[_requestType] = isBucketAuthorized(bucket, 'bucketGet', canonicalID, authInfo,

lib/api/apiUtils/object/abortMultipartUpload.js

@@ -22,10 +22,11 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log,
     // but the requestType is the more general 'objectDelete'
     const metadataValParams = Object.assign({}, metadataValMPUparams);
     metadataValParams.requestType = 'objectPut';
+    const authzIdentityResult = request ? request.actionImplicitDenies : true;
 
     async.waterfall([
         function checkDestBucketVal(next) {
-            metadataValidateBucketAndObj(metadataValParams, log,
+            metadataValidateBucketAndObj(metadataValParams, authzIdentityResult, log,
                 (err, destinationBucket) => {
                     if (err) {
                         return next(err, destinationBucket);
@@ -56,9 +57,15 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log,
         function abortExternalMpu(mpuBucket, mpuOverviewObj, destBucket,
         next) {
             const location = mpuOverviewObj.controllingLocationConstraint;
+            const originalIdentityImpDenies = request.actionImplicitDenies;
+            // eslint-disable-next-line no-param-reassign
+            // eslint-disable-next-line no-param-reassign
+            delete request.actionImplicitDenies;
             return data.abortMPU(objectKey, uploadId, location, bucketName,
-            request, destBucket, locationConstraintCheck, log,
+                request, destBucket, locationConstraintCheck, log,
             (err, skipDataDelete) => {
+                // eslint-disable-next-line no-param-reassign
+                request.actionImplicitDenies = originalIdentityImpDenies;
                 if (err) {
                     return next(err, destBucket);
                 }

lib/api/apiUtils/object/objectLockHelpers.js

@@ -3,6 +3,7 @@ const moment = require('moment');
 
 const { config } = require('../../../Config');
 const vault = require('../../../auth/vault');
+const { evaluateBucketPolicyWithIAM } = require('../authorization/permissionChecks');
 
 /**
  * Calculates retain until date for the locked object version
@@ -302,15 +303,37 @@ function checkUserGovernanceBypass(request, authInfo, bucketMD, objectKey, log,
             if (err) {
                 return cb(err);
             }
-            if (authorizationResults[0].isAllowed !== true) {
+
+            // Deny immediately if there is any explicit deny
+            const explicitDenyExists = authorizationResults.some(
+                authzResult => authzResult.isAllowed === false && authzResult.isImplicit === false);
+            if (explicitDenyExists) {
                 log.trace('authorization check failed for user',
                     {
                         'method': 'checkUserPolicyGovernanceBypass',
                         's3:BypassGovernanceRetention': false,
                     });
                 return cb(errors.AccessDenied);
             }
-            return cb(null);
+
+            // Convert authorization results into an easier to handle format
+            const iamAuthzResults = authorizationResults.reduce((acc, curr, idx) => {
+                // eslint-disable-next-line no-param-reassign
+                acc[requestContextParams[idx].apiMethod] = curr.isImplicit;
+                return acc;
+            }, {});
+
+            // Evaluate against the bucket policies
+            const areAllActionsAllowed = evaluateBucketPolicyWithIAM(
+                bucketMD,
+                Object.keys(iamAuthzResults),
+                authInfo.getCanonicalID(),
+                authInfo,
+                iamAuthzResults,
+                log,
+                request);
+
+            return cb(areAllActionsAllowed ? null : errors.AccessDenied);
         });
 }
 
@@ -322,4 +345,4 @@ module.exports = {
     hasGovernanceBypassHeader,
     checkUserGovernanceBypass,
     ObjectLockInfo,
-};
+};

lib/api/bucketHead.js

@@ -21,7 +21,7 @@ function bucketHead(authInfo, request, log, callback) {
         requestType: 'bucketHead',
         request,
     };
-    metadataValidateBucket(metadataValParams, log, (err, bucket) => {
+    metadataValidateBucket(metadataValParams, request.actionImplicitDenies, log, (err, bucket) => {
         const corsHeaders = collectCorsHeaders(request.headers.origin,
             request.method, bucket);
         if (err) {

lib/api/completeMultipartUpload.js

@@ -120,7 +120,7 @@ function completeMultipartUpload(authInfo, request, log, callback) {
                 // at the destinationBucket level are same as objectPut
                 requestType: 'objectPut',
             };
-            metadataValidateBucketAndObj(metadataValParams, log, next);
+            metadataValidateBucketAndObj(metadataValParams, request.actionImplicitDenies, log, next);
         },
         function validateMultipart(destBucket, objMD, next) {
             if (objMD) {
@@ -190,9 +190,14 @@ function completeMultipartUpload(authInfo, request, log, callback) {
             const mdInfo = { storedParts, mpuOverviewKey, splitter };
             const mpuInfo =
                 { objectKey, uploadId, jsonList, bucketName, destBucket };
+            const originalIdentityImpDenies = request.actionImplicitDenies;
+            // eslint-disable-next-line no-param-reassign
+            delete request.actionImplicitDenies;
             return data.completeMPU(request, mpuInfo, mdInfo, location,
             null, null, null, locationConstraintCheck, log,
             (err, completeObjData) => {
+                // eslint-disable-next-line no-param-reassign
+                request.actionImplicitDenies = originalIdentityImpDenies;
                 if (err) {
                     return next(err, destBucket);
                 }

lib/api/initiateMultipartUpload.js

@@ -253,7 +253,7 @@ function initiateMultipartUpload(authInfo, request, log, callback) {
     }
 
     async.waterfall([
-        next => metadataValidateBucketAndObj(metadataValParams, log,
+        next => metadataValidateBucketAndObj(metadataValParams, request.actionImplicitDenies, log,
             (error, destinationBucket) => {
                 const corsHeaders = collectCorsHeaders(request.headers.origin,
                     request.method, destinationBucket);

lib/api/listMultipartUploads.js

@@ -104,7 +104,7 @@ function listMultipartUploads(authInfo, request, log, callback) {
         function waterfall1(next) {
             // Check final destination bucket for authorization rather
             // than multipart upload bucket
-            metadataValidateBucket(metadataValParams, log,
+            metadataValidateBucket(metadataValParams, request.actionImplicitDenies, log,
                 (err, bucket) => next(err, bucket));
         },
         function getMPUBucket(bucket, next) {

lib/api/listParts.js

@@ -112,7 +112,7 @@ function listParts(authInfo, request, log, callback) {
 
     async.waterfall([
         function checkDestBucketVal(next) {
-            metadataValidateBucketAndObj(metadataValParams, log,
+            metadataValidateBucketAndObj(metadataValParams, request.actionImplicitDenies, log,
                 (err, destinationBucket) => {
                     if (err) {
                         return next(err, destinationBucket, null);
@@ -150,8 +150,13 @@ function listParts(authInfo, request, log, callback) {
                 mpuOverviewObj,
                 destBucket,
             };
+            const originalIdentityImpDenies = request.actionImplicitDenies;
+            // eslint-disable-next-line no-param-reassign
+            delete request.actionImplicitDenies;
             return data.listParts(mpuInfo, request, locationConstraintCheck,
             log, (err, backendPartList) => {
+                // eslint-disable-next-line no-param-reassign
+                request.actionImplicitDenies = originalIdentityImpDenies;
                 if (err) {
                     return next(err, destBucket);
                 }

lib/api/multiObjectDelete.js

@@ -11,7 +11,7 @@ const collectCorsHeaders = require('../utilities/collectCorsHeaders');
 const metadata = require('../metadata/wrapper');
 const services = require('../services');
 const vault = require('../auth/vault');
-const { isBucketAuthorized } =
+const { isBucketAuthorized, evaluateBucketPolicyWithIAM } =
     require('./apiUtils/authorization/permissionChecks');
 const { preprocessingVersioningDelete }
     = require('./apiUtils/object/versioning');
@@ -385,15 +385,37 @@ function multiObjectDelete(authInfo, request, log, callback) {
                     return next(null, quietSetting, objects);
                 });
         },
-        function checkPolicies(quietSetting, objects, next) {
+        function checkBucketMetadata(quietSetting, objects, next) {
+            const errorResults = [];
+            return metadata.getBucket(bucketName, log, (err, bucketMD) => {
+                if (err) {
+                    log.trace('error retrieving bucket metadata', { error: err });
+                    return next(err);
+                }
+                if (bucketShield(bucketMD, 'objectDelete')) {
+                    return next(errors.NoSuchBucket);
+                }
+                if (!isBucketAuthorized(bucketMD, 'objectDelete', canonicalID, authInfo,
+                    request.actionImplicitDenies, authInfo)) {
+                    log.trace("access denied due to bucket acl's");
+                    objects.forEach(entry => {
+                        errorResults.push({
+                            entry,
+                            error: errors.AccessDenied,
+                        });
+                    });
+                    return next(null, quietSetting, errorResults, [], bucketMD);
+                }
+                return next(null, quietSetting, errorResults, objects, bucketMD);
+            });
+        },
+        function checkPolicies(quietSetting, errorResults, objects, bucketMD, next) {
             // track keys that are still on track to be deleted
             const inPlay = [];
-            const errorResults = [];
+
             // if request from account, no need to check policies
-            // all objects are inPlay so send array of object keys
-            // as inPlay argument
             if (!authInfo.isRequesterAnIAMUser()) {
-                return next(null, quietSetting, errorResults, objects);
+                return next(null, quietSetting, errorResults, objects, bucketMD);
             }
 
             // TODO: once arsenal's extractParams is separated from doAuth
@@ -429,15 +451,14 @@ function multiObjectDelete(authInfo, request, log, callback) {
             };
             return vault.checkPolicies(requestContextParams, authInfo.getArn(),
                 log, (err, authorizationResults) => {
-                    // there were no policies so received a blanket AccessDenied
                     if (err && err.is.AccessDenied) {
                         objects.forEach(entry => {
                             errorResults.push({
                                 entry,
                                 error: errors.AccessDenied });
                         });
                         // send empty array for inPlay
-                        return next(null, quietSetting, errorResults, []);
+                        return next(null, quietSetting, errorResults, [], bucketMD);
                     }
                     if (err) {
                         log.trace('error checking policies', {
@@ -455,6 +476,16 @@ function multiObjectDelete(authInfo, request, log, callback) {
                         });
                         return next(errors.InternalError);
                     }
+
+                    // Convert authorization results into an easier to handle format
+                    const iamAuthzResults = authorizationResults.reduce((acc, curr, idx) => {
+                        const apiMethod = requestContextParams[idx].apiMethod;
+                        // eslint-disable-next-line no-param-reassign
+                        acc[apiMethod] = curr.isImplicit;
+                        return acc;
+                    }, {});
+
+
                     for (let i = 0; i < authorizationResults.length; i++) {
                         const result = authorizationResults[i];
                         // result is { isAllowed: true,
@@ -470,7 +501,27 @@ function multiObjectDelete(authInfo, request, log, callback) {
                             key: result.arn.slice(slashIndex + 1),
                             versionId: result.versionId,
                         };
-                        if (result.isAllowed) {
+
+                        // Deny immediately if there is an explicit deny
+                        if (!result.isImplicit && !result.isAllowed) {
+                            errorResults.push({
+                                entry,
+                                error: errors.AccessDenied,
+                            });
+                            continue;
+                        }
+
+                        // Evaluate against the bucket policies
+                        const areAllActionsAllowed = evaluateBucketPolicyWithIAM(
+                            bucketMD,
+                            Object.keys(iamAuthzResults),
+                            canonicalID,
+                            authInfo,
+                            iamAuthzResults,
+                            log,
+                            request);
+
+                        if (areAllActionsAllowed) {
                             inPlay.push(entry);
                         } else {
                             errorResults.push({
@@ -479,51 +530,9 @@ function multiObjectDelete(authInfo, request, log, callback) {
                             });
                         }
                     }
-                    return next(null, quietSetting, errorResults, inPlay);
+                    return next(null, quietSetting, errorResults, inPlay, bucketMD);
                 });
         },
-        function checkBucketMetadata(quietSetting, errorResults, inPlay, next) {
-            // if no objects in play, no need to check ACLs / get metadata,
-            // just move on if there is no Origin header
-            if (inPlay.length === 0 && !request.headers.origin) {
-                return next(null, quietSetting, errorResults, inPlay,
-                    undefined);
-            }
-            return metadata.getBucket(bucketName, log, (err, bucketMD) => {
-                if (err) {
-                    log.trace('error retrieving bucket metadata',
-                        { error: err });
-                    return next(err);
-                }
-                // check whether bucket has transient or deleted flag
-                if (bucketShield(bucketMD, 'objectDelete')) {
-                    return next(errors.NoSuchBucket);
-                }
-                // if no objects in play, no need to check ACLs
-                if (inPlay.length === 0) {
-                    return next(null, quietSetting, errorResults, inPlay,
-                        bucketMD);
-                }
-                if (!isBucketAuthorized(bucketMD, 'objectDelete', canonicalID, authInfo,
-                    request.actionImplicitDenies, log, request)) {
-                    log.trace("access denied due to bucket acl's");
-                    // if access denied at the bucket level, no access for
-                    // any of the objects so all results will be error results
-                    inPlay.forEach(entry => {
-                        errorResults.push({
-                            entry,
-                            error: errors.AccessDenied,
-                        });
-                    });
-                    // by sending an empty array as the inPlay array
-                    // async.forEachLimit below will not actually
-                    // make any calls to metadata or data but will continue on
-                    // to the next step to build xml
-                    return next(null, quietSetting, errorResults, [], bucketMD);
-                }
-                return next(null, quietSetting, errorResults, inPlay, bucketMD);
-            });
-        },
         function getObjMetadataAndDeleteStep(quietSetting, errorResults, inPlay,
             bucket, next) {
             return getObjMetadataAndDelete(authInfo, canonicalID, request,