[jira] [Commented] (HBASE-11384) [Visibility Controller]Check for users covering authorizations for every mutation

[Andrew Purtell (JIRA)]

    [ 
https://issues.apache.org/jira/browse/HBASE-11384?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14043928#comment-14043928
 ] 

Andrew Purtell commented on HBASE-11384:
----------------------------------------

Patch looks good.

{code}
+
+  /**
+   * Used with visibility expression. Setting this would mean that for every
+   * mutation the labels in the visibility expressions are validated against 
the
+   * labels associated with the user issuing the mutation. If not found then 
the
+   * mutation would be failed.
+   * 
{code}

Please rephrase the above. 

{code}
@@ -938,7 +944,8 @@ public class VisibilityController extends 
BaseRegionObserver implements MasterOb
     }
   }
 
-  private void getLabelOrdinals(ExpressionNode node, List<Integer> 
labelOrdinals)
+  private void getLabelOrdinals(ExpressionNode node, List<Integer> 
labelOrdinals,
+      List<Integer> auths, String userName)
       throws IOException, InvalidLabelException {
     if (node.isSingleNode()) {
       String identifier = null;
{code}

Would it be easier to follow the VC logic if getLabelOrdinals does exactly that 
without side effects like making an access control decision? Consider moving 
checkAuths out of getLabelOrdinals if that seems reasonable. 

{code}
@@ -965,7 +974,18 @@ public class VisibilityController extends 
BaseRegionObserver implements MasterOb
     } else {
       List<ExpressionNode> childExps = ((NonLeafExpressionNode) 
node).getChildExps();
       for (ExpressionNode child : childExps) {
-        getLabelOrdinals(child, labelOrdinals);
+        getLabelOrdinals(child, labelOrdinals, auths, userName);
+      }
+    }
+  }
+
+  private void checkAuths(List<Integer> auths, String userName, int 
labelOrdinal, 
+      String identifier) throws InvalidLabelException {
+    if (auths != null) {
+      if (!auths.contains(labelOrdinal)) {
+        throw new InvalidLabelException("Visibility label "
+            + identifier + " not associated with user "
+            + userName);
       }
     }
   }
{code}

Should that be AccessDeniedException? It's a properly formatted label; the user 
just isn't allowed to use one of the given auths. 

bq. For the Puts that happen through MR also should be validated against the 
user? Currently we are supporting loading of Visibility labels thro ImportTSV 
tool.

I don't think that's necessary. We don't validate during bulk loading of HFiles 
either. I checked the Accumulo docs and looks like they don't validate during 
bulk loading actions either.

> [Visibility Controller]Check for users covering authorizations for every 
> mutation
> ---------------------------------------------------------------------------------
>
>                 Key: HBASE-11384
>                 URL: https://issues.apache.org/jira/browse/HBASE-11384
>             Project: HBase
>          Issue Type: Sub-task
>    Affects Versions: 0.98.3
>            Reporter: ramkrishna.s.vasudevan
>            Assignee: ramkrishna.s.vasudevan
>             Fix For: 0.99.0, 0.98.5
>
>         Attachments: HBASE-11384.patch
>
>
> As part of discussions, it is better that every mutation either Put/Delete 
> with Visibility expressions should validate if the expression has labels for 
> which the user has authorization.  If not fail the mutation.
> Suppose User A is assoicated with A,B and C.  The put has a visibility 
> expression A&D. Then fail the mutation as D is not associated with User A.



--
This message was sent by Atlassian JIRA
(v6.2#6252)