You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@phoenix.apache.org by el...@apache.org on 2017/02/17 17:45:20 UTC

[1/7] phoenix git commit: PHOENIX-3684 Handle _HOST in principal specified in JDBC url

Repository: phoenix
Updated Branches:
  refs/heads/4.9-HBase-0.98 aee05ae7c -> 46c4cdd70
  refs/heads/4.9-HBase-1.1 8a589a694 -> 1fb63c96b
  refs/heads/4.9-HBase-1.2 a967db5d8 -> 485551151
  refs/heads/4.x-HBase-0.98 4728a89e3 -> 54ae6f3ac
  refs/heads/4.x-HBase-1.1 e7a3975ed -> 893bad336
  refs/heads/4.x-HBase-1.3 d96a7551a -> 0099ec5e1
  refs/heads/master c3b16cef5 -> 130f29dc5


PHOENIX-3684 Handle _HOST in principal specified in JDBC url

When the user provides a principal with the _HOST "magic string"
in it, Phoenix was incorrectly re-logging that user in. This results
in an application that creates a new HConnection and ZK connection
for every Phoenix connection which saturates connections to ZK.


Project: http://git-wip-us.apache.org/repos/asf/phoenix/repo
Commit: http://git-wip-us.apache.org/repos/asf/phoenix/commit/130f29dc
Tree: http://git-wip-us.apache.org/repos/asf/phoenix/tree/130f29dc
Diff: http://git-wip-us.apache.org/repos/asf/phoenix/diff/130f29dc

Branch: refs/heads/master
Commit: 130f29dc5ca012868f4951849ca1af83577c4ccb
Parents: c3b16ce
Author: Josh Elser <el...@apache.org>
Authored: Thu Feb 16 14:47:35 2017 -0500
Committer: Josh Elser <el...@apache.org>
Committed: Fri Feb 17 11:32:01 2017 -0500

----------------------------------------------------------------------
 .../phoenix/jdbc/PhoenixEmbeddedDriver.java     | 15 ++++-
 .../phoenix/jdbc/PhoenixEmbeddedDriverTest.java | 10 +++
 .../phoenix/jdbc/SecureUserConnectionsTest.java | 66 +++++++++++++++++++-
 3 files changed, 88 insertions(+), 3 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/phoenix/blob/130f29dc/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java b/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java
index d41826f..2a496c6 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java
@@ -372,7 +372,7 @@ public abstract class PhoenixEmbeddedDriver implements Driver, SQLCloseable {
                     try {
                         // Check if we need to authenticate with kerberos so that we cache the correct ConnectionInfo
                         UserGroupInformation currentUser = UserGroupInformation.getCurrentUser();
-                        if (!currentUser.hasKerberosCredentials() || !currentUser.getUserName().equals(principal)) {
+                        if (!currentUser.hasKerberosCredentials() || !isSameName(currentUser.getUserName(), principal)) {
                             synchronized (KERBEROS_LOGIN_LOCK) {
                                 // Double check the current user, might have changed since we checked last. Don't want
                                 // to re-login if it's the same user.
@@ -402,6 +402,19 @@ public abstract class PhoenixEmbeddedDriver implements Driver, SQLCloseable {
             return new ConnectionInfo(zookeeperQuorum, port, rootNode, principal, keytab);
         }
 
+        // Visible for testing
+        static boolean isSameName(String currentName, String newName) throws IOException {
+            return isSameName(currentName, newName, null);
+        }
+
+        static boolean isSameName(String currentName, String newName, String hostname) throws IOException {
+            // Make sure to replace "_HOST" if it exists before comparing the principals.
+            if (newName.contains(org.apache.hadoop.security.SecurityUtil.HOSTNAME_PATTERN)) {
+                newName = org.apache.hadoop.security.SecurityUtil.getServerPrincipal(newName, hostname);
+            }
+            return currentName.equals(newName);
+        }
+
         /**
          * Constructs a Configuration object to use when performing a Kerberos login.
          * @param props QueryServices properties

http://git-wip-us.apache.org/repos/asf/phoenix/blob/130f29dc/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java
index 9d7fbfc..59465e8 100644
--- a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java
+++ b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java
@@ -158,4 +158,14 @@ public class PhoenixEmbeddedDriverTest {
         assertTrue(driver.acceptsURL("jdbc:phoenix:localhost:123;untest=true;foo=bar"));
         DriverManager.deregisterDriver(driver);
     }
+
+    @Test
+    public void testPrincipalsMatching() throws Exception {
+        assertTrue(ConnectionInfo.isSameName("user@EXAMPLE.COM", "user@EXAMPLE.COM"));
+        assertTrue(ConnectionInfo.isSameName("user/localhost@EXAMPLE.COM", "user/localhost@EXAMPLE.COM"));
+        // the user provided name might have a _HOST in it, which should be replaced by the hostname
+        assertTrue(ConnectionInfo.isSameName("user/localhost@EXAMPLE.COM", "user/_HOST@EXAMPLE.COM", "localhost"));
+        assertFalse(ConnectionInfo.isSameName("user/foobar@EXAMPLE.COM", "user/_HOST@EXAMPLE.COM", "localhost"));
+        assertFalse(ConnectionInfo.isSameName("user@EXAMPLE.COM", "user/_HOST@EXAMPLE.COM", "localhost"));
+    }
 }

http://git-wip-us.apache.org/repos/asf/phoenix/blob/130f29dc/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java
index 4996f94..5a99b69 100644
--- a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java
+++ b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java
@@ -62,6 +62,7 @@ public class SecureUserConnectionsTest {
     private static final File KEYTAB_DIR = new File(TEMP_DIR, "keytabs");
     private static final File KDC_DIR = new File(TEMP_DIR, "kdc");
     private static final List<File> USER_KEYTAB_FILES = new ArrayList<>();
+    private static final List<File> SERVICE_KEYTAB_FILES = new ArrayList<>();
     private static final int NUM_USERS = 3;
     private static final Properties EMPTY_PROPERTIES = new Properties();
     private static final String BASE_URL = PhoenixRuntime.JDBC_PROTOCOL + ":localhost:2181";
@@ -90,6 +91,7 @@ public class SecureUserConnectionsTest {
                 + " attempts.", started);
 
         createUsers(NUM_USERS);
+        createServiceUsers(NUM_USERS);
 
         final Configuration conf = new Configuration(false);
         conf.set(CommonConfigurationKeys.HADOOP_SECURITY_AUTHENTICATION, "kerberos");
@@ -163,6 +165,16 @@ public class SecureUserConnectionsTest {
         }
     }
 
+    private static void createServiceUsers(int numUsers) throws Exception {
+        assertNotNull("KDC is null, was setup method called?", KDC);
+        for (int i = 1; i <= numUsers; i++) {
+            String principal = "user" + i + "/localhost";
+            File keytabFile = new File(KEYTAB_DIR, "user" + i + ".service.keytab");
+            KDC.createPrincipal(keytabFile, principal);
+            SERVICE_KEYTAB_FILES.add(keytabFile);
+        }
+    }
+
     /**
      * Returns the principal for a user.
      *
@@ -172,6 +184,10 @@ public class SecureUserConnectionsTest {
         return "user" + offset + "@" + KDC.getRealm();
     }
 
+    private static String getServicePrincipal(int offset) {
+        return "user" + offset + "/localhost@" + KDC.getRealm();
+    }
+
     /**
      * Returns the keytab file for the corresponding principal with the same {@code offset}.
      * Requires {@link #createUsers(int)} to have been called with a value greater than {@code offset}.
@@ -179,8 +195,16 @@ public class SecureUserConnectionsTest {
      * @param offset The "number" for the principal whose keytab should be returned. One-based, not zero-based.
      */
     public static File getUserKeytabFile(int offset) {
-        assertTrue("Invalid offset: " + offset, (offset - 1) >= 0 && (offset - 1) < USER_KEYTAB_FILES.size());
-        return USER_KEYTAB_FILES.get(offset - 1);
+        return getKeytabFile(offset, USER_KEYTAB_FILES);
+    }
+
+    public static File getServiceKeytabFile(int offset) {
+        return getKeytabFile(offset, SERVICE_KEYTAB_FILES);
+    }
+
+    private static File getKeytabFile(int offset, List<File> keytabs) {
+        assertTrue("Invalid offset: " + offset, (offset - 1) >= 0 && (offset - 1) < keytabs.size());
+        return keytabs.get(offset - 1);
     }
 
     private String joinUserAuthentication(String origUrl, String principal, File keytab) {
@@ -389,6 +413,44 @@ public class SecureUserConnectionsTest {
         verifyAllConnectionsAreKerberosBased(connections);
     }
 
+    @Test
+    public void testHostSubstitutionInUrl() throws Exception {
+        final HashSet<ConnectionInfo> connections = new HashSet<>();
+        final String princ1 = getServicePrincipal(1);
+        final File keytab1 = getServiceKeytabFile(1);
+        final String princ2 = getServicePrincipal(2);
+        final File keytab2 = getServiceKeytabFile(2);
+        final String url1 = joinUserAuthentication(BASE_URL, princ1, keytab1);
+        final String url2 = joinUserAuthentication(BASE_URL, princ2, keytab2);
+
+        // Using the same UGI should result in two equivalent ConnectionInfo objects
+        connections.add(ConnectionInfo.create(url1).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(1, connections.size());
+        // Sanity check
+        verifyAllConnectionsAreKerberosBased(connections);
+
+        // Logging in as the same user again should not duplicate connections
+        connections.add(ConnectionInfo.create(url1).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(1, connections.size());
+        // Sanity check
+        verifyAllConnectionsAreKerberosBased(connections);
+
+        // Add a second one.
+        connections.add(ConnectionInfo.create(url2).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(2, connections.size());
+        verifyAllConnectionsAreKerberosBased(connections);
+
+        // Again, verify this user is not duplicated
+        connections.add(ConnectionInfo.create(url2).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(2, connections.size());
+        verifyAllConnectionsAreKerberosBased(connections);
+
+        // Because the UGI instances are unique, so are the connections
+        connections.add(ConnectionInfo.create(url1).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(3, connections.size());
+        verifyAllConnectionsAreKerberosBased(connections);
+    }
+
     private void verifyAllConnectionsAreKerberosBased(Collection<ConnectionInfo> connections) {
         for (ConnectionInfo cnxnInfo : connections) {
             assertTrue("ConnectionInfo does not have kerberos credentials: " + cnxnInfo, cnxnInfo.getUser().getUGI().hasKerberosCredentials());


[3/7] phoenix git commit: PHOENIX-3684 Handle _HOST in principal specified in JDBC url

Posted by el...@apache.org.
PHOENIX-3684 Handle _HOST in principal specified in JDBC url

When the user provides a principal with the _HOST "magic string"
in it, Phoenix was incorrectly re-logging that user in. This results
in an application that creates a new HConnection and ZK connection
for every Phoenix connection which saturates connections to ZK.


Project: http://git-wip-us.apache.org/repos/asf/phoenix/repo
Commit: http://git-wip-us.apache.org/repos/asf/phoenix/commit/893bad33
Tree: http://git-wip-us.apache.org/repos/asf/phoenix/tree/893bad33
Diff: http://git-wip-us.apache.org/repos/asf/phoenix/diff/893bad33

Branch: refs/heads/4.x-HBase-1.1
Commit: 893bad336e1e2aae4974cd9b26bcbbb537e16f39
Parents: e7a3975
Author: Josh Elser <el...@apache.org>
Authored: Thu Feb 16 14:47:35 2017 -0500
Committer: Josh Elser <el...@apache.org>
Committed: Fri Feb 17 11:49:15 2017 -0500

----------------------------------------------------------------------
 .../phoenix/jdbc/PhoenixEmbeddedDriver.java     | 15 ++++-
 .../phoenix/jdbc/PhoenixEmbeddedDriverTest.java | 10 +++
 .../phoenix/jdbc/SecureUserConnectionsTest.java | 66 +++++++++++++++++++-
 3 files changed, 88 insertions(+), 3 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/phoenix/blob/893bad33/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java b/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java
index d41826f..2a496c6 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java
@@ -372,7 +372,7 @@ public abstract class PhoenixEmbeddedDriver implements Driver, SQLCloseable {
                     try {
                         // Check if we need to authenticate with kerberos so that we cache the correct ConnectionInfo
                         UserGroupInformation currentUser = UserGroupInformation.getCurrentUser();
-                        if (!currentUser.hasKerberosCredentials() || !currentUser.getUserName().equals(principal)) {
+                        if (!currentUser.hasKerberosCredentials() || !isSameName(currentUser.getUserName(), principal)) {
                             synchronized (KERBEROS_LOGIN_LOCK) {
                                 // Double check the current user, might have changed since we checked last. Don't want
                                 // to re-login if it's the same user.
@@ -402,6 +402,19 @@ public abstract class PhoenixEmbeddedDriver implements Driver, SQLCloseable {
             return new ConnectionInfo(zookeeperQuorum, port, rootNode, principal, keytab);
         }
 
+        // Visible for testing
+        static boolean isSameName(String currentName, String newName) throws IOException {
+            return isSameName(currentName, newName, null);
+        }
+
+        static boolean isSameName(String currentName, String newName, String hostname) throws IOException {
+            // Make sure to replace "_HOST" if it exists before comparing the principals.
+            if (newName.contains(org.apache.hadoop.security.SecurityUtil.HOSTNAME_PATTERN)) {
+                newName = org.apache.hadoop.security.SecurityUtil.getServerPrincipal(newName, hostname);
+            }
+            return currentName.equals(newName);
+        }
+
         /**
          * Constructs a Configuration object to use when performing a Kerberos login.
          * @param props QueryServices properties

http://git-wip-us.apache.org/repos/asf/phoenix/blob/893bad33/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java
index 9d7fbfc..59465e8 100644
--- a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java
+++ b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java
@@ -158,4 +158,14 @@ public class PhoenixEmbeddedDriverTest {
         assertTrue(driver.acceptsURL("jdbc:phoenix:localhost:123;untest=true;foo=bar"));
         DriverManager.deregisterDriver(driver);
     }
+
+    @Test
+    public void testPrincipalsMatching() throws Exception {
+        assertTrue(ConnectionInfo.isSameName("user@EXAMPLE.COM", "user@EXAMPLE.COM"));
+        assertTrue(ConnectionInfo.isSameName("user/localhost@EXAMPLE.COM", "user/localhost@EXAMPLE.COM"));
+        // the user provided name might have a _HOST in it, which should be replaced by the hostname
+        assertTrue(ConnectionInfo.isSameName("user/localhost@EXAMPLE.COM", "user/_HOST@EXAMPLE.COM", "localhost"));
+        assertFalse(ConnectionInfo.isSameName("user/foobar@EXAMPLE.COM", "user/_HOST@EXAMPLE.COM", "localhost"));
+        assertFalse(ConnectionInfo.isSameName("user@EXAMPLE.COM", "user/_HOST@EXAMPLE.COM", "localhost"));
+    }
 }

http://git-wip-us.apache.org/repos/asf/phoenix/blob/893bad33/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java
index 4996f94..5a99b69 100644
--- a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java
+++ b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java
@@ -62,6 +62,7 @@ public class SecureUserConnectionsTest {
     private static final File KEYTAB_DIR = new File(TEMP_DIR, "keytabs");
     private static final File KDC_DIR = new File(TEMP_DIR, "kdc");
     private static final List<File> USER_KEYTAB_FILES = new ArrayList<>();
+    private static final List<File> SERVICE_KEYTAB_FILES = new ArrayList<>();
     private static final int NUM_USERS = 3;
     private static final Properties EMPTY_PROPERTIES = new Properties();
     private static final String BASE_URL = PhoenixRuntime.JDBC_PROTOCOL + ":localhost:2181";
@@ -90,6 +91,7 @@ public class SecureUserConnectionsTest {
                 + " attempts.", started);
 
         createUsers(NUM_USERS);
+        createServiceUsers(NUM_USERS);
 
         final Configuration conf = new Configuration(false);
         conf.set(CommonConfigurationKeys.HADOOP_SECURITY_AUTHENTICATION, "kerberos");
@@ -163,6 +165,16 @@ public class SecureUserConnectionsTest {
         }
     }
 
+    private static void createServiceUsers(int numUsers) throws Exception {
+        assertNotNull("KDC is null, was setup method called?", KDC);
+        for (int i = 1; i <= numUsers; i++) {
+            String principal = "user" + i + "/localhost";
+            File keytabFile = new File(KEYTAB_DIR, "user" + i + ".service.keytab");
+            KDC.createPrincipal(keytabFile, principal);
+            SERVICE_KEYTAB_FILES.add(keytabFile);
+        }
+    }
+
     /**
      * Returns the principal for a user.
      *
@@ -172,6 +184,10 @@ public class SecureUserConnectionsTest {
         return "user" + offset + "@" + KDC.getRealm();
     }
 
+    private static String getServicePrincipal(int offset) {
+        return "user" + offset + "/localhost@" + KDC.getRealm();
+    }
+
     /**
      * Returns the keytab file for the corresponding principal with the same {@code offset}.
      * Requires {@link #createUsers(int)} to have been called with a value greater than {@code offset}.
@@ -179,8 +195,16 @@ public class SecureUserConnectionsTest {
      * @param offset The "number" for the principal whose keytab should be returned. One-based, not zero-based.
      */
     public static File getUserKeytabFile(int offset) {
-        assertTrue("Invalid offset: " + offset, (offset - 1) >= 0 && (offset - 1) < USER_KEYTAB_FILES.size());
-        return USER_KEYTAB_FILES.get(offset - 1);
+        return getKeytabFile(offset, USER_KEYTAB_FILES);
+    }
+
+    public static File getServiceKeytabFile(int offset) {
+        return getKeytabFile(offset, SERVICE_KEYTAB_FILES);
+    }
+
+    private static File getKeytabFile(int offset, List<File> keytabs) {
+        assertTrue("Invalid offset: " + offset, (offset - 1) >= 0 && (offset - 1) < keytabs.size());
+        return keytabs.get(offset - 1);
     }
 
     private String joinUserAuthentication(String origUrl, String principal, File keytab) {
@@ -389,6 +413,44 @@ public class SecureUserConnectionsTest {
         verifyAllConnectionsAreKerberosBased(connections);
     }
 
+    @Test
+    public void testHostSubstitutionInUrl() throws Exception {
+        final HashSet<ConnectionInfo> connections = new HashSet<>();
+        final String princ1 = getServicePrincipal(1);
+        final File keytab1 = getServiceKeytabFile(1);
+        final String princ2 = getServicePrincipal(2);
+        final File keytab2 = getServiceKeytabFile(2);
+        final String url1 = joinUserAuthentication(BASE_URL, princ1, keytab1);
+        final String url2 = joinUserAuthentication(BASE_URL, princ2, keytab2);
+
+        // Using the same UGI should result in two equivalent ConnectionInfo objects
+        connections.add(ConnectionInfo.create(url1).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(1, connections.size());
+        // Sanity check
+        verifyAllConnectionsAreKerberosBased(connections);
+
+        // Logging in as the same user again should not duplicate connections
+        connections.add(ConnectionInfo.create(url1).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(1, connections.size());
+        // Sanity check
+        verifyAllConnectionsAreKerberosBased(connections);
+
+        // Add a second one.
+        connections.add(ConnectionInfo.create(url2).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(2, connections.size());
+        verifyAllConnectionsAreKerberosBased(connections);
+
+        // Again, verify this user is not duplicated
+        connections.add(ConnectionInfo.create(url2).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(2, connections.size());
+        verifyAllConnectionsAreKerberosBased(connections);
+
+        // Because the UGI instances are unique, so are the connections
+        connections.add(ConnectionInfo.create(url1).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(3, connections.size());
+        verifyAllConnectionsAreKerberosBased(connections);
+    }
+
     private void verifyAllConnectionsAreKerberosBased(Collection<ConnectionInfo> connections) {
         for (ConnectionInfo cnxnInfo : connections) {
             assertTrue("ConnectionInfo does not have kerberos credentials: " + cnxnInfo, cnxnInfo.getUser().getUGI().hasKerberosCredentials());


[6/7] phoenix git commit: PHOENIX-3684 Handle _HOST in principal specified in JDBC url

Posted by el...@apache.org.
PHOENIX-3684 Handle _HOST in principal specified in JDBC url

When the user provides a principal with the _HOST "magic string"
in it, Phoenix was incorrectly re-logging that user in. This results
in an application that creates a new HConnection and ZK connection
for every Phoenix connection which saturates connections to ZK.


Project: http://git-wip-us.apache.org/repos/asf/phoenix/repo
Commit: http://git-wip-us.apache.org/repos/asf/phoenix/commit/1fb63c96
Tree: http://git-wip-us.apache.org/repos/asf/phoenix/tree/1fb63c96
Diff: http://git-wip-us.apache.org/repos/asf/phoenix/diff/1fb63c96

Branch: refs/heads/4.9-HBase-1.1
Commit: 1fb63c96b9ecfca8da1617808c75281ca9e70a10
Parents: 8a589a6
Author: Josh Elser <el...@apache.org>
Authored: Thu Feb 16 14:47:35 2017 -0500
Committer: Josh Elser <el...@apache.org>
Committed: Fri Feb 17 12:06:29 2017 -0500

----------------------------------------------------------------------
 .../phoenix/jdbc/PhoenixEmbeddedDriver.java     | 15 ++++-
 .../phoenix/jdbc/PhoenixEmbeddedDriverTest.java | 10 +++
 .../phoenix/jdbc/SecureUserConnectionsTest.java | 66 +++++++++++++++++++-
 3 files changed, 88 insertions(+), 3 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/phoenix/blob/1fb63c96/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java b/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java
index 272fb22..2eca3d6 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java
@@ -350,7 +350,7 @@ public abstract class PhoenixEmbeddedDriver implements Driver, SQLCloseable {
                     try {
                         // Check if we need to authenticate with kerberos so that we cache the correct ConnectionInfo
                         UserGroupInformation currentUser = UserGroupInformation.getCurrentUser();
-                        if (!currentUser.hasKerberosCredentials() || !currentUser.getUserName().equals(principal)) {
+                        if (!currentUser.hasKerberosCredentials() || !isSameName(currentUser.getUserName(), principal)) {
                             synchronized (KERBEROS_LOGIN_LOCK) {
                                 // Double check the current user, might have changed since we checked last. Don't want
                                 // to re-login if it's the same user.
@@ -380,6 +380,19 @@ public abstract class PhoenixEmbeddedDriver implements Driver, SQLCloseable {
             return new ConnectionInfo(zookeeperQuorum, port, rootNode, principal, keytab);
         }
 
+        // Visible for testing
+        static boolean isSameName(String currentName, String newName) throws IOException {
+            return isSameName(currentName, newName, null);
+        }
+
+        static boolean isSameName(String currentName, String newName, String hostname) throws IOException {
+            // Make sure to replace "_HOST" if it exists before comparing the principals.
+            if (newName.contains(org.apache.hadoop.security.SecurityUtil.HOSTNAME_PATTERN)) {
+                newName = org.apache.hadoop.security.SecurityUtil.getServerPrincipal(newName, hostname);
+            }
+            return currentName.equals(newName);
+        }
+
         /**
          * Constructs a Configuration object to use when performing a Kerberos login.
          * @param props QueryServices properties

http://git-wip-us.apache.org/repos/asf/phoenix/blob/1fb63c96/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java
index 98a028c..475307f 100644
--- a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java
+++ b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java
@@ -154,4 +154,14 @@ public class PhoenixEmbeddedDriverTest {
         assertTrue(driver.acceptsURL("jdbc:phoenix:localhost:123;untest=true;foo=bar"));
         DriverManager.deregisterDriver(driver);
     }
+
+    @Test
+    public void testPrincipalsMatching() throws Exception {
+        assertTrue(ConnectionInfo.isSameName("user@EXAMPLE.COM", "user@EXAMPLE.COM"));
+        assertTrue(ConnectionInfo.isSameName("user/localhost@EXAMPLE.COM", "user/localhost@EXAMPLE.COM"));
+        // the user provided name might have a _HOST in it, which should be replaced by the hostname
+        assertTrue(ConnectionInfo.isSameName("user/localhost@EXAMPLE.COM", "user/_HOST@EXAMPLE.COM", "localhost"));
+        assertFalse(ConnectionInfo.isSameName("user/foobar@EXAMPLE.COM", "user/_HOST@EXAMPLE.COM", "localhost"));
+        assertFalse(ConnectionInfo.isSameName("user@EXAMPLE.COM", "user/_HOST@EXAMPLE.COM", "localhost"));
+    }
 }

http://git-wip-us.apache.org/repos/asf/phoenix/blob/1fb63c96/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java
index 4996f94..5a99b69 100644
--- a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java
+++ b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java
@@ -62,6 +62,7 @@ public class SecureUserConnectionsTest {
     private static final File KEYTAB_DIR = new File(TEMP_DIR, "keytabs");
     private static final File KDC_DIR = new File(TEMP_DIR, "kdc");
     private static final List<File> USER_KEYTAB_FILES = new ArrayList<>();
+    private static final List<File> SERVICE_KEYTAB_FILES = new ArrayList<>();
     private static final int NUM_USERS = 3;
     private static final Properties EMPTY_PROPERTIES = new Properties();
     private static final String BASE_URL = PhoenixRuntime.JDBC_PROTOCOL + ":localhost:2181";
@@ -90,6 +91,7 @@ public class SecureUserConnectionsTest {
                 + " attempts.", started);
 
         createUsers(NUM_USERS);
+        createServiceUsers(NUM_USERS);
 
         final Configuration conf = new Configuration(false);
         conf.set(CommonConfigurationKeys.HADOOP_SECURITY_AUTHENTICATION, "kerberos");
@@ -163,6 +165,16 @@ public class SecureUserConnectionsTest {
         }
     }
 
+    private static void createServiceUsers(int numUsers) throws Exception {
+        assertNotNull("KDC is null, was setup method called?", KDC);
+        for (int i = 1; i <= numUsers; i++) {
+            String principal = "user" + i + "/localhost";
+            File keytabFile = new File(KEYTAB_DIR, "user" + i + ".service.keytab");
+            KDC.createPrincipal(keytabFile, principal);
+            SERVICE_KEYTAB_FILES.add(keytabFile);
+        }
+    }
+
     /**
      * Returns the principal for a user.
      *
@@ -172,6 +184,10 @@ public class SecureUserConnectionsTest {
         return "user" + offset + "@" + KDC.getRealm();
     }
 
+    private static String getServicePrincipal(int offset) {
+        return "user" + offset + "/localhost@" + KDC.getRealm();
+    }
+
     /**
      * Returns the keytab file for the corresponding principal with the same {@code offset}.
      * Requires {@link #createUsers(int)} to have been called with a value greater than {@code offset}.
@@ -179,8 +195,16 @@ public class SecureUserConnectionsTest {
      * @param offset The "number" for the principal whose keytab should be returned. One-based, not zero-based.
      */
     public static File getUserKeytabFile(int offset) {
-        assertTrue("Invalid offset: " + offset, (offset - 1) >= 0 && (offset - 1) < USER_KEYTAB_FILES.size());
-        return USER_KEYTAB_FILES.get(offset - 1);
+        return getKeytabFile(offset, USER_KEYTAB_FILES);
+    }
+
+    public static File getServiceKeytabFile(int offset) {
+        return getKeytabFile(offset, SERVICE_KEYTAB_FILES);
+    }
+
+    private static File getKeytabFile(int offset, List<File> keytabs) {
+        assertTrue("Invalid offset: " + offset, (offset - 1) >= 0 && (offset - 1) < keytabs.size());
+        return keytabs.get(offset - 1);
     }
 
     private String joinUserAuthentication(String origUrl, String principal, File keytab) {
@@ -389,6 +413,44 @@ public class SecureUserConnectionsTest {
         verifyAllConnectionsAreKerberosBased(connections);
     }
 
+    @Test
+    public void testHostSubstitutionInUrl() throws Exception {
+        final HashSet<ConnectionInfo> connections = new HashSet<>();
+        final String princ1 = getServicePrincipal(1);
+        final File keytab1 = getServiceKeytabFile(1);
+        final String princ2 = getServicePrincipal(2);
+        final File keytab2 = getServiceKeytabFile(2);
+        final String url1 = joinUserAuthentication(BASE_URL, princ1, keytab1);
+        final String url2 = joinUserAuthentication(BASE_URL, princ2, keytab2);
+
+        // Using the same UGI should result in two equivalent ConnectionInfo objects
+        connections.add(ConnectionInfo.create(url1).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(1, connections.size());
+        // Sanity check
+        verifyAllConnectionsAreKerberosBased(connections);
+
+        // Logging in as the same user again should not duplicate connections
+        connections.add(ConnectionInfo.create(url1).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(1, connections.size());
+        // Sanity check
+        verifyAllConnectionsAreKerberosBased(connections);
+
+        // Add a second one.
+        connections.add(ConnectionInfo.create(url2).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(2, connections.size());
+        verifyAllConnectionsAreKerberosBased(connections);
+
+        // Again, verify this user is not duplicated
+        connections.add(ConnectionInfo.create(url2).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(2, connections.size());
+        verifyAllConnectionsAreKerberosBased(connections);
+
+        // Because the UGI instances are unique, so are the connections
+        connections.add(ConnectionInfo.create(url1).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(3, connections.size());
+        verifyAllConnectionsAreKerberosBased(connections);
+    }
+
     private void verifyAllConnectionsAreKerberosBased(Collection<ConnectionInfo> connections) {
         for (ConnectionInfo cnxnInfo : connections) {
             assertTrue("ConnectionInfo does not have kerberos credentials: " + cnxnInfo, cnxnInfo.getUser().getUGI().hasKerberosCredentials());


[7/7] phoenix git commit: PHOENIX-3684 Handle _HOST in principal specified in JDBC url

Posted by el...@apache.org.
PHOENIX-3684 Handle _HOST in principal specified in JDBC url

When the user provides a principal with the _HOST "magic string"
in it, Phoenix was incorrectly re-logging that user in. This results
in an application that creates a new HConnection and ZK connection
for every Phoenix connection which saturates connections to ZK.


Project: http://git-wip-us.apache.org/repos/asf/phoenix/repo
Commit: http://git-wip-us.apache.org/repos/asf/phoenix/commit/46c4cdd7
Tree: http://git-wip-us.apache.org/repos/asf/phoenix/tree/46c4cdd7
Diff: http://git-wip-us.apache.org/repos/asf/phoenix/diff/46c4cdd7

Branch: refs/heads/4.9-HBase-0.98
Commit: 46c4cdd70220d72f4c3db828665131d8573769b4
Parents: aee05ae
Author: Josh Elser <el...@apache.org>
Authored: Thu Feb 16 14:47:35 2017 -0500
Committer: Josh Elser <el...@apache.org>
Committed: Fri Feb 17 12:13:27 2017 -0500

----------------------------------------------------------------------
 .../phoenix/jdbc/PhoenixEmbeddedDriver.java     | 15 ++++-
 .../phoenix/jdbc/PhoenixEmbeddedDriverTest.java | 10 +++
 .../phoenix/jdbc/SecureUserConnectionsTest.java | 66 +++++++++++++++++++-
 3 files changed, 88 insertions(+), 3 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/phoenix/blob/46c4cdd7/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java b/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java
index 272fb22..2eca3d6 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java
@@ -350,7 +350,7 @@ public abstract class PhoenixEmbeddedDriver implements Driver, SQLCloseable {
                     try {
                         // Check if we need to authenticate with kerberos so that we cache the correct ConnectionInfo
                         UserGroupInformation currentUser = UserGroupInformation.getCurrentUser();
-                        if (!currentUser.hasKerberosCredentials() || !currentUser.getUserName().equals(principal)) {
+                        if (!currentUser.hasKerberosCredentials() || !isSameName(currentUser.getUserName(), principal)) {
                             synchronized (KERBEROS_LOGIN_LOCK) {
                                 // Double check the current user, might have changed since we checked last. Don't want
                                 // to re-login if it's the same user.
@@ -380,6 +380,19 @@ public abstract class PhoenixEmbeddedDriver implements Driver, SQLCloseable {
             return new ConnectionInfo(zookeeperQuorum, port, rootNode, principal, keytab);
         }
 
+        // Visible for testing
+        static boolean isSameName(String currentName, String newName) throws IOException {
+            return isSameName(currentName, newName, null);
+        }
+
+        static boolean isSameName(String currentName, String newName, String hostname) throws IOException {
+            // Make sure to replace "_HOST" if it exists before comparing the principals.
+            if (newName.contains(org.apache.hadoop.security.SecurityUtil.HOSTNAME_PATTERN)) {
+                newName = org.apache.hadoop.security.SecurityUtil.getServerPrincipal(newName, hostname);
+            }
+            return currentName.equals(newName);
+        }
+
         /**
          * Constructs a Configuration object to use when performing a Kerberos login.
          * @param props QueryServices properties

http://git-wip-us.apache.org/repos/asf/phoenix/blob/46c4cdd7/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java
index 98a028c..475307f 100644
--- a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java
+++ b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java
@@ -154,4 +154,14 @@ public class PhoenixEmbeddedDriverTest {
         assertTrue(driver.acceptsURL("jdbc:phoenix:localhost:123;untest=true;foo=bar"));
         DriverManager.deregisterDriver(driver);
     }
+
+    @Test
+    public void testPrincipalsMatching() throws Exception {
+        assertTrue(ConnectionInfo.isSameName("user@EXAMPLE.COM", "user@EXAMPLE.COM"));
+        assertTrue(ConnectionInfo.isSameName("user/localhost@EXAMPLE.COM", "user/localhost@EXAMPLE.COM"));
+        // the user provided name might have a _HOST in it, which should be replaced by the hostname
+        assertTrue(ConnectionInfo.isSameName("user/localhost@EXAMPLE.COM", "user/_HOST@EXAMPLE.COM", "localhost"));
+        assertFalse(ConnectionInfo.isSameName("user/foobar@EXAMPLE.COM", "user/_HOST@EXAMPLE.COM", "localhost"));
+        assertFalse(ConnectionInfo.isSameName("user@EXAMPLE.COM", "user/_HOST@EXAMPLE.COM", "localhost"));
+    }
 }

http://git-wip-us.apache.org/repos/asf/phoenix/blob/46c4cdd7/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java
index 4996f94..5a99b69 100644
--- a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java
+++ b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java
@@ -62,6 +62,7 @@ public class SecureUserConnectionsTest {
     private static final File KEYTAB_DIR = new File(TEMP_DIR, "keytabs");
     private static final File KDC_DIR = new File(TEMP_DIR, "kdc");
     private static final List<File> USER_KEYTAB_FILES = new ArrayList<>();
+    private static final List<File> SERVICE_KEYTAB_FILES = new ArrayList<>();
     private static final int NUM_USERS = 3;
     private static final Properties EMPTY_PROPERTIES = new Properties();
     private static final String BASE_URL = PhoenixRuntime.JDBC_PROTOCOL + ":localhost:2181";
@@ -90,6 +91,7 @@ public class SecureUserConnectionsTest {
                 + " attempts.", started);
 
         createUsers(NUM_USERS);
+        createServiceUsers(NUM_USERS);
 
         final Configuration conf = new Configuration(false);
         conf.set(CommonConfigurationKeys.HADOOP_SECURITY_AUTHENTICATION, "kerberos");
@@ -163,6 +165,16 @@ public class SecureUserConnectionsTest {
         }
     }
 
+    private static void createServiceUsers(int numUsers) throws Exception {
+        assertNotNull("KDC is null, was setup method called?", KDC);
+        for (int i = 1; i <= numUsers; i++) {
+            String principal = "user" + i + "/localhost";
+            File keytabFile = new File(KEYTAB_DIR, "user" + i + ".service.keytab");
+            KDC.createPrincipal(keytabFile, principal);
+            SERVICE_KEYTAB_FILES.add(keytabFile);
+        }
+    }
+
     /**
      * Returns the principal for a user.
      *
@@ -172,6 +184,10 @@ public class SecureUserConnectionsTest {
         return "user" + offset + "@" + KDC.getRealm();
     }
 
+    private static String getServicePrincipal(int offset) {
+        return "user" + offset + "/localhost@" + KDC.getRealm();
+    }
+
     /**
      * Returns the keytab file for the corresponding principal with the same {@code offset}.
      * Requires {@link #createUsers(int)} to have been called with a value greater than {@code offset}.
@@ -179,8 +195,16 @@ public class SecureUserConnectionsTest {
      * @param offset The "number" for the principal whose keytab should be returned. One-based, not zero-based.
      */
     public static File getUserKeytabFile(int offset) {
-        assertTrue("Invalid offset: " + offset, (offset - 1) >= 0 && (offset - 1) < USER_KEYTAB_FILES.size());
-        return USER_KEYTAB_FILES.get(offset - 1);
+        return getKeytabFile(offset, USER_KEYTAB_FILES);
+    }
+
+    public static File getServiceKeytabFile(int offset) {
+        return getKeytabFile(offset, SERVICE_KEYTAB_FILES);
+    }
+
+    private static File getKeytabFile(int offset, List<File> keytabs) {
+        assertTrue("Invalid offset: " + offset, (offset - 1) >= 0 && (offset - 1) < keytabs.size());
+        return keytabs.get(offset - 1);
     }
 
     private String joinUserAuthentication(String origUrl, String principal, File keytab) {
@@ -389,6 +413,44 @@ public class SecureUserConnectionsTest {
         verifyAllConnectionsAreKerberosBased(connections);
     }
 
+    @Test
+    public void testHostSubstitutionInUrl() throws Exception {
+        final HashSet<ConnectionInfo> connections = new HashSet<>();
+        final String princ1 = getServicePrincipal(1);
+        final File keytab1 = getServiceKeytabFile(1);
+        final String princ2 = getServicePrincipal(2);
+        final File keytab2 = getServiceKeytabFile(2);
+        final String url1 = joinUserAuthentication(BASE_URL, princ1, keytab1);
+        final String url2 = joinUserAuthentication(BASE_URL, princ2, keytab2);
+
+        // Using the same UGI should result in two equivalent ConnectionInfo objects
+        connections.add(ConnectionInfo.create(url1).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(1, connections.size());
+        // Sanity check
+        verifyAllConnectionsAreKerberosBased(connections);
+
+        // Logging in as the same user again should not duplicate connections
+        connections.add(ConnectionInfo.create(url1).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(1, connections.size());
+        // Sanity check
+        verifyAllConnectionsAreKerberosBased(connections);
+
+        // Add a second one.
+        connections.add(ConnectionInfo.create(url2).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(2, connections.size());
+        verifyAllConnectionsAreKerberosBased(connections);
+
+        // Again, verify this user is not duplicated
+        connections.add(ConnectionInfo.create(url2).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(2, connections.size());
+        verifyAllConnectionsAreKerberosBased(connections);
+
+        // Because the UGI instances are unique, so are the connections
+        connections.add(ConnectionInfo.create(url1).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(3, connections.size());
+        verifyAllConnectionsAreKerberosBased(connections);
+    }
+
     private void verifyAllConnectionsAreKerberosBased(Collection<ConnectionInfo> connections) {
         for (ConnectionInfo cnxnInfo : connections) {
             assertTrue("ConnectionInfo does not have kerberos credentials: " + cnxnInfo, cnxnInfo.getUser().getUGI().hasKerberosCredentials());


[2/7] phoenix git commit: PHOENIX-3684 Handle _HOST in principal specified in JDBC url

Posted by el...@apache.org.
PHOENIX-3684 Handle _HOST in principal specified in JDBC url

When the user provides a principal with the _HOST "magic string"
in it, Phoenix was incorrectly re-logging that user in. This results
in an application that creates a new HConnection and ZK connection
for every Phoenix connection which saturates connections to ZK.


Project: http://git-wip-us.apache.org/repos/asf/phoenix/repo
Commit: http://git-wip-us.apache.org/repos/asf/phoenix/commit/0099ec5e
Tree: http://git-wip-us.apache.org/repos/asf/phoenix/tree/0099ec5e
Diff: http://git-wip-us.apache.org/repos/asf/phoenix/diff/0099ec5e

Branch: refs/heads/4.x-HBase-1.3
Commit: 0099ec5e17d79dcfa1636f41acfc2af83b0f8553
Parents: d96a755
Author: Josh Elser <el...@apache.org>
Authored: Thu Feb 16 14:47:35 2017 -0500
Committer: Josh Elser <el...@apache.org>
Committed: Fri Feb 17 11:43:31 2017 -0500

----------------------------------------------------------------------
 .../phoenix/jdbc/PhoenixEmbeddedDriver.java     | 15 ++++-
 .../phoenix/jdbc/PhoenixEmbeddedDriverTest.java | 10 +++
 .../phoenix/jdbc/SecureUserConnectionsTest.java | 66 +++++++++++++++++++-
 3 files changed, 88 insertions(+), 3 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/phoenix/blob/0099ec5e/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java b/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java
index d41826f..2a496c6 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java
@@ -372,7 +372,7 @@ public abstract class PhoenixEmbeddedDriver implements Driver, SQLCloseable {
                     try {
                         // Check if we need to authenticate with kerberos so that we cache the correct ConnectionInfo
                         UserGroupInformation currentUser = UserGroupInformation.getCurrentUser();
-                        if (!currentUser.hasKerberosCredentials() || !currentUser.getUserName().equals(principal)) {
+                        if (!currentUser.hasKerberosCredentials() || !isSameName(currentUser.getUserName(), principal)) {
                             synchronized (KERBEROS_LOGIN_LOCK) {
                                 // Double check the current user, might have changed since we checked last. Don't want
                                 // to re-login if it's the same user.
@@ -402,6 +402,19 @@ public abstract class PhoenixEmbeddedDriver implements Driver, SQLCloseable {
             return new ConnectionInfo(zookeeperQuorum, port, rootNode, principal, keytab);
         }
 
+        // Visible for testing
+        static boolean isSameName(String currentName, String newName) throws IOException {
+            return isSameName(currentName, newName, null);
+        }
+
+        static boolean isSameName(String currentName, String newName, String hostname) throws IOException {
+            // Make sure to replace "_HOST" if it exists before comparing the principals.
+            if (newName.contains(org.apache.hadoop.security.SecurityUtil.HOSTNAME_PATTERN)) {
+                newName = org.apache.hadoop.security.SecurityUtil.getServerPrincipal(newName, hostname);
+            }
+            return currentName.equals(newName);
+        }
+
         /**
          * Constructs a Configuration object to use when performing a Kerberos login.
          * @param props QueryServices properties

http://git-wip-us.apache.org/repos/asf/phoenix/blob/0099ec5e/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java
index 9d7fbfc..59465e8 100644
--- a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java
+++ b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java
@@ -158,4 +158,14 @@ public class PhoenixEmbeddedDriverTest {
         assertTrue(driver.acceptsURL("jdbc:phoenix:localhost:123;untest=true;foo=bar"));
         DriverManager.deregisterDriver(driver);
     }
+
+    @Test
+    public void testPrincipalsMatching() throws Exception {
+        assertTrue(ConnectionInfo.isSameName("user@EXAMPLE.COM", "user@EXAMPLE.COM"));
+        assertTrue(ConnectionInfo.isSameName("user/localhost@EXAMPLE.COM", "user/localhost@EXAMPLE.COM"));
+        // the user provided name might have a _HOST in it, which should be replaced by the hostname
+        assertTrue(ConnectionInfo.isSameName("user/localhost@EXAMPLE.COM", "user/_HOST@EXAMPLE.COM", "localhost"));
+        assertFalse(ConnectionInfo.isSameName("user/foobar@EXAMPLE.COM", "user/_HOST@EXAMPLE.COM", "localhost"));
+        assertFalse(ConnectionInfo.isSameName("user@EXAMPLE.COM", "user/_HOST@EXAMPLE.COM", "localhost"));
+    }
 }

http://git-wip-us.apache.org/repos/asf/phoenix/blob/0099ec5e/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java
index 4996f94..5a99b69 100644
--- a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java
+++ b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java
@@ -62,6 +62,7 @@ public class SecureUserConnectionsTest {
     private static final File KEYTAB_DIR = new File(TEMP_DIR, "keytabs");
     private static final File KDC_DIR = new File(TEMP_DIR, "kdc");
     private static final List<File> USER_KEYTAB_FILES = new ArrayList<>();
+    private static final List<File> SERVICE_KEYTAB_FILES = new ArrayList<>();
     private static final int NUM_USERS = 3;
     private static final Properties EMPTY_PROPERTIES = new Properties();
     private static final String BASE_URL = PhoenixRuntime.JDBC_PROTOCOL + ":localhost:2181";
@@ -90,6 +91,7 @@ public class SecureUserConnectionsTest {
                 + " attempts.", started);
 
         createUsers(NUM_USERS);
+        createServiceUsers(NUM_USERS);
 
         final Configuration conf = new Configuration(false);
         conf.set(CommonConfigurationKeys.HADOOP_SECURITY_AUTHENTICATION, "kerberos");
@@ -163,6 +165,16 @@ public class SecureUserConnectionsTest {
         }
     }
 
+    private static void createServiceUsers(int numUsers) throws Exception {
+        assertNotNull("KDC is null, was setup method called?", KDC);
+        for (int i = 1; i <= numUsers; i++) {
+            String principal = "user" + i + "/localhost";
+            File keytabFile = new File(KEYTAB_DIR, "user" + i + ".service.keytab");
+            KDC.createPrincipal(keytabFile, principal);
+            SERVICE_KEYTAB_FILES.add(keytabFile);
+        }
+    }
+
     /**
      * Returns the principal for a user.
      *
@@ -172,6 +184,10 @@ public class SecureUserConnectionsTest {
         return "user" + offset + "@" + KDC.getRealm();
     }
 
+    private static String getServicePrincipal(int offset) {
+        return "user" + offset + "/localhost@" + KDC.getRealm();
+    }
+
     /**
      * Returns the keytab file for the corresponding principal with the same {@code offset}.
      * Requires {@link #createUsers(int)} to have been called with a value greater than {@code offset}.
@@ -179,8 +195,16 @@ public class SecureUserConnectionsTest {
      * @param offset The "number" for the principal whose keytab should be returned. One-based, not zero-based.
      */
     public static File getUserKeytabFile(int offset) {
-        assertTrue("Invalid offset: " + offset, (offset - 1) >= 0 && (offset - 1) < USER_KEYTAB_FILES.size());
-        return USER_KEYTAB_FILES.get(offset - 1);
+        return getKeytabFile(offset, USER_KEYTAB_FILES);
+    }
+
+    public static File getServiceKeytabFile(int offset) {
+        return getKeytabFile(offset, SERVICE_KEYTAB_FILES);
+    }
+
+    private static File getKeytabFile(int offset, List<File> keytabs) {
+        assertTrue("Invalid offset: " + offset, (offset - 1) >= 0 && (offset - 1) < keytabs.size());
+        return keytabs.get(offset - 1);
     }
 
     private String joinUserAuthentication(String origUrl, String principal, File keytab) {
@@ -389,6 +413,44 @@ public class SecureUserConnectionsTest {
         verifyAllConnectionsAreKerberosBased(connections);
     }
 
+    @Test
+    public void testHostSubstitutionInUrl() throws Exception {
+        final HashSet<ConnectionInfo> connections = new HashSet<>();
+        final String princ1 = getServicePrincipal(1);
+        final File keytab1 = getServiceKeytabFile(1);
+        final String princ2 = getServicePrincipal(2);
+        final File keytab2 = getServiceKeytabFile(2);
+        final String url1 = joinUserAuthentication(BASE_URL, princ1, keytab1);
+        final String url2 = joinUserAuthentication(BASE_URL, princ2, keytab2);
+
+        // Using the same UGI should result in two equivalent ConnectionInfo objects
+        connections.add(ConnectionInfo.create(url1).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(1, connections.size());
+        // Sanity check
+        verifyAllConnectionsAreKerberosBased(connections);
+
+        // Logging in as the same user again should not duplicate connections
+        connections.add(ConnectionInfo.create(url1).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(1, connections.size());
+        // Sanity check
+        verifyAllConnectionsAreKerberosBased(connections);
+
+        // Add a second one.
+        connections.add(ConnectionInfo.create(url2).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(2, connections.size());
+        verifyAllConnectionsAreKerberosBased(connections);
+
+        // Again, verify this user is not duplicated
+        connections.add(ConnectionInfo.create(url2).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(2, connections.size());
+        verifyAllConnectionsAreKerberosBased(connections);
+
+        // Because the UGI instances are unique, so are the connections
+        connections.add(ConnectionInfo.create(url1).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(3, connections.size());
+        verifyAllConnectionsAreKerberosBased(connections);
+    }
+
     private void verifyAllConnectionsAreKerberosBased(Collection<ConnectionInfo> connections) {
         for (ConnectionInfo cnxnInfo : connections) {
             assertTrue("ConnectionInfo does not have kerberos credentials: " + cnxnInfo, cnxnInfo.getUser().getUGI().hasKerberosCredentials());


[4/7] phoenix git commit: PHOENIX-3684 Handle _HOST in principal specified in JDBC url

Posted by el...@apache.org.
PHOENIX-3684 Handle _HOST in principal specified in JDBC url

When the user provides a principal with the _HOST "magic string"
in it, Phoenix was incorrectly re-logging that user in. This results
in an application that creates a new HConnection and ZK connection
for every Phoenix connection which saturates connections to ZK.


Project: http://git-wip-us.apache.org/repos/asf/phoenix/repo
Commit: http://git-wip-us.apache.org/repos/asf/phoenix/commit/54ae6f3a
Tree: http://git-wip-us.apache.org/repos/asf/phoenix/tree/54ae6f3a
Diff: http://git-wip-us.apache.org/repos/asf/phoenix/diff/54ae6f3a

Branch: refs/heads/4.x-HBase-0.98
Commit: 54ae6f3accafcabb9a5e7623f0cc58abd6822abb
Parents: 4728a89
Author: Josh Elser <el...@apache.org>
Authored: Thu Feb 16 14:47:35 2017 -0500
Committer: Josh Elser <el...@apache.org>
Committed: Fri Feb 17 11:54:41 2017 -0500

----------------------------------------------------------------------
 .../phoenix/jdbc/PhoenixEmbeddedDriver.java     | 15 ++++-
 .../phoenix/jdbc/PhoenixEmbeddedDriverTest.java | 10 +++
 .../phoenix/jdbc/SecureUserConnectionsTest.java | 66 +++++++++++++++++++-
 3 files changed, 88 insertions(+), 3 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/phoenix/blob/54ae6f3a/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java b/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java
index d41826f..2a496c6 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java
@@ -372,7 +372,7 @@ public abstract class PhoenixEmbeddedDriver implements Driver, SQLCloseable {
                     try {
                         // Check if we need to authenticate with kerberos so that we cache the correct ConnectionInfo
                         UserGroupInformation currentUser = UserGroupInformation.getCurrentUser();
-                        if (!currentUser.hasKerberosCredentials() || !currentUser.getUserName().equals(principal)) {
+                        if (!currentUser.hasKerberosCredentials() || !isSameName(currentUser.getUserName(), principal)) {
                             synchronized (KERBEROS_LOGIN_LOCK) {
                                 // Double check the current user, might have changed since we checked last. Don't want
                                 // to re-login if it's the same user.
@@ -402,6 +402,19 @@ public abstract class PhoenixEmbeddedDriver implements Driver, SQLCloseable {
             return new ConnectionInfo(zookeeperQuorum, port, rootNode, principal, keytab);
         }
 
+        // Visible for testing
+        static boolean isSameName(String currentName, String newName) throws IOException {
+            return isSameName(currentName, newName, null);
+        }
+
+        static boolean isSameName(String currentName, String newName, String hostname) throws IOException {
+            // Make sure to replace "_HOST" if it exists before comparing the principals.
+            if (newName.contains(org.apache.hadoop.security.SecurityUtil.HOSTNAME_PATTERN)) {
+                newName = org.apache.hadoop.security.SecurityUtil.getServerPrincipal(newName, hostname);
+            }
+            return currentName.equals(newName);
+        }
+
         /**
          * Constructs a Configuration object to use when performing a Kerberos login.
          * @param props QueryServices properties

http://git-wip-us.apache.org/repos/asf/phoenix/blob/54ae6f3a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java
index 9d7fbfc..59465e8 100644
--- a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java
+++ b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java
@@ -158,4 +158,14 @@ public class PhoenixEmbeddedDriverTest {
         assertTrue(driver.acceptsURL("jdbc:phoenix:localhost:123;untest=true;foo=bar"));
         DriverManager.deregisterDriver(driver);
     }
+
+    @Test
+    public void testPrincipalsMatching() throws Exception {
+        assertTrue(ConnectionInfo.isSameName("user@EXAMPLE.COM", "user@EXAMPLE.COM"));
+        assertTrue(ConnectionInfo.isSameName("user/localhost@EXAMPLE.COM", "user/localhost@EXAMPLE.COM"));
+        // the user provided name might have a _HOST in it, which should be replaced by the hostname
+        assertTrue(ConnectionInfo.isSameName("user/localhost@EXAMPLE.COM", "user/_HOST@EXAMPLE.COM", "localhost"));
+        assertFalse(ConnectionInfo.isSameName("user/foobar@EXAMPLE.COM", "user/_HOST@EXAMPLE.COM", "localhost"));
+        assertFalse(ConnectionInfo.isSameName("user@EXAMPLE.COM", "user/_HOST@EXAMPLE.COM", "localhost"));
+    }
 }

http://git-wip-us.apache.org/repos/asf/phoenix/blob/54ae6f3a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java
index 4996f94..5a99b69 100644
--- a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java
+++ b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java
@@ -62,6 +62,7 @@ public class SecureUserConnectionsTest {
     private static final File KEYTAB_DIR = new File(TEMP_DIR, "keytabs");
     private static final File KDC_DIR = new File(TEMP_DIR, "kdc");
     private static final List<File> USER_KEYTAB_FILES = new ArrayList<>();
+    private static final List<File> SERVICE_KEYTAB_FILES = new ArrayList<>();
     private static final int NUM_USERS = 3;
     private static final Properties EMPTY_PROPERTIES = new Properties();
     private static final String BASE_URL = PhoenixRuntime.JDBC_PROTOCOL + ":localhost:2181";
@@ -90,6 +91,7 @@ public class SecureUserConnectionsTest {
                 + " attempts.", started);
 
         createUsers(NUM_USERS);
+        createServiceUsers(NUM_USERS);
 
         final Configuration conf = new Configuration(false);
         conf.set(CommonConfigurationKeys.HADOOP_SECURITY_AUTHENTICATION, "kerberos");
@@ -163,6 +165,16 @@ public class SecureUserConnectionsTest {
         }
     }
 
+    private static void createServiceUsers(int numUsers) throws Exception {
+        assertNotNull("KDC is null, was setup method called?", KDC);
+        for (int i = 1; i <= numUsers; i++) {
+            String principal = "user" + i + "/localhost";
+            File keytabFile = new File(KEYTAB_DIR, "user" + i + ".service.keytab");
+            KDC.createPrincipal(keytabFile, principal);
+            SERVICE_KEYTAB_FILES.add(keytabFile);
+        }
+    }
+
     /**
      * Returns the principal for a user.
      *
@@ -172,6 +184,10 @@ public class SecureUserConnectionsTest {
         return "user" + offset + "@" + KDC.getRealm();
     }
 
+    private static String getServicePrincipal(int offset) {
+        return "user" + offset + "/localhost@" + KDC.getRealm();
+    }
+
     /**
      * Returns the keytab file for the corresponding principal with the same {@code offset}.
      * Requires {@link #createUsers(int)} to have been called with a value greater than {@code offset}.
@@ -179,8 +195,16 @@ public class SecureUserConnectionsTest {
      * @param offset The "number" for the principal whose keytab should be returned. One-based, not zero-based.
      */
     public static File getUserKeytabFile(int offset) {
-        assertTrue("Invalid offset: " + offset, (offset - 1) >= 0 && (offset - 1) < USER_KEYTAB_FILES.size());
-        return USER_KEYTAB_FILES.get(offset - 1);
+        return getKeytabFile(offset, USER_KEYTAB_FILES);
+    }
+
+    public static File getServiceKeytabFile(int offset) {
+        return getKeytabFile(offset, SERVICE_KEYTAB_FILES);
+    }
+
+    private static File getKeytabFile(int offset, List<File> keytabs) {
+        assertTrue("Invalid offset: " + offset, (offset - 1) >= 0 && (offset - 1) < keytabs.size());
+        return keytabs.get(offset - 1);
     }
 
     private String joinUserAuthentication(String origUrl, String principal, File keytab) {
@@ -389,6 +413,44 @@ public class SecureUserConnectionsTest {
         verifyAllConnectionsAreKerberosBased(connections);
     }
 
+    @Test
+    public void testHostSubstitutionInUrl() throws Exception {
+        final HashSet<ConnectionInfo> connections = new HashSet<>();
+        final String princ1 = getServicePrincipal(1);
+        final File keytab1 = getServiceKeytabFile(1);
+        final String princ2 = getServicePrincipal(2);
+        final File keytab2 = getServiceKeytabFile(2);
+        final String url1 = joinUserAuthentication(BASE_URL, princ1, keytab1);
+        final String url2 = joinUserAuthentication(BASE_URL, princ2, keytab2);
+
+        // Using the same UGI should result in two equivalent ConnectionInfo objects
+        connections.add(ConnectionInfo.create(url1).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(1, connections.size());
+        // Sanity check
+        verifyAllConnectionsAreKerberosBased(connections);
+
+        // Logging in as the same user again should not duplicate connections
+        connections.add(ConnectionInfo.create(url1).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(1, connections.size());
+        // Sanity check
+        verifyAllConnectionsAreKerberosBased(connections);
+
+        // Add a second one.
+        connections.add(ConnectionInfo.create(url2).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(2, connections.size());
+        verifyAllConnectionsAreKerberosBased(connections);
+
+        // Again, verify this user is not duplicated
+        connections.add(ConnectionInfo.create(url2).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(2, connections.size());
+        verifyAllConnectionsAreKerberosBased(connections);
+
+        // Because the UGI instances are unique, so are the connections
+        connections.add(ConnectionInfo.create(url1).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(3, connections.size());
+        verifyAllConnectionsAreKerberosBased(connections);
+    }
+
     private void verifyAllConnectionsAreKerberosBased(Collection<ConnectionInfo> connections) {
         for (ConnectionInfo cnxnInfo : connections) {
             assertTrue("ConnectionInfo does not have kerberos credentials: " + cnxnInfo, cnxnInfo.getUser().getUGI().hasKerberosCredentials());


[5/7] phoenix git commit: PHOENIX-3684 Handle _HOST in principal specified in JDBC url

Posted by el...@apache.org.
PHOENIX-3684 Handle _HOST in principal specified in JDBC url

When the user provides a principal with the _HOST "magic string"
in it, Phoenix was incorrectly re-logging that user in. This results
in an application that creates a new HConnection and ZK connection
for every Phoenix connection which saturates connections to ZK.


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

Branch: refs/heads/4.9-HBase-1.2
Commit: 4855511513ed864b6c6866ce2ca592a412da0c46
Parents: a967db5
Author: Josh Elser <el...@apache.org>
Authored: Thu Feb 16 14:47:35 2017 -0500
Committer: Josh Elser <el...@apache.org>
Committed: Fri Feb 17 12:00:45 2017 -0500

----------------------------------------------------------------------
 .../phoenix/jdbc/PhoenixEmbeddedDriver.java     | 15 ++++-
 .../phoenix/jdbc/PhoenixEmbeddedDriverTest.java | 10 +++
 .../phoenix/jdbc/SecureUserConnectionsTest.java | 66 +++++++++++++++++++-
 3 files changed, 88 insertions(+), 3 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/phoenix/blob/48555115/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java b/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java
index 272fb22..2eca3d6 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java
@@ -350,7 +350,7 @@ public abstract class PhoenixEmbeddedDriver implements Driver, SQLCloseable {
                     try {
                         // Check if we need to authenticate with kerberos so that we cache the correct ConnectionInfo
                         UserGroupInformation currentUser = UserGroupInformation.getCurrentUser();
-                        if (!currentUser.hasKerberosCredentials() || !currentUser.getUserName().equals(principal)) {
+                        if (!currentUser.hasKerberosCredentials() || !isSameName(currentUser.getUserName(), principal)) {
                             synchronized (KERBEROS_LOGIN_LOCK) {
                                 // Double check the current user, might have changed since we checked last. Don't want
                                 // to re-login if it's the same user.
@@ -380,6 +380,19 @@ public abstract class PhoenixEmbeddedDriver implements Driver, SQLCloseable {
             return new ConnectionInfo(zookeeperQuorum, port, rootNode, principal, keytab);
         }
 
+        // Visible for testing
+        static boolean isSameName(String currentName, String newName) throws IOException {
+            return isSameName(currentName, newName, null);
+        }
+
+        static boolean isSameName(String currentName, String newName, String hostname) throws IOException {
+            // Make sure to replace "_HOST" if it exists before comparing the principals.
+            if (newName.contains(org.apache.hadoop.security.SecurityUtil.HOSTNAME_PATTERN)) {
+                newName = org.apache.hadoop.security.SecurityUtil.getServerPrincipal(newName, hostname);
+            }
+            return currentName.equals(newName);
+        }
+
         /**
          * Constructs a Configuration object to use when performing a Kerberos login.
          * @param props QueryServices properties

http://git-wip-us.apache.org/repos/asf/phoenix/blob/48555115/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java
index 98a028c..475307f 100644
--- a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java
+++ b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriverTest.java
@@ -154,4 +154,14 @@ public class PhoenixEmbeddedDriverTest {
         assertTrue(driver.acceptsURL("jdbc:phoenix:localhost:123;untest=true;foo=bar"));
         DriverManager.deregisterDriver(driver);
     }
+
+    @Test
+    public void testPrincipalsMatching() throws Exception {
+        assertTrue(ConnectionInfo.isSameName("user@EXAMPLE.COM", "user@EXAMPLE.COM"));
+        assertTrue(ConnectionInfo.isSameName("user/localhost@EXAMPLE.COM", "user/localhost@EXAMPLE.COM"));
+        // the user provided name might have a _HOST in it, which should be replaced by the hostname
+        assertTrue(ConnectionInfo.isSameName("user/localhost@EXAMPLE.COM", "user/_HOST@EXAMPLE.COM", "localhost"));
+        assertFalse(ConnectionInfo.isSameName("user/foobar@EXAMPLE.COM", "user/_HOST@EXAMPLE.COM", "localhost"));
+        assertFalse(ConnectionInfo.isSameName("user@EXAMPLE.COM", "user/_HOST@EXAMPLE.COM", "localhost"));
+    }
 }

http://git-wip-us.apache.org/repos/asf/phoenix/blob/48555115/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java
index 4996f94..5a99b69 100644
--- a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java
+++ b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/SecureUserConnectionsTest.java
@@ -62,6 +62,7 @@ public class SecureUserConnectionsTest {
     private static final File KEYTAB_DIR = new File(TEMP_DIR, "keytabs");
     private static final File KDC_DIR = new File(TEMP_DIR, "kdc");
     private static final List<File> USER_KEYTAB_FILES = new ArrayList<>();
+    private static final List<File> SERVICE_KEYTAB_FILES = new ArrayList<>();
     private static final int NUM_USERS = 3;
     private static final Properties EMPTY_PROPERTIES = new Properties();
     private static final String BASE_URL = PhoenixRuntime.JDBC_PROTOCOL + ":localhost:2181";
@@ -90,6 +91,7 @@ public class SecureUserConnectionsTest {
                 + " attempts.", started);
 
         createUsers(NUM_USERS);
+        createServiceUsers(NUM_USERS);
 
         final Configuration conf = new Configuration(false);
         conf.set(CommonConfigurationKeys.HADOOP_SECURITY_AUTHENTICATION, "kerberos");
@@ -163,6 +165,16 @@ public class SecureUserConnectionsTest {
         }
     }
 
+    private static void createServiceUsers(int numUsers) throws Exception {
+        assertNotNull("KDC is null, was setup method called?", KDC);
+        for (int i = 1; i <= numUsers; i++) {
+            String principal = "user" + i + "/localhost";
+            File keytabFile = new File(KEYTAB_DIR, "user" + i + ".service.keytab");
+            KDC.createPrincipal(keytabFile, principal);
+            SERVICE_KEYTAB_FILES.add(keytabFile);
+        }
+    }
+
     /**
      * Returns the principal for a user.
      *
@@ -172,6 +184,10 @@ public class SecureUserConnectionsTest {
         return "user" + offset + "@" + KDC.getRealm();
     }
 
+    private static String getServicePrincipal(int offset) {
+        return "user" + offset + "/localhost@" + KDC.getRealm();
+    }
+
     /**
      * Returns the keytab file for the corresponding principal with the same {@code offset}.
      * Requires {@link #createUsers(int)} to have been called with a value greater than {@code offset}.
@@ -179,8 +195,16 @@ public class SecureUserConnectionsTest {
      * @param offset The "number" for the principal whose keytab should be returned. One-based, not zero-based.
      */
     public static File getUserKeytabFile(int offset) {
-        assertTrue("Invalid offset: " + offset, (offset - 1) >= 0 && (offset - 1) < USER_KEYTAB_FILES.size());
-        return USER_KEYTAB_FILES.get(offset - 1);
+        return getKeytabFile(offset, USER_KEYTAB_FILES);
+    }
+
+    public static File getServiceKeytabFile(int offset) {
+        return getKeytabFile(offset, SERVICE_KEYTAB_FILES);
+    }
+
+    private static File getKeytabFile(int offset, List<File> keytabs) {
+        assertTrue("Invalid offset: " + offset, (offset - 1) >= 0 && (offset - 1) < keytabs.size());
+        return keytabs.get(offset - 1);
     }
 
     private String joinUserAuthentication(String origUrl, String principal, File keytab) {
@@ -389,6 +413,44 @@ public class SecureUserConnectionsTest {
         verifyAllConnectionsAreKerberosBased(connections);
     }
 
+    @Test
+    public void testHostSubstitutionInUrl() throws Exception {
+        final HashSet<ConnectionInfo> connections = new HashSet<>();
+        final String princ1 = getServicePrincipal(1);
+        final File keytab1 = getServiceKeytabFile(1);
+        final String princ2 = getServicePrincipal(2);
+        final File keytab2 = getServiceKeytabFile(2);
+        final String url1 = joinUserAuthentication(BASE_URL, princ1, keytab1);
+        final String url2 = joinUserAuthentication(BASE_URL, princ2, keytab2);
+
+        // Using the same UGI should result in two equivalent ConnectionInfo objects
+        connections.add(ConnectionInfo.create(url1).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(1, connections.size());
+        // Sanity check
+        verifyAllConnectionsAreKerberosBased(connections);
+
+        // Logging in as the same user again should not duplicate connections
+        connections.add(ConnectionInfo.create(url1).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(1, connections.size());
+        // Sanity check
+        verifyAllConnectionsAreKerberosBased(connections);
+
+        // Add a second one.
+        connections.add(ConnectionInfo.create(url2).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(2, connections.size());
+        verifyAllConnectionsAreKerberosBased(connections);
+
+        // Again, verify this user is not duplicated
+        connections.add(ConnectionInfo.create(url2).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(2, connections.size());
+        verifyAllConnectionsAreKerberosBased(connections);
+
+        // Because the UGI instances are unique, so are the connections
+        connections.add(ConnectionInfo.create(url1).normalize(ReadOnlyProps.EMPTY_PROPS, EMPTY_PROPERTIES));
+        assertEquals(3, connections.size());
+        verifyAllConnectionsAreKerberosBased(connections);
+    }
+
     private void verifyAllConnectionsAreKerberosBased(Collection<ConnectionInfo> connections) {
         for (ConnectionInfo cnxnInfo : connections) {
             assertTrue("ConnectionInfo does not have kerberos credentials: " + cnxnInfo, cnxnInfo.getUser().getUGI().hasKerberosCredentials());