diff --git a/lib/policyEvaluator/evaluator.ts b/lib/policyEvaluator/evaluator.ts index 428323349..01626d7e6 100644 --- a/lib/policyEvaluator/evaluator.ts +++ b/lib/policyEvaluator/evaluator.ts @@ -13,7 +13,11 @@ const operatorsWithVariables = ['StringEquals', 'StringNotEquals', const operatorsWithNegation = ['StringNotEquals', 'StringNotEqualsIgnoreCase', 'StringNotLike', 'ArnNotEquals', 'ArnNotLike', 'NumericNotEquals']; -const tagConditions = new Set(['s3:ExistingObjectTag', 's3:RequestObjectTagKey', 's3:RequestObjectTagKeys']); +const tagConditions = new Set([ + 's3:ExistingObjectTag', + 's3:RequestObjectTagKey', + 's3:RequestObjectTagKeys', +]); /** @@ -24,11 +28,11 @@ const tagConditions = new Set(['s3:ExistingObjectTag', 's3:RequestObjectTagKey', * @param log - logger * @return true if applicable, false if not */ -export const isResourceApplicable = ( +export function isResourceApplicable( requestContext: RequestContext, statementResource: string | string[], log: Logger, -): boolean => { +): boolean { const resource = requestContext.getResource(); if (!Array.isArray(statementResource)) { // eslint-disable-next-line no-param-reassign @@ -59,7 +63,7 @@ export const isResourceApplicable = ( { requestResource: resource }); // If no match found, no resource is applicable return false; -}; +} /** * Check whether action in policy statement applies to request @@ -69,11 +73,11 @@ export const isResourceApplicable = ( * @param log - logger * @return true if applicable, false if not */ -export const isActionApplicable = ( +export function isActionApplicable( requestAction: string, statementAction: string | string[], log: Logger, -): boolean => { +): boolean { if (!Array.isArray(statementAction)) { // eslint-disable-next-line no-param-reassign statementAction = [statementAction]; @@ -95,28 +99,29 @@ export const isActionApplicable = ( { requestAction }); // If no match found, return false return false; -}; +} /** * Check whether request meets policy conditions - * @param requestContext - info about request - * @param statementCondition - Condition statement from policy - * @param log - logger - * @return contains whether conditions are allowed and whether they - * contain any tag condition keys + * @param {RequestContext} requestContext - info about request + * @param {object} statementCondition - Condition statement from policy + * @param {Logger} log - logger + * @return {boolean|null} a condition evaluation result, one of: + * - true: condition is met + * - false: condition is not met + * - null: condition evaluation requires additional info to be + * provided (namely, for tag conditions, request tags and/or object + * tags have to be provided to evaluate the condition) */ -export const meetConditions = ( +export function meetConditions( requestContext: RequestContext, statementCondition: any, log: Logger, -) => { +): boolean | null { + let hasTagConditions = false; // The Condition portion of a policy is an object with different // operators as keys - const conditionEval = {}; - const operators = Object.keys(statementCondition); - const length = operators.length; - for (let i = 0; i < length; i++) { - const operator = operators[i]; + for (const operator of Object.keys(statementCondition)) { const hasPrefix = operator.includes(':'); const hasIfExistsCondition = operator.endsWith('IfExists'); // If has "IfExists" added to operator name, or operator has "ForAnyValue" or @@ -135,10 +140,6 @@ export const meetConditions = ( // Note: this should be the actual operator name, not the bareOperator const conditionsWithSameOperator = statementCondition[operator]; const conditionKeys = Object.keys(conditionsWithSameOperator); - if (conditionKeys.some(key => tagConditions.has(key)) && !requestContext.getNeedTagEval()) { - // @ts-expect-error - conditionEval.tagConditions = true; - } const conditionKeysLength = conditionKeys.length; for (let j = 0; j < conditionKeysLength; j++) { const key = conditionKeys[j]; @@ -155,6 +156,10 @@ export const meetConditions = ( // tag key is included in condition key and needs to be // moved to value for evaluation, otherwise key/value are unchanged const [transformedKey, transformedValue] = transformTagKeyValue(key, value); + if (tagConditions.has(transformedKey) && !requestContext.getNeedTagEval()) { + hasTagConditions = true; + continue; + } // Pull key using requestContext // TODO: If applicable to S3, handle policy set operations // where a keyBasedOnRequestContext returns multiple values and @@ -180,7 +185,7 @@ export const meetConditions = ( log.trace('condition not satisfied due to ' + 'missing info', { operator, conditionKey: transformedKey, policyValue: transformedValue }); - return { allow: false }; + return false; } // If condition operator prefix is included, the key should be an array if (prefix && !Array.isArray(keyBasedOnRequestContext)) { @@ -195,14 +200,16 @@ export const meetConditions = ( if (!operatorFunction(keyBasedOnRequestContext, transformedValue, prefix)) { log.trace('did not satisfy condition', { operator: bareOperator, keyBasedOnRequestContext, policyValue: transformedValue }); - return { allow: false }; + return false; } } } - // @ts-expect-error - conditionEval.allow = true; - return conditionEval; -}; + // one or more conditions required tag info to be evaluated + if (hasTagConditions) { + return null; + } + return true; +} /** * Evaluate whether a request is permitted under a policy. @@ -215,13 +222,15 @@ export const meetConditions = ( * @return Allow if permitted, Deny if not permitted or Neutral * if not applicable */ -export const evaluatePolicy = ( +export function evaluatePolicy( requestContext: RequestContext, policy: any, log: Logger, -): string => { +): string { // TODO: For bucket policies need to add Principal evaluation - let verdict = 'Neutral'; + let allow = false; + let allowWithTagCondition = false; + let denyWithTagCondition = false; if (!Array.isArray(policy.Statement)) { // eslint-disable-next-line no-param-reassign @@ -258,10 +267,18 @@ export const evaluatePolicy = ( } const conditionEval = currentStatement.Condition ? meetConditions(requestContext, currentStatement.Condition, log) : - null; + true; // If do not meet conditions move on to next statement - // @ts-expect-error - if (conditionEval && !conditionEval.allow) { + if (conditionEval === false) { + continue; + } + // If condition needs tag info to be evaluated, mark and move on to next statement + if (conditionEval === null) { + if (currentStatement.Effect === 'Deny') { + denyWithTagCondition = true; + } else { + allowWithTagCondition = true; + } continue; } if (currentStatement.Effect === 'Deny') { @@ -270,17 +287,27 @@ export const evaluatePolicy = ( return 'Deny'; } log.trace('Allow statement applies'); - // If statement is applicable, conditions are met and Effect is - // to Allow, set verdict to Allow + // statement is applicable, conditions are met and Effect is + // to Allow + allow = true; + } + let verdict; + if (denyWithTagCondition) { + // priority is on checking tags to potentially deny + verdict = 'DenyWithTagCondition'; + } else if (allow) { + // at least one statement is an allow verdict = 'Allow'; - // @ts-expect-error - if (conditionEval && conditionEval.tagConditions) { - verdict = 'NeedTagConditionEval'; - } + } else if (allowWithTagCondition) { + // all allow statements need tag checks + verdict = 'AllowWithTagCondition'; + } else { + // no statement matched to allow or deny + verdict = 'Neutral'; } log.trace('result of evaluating single policy', { verdict }); return verdict; -}; +} /** * Evaluate whether a request is permitted under a policy. @@ -293,24 +320,43 @@ export const evaluatePolicy = ( * @return Allow if permitted, Deny if not permitted. * Default is to Deny. Deny overrides an Allow */ -export const evaluateAllPolicies = ( +export function evaluateAllPolicies( requestContext: RequestContext, allPolicies: any[], log: Logger, -): string => { +): string { log.trace('evaluating all policies'); - let verdict = 'Deny'; + let allow = false; + let allowWithTagCondition = false; + let denyWithTagCondition = false; for (let i = 0; i < allPolicies.length; i++) { - const singlePolicyVerdict = - evaluatePolicy(requestContext, allPolicies[i], log); + const singlePolicyVerdict = evaluatePolicy(requestContext, allPolicies[i], log); // If there is any Deny, just return Deny if (singlePolicyVerdict === 'Deny') { return 'Deny'; } if (singlePolicyVerdict === 'Allow') { + allow = true; + } else if (singlePolicyVerdict === 'AllowWithTagCondition') { + allowWithTagCondition = true; + } else if (singlePolicyVerdict === 'DenyWithTagCondition') { + denyWithTagCondition = true; + } // else 'Neutral' + } + let verdict; + if (allow) { + if (denyWithTagCondition) { + verdict = 'NeedTagConditionEval'; + } else { verdict = 'Allow'; } + } else { + if (allowWithTagCondition) { + verdict = 'NeedTagConditionEval'; + } else { + verdict = 'Deny'; + } } - log.trace('result of evaluating all pollicies', { verdict }); + log.trace('result of evaluating all policies', { verdict }); return verdict; -}; +} diff --git a/lib/policyEvaluator/principal.ts b/lib/policyEvaluator/principal.ts index 264bf3037..daa21c4f7 100644 --- a/lib/policyEvaluator/principal.ts +++ b/lib/policyEvaluator/principal.ts @@ -23,15 +23,22 @@ export default class Principal { * @param statement - Statement policy field * @return True if meet conditions */ - static _evaluateCondition( + static _evaluateStatement( params: Params, statement: Statement, - // TODO Fix return type - ): any { + ): 'Neutral' | 'Allow' | 'Deny' { + const reverse = !!statement.NotPrincipal; + if (reverse) { + // In case of anonymous NotPrincipal, this will neutral everyone + return 'Neutral'; + } if (statement.Condition) { - return meetConditions(params.rc, statement.Condition, params.log); + const conditionEval = meetConditions(params.rc, statement.Condition, params.log); + if (conditionEval === false || conditionEval === null) { + return 'Neutral'; + } } - return true; + return statement.Effect; } /** @@ -48,19 +55,12 @@ export default class Principal { statement: Statement, valids: Valid, ): 'Neutral' | 'Allow' | 'Deny' { - const reverse = !!statement.NotPrincipal; const principal = (statement.Principal || statement.NotPrincipal)!; - if (typeof principal === 'string' && principal === '*') { - if (reverse) { - // In case of anonymous NotPrincipal, this will neutral everyone - return 'Neutral'; - } - const conditionEval = Principal._evaluateCondition(params, statement); - if (!conditionEval || conditionEval.allow === false) { - return 'Neutral'; + const reverse = !!statement.NotPrincipal; + if (typeof principal === 'string') { + if (principal === '*') { + return Principal._evaluateStatement(params, statement); } - return statement.Effect; - } else if (typeof principal === 'string') { return 'Deny'; } let ref = []; @@ -82,28 +82,8 @@ export default class Principal { } toCheck = Array.isArray(toCheck) ? toCheck : [toCheck]; ref = Array.isArray(ref) ? ref : [ref]; - if (toCheck.indexOf('*') !== -1) { - if (reverse) { - return 'Neutral'; - } - const conditionEval = Principal._evaluateCondition(params, statement); - if (!conditionEval || conditionEval.allow === false) { - return 'Neutral'; - } - return statement.Effect; - } - const len = ref.length; - for (let i = 0; i < len; ++i) { - if (toCheck.indexOf(ref[i]) !== -1) { - if (reverse) { - return 'Neutral'; - } - const conditionEval = Principal._evaluateCondition(params, statement); - if (!conditionEval || conditionEval.allow === false) { - return 'Neutral'; - } - return statement.Effect; - } + if (toCheck.includes('*') || ref.some(r => toCheck.includes(r))) { + return Principal._evaluateStatement(params, statement); } if (reverse) { return statement.Effect; diff --git a/tests/unit/policyEvaluator.spec.js b/tests/unit/policyEvaluator.spec.js index 0605dbb4f..e534ebdcf 100644 --- a/tests/unit/policyEvaluator.spec.js +++ b/tests/unit/policyEvaluator.spec.js @@ -1162,30 +1162,6 @@ describe('policyEvaluator', () => { check(requestContext, rcModifiers, policy, 'Neutral'); }); - it('should allow with StringEquals operator and ExistingObjectTag ' + - 'key if meet condition', () => { - policy.Statement.Condition = { - StringEquals: { 's3:ExistingObjectTag/tagKey': 'tagValue' }, - }; - const rcModifiers = { - _existingObjTag: 'tagKey=tagValue', - _needTagEval: true, - }; - check(requestContext, rcModifiers, policy, 'Allow'); - }); - - it('should allow StringEquals operator and RequestObjectTag ' + - 'key if meet condition', () => { - policy.Statement.Condition = { - StringEquals: { 's3:RequestObjectTagKey/tagKey': 'tagValue' }, - }; - const rcModifiers = { - _requestObjTags: 'tagKey=tagValue', - _needTagEval: true, - }; - check(requestContext, rcModifiers, policy, 'Allow'); - }); - it('should allow with ForAnyValue prefix if meet condition', () => { policy.Statement.Condition = { 'ForAnyValue:StringLike': { 's3:RequestObjectTagKeys': ['tagOne', 'tagTwo'] }, @@ -1208,7 +1184,7 @@ describe('policyEvaluator', () => { check(requestContext, rcModifiers, policy, 'Allow'); }); - it('should not allow with ForAnyValue prefix if do not meet condition', () => { + it('should be neutral with ForAnyValue prefix if do not meet condition', () => { policy.Statement.Condition = { 'ForAnyValue:StringLike': { 's3:RequestObjectTagKeys': ['tagOne', 'tagTwo'] }, }; @@ -1219,12 +1195,12 @@ describe('policyEvaluator', () => { check(requestContext, rcModifiers, policy, 'Neutral'); }); - it('should not allow with ForAllValues prefix if do not meet condition', () => { + it('should be neutral with ForAllValues prefix if do not meet condition', () => { policy.Statement.Condition = { 'ForAllValues:StringLike': { 's3:RequestObjectTagKeys': ['tagOne', 'tagTwo'] }, }; const rcModifiers = { - _requestObjTags: 'tagThree=keyThree&tagFour=keyFour', + _requestObjTags: 'tagOne=keyOne&tagThree=keyThree', _needTagEval: true, }; check(requestContext, rcModifiers, policy, 'Neutral'); @@ -1241,6 +1217,203 @@ describe('policyEvaluator', () => { check(requestContext, rcModifiers, policy, 'Neutral'); }); }); + + describe('with multiple statements', () => { + beforeEach(() => { + requestContext = new RequestContext({}, {}, 'bucket', + undefined, undefined, undefined, 'objectPut', 's3'); + requestContext.setRequesterInfo({}); + }); + + const TestMatrix = [ + { + statementsToEvaluate: [], + expectedPolicyEvaluation: 'Neutral', + }, + { + statementsToEvaluate: [ + { effect: 'Allow', meetConditions: true }, + ], + expectedPolicyEvaluation: 'Allow', + }, + { + statementsToEvaluate: [ + { effect: 'Allow', meetConditions: false }, + ], + expectedPolicyEvaluation: 'Neutral', + }, + { + statementsToEvaluate: [ + { effect: 'Deny', meetConditions: true }, + ], + expectedPolicyEvaluation: 'Deny', + }, + { + statementsToEvaluate: [ + { effect: 'Deny', meetConditions: false }, + ], + expectedPolicyEvaluation: 'Neutral', + }, + { + statementsToEvaluate: [ + { effect: 'Allow', meetConditions: false }, + { effect: 'Allow', meetConditions: true }, + ], + expectedPolicyEvaluation: 'Allow', + }, + { + statementsToEvaluate: [ + { effect: 'Allow', meetConditions: false }, + { effect: 'Allow', meetConditions: false }, + ], + expectedPolicyEvaluation: 'Neutral', + }, + { + statementsToEvaluate: [ + { effect: 'Allow', meetConditions: false }, + { effect: 'Deny', meetConditions: false }, + ], + expectedPolicyEvaluation: 'Neutral', + }, + { + statementsToEvaluate: [ + { effect: 'Allow', meetConditions: true }, + { effect: 'Deny', meetConditions: true }, + ], + expectedPolicyEvaluation: 'Deny', + }, + { + statementsToEvaluate: [ + { effect: 'Deny', meetConditions: true }, + { effect: 'Allow', meetConditions: true }, + ], + expectedPolicyEvaluation: 'Deny', + }, + { + statementsToEvaluate: [ + { effect: 'Allow', meetConditions: true }, + { effect: 'Deny', meetConditions: false }, + ], + expectedPolicyEvaluation: 'Allow', + }, + { + statementsToEvaluate: [ + { effect: 'Allow', meetConditions: null }, + ], + expectedPolicyEvaluation: 'AllowWithTagCondition', + }, + { + statementsToEvaluate: [ + { effect: 'Allow', meetConditions: null }, + { effect: 'Allow', meetConditions: null }, + ], + expectedPolicyEvaluation: 'AllowWithTagCondition', + }, + { + statementsToEvaluate: [ + { effect: 'Deny', meetConditions: null }, + ], + expectedPolicyEvaluation: 'DenyWithTagCondition', + }, + { + statementsToEvaluate: [ + { effect: 'Deny', meetConditions: null }, + { effect: 'Deny', meetConditions: null }, + ], + expectedPolicyEvaluation: 'DenyWithTagCondition', + }, + { + statementsToEvaluate: [ + { effect: 'Allow', meetConditions: true }, + { effect: 'Allow', meetConditions: null }, + ], + expectedPolicyEvaluation: 'Allow', + }, + { + statementsToEvaluate: [ + { effect: 'Allow', meetConditions: false }, + { effect: 'Allow', meetConditions: null }, + ], + expectedPolicyEvaluation: 'AllowWithTagCondition', + }, + { + statementsToEvaluate: [ + { effect: 'Deny', meetConditions: true }, + { effect: 'Deny', meetConditions: null }, + ], + expectedPolicyEvaluation: 'Deny', + }, + { + statementsToEvaluate: [ + { effect: 'Deny', meetConditions: true }, + { effect: 'Allow', meetConditions: null }, + ], + expectedPolicyEvaluation: 'Deny', + }, + { + statementsToEvaluate: [ + { effect: 'Deny', meetConditions: false }, + { effect: 'Deny', meetConditions: null }, + ], + expectedPolicyEvaluation: 'DenyWithTagCondition', + }, + { + statementsToEvaluate: [ + { effect: 'Allow', meetConditions: true }, + { effect: 'Deny', meetConditions: null }, + ], + expectedPolicyEvaluation: 'DenyWithTagCondition', + }, + { + statementsToEvaluate: [ + { effect: 'Allow', meetConditions: null }, + { effect: 'Deny', meetConditions: null }, + ], + expectedPolicyEvaluation: 'DenyWithTagCondition', + }, + ]; + + TestMatrix.forEach(testCase => { + const policyDesc = testCase.statementsToEvaluate + .map(statement => `${statement.effect}(met:${statement.meetConditions})`) + .join(', '); + it(`policy with statements evaluating individually to [${policyDesc}] ` + + `should return ${testCase.expectedPolicyEvaluation}`, () => { + const policy = { + Version: '2012-10-17', + Statement: testCase.statementsToEvaluate.map(statement => { + let condition; + if (statement.meetConditions === true) { + condition = { + StringEquals: { 'aws:UserAgent': 'CyberSquaw' }, + }; + } else if (statement.meetConditions === false) { + condition = { + StringEquals: { 'aws:UserAgent': 'OtherAgent' }, + }; + } else if (statement.meetConditions === null) { + condition = { + StringEquals: { 's3:ExistingObjectTag/tagKey': 'tagValue' }, + }; + } + return { + Effect: statement.effect, + Action: 's3:PutObject', + Resource: 'arn:aws:s3:::bucket', + Condition: condition, + }; + }), + }; + requestContext.setHeaders({ + 'user-agent': 'CyberSquaw', + }); + requestContext.setNeedTagEval(false); + + const result = evaluatePolicy(requestContext, policy, log); + assert.strictEqual(result, testCase.expectedPolicyEvaluation); + }); + }); + }); }); describe('evaluate multiple policies', () => { @@ -1266,17 +1439,152 @@ describe('policyEvaluator', () => { assert.strictEqual(result, 'Deny'); }); - it('should deny access if request resource is not in any policy', - () => { + 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'); + }); + + const TestMatrixPolicies = { + Allow: { + Version: '2012-10-17', + Statement: { + Effect: 'Allow', + Action: 's3:*', + Resource: '*', + }, + }, + Neutral: { + Version: '2012-10-17', + Statement: { + Effect: 'Allow', + Action: 's3:*', + Resource: 'arn:aws:s3:::other-bucket', + }, + }, + Deny: { + Version: '2012-10-17', + Statement: { + Effect: 'Deny', + Action: 's3:*', + Resource: '*', + }, + }, + AllowWithTagCondition: { + Version: '2012-10-17', + Statement: { + Effect: 'Allow', + Action: 's3:*', + Resource: '*', + Condition: { + StringEquals: { + 's3:ExistingObjectTag/tagKey': 'tagValue', + }, + }, + }, + }, + DenyWithTagCondition: { + Version: '2012-10-17', + Statement: { + Effect: 'Deny', + Action: 's3:*', + Resource: '*', + Condition: { + StringEquals: { + 's3:ExistingObjectTag/tagKey': 'tagValue', + }, + }, + }, + }, + }; + + 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(', ')}] ` + + `should return ${testCase.expectedPolicyEvaluation}`, () => { requestContext = new RequestContext({}, {}, - 'notbucket', undefined, + 'my_favorite_bucket', undefined, undefined, undefined, 'objectGet', 's3'); requestContext.setRequesterInfo({}); - const result = evaluateAllPolicies(requestContext, - [samples['Multi-Statement Policy'], - samples['Variable Bucket Policy']], log); - assert.strictEqual(result, 'Deny'); + const result = evaluateAllPolicies( + requestContext, + testCase.policiesToEvaluate.map(policyName => TestMatrixPolicies[policyName]), + log); + assert.strictEqual(result, testCase.expectedPolicyEvaluation); }); + }); }); });