Merge branches 'w/8.1/bugfix/ARSN-255-revampEvaluatePolicyForTagConditions' and 'q/1989/7.10/bugfix/ARSN-255-revampEvaluatePolicyForTagConditions' into tmp/octopus/q/8.1

bert-e · bert-e · commit a4f163f466e0 · 2022-08-17T22:01:22.000Z
diff --git 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
@@ -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
