CLDSRV-430: add delete API implicit deny logic

scality · Dec 5, 2023 · b3ad1e6 · b3ad1e6
1 parent b4f0d34
commit b3ad1e6
Show file tree

Hide file tree

Showing 17 changed files with 89 additions and 64 deletions.
diff --git a/lib/api/bucketDelete.js b/lib/api/bucketDelete.js
@@ -2,7 +2,7 @@ const { errors } = require('arsenal');
 
 const collectCorsHeaders = require('../utilities/collectCorsHeaders');
 const deleteBucket = require('./apiUtils/bucket/bucketDeletion');
-const { metadataValidateBucket } = require('../metadata/metadataUtils');
+const { standardMetadataValidateBucket } = require('../metadata/metadataUtils');
 const { pushMetric } = require('../utapi/utilities');
 
 /**
@@ -31,7 +31,7 @@ function bucketDelete(authInfo, request, log, cb) {
         request,
     };
 
-    return metadataValidateBucket(metadataValParams, log,
+    return standardMetadataValidateBucket(metadataValParams, request.actionImplicitDenies, log,
         (err, bucketMD) => {
             const corsHeaders = collectCorsHeaders(request.headers.origin,
                 request.method, bucketMD);

diff --git a/lib/api/bucketDeleteCors.js b/lib/api/bucketDeleteCors.js
@@ -33,7 +33,8 @@ function bucketDeleteCors(authInfo, request, log, callback) {
         }
         log.trace('found bucket in metadata');
 
-        if (!isBucketAuthorized(bucket, requestType, canonicalID, authInfo, log, request)) {
+        if (!isBucketAuthorized(bucket, requestType, canonicalID, authInfo, log, request,
+            request.actionImplicitDenies)) {
             log.debug('access denied for user on bucket', {
                 requestType,
                 method: 'bucketDeleteCors',

diff --git a/lib/api/bucketDeleteEncryption.js b/lib/api/bucketDeleteEncryption.js
@@ -1,7 +1,7 @@
 const async = require('async');
 
 const metadata = require('../metadata/wrapper');
-const { metadataValidateBucket } = require('../metadata/metadataUtils');
+const { standardMetadataValidateBucket } = require('../metadata/metadataUtils');
 const { pushMetric } = require('../utapi/utilities');
 const collectCorsHeaders = require('../utilities/collectCorsHeaders');
 const { checkExpectedBucketOwner } = require('./apiUtils/authorization/bucketOwner');
@@ -26,7 +26,7 @@ function bucketDeleteEncryption(authInfo, request, log, callback) {
     };
 
     return async.waterfall([
-        next => metadataValidateBucket(metadataValParams, log, next),
+        next => standardMetadataValidateBucket(metadataValParams, request.actionImplicitDenies, log, next),
         (bucket, next) => checkExpectedBucketOwner(request.headers, bucket, log, err => next(err, bucket)),
         (bucket, next) => {
             const sseConfig = bucket.getServerSideEncryption();

diff --git a/lib/api/bucketDeleteLifecycle.js b/lib/api/bucketDeleteLifecycle.js
@@ -1,5 +1,5 @@
 const metadata = require('../metadata/wrapper');
-const { metadataValidateBucket } = require('../metadata/metadataUtils');
+const { standardMetadataValidateBucket } = require('../metadata/metadataUtils');
 const { pushMetric } = require('../utapi/utilities');
 const collectCorsHeaders = require('../utilities/collectCorsHeaders');
 
@@ -20,7 +20,7 @@ function bucketDeleteLifecycle(authInfo, request, log, callback) {
         requestType: 'bucketDeleteLifecycle',
         request,
     };
-    return metadataValidateBucket(metadataValParams, log, (err, bucket) => {
+    return standardMetadataValidateBucket(metadataValParams, request.actionImplicitDenies, log, (err, bucket) => {
         const corsHeaders = collectCorsHeaders(headers.origin, method, bucket);
         if (err) {
             log.debug('error processing request', {

diff --git a/lib/api/bucketDeletePolicy.js b/lib/api/bucketDeletePolicy.js
@@ -1,5 +1,5 @@
 const metadata = require('../metadata/wrapper');
-const { metadataValidateBucket } = require('../metadata/metadataUtils');
+const { standardMetadataValidateBucket } = require('../metadata/metadataUtils');
 const collectCorsHeaders = require('../utilities/collectCorsHeaders');
 
 /**
@@ -19,7 +19,7 @@ function bucketDeletePolicy(authInfo, request, log, callback) {
         requestType: 'bucketDeletePolicy',
         request,
     };
-    return metadataValidateBucket(metadataValParams, log, (err, bucket) => {
+    return standardMetadataValidateBucket(metadataValParams, request.actionImplicitDenies, log, (err, bucket) => {
         const corsHeaders = collectCorsHeaders(headers.origin, method, bucket);
         if (err) {
             log.debug('error processing request', {

diff --git a/lib/api/bucketDeleteReplication.js b/lib/api/bucketDeleteReplication.js
@@ -1,5 +1,5 @@
 const metadata = require('../metadata/wrapper');
-const { metadataValidateBucket } = require('../metadata/metadataUtils');
+const { standardMetadataValidateBucket } = require('../metadata/metadataUtils');
 const { pushMetric } = require('../utapi/utilities');
 const collectCorsHeaders = require('../utilities/collectCorsHeaders');
 
@@ -20,7 +20,7 @@ function bucketDeleteReplication(authInfo, request, log, callback) {
         requestType: 'bucketDeleteReplication',
         request,
     };
-    return metadataValidateBucket(metadataValParams, log, (err, bucket) => {
+    return standardMetadataValidateBucket(metadataValParams, request.actionImplicitDenies, log, (err, bucket) => {
         const corsHeaders = collectCorsHeaders(headers.origin, method, bucket);
         if (err) {
             log.debug('error processing request', {

diff --git a/lib/api/bucketDeleteWebsite.js b/lib/api/bucketDeleteWebsite.js
@@ -25,7 +25,8 @@ function bucketDeleteWebsite(authInfo, request, log, callback) {
         }
         log.trace('found bucket in metadata');
 
-        if (!isBucketAuthorized(bucket, requestType, canonicalID, authInfo, log, request)) {
+        if (!isBucketAuthorized(bucket, requestType, canonicalID, authInfo, log, request,
+            request.actionImplicitDenies)) {
             log.debug('access denied for user on bucket', {
                 requestType,
                 method: 'bucketDeleteWebsite',

diff --git a/lib/api/multiObjectDelete.js b/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,46 @@ 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);
+                }
+                // check whether bucket has transient or deleted flag
+                if (bucketShield(bucketMD, 'objectDelete')) {
+                    return next(errors.NoSuchBucket);
+                }
+                if (!isBucketAuthorized(bucketMD, 'objectDelete', canonicalID, authInfo, log, request,
+                    request.actionImplicitDenies)) {
+                    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
+                    objects.forEach(entry => {
+                        errorResults.push({
+                            entry,
+                            error: errors.AccessDenied,
+                        });
+                    });
+                    // by sending an empty array as the objects 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, 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
@@ -437,7 +468,7 @@ function multiObjectDelete(authInfo, request, log, callback) {
                                 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 +486,11 @@ function multiObjectDelete(authInfo, request, log, callback) {
                         });
                         return next(errors.InternalError);
                     }
+                     // Convert authorization results into an easier to handle format
+                    const actionImplicitDenies = authorizationResults.reduce((acc, curr, idx) => {
+                        const apiMethod = requestContextParams[idx].apiMethod;
+                        return Object.assign({}, acc, { [apiMethod]: curr.isImplicit });
+                    }, {});
                     for (let i = 0; i < authorizationResults.length; i++) {
                         const result = authorizationResults[i];
                         // result is { isAllowed: true,
@@ -470,7 +506,26 @@ 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(actionImplicitDenies),
+                            canonicalID,
+                            authInfo,
+                            actionImplicitDenies,
+                            log,
+                            request);
+
+                        if (areAllActionsAllowed) {
                             inPlay.push(entry);
                         } else {
                             errorResults.push({
@@ -479,50 +534,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, 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,

diff --git a/lib/api/objectDelete.js b/lib/api/objectDelete.js
@@ -8,7 +8,7 @@ const { pushMetric } = require('../utapi/utilities');
 const createAndStoreObject = require('./apiUtils/object/createAndStoreObject');
 const { decodeVersionId, preprocessingVersioningDelete }
     = require('./apiUtils/object/versioning');
-const { metadataValidateBucketAndObj } = require('../metadata/metadataUtils');
+const { standardMetadataValidateBucketAndObj } = require('../metadata/metadataUtils');
 const { hasGovernanceBypassHeader, checkUserGovernanceBypass, ObjectLockInfo }
     = require('./apiUtils/object/objectLockHelpers');
 const { config } = require('../Config');
@@ -56,7 +56,7 @@ function objectDelete(authInfo, request, log, cb) {
     const canonicalID = authInfo.getCanonicalID();
     return async.waterfall([
         function validateBucketAndObj(next) {
-            return metadataValidateBucketAndObj(valParams, log,
+            return standardMetadataValidateBucketAndObj(valParams, request.actionImplicitDenies, log,
             (err, bucketMD, objMD) => {
                 if (err) {
                     return next(err, bucketMD);

diff --git a/lib/api/objectDeleteTagging.js b/lib/api/objectDeleteTagging.js
@@ -4,7 +4,7 @@ const { errors } = require('arsenal');
 const { decodeVersionId, getVersionIdResHeader }
     = require('./apiUtils/object/versioning');
 
-const { metadataValidateBucketAndObj } = require('../metadata/metadataUtils');
+const { standardMetadataValidateBucketAndObj } = require('../metadata/metadataUtils');
 const { pushMetric } = require('../utapi/utilities');
 const collectCorsHeaders = require('../utilities/collectCorsHeaders');
 const metadata = require('../metadata/wrapper');
@@ -46,7 +46,7 @@ function objectDeleteTagging(authInfo, request, log, callback) {
     };
 
     return async.waterfall([
-        next => metadataValidateBucketAndObj(metadataValParams, log,
+        next => standardMetadataValidateBucketAndObj(metadataValParams, request.actionImplicitDenies, log,
           (err, bucket, objectMD) => {
               if (err) {
                   log.trace('request authorization failed',

diff --git a/tests/unit/api/bucketDelete.js b/tests/unit/api/bucketDelete.js
@@ -88,6 +88,7 @@ describe('bucketDelete API', () => {
         namespace,
         headers: {},
         url: `/${bucketName}`,
+        actionImplicitDenies: false,
     };
 
     const initiateRequest = {
@@ -96,6 +97,7 @@ describe('bucketDelete API', () => {
         objectKey: objectName,
         headers: { host: `${bucketName}.s3.amazonaws.com` },
         url: `/${objectName}?uploads`,
+        actionImplicitDenies: false,
     };
 
     it('should return an error if the bucket is not empty', done => {

diff --git a/tests/unit/api/bucketDeleteCors.js b/tests/unit/api/bucketDeleteCors.js
@@ -19,6 +19,7 @@ const testBucketPutRequest = {
     bucketName,
     headers: { host: `${bucketName}.s3.amazonaws.com` },
     url: '/',
+    actionImplicitDenies: false,
 };
 const testBucketPutCorsRequest =
     corsUtil.createBucketCorsRequest('PUT', bucketName);

diff --git a/tests/unit/api/bucketDeleteEncryption.js b/tests/unit/api/bucketDeleteEncryption.js
@@ -13,6 +13,7 @@ const bucketPutRequest = {
     bucketName,
     headers: { host: `${bucketName}.s3.amazonaws.com` },
     url: '/',
+    actionImplicitDenies: false,
 };
 
 describe('bucketDeleteEncryption API', () => {

diff --git a/tests/unit/api/bucketDeleteLifecycle.js b/tests/unit/api/bucketDeleteLifecycle.js
@@ -19,6 +19,7 @@ function _makeRequest(includeXml) {
         bucketName,
         headers: { host: `${bucketName}.s3.amazonaws.com` },
         url: '/',
+        actionImplicitDenies: false,
     };
     if (includeXml) {
         request.post = '<LifecycleConfiguration ' +

diff --git a/tests/unit/api/bucketDeletePolicy.js b/tests/unit/api/bucketDeletePolicy.js
@@ -19,6 +19,7 @@ function _makeRequest(includePolicy) {
         bucketName,
         headers: { host: `${bucketName}.s3.amazonaws.com` },
         url: '/',
+        actionImplicitDenies: false,
     };
     if (includePolicy) {
         const examplePolicy = {

diff --git a/tests/unit/api/bucketDeleteWebsite.js b/tests/unit/api/bucketDeleteWebsite.js
@@ -20,6 +20,7 @@ const testBucketPutRequest = {
     bucketName,
     headers: { host: `${bucketName}.s3.amazonaws.com` },
     url: '/',
+    actionImplicitDenies: false,
 };
 const testBucketDeleteWebsiteRequest = {
     bucketName,
@@ -28,6 +29,7 @@ const testBucketDeleteWebsiteRequest = {
     },
     url: '/?website',
     query: { website: '' },
+    actionImplicitDenies: false,
 };
 const testBucketPutWebsiteRequest = Object.assign({ post: config.getXml() },
     testBucketDeleteWebsiteRequest);

diff --git a/tests/unit/api/objectDeleteTagging.js b/tests/unit/api/objectDeleteTagging.js
@@ -22,6 +22,7 @@ const testBucketPutRequest = {
     bucketName,
     headers: { host: `${bucketName}.s3.amazonaws.com` },
     url: '/',
+    actionImplicitDenies: false,
 };
 
 const testPutObjectRequest = new DummyRequest({