From fd666f6ee0087a6a8dec78673f319217464fd820 Mon Sep 17 00:00:00 2001 From: Josh Elser Date: Mon, 26 Jun 2017 15:00:29 -0400 Subject: [PATCH 1/2] ACCUMULO-4665 Use UGI with real Kerberos credentials UGI supports the notion of users without credentials being "proxied" (riding on top of) another user which does have credentials. This is authorized via configuration. These changes allow this scenario more naturally and remove unnecessarily strict assertions in KerberosToken. --- .../client/security/tokens/KerberosToken.java | 19 ++--- .../apache/accumulo/core/rpc/ThriftUtil.java | 23 +++++- .../test/functional/KerberosProxyIT.java | 70 ++++++++++++++++++- 3 files changed, 95 insertions(+), 17 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java b/core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java index 284a8386829..5bcab1a9518 100644 --- a/core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java +++ b/core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java @@ -29,6 +29,7 @@ import javax.security.auth.DestroyFailedException; import org.apache.hadoop.security.UserGroupInformation; +import org.apache.hadoop.security.UserGroupInformation.AuthenticationMethod; /** * Authentication token for Kerberos authenticated clients @@ -51,11 +52,11 @@ public class KerberosToken implements AuthenticationToken { * The user that is logged in */ public KerberosToken(String principal) throws IOException { - requireNonNull(principal); + this.principal = requireNonNull(principal); final UserGroupInformation ugi = UserGroupInformation.getCurrentUser(); - checkArgument(ugi.hasKerberosCredentials(), "Subject is not logged in via Kerberos"); - checkArgument(principal.equals(ugi.getUserName()), "Provided principal does not match currently logged-in user"); - this.principal = ugi.getUserName(); + if (AuthenticationMethod.KERBEROS == ugi.getAuthenticationMethod()) { + checkArgument(ugi.hasKerberosCredentials(), "Subject is not logged in via Kerberos"); + } } /** @@ -70,18 +71,12 @@ public KerberosToken(String principal) throws IOException { * Should the current Hadoop user be replaced with this user */ public KerberosToken(String principal, File keytab, boolean replaceCurrentUser) throws IOException { - requireNonNull(principal, "Principal was null"); - requireNonNull(keytab, "Keytab was null"); + this.principal = requireNonNull(principal, "Principal was null"); + this.keytab = requireNonNull(keytab, "Keytab was null"); checkArgument(keytab.exists() && keytab.isFile(), "Keytab was not a normal file"); - UserGroupInformation ugi; if (replaceCurrentUser) { UserGroupInformation.loginUserFromKeytab(principal, keytab.getAbsolutePath()); - ugi = UserGroupInformation.getCurrentUser(); - } else { - ugi = UserGroupInformation.loginUserFromKeytabAndReturnUGI(principal, keytab.getAbsolutePath()); } - this.principal = ugi.getUserName(); - this.keytab = keytab; } /** diff --git a/core/src/main/java/org/apache/accumulo/core/rpc/ThriftUtil.java b/core/src/main/java/org/apache/accumulo/core/rpc/ThriftUtil.java index deee3fed2bb..96248c6ba7d 100644 --- a/core/src/main/java/org/apache/accumulo/core/rpc/ThriftUtil.java +++ b/core/src/main/java/org/apache/accumulo/core/rpc/ThriftUtil.java @@ -35,6 +35,7 @@ import org.apache.accumulo.core.rpc.SaslConnectionParams.SaslMechanism; import org.apache.accumulo.core.tabletserver.thrift.TabletClientService; import org.apache.hadoop.security.UserGroupInformation; +import org.apache.hadoop.security.UserGroupInformation.AuthenticationMethod; import org.apache.thrift.TException; import org.apache.thrift.TServiceClient; import org.apache.thrift.TServiceClientFactory; @@ -282,13 +283,31 @@ public static TTransport createClientTransport(HostAndPort address, int timeout, try { // Log in via UGI, ensures we have logged in with our KRB credentials final UserGroupInformation currentUser = UserGroupInformation.getCurrentUser(); + final UserGroupInformation userForRpc; + if (AuthenticationMethod.PROXY == currentUser.getAuthenticationMethod()) { + // A "proxy" user is when the real (Kerberos) credentials are for a user + // other than the one we're acting as. When we make an RPC though, we need to make sure + // that the current user is the user that has some credentials. + if (currentUser.getRealUser() != null) { + userForRpc = currentUser.getRealUser(); + log.trace("{} is a proxy user, using real user instead {}", currentUser, userForRpc); + } else { + // The current user has no credentials, let it fail naturally at the RPC layer (no ticket) + // We know this won't work, but we can't do anything else + log.warn("The current user is a proxy user but there is no underlying real user (RPCs will fail): {}", currentUser); + userForRpc = currentUser; + } + } else { + // The normal case: the current user has its own ticket + userForRpc = currentUser; + } // Is this pricey enough that we want to cache it? final String hostname = InetAddress.getByName(address.getHostText()).getCanonicalHostName(); final SaslMechanism mechanism = saslParams.getMechanism(); - log.trace("Opening transport to server as {} to {}/{} using {}", currentUser, saslParams.getKerberosServerPrimary(), hostname, mechanism); + log.trace("Opening transport to server as {} to {}/{} using {}", userForRpc, saslParams.getKerberosServerPrimary(), hostname, mechanism); // Create the client SASL transport using the information for the server // Despite the 'protocol' argument seeming to be useless, it *must* be the primary of the server being connected to @@ -296,7 +315,7 @@ public static TTransport createClientTransport(HostAndPort address, int timeout, saslParams.getSaslProperties(), saslParams.getCallbackHandler(), transport); // Wrap it all in a processor which will run with a doAs the current user - transport = new UGIAssumingTransport(transport, currentUser); + transport = new UGIAssumingTransport(transport, userForRpc); // Open the transport transport.open(); diff --git a/test/src/test/java/org/apache/accumulo/test/functional/KerberosProxyIT.java b/test/src/test/java/org/apache/accumulo/test/functional/KerberosProxyIT.java index e03a1a8d88c..c57e5f1fd0e 100644 --- a/test/src/test/java/org/apache/accumulo/test/functional/KerberosProxyIT.java +++ b/test/src/test/java/org/apache/accumulo/test/functional/KerberosProxyIT.java @@ -18,6 +18,7 @@ import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import java.io.File; @@ -26,6 +27,7 @@ import java.net.ConnectException; import java.net.InetAddress; import java.nio.ByteBuffer; +import java.security.PrivilegedExceptionAction; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -33,10 +35,15 @@ import java.util.Properties; import org.apache.accumulo.cluster.ClusterUser; +import org.apache.accumulo.core.client.Connector; +import org.apache.accumulo.core.client.Scanner; +import org.apache.accumulo.core.client.ZooKeeperInstance; import org.apache.accumulo.core.client.security.tokens.KerberosToken; import org.apache.accumulo.core.client.security.tokens.PasswordToken; import org.apache.accumulo.core.conf.Property; import org.apache.accumulo.core.rpc.UGIAssumingTransport; +import org.apache.accumulo.core.security.Authorizations; +import org.apache.accumulo.core.security.TablePermission; import org.apache.accumulo.harness.AccumuloIT; import org.apache.accumulo.harness.MiniClusterConfigurationCallback; import org.apache.accumulo.harness.MiniClusterHarness; @@ -68,6 +75,7 @@ import org.hamcrest.TypeSafeMatcher; import org.junit.After; import org.junit.AfterClass; +import org.junit.Assert; import org.junit.Before; import org.junit.BeforeClass; import org.junit.Rule; @@ -77,12 +85,17 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import com.google.common.base.Throwables; +import com.google.common.collect.Iterables; + /** - * Tests impersonation of clients by the proxy over SASL + * Tests impersonation of clients over Kerberos+SASL. "Proxy" may be referring to the Accumulo Proxy service or it may be referring to the notion of a username + * overriding the real username of the actual credentials used in the system. Beware of the context of the word "proxy". */ @Category(MiniClusterOnlyTests.class) public class KerberosProxyIT extends AccumuloIT { private static final Logger log = LoggerFactory.getLogger(KerberosProxyIT.class); + private static final String PROXIED_USER = "proxied_user"; @Rule public ExpectedException thrown = ExpectedException.none(); @@ -141,8 +154,9 @@ public void startMac() throws Exception { public void configureMiniCluster(MiniAccumuloConfigImpl cfg, Configuration coreSite) { cfg.setNumTservers(1); Map siteCfg = cfg.getSiteConfig(); - // Allow the proxy to impersonate the client user, but no one else - siteCfg.put(Property.INSTANCE_RPC_SASL_ALLOWED_USER_IMPERSONATION.getKey(), proxyPrincipal + ":" + kdc.getRootUser().getPrincipal()); + // Allow the proxy to impersonate the "root" Accumulo user and our one special user. + siteCfg.put(Property.INSTANCE_RPC_SASL_ALLOWED_USER_IMPERSONATION.getKey(), + proxyPrincipal + ":" + kdc.getRootUser().getPrincipal() + "," + kdc.qualifyUser(PROXIED_USER)); siteCfg.put(Property.INSTANCE_RPC_SASL_ALLOWED_HOST_IMPERSONATION.getKey(), "*"); cfg.setSiteConfig(siteCfg); } @@ -463,6 +477,56 @@ public void testMismatchPrincipals() throws Exception { } } + @Test + public void proxiedUserAccessWithoutAccumuloProxy() throws Exception { + final String tableName = getUniqueNames(1)[0]; + ClusterUser rootUser = kdc.getRootUser(); + final UserGroupInformation rootUgi = UserGroupInformation.loginUserFromKeytabAndReturnUGI(rootUser.getPrincipal(), rootUser.getKeytab().getAbsolutePath()); + final UserGroupInformation realUgi = UserGroupInformation.loginUserFromKeytabAndReturnUGI(proxyPrincipal, proxyKeytab.getAbsolutePath()); + final String userWithoutCredentials = kdc.qualifyUser(PROXIED_USER); + final UserGroupInformation proxyUser = UserGroupInformation.createProxyUser(userWithoutCredentials, realUgi); + + // Create a table and user, grant permission to our user to read that table. + rootUgi.doAs(new PrivilegedExceptionAction() { + @Override + public Void run() throws Exception { + ZooKeeperInstance inst = new ZooKeeperInstance(mac.getClientConfig()); + Connector conn = inst.getConnector(rootUgi.getUserName(), new KerberosToken()); + conn.tableOperations().create(tableName); + conn.securityOperations().createLocalUser(userWithoutCredentials, new PasswordToken("ignored")); + conn.securityOperations().grantTablePermission(userWithoutCredentials, tableName, TablePermission.READ); + return null; + } + }); + realUgi.doAs(new PrivilegedExceptionAction() { + @Override + public Void run() throws Exception { + ZooKeeperInstance inst = new ZooKeeperInstance(mac.getClientConfig()); + Connector conn = inst.getConnector(proxyPrincipal, new KerberosToken()); + try { + Scanner s = conn.createScanner(tableName, Authorizations.EMPTY); + s.iterator().hasNext(); + Assert.fail("Expected to see an exception"); + } catch (RuntimeException e) { + int numSecurityExceptionsSeen = Iterables.size(Iterables.filter(Throwables.getCausalChain(e), + org.apache.accumulo.core.client.AccumuloSecurityException.class)); + assertTrue("Expected to see at least one AccumuloSecurityException, but saw: " + Throwables.getStackTraceAsString(e), numSecurityExceptionsSeen > 0); + } + return null; + } + }); + proxyUser.doAs(new PrivilegedExceptionAction() { + @Override + public Void run() throws Exception { + ZooKeeperInstance inst = new ZooKeeperInstance(mac.getClientConfig()); + Connector conn = inst.getConnector(userWithoutCredentials, new KerberosToken(userWithoutCredentials)); + Scanner s = conn.createScanner(tableName, Authorizations.EMPTY); + assertFalse(s.iterator().hasNext()); + return null; + } + }); + } + private static class ThriftExceptionMatchesPattern extends TypeSafeMatcher { private String pattern; From 4c9dc0269cae5e4f3062f82dbfc32048f8162d1a Mon Sep 17 00:00:00 2001 From: Josh Elser Date: Mon, 26 Jun 2017 17:19:36 -0400 Subject: [PATCH 2/2] Address review comments from Keith: * Fix up warning message to be less aggressive * Expand on the test case for proxied users on another's kerberos credentials --- .../apache/accumulo/core/rpc/ThriftUtil.java | 2 +- .../test/functional/KerberosProxyIT.java | 55 ++++++++++++++++--- 2 files changed, 48 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/core/rpc/ThriftUtil.java b/core/src/main/java/org/apache/accumulo/core/rpc/ThriftUtil.java index 96248c6ba7d..97d0735fbb2 100644 --- a/core/src/main/java/org/apache/accumulo/core/rpc/ThriftUtil.java +++ b/core/src/main/java/org/apache/accumulo/core/rpc/ThriftUtil.java @@ -294,7 +294,7 @@ public static TTransport createClientTransport(HostAndPort address, int timeout, } else { // The current user has no credentials, let it fail naturally at the RPC layer (no ticket) // We know this won't work, but we can't do anything else - log.warn("The current user is a proxy user but there is no underlying real user (RPCs will fail): {}", currentUser); + log.warn("The current user is a proxy user but there is no underlying real user (likely that RPCs will fail): {}", currentUser); userForRpc = currentUser; } } else { diff --git a/test/src/test/java/org/apache/accumulo/test/functional/KerberosProxyIT.java b/test/src/test/java/org/apache/accumulo/test/functional/KerberosProxyIT.java index c57e5f1fd0e..94491d4cd92 100644 --- a/test/src/test/java/org/apache/accumulo/test/functional/KerberosProxyIT.java +++ b/test/src/test/java/org/apache/accumulo/test/functional/KerberosProxyIT.java @@ -95,7 +95,7 @@ @Category(MiniClusterOnlyTests.class) public class KerberosProxyIT extends AccumuloIT { private static final Logger log = LoggerFactory.getLogger(KerberosProxyIT.class); - private static final String PROXIED_USER = "proxied_user"; + private static final String PROXIED_USER1 = "proxied_user1", PROXIED_USER2 = "proxied_user2", PROXIED_USER3 = "proxied_user3"; @Rule public ExpectedException thrown = ExpectedException.none(); @@ -156,7 +156,7 @@ public void configureMiniCluster(MiniAccumuloConfigImpl cfg, Configuration coreS Map siteCfg = cfg.getSiteConfig(); // Allow the proxy to impersonate the "root" Accumulo user and our one special user. siteCfg.put(Property.INSTANCE_RPC_SASL_ALLOWED_USER_IMPERSONATION.getKey(), - proxyPrincipal + ":" + kdc.getRootUser().getPrincipal() + "," + kdc.qualifyUser(PROXIED_USER)); + proxyPrincipal + ":" + kdc.getRootUser().getPrincipal() + "," + kdc.qualifyUser(PROXIED_USER1) + "," + kdc.qualifyUser(PROXIED_USER2)); siteCfg.put(Property.INSTANCE_RPC_SASL_ALLOWED_HOST_IMPERSONATION.getKey(), "*"); cfg.setSiteConfig(siteCfg); } @@ -483,8 +483,12 @@ public void proxiedUserAccessWithoutAccumuloProxy() throws Exception { ClusterUser rootUser = kdc.getRootUser(); final UserGroupInformation rootUgi = UserGroupInformation.loginUserFromKeytabAndReturnUGI(rootUser.getPrincipal(), rootUser.getKeytab().getAbsolutePath()); final UserGroupInformation realUgi = UserGroupInformation.loginUserFromKeytabAndReturnUGI(proxyPrincipal, proxyKeytab.getAbsolutePath()); - final String userWithoutCredentials = kdc.qualifyUser(PROXIED_USER); - final UserGroupInformation proxyUser = UserGroupInformation.createProxyUser(userWithoutCredentials, realUgi); + final String userWithoutCredentials1 = kdc.qualifyUser(PROXIED_USER1); + final String userWithoutCredentials2 = kdc.qualifyUser(PROXIED_USER2); + final String userWithoutCredentials3 = kdc.qualifyUser(PROXIED_USER3); + final UserGroupInformation proxyUser1 = UserGroupInformation.createProxyUser(userWithoutCredentials1, realUgi); + final UserGroupInformation proxyUser2 = UserGroupInformation.createProxyUser(userWithoutCredentials2, realUgi); + final UserGroupInformation proxyUser3 = UserGroupInformation.createProxyUser(userWithoutCredentials3, realUgi); // Create a table and user, grant permission to our user to read that table. rootUgi.doAs(new PrivilegedExceptionAction() { @@ -493,8 +497,10 @@ public Void run() throws Exception { ZooKeeperInstance inst = new ZooKeeperInstance(mac.getClientConfig()); Connector conn = inst.getConnector(rootUgi.getUserName(), new KerberosToken()); conn.tableOperations().create(tableName); - conn.securityOperations().createLocalUser(userWithoutCredentials, new PasswordToken("ignored")); - conn.securityOperations().grantTablePermission(userWithoutCredentials, tableName, TablePermission.READ); + conn.securityOperations().createLocalUser(userWithoutCredentials1, new PasswordToken("ignored")); + conn.securityOperations().grantTablePermission(userWithoutCredentials1, tableName, TablePermission.READ); + conn.securityOperations().createLocalUser(userWithoutCredentials3, new PasswordToken("ignored")); + conn.securityOperations().grantTablePermission(userWithoutCredentials3, tableName, TablePermission.READ); return null; } }); @@ -515,16 +521,49 @@ public Void run() throws Exception { return null; } }); - proxyUser.doAs(new PrivilegedExceptionAction() { + // Allowed to be proxied and has read permission + proxyUser1.doAs(new PrivilegedExceptionAction() { @Override public Void run() throws Exception { ZooKeeperInstance inst = new ZooKeeperInstance(mac.getClientConfig()); - Connector conn = inst.getConnector(userWithoutCredentials, new KerberosToken(userWithoutCredentials)); + Connector conn = inst.getConnector(userWithoutCredentials1, new KerberosToken(userWithoutCredentials1)); Scanner s = conn.createScanner(tableName, Authorizations.EMPTY); assertFalse(s.iterator().hasNext()); return null; } }); + // Allowed to be proxied but does not have read permission + proxyUser2.doAs(new PrivilegedExceptionAction() { + @Override + public Void run() throws Exception { + ZooKeeperInstance inst = new ZooKeeperInstance(mac.getClientConfig()); + Connector conn = inst.getConnector(userWithoutCredentials2, new KerberosToken(userWithoutCredentials3)); + try { + Scanner s = conn.createScanner(tableName, Authorizations.EMPTY); + s.iterator().hasNext(); + Assert.fail("Expected to see an exception"); + } catch (RuntimeException e) { + int numSecurityExceptionsSeen = Iterables.size(Iterables.filter(Throwables.getCausalChain(e), + org.apache.accumulo.core.client.AccumuloSecurityException.class)); + assertTrue("Expected to see at least one AccumuloSecurityException, but saw: " + Throwables.getStackTraceAsString(e), numSecurityExceptionsSeen > 0); + } + return null; + } + }); + // Has read permission but is not allowed to be proxied + proxyUser3.doAs(new PrivilegedExceptionAction() { + @Override + public Void run() throws Exception { + ZooKeeperInstance inst = new ZooKeeperInstance(mac.getClientConfig()); + try { + inst.getConnector(userWithoutCredentials3, new KerberosToken(userWithoutCredentials3)); + Assert.fail("Should not be able to create a Connector as this user cannot be proxied"); + } catch (org.apache.accumulo.core.client.AccumuloSecurityException e) { + // Expected, this user cannot be proxied + } + return null; + } + }); } private static class ThriftExceptionMatchesPattern extends TypeSafeMatcher {