From 0f5d8517b526f74e94b989b11c0eb68f33acf43d Mon Sep 17 00:00:00 2001 From: Mark Bean Date: Thu, 19 May 2022 21:22:04 +0000 Subject: [PATCH] NIFI-5378 Prevent duplicate keys with different values in nifi.properties This closes #6061 Signed-off-by: David Handermann --- .../nifi/properties/NiFiPropertiesLoader.java | 61 ++++++++++++++++--- .../properties/NiFiPropertiesLoaderTest.java | 26 +++++++- .../conf/duplicates.nifi.properties | 19 ++++++ .../properties/conf/flow.nifi.properties | 3 + 4 files changed, 96 insertions(+), 13 deletions(-) create mode 100644 nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/test/resources/properties/conf/duplicates.nifi.properties diff --git a/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/main/java/org/apache/nifi/properties/NiFiPropertiesLoader.java b/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/main/java/org/apache/nifi/properties/NiFiPropertiesLoader.java index c2958b4acb..d893e8fd01 100644 --- a/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/main/java/org/apache/nifi/properties/NiFiPropertiesLoader.java +++ b/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 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 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 duplicateKeys = new HashSet<>(); // duplicate key with different values + private final Set redundantKeys = new HashSet<>(); // duplicate key with same value + public DuplicateDetectingProperties() { + super(); + } + + public Set duplicateKeySet() { + return duplicateKeys; + } + + public Set 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; + } + } } diff --git a/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/test/java/org/apache/nifi/properties/NiFiPropertiesLoaderTest.java b/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/test/java/org/apache/nifi/properties/NiFiPropertiesLoaderTest.java index a87ef218af..835e1f7129 100644 --- a/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/test/java/org/apache/nifi/properties/NiFiPropertiesLoaderTest.java +++ b/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(); diff --git a/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/test/resources/properties/conf/duplicates.nifi.properties b/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/test/resources/properties/conf/duplicates.nifi.properties new file mode 100644 index 0000000000..1ad45f1617 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/test/resources/properties/conf/flow.nifi.properties b/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/test/resources/properties/conf/flow.nifi.properties index 77241d5bee..eeac375b29 100644 --- a/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/test/resources/properties/conf/flow.nifi.properties +++ b/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