commit 3448fe9e18795245efac358d6b43405248c97697

Author: Kan Zhang <kan@yahoo-inc.com>
Date:   Tue Jan 18 15:25:13 2011 -0800

    patch based on 202


git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/branches/branch-0.20-security-202@1077815 13f79535-47bb-0310-9956-ffa450edef68

src/core/org/apache/hadoop/ipc/Client.java

@@ -1230,7 +1230,7 @@ public class Client {
                    + protocol.getCanonicalName());
          }
          return SecurityUtil.getServerPrincipal(conf.get(serverKey), address
-             .getAddress().getCanonicalHostName());
+             .getAddress());
        }
        return null;
      }

src/core/org/apache/hadoop/ipc/Server.java

@@ -838,8 +838,8 @@ public abstract class Server {
     // 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;
@@ -876,12 +876,11 @@ public abstract class Server {
       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<Call>();
@@ -904,8 +903,8 @@ public abstract class Server {
       return hostAddress;
     }
 
-    public String getHostName() {
-      return hostName;
+    public InetAddress getHostInetAddress() {
+      return addr;
     }
     
     public void setLastContact(long lastContact) {
@@ -1313,7 +1312,7 @@ public abstract class Server {
             && (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);
         }
@@ -1637,12 +1636,12 @@ public abstract class Server {
    * 
    * @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;
@@ -1652,7 +1651,7 @@ public abstract class Server {
         throw new AuthorizationException("Unknown protocol: " + 
                                          connection.getProtocol());
       }
-      ServiceAuthorizationManager.authorize(user, protocol, getConf(), hostname);
+      ServiceAuthorizationManager.authorize(user, protocol, getConf(), addr);
     }
   }
   

src/core/org/apache/hadoop/security/SecurityUtil.java

@@ -114,8 +114,8 @@ public class SecurityUtil {
   }
   
   /**
-   * 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.
    * 
@@ -128,24 +128,57 @@ public class SecurityUtil {
    */
   public static String getServerPrincipal(String principalConfig,
       String hostname) throws IOException {
-    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];
+    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;
+    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 {

src/core/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;
 
@@ -26,7 +27,6 @@ import org.apache.commons.logging.LogFactory;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.fs.CommonConfigurationKeys;
 import org.apache.hadoop.security.KerberosInfo;
-import org.apache.hadoop.security.KerberosName;
 import org.apache.hadoop.security.SecurityUtil;
 import org.apache.hadoop.security.UserGroupInformation;
 
@@ -65,13 +65,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 static void authorize(UserGroupInformation user, 
                                Class<?> protocol,
                                Configuration conf,
-                               String hostname
+                               InetAddress addr
                                ) throws AuthorizationException {
     AccessControlList acl = protocolToAcl.get(protocol);
     if (acl == null) {
@@ -85,39 +85,24 @@ public class ServiceAuthorizationManager {
     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
-      }
-    }
-    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);
   }

src/test/org/apache/hadoop/security/TestSecurityUtil.java

@@ -19,8 +19,10 @@ package org.apache.hadoop.security;
 import static org.junit.Assert.*;
 
 import java.io.IOException;
+import java.net.InetAddress;
 
 import org.junit.Test;
+import org.mockito.Mockito;
 
 public class TestSecurityUtil {
   @Test
@@ -38,27 +40,46 @@ public class TestSecurityUtil {
   
   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"));
   }
 }