diff --git a/lib/api/bucketDelete.js b/lib/api/bucketDelete.js index a64a162c04..7c3db11a38 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'); const monitoring = require('../utilities/metrics'); @@ -34,7 +34,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 01cacf213f..d8e3336f72 100644 --- a/lib/api/bucketDeleteCors.js +++ b/lib/api/bucketDeleteCors.js @@ -38,7 +38,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 a8850e034a..2e5322aca3 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'); const monitoring = require('../utilities/metrics'); @@ -21,7 +21,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 c40100dd58..fe11566fa2 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'); const monitoring = require('../utilities/metrics'); @@ -21,7 +21,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 f887441003..d2fc28d917 100644 --- a/lib/api/bucketDeleteWebsite.js +++ b/lib/api/bucketDeleteWebsite.js @@ -30,7 +30,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 ce5963327f..7885b284f7 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'); @@ -488,15 +488,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 @@ -540,7 +571,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', { @@ -558,6 +589,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, @@ -573,7 +609,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({ @@ -582,50 +637,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 8ec1578a1f..1e8fbe37b5 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 monitoring = require('../utilities/metrics'); const { hasGovernanceBypassHeader, checkUserGovernanceBypass, ObjectLockInfo } = require('./apiUtils/object/objectLockHelpers'); @@ -61,7 +61,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 b2a465298d..add8d377a1 100644 --- a/lib/api/objectDeleteTagging.js +++ b/lib/api/objectDeleteTagging.js @@ -4,7 +4,7 @@ const { errors } = require('arsenal'); const { decodeVersionId, getVersionIdResHeader, getVersionSpecificMetadataOptions } = require('./apiUtils/object/versioning'); -const { metadataValidateBucketAndObj } = require('../metadata/metadataUtils'); +const { standardMetadataValidateBucketAndObj } = require('../metadata/metadataUtils'); const { pushMetric } = require('../utapi/utilities'); const monitoring = require('../utilities/metrics'); const collectCorsHeaders = require('../utilities/collectCorsHeaders'); @@ -49,7 +49,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/package.json b/package.json index eb7f05780f..79fc0f6703 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "s3", - "version": "7.70.33", + "version": "7.70.34", "description": "S3 connector", "main": "index.js", "engines": { 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 = '