From 762a83e044b84250c6e2543e02f48136361ea3eb Mon Sep 17 00:00:00 2001 From: Steve Loughran Date: Tue, 8 Jun 2021 21:56:40 +0100 Subject: [PATCH] HADOOP-17631. Configuration ${env.VAR:-FALLBACK} to eval FALLBACK when restrictSystemProps=true (#2977) Contributed by Steve Loughran. --- .../org/apache/hadoop/conf/Configuration.java | 79 ++++++++++++------- .../apache/hadoop/conf/TestConfiguration.java | 56 +++++++++++++ 2 files changed, 106 insertions(+), 29 deletions(-) diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java index e0e7ac3960..e4e36a24a6 100755 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java @@ -1139,36 +1139,37 @@ private String substituteVars(String expr) { final String var = eval.substring(varBounds[SUB_START_IDX], varBounds[SUB_END_IDX]); String val = null; - if (!restrictSystemProps) { - try { - if (var.startsWith("env.") && 4 < var.length()) { - String v = var.substring(4); - int i = 0; - for (; i < v.length(); i++) { - char c = v.charAt(i); - if (c == ':' && i < v.length() - 1 && v.charAt(i + 1) == '-') { - val = getenv(v.substring(0, i)); - if (val == null || val.length() == 0) { - val = v.substring(i + 2); - } - break; - } else if (c == '-') { - val = getenv(v.substring(0, i)); - if (val == null) { - val = v.substring(i + 1); - } - break; + try { + // evaluate system properties or environment variables even when + // the configuration is restricted -the restrictions are enforced + // in the getenv/getProperty calls + if (var.startsWith("env.") && 4 < var.length()) { + String v = var.substring(4); + int i = 0; + for (; i < v.length(); i++) { + char c = v.charAt(i); + if (c == ':' && i < v.length() - 1 && v.charAt(i + 1) == '-') { + val = getenv(v.substring(0, i)); + if (val == null || val.length() == 0) { + val = v.substring(i + 2); } + break; + } else if (c == '-') { + val = getenv(v.substring(0, i)); + if (val == null) { + val = v.substring(i + 1); + } + break; } - if (i == v.length()) { - val = getenv(v); - } - } else { - val = getProperty(var); } - } catch (SecurityException se) { - LOG.warn("Unexpected SecurityException in Configuration", se); + if (i == v.length()) { + val = getenv(v); + } + } else { + val = getProperty(var); } + } catch (SecurityException se) { + LOG.warn("Unexpected SecurityException in Configuration", se); } if (val == null) { val = getRaw(var); @@ -1194,13 +1195,33 @@ private String substituteVars(String expr) { throw new IllegalStateException("Variable substitution depth too large: " + MAX_SUBST + " " + expr); } - + + /** + * Get the environment variable value if + * {@link #restrictSystemProps} does not block this. + * @param name environment variable name. + * @return the value or null if either it is unset or access forbidden. + */ String getenv(String name) { - return System.getenv(name); + if (!restrictSystemProps) { + return System.getenv(name); + } else { + return null; + } } + /** + * Get a system property value if + * {@link #restrictSystemProps} does not block this. + * @param key property key + * @return the value or null if either it is unset or access forbidden. + */ String getProperty(String key) { - return System.getProperty(key); + if (!restrictSystemProps) { + return System.getProperty(key); + } else { + return null; + } } /** diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java index 085b204425..3748eed122 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java @@ -485,6 +485,62 @@ public void testEnvDefault() throws IOException { } } + /** + * Verify that when a configuration is restricted, environment + * variables and system properties will be unresolved. + * The fallback patterns for the variables are still parsed. + */ + @Test + public void testRestrictedEnv() throws IOException { + // this test relies on env.PATH being set on all platforms a + // test run will take place on, and the java.version sysprop + // set in all JVMs. + // Restricted configurations will not get access to these values, so + // will either be unresolved or, for env vars with fallbacks: the fallback + // values. + + conf.setRestrictSystemProperties(true); + + out = new BufferedWriter(new FileWriter(CONFIG)); + startConfig(); + // a simple property to reference + declareProperty("d", "D", "D"); + + // system property evaluation stops working completely + declareProperty("system1", "${java.version}", "${java.version}"); + + // the env variable does not resolve + declareProperty("secret1", "${env.PATH}", "${env.PATH}"); + + // but all the fallback options do work + declareProperty("secret2", "${env.PATH-a}", "a"); + declareProperty("secret3", "${env.PATH:-b}", "b"); + declareProperty("secret4", "${env.PATH:-}", ""); + declareProperty("secret5", "${env.PATH-}", ""); + // special case + declareProperty("secret6", "${env.PATH:}", "${env.PATH:}"); + // safety check + declareProperty("secret7", "${env.PATH:--}", "-"); + + // recursive eval of the fallback + declareProperty("secret8", "${env.PATH:-${d}}", "D"); + + // if the fallback doesn't resolve, the result is the whole variable raw. + declareProperty("secret9", "${env.PATH:-$d}}", "${env.PATH:-$d}}"); + + endConfig(); + Path fileResource = new Path(CONFIG); + conf.addResource(fileResource); + + for (Prop p : props) { + System.out.println("p=" + p.name); + String gotVal = conf.get(p.name); + String gotRawVal = conf.getRaw(p.name); + assertEq(p.val, gotRawVal); + assertEq(p.expectEval, gotVal); + } + } + @Test public void testFinalParam() throws IOException { out=new BufferedWriter(new FileWriter(CONFIG));