Permission subset checks when creating roles #64

billkalter · 2016-11-10T21:34:13Z

Github Issue

What Are We Doing Here?

This pull request contains the following overarching changes, also each of these contains several smaller details to make the overall implementation possible:

Added ability to determine if one Condition is a subset of another Condition. For example, like("ge*us") is a subset of like("g*s").
Expanded permission implication to include open-ended conditions. For example, sor|if(in("read","update","create_table","drop_table"))|if({..,"client":+}) implies sor|if(in("read","update"))|if({..,"client":"testclient"}), since for all permissions p if the latter implies p the former also implies p.
Enforced that when addition permissions to a role the calling use must have permission to perform the permissions he is granting.

How to Test and Verify

Check out this PR
Start EmoDB locally
Run the following sequence and verify similar results:

$ curl -XPOST 'localhost:8081/tasks/role?action=update&role=delegation&permit=system|manage_roles&permit=sor|read|*&APIKey=local_admin'
Role updated.
delegation has 2 permissions
- sor|read|*
- system|manage_roles
$ curl -XPOST 'localhost:8081/tasks/api-key?action=create&owner=test&description=test&role=delegation&APIKey=local_admin'
API key: md4bastpqptnob9buiewz3g49kfvzi52ispfsjg32a2rfmyx

Warning:  This is your only chance to see this key.  Save it somewhere now.
$ curl -XPOST 'localhost:8081/tasks/role?action=update&role=illegal&permit=sor|*&APIKey=md4bastpqptnob9buiewz3g49kfvzi52ispfsjg32a2rfmyx'
You do not has sufficient permissions to grant the following permission(s):
- sor|*
Please remove or rewrite the above permission(s) constrained within your permissions

Risk

The main risk associated with this ticket is potentially breaking the ability to create and update roles or a regression on evaluating permissions.

Level

Somewhere between Medium and High. This change affects core Emo functionality -- security -- but does not put data at risk.

Required Testing

Manual

Risk Summary

In addition to the risks mentioned above and in the corresponding ticket, the biggest risk is in an incorrect implementation of condition subset and permission implication checks. Any issues where a user can grant wider permissions than he has is a potential security hole.

Code Review Checklist

Tests are included. If not, make sure you leave us a line or two for the reason.
Pulled down the PR and performed verification of at least being able to
build and run.
Well documented, including updates to any necessary markdown files. When
we inevitably come back to this code it will only take hours to figure out, not
days.
Consistent/Clear/Thoughtful? We are better with this code. We also aren't
a victim of rampaging consistency, and should be using this course of action.
We don't have coding standards out yet for this project, so please make sure to address any feedback regarding STYLE so the codebase remains consistent.
PR has a valid summary, and a good description.

sujithvaddi

@billkalter added few comments.

spent enough time in cross checking the new overlaps() and isSubsetof() condition implementations with my own examples as well as with Subset Evaluator Visit() cases. I think your SubsetEvaluatorTest covered all the type permutations of <Condition, Condition>, and I found nothing in there to be added to disprove any case.
verified overlaps() and isSubsetOf() implementations for comparison, like and contains conditions.
verified the Subset Evaluator (and Distinct and Inverse Evaluators which are used with in it).
verified the tests.

sujithvaddi · 2017-05-17T15:58:49Z

sor-api/src/main/java/com/bazaarvoice/emodb/sor/condition/impl/ComparisonConditionImpl.java

+        Comparable rValue;
+
+        if (lObject instanceof Number) {
+            if (!(rObject instanceof Number)) {


Minor: if (lObject instance Number && rObject instanceof Number ) and leaving false case to the last else.

It's a very minor efficiency gain, but if lObject is a Number then the next condition, which evaluates for lObject instanceof String, is always false, so it saves the unnecessary condition check.

I though its easy to read the code that way with only 2 if cases and one else for anything else.

sujithvaddi · 2017-05-17T16:00:44Z

sor-api/src/main/java/com/bazaarvoice/emodb/sor/condition/impl/ComparisonConditionImpl.java

+    @SuppressWarnings("unchecked")
+    @Override
+    public boolean isSubsetOf(ComparisonCondition condition) {
+        Object lObject = getValue();


Minor: @why not just use _value directly just to be consistent with the overlaps() implementation or the other way around.
Same with the getcomparision() usage line: 101

sujithvaddi · 2017-05-17T18:42:23Z

sor-api/src/main/java/com/bazaarvoice/emodb/sor/condition/eval/DistinctEvaluator.java

+     */
+    @Override
+    public Boolean visit(AndCondition left, Condition right) {
+        return false;


I think the comments are crystal clear that these methods are not called anywhere.....But I feel like it's better to throw NotImplementedException or UnsupportedOperationException instead of returning false values.
Applies to all the unused methods in DistinctEvaluator and InverseEvaluator.

sujithvaddi · 2017-05-18T15:47:41Z

quality/integration/src/test/java/test/integration/auth/RoleAdminTaskTest.java

+                "role", "any-role"),
+                new PrintWriter(out));
+
+        assertEquals(out.toString(), "Not authorized\n");


question here about the /role design - we are verifying that the API key created has no permission to view some random role, which is good but do you think it should have permission to view its own roles? the test gives same result even with "insufficient-role" on line 100. /api-key view is just showing the roles associated with it, (but is there any other way to see the permissions for a api-key)

Interesting point.

sujithvaddi · 2017-05-18T15:55:18Z

quality/integration/src/test/java/test/integration/auth/RoleAdminTaskTest.java

+                "action", "update",
+                "role", "new-role",
+                "permit", "sor|update|if({..,\"foo\":\"bar\"})",
+                "permit", "queue|post|*"),


Just a thought about the subset permission - even though A is subset of A, do we have any reason to prevent creating a new role with the different name with the same permission(s)? One advantage in preventing that is we can reuse the existing roles instead of just growing with the duplicate roles with the same permissions. Also, why would a user with ApiKey with sor|* permissions create a new role with the same sor|* to hand over to the other teams?

I agree that it opens the door for redundant roles, but I don't think it should be prohibited. For example, it may be that the new role today shares the same permissions, but by creating the new role it can be modified in the future without needing to be concerned about either unintended consequences of modifying the existing role or hunting down all users with the reused role and re-assigning their roles in the future.

sujithvaddi · 2017-05-18T18:50:21Z

sor-api/src/main/java/com/bazaarvoice/emodb/sor/condition/eval/SubsetEvaluator.java

+
+        @Override
+        public Boolean visit(EqualCondition right, Void context) {
+            return false;


similar comment as the other one - should we throw a UnsupportedOperationException for these cases which are not implemented in the default Left Visitor? The subclasses are implementing if they are needed, otherwise its better to know I think.

In this case it's not that they're unsupported, it's a convenience for sub-classes. The matrix of condition pairs which have meaningful subset evaluations is somewhat sparse, so by providing defaults of false the subsets only need to override those subsets which could potentially return true.

yeah - agreed. I'm fine with leaving the default implementation returning false and subclasses only implementing if they want to return true, specially considering the sparseness as you mentioned.

sujithvaddi · 2017-05-18T19:01:41Z

Also we need some rebasing...

… role grants restrict to caller's permissions

billkalter · 2017-05-19T16:24:32Z

@sujithvaddi Addressed feedback and rebased

sujithvaddi

@billkalter 👍

billkalter added the enhancement label Nov 10, 2016

billkalter mentioned this pull request Mar 8, 2017

Refactored permissions for roles and API keys #97

Merged

5 tasks

billkalter force-pushed the emo-6184 branch from c526518 to 7efeaa8 Compare March 21, 2017 13:43

sujithvaddi reviewed May 18, 2017

View reviewed changes

billkalter added 2 commits May 19, 2017 09:46

Expanded ability to check permissions for subset permissions; enforce…

99c2d22

… role grants restrict to caller's permissions

Addressed feedback

02680db

billkalter force-pushed the emo-6184 branch from 7efeaa8 to 02680db Compare May 19, 2017 15:41

sujithvaddi approved these changes May 19, 2017

View reviewed changes

billkalter merged commit 6f97791 into bazaarvoice:master May 22, 2017

billkalter deleted the emo-6184 branch May 22, 2017 16:50

billkalter pushed a commit to billkalter/emodb that referenced this pull request May 22, 2017

Rebased from master and adjusted for changes from PR bazaarvoice#64

3fd4de0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Permission subset checks when creating roles #64

Permission subset checks when creating roles #64

billkalter commented Nov 10, 2016

sujithvaddi left a comment

sujithvaddi May 17, 2017

billkalter May 19, 2017

sujithvaddi May 19, 2017

sujithvaddi May 17, 2017

sujithvaddi May 17, 2017

sujithvaddi May 18, 2017

billkalter May 19, 2017

sujithvaddi May 18, 2017

billkalter May 19, 2017

sujithvaddi May 18, 2017

billkalter May 19, 2017

sujithvaddi May 19, 2017

sujithvaddi commented May 18, 2017

billkalter commented May 19, 2017

sujithvaddi left a comment

Permission subset checks when creating roles #64

Permission subset checks when creating roles #64

Conversation

billkalter commented Nov 10, 2016

Github Issue

What Are We Doing Here?

How to Test and Verify

Risk

Level

Required Testing

Risk Summary

Code Review Checklist

sujithvaddi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sujithvaddi commented May 18, 2017

billkalter commented May 19, 2017

sujithvaddi left a comment

Choose a reason for hiding this comment