From 2b0eb9ddd169fb84476d6d0df26239c8ed8cd577 Mon Sep 17 00:00:00 2001 From: Will Toozs Date: Mon, 11 Sep 2023 18:05:21 +0200 Subject: [PATCH 1/9] CLDSRV-424: api call updated with implicit deny logic change variable names for clarity edit: update arsenal package --- lib/api/api.js | 40 ++++++++++++++++++++++++++++++++++++---- package.json | 2 +- yarn.lock | 4 ++-- 3 files changed, 39 insertions(+), 7 deletions(-) diff --git a/lib/api/api.js b/lib/api/api.js index bd9e7905f7..0547ac8d45 100644 --- a/lib/api/api.js +++ b/lib/api/api.js @@ -107,6 +107,7 @@ const api = { // no need to check auth on website or cors preflight requests if (apiMethod === 'websiteGet' || apiMethod === 'websiteHead' || apiMethod === 'corsPreflight') { + request.actionImplicitDenies = false; return this[apiMethod](request, log, callback); } @@ -129,15 +130,25 @@ const api = { const requestContexts = prepareRequestContexts(apiMethod, request, sourceBucket, sourceObject, sourceVersionId); + // Extract all the _apiMethods and store them in an array + const apiMethods = requestContexts ? requestContexts.map(context => context._apiMethod) : []; + // Attach the names to the current request + // eslint-disable-next-line no-param-reassign + request.apiMethods = apiMethods; function checkAuthResults(authResults) { let returnTagCount = true; + const isImplicitDeny = {}; + let isOnlyImplicitDeny = true; if (apiMethod === 'objectGet') { // first item checks s3:GetObject(Version) action - if (!authResults[0].isAllowed) { + if (!authResults[0].isAllowed && !authResults[0].isImplicit) { log.trace('get object authorization denial from Vault'); return errors.AccessDenied; } + // TODO add support for returnTagCount in the bucket policy + // checks + isImplicitDeny[authResults[0].action] = authResults[0].isImplicit; // second item checks s3:GetObject(Version)Tagging action if (!authResults[1].isAllowed) { log.trace('get tagging authorization denial ' + @@ -146,13 +157,25 @@ const api = { } } else { for (let i = 0; i < authResults.length; i++) { - if (!authResults[i].isAllowed) { + isImplicitDeny[authResults[i].action] = true; + if (!authResults[i].isAllowed && !authResults[i].isImplicit) { + // Any explicit deny rejects the current API call log.trace('authorization denial from Vault'); return errors.AccessDenied; + } else if (authResults[i].isAllowed) { + // If the action is allowed, the result is not implicit + // Deny. + isImplicitDeny[authResults[i].action] = false; + isOnlyImplicitDeny = false; } } } - return returnTagCount; + // These two APIs cannot use ACLs or Bucket Policies, hence, any + // implicit deny from vault must be treated as an explicit deny. + if ((apiMethod === 'bucketPut' || apiMethod === 'serviceGet') && isOnlyImplicitDeny) { + return errors.AccessDenied; + } + return { returnTagCount, isImplicitDeny }; } return async.waterfall([ @@ -230,7 +253,16 @@ const api = { if (checkedResults instanceof Error) { return callback(checkedResults); } - returnTagCount = checkedResults; + returnTagCount = checkedResults.returnTagCount; + request.actionImplicitDenies = checkedResults.isImplicitDeny; + } else { + // create an object of keys apiMethods with all values to false: + // for backward compatibility, all apiMethods are allowed by default + // thus it is explicitly allowed, so implicit deny is false + request.actionImplicitDenies = apiMethods.reduce((acc, curr) => { + acc[curr] = false; + return acc; + }, {}); } if (apiMethod === 'objectPut' || apiMethod === 'objectPutPart') { request._response = response; diff --git a/package.json b/package.json index 8b79e537fd..00888e576c 100644 --- a/package.json +++ b/package.json @@ -20,7 +20,7 @@ "homepage": "https://github.com/scality/S3#readme", "dependencies": { "@hapi/joi": "^17.1.0", - "arsenal": "git+https://github.com/scality/arsenal#7.10.48", + "arsenal": "git+https://github.com/scality/arsenal#df5ff0f4006ee0a21269e139567fd5c425a4225f", "async": "~2.5.0", "aws-sdk": "2.905.0", "azure-storage": "^2.1.0", diff --git a/yarn.lock b/yarn.lock index 2b6f432141..f9d1e14883 100644 --- a/yarn.lock +++ b/yarn.lock @@ -488,9 +488,9 @@ arraybuffer.slice@~0.0.7: optionalDependencies: ioctl "^2.0.2" -"arsenal@git+https://github.com/scality/arsenal#7.10.48": +"arsenal@git+https://github.com/scality/arsenal#df5ff0f4006ee0a21269e139567fd5c425a4225f": version "7.10.48" - resolved "git+https://github.com/scality/arsenal#f49cea3914390880008e3d41cedb1a02f9d99f39" + resolved "git+https://github.com/scality/arsenal#df5ff0f4006ee0a21269e139567fd5c425a4225f" dependencies: "@types/async" "^3.2.12" "@types/utf8" "^3.0.1" From 9712ebd12aa60d1703d68c37f2baea4d0462036e Mon Sep 17 00:00:00 2001 From: Maha Benzekri Date: Fri, 3 Nov 2023 10:54:03 +0100 Subject: [PATCH 2/9] CLDSRV-424:ARSN version bump --- package.json | 2 +- yarn.lock | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/package.json b/package.json index 00888e576c..f983a22184 100644 --- a/package.json +++ b/package.json @@ -20,7 +20,7 @@ "homepage": "https://github.com/scality/S3#readme", "dependencies": { "@hapi/joi": "^17.1.0", - "arsenal": "git+https://github.com/scality/arsenal#df5ff0f4006ee0a21269e139567fd5c425a4225f", + "arsenal": "git+https://github.com/scality/arsenal#7.10.49", "async": "~2.5.0", "aws-sdk": "2.905.0", "azure-storage": "^2.1.0", diff --git a/yarn.lock b/yarn.lock index f9d1e14883..02cd927998 100644 --- a/yarn.lock +++ b/yarn.lock @@ -488,9 +488,9 @@ arraybuffer.slice@~0.0.7: optionalDependencies: ioctl "^2.0.2" -"arsenal@git+https://github.com/scality/arsenal#df5ff0f4006ee0a21269e139567fd5c425a4225f": - version "7.10.48" - resolved "git+https://github.com/scality/arsenal#df5ff0f4006ee0a21269e139567fd5c425a4225f" +"arsenal@git+https://github.com/scality/arsenal#7.10.49": + version "7.10.49" + resolved "git+https://github.com/scality/arsenal#fbf5562a1180055249745881c1a324562d7cdc8a" dependencies: "@types/async" "^3.2.12" "@types/utf8" "^3.0.1" From 46ce0a9082b2965e8be203bbeaca6731f0f49699 Mon Sep 17 00:00:00 2001 From: Maha Benzekri Date: Fri, 3 Nov 2023 11:02:01 +0100 Subject: [PATCH 3/9] CLDSRV-424:CLDSRV version bump Update lib/api/api.js Co-authored-by: Jonathan Gramain --- lib/api/api.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/api/api.js b/lib/api/api.js index 0547ac8d45..c326d0ac56 100644 --- a/lib/api/api.js +++ b/lib/api/api.js @@ -162,7 +162,8 @@ const api = { // Any explicit deny rejects the current API call log.trace('authorization denial from Vault'); return errors.AccessDenied; - } else if (authResults[i].isAllowed) { + } + if (authResults[i].isAllowed) { // If the action is allowed, the result is not implicit // Deny. isImplicitDeny[authResults[i].action] = false; From 698f5a44d4b83ab0d5272a953718cd02e8faa72c Mon Sep 17 00:00:00 2001 From: Will Toozs Date: Fri, 25 Aug 2023 15:23:37 +0200 Subject: [PATCH 4/9] CLDSRV-426: update ACL permission checks for implicitDeny logic CLDSRV-426:fixups on ACL permission checks for implicitDeny logic CLDSRV-426:better readability on ACL permission --- constants.js | 5 +++ .../authorization/permissionChecks.js | 45 +++++++++++++------ 2 files changed, 37 insertions(+), 13 deletions(-) diff --git a/constants.js b/constants.js index d0d35b9951..6353150de5 100644 --- a/constants.js +++ b/constants.js @@ -174,6 +174,11 @@ const constants = { 'user', 'bucket', ], + arrayOfAllowed: [ + 'objectPutTagging', + 'objectPutLegalHold', + 'objectPutRetention', + ], allowedUtapiEventFilterStates: ['allow', 'deny'], // The AWS assumed Role resource type assumedRoleArnResourceType: 'assumed-role', diff --git a/lib/api/apiUtils/authorization/permissionChecks.js b/lib/api/apiUtils/authorization/permissionChecks.js index 13cefee9f3..46d42046ab 100644 --- a/lib/api/apiUtils/authorization/permissionChecks.js +++ b/lib/api/apiUtils/authorization/permissionChecks.js @@ -2,19 +2,32 @@ const { evaluators, actionMaps, RequestContext } = require('arsenal').policies; const constants = require('../../../../constants'); const { allAuthedUsersId, bucketOwnerActions, logId, publicId, - assumedRoleArnResourceType, backbeatLifecycleSessionName } = constants; + assumedRoleArnResourceType, backbeatLifecycleSessionName, arrayOfAllowed } = constants; // whitelist buckets to allow public read on objects const publicReadBuckets = process.env.ALLOW_PUBLIC_READ_BUCKETS ? process.env.ALLOW_PUBLIC_READ_BUCKETS.split(',') : []; -function checkBucketAcls(bucket, requestType, canonicalID) { +function checkBucketAcls(bucket, requestType, canonicalID, mainApiCall) { + // Same logic applies on the Versioned APIs, so let's simplify it. + const requestTypeParsed = requestType.endsWith('Version') ? + requestType.slice(0, 'Version'.length * -1) : requestType; if (bucket.getOwner() === canonicalID) { return true; } + if (mainApiCall === 'objectGet') { + if (requestTypeParsed === 'objectGetTagging') { + return true; + } + } + if (mainApiCall === 'objectPut') { + if (arrayOfAllowed.includes(requestTypeParsed)) { + return true; + } + } const bucketAcl = bucket.getAcl(); - if (requestType === 'bucketGet' || requestType === 'bucketHead') { + if (requestTypeParsed === 'bucketGet' || requestTypeParsed === 'bucketHead') { if (bucketAcl.Canned === 'public-read' || bucketAcl.Canned === 'public-read-write' || (bucketAcl.Canned === 'authenticated-read' @@ -32,7 +45,7 @@ function checkBucketAcls(bucket, requestType, canonicalID) { return true; } } - if (requestType === 'bucketGetACL') { + if (requestTypeParsed === 'bucketGetACL') { if ((bucketAcl.Canned === 'log-delivery-write' && canonicalID === logId) || bucketAcl.FULL_CONTROL.indexOf(canonicalID) > -1 @@ -48,7 +61,7 @@ function checkBucketAcls(bucket, requestType, canonicalID) { } } - if (requestType === 'bucketPutACL') { + if (requestTypeParsed === 'bucketPutACL') { if (bucketAcl.FULL_CONTROL.indexOf(canonicalID) > -1 || bucketAcl.WRITE_ACP.indexOf(canonicalID) > -1) { return true; @@ -62,11 +75,7 @@ function checkBucketAcls(bucket, requestType, canonicalID) { } } - if (requestType === 'bucketDelete' && bucket.getOwner() === canonicalID) { - return true; - } - - if (requestType === 'objectDelete' || requestType === 'objectPut') { + if (requestTypeParsed === 'objectDelete' || requestTypeParsed === 'objectPut') { if (bucketAcl.Canned === 'public-read-write' || bucketAcl.FULL_CONTROL.indexOf(canonicalID) > -1 || bucketAcl.WRITE.indexOf(canonicalID) > -1) { @@ -86,11 +95,12 @@ function checkBucketAcls(bucket, requestType, canonicalID) { // objectPutACL, objectGetACL, objectHead or objectGet, the bucket // authorization check should just return true so can move on to check // rights at the object level. - return (requestType === 'objectPutACL' || requestType === 'objectGetACL' || - requestType === 'objectGet' || requestType === 'objectHead'); + return (requestTypeParsed === 'objectPutACL' || requestTypeParsed === 'objectGetACL' || + requestTypeParsed === 'objectGet' || requestTypeParsed === 'objectHead'); } -function checkObjectAcls(bucket, objectMD, requestType, canonicalID) { +function checkObjectAcls(bucket, objectMD, requestType, canonicalID, requesterIsNotUser, + isUserUnauthenticated, mainApiCall) { const bucketOwner = bucket.getOwner(); // acls don't distinguish between users and accounts, so both should be allowed if (bucketOwnerActions.includes(requestType) @@ -100,6 +110,15 @@ function checkObjectAcls(bucket, objectMD, requestType, canonicalID) { if (objectMD['owner-id'] === canonicalID) { return true; } + + // Backward compatibility + if (mainApiCall === 'objectGet') { + if ((isUserUnauthenticated || (requesterIsNotUser && bucketOwner === objectMD['owner-id'])) + && requestType === 'objectGetTagging') { + return true; + } + } + if (!objectMD.acl) { return false; } From 10a0672b68bc482de85aa0ac5316fb012c9ed8ab Mon Sep 17 00:00:00 2001 From: Will Toozs Date: Fri, 25 Aug 2023 15:23:54 +0200 Subject: [PATCH 5/9] CLDSRV-426: add tests for ACL permission check updates CLDSRV-426: additionnal test for ACL permission --- tests/unit/auth/permissionChecks.js | 317 ++++++++++++++++++++++++++++ 1 file changed, 317 insertions(+) create mode 100644 tests/unit/auth/permissionChecks.js diff --git a/tests/unit/auth/permissionChecks.js b/tests/unit/auth/permissionChecks.js new file mode 100644 index 0000000000..e9d33999da --- /dev/null +++ b/tests/unit/auth/permissionChecks.js @@ -0,0 +1,317 @@ +const assert = require('assert'); +const { checkBucketAcls, checkObjectAcls } = require('../../../lib/api/apiUtils/authorization/permissionChecks'); +const constants = require('../../../constants'); + +const { bucketOwnerActions, logId } = constants; + +describe('checkBucketAcls', () => { + const mockBucket = { + getOwner: () => 'ownerId', + getAcl: () => ({ + Canned: '', + FULL_CONTROL: [], + READ: [], + READ_ACP: [], + WRITE: [], + WRITE_ACP: [], + }), + }; + + const testScenarios = [ + { + description: 'should return true if bucket owner matches canonicalID', + input: { + bucketAcl: {}, requestType: 'anyType', canonicalID: 'ownerId', mainApiCall: 'anyApiCall', + }, + expected: true, + }, + { + description: 'should return true for objectGetTagging when mainApiCall is objectGet', + input: { + bucketAcl: {}, requestType: 'objectGetTagging', canonicalID: 'anyId', mainApiCall: 'objectGet', + }, + expected: true, + }, + { + description: 'should return true for objectPutTagging when mainApiCall is objectPut', + input: { + bucketAcl: {}, requestType: 'objectPutTagging', canonicalID: 'anyId', mainApiCall: 'objectPut', + }, + expected: true, + }, + { + description: 'should return true for objectPutLegalHold when mainApiCall is objectPut', + input: { + bucketAcl: {}, requestType: 'objectPutLegalHold', canonicalID: 'anyId', mainApiCall: 'objectPut', + }, + expected: true, + }, + { + description: 'should return true for objectPutRetention when mainApiCall is objectPut', + input: { + bucketAcl: {}, requestType: 'objectPutRetention', canonicalID: 'anyId', mainApiCall: 'objectPut', + }, + expected: true, + }, + { + description: 'should return true for bucketGet if canned acl is public-read-write', + input: { + bucketAcl: { Canned: 'public-read-write' }, + requestType: 'bucketGet', + canonicalID: 'anyId', + mainApiCall: 'anyApiCall', + }, + expected: true, + }, + { + description: 'should return true for bucketGet if canned acl is authenticated-read and id is not publicId', + input: { + bucketAcl: { Canned: 'authenticated-read' }, + requestType: 'bucketGet', + canonicalID: 'anyIdNotPublic', + mainApiCall: 'anyApiCall', + }, + expected: true, + }, + { + description: 'should return true for bucketHead if canned acl is public-read', + input: { + bucketAcl: { Canned: 'public-read' }, + requestType: 'bucketHead', + canonicalID: 'anyId', + mainApiCall: 'anyApiCall', + }, + expected: true, + }, + { + description: 'should return false for bucketPut even if canonicalID has FULL_CONTROL and write access ', + input: { + bucketAcl: { + FULL_CONTROL: ['anyId'], + WRITE: ['anyId'], + }, + requestType: 'bucketPut', + canonicalID: 'anyId', + mainApiCall: 'anyApiCall', + }, + expected: false, + }, + { + description: 'should return true for log-delivery-write ACL when canonicalID matches logId', + input: { + bucketAcl: { Canned: 'log-delivery-write' }, + requestType: 'bucketGetACL', + canonicalID: logId, + mainApiCall: 'anyApiCall', + }, + expected: true, + }, + { + description: 'should return false when the canonicalID is not the owner and has no ACL permissions', + input: { + bucketAcl: { + FULL_CONTROL: ['someOtherId'], + WRITE: ['someOtherId'], + }, + requestType: 'objectPut', + canonicalID: 'anyId', + mainApiCall: 'anyApiCall', + }, + expected: false, + }, + { + description: 'should return false for bucketPutACL if canonicalID does not have ACL permissions', + input: { + bucketAcl: { + FULL_CONTROL: ['someOtherId'], + WRITE_ACP: ['someOtherId'], + }, + requestType: 'bucketPutACL', + canonicalID: 'anyId', + mainApiCall: 'anyApiCall', + }, + expected: false, + }, + { + description: 'should return true for bucketGet if canonicalID has FULL_CONTROL access', + input: { + bucketAcl: { FULL_CONTROL: ['anyId'], READ: [] }, + requestType: 'bucketGet', + canonicalID: 'anyId', + mainApiCall: 'anyApiCall', + }, + expected: true, + }, + { + description: 'should return true for bucketGetACL if canonicalID has FULL_CONTROL', + input: { + bucketAcl: { FULL_CONTROL: ['anyId'], READ_ACP: [] }, + requestType: 'bucketGetACL', + canonicalID: 'anyId', + mainApiCall: 'anyApiCall', + }, + expected: true, + }, + { + description: 'should return true for objectDelete if bucketAcl.Canned is public-read-write', + input: { + bucketAcl: { Canned: 'public-read-write' }, + requestType: 'objectDelete', + canonicalID: 'anyId', + mainApiCall: 'anyApiCall', + }, + expected: true, + }, + { + description: 'should return true for requestType ending with "Version"', + input: { + bucketAcl: {}, + requestType: 'objectGetVersion', + canonicalID: 'anyId', + mainApiCall: 'objectGet', + }, + expected: true, + }, + { + description: 'should return true for objectPutACL', + input: { + bucketAcl: {}, + requestType: 'objectPutACL', + canonicalID: 'anyId', + mainApiCall: 'anyApiCall', + }, + expected: true, + }, + { + description: 'should return true for objectGetACL', + input: { + bucketAcl: {}, + requestType: 'objectGetACL', + canonicalID: 'anyId', + mainApiCall: 'anyApiCall', + }, + expected: true, + }, + { + description: 'should return false for unmatched scenarios', + input: { + bucketAcl: {}, + requestType: 'unmatchedRequest', + canonicalID: 'anyId', + mainApiCall: 'anyApiCall', + }, + expected: false, + }, + ]; + + testScenarios.forEach(scenario => { + it(scenario.description, () => { + // Mock the bucket based on the test scenario's input + mockBucket.getAcl = () => scenario.input.bucketAcl; + + const result = checkBucketAcls(mockBucket, + scenario.input.requestType, scenario.input.canonicalID, scenario.input.mainApiCall); + assert.strictEqual(result, scenario.expected); + }); + }); +}); + +describe('checkObjectAcls', () => { + const mockBucket = { + getOwner: () => 'bucketOwnerId', + getName: () => 'bucketName', + getAcl: () => ({ Canned: '' }), + }; + const mockObjectMD = { + 'owner-id': 'objectOwnerId', + 'acl': { + Canned: '', + FULL_CONTROL: [], + READ: [], + READ_ACP: [], + WRITE: [], + WRITE_ACP: [], + }, + }; + + it('should return true if request type is in bucketOwnerActions and bucket owner matches canonicalID', () => { + assert.strictEqual(checkObjectAcls(mockBucket, mockObjectMD, bucketOwnerActions[0], + 'bucketOwnerId', false, false, 'anyApiCall'), true); + }); + + it('should return true if objectMD owner matches canonicalID', () => { + assert.strictEqual(checkObjectAcls(mockBucket, mockObjectMD, 'anyType', + 'objectOwnerId', false, false, 'anyApiCall'), true); + }); + + it('should return true for objectGetTagging when mainApiCall is objectGet and conditions met', () => { + assert.strictEqual(checkObjectAcls(mockBucket, mockObjectMD, 'objectGetTagging', + 'anyIdNotPublic', true, true, 'objectGet'), true); + }); + + it('should return false if no acl provided in objectMD', () => { + const objMDWithoutAcl = Object.assign({}, mockObjectMD); + delete objMDWithoutAcl.acl; + assert.strictEqual(checkObjectAcls(mockBucket, objMDWithoutAcl, 'anyType', + 'anyId', false, false, 'anyApiCall'), false); + }); + + const tests = [ + { + acl: 'public-read', reqType: 'objectGet', id: 'anyIdNotPublic', expected: true, + }, + { + acl: 'public-read-write', reqType: 'objectGet', id: 'anyIdNotPublic', expected: true, + }, + { + acl: 'authenticated-read', reqType: 'objectGet', id: 'anyIdNotPublic', expected: true, + }, + { + acl: 'bucket-owner-read', reqType: 'objectGet', id: 'bucketOwnerId', expected: true, + }, + { + acl: 'bucket-owner-full-control', reqType: 'objectGet', id: 'bucketOwnerId', expected: true, + }, + { + aclList: ['someId', 'anyIdNotPublic'], + aclField: 'FULL_CONTROL', + reqType: 'objectGet', + id: 'anyIdNotPublic', + expected: true, + }, + { + aclList: ['someId', 'anyIdNotPublic'], + aclField: 'READ', + reqType: 'objectGet', + id: 'anyIdNotPublic', + expected: true, + }, + { reqType: 'objectPut', id: 'anyId', expected: true }, + { reqType: 'objectDelete', id: 'anyId', expected: true }, + { + aclList: ['anyId'], aclField: 'FULL_CONTROL', reqType: 'objectPutACL', id: 'anyId', expected: true, + }, + { + aclList: ['anyId'], aclField: 'FULL_CONTROL', reqType: 'objectGetACL', id: 'anyId', expected: true, + }, + { + acl: '', reqType: 'objectGet', id: 'randomId', expected: false, + }, + ]; + + tests.forEach(test => { + it(`should return ${test.expected} for ${test.reqType} with ACL as ${test.acl + || (`${test.aclField}:${JSON.stringify(test.aclList)}`)}`, () => { + if (test.acl) { + mockObjectMD.acl.Canned = test.acl; + } else if (test.aclList && test.aclField) { + mockObjectMD.acl[test.aclField] = test.aclList; + } + + assert.strictEqual( + checkObjectAcls(mockBucket, mockObjectMD, test.reqType, test.id, false, false, 'anyApiCall'), + test.expected, + ); + }); + }); +}); From 7aa326cba9f13975f8cdba885a2ea75be5503414 Mon Sep 17 00:00:00 2001 From: Will Toozs Date: Mon, 28 Aug 2023 11:38:50 +0200 Subject: [PATCH 6/9] =?UTF-8?q?CLDSRV-427:=20update=20bucket/object=20perm?= =?UTF-8?q?=20checks=20to=20account=20for=20implicit=20=E2=80=A6=20?= =?UTF-8?q?=E2=80=A6denies?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit (cherry picked from commit c01898f1a002508e9c29662a2a5b07a4b0a26dc8) --- .../authorization/permissionChecks.js | 179 +++++++++++++++--- 1 file changed, 157 insertions(+), 22 deletions(-) diff --git a/lib/api/apiUtils/authorization/permissionChecks.js b/lib/api/apiUtils/authorization/permissionChecks.js index 46d42046ab..84522eebce 100644 --- a/lib/api/apiUtils/authorization/permissionChecks.js +++ b/lib/api/apiUtils/authorization/permissionChecks.js @@ -287,7 +287,26 @@ function checkBucketPolicy(policy, requestType, canonicalID, arn, bucketOwner, l return permission; } -function isBucketAuthorized(bucket, requestType, canonicalID, authInfo, log, request) { +function isBucketAuthorizedNew(bucket, requestTypes, canonicalID, authInfo, iamAuthzResults, log, request) { + if (!Array.isArray(requestTypes)) { + // eslint-disable-next-line no-param-reassign + requestTypes = [requestTypes]; + } + if (iamAuthzResults === false) { + // eslint-disable-next-line no-param-reassign + iamAuthzResults = {}; + } + // By default, all missing actions are defined as allowed from IAM, to be + // backward compatible + requestTypes.forEach(requestType => { + if (iamAuthzResults[requestType] === undefined) { + // eslint-disable-next-line no-param-reassign + iamAuthzResults[requestType] = false; + } + }); + const mainApiCall = requestTypes[0]; + const results = {}; + requestTypes.forEach(_requestType => { // Check to see if user is authorized to perform a // particular action on bucket based on ACLs. // TODO: Add IAM checks @@ -299,63 +318,178 @@ function isBucketAuthorized(bucket, requestType, canonicalID, authInfo, log, req } // if the bucket owner is an account, users should not have default access if ((bucket.getOwner() === canonicalID) && requesterIsNotUser) { - return true; + results[_requestType] = iamAuthzResults[_requestType] === false; + return; } - const aclPermission = checkBucketAcls(bucket, requestType, canonicalID); + const aclPermission = checkBucketAcls(bucket, _requestType, canonicalID, mainApiCall); const bucketPolicy = bucket.getBucketPolicy(); if (!bucketPolicy) { - return aclPermission; + results[_requestType] = iamAuthzResults[_requestType] === false && aclPermission; + return; } - const bucketPolicyPermission = checkBucketPolicy(bucketPolicy, requestType, + const bucketPolicyPermission = checkBucketPolicy(bucketPolicy, _requestType, canonicalID, arn, bucket.getOwner(), log, request); if (bucketPolicyPermission === 'explicitDeny') { - return false; - } - return (aclPermission || (bucketPolicyPermission === 'allow')); + results[_requestType] = false; + return; + } + // If the bucket policy returns an allow, we accept the request, as the + // IAM response here is either Allow or implicit deny. + if (bucketPolicyPermission === 'allow') { + results[_requestType] = true; + return; } + results[_requestType] = iamAuthzResults[_requestType] === false && aclPermission; + }); -function isObjAuthorized(bucket, objectMD, requestType, canonicalID, authInfo, log, request) { + // final result is true if all the results are true + return Object.keys(results).every(key => results[key] === true); +} + +function isBucketAuthorized(bucket, requestType, canonicalID, authInfo, log, request) { + const iamAuthzResults = {}; + const result = isBucketAuthorizedNew(bucket, [requestType], canonicalID, authInfo, iamAuthzResults, log, request); + + return result; +} + +function isObjAuthorizedNew(bucket, objectMD, requestTypes, canonicalID, authInfo, iamAuthzResults, log, request) { + if (!Array.isArray(requestTypes)) { + // eslint-disable-next-line no-param-reassign + requestTypes = [requestTypes]; + } + // By default, all missing actions are defined as allowed from IAM, to be + // backward compatible + if (iamAuthzResults === false) { + // eslint-disable-next-line no-param-reassign + iamAuthzResults = {}; + } + requestTypes.forEach(requestType => { + if (iamAuthzResults[requestType] === undefined) { + // eslint-disable-next-line no-param-reassign + iamAuthzResults[requestType] = false; + } + }); + const results = {}; + const mainApiCall = requestTypes[0]; + requestTypes.forEach(_requestType => { + const parsedMethodName = _requestType.endsWith('Version') ? + _requestType.slice(0, -7) : _requestType; const bucketOwner = bucket.getOwner(); if (!objectMD) { // User is already authorized on the bucket for FULL_CONTROL or WRITE or // bucket has canned ACL public-read-write - if (requestType === 'objectPut' || requestType === 'objectDelete') { - return true; + if (parsedMethodName === 'objectPut' || parsedMethodName === 'objectDelete') { + results[_requestType] = iamAuthzResults[_requestType] === false; + return; } // check bucket has read access // 'bucketGet' covers listObjects and listMultipartUploads, bucket read actions - return isBucketAuthorized(bucket, 'bucketGet', canonicalID, authInfo, log, request); + results[_requestType] = isBucketAuthorized(bucket, 'bucketGet', canonicalID, authInfo, false, log, request); + return; } let requesterIsNotUser = true; let arn = null; + let isUserUnauthenticated = false; if (authInfo) { requesterIsNotUser = !authInfo.isRequesterAnIAMUser(); arn = authInfo.getArn(); + isUserUnauthenticated = arn === undefined; } if (objectMD['owner-id'] === canonicalID && requesterIsNotUser) { - return true; + results[_requestType] = iamAuthzResults[_requestType] === false; + return; } // account is authorized if: // - requesttype is included in bucketOwnerActions and // - account is the bucket owner // - requester is account, not user - if (bucketOwnerActions.includes(requestType) + if (bucketOwnerActions.includes(parsedMethodName) && (bucketOwner === canonicalID) && requesterIsNotUser) { - return true; + results[_requestType] = iamAuthzResults[_requestType] === false; + return; + } + const aclPermission = checkObjectAcls(bucket, objectMD, parsedMethodName, + canonicalID, requesterIsNotUser, isUserUnauthenticated, mainApiCall); + const bucketPolicy = bucket.getBucketPolicy(); + if (!bucketPolicy) { + results[_requestType] = iamAuthzResults[_requestType] === false && aclPermission; + return; + } + const bucketPolicyPermission = checkBucketPolicy(bucketPolicy, _requestType, + canonicalID, arn, bucket.getOwner(), log, request); + if (bucketPolicyPermission === 'explicitDeny') { + results[_requestType] = false; + return; + } + // If the bucket policy returns an allow, we accept the request, as the + // IAM response here is either Allow or implicit deny. + if (bucketPolicyPermission === 'allow') { + results[_requestType] = true; + return; + } + results[_requestType] = iamAuthzResults[_requestType] === false && aclPermission; + }); + + // final result is true if all the results are true + return Object.keys(results).every(key => results[key] === true); +} + +function isObjAuthorized(bucket, objectMD, requestType, canonicalID, authInfo, log, request) { + const iamAuthzResults = {}; + const result = isObjAuthorizedNew(bucket, objectMD, + [requestType], canonicalID, authInfo, iamAuthzResults, log, request); + + return result; +} + +function evaluateBucketPolicyWithIAM(bucket, requestTypes, canonicalID, authInfo, iamAuthzResults, log, request) { + if (!Array.isArray(requestTypes)) { + // eslint-disable-next-line no-param-reassign + requestTypes = [requestTypes]; + } + if (iamAuthzResults === false) { + // eslint-disable-next-line no-param-reassign + iamAuthzResults = {}; + } + // By default, all missing actions are defined as allowed from IAM, to be + // backward compatible + requestTypes.forEach(requestType => { + if (iamAuthzResults[requestType] === undefined) { + // eslint-disable-next-line no-param-reassign + iamAuthzResults[requestType] = false; + } + }); + + const results = {}; + requestTypes.forEach(_requestType => { + let arn = null; + if (authInfo) { + arn = authInfo.getArn(); } - const aclPermission = checkObjectAcls(bucket, objectMD, requestType, - canonicalID); const bucketPolicy = bucket.getBucketPolicy(); if (!bucketPolicy) { - return aclPermission; + results[_requestType] = iamAuthzResults[_requestType] === false; + return; } - const bucketPolicyPermission = checkBucketPolicy(bucketPolicy, requestType, + const bucketPolicyPermission = checkBucketPolicy(bucketPolicy, _requestType, canonicalID, arn, bucket.getOwner(), log, request); if (bucketPolicyPermission === 'explicitDeny') { - return false; - } - return (aclPermission || (bucketPolicyPermission === 'allow')); + results[_requestType] = false; + return; + } + // If the bucket policy returns an allow, we accept the request, as the + // IAM response here is either Allow or implicit deny. + if (bucketPolicyPermission === 'allow') { + results[_requestType] = true; + return; + } + results[_requestType] = iamAuthzResults[_requestType] === false; + }); + + // final result is true if all the results are true + return Object.keys(results).every(key => results[key] === true); } function _checkResource(resource, bucketArn) { @@ -414,4 +548,5 @@ module.exports = { checkObjectAcls, validatePolicyResource, isLifecycleSession, + evaluateBucketPolicyWithIAM, }; From a7396a721c13f911f5e92625df75f2295d306bf9 Mon Sep 17 00:00:00 2001 From: Maha Benzekri Date: Fri, 3 Nov 2023 10:15:23 +0100 Subject: [PATCH 7/9] CLDSRV-427: linting fixups and retrocompatibility changes --- .../authorization/permissionChecks.js | 159 ++++++++---------- 1 file changed, 72 insertions(+), 87 deletions(-) diff --git a/lib/api/apiUtils/authorization/permissionChecks.js b/lib/api/apiUtils/authorization/permissionChecks.js index 84522eebce..5fc2ca19be 100644 --- a/lib/api/apiUtils/authorization/permissionChecks.js +++ b/lib/api/apiUtils/authorization/permissionChecks.js @@ -287,87 +287,79 @@ function checkBucketPolicy(policy, requestType, canonicalID, arn, bucketOwner, l return permission; } -function isBucketAuthorizedNew(bucket, requestTypes, canonicalID, authInfo, iamAuthzResults, log, request) { +function isBucketAuthorized(bucket, requestTypes, canonicalID, authInfo, log, request, actionImplicitDenies) { if (!Array.isArray(requestTypes)) { // eslint-disable-next-line no-param-reassign requestTypes = [requestTypes]; } - if (iamAuthzResults === false) { + if (!actionImplicitDenies) { // eslint-disable-next-line no-param-reassign - iamAuthzResults = {}; + actionImplicitDenies = {}; } // By default, all missing actions are defined as allowed from IAM, to be // backward compatible requestTypes.forEach(requestType => { - if (iamAuthzResults[requestType] === undefined) { + if (actionImplicitDenies[requestType] === undefined) { // eslint-disable-next-line no-param-reassign - iamAuthzResults[requestType] = false; + actionImplicitDenies[requestType] = false; } }); const mainApiCall = requestTypes[0]; const results = {}; requestTypes.forEach(_requestType => { - // Check to see if user is authorized to perform a - // particular action on bucket based on ACLs. - // TODO: Add IAM checks - let requesterIsNotUser = true; - let arn = null; - if (authInfo) { - requesterIsNotUser = !authInfo.isRequesterAnIAMUser(); - arn = authInfo.getArn(); - } - // if the bucket owner is an account, users should not have default access - if ((bucket.getOwner() === canonicalID) && requesterIsNotUser) { - results[_requestType] = iamAuthzResults[_requestType] === false; + // Check to see if user is authorized to perform a + // particular action on bucket based on ACLs. + // TODO: Add IAM checks + let requesterIsNotUser = true; + let arn = null; + if (authInfo) { + requesterIsNotUser = !authInfo.isRequesterAnIAMUser(); + arn = authInfo.getArn(); + } + // if the bucket owner is an account, users should not have default access + if ((bucket.getOwner() === canonicalID) && requesterIsNotUser) { + results[_requestType] = actionImplicitDenies[_requestType] === false; return; - } + } const aclPermission = checkBucketAcls(bucket, _requestType, canonicalID, mainApiCall); - const bucketPolicy = bucket.getBucketPolicy(); - if (!bucketPolicy) { - results[_requestType] = iamAuthzResults[_requestType] === false && aclPermission; + const bucketPolicy = bucket.getBucketPolicy(); + if (!bucketPolicy) { + results[_requestType] = actionImplicitDenies[_requestType] === false && aclPermission; return; - } + } const bucketPolicyPermission = checkBucketPolicy(bucketPolicy, _requestType, - canonicalID, arn, bucket.getOwner(), log, request); - if (bucketPolicyPermission === 'explicitDeny') { + canonicalID, arn, bucket.getOwner(), log, request); + if (bucketPolicyPermission === 'explicitDeny') { results[_requestType] = false; return; - } + } // If the bucket policy returns an allow, we accept the request, as the // IAM response here is either Allow or implicit deny. if (bucketPolicyPermission === 'allow') { results[_requestType] = true; return; -} - results[_requestType] = iamAuthzResults[_requestType] === false && aclPermission; + } + results[_requestType] = actionImplicitDenies[_requestType] === false && aclPermission; }); // final result is true if all the results are true return Object.keys(results).every(key => results[key] === true); } - -function isBucketAuthorized(bucket, requestType, canonicalID, authInfo, log, request) { - const iamAuthzResults = {}; - const result = isBucketAuthorizedNew(bucket, [requestType], canonicalID, authInfo, iamAuthzResults, log, request); - - return result; -} - -function isObjAuthorizedNew(bucket, objectMD, requestTypes, canonicalID, authInfo, iamAuthzResults, log, request) { +function isObjAuthorized(bucket, objectMD, requestTypes, canonicalID, authInfo, log, request, actionImplicitDenies) { if (!Array.isArray(requestTypes)) { // eslint-disable-next-line no-param-reassign requestTypes = [requestTypes]; } // By default, all missing actions are defined as allowed from IAM, to be // backward compatible - if (iamAuthzResults === false) { + if (!actionImplicitDenies) { // eslint-disable-next-line no-param-reassign - iamAuthzResults = {}; + actionImplicitDenies = {}; } requestTypes.forEach(requestType => { - if (iamAuthzResults[requestType] === undefined) { + if (actionImplicitDenies[requestType] === undefined) { // eslint-disable-next-line no-param-reassign - iamAuthzResults[requestType] = false; + actionImplicitDenies[requestType] = false; } }); const results = {}; @@ -375,46 +367,46 @@ function isObjAuthorizedNew(bucket, objectMD, requestTypes, canonicalID, authInf requestTypes.forEach(_requestType => { const parsedMethodName = _requestType.endsWith('Version') ? _requestType.slice(0, -7) : _requestType; - const bucketOwner = bucket.getOwner(); - if (!objectMD) { - // User is already authorized on the bucket for FULL_CONTROL or WRITE or - // bucket has canned ACL public-read-write + const bucketOwner = bucket.getOwner(); + 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] = iamAuthzResults[_requestType] === false; + 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, false, log, request); + } + // check bucket has read access + // 'bucketGet' covers listObjects and listMultipartUploads, bucket read actions + results[_requestType] = isBucketAuthorized(bucket, 'bucketGet', canonicalID, authInfo, log, request, false); return; - } - let requesterIsNotUser = true; - let arn = null; + } + let requesterIsNotUser = true; + let arn = null; let isUserUnauthenticated = false; - if (authInfo) { - requesterIsNotUser = !authInfo.isRequesterAnIAMUser(); - arn = authInfo.getArn(); + if (authInfo) { + requesterIsNotUser = !authInfo.isRequesterAnIAMUser(); + arn = authInfo.getArn(); isUserUnauthenticated = arn === undefined; - } - if (objectMD['owner-id'] === canonicalID && requesterIsNotUser) { - results[_requestType] = iamAuthzResults[_requestType] === false; + } + if (objectMD['owner-id'] === canonicalID && requesterIsNotUser) { + results[_requestType] = actionImplicitDenies[_requestType] === false; return; - } - // account is authorized if: - // - requesttype is included in bucketOwnerActions and - // - account is the bucket owner - // - requester is account, not user + } + // account is authorized if: + // - requesttype is included in bucketOwnerActions and + // - account is the bucket owner + // - requester is account, not user if (bucketOwnerActions.includes(parsedMethodName) && (bucketOwner === canonicalID) && requesterIsNotUser) { - results[_requestType] = iamAuthzResults[_requestType] === false; + results[_requestType] = actionImplicitDenies[_requestType] === false; return; } const aclPermission = checkObjectAcls(bucket, objectMD, parsedMethodName, canonicalID, requesterIsNotUser, isUserUnauthenticated, mainApiCall); const bucketPolicy = bucket.getBucketPolicy(); if (!bucketPolicy) { - results[_requestType] = iamAuthzResults[_requestType] === false && aclPermission; + results[_requestType] = actionImplicitDenies[_requestType] === false && aclPermission; return; } const bucketPolicyPermission = checkBucketPolicy(bucketPolicy, _requestType, @@ -429,36 +421,28 @@ function isObjAuthorizedNew(bucket, objectMD, requestTypes, canonicalID, authInf results[_requestType] = true; return; } - results[_requestType] = iamAuthzResults[_requestType] === false && aclPermission; + results[_requestType] = actionImplicitDenies[_requestType] === false && aclPermission; }); // final result is true if all the results are true return Object.keys(results).every(key => results[key] === true); } -function isObjAuthorized(bucket, objectMD, requestType, canonicalID, authInfo, log, request) { - const iamAuthzResults = {}; - const result = isObjAuthorizedNew(bucket, objectMD, - [requestType], canonicalID, authInfo, iamAuthzResults, log, request); - - return result; -} - -function evaluateBucketPolicyWithIAM(bucket, requestTypes, canonicalID, authInfo, iamAuthzResults, log, request) { +function evaluateBucketPolicyWithIAM(bucket, requestTypes, canonicalID, authInfo, actionImplicitDenies, log, request) { if (!Array.isArray(requestTypes)) { // eslint-disable-next-line no-param-reassign requestTypes = [requestTypes]; } - if (iamAuthzResults === false) { + if (actionImplicitDenies === false) { // eslint-disable-next-line no-param-reassign - iamAuthzResults = {}; + actionImplicitDenies = {}; } // By default, all missing actions are defined as allowed from IAM, to be // backward compatible requestTypes.forEach(requestType => { - if (iamAuthzResults[requestType] === undefined) { + if (actionImplicitDenies[requestType] === undefined) { // eslint-disable-next-line no-param-reassign - iamAuthzResults[requestType] = false; + actionImplicitDenies[requestType] = false; } }); @@ -467,25 +451,25 @@ function evaluateBucketPolicyWithIAM(bucket, requestTypes, canonicalID, authInfo let arn = null; if (authInfo) { arn = authInfo.getArn(); - } - const bucketPolicy = bucket.getBucketPolicy(); - if (!bucketPolicy) { - results[_requestType] = iamAuthzResults[_requestType] === false; + } + const bucketPolicy = bucket.getBucketPolicy(); + if (!bucketPolicy) { + results[_requestType] = actionImplicitDenies[_requestType] === false; return; - } + } const bucketPolicyPermission = checkBucketPolicy(bucketPolicy, _requestType, canonicalID, arn, bucket.getOwner(), log, request); - if (bucketPolicyPermission === 'explicitDeny') { + if (bucketPolicyPermission === 'explicitDeny') { results[_requestType] = false; return; - } + } // If the bucket policy returns an allow, we accept the request, as the // IAM response here is either Allow or implicit deny. if (bucketPolicyPermission === 'allow') { results[_requestType] = true; return; } - results[_requestType] = iamAuthzResults[_requestType] === false; + results[_requestType] = actionImplicitDenies[_requestType] === false; }); // final result is true if all the results are true @@ -550,3 +534,4 @@ module.exports = { isLifecycleSession, evaluateBucketPolicyWithIAM, }; + From 6fb225eb5cc61366778ddf263b581d806723aa13 Mon Sep 17 00:00:00 2001 From: Maha Benzekri Date: Mon, 6 Nov 2023 21:24:10 +0100 Subject: [PATCH 8/9] CLDSRV-427: functions refactor --- .../authorization/permissionChecks.js | 205 ++++++------------ 1 file changed, 66 insertions(+), 139 deletions(-) diff --git a/lib/api/apiUtils/authorization/permissionChecks.js b/lib/api/apiUtils/authorization/permissionChecks.js index 5fc2ca19be..a52f98419b 100644 --- a/lib/api/apiUtils/authorization/permissionChecks.js +++ b/lib/api/apiUtils/authorization/permissionChecks.js @@ -287,65 +287,12 @@ function checkBucketPolicy(policy, requestType, canonicalID, arn, bucketOwner, l return permission; } -function isBucketAuthorized(bucket, requestTypes, canonicalID, authInfo, log, request, actionImplicitDenies) { - if (!Array.isArray(requestTypes)) { +function isAuthorized(method, bucket, objectMD, requestTypes, canonicalID, authInfo, log, request, + actionImplicitDenies, withIam) { + if (!withIam) { // eslint-disable-next-line no-param-reassign - requestTypes = [requestTypes]; - } - if (!actionImplicitDenies) { - // eslint-disable-next-line no-param-reassign - actionImplicitDenies = {}; + withIam = false; } - // By default, all missing actions are defined as allowed from IAM, to be - // backward compatible - requestTypes.forEach(requestType => { - if (actionImplicitDenies[requestType] === undefined) { - // eslint-disable-next-line no-param-reassign - actionImplicitDenies[requestType] = false; - } - }); - const mainApiCall = requestTypes[0]; - const results = {}; - requestTypes.forEach(_requestType => { - // Check to see if user is authorized to perform a - // particular action on bucket based on ACLs. - // TODO: Add IAM checks - let requesterIsNotUser = true; - let arn = null; - if (authInfo) { - requesterIsNotUser = !authInfo.isRequesterAnIAMUser(); - arn = authInfo.getArn(); - } - // if the bucket owner is an account, users should not have default access - if ((bucket.getOwner() === canonicalID) && requesterIsNotUser) { - results[_requestType] = actionImplicitDenies[_requestType] === false; - return; - } - const aclPermission = checkBucketAcls(bucket, _requestType, canonicalID, mainApiCall); - const bucketPolicy = bucket.getBucketPolicy(); - if (!bucketPolicy) { - results[_requestType] = actionImplicitDenies[_requestType] === false && aclPermission; - return; - } - const bucketPolicyPermission = checkBucketPolicy(bucketPolicy, _requestType, - canonicalID, arn, bucket.getOwner(), log, request); - if (bucketPolicyPermission === 'explicitDeny') { - results[_requestType] = false; - return; - } - // If the bucket policy returns an allow, we accept the request, as the - // IAM response here is either Allow or implicit deny. - if (bucketPolicyPermission === 'allow') { - results[_requestType] = true; - return; - } - results[_requestType] = actionImplicitDenies[_requestType] === false && aclPermission; - }); - - // final result is true if all the results are true - return Object.keys(results).every(key => results[key] === true); -} -function isObjAuthorized(bucket, objectMD, requestTypes, canonicalID, authInfo, log, request, actionImplicitDenies) { if (!Array.isArray(requestTypes)) { // eslint-disable-next-line no-param-reassign requestTypes = [requestTypes]; @@ -362,55 +309,68 @@ function isObjAuthorized(bucket, objectMD, requestTypes, canonicalID, authInfo, actionImplicitDenies[requestType] = false; } }); + const results = {}; const mainApiCall = requestTypes[0]; requestTypes.forEach(_requestType => { - const parsedMethodName = _requestType.endsWith('Version') ? - _requestType.slice(0, -7) : _requestType; + const parsedMethodName = _requestType.endsWith('Version') ? _requestType.slice(0, -7) : _requestType; const bucketOwner = bucket.getOwner(); - 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; - } - // check bucket has read access - // 'bucketGet' covers listObjects and listMultipartUploads, bucket read actions - results[_requestType] = isBucketAuthorized(bucket, 'bucketGet', canonicalID, authInfo, log, request, false); - return; - } - let requesterIsNotUser = true; - let arn = null; + let requesterIsNotUser = false; + let arn; let isUserUnauthenticated = false; if (authInfo) { requesterIsNotUser = !authInfo.isRequesterAnIAMUser(); arn = authInfo.getArn(); isUserUnauthenticated = arn === undefined; } - if (objectMD['owner-id'] === canonicalID && requesterIsNotUser) { - results[_requestType] = actionImplicitDenies[_requestType] === false; - return; - } - // account is authorized if: - // - requesttype is included in bucketOwnerActions and - // - account is the bucket owner - // - requester is account, not user - if (bucketOwnerActions.includes(parsedMethodName) - && (bucketOwner === canonicalID) - && requesterIsNotUser) { - results[_requestType] = actionImplicitDenies[_requestType] === false; - return; + if (method === 'isBucketAuthorized') { + const isBucketOwner = bucketOwner === canonicalID; + if (isBucketOwner && requesterIsNotUser && !withIam) { + results[_requestType] = actionImplicitDenies[_requestType] === false; + return; + } + } else { + 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; + } + // check bucket has read access + // 'bucketGet' covers listObjects and listMultipartUploads, bucket read actions + results[_requestType] = isAuthorized('isBucketAuthorized', bucket, null, 'bucketGet', canonicalID, + authInfo, log, request, false); + return; + } + if (objectMD['owner-id'] === canonicalID && requesterIsNotUser) { + results[_requestType] = actionImplicitDenies[_requestType] === false; + return; + } + // account is authorized if: + // - requesttype is included in bucketOwnerActions and + // - account is the bucket owner + // - requester is account, not user + if (bucketOwnerActions.includes(parsedMethodName) + && (bucketOwner === canonicalID) + && requesterIsNotUser) { + results[_requestType] = actionImplicitDenies[_requestType] === false; + return; + } } - const aclPermission = checkObjectAcls(bucket, objectMD, parsedMethodName, - canonicalID, requesterIsNotUser, isUserUnauthenticated, mainApiCall); + const aclPermission = method === 'isBucketAuthorized' ? + checkBucketAcls(bucket, _requestType, canonicalID, mainApiCall) : + checkObjectAcls(bucket, objectMD, parsedMethodName, canonicalID, + requesterIsNotUser, isUserUnauthenticated, mainApiCall); const bucketPolicy = bucket.getBucketPolicy(); if (!bucketPolicy) { - results[_requestType] = actionImplicitDenies[_requestType] === false && aclPermission; + results[_requestType] = withIam === true ? actionImplicitDenies[_requestType] === false : + actionImplicitDenies[_requestType] === false && aclPermission; return; } - const bucketPolicyPermission = checkBucketPolicy(bucketPolicy, _requestType, - canonicalID, arn, bucket.getOwner(), log, request); + const bucketPolicyPermission = checkBucketPolicy(bucketPolicy, _requestType, canonicalID, + arn, bucketOwner, log, request); + if (bucketPolicyPermission === 'explicitDeny') { results[_requestType] = false; return; @@ -421,59 +381,26 @@ function isObjAuthorized(bucket, objectMD, requestTypes, canonicalID, authInfo, results[_requestType] = true; return; } - results[_requestType] = actionImplicitDenies[_requestType] === false && aclPermission; + // eslint-disable-next-line max-len + results[_requestType] = withIam === true ? actionImplicitDenies[_requestType] === false : + actionImplicitDenies[_requestType] === false && aclPermission; }); - - // final result is true if all the results are true - return Object.keys(results).every(key => results[key] === true); + return Object.values(results).every(result => result === true); } -function evaluateBucketPolicyWithIAM(bucket, requestTypes, canonicalID, authInfo, actionImplicitDenies, log, request) { - if (!Array.isArray(requestTypes)) { - // eslint-disable-next-line no-param-reassign - requestTypes = [requestTypes]; - } - if (actionImplicitDenies === false) { - // eslint-disable-next-line no-param-reassign - actionImplicitDenies = {}; - } - // By default, all missing actions are defined as allowed from IAM, to be - // backward compatible - requestTypes.forEach(requestType => { - if (actionImplicitDenies[requestType] === undefined) { - // eslint-disable-next-line no-param-reassign - actionImplicitDenies[requestType] = false; - } - }); +function isBucketAuthorized(bucket, requestTypes, canonicalID, authInfo, log, request, actionImplicitDenies) { + return isAuthorized('isBucketAuthorized', bucket, null, requestTypes, canonicalID, authInfo, log, + request, actionImplicitDenies); +} - const results = {}; - requestTypes.forEach(_requestType => { - let arn = null; - if (authInfo) { - arn = authInfo.getArn(); - } - const bucketPolicy = bucket.getBucketPolicy(); - if (!bucketPolicy) { - results[_requestType] = actionImplicitDenies[_requestType] === false; - return; - } - const bucketPolicyPermission = checkBucketPolicy(bucketPolicy, _requestType, - canonicalID, arn, bucket.getOwner(), log, request); - if (bucketPolicyPermission === 'explicitDeny') { - results[_requestType] = false; - return; - } - // If the bucket policy returns an allow, we accept the request, as the - // IAM response here is either Allow or implicit deny. - if (bucketPolicyPermission === 'allow') { - results[_requestType] = true; - return; - } - results[_requestType] = actionImplicitDenies[_requestType] === false; - }); +function evaluateBucketPolicyWithIAM(bucket, requestTypes, canonicalID, authInfo, log, request, actionImplicitDenies) { + return isAuthorized('isBucketAuthorized', bucket, null, requestTypes, canonicalID, authInfo, log, + request, actionImplicitDenies, true); +} - // final result is true if all the results are true - return Object.keys(results).every(key => results[key] === true); +function isObjAuthorized(bucket, objectMD, requestTypes, canonicalID, authInfo, log, request, actionImplicitDenies) { + return isAuthorized('isObjAuthorized', bucket, objectMD, requestTypes, canonicalID, authInfo, log, + request, actionImplicitDenies); } function _checkResource(resource, bucketArn) { From 33d7c99e0c4df43b97e5416cfc5dcefd4b00629e Mon Sep 17 00:00:00 2001 From: Maha Benzekri Date: Fri, 10 Nov 2023 11:17:35 +0100 Subject: [PATCH 9/9] CLDSRV-427: Improving functions using helper function - In this commit , I added a helper (processBucketPolicy) function for the bycket policies checks that are shared between the isbucketAuthorized, isObjAuthorized and evaluateBucketPolicyWithIAM for a better code readability and to avoid long functions. --- .../authorization/permissionChecks.js | 244 ++++++++++-------- 1 file changed, 141 insertions(+), 103 deletions(-) diff --git a/lib/api/apiUtils/authorization/permissionChecks.js b/lib/api/apiUtils/authorization/permissionChecks.js index a52f98419b..cbb459a5c3 100644 --- a/lib/api/apiUtils/authorization/permissionChecks.js +++ b/lib/api/apiUtils/authorization/permissionChecks.js @@ -1,17 +1,19 @@ const { evaluators, actionMaps, RequestContext } = require('arsenal').policies; const constants = require('../../../../constants'); -const { allAuthedUsersId, bucketOwnerActions, logId, publicId, - assumedRoleArnResourceType, backbeatLifecycleSessionName, arrayOfAllowed } = constants; +const { + allAuthedUsersId, bucketOwnerActions, logId, publicId, + assumedRoleArnResourceType, backbeatLifecycleSessionName, arrayOfAllowed, +} = constants; // whitelist buckets to allow public read on objects -const publicReadBuckets = process.env.ALLOW_PUBLIC_READ_BUCKETS ? - process.env.ALLOW_PUBLIC_READ_BUCKETS.split(',') : []; +const publicReadBuckets = process.env.ALLOW_PUBLIC_READ_BUCKETS + ? process.env.ALLOW_PUBLIC_READ_BUCKETS.split(',') : []; function checkBucketAcls(bucket, requestType, canonicalID, mainApiCall) { // Same logic applies on the Versioned APIs, so let's simplify it. - const requestTypeParsed = requestType.endsWith('Version') ? - requestType.slice(0, 'Version'.length * -1) : requestType; + const requestTypeParsed = requestType.endsWith('Version') + ? requestType.slice(0, -7) : requestType; if (bucket.getOwner() === canonicalID) { return true; } @@ -95,8 +97,8 @@ function checkBucketAcls(bucket, requestType, canonicalID, mainApiCall) { // objectPutACL, objectGetACL, objectHead or objectGet, the bucket // authorization check should just return true so can move on to check // rights at the object level. - return (requestTypeParsed === 'objectPutACL' || requestTypeParsed === 'objectGetACL' || - requestTypeParsed === 'objectGet' || requestTypeParsed === 'objectHead'); + return (requestTypeParsed === 'objectPutACL' || requestTypeParsed === 'objectGetACL' + || requestTypeParsed === 'objectGet' || requestTypeParsed === 'objectHead'); } function checkObjectAcls(bucket, objectMD, requestType, canonicalID, requesterIsNotUser, @@ -188,9 +190,9 @@ function checkObjectAcls(bucket, objectMD, requestType, canonicalID, requesterIs // allow public reads on buckets that are whitelisted for anonymous reads // TODO: remove this after bucket policies are implemented const bucketAcl = bucket.getAcl(); - const allowPublicReads = publicReadBuckets.includes(bucket.getName()) && - bucketAcl.Canned === 'public-read' && - (requestType === 'objectGet' || requestType === 'objectHead'); + const allowPublicReads = publicReadBuckets.includes(bucket.getName()) + && bucketAcl.Canned === 'public-read' + && (requestType === 'objectGet' || requestType === 'objectHead'); if (allowPublicReads) { return true; } @@ -287,120 +289,157 @@ function checkBucketPolicy(policy, requestType, canonicalID, arn, bucketOwner, l return permission; } -function isAuthorized(method, bucket, objectMD, requestTypes, canonicalID, authInfo, log, request, - actionImplicitDenies, withIam) { - if (!withIam) { +function processBucketPolicy(requestType, bucket, canonicalID, arn, bucketOwner, log, + request, aclPermission, results, actionImplicitDenies) { + const bucketPolicy = bucket.getBucketPolicy(); + + if (!bucketPolicy) { // eslint-disable-next-line no-param-reassign - withIam = false; + results[requestType] = actionImplicitDenies[requestType] === false && aclPermission; + } else { + const bucketPolicyPermission = checkBucketPolicy(bucketPolicy, requestType, canonicalID, arn, + bucketOwner, log, request); + + if (bucketPolicyPermission === 'explicitDeny') { + // eslint-disable-next-line no-param-reassign + results[requestType] = false; + } else if (bucketPolicyPermission === 'allow') { + // eslint-disable-next-line no-param-reassign + results[requestType] = true; + } else { + // eslint-disable-next-line no-param-reassign + results[requestType] = actionImplicitDenies[requestType] === false && aclPermission; + } } + return results[requestType]; +} + +function isBucketAuthorized(bucket, requestTypes, canonicalID, authInfo, log, request, actionImplicitDenies = {}) { if (!Array.isArray(requestTypes)) { // eslint-disable-next-line no-param-reassign requestTypes = [requestTypes]; } - // By default, all missing actions are defined as allowed from IAM, to be - // backward compatible if (!actionImplicitDenies) { // eslint-disable-next-line no-param-reassign actionImplicitDenies = {}; } - requestTypes.forEach(requestType => { - if (actionImplicitDenies[requestType] === undefined) { + const mainApiCall = requestTypes[0]; + const results = {}; + return requestTypes.every(_requestType => { + // By default, all missing actions are defined as allowed from IAM, to be + // backward compatible + if (actionImplicitDenies[_requestType] === undefined) { // eslint-disable-next-line no-param-reassign - actionImplicitDenies[requestType] = false; + actionImplicitDenies[_requestType] = false; + } + // Check to see if user is authorized to perform a + // particular action on bucket based on ACLs. + // TODO: Add IAM checks + let requesterIsNotUser = true; + let arn = null; + if (authInfo) { + requesterIsNotUser = !authInfo.isRequesterAnIAMUser(); + arn = authInfo.getArn(); + } + // if the bucket owner is an account, users should not have default access + if ((bucket.getOwner() === canonicalID) && requesterIsNotUser) { + results[_requestType] = actionImplicitDenies[_requestType] === false; + return results[_requestType]; } + const aclPermission = checkBucketAcls(bucket, _requestType, canonicalID, mainApiCall); + return processBucketPolicy(_requestType, bucket, canonicalID, arn, bucket.getOwner(), log, + request, aclPermission, results, actionImplicitDenies); }); +} +function evaluateBucketPolicyWithIAM(bucket, requestTypes, canonicalID, authInfo, actionImplicitDenies = {}, + log, request) { + if (!Array.isArray(requestTypes)) { + // eslint-disable-next-line no-param-reassign + requestTypes = [requestTypes]; + } + if (!actionImplicitDenies) { + // eslint-disable-next-line no-param-reassign + actionImplicitDenies = {}; + } + const results = {}; + return requestTypes.every(_requestType => { + // By default, all missing actions are defined as allowed from IAM, to be + // backward compatible + if (actionImplicitDenies[_requestType] === undefined) { + // eslint-disable-next-line no-param-reassign + actionImplicitDenies[_requestType] = false; + } + let arn = null; + if (authInfo) { + arn = authInfo.getArn(); + } + return processBucketPolicy(_requestType, bucket, canonicalID, arn, bucket.getOwner(), log, + request, true, results, actionImplicitDenies); + }); +} + +function isObjAuthorized(bucket, objectMD, requestTypes, canonicalID, authInfo, log, request, + actionImplicitDenies = {}) { + if (!Array.isArray(requestTypes)) { + // eslint-disable-next-line no-param-reassign + requestTypes = [requestTypes]; + } + if (!actionImplicitDenies) { + // eslint-disable-next-line no-param-reassign + actionImplicitDenies = {}; + } const results = {}; const mainApiCall = requestTypes[0]; - requestTypes.forEach(_requestType => { - const parsedMethodName = _requestType.endsWith('Version') ? _requestType.slice(0, -7) : _requestType; + return requestTypes.every(_requestType => { + // By default, all missing actions are defined as allowed from IAM, to be + // backward compatible + if (actionImplicitDenies[_requestType] === undefined) { + // eslint-disable-next-line no-param-reassign + actionImplicitDenies[_requestType] = false; + } + const parsedMethodName = _requestType.endsWith('Version') + ? _requestType.slice(0, -7) : _requestType; const bucketOwner = bucket.getOwner(); - let requesterIsNotUser = false; - let arn; + 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 results[_requestType]; + } + // check bucket has read access + // 'bucketGet' covers listObjects and listMultipartUploads, bucket read actions + results[_requestType] = isBucketAuthorized(bucket, 'bucketGet', canonicalID, authInfo, log, request); + return results[_requestType]; + } + let requesterIsNotUser = true; + let arn = null; let isUserUnauthenticated = false; if (authInfo) { requesterIsNotUser = !authInfo.isRequesterAnIAMUser(); arn = authInfo.getArn(); isUserUnauthenticated = arn === undefined; } - if (method === 'isBucketAuthorized') { - const isBucketOwner = bucketOwner === canonicalID; - if (isBucketOwner && requesterIsNotUser && !withIam) { - results[_requestType] = actionImplicitDenies[_requestType] === false; - return; - } - } else { - 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; - } - // check bucket has read access - // 'bucketGet' covers listObjects and listMultipartUploads, bucket read actions - results[_requestType] = isAuthorized('isBucketAuthorized', bucket, null, 'bucketGet', canonicalID, - authInfo, log, request, false); - return; - } - if (objectMD['owner-id'] === canonicalID && requesterIsNotUser) { - results[_requestType] = actionImplicitDenies[_requestType] === false; - return; - } - // account is authorized if: - // - requesttype is included in bucketOwnerActions and - // - account is the bucket owner - // - requester is account, not user - if (bucketOwnerActions.includes(parsedMethodName) - && (bucketOwner === canonicalID) - && requesterIsNotUser) { - results[_requestType] = actionImplicitDenies[_requestType] === false; - return; - } - } - const aclPermission = method === 'isBucketAuthorized' ? - checkBucketAcls(bucket, _requestType, canonicalID, mainApiCall) : - checkObjectAcls(bucket, objectMD, parsedMethodName, canonicalID, - requesterIsNotUser, isUserUnauthenticated, mainApiCall); - const bucketPolicy = bucket.getBucketPolicy(); - if (!bucketPolicy) { - results[_requestType] = withIam === true ? actionImplicitDenies[_requestType] === false : - actionImplicitDenies[_requestType] === false && aclPermission; - return; - } - const bucketPolicyPermission = checkBucketPolicy(bucketPolicy, _requestType, canonicalID, - arn, bucketOwner, log, request); - - if (bucketPolicyPermission === 'explicitDeny') { - results[_requestType] = false; - return; + if (objectMD['owner-id'] === canonicalID && requesterIsNotUser) { + results[_requestType] = actionImplicitDenies[_requestType] === false; + return results[_requestType]; } - // If the bucket policy returns an allow, we accept the request, as the - // IAM response here is either Allow or implicit deny. - if (bucketPolicyPermission === 'allow') { - results[_requestType] = true; - return; + // account is authorized if: + // - requesttype is included in bucketOwnerActions and + // - account is the bucket owner + // - requester is account, not user + if (bucketOwnerActions.includes(parsedMethodName) + && (bucketOwner === canonicalID) + && requesterIsNotUser) { + results[_requestType] = actionImplicitDenies[_requestType] === false; + return results[_requestType]; } - // eslint-disable-next-line max-len - results[_requestType] = withIam === true ? actionImplicitDenies[_requestType] === false : - actionImplicitDenies[_requestType] === false && aclPermission; + const aclPermission = checkObjectAcls(bucket, objectMD, parsedMethodName, + canonicalID, requesterIsNotUser, isUserUnauthenticated, mainApiCall); + return processBucketPolicy(_requestType, bucket, canonicalID, arn, bucketOwner, + log, request, aclPermission, results, actionImplicitDenies); }); - return Object.values(results).every(result => result === true); -} - -function isBucketAuthorized(bucket, requestTypes, canonicalID, authInfo, log, request, actionImplicitDenies) { - return isAuthorized('isBucketAuthorized', bucket, null, requestTypes, canonicalID, authInfo, log, - request, actionImplicitDenies); -} - -function evaluateBucketPolicyWithIAM(bucket, requestTypes, canonicalID, authInfo, log, request, actionImplicitDenies) { - return isAuthorized('isBucketAuthorized', bucket, null, requestTypes, canonicalID, authInfo, log, - request, actionImplicitDenies, true); -} - -function isObjAuthorized(bucket, objectMD, requestTypes, canonicalID, authInfo, log, request, actionImplicitDenies) { - return isAuthorized('isObjAuthorized', bucket, objectMD, requestTypes, canonicalID, authInfo, log, - request, actionImplicitDenies); } function _checkResource(resource, bucketArn) { @@ -447,9 +486,9 @@ function isLifecycleSession(arn) { const resourceType = resourceNames[0]; const sessionName = resourceNames[resourceNames.length - 1]; - return (service === 'sts' && - resourceType === assumedRoleArnResourceType && - sessionName === backbeatLifecycleSessionName); + return (service === 'sts' + && resourceType === assumedRoleArnResourceType + && sessionName === backbeatLifecycleSessionName); } module.exports = { @@ -461,4 +500,3 @@ module.exports = { isLifecycleSession, evaluateBucketPolicyWithIAM, }; -