From a3470c65d8b4e205c8a16d0c0b8dad10d0134bb8 Mon Sep 17 00:00:00 2001 From: Steve Loughran Date: Fri, 30 Nov 2018 11:33:19 +0000 Subject: [PATCH] HADOOP-15959. Revert "HADOOP-12751. While using kerberos Hadoop incorrectly assumes names with '@' to be non-simple" --- .../authentication/util/KerberosName.java | 9 ++-- .../TestKerberosAuthenticationHandler.java | 7 ++- .../authentication/util/TestKerberosName.java | 17 +++++-- .../org/apache/hadoop/security/KDiag.java | 46 +------------------ .../src/site/markdown/SecureMode.md | 6 --- .../org/apache/hadoop/security/TestKDiag.java | 16 ------- .../security/TestUserGroupInformation.java | 27 ++++------- 7 files changed, 33 insertions(+), 95 deletions(-) diff --git a/hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/util/KerberosName.java b/hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/util/KerberosName.java index 5f50573b974..5eafd48f594 100644 --- a/hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/util/KerberosName.java +++ b/hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/util/KerberosName.java @@ -324,8 +324,8 @@ public class KerberosName { } } if (result != null && nonSimplePattern.matcher(result).find()) { - LOG.info("Non-simple name {} after auth_to_local rule {}", - result, this); + throw new NoMatchingRule("Non-simple name " + result + + " after auth_to_local rule " + this); } if (toLowerCase && result != null) { result = result.toLowerCase(Locale.ENGLISH); @@ -378,7 +378,7 @@ public class KerberosName { /** * Get the translation of the principal name into an operating system * user name. - * @return the user name + * @return the short name * @throws IOException throws if something is wrong with the rules */ public String getShortName() throws IOException { @@ -398,8 +398,7 @@ public class KerberosName { return result; } } - LOG.info("No auth_to_local rules applied to {}", this); - return toString(); + throw new NoMatchingRule("No rules applied to " + toString()); } /** diff --git a/hadoop-common-project/hadoop-auth/src/test/java/org/apache/hadoop/security/authentication/server/TestKerberosAuthenticationHandler.java b/hadoop-common-project/hadoop-auth/src/test/java/org/apache/hadoop/security/authentication/server/TestKerberosAuthenticationHandler.java index e3444ef4b92..408563f2993 100644 --- a/hadoop-common-project/hadoop-auth/src/test/java/org/apache/hadoop/security/authentication/server/TestKerberosAuthenticationHandler.java +++ b/hadoop-common-project/hadoop-auth/src/test/java/org/apache/hadoop/security/authentication/server/TestKerberosAuthenticationHandler.java @@ -109,7 +109,12 @@ public class TestKerberosAuthenticationHandler kn = new KerberosName("bar@BAR"); Assert.assertEquals("bar", kn.getShortName()); kn = new KerberosName("bar@FOO"); - Assert.assertEquals("bar@FOO", kn.getShortName()); + try { + kn.getShortName(); + Assert.fail(); + } + catch (Exception ex) { + } } @Test(timeout=60000) diff --git a/hadoop-common-project/hadoop-auth/src/test/java/org/apache/hadoop/security/authentication/util/TestKerberosName.java b/hadoop-common-project/hadoop-auth/src/test/java/org/apache/hadoop/security/authentication/util/TestKerberosName.java index c584fcebee8..2db0df4d540 100644 --- a/hadoop-common-project/hadoop-auth/src/test/java/org/apache/hadoop/security/authentication/util/TestKerberosName.java +++ b/hadoop-common-project/hadoop-auth/src/test/java/org/apache/hadoop/security/authentication/util/TestKerberosName.java @@ -72,14 +72,23 @@ public class TestKerberosName { } } + private void checkBadTranslation(String from) { + System.out.println("Checking bad translation for " + from); + KerberosName nm = new KerberosName(from); + try { + nm.getShortName(); + Assert.fail("didn't get exception for " + from); + } catch (IOException ie) { + // PASS + } + } + @Test public void testAntiPatterns() throws Exception { checkBadName("owen/owen/owen@FOO.COM"); checkBadName("owen@foo/bar.com"); - - // no rules applied, these should pass - checkTranslation("foo@ACME.COM", "foo@ACME.COM"); - checkTranslation("root/joe@FOO.COM", "root/joe@FOO.COM"); + checkBadTranslation("foo@ACME.COM"); + checkBadTranslation("root/joe@FOO.COM"); } @Test diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/KDiag.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/KDiag.java index d71269d6d06..f75e4740ae4 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/KDiag.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/KDiag.java @@ -25,7 +25,6 @@ import org.apache.directory.shared.kerberos.components.EncryptionKey; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.conf.Configured; import org.apache.hadoop.io.Text; -import org.apache.hadoop.security.authentication.util.KerberosName; import org.apache.hadoop.security.token.Token; import org.apache.hadoop.security.token.TokenIdentifier; import org.apache.hadoop.util.ExitUtil; @@ -54,7 +53,6 @@ import java.util.Collections; import java.util.Date; import java.util.LinkedList; import java.util.List; -import java.util.regex.Pattern; import static org.apache.hadoop.fs.CommonConfigurationKeysPublic.*; import static org.apache.hadoop.security.UserGroupInformation.*; @@ -130,12 +128,6 @@ public class KDiag extends Configured implements Tool, Closeable { private boolean nofail = false; private boolean nologin = false; private boolean jaas = false; - private boolean checkShortName = false; - - /** - * A pattern that recognizes simple/non-simple names. Per KerberosName - */ - private static final Pattern nonSimplePattern = Pattern.compile("[/@]"); /** * Flag set to true if a {@link #verify(boolean, String, String, Object...)} @@ -164,8 +156,6 @@ public class KDiag extends Configured implements Tool, Closeable { public static final String ARG_SECURE = "--secure"; - public static final String ARG_VERIFYSHORTNAME = "--verifyshortname"; - @SuppressWarnings("IOResourceOpenedButNotSafelyClosed") public KDiag(Configuration conf, PrintWriter out, @@ -209,7 +199,6 @@ public class KDiag extends Configured implements Tool, Closeable { nofail = popOption(ARG_NOFAIL, args); jaas = popOption(ARG_JAAS, args); nologin = popOption(ARG_NOLOGIN, args); - checkShortName = popOption(ARG_VERIFYSHORTNAME, args); // look for list of resources String resource; @@ -255,9 +244,7 @@ public class KDiag extends Configured implements Tool, Closeable { + arg(ARG_NOLOGIN, "", "Do not attempt to log in") + arg(ARG_OUTPUT, "", "Write output to a file") + arg(ARG_RESOURCE, "", "Load an XML configuration resource") - + arg(ARG_SECURE, "", "Require the hadoop configuration to be secure") - + arg(ARG_VERIFYSHORTNAME, ARG_PRINCIPAL + " ", - "Verify the short name of the specific principal does not contain '@' or '/'"); + + arg(ARG_SECURE, "", "Require the hadoop configuration to be secure"); } private String arg(String name, String params, String meaning) { @@ -290,7 +277,6 @@ public class KDiag extends Configured implements Tool, Closeable { println("%s = %d", ARG_KEYLEN, minKeyLength); println("%s = %s", ARG_KEYTAB, keytab); println("%s = %s", ARG_PRINCIPAL, principal); - println("%s = %s", ARG_VERIFYSHORTNAME, checkShortName); // Fail fast on a JVM without JCE installed. validateKeyLength(); @@ -390,10 +376,6 @@ public class KDiag extends Configured implements Tool, Closeable { validateJAAS(jaas); validateNTPConf(); - if (checkShortName) { - validateShortName(); - } - if (!nologin) { title("Logging in"); if (keytab != null) { @@ -447,32 +429,6 @@ public class KDiag extends Configured implements Tool, Closeable { aesLen, minKeyLength); } - /** - * Verify whether auth_to_local rules transform a principal name - *

- * Having a local user name "bar@foo.com" may be harmless, so it is noted at - * info. However if what was intended is a transformation to "bar" - * it can be difficult to debug, hence this check. - */ - protected void validateShortName() { - failif(principal == null, CAT_KERBEROS, "No principal defined"); - - try { - KerberosName kn = new KerberosName(principal); - String result = kn.getShortName(); - if (nonSimplePattern.matcher(result).find()) { - warn(CAT_KERBEROS, principal + " short name: " + result - + " still contains @ or /"); - } - } catch (IOException e) { - throw new KerberosDiagsFailure(CAT_KERBEROS, e, - "Failed to get short name for " + principal, e); - } catch (IllegalArgumentException e) { - error(CAT_KERBEROS, "KerberosName(" + principal + ") failed: %s\n%s", - e, StringUtils.stringifyException(e)); - } - } - /** * Get the default realm. *

diff --git a/hadoop-common-project/hadoop-common/src/site/markdown/SecureMode.md b/hadoop-common-project/hadoop-common/src/site/markdown/SecureMode.md index dc2d48f437d..d206b534180 100644 --- a/hadoop-common-project/hadoop-common/src/site/markdown/SecureMode.md +++ b/hadoop-common-project/hadoop-common/src/site/markdown/SecureMode.md @@ -476,7 +476,6 @@ KDiag: Diagnose Kerberos Problems [--out ] : Write output to a file. [--resource ] : Load an XML configuration resource. [--secure] : Require the hadoop configuration to be secure. - [--verifyshortname ]: Verify the short name of the specific principal does not contain '@' or '/' ``` #### `--jaas`: Require a JAAS file to be defined in `java.security.auth.login.config`. @@ -575,11 +574,6 @@ or implicitly set to "simple": Needless to say, an application so configured cannot talk to a secure Hadoop cluster. -#### `--verifyshortname `: validate the short name of a principal - -This verifies that the short name of a principal contains neither the `"@"` -nor `"/"` characters. - ### Example ``` diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestKDiag.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestKDiag.java index b81c4c51766..1ba11cc0c9e 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestKDiag.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestKDiag.java @@ -164,22 +164,6 @@ public class TestKDiag extends Assert { ARG_PRINCIPAL, "foo@EXAMPLE.COM"); } - @Test - public void testKerberosName() throws Throwable { - kdiagFailure(ARG_KEYLEN, KEYLEN, - ARG_VERIFYSHORTNAME, - ARG_PRINCIPAL, "foo/foo/foo@BAR.COM"); - } - - @Test - public void testShortName() throws Throwable { - kdiag(ARG_KEYLEN, KEYLEN, - ARG_KEYTAB, keytab.getAbsolutePath(), - ARG_PRINCIPAL, - ARG_VERIFYSHORTNAME, - ARG_PRINCIPAL, "foo@EXAMPLE.COM"); - } - @Test public void testFileOutput() throws Throwable { File f = new File("target/kdiag.txt"); diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java index a6b98235abf..2974f3e3a50 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java @@ -297,15 +297,10 @@ public class TestUserGroupInformation { UserGroupInformation.setConfiguration(conf); testConstructorSuccess("user1", "user1"); testConstructorSuccess("user4@OTHER.REALM", "other-user4"); - - // pass through test, no transformation - testConstructorSuccess("user2@DEFAULT.REALM", "user2@DEFAULT.REALM"); - testConstructorSuccess("user3/cron@DEFAULT.REALM", "user3/cron@DEFAULT.REALM"); - testConstructorSuccess("user5/cron@OTHER.REALM", "user5/cron@OTHER.REALM"); - - // failures - testConstructorFailures("user6@example.com@OTHER.REALM"); - testConstructorFailures("user7@example.com@DEFAULT.REALM"); + // failure test + testConstructorFailures("user2@DEFAULT.REALM"); + testConstructorFailures("user3/cron@DEFAULT.REALM"); + testConstructorFailures("user5/cron@OTHER.REALM"); testConstructorFailures(null); testConstructorFailures(""); } @@ -319,13 +314,10 @@ public class TestUserGroupInformation { testConstructorSuccess("user1", "user1"); testConstructorSuccess("user2@DEFAULT.REALM", "user2"); - testConstructorSuccess("user3/cron@DEFAULT.REALM", "user3"); - - // no rules applied, local name remains the same - testConstructorSuccess("user4@OTHER.REALM", "user4@OTHER.REALM"); - testConstructorSuccess("user5/cron@OTHER.REALM", "user5/cron@OTHER.REALM"); - + testConstructorSuccess("user3/cron@DEFAULT.REALM", "user3"); // failure test + testConstructorFailures("user4@OTHER.REALM"); + testConstructorFailures("user5/cron@OTHER.REALM"); testConstructorFailures(null); testConstructorFailures(""); } @@ -366,9 +358,8 @@ public class TestUserGroupInformation { } catch (IllegalArgumentException e) { String expect = (userName == null || userName.isEmpty()) ? "Null user" : "Illegal principal name "+userName; - String expect2 = "Malformed Kerberos name: "+userName; - assertTrue("Did not find "+ expect + " or " + expect2 + " in " + e, - e.toString().contains(expect) || e.toString().contains(expect2)); + assertTrue("Did not find "+ expect + " in " + e, + e.toString().contains(expect)); } }