HADOOP-17631. Configuration ${env.VAR:-FALLBACK} to eval FALLBACK when restrictSystemProps=true (#2977)

Contributed by Steve Loughran.

Change-Id: I9b82109eddeb659c01896152cf603d458e2a04cd

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;
+    }
   }
 
   /**

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java

@@ -490,6 +490,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));