Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvement/arsn 362 implicit deny #2181

Merged
merged 5 commits into from
Nov 3, 2023
+124 −27
Merged

Improvement/arsn 362 implicit deny #2181

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
79c82a4
ARSN-362: add implicit deny logic to policy evaluation
Sep 11, 2023
39988e5
ARSN-362: add implicit deny logic to policy eval tests
Sep 11, 2023
7777831
ARSN-362: change new function name for clarity
benzekrimaha Oct 30, 2023
df5ff0f
ARSN-362:fixups on impl deny policy tests
benzekrimaha Oct 30, 2023
fbf5562
bump arsenal version
benzekrimaha Oct 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 19 additions & 3 deletions lib/policyEvaluator/evaluator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,7 @@ export function evaluatePolicy(
}

/**
* @deprecated Upgrade to standardEvaluateAllPolicies
* Evaluate whether a request is permitted under a policy.
* @param requestContext - Info necessary to
* evaluate permission
Expand All @@ -325,6 +326,16 @@ export function evaluateAllPolicies(
allPolicies: any[],
log: Logger,
): string {
return standardEvaluateAllPolicies(requestContext, allPolicies, log).verdict;
}
export function standardEvaluateAllPolicies(
requestContext: RequestContext,
allPolicies: any[],
log: Logger,
): {
verdict: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a discussion about this some time ago with William maybe in a PR (I can't find the ref) it was about returning a single state string like ImplicitDeny or ExplicitDeny rather than an object with a boolean, to make it simpler and more visual, forcing to handle all states explicitly. There could have been reasons to stick to using a boolean maybe for backward-compatibility, since I'm not aware of this just want to make sure it was a conscious decision.

Copy link
Contributor Author

@benzekrimaha benzekrimaha Oct 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I indeed found the comment you are refering to here : https://github.com/scality/citadel/pull/165#discussion_r1276904304
I'm looping @williamlardier in for more insight on this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is for backward compatibility, that we use the boolean, given that we want to keep both flows. With a boolean, we can minimize the changes applied to our code base.

But our last discussion was about the request contexts responses (i.e., in Vault), not the internal logic. The request context API must be updated as stated in the design, but the evaluation logic can stay like that, unless we see benefits of having a uniform authz result starting at Arsenal level (maybe by creating a new typescript type or enum). Then in cloudserver we can check the authz result using string comparisons, and update the functions here (and associated tests).

As discussed with Maha already, if we want to deviate from the design (with good reasons), we must update the design. Here, the current code looks compatible with it, but I think having a unified implementation would be better indeed. It will require creating the new authz types in arsenal, update the functions, and then reuse them in Vault/Cloudserver.

isImplicit: boolean;
} {
log.trace('evaluating all policies');
let allow = false;
let allowWithTagCondition = false;
Expand All @@ -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;
Expand All @@ -344,6 +358,7 @@ export function evaluateAllPolicies(
} // else 'Neutral'
}
let verdict;
let isImplicit = false;
if (allow) {
if (denyWithTagCondition) {
verdict = 'NeedTagConditionEval';
Expand All @@ -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 };
}
127 changes: 104 additions & 23 deletions tests/unit/policyEvaluator.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 standardEvaluateAllPolicies = evaluator.standardEvaluateAllPolicies;
const handleWildcards =
require('../../lib/policyEvaluator/utils/wildcards').handleWildcards;
const substituteVariables =
Expand Down Expand Up @@ -1422,33 +1423,42 @@ describe('policyEvaluator', () => {
'my_favorite_bucket', undefined,
undefined, undefined, 'bucketDelete', 's3');
requestContext.setRequesterInfo({});
const result = evaluateAllPolicies(requestContext,
const result = standardEvaluateAllPolicies(requestContext,
[samples['arn:aws:iam::aws:policy/AmazonS3FullAccess'],
samples['Deny Bucket Policy']], log);
assert.strictEqual(result, 'Deny');
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 = evaluateAllPolicies(requestContext,
const result = standardEvaluateAllPolicies(requestContext,
[samples['Multi-Statement Policy'],
samples['Variable Bucket Policy']], log);
assert.strictEqual(result, 'Deny');
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 = evaluateAllPolicies(requestContext, [
const result = standardEvaluateAllPolicies(requestContext, [
samples['Multi-Statement Policy'],
samples['Variable Bucket Policy'],
], log);
assert.strictEqual(result, 'Deny');
assert.deepStrictEqual(result, {
verdict: 'Deny',
isImplicit: true,
});
});

const TestMatrixPolicies = {
Expand Down Expand Up @@ -1507,70 +1517,141 @@ describe('policyEvaluator', () => {
const TestMatrix = [
{
policiesToEvaluate: [],
expectedPolicyEvaluation: 'Deny',
expectedPolicyEvaluation: {
verdict: 'Deny',
isImplicit: true,
},
},
{
policiesToEvaluate: ['Allow'],
expectedPolicyEvaluation: 'Allow',
expectedPolicyEvaluation: {
verdict: 'Allow',
isImplicit: false,
},
},
{
policiesToEvaluate: ['Neutral'],
expectedPolicyEvaluation: 'Deny',
expectedPolicyEvaluation: {
verdict: 'Deny',
isImplicit: true,
},
},
{
policiesToEvaluate: ['Deny'],
expectedPolicyEvaluation: 'Deny',
expectedPolicyEvaluation: {
verdict: 'Deny',
isImplicit: false,
},
},
{
policiesToEvaluate: ['Allow', 'Allow'],
expectedPolicyEvaluation: 'Allow',
expectedPolicyEvaluation: {
verdict: 'Allow',
isImplicit: false,
},
},
{
policiesToEvaluate: ['Allow', 'Neutral'],
expectedPolicyEvaluation: 'Allow',
expectedPolicyEvaluation: {
verdict: 'Allow',
isImplicit: false,
},
},
{
policiesToEvaluate: ['Neutral', 'Allow'],
expectedPolicyEvaluation: 'Allow',
expectedPolicyEvaluation: {
verdict: 'Allow',
isImplicit: false,
},
},
{
policiesToEvaluate: ['Neutral', 'Neutral'],
expectedPolicyEvaluation: 'Deny',
expectedPolicyEvaluation: {
verdict: 'Deny',
isImplicit: true,
},
},
{
policiesToEvaluate: ['Neutral', 'Deny'],
expectedPolicyEvaluation: {
verdict: 'Deny',
isImplicit: false,
},
},
{
policiesToEvaluate: ['Allow', 'Deny'],
expectedPolicyEvaluation: 'Deny',
expectedPolicyEvaluation: {
verdict: 'Deny',
isImplicit: false,
},
},
{
policiesToEvaluate: ['AllowWithTagCondition'],
expectedPolicyEvaluation: 'NeedTagConditionEval',
expectedPolicyEvaluation: {
verdict: 'NeedTagConditionEval',
isImplicit: false,
},
},
{
policiesToEvaluate: ['Allow', 'AllowWithTagCondition'],
expectedPolicyEvaluation: 'Allow',
expectedPolicyEvaluation: {
verdict: 'Allow',
isImplicit: false,
},
},
{
policiesToEvaluate: ['DenyWithTagCondition'],
expectedPolicyEvaluation: 'Deny',
expectedPolicyEvaluation: {
verdict: 'Deny',
isImplicit: true,
},
},
{
policiesToEvaluate: ['Allow', 'DenyWithTagCondition'],
expectedPolicyEvaluation: 'NeedTagConditionEval',
expectedPolicyEvaluation: {
verdict: 'NeedTagConditionEval',
isImplicit: false,
},
},
{
policiesToEvaluate: ['AllowWithTagCondition', 'DenyWithTagCondition'],
expectedPolicyEvaluation: 'NeedTagConditionEval',
expectedPolicyEvaluation: {
verdict: 'NeedTagConditionEval',
isImplicit: false,
},
},
{
policiesToEvaluate: ['AllowWithTagCondition', 'DenyWithTagCondition', 'Deny'],
expectedPolicyEvaluation: 'Deny',
expectedPolicyEvaluation: {
verdict: 'Deny',
isImplicit: false,
},
},
{
policiesToEvaluate: ['DenyWithTagCondition', 'AllowWithTagCondition', 'Allow'],
expectedPolicyEvaluation: 'NeedTagConditionEval',
expectedPolicyEvaluation: {
verdict: 'NeedTagConditionEval',
isImplicit: false,
},
},
];

TestMatrix.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 = standardEvaluateAllPolicies(
requestContext,
testCase.policiesToEvaluate.map(policyName => TestMatrixPolicies[policyName]),
log);
assert.deepStrictEqual(result, testCase.expectedPolicyEvaluation);
});
});


TestMatrix.forEach(testCase => {
it(`policies evaluating individually to [${testCase.policiesToEvaluate.join(', ')}] `
+ `should return ${testCase.expectedPolicyEvaluation}`, () => {
Expand All @@ -1582,7 +1663,7 @@ describe('policyEvaluator', () => {
requestContext,
testCase.policiesToEvaluate.map(policyName => TestMatrixPolicies[policyName]),
log);
assert.strictEqual(result, testCase.expectedPolicyEvaluation);
assert.strictEqual(result, testCase.expectedPolicyEvaluation.verdict);
});
});
});
Expand Down
Loading