From 79c82a4c3dae84c821e2f5576785c27f2101ab68 Mon Sep 17 00:00:00 2001 From: Will Toozs Date: Mon, 11 Sep 2023 20:55:51 +0200 Subject: [PATCH 1/8] ARSN-362: add implicit deny logic to policy evaluation --- lib/policyEvaluator/evaluator.ts | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/lib/policyEvaluator/evaluator.ts b/lib/policyEvaluator/evaluator.ts index 01626d7e6..0d753fd6b 100644 --- a/lib/policyEvaluator/evaluator.ts +++ b/lib/policyEvaluator/evaluator.ts @@ -310,6 +310,7 @@ export function evaluatePolicy( } /** + * @deprecated Upgrade to evaluateAllPoliciesNew * Evaluate whether a request is permitted under a policy. * @param requestContext - Info necessary to * evaluate permission @@ -325,6 +326,16 @@ export function evaluateAllPolicies( allPolicies: any[], log: Logger, ): string { + return evaluateAllPoliciesNew(requestContext, allPolicies, log).verdict; +} +export function evaluateAllPoliciesNew( + requestContext: RequestContext, + allPolicies: any[], + log: Logger, +): { + verdict: string; + isImplicit: boolean; +} { log.trace('evaluating all policies'); let allow = false; let allowWithTagCondition = false; @@ -333,7 +344,10 @@ export function evaluateAllPolicies( const singlePolicyVerdict = evaluatePolicy(requestContext, allPolicies[i], log); // If there is any Deny, just return Deny if (singlePolicyVerdict === 'Deny') { - return 'Deny'; + return { + verdict: 'Deny', + isImplicit: false, + }; } if (singlePolicyVerdict === 'Allow') { allow = true; @@ -344,6 +358,7 @@ export function evaluateAllPolicies( } // else 'Neutral' } let verdict; + let isImplicit = false; if (allow) { if (denyWithTagCondition) { verdict = 'NeedTagConditionEval'; @@ -355,8 +370,9 @@ export function evaluateAllPolicies( verdict = 'NeedTagConditionEval'; } else { verdict = 'Deny'; + isImplicit = true; } } - log.trace('result of evaluating all policies', { verdict }); - return verdict; + log.trace('result of evaluating all policies', { verdict, isImplicit }); + return { verdict, isImplicit }; } From 39988e52e200efb36cd9857ad38d018f67bb2fc4 Mon Sep 17 00:00:00 2001 From: Will Toozs Date: Mon, 11 Sep 2023 20:56:06 +0200 Subject: [PATCH 2/8] ARSN-362: add implicit deny logic to policy eval tests --- tests/unit/policyEvaluator.spec.js | 174 +++++++++++++++++++++++++++++ 1 file changed, 174 insertions(+) diff --git a/tests/unit/policyEvaluator.spec.js b/tests/unit/policyEvaluator.spec.js index e534ebdcf..ef0f76d1e 100644 --- a/tests/unit/policyEvaluator.spec.js +++ b/tests/unit/policyEvaluator.spec.js @@ -6,6 +6,7 @@ const fakeTimers = require('@sinonjs/fake-timers'); const evaluator = require('../../lib/policyEvaluator/evaluator'); const evaluatePolicy = evaluator.evaluatePolicy; const evaluateAllPolicies = evaluator.evaluateAllPolicies; +const evaluateAllPoliciesNew = evaluator.evaluateAllPoliciesNew; const handleWildcards = require('../../lib/policyEvaluator/utils/wildcards').handleWildcards; const substituteVariables = @@ -1451,6 +1452,49 @@ describe('policyEvaluator', () => { assert.strictEqual(result, 'Deny'); }); + it('should deny access if any policy results in a Deny', () => { + requestContext = new RequestContext({}, {}, + 'my_favorite_bucket', undefined, + undefined, undefined, 'bucketDelete', 's3'); + requestContext.setRequesterInfo({}); + const result = evaluateAllPoliciesNew(requestContext, + [samples['arn:aws:iam::aws:policy/AmazonS3FullAccess'], + samples['Deny Bucket Policy']], log); + assert.deepStrictEqual(result, { + verdict: 'Deny', + isImplicit: false, + }); + }); + + it('should deny access if request action is not in any policy', () => { + requestContext = new RequestContext({}, {}, + 'notVeryPrivate', undefined, + undefined, undefined, 'bucketDelete', 's3'); + requestContext.setRequesterInfo({}); + const result = evaluateAllPoliciesNew(requestContext, + [samples['Multi-Statement Policy'], + samples['Variable Bucket Policy']], log); + assert.deepStrictEqual(result, { + verdict: 'Deny', + isImplicit: true, + }); + }); + + it('should deny access if request resource is not in any policy', () => { + requestContext = new RequestContext({}, {}, + 'notbucket', undefined, + undefined, undefined, 'objectGet', 's3'); + requestContext.setRequesterInfo({}); + const result = evaluateAllPoliciesNew(requestContext, [ + samples['Multi-Statement Policy'], + samples['Variable Bucket Policy'], + ], log); + assert.deepStrictEqual(result, { + verdict: 'Deny', + isImplicit: true, + }); + }); + const TestMatrixPolicies = { Allow: { Version: '2012-10-17', @@ -1504,6 +1548,136 @@ describe('policyEvaluator', () => { }, }; + const TestMatrixV2 = [ + { + policiesToEvaluate: [], + expectedPolicyEvaluation: { + verdict: 'Deny', + isImplicit: true, + }, + }, + { + policiesToEvaluate: ['Allow'], + expectedPolicyEvaluation: { + verdict: 'Allow', + isImplicit: false, + }, + }, + { + policiesToEvaluate: ['Neutral'], + expectedPolicyEvaluation: { + verdict: 'Deny', + isImplicit: true, + }, + }, + { + policiesToEvaluate: ['Deny'], + expectedPolicyEvaluation: { + verdict: 'Deny', + isImplicit: false, + }, + }, + { + policiesToEvaluate: ['Allow', 'Allow'], + expectedPolicyEvaluation: { + verdict: 'Allow', + isImplicit: false, + }, + }, + { + policiesToEvaluate: ['Allow', 'Neutral'], + expectedPolicyEvaluation: { + verdict: 'Allow', + isImplicit: false, + }, + }, + { + policiesToEvaluate: ['Neutral', 'Allow'], + expectedPolicyEvaluation: { + verdict: 'Allow', + isImplicit: false, + }, + }, + { + policiesToEvaluate: ['Neutral', 'Neutral'], + expectedPolicyEvaluation: { + verdict: 'Deny', + isImplicit: true, + }, + }, + { + policiesToEvaluate: ['Allow', 'Deny'], + expectedPolicyEvaluation: { + verdict: 'Deny', + isImplicit: false, + }, + }, + { + policiesToEvaluate: ['AllowWithTagCondition'], + expectedPolicyEvaluation: { + verdict: 'NeedTagConditionEval', + isImplicit: false, + }, + }, + { + policiesToEvaluate: ['Allow', 'AllowWithTagCondition'], + expectedPolicyEvaluation: { + verdict: 'Allow', + isImplicit: false, + }, + }, + { + policiesToEvaluate: ['DenyWithTagCondition'], + expectedPolicyEvaluation: { + verdict: 'Deny', + isImplicit: true, + }, + }, + { + policiesToEvaluate: ['Allow', 'DenyWithTagCondition'], + expectedPolicyEvaluation: { + verdict: 'NeedTagConditionEval', + isImplicit: false, + }, + }, + { + policiesToEvaluate: ['AllowWithTagCondition', 'DenyWithTagCondition'], + expectedPolicyEvaluation: { + verdict: 'NeedTagConditionEval', + isImplicit: false, + }, + }, + { + policiesToEvaluate: ['AllowWithTagCondition', 'DenyWithTagCondition', 'Deny'], + expectedPolicyEvaluation: { + verdict: 'Deny', + isImplicit: false, + }, + }, + { + policiesToEvaluate: ['DenyWithTagCondition', 'AllowWithTagCondition', 'Allow'], + expectedPolicyEvaluation: { + verdict: 'NeedTagConditionEval', + isImplicit: false, + }, + }, + ]; + + TestMatrixV2.forEach(testCase => { + it(`policies evaluating individually to [${testCase.policiesToEvaluate.join(', ')}] ` + + `should return ${testCase.expectedPolicyEvaluation}`, () => { + requestContext = new RequestContext({}, {}, + 'my_favorite_bucket', undefined, + undefined, undefined, 'objectGet', 's3'); + requestContext.setRequesterInfo({}); + const result = evaluateAllPoliciesNew( + requestContext, + testCase.policiesToEvaluate.map(policyName => TestMatrixPolicies[policyName]), + log); + assert.deepStrictEqual(result, testCase.expectedPolicyEvaluation); + }); + }); + const TestMatrix = [ { policiesToEvaluate: [], From 777783171acfb77311bbc5836ee0d78d071dfd43 Mon Sep 17 00:00:00 2001 From: Maha Benzekri Date: Mon, 30 Oct 2023 09:36:56 +0100 Subject: [PATCH 3/8] ARSN-362: change new function name for clarity --- lib/policyEvaluator/evaluator.ts | 6 +++--- tests/unit/policyEvaluator.spec.js | 10 +++++----- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/policyEvaluator/evaluator.ts b/lib/policyEvaluator/evaluator.ts index 0d753fd6b..200816fa3 100644 --- a/lib/policyEvaluator/evaluator.ts +++ b/lib/policyEvaluator/evaluator.ts @@ -310,7 +310,7 @@ export function evaluatePolicy( } /** - * @deprecated Upgrade to evaluateAllPoliciesNew + * @deprecated Upgrade to standardEvaluateAllPolicies * Evaluate whether a request is permitted under a policy. * @param requestContext - Info necessary to * evaluate permission @@ -326,9 +326,9 @@ export function evaluateAllPolicies( allPolicies: any[], log: Logger, ): string { - return evaluateAllPoliciesNew(requestContext, allPolicies, log).verdict; + return standardEvaluateAllPolicies(requestContext, allPolicies, log).verdict; } -export function evaluateAllPoliciesNew( +export function standardEvaluateAllPolicies( requestContext: RequestContext, allPolicies: any[], log: Logger, diff --git a/tests/unit/policyEvaluator.spec.js b/tests/unit/policyEvaluator.spec.js index ef0f76d1e..33477c886 100644 --- a/tests/unit/policyEvaluator.spec.js +++ b/tests/unit/policyEvaluator.spec.js @@ -6,7 +6,7 @@ const fakeTimers = require('@sinonjs/fake-timers'); const evaluator = require('../../lib/policyEvaluator/evaluator'); const evaluatePolicy = evaluator.evaluatePolicy; const evaluateAllPolicies = evaluator.evaluateAllPolicies; -const evaluateAllPoliciesNew = evaluator.evaluateAllPoliciesNew; +const standardEvaluateAllPolicies = evaluator.standardEvaluateAllPolicies; const handleWildcards = require('../../lib/policyEvaluator/utils/wildcards').handleWildcards; const substituteVariables = @@ -1457,7 +1457,7 @@ describe('policyEvaluator', () => { 'my_favorite_bucket', undefined, undefined, undefined, 'bucketDelete', 's3'); requestContext.setRequesterInfo({}); - const result = evaluateAllPoliciesNew(requestContext, + const result = standardEvaluateAllPolicies(requestContext, [samples['arn:aws:iam::aws:policy/AmazonS3FullAccess'], samples['Deny Bucket Policy']], log); assert.deepStrictEqual(result, { @@ -1471,7 +1471,7 @@ describe('policyEvaluator', () => { 'notVeryPrivate', undefined, undefined, undefined, 'bucketDelete', 's3'); requestContext.setRequesterInfo({}); - const result = evaluateAllPoliciesNew(requestContext, + const result = standardEvaluateAllPolicies(requestContext, [samples['Multi-Statement Policy'], samples['Variable Bucket Policy']], log); assert.deepStrictEqual(result, { @@ -1485,7 +1485,7 @@ describe('policyEvaluator', () => { 'notbucket', undefined, undefined, undefined, 'objectGet', 's3'); requestContext.setRequesterInfo({}); - const result = evaluateAllPoliciesNew(requestContext, [ + const result = standardEvaluateAllPolicies(requestContext, [ samples['Multi-Statement Policy'], samples['Variable Bucket Policy'], ], log); @@ -1670,7 +1670,7 @@ describe('policyEvaluator', () => { 'my_favorite_bucket', undefined, undefined, undefined, 'objectGet', 's3'); requestContext.setRequesterInfo({}); - const result = evaluateAllPoliciesNew( + const result = standardEvaluateAllPolicies( requestContext, testCase.policiesToEvaluate.map(policyName => TestMatrixPolicies[policyName]), log); From df5ff0f4006ee0a21269e139567fd5c425a4225f Mon Sep 17 00:00:00 2001 From: Maha Benzekri Date: Mon, 30 Oct 2023 09:38:45 +0100 Subject: [PATCH 4/8] ARSN-362:fixups on impl deny policy tests As the evaluateAllPolicies function is using the result of the standardEvaluateAllPolicies , the redundant tests are removed. The test that was kept is only to show that we use the result.verdict in old flow evaluation. --- tests/unit/policyEvaluator.spec.js | 113 +++-------------------------- 1 file changed, 10 insertions(+), 103 deletions(-) diff --git a/tests/unit/policyEvaluator.spec.js b/tests/unit/policyEvaluator.spec.js index 33477c886..91fa9ab2d 100644 --- a/tests/unit/policyEvaluator.spec.js +++ b/tests/unit/policyEvaluator.spec.js @@ -1418,40 +1418,6 @@ describe('policyEvaluator', () => { }); describe('evaluate multiple policies', () => { - it('should deny access if any policy results in a Deny', () => { - requestContext = new RequestContext({}, {}, - 'my_favorite_bucket', undefined, - undefined, undefined, 'bucketDelete', 's3'); - requestContext.setRequesterInfo({}); - const result = evaluateAllPolicies(requestContext, - [samples['arn:aws:iam::aws:policy/AmazonS3FullAccess'], - samples['Deny Bucket Policy']], log); - assert.strictEqual(result, 'Deny'); - }); - - it('should deny access if request action is not in any policy', () => { - requestContext = new RequestContext({}, {}, - 'notVeryPrivate', undefined, - undefined, undefined, 'bucketDelete', 's3'); - requestContext.setRequesterInfo({}); - const result = evaluateAllPolicies(requestContext, - [samples['Multi-Statement Policy'], - samples['Variable Bucket Policy']], log); - assert.strictEqual(result, 'Deny'); - }); - - it('should deny access if request resource is not in any policy', () => { - requestContext = new RequestContext({}, {}, - 'notbucket', undefined, - undefined, undefined, 'objectGet', 's3'); - requestContext.setRequesterInfo({}); - const result = evaluateAllPolicies(requestContext, [ - samples['Multi-Statement Policy'], - samples['Variable Bucket Policy'], - ], log); - assert.strictEqual(result, 'Deny'); - }); - it('should deny access if any policy results in a Deny', () => { requestContext = new RequestContext({}, {}, 'my_favorite_bucket', undefined, @@ -1548,7 +1514,7 @@ describe('policyEvaluator', () => { }, }; - const TestMatrixV2 = [ + const TestMatrix = [ { policiesToEvaluate: [], expectedPolicyEvaluation: { @@ -1605,6 +1571,13 @@ describe('policyEvaluator', () => { isImplicit: true, }, }, + { + policiesToEvaluate: ['Neutral', 'Deny'], + expectedPolicyEvaluation: { + verdict: 'Deny', + isImplicit: false, + }, + }, { policiesToEvaluate: ['Allow', 'Deny'], expectedPolicyEvaluation: { @@ -1663,7 +1636,7 @@ describe('policyEvaluator', () => { }, ]; - TestMatrixV2.forEach(testCase => { + TestMatrix.forEach(testCase => { it(`policies evaluating individually to [${testCase.policiesToEvaluate.join(', ')}] ` + `should return ${testCase.expectedPolicyEvaluation}`, () => { requestContext = new RequestContext({}, {}, @@ -1678,72 +1651,6 @@ describe('policyEvaluator', () => { }); }); - const TestMatrix = [ - { - policiesToEvaluate: [], - expectedPolicyEvaluation: 'Deny', - }, - { - policiesToEvaluate: ['Allow'], - expectedPolicyEvaluation: 'Allow', - }, - { - policiesToEvaluate: ['Neutral'], - expectedPolicyEvaluation: 'Deny', - }, - { - policiesToEvaluate: ['Deny'], - expectedPolicyEvaluation: 'Deny', - }, - { - policiesToEvaluate: ['Allow', 'Allow'], - expectedPolicyEvaluation: 'Allow', - }, - { - policiesToEvaluate: ['Allow', 'Neutral'], - expectedPolicyEvaluation: 'Allow', - }, - { - policiesToEvaluate: ['Neutral', 'Allow'], - expectedPolicyEvaluation: 'Allow', - }, - { - policiesToEvaluate: ['Neutral', 'Neutral'], - expectedPolicyEvaluation: 'Deny', - }, - { - policiesToEvaluate: ['Allow', 'Deny'], - expectedPolicyEvaluation: 'Deny', - }, - { - policiesToEvaluate: ['AllowWithTagCondition'], - expectedPolicyEvaluation: 'NeedTagConditionEval', - }, - { - policiesToEvaluate: ['Allow', 'AllowWithTagCondition'], - expectedPolicyEvaluation: 'Allow', - }, - { - policiesToEvaluate: ['DenyWithTagCondition'], - expectedPolicyEvaluation: 'Deny', - }, - { - policiesToEvaluate: ['Allow', 'DenyWithTagCondition'], - expectedPolicyEvaluation: 'NeedTagConditionEval', - }, - { - policiesToEvaluate: ['AllowWithTagCondition', 'DenyWithTagCondition'], - expectedPolicyEvaluation: 'NeedTagConditionEval', - }, - { - policiesToEvaluate: ['AllowWithTagCondition', 'DenyWithTagCondition', 'Deny'], - expectedPolicyEvaluation: 'Deny', - }, - { - policiesToEvaluate: ['DenyWithTagCondition', 'AllowWithTagCondition', 'Allow'], - expectedPolicyEvaluation: 'NeedTagConditionEval', - }, - ]; TestMatrix.forEach(testCase => { it(`policies evaluating individually to [${testCase.policiesToEvaluate.join(', ')}] ` @@ -1756,7 +1663,7 @@ describe('policyEvaluator', () => { requestContext, testCase.policiesToEvaluate.map(policyName => TestMatrixPolicies[policyName]), log); - assert.strictEqual(result, testCase.expectedPolicyEvaluation); + assert.strictEqual(result, testCase.expectedPolicyEvaluation.verdict); }); }); }); From fbf5562a1180055249745881c1a324562d7cdc8a Mon Sep 17 00:00:00 2001 From: Maha Benzekri Date: Mon, 30 Oct 2023 16:08:14 +0100 Subject: [PATCH 5/8] bump arsenal version --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index d9cc0b92b..a8f3ae639 100644 --- a/package.json +++ b/package.json @@ -3,7 +3,7 @@ "engines": { "node": ">=16" }, - "version": "7.10.48", + "version": "7.10.49", "description": "Common utilities for the S3 project components", "main": "build/index.js", "repository": { From 13d349d2112bfd89a8dc257fa16482e1d17ae013 Mon Sep 17 00:00:00 2001 From: Maha Benzekri Date: Mon, 30 Oct 2023 16:40:00 +0100 Subject: [PATCH 6/8] fix --- package.json | 4 ---- 1 file changed, 4 deletions(-) diff --git a/package.json b/package.json index d2eed4199..fb3df1ad1 100644 --- a/package.json +++ b/package.json @@ -84,13 +84,9 @@ "test": "jest tests/unit/policyEvaluator.spec.js", "build": "tsc", "prepare": "yarn build", -<<<<<<< HEAD "ft_test": "jest tests/functional --testTimeout=120000 --forceExit", "coverage": "nyc --clean jest tests --coverage --testTimeout=120000 --forceExit", "build_doc": "cd documentation/listingAlgos/pics; dot -Tsvg delimiterVersionsStateChart.dot > delimiterVersionsStateChart.svg" -======= - "ft_test": "jest tests/functional --testTimeout=120000 --forceExit" ->>>>>>> origin/w/7.70/improvement/ARSN-362-implicitDeny }, "private": true, "jest": { From 1509f1bdfe3dca7437549243712fcc2c45f141ff Mon Sep 17 00:00:00 2001 From: Maha Benzekri Date: Mon, 30 Oct 2023 16:47:32 +0100 Subject: [PATCH 7/8] fix --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index fb3df1ad1..39c36c4e9 100644 --- a/package.json +++ b/package.json @@ -81,7 +81,7 @@ "lint": "eslint $(git ls-files '*.js')", "lint_md": "mdlint $(git ls-files '*.md')", "lint_yml": "yamllint $(git ls-files '*.yml')", - "test": "jest tests/unit/policyEvaluator.spec.js", + "test": "jest tests/unit", "build": "tsc", "prepare": "yarn build", "ft_test": "jest tests/functional --testTimeout=120000 --forceExit", From 29ef2ef265e8cccb02b282be1c1316ab67f65a97 Mon Sep 17 00:00:00 2001 From: Maha Benzekri Date: Mon, 30 Oct 2023 16:51:41 +0100 Subject: [PATCH 8/8] fixup --- package.json | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/package.json b/package.json index 4984a9550..735a9942b 100644 --- a/package.json +++ b/package.json @@ -79,10 +79,11 @@ "lint": "eslint $(git ls-files '*.js')", "lint_md": "mdlint $(git ls-files '*.md')", "lint_yml": "yamllint $(git ls-files '*.yml')", - "test": "jest tests/unit/policyEvaluator.spec.js", + "test": "jest tests/unit", "build": "tsc", "prepare": "yarn build", - "ft_test": "jest tests/functional --testTimeout=120000 --forceExit" + "ft_test": "jest tests/functional --testTimeout=120000 --forceExit", + "build_doc": "cd documentation/listingAlgos/pics; dot -Tsvg delimiterVersionsStateChart.dot > delimiterVersionsStateChart.svg" }, "private": true, "jest": {