HADOOP-18972: Copy SASL properties map when returning from SaslPropertiesResolver

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/SaslPropertiesResolver.java

@@ -19,6 +19,7 @@
 
 import java.net.InetAddress;
 import java.util.Map;
+import java.util.SortedMap;
 import java.util.TreeMap;
 
 import javax.security.sasl.Sasl;
@@ -30,6 +31,8 @@
 import org.apache.hadoop.util.ReflectionUtils;
 import org.apache.hadoop.util.StringUtils;
 
+import static java.util.Objects.requireNonNull;
+
 /**
  * Provides SaslProperties to be used for a connection.
  * The default implementation is to read the values from configuration.
@@ -38,7 +41,7 @@
  *
  */
 public class SaslPropertiesResolver implements Configurable{
-  private Map<String,String> properties;
+  private SortedMap<String, String> properties;
   Configuration conf;
 
   /**
@@ -82,7 +85,7 @@ public Configuration getConf() {
    * @return sasl Properties
    */
   public Map<String,String> getDefaultProperties() {
-    return properties;
+    return cloneProperties();
   }
 
   /**
@@ -91,7 +94,7 @@ public Map<String,String> getDefaultProperties() {
    * @return the sasl properties to be used for the connection.
    */
   public Map<String, String> getServerProperties(InetAddress clientAddress){
-    return properties;
+    return cloneProperties();
   }
 
   /**
@@ -111,7 +114,7 @@ public Map<String, String> getServerProperties(InetAddress clientAddress,
    * @return the sasl properties to be used for the connection.
    */
   public Map<String, String> getClientProperties(InetAddress serverAddress){
-    return properties;
+    return cloneProperties();
   }
 
   /**
@@ -125,6 +128,12 @@ public Map<String, String> getClientProperties(InetAddress serverAddress,
     return getClientProperties(serverAddress);
   }
 
+  private Map<String, String> cloneProperties() {
+    requireNonNull(properties,
+        "properties map in SaslPropertiesResolver was null, did you call setConf()?");
+    return new TreeMap<>(properties);
+  }
+
   /**
    * A util function to retrieve specific additional sasl property from config.
    * Used by subclasses to read sasl properties used by themselves.

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestSaslPropertiesResolver.java

@@ -0,0 +1,73 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.security;
+
+import static org.apache.hadoop.fs.CommonConfigurationKeysPublic.HADOOP_RPC_PROTECTION;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+import java.util.Map;
+import javax.security.sasl.Sasl;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.test.AbstractHadoopTestBase;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+public class TestSaslPropertiesResolver extends AbstractHadoopTestBase {
+
+  private static final InetAddress LOCALHOST = new InetSocketAddress("127.0.0.1", 1).getAddress();
+
+  private SaslPropertiesResolver resolver;
+
+  @BeforeEach
+  public void setup() {
+    Configuration conf = new Configuration();
+    conf.set(HADOOP_RPC_PROTECTION, "privacy");
+    resolver = new SaslPropertiesResolver();
+    resolver.setConf(conf);
+  }
+
+  @Test
+  public void testResolverDoesNotMutate() {
+    assertPrivacyQop(resolver.getDefaultProperties());
+    setAuthenticationQop(resolver.getDefaultProperties());
+    // After changing the map returned by SaslPropertiesResolver, future maps are not affected
+    assertPrivacyQop(resolver.getDefaultProperties());
+
+    assertPrivacyQop(resolver.getClientProperties(LOCALHOST));
+    setAuthenticationQop(resolver.getClientProperties(LOCALHOST));
+    // After changing the map returned by SaslPropertiesResolver, future maps are not affected
+    assertPrivacyQop(resolver.getClientProperties(LOCALHOST));
+
+    assertPrivacyQop(resolver.getServerProperties(LOCALHOST));
+    setAuthenticationQop(resolver.getServerProperties(LOCALHOST));
+    // After changing the map returned by SaslPropertiesResolver, future maps are not affected
+    assertPrivacyQop(resolver.getServerProperties(LOCALHOST));
+  }
+
+  private static void setAuthenticationQop(Map<String, String> saslProperties) {
+    saslProperties.put(Sasl.QOP, SaslRpcServer.QualityOfProtection.AUTHENTICATION.getSaslQop());
+  }
+
+  private static void assertPrivacyQop(Map<String, String> saslProperties) {
+    assertThat(SaslRpcServer.QualityOfProtection.PRIVACY.getSaslQop()).isEqualTo(
+        saslProperties.get(Sasl.QOP));
+  }
+
+}