diff --git a/lib/api/bucketDelete.js b/lib/api/bucketDelete.js index 56553d827b..289a665b68 100644 --- 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 index 9518229a24..114b089b59 100644 --- 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 index 793516fc53..4179564e06 100644 --- 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 index 0d6bd4037c..e38a9326b8 100644 --- 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 index d5a85d0bbd..da680e1c9b 100644 --- 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 index 4a93a9bcb9..9279acc20b 100644 --- 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 index 587517a730..3577787bf3 100644 --- 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 index 85a794754e..07bb01b13c 100644 --- 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 index 34e09dfe0d..56bde51da7 100644 --- 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 index c5618a840b..0b496c561d 100644 --- 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 index f627a87c95..ab06ae89d8 100644 --- 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 index 5706647766..981fd62a8d 100644 --- 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 index eca7b261a7..8d231ab131 100644 --- 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 index a36847101a..7654018b32 100644 --- 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 = '