briansolo1985 commented on code in PR #8151:
URL: https://github.com/apache/nifi/pull/8151#discussion_r1442662592
##########
minifi/minifi-bootstrap/src/test/java/org/apache/nifi/minifi/bootstrap/status/reporters/StatusLoggerTest.java:
##########
@@ -142,10 +144,7 @@ public void testError() {
// Exception testing
Review Comment:
I know it's not your code, but please remove the comment, as it does not
hold any additional or valuable information
##########
minifi/minifi-nar-bundles/minifi-framework-bundle/minifi-framework/minifi-properties-loader/src/main/java/org/apache/nifi/minifi/properties/DuplicateDetectingProperties.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * 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.nifi.minifi.properties;
+
+import java.util.HashSet;
+import java.util.Properties;
+import java.util.Set;
+
+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<>();
+ private final Set<String> redundantKeys = new HashSet<>();
+
+ 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;
Review Comment:
I know it's just a copy paste code, but this return is unnecessary.
Practically we always return with value.
##########
minifi/minifi-nar-bundles/minifi-framework-bundle/minifi-framework/minifi-properties-loader/src/main/java/org/apache/nifi/minifi/properties/BootstrapProperties.java:
##########
@@ -41,4 +50,22 @@ public String getProperty(String key) {
.orElseGet(() -> super.getProperty(key));
}
+ @Override
+ public String getProperty(String key, String defaultValue) {
Review Comment:
With introducing this method the previous one could be refactored to
```
@Override
public String getProperty(String key) {
return getProperty(key, null);
}
```
##########
minifi/minifi-commons/minifi-commons-api/src/main/java/org/apache/nifi/minifi/commons/api/MiNiFiProperties.java:
##########
@@ -63,7 +63,7 @@ public enum MiNiFiProperties {
NIFI_MINIFI_SECURITY_SSL_PROTOCOL("nifi.minifi.security.ssl.protocol",
null, false, false, VALID),
NIFI_MINIFI_FLOW_USE_PARENT_SSL("nifi.minifi.flow.use.parent.ssl", null,
false, true, VALID),
NIFI_MINIFI_SENSITIVE_PROPS_KEY("nifi.minifi.sensitive.props.key", null,
true, false, VALID),
-
NIFI_MINIFI_SENSITIVE_PROPS_ALGORITHM("nifi.minifi.sensitive.props.algorithm",
null, true, false, VALID),
+
NIFI_MINIFI_SENSITIVE_PROPS_ALGORITHM("nifi.minifi.sensitive.props.algorithm",
null, false, false, VALID),
Review Comment:
Nice catch!
##########
minifi/minifi-bootstrap/src/test/java/org/apache/nifi/minifi/bootstrap/service/GracefulShutdownParameterProviderTest.java:
##########
@@ -46,9 +49,11 @@ class GracefulShutdownParameterProviderTest {
@NullSource
@ValueSource(strings = {"notAnInteger", "-1"})
void
testGetBootstrapPropertiesShouldReturnDefaultShutdownPropertyValue(String
shutdownProperty) throws IOException {
- Properties properties = new Properties();
+ BootstrapProperties properties = mock(BootstrapProperties.class);
if (shutdownProperty != null) {
- properties.setProperty(GRACEFUL_SHUTDOWN_PROP, shutdownProperty);
Review Comment:
We could eliminate the if with
Optional.ofNullable(shutdownProperty).orElse(DEFAULT_GRACEFUL_SHUTDOWN_VALUE)
##########
minifi/minifi-bootstrap/src/test/java/org/apache/nifi/minifi/bootstrap/configuration/ingestors/FileChangeIngestorTest.java:
##########
@@ -60,15 +63,15 @@ public void setUp() {
notifierSpy = Mockito.spy(new FileChangeIngestor());
mockDifferentiator = mock(Differentiator.class);
testNotifier = mock(ConfigurationChangeNotifier.class);
+ testProperties = mock(BootstrapProperties.class);
setMocks();
- testProperties = new Properties();
- testProperties.put(FileChangeIngestor.CONFIG_FILE_PATH_KEY,
TEST_CONFIG_PATH);
- testProperties.put(PullHttpChangeIngestor.OVERRIDE_SECURITY, "true");
- testProperties.put(PULL_HTTP_BASE_KEY + ".override.core", "true");
- testProperties.put(FileChangeIngestor.POLLING_PERIOD_INTERVAL_KEY,
FileChangeIngestor.DEFAULT_POLLING_PERIOD_INTERVAL);
- testProperties.put(MiNiFiProperties.NIFI_MINIFI_FLOW_CONFIG.getKey(),
MiNiFiProperties.NIFI_MINIFI_FLOW_CONFIG.getDefaultValue());
+
when(testProperties.getProperty(FileChangeIngestor.CONFIG_FILE_PATH_KEY)).thenReturn(TEST_CONFIG_PATH);
Review Comment:
Nice refactor
##########
minifi/minifi-toolkit/minifi-toolkit-assembly/README.md:
##########
@@ -60,6 +61,87 @@ After downloading the binary and extracting it, to run the
MiNiFi Toolkit Conver
## Note
It's not guaranteed in all circumstances that the migration will result in a
correct flow. For example if a processor's configuration has changed between
version, the conversion tool won't be aware of this, and will use the
deprecated property names. You will need to fix such issues manually.
+# <a id="encrypt-sensitive-properties-in-bootstrapconf"
href="#encrypt-sensitive-properties-in-bootstrapconf">Encrypting Sensitive
Properties in bootstrap.conf</a>
Review Comment:
Thanks for the detailed docs
##########
minifi/minifi-bootstrap/src/main/java/org/apache/nifi/minifi/bootstrap/service/MiNiFiExecCommandProvider.java:
##########
@@ -159,15 +160,16 @@ private String buildClassPath(File confDir, File libDir) {
.collect(joining(File.pathSeparator));
}
- private List<String> getJavaAdditionalArgs(Properties props) {
- return props.entrySet()
+ private List<String> getJavaAdditionalArgs(BootstrapProperties props) {
+ return props.getPropertyKeys()
.stream()
- .filter(entry -> ((String)
entry.getKey()).startsWith(JAVA_ARG_KEY_PREFIX))
- .map(entry -> (String) entry.getValue())
+ .filter(entry -> entry.startsWith(JAVA_ARG_KEY_PREFIX))
Review Comment:
Lamdba variable should now be called key instead of entry
##########
minifi/minifi-nar-bundles/minifi-framework-bundle/minifi-framework/minifi-properties-loader/src/main/java/org/apache/nifi/minifi/properties/BootstrapPropertiesLoader.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * 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.nifi.minifi.properties;
+
+import static java.lang.String.format;
+import static
org.apache.nifi.minifi.commons.utils.SensitivePropertyUtils.MINIFI_BOOTSTRAP_SENSITIVE_KEY;
+import static
org.apache.nifi.minifi.commons.utils.SensitivePropertyUtils.getFormattedKey;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.InputStream;
+import java.util.Properties;
+import org.apache.nifi.properties.AesGcmSensitivePropertyProvider;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class BootstrapPropertiesLoader {
+
+ private static final Logger LOGGER =
LoggerFactory.getLogger(BootstrapPropertiesLoader.class);
+
+ public static BootstrapProperties load(File file) {
+ ProtectedBootstrapProperties protectedProperties =
loadProtectedProperties(file);
+ if (protectedProperties.hasProtectedKeys()) {
+ String sensitiveKey =
protectedProperties.getApplicationProperties().getProperty(MINIFI_BOOTSTRAP_SENSITIVE_KEY);
+ validateSensitiveKeyProperty(sensitiveKey);
+ String keyHex = getFormattedKey(sensitiveKey);
+ protectedProperties.addSensitivePropertyProvider(new
AesGcmSensitivePropertyProvider(keyHex));
+ }
+ return protectedProperties.getUnprotectedProperties();
+ }
+
+ public static ProtectedBootstrapProperties loadProtectedProperties(File
file) {
+ if (file == null || !file.exists() || !file.canRead()) {
Review Comment:
This code is duplicated, and present in the same form in
MinifiPropertiesLoader class.
I suggest to create an interface and extract the common code to there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
