NIFI-5378 Prevent duplicate keys with different values in nifi.properties

This closes #6061

Signed-off-by: David Handermann <exceptionfactory@apache.org>

nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/main/java/org/apache/nifi/properties/NiFiPropertiesLoader.java

@@ -36,6 +36,7 @@ import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.security.SecureRandom;
 import java.util.Base64;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Properties;
 import java.util.Set;
@@ -122,20 +123,29 @@ public class NiFiPropertiesLoader {
         }
 
         logger.info("Loading Application Properties [{}]", file);
-        final Properties properties = new Properties();
+        final DuplicateDetectingProperties rawProperties = new DuplicateDetectingProperties();
+
         try (final InputStream inputStream = new BufferedInputStream(new FileInputStream(file))) {
-            properties.load(inputStream);
-
-            final Set<String> keys = properties.stringPropertyNames();
-            for (final String key : keys) {
-                final String property = properties.getProperty(key);
-                properties.setProperty(key, property.trim());
-            }
-
-            return new ProtectedNiFiProperties(properties);
+            rawProperties.load(inputStream);
         } catch (final Exception e) {
             throw new RuntimeException(String.format("Loading Application Properties [%s] failed", file), e);
         }
+
+        if (!rawProperties.redundantKeySet().isEmpty()) {
+            logger.warn("Duplicate property keys with the same value were detected in the properties file: {}", String.join(", ", rawProperties.redundantKeySet()));
+        }
+        if (!rawProperties.duplicateKeySet().isEmpty()) {
+            throw new IllegalArgumentException("Duplicate property keys with different values were detected in the properties file: " + String.join(", ", rawProperties.duplicateKeySet()));
+        }
+
+        final Properties properties = new Properties();
+        final Set<String> keys = rawProperties.stringPropertyNames();
+        for (final String key : keys) {
+            final String property = rawProperties.getProperty(key);
+            properties.setProperty(key, property.trim());
+        }
+
+        return new ProtectedNiFiProperties(properties);
     }
 
     /**
@@ -289,4 +299,35 @@ public class NiFiPropertiesLoader {
             }
         }
     }
+
+    private static class DuplicateDetectingProperties extends Properties {
+        // Only need to retain Properties key. This will help prevent possible inadvertent exposure of sensitive Properties value
+        private final Set<String> duplicateKeys = new HashSet<>();  // duplicate key with different values
+        private final Set<String> redundantKeys = new HashSet<>();  // duplicate key with same value
+        public DuplicateDetectingProperties() {
+            super();
+        }
+
+        public Set<String> duplicateKeySet() {
+            return duplicateKeys;
+        }
+
+        public Set<String> redundantKeySet() {
+            return redundantKeys;
+        }
+
+        @Override
+        public Object put(Object key, Object value) {
+            Object existingValue = super.put(key, value);
+            if (existingValue != null) {
+                if (existingValue.toString().equals(value.toString())) {
+                    redundantKeys.add(key.toString());
+                    return existingValue;
+                } else {
+                    duplicateKeys.add(key.toString());
+                }
+            }
+            return value;
+        }
+    }
 }

nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/test/java/org/apache/nifi/properties/NiFiPropertiesLoaderTest.java

@@ -25,18 +25,17 @@ import java.net.URL;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
 class NiFiPropertiesLoaderTest {
     private static final String NULL_PATH = null;
 
     private static final String EMPTY_PATH = "/properties/conf/empty.nifi.properties";
-
     private static final String FLOW_PATH = "/properties/conf/flow.nifi.properties";
-
     private static final String PROTECTED_PATH = "/properties/conf/protected.nifi.properties";
+    private static final String DUPLICATE_PROPERTIES_PATH = "/properties/conf/duplicates.nifi.properties";
 
     private static final String HEXADECIMAL_KEY = "12345678123456788765432187654321";
-
     private static final String EXPECTED_PASSWORD = "propertyValue";
 
     @AfterEach
@@ -44,6 +43,27 @@ class NiFiPropertiesLoaderTest {
         System.clearProperty(NiFiProperties.PROPERTIES_FILE_PATH);
     }
 
+    @Test
+    void testDuplicateProperties() {
+        final URL resource = NiFiPropertiesLoaderTest.class.getResource(DUPLICATE_PROPERTIES_PATH);
+        assertNotNull(resource);
+
+        final String path = resource.getPath();
+
+        System.setProperty(NiFiProperties.PROPERTIES_FILE_PATH, path);
+
+        final NiFiPropertiesLoader loader = NiFiPropertiesLoader.withKey(String.class.getSimpleName());
+
+        Exception exception = assertThrows(IllegalArgumentException.class, () -> {
+            final NiFiProperties properties = loader.get();
+        });
+
+        String expectedMessage = "Duplicate property keys with different values were detected in the properties file: another.duplicate, nifi.flow.configuration.file";
+        String actualMessage = exception.getMessage();
+
+        assertTrue(actualMessage.contains(expectedMessage));
+    }
+
     @Test
     void testGetPropertiesNotFound() {
         final NiFiPropertiesLoader loader = new NiFiPropertiesLoader();

nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/test/resources/properties/conf/duplicates.nifi.properties

@@ -0,0 +1,19 @@
+# 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.
+
+nifi.flow.configuration.file=./conf/flow.xml.gz
+nifi.flow.configuration.file=./conf/flow.json.gz
+another.duplicate=foo
+another.duplicate=bar

nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/test/resources/properties/conf/flow.nifi.properties

@@ -15,3 +15,6 @@
 
 nifi.flow.configuration.file=./conf/flow.xml.gz
 nifi.flow.configuration.json.file=./conf/flow.json.gz
+# duplicate with the same value intentionally added to verify this is valid
+# while bad form, it will generate only a warning, not a critical error
+nifi.flow.configuration.json.file=./conf/flow.json.gz