sentry-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ak...@apache.org
Subject sentry git commit: SENTRY-1609: DelegateSentryStore is subject to JDQL injection (Alex Kolbasov, Review by: Vamsee Yarlagadda and kalyan kumar kalvagadda)
Date Wed, 08 Feb 2017 20:32:25 GMT
Repository: sentry
Updated Branches:
  refs/heads/sentry-ha-redesign a79f923dc -> 5849ab0e5


SENTRY-1609: DelegateSentryStore is subject to JDQL injection (Alex Kolbasov, Review by: Vamsee
Yarlagadda and kalyan kumar kalvagadda)

The fix re-implements getGroupsByRoles() using SentryStore methods instead of directly issuing
queries.


Project: http://git-wip-us.apache.org/repos/asf/sentry/repo
Commit: http://git-wip-us.apache.org/repos/asf/sentry/commit/5849ab0e
Tree: http://git-wip-us.apache.org/repos/asf/sentry/tree/5849ab0e
Diff: http://git-wip-us.apache.org/repos/asf/sentry/diff/5849ab0e

Branch: refs/heads/sentry-ha-redesign
Commit: 5849ab0e530a8e3a250b7e58fde211842960288f
Parents: a79f923
Author: Alexander Kolbasov <akolb@cloudera.com>
Authored: Wed Feb 8 12:29:52 2017 -0800
Committer: Alexander Kolbasov <akolb@cloudera.com>
Committed: Wed Feb 8 12:29:52 2017 -0800

----------------------------------------------------------------------
 .../service/persistent/DelegateSentryStore.java | 51 ++++++++------------
 .../db/service/persistent/SentryStore.java      |  2 +-
 2 files changed, 22 insertions(+), 31 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/sentry/blob/5849ab0e/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
----------------------------------------------------------------------
diff --git a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
index 23f0891..0c5d019 100644
--- a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
+++ b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
@@ -20,12 +20,10 @@ package org.apache.sentry.provider.db.generic.service.persistent;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashSet;
-import java.util.LinkedList;
 import java.util.List;
 import java.util.Set;
 
 import javax.jdo.PersistenceManager;
-import javax.jdo.Query;
 
 import org.apache.hadoop.conf.Configuration;
 import org.apache.sentry.core.common.exception.SentryUserException;
@@ -45,7 +43,6 @@ import org.apache.sentry.provider.db.service.thrift.TSentryRole;
 import org.apache.sentry.service.thrift.ServiceConstants.ServerConfig;
 
 import com.google.common.annotations.VisibleForTesting;
-import com.google.common.base.Joiner;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableSet;
@@ -252,37 +249,31 @@ public class DelegateSentryStore implements SentryStoreLayer {
   @Override
   public Set<String> getGroupsByRoles(final String component, final Set<String>
roles)
       throws Exception {
+    // In all calls roles contain exactly one group
     if (roles.isEmpty()) {
       return Collections.emptySet();
     }
 
-    return delegate.getTransactionManager().executeTransaction(
-      new TransactionBlock<Set<String>>() {
-        public Set<String> execute(PersistenceManager pm) throws Exception {
-          //get groups by roles
-          final Set<String> trimmedRoles = SentryStore.toTrimedLower(roles);
-
-          Query query = pm.newQuery(MSentryGroup.class);
-          StringBuilder filters = new StringBuilder();
-          query.declareVariables("MSentryRole role");
-          List<String> rolesFiler = new LinkedList<>();
-          for (String role : trimmedRoles) {
-            rolesFiler.add("role.roleName == \"" + role + "\" ");
-          }
-          filters.append("roles.contains(role) " + "&& (" + Joiner.on(" || ").join(rolesFiler)
+ ")");
-          query.setFilter(filters.toString());
-          @SuppressWarnings("unchecked")
-          List<MSentryGroup> groups = (List<MSentryGroup>)query.execute();
-          if (groups.isEmpty()) {
-            return Collections.emptySet();
-          }
-          Set<String> groupNames = new HashSet<>(groups.size());
-          for (MSentryGroup group : groups) {
-            groupNames.add(group.getGroupName());
-          }
-          return groupNames;
-        }
-      });
+    // Collect resulting group names in a set
+    Set<String> groupNames = new HashSet<>();
+    for (String role : roles) {
+      MSentryRole sentryRole = null;
+      try {
+        sentryRole = delegate.getMSentryRoleByName(role);
+      }
+      catch (SentryNoSuchObjectException e) {
+        // Role disappeared - not a big deal, just ognore it
+        continue;
+      }
+      // Collect all group names for this role.
+      // Since we use a set, a group can appear multiple times and will only
+      // show up once in a set
+      for (MSentryGroup group : sentryRole.getGroups()) {
+        groupNames.add(group.getGroupName());
+      }
+    }
+
+    return groupNames;
   }
 
   @Override

http://git-wip-us.apache.org/repos/asf/sentry/blob/5849ab0e/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
----------------------------------------------------------------------
diff --git a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
index bfc6da5..4cc123f 100644
--- a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
+++ b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
@@ -1197,7 +1197,7 @@ public class SentryStore {
   }
 
   @VisibleForTesting
-  MSentryRole getMSentryRoleByName(final String roleName) throws Exception {
+  public MSentryRole getMSentryRoleByName(final String roleName) throws Exception {
     return tm.executeTransaction(
         new TransactionBlock<MSentryRole>() {
           public MSentryRole execute(PersistenceManager pm) throws Exception {


Mime
View raw message