diff --git a/CHANGES.txt b/CHANGES.txt index c714a58da0..4b74f2c9ba 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -42,6 +42,9 @@ Trunk (unreleased changes) HADOOP-6995. Allow wildcards to be used in ProxyUsers configurations. (todd) + HADOOP-7104. Remove unnecessary DNS reverse lookups from RPC layer + (Kan Zhang via todd) + OPTIMIZATIONS BUG FIXES diff --git a/src/java/org/apache/hadoop/ipc/Client.java b/src/java/org/apache/hadoop/ipc/Client.java index 887e834018..f671ab27ce 100644 --- a/src/java/org/apache/hadoop/ipc/Client.java +++ b/src/java/org/apache/hadoop/ipc/Client.java @@ -1268,7 +1268,7 @@ private static String getRemotePrincipal(Configuration conf, + protocol.getCanonicalName()); } return SecurityUtil.getServerPrincipal(conf.get(serverKey), address - .getAddress().getCanonicalHostName()); + .getAddress()); } return null; } diff --git a/src/java/org/apache/hadoop/ipc/Server.java b/src/java/org/apache/hadoop/ipc/Server.java index afdcdb2b0d..f0813355d5 100644 --- a/src/java/org/apache/hadoop/ipc/Server.java +++ b/src/java/org/apache/hadoop/ipc/Server.java @@ -851,8 +851,8 @@ public class Connection { // Cache the remote host & port info so that even if the socket is // disconnected, we can say where it used to connect to. private String hostAddress; - private String hostName; private int remotePort; + private InetAddress addr; ConnectionHeader header = new ConnectionHeader(); Class protocol; @@ -889,12 +889,11 @@ public Connection(SelectionKey key, SocketChannel channel, this.unwrappedData = null; this.unwrappedDataLengthBuffer = ByteBuffer.allocate(4); this.socket = channel.socket(); - InetAddress addr = socket.getInetAddress(); + this.addr = socket.getInetAddress(); if (addr == null) { this.hostAddress = "*Unknown*"; } else { this.hostAddress = addr.getHostAddress(); - this.hostName = addr.getCanonicalHostName(); } this.remotePort = socket.getPort(); this.responseQueue = new LinkedList(); @@ -917,8 +916,8 @@ public String getHostAddress() { return hostAddress; } - public String getHostName() { - return hostName; + public InetAddress getHostInetAddress() { + return addr; } public void setLastContact(long lastContact) { @@ -1326,7 +1325,7 @@ private boolean authorizeConnection() throws IOException { && (authMethod != AuthMethod.DIGEST)) { ProxyUsers.authorize(user, this.getHostAddress(), conf); } - authorize(user, header, getHostName()); + authorize(user, header, getHostInetAddress()); if (LOG.isDebugEnabled()) { LOG.debug("Successfully authorized " + header); } @@ -1666,12 +1665,12 @@ public abstract Writable call(Class protocol, * * @param user client user * @param connection incoming connection - * @param hostname fully-qualified domain name of incoming connection + * @param addr InetAddress of incoming connection * @throws AuthorizationException when the client isn't authorized to talk the protocol */ public void authorize(UserGroupInformation user, ConnectionHeader connection, - String hostname + InetAddress addr ) throws AuthorizationException { if (authorize) { Class protocol = null; @@ -1681,7 +1680,7 @@ public void authorize(UserGroupInformation user, throw new AuthorizationException("Unknown protocol: " + connection.getProtocol()); } - serviceAuthorizationManager.authorize(user, protocol, getConf(), hostname); + serviceAuthorizationManager.authorize(user, protocol, getConf(), addr); } } diff --git a/src/java/org/apache/hadoop/security/SecurityUtil.java b/src/java/org/apache/hadoop/security/SecurityUtil.java index 44ef31ef32..4b2dff2035 100644 --- a/src/java/org/apache/hadoop/security/SecurityUtil.java +++ b/src/java/org/apache/hadoop/security/SecurityUtil.java @@ -135,8 +135,8 @@ public static void fetchServiceTicket(URL remoteHost) throws IOException { } /** - * Convert Kerberos principal name conf values to valid Kerberos principal - * names. It replaces $host in the conf values with hostname, which should be + * Convert Kerberos principal name pattern to valid Kerberos principal + * names. It replaces hostname pattern with hostname, which should be * fully-qualified domain name. If hostname is null or "0.0.0.0", it uses * dynamically looked-up fqdn of the current host instead. * @@ -149,24 +149,57 @@ public static void fetchServiceTicket(URL remoteHost) throws IOException { */ public static String getServerPrincipal(String principalConfig, String hostname) throws IOException { + String[] components = getComponents(principalConfig); + if (components == null || components.length != 3 + || !components[1].equals(HOSTNAME_PATTERN)) { + return principalConfig; + } else { + return replacePattern(components, hostname); + } + } + + /** + * Convert Kerberos principal name pattern to valid Kerberos principal names. + * This method is similar to {@link #getServerPrincipal(String, String)}, + * except 1) the reverse DNS lookup from addr to hostname is done only when + * necessary, 2) param addr can't be null (no default behavior of using local + * hostname when addr is null). + * + * @param principalConfig + * Kerberos principal name pattern to convert + * @param addr + * InetAddress of the host used for substitution + * @return converted Kerberos principal name + * @throws IOException + */ + public static String getServerPrincipal(String principalConfig, + InetAddress addr) throws IOException { + String[] components = getComponents(principalConfig); + if (components == null || components.length != 3 + || !components[1].equals(HOSTNAME_PATTERN)) { + return principalConfig; + } else { + if (addr == null) { + throw new IOException("Can't replace " + HOSTNAME_PATTERN + + " pattern since client address is null"); + } + return replacePattern(components, addr.getCanonicalHostName()); + } + } + + private static String[] getComponents(String principalConfig) { if (principalConfig == null) return null; - String[] components = principalConfig.split("[/@]"); - if (components.length != 3) { - throw new IOException( - "Kerberos service principal name isn't configured properly " - + "(should have 3 parts): " + principalConfig); - } - - if (components[1].equals(HOSTNAME_PATTERN)) { - String fqdn = hostname; - if (fqdn == null || fqdn.equals("") || fqdn.equals("0.0.0.0")) { - fqdn = getLocalHostName(); - } - return components[0] + "/" + fqdn + "@" + components[2]; - } else { - return principalConfig; + return principalConfig.split("[/@]"); + } + + private static String replacePattern(String[] components, String hostname) + throws IOException { + String fqdn = hostname; + if (fqdn == null || fqdn.equals("") || fqdn.equals("0.0.0.0")) { + fqdn = getLocalHostName(); } + return components[0] + "/" + fqdn + "@" + components[2]; } static String getLocalHostName() throws UnknownHostException { diff --git a/src/java/org/apache/hadoop/security/authorize/ServiceAuthorizationManager.java b/src/java/org/apache/hadoop/security/authorize/ServiceAuthorizationManager.java index a73fa2cd9f..12aa933177 100644 --- a/src/java/org/apache/hadoop/security/authorize/ServiceAuthorizationManager.java +++ b/src/java/org/apache/hadoop/security/authorize/ServiceAuthorizationManager.java @@ -18,6 +18,7 @@ package org.apache.hadoop.security.authorize; import java.io.IOException; +import java.net.InetAddress; import java.util.IdentityHashMap; import java.util.Map; import java.util.Set; @@ -30,7 +31,6 @@ import org.apache.hadoop.fs.CommonConfigurationKeys; import org.apache.hadoop.security.KerberosInfo; import org.apache.hadoop.security.SecurityUtil; -import org.apache.hadoop.security.KerberosName; import org.apache.hadoop.security.UserGroupInformation; /** @@ -71,13 +71,13 @@ public class ServiceAuthorizationManager { * @param user user accessing the service * @param protocol service being accessed * @param conf configuration to use - * @param hostname fully qualified domain name of the client + * @param addr InetAddress of the client * @throws AuthorizationException on authorization failure */ public void authorize(UserGroupInformation user, Class protocol, Configuration conf, - String hostname + InetAddress addr ) throws AuthorizationException { AccessControlList acl = protocolToAcl.get(protocol); if (acl == null) { @@ -91,41 +91,24 @@ public void authorize(UserGroupInformation user, if (krbInfo != null) { String clientKey = krbInfo.clientPrincipal(); if (clientKey != null && !clientKey.equals("")) { - if (hostname == null) { - throw new AuthorizationException( - "Can't authorize client when client hostname is null"); - } try { clientPrincipal = SecurityUtil.getServerPrincipal( - conf.get(clientKey), hostname); + conf.get(clientKey), addr); } catch (IOException e) { throw (AuthorizationException) new AuthorizationException( "Can't figure out Kerberos principal name for connection from " - + hostname + " for user=" + user + " protocol=" + protocol) + + addr + " for user=" + user + " protocol=" + protocol) .initCause(e); } } } - // when authorizing use the short name only - String shortName = clientPrincipal; - if(clientPrincipal != null ) { - try { - shortName = new KerberosName(clientPrincipal).getShortName(); - } catch (IOException e) { - LOG.warn("couldn't get short name from " + clientPrincipal, e); - // just keep going - } - } - if(LOG.isDebugEnabled()) { - LOG.debug("for protocol authorization compare (" + clientPrincipal + - "): " + shortName + " with " + user.getShortUserName()); - } - if((shortName != null && !shortName.equals(user.getShortUserName())) || + if((clientPrincipal != null && !clientPrincipal.equals(user.getUserName())) || !acl.isUserAllowed(user)) { - AUDITLOG.warn(AUTHZ_FAILED_FOR + user + " for protocol="+protocol); + AUDITLOG.warn(AUTHZ_FAILED_FOR + user + " for protocol=" + protocol + + ", expected client Kerberos principal is " + clientPrincipal); throw new AuthorizationException("User " + user + - " is not authorized for protocol " + - protocol); + " is not authorized for protocol " + protocol + + ", expected client Kerberos principal is " + clientPrincipal); } AUDITLOG.info(AUTHZ_SUCCESSFULL_FOR + user + " for protocol="+protocol); } diff --git a/src/test/core/org/apache/hadoop/security/TestSecurityUtil.java b/src/test/core/org/apache/hadoop/security/TestSecurityUtil.java index d5a3a25f90..b2bfaf383a 100644 --- a/src/test/core/org/apache/hadoop/security/TestSecurityUtil.java +++ b/src/test/core/org/apache/hadoop/security/TestSecurityUtil.java @@ -18,14 +18,16 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.assertEquals; import java.io.IOException; +import java.net.InetAddress; import javax.security.auth.kerberos.KerberosPrincipal; import org.apache.hadoop.conf.Configuration; -import org.junit.Assert; import org.junit.Test; +import org.mockito.Mockito; public class TestSecurityUtil { @Test @@ -50,28 +52,47 @@ public void isOriginalTGTReturnsCorrectValues() { private void verify(String original, String hostname, String expected) throws IOException { - assertTrue(SecurityUtil.getServerPrincipal(original, hostname).equals( - expected)); - assertTrue(SecurityUtil.getServerPrincipal(original, null).equals( - expected)); - assertTrue(SecurityUtil.getServerPrincipal(original, "").equals( - expected)); - assertTrue(SecurityUtil.getServerPrincipal(original, "0.0.0.0").equals( - expected)); + assertEquals(expected, + SecurityUtil.getServerPrincipal(original, hostname)); + + InetAddress addr = mockAddr(hostname); + assertEquals(expected, + SecurityUtil.getServerPrincipal(original, addr)); } + private InetAddress mockAddr(String reverseTo) { + InetAddress mock = Mockito.mock(InetAddress.class); + Mockito.doReturn(reverseTo).when(mock).getCanonicalHostName(); + return mock; + } + @Test public void testGetServerPrincipal() throws IOException { String service = "hdfs/"; String realm = "@REALM"; - String hostname = SecurityUtil.getLocalHostName(); + String hostname = "foohost"; + String userPrincipal = "foo@FOOREALM"; String shouldReplace = service + SecurityUtil.HOSTNAME_PATTERN + realm; String replaced = service + hostname + realm; verify(shouldReplace, hostname, replaced); String shouldNotReplace = service + SecurityUtil.HOSTNAME_PATTERN + "NAME" + realm; verify(shouldNotReplace, hostname, shouldNotReplace); - verify(shouldNotReplace, shouldNotReplace, shouldNotReplace); + verify(userPrincipal, hostname, userPrincipal); + // testing reverse DNS lookup doesn't happen + InetAddress notUsed = Mockito.mock(InetAddress.class); + assertEquals(shouldNotReplace, + SecurityUtil.getServerPrincipal(shouldNotReplace, notUsed)); + Mockito.verify(notUsed, Mockito.never()).getCanonicalHostName(); + } + + @Test + public void testLocalHostNameForNullOrWild() throws Exception { + String local = SecurityUtil.getLocalHostName(); + assertEquals("hdfs/" + local + "@REALM", + SecurityUtil.getServerPrincipal("hdfs/_HOST@REALM", (String)null)); + assertEquals("hdfs/" + local + "@REALM", + SecurityUtil.getServerPrincipal("hdfs/_HOST@REALM", "0.0.0.0")); } @Test