From ca00292dd9f96120f8fd33ce1a25998da6968ca8 Mon Sep 17 00:00:00 2001 From: Will Toozs Date: Fri, 25 Aug 2023 10:29:54 +0200 Subject: [PATCH 1/2] 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..c18ebdcef 100644 --- a/lib/policyEvaluator/evaluator.ts +++ b/lib/policyEvaluator/evaluator.ts @@ -310,6 +310,7 @@ export function evaluatePolicy( } /** + * @deprecated Upgrade to evaluateAllPoliciesV2 * 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 evaluateAllPoliciesV2(requestContext, allPolicies, log).verdict; +} +export function evaluateAllPoliciesV2( + 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 227b869869cc3aab497ed6c869ef727aee0a532f Mon Sep 17 00:00:00 2001 From: Will Toozs Date: Fri, 25 Aug 2023 10:30:04 +0200 Subject: [PATCH 2/2] 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..8470864dc 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 evaluateAllPoliciesV2 = evaluator.evaluateAllPoliciesV2; 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 = evaluateAllPoliciesV2(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 = evaluateAllPoliciesV2(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 = evaluateAllPoliciesV2(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 = evaluateAllPoliciesV2( + requestContext, + testCase.policiesToEvaluate.map(policyName => TestMatrixPolicies[policyName]), + log); + assert.deepStrictEqual(result, testCase.expectedPolicyEvaluation); + }); + }); + const TestMatrix = [ { policiesToEvaluate: [],