NIFI-12501 Encrypt MiNiFi bootstrap properties #8151

ferencerdei · 2023-12-12T12:44:55Z

Summary

This change makes it possible to encrypt bootstrap.conf file. The properties used in bootstrap classes and the MiNiFi framework will handle these (separate bootstrap/minifi property loaders are defined).

Added an encrypt config functionality to the MiNiFi toolkit that generates a random encryption key and encrypts sensitive values in the defined bootstrap.conf.

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Apache NiFi Jira issue created

Pull Request Tracking

Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

Pull Request based on current revision of the main branch
Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

Build completed using mvn clean install -P contrib-check
- JDK 21

Licensing

New dependencies are compatible with the Apache License 2.0 according to the License Policy
New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

Documentation formatting appears as expected in rendered files

exceptionfactory

Thanks for putting together this new feature @ferencerdei.

As much of the implementation is based on the existing NiFi approach, the code looks straightforward in general. However, there appear to be a few places where copying some of the NiFi approach brought over some elements that may not be needed. This is particularly true of the nifi-property-protection-factory library, which has many transitive dependencies that do not apply to MiNiFi. That is one high-level item that should be addressed. I also noted a handful of other items to consider.

exceptionfactory · 2023-12-13T02:48:18Z

...tstrap/src/main/java/org/apache/nifi/minifi/bootstrap/service/MiNiFiExecCommandProvider.java

+        return bootstrapProperties.containsKey(MINIFI_BOOTSTRAP_SENSITIVE_KEY) && !StringUtils.isBlank(bootstrapProperties.getProperty(MINIFI_BOOTSTRAP_SENSITIVE_KEY));
+    }
+
+    private Path createSensitiveKeyFile(File confDir) {


Is there a reason for writing out the sensitive key value to a new file as opposed to having the new process read the key from the bootstrap configuration itself? Although NiFi bootstrap has done some similar things with other values, this is not an optimal approach as it makes additional copies on the file system. This is one example of an approach where some additional consideration is worthwhile to implement a better solution.

I wanted to keep to logic as consistent as possible with the NiFi, so we can easily switch when we change the bootstrap / application properties file structure to the same as in NiFi. But yea, It can be read from bootstrap directly (it could be even in NiFi.)

Thanks for the reply, given that this is a new implementation, although based on the existing one, this is a good opportunity to change the approach for MiNiFi itself.

exceptionfactory · 2023-12-13T02:49:25Z

...tstrap/src/main/java/org/apache/nifi/minifi/bootstrap/service/MiNiFiExecCommandProvider.java

+        if (isSensitiveKeyPresent(bootstrapProperties)) {
+            Path sensitiveKeyFile = createSensitiveKeyFile(confDir);
+            writeSensitiveKeyFile(bootstrapProperties, sensitiveKeyFile);
+            sensitiveParameter = "-K " + sensitiveKeyFile.toFile().getAbsolutePath();


As mentioned in writing the key, it seems like it one option would be to pass the path to the bootstrap configuration and having the called process read the file, instead of creating a new copy of the key in a new file.

ok, I'll apply that change

...ns/minifi-commons-utils/src/main/java/org/apache/nifi/minifi/commons/utils/PropertyUtil.java

...minifi-nar-bundles/minifi-framework-bundle/minifi-framework/minifi-properties-loader/pom.xml

minifi/minifi-nar-bundles/minifi-framework-bundle/pom.xml

minifi/minifi-toolkit/minifi-toolkit-assembly/README.md

...-config/src/main/java/org/apache/nifi/minifi/toolkit/config/command/MiNiFiEncryptConfig.java

ferencerdei

Thanks for the review, I've applied the suggested changes

ferencerdei · 2023-12-13T07:54:39Z

...tstrap/src/main/java/org/apache/nifi/minifi/bootstrap/service/MiNiFiExecCommandProvider.java

+        return bootstrapProperties.containsKey(MINIFI_BOOTSTRAP_SENSITIVE_KEY) && !StringUtils.isBlank(bootstrapProperties.getProperty(MINIFI_BOOTSTRAP_SENSITIVE_KEY));
+    }
+
+    private Path createSensitiveKeyFile(File confDir) {


I wanted to keep to logic as consistent as possible with the NiFi, so we can easily switch when we change the bootstrap / application properties file structure to the same as in NiFi. But yea, It can be read from bootstrap directly (it could be even in NiFi.)

...minifi-nar-bundles/minifi-framework-bundle/minifi-framework/minifi-properties-loader/pom.xml

ferencerdei · 2023-12-13T08:09:46Z

...tstrap/src/main/java/org/apache/nifi/minifi/bootstrap/service/MiNiFiExecCommandProvider.java

+        if (isSensitiveKeyPresent(bootstrapProperties)) {
+            Path sensitiveKeyFile = createSensitiveKeyFile(confDir);
+            writeSensitiveKeyFile(bootstrapProperties, sensitiveKeyFile);
+            sensitiveParameter = "-K " + sensitiveKeyFile.toFile().getAbsolutePath();


ok, I'll apply that change

...ns/minifi-commons-utils/src/main/java/org/apache/nifi/minifi/commons/utils/PropertyUtil.java

...-config/src/main/java/org/apache/nifi/minifi/toolkit/config/command/MiNiFiEncryptConfig.java

minifi/minifi-toolkit/minifi-toolkit-assembly/README.md

...work-bundle/minifi-framework/minifi-runtime/src/main/java/org/apache/nifi/minifi/MiNiFi.java

...roperties-loader/src/main/java/org/apache/nifi/minifi/properties/MiNiFiPropertiesLoader.java

...ies-loader/src/main/java/org/apache/nifi/minifi/properties/ProtectedBootstrapProperties.java

...erties-loader/src/main/java/org/apache/nifi/minifi/properties/ProtectedMiNiFiProperties.java

briansolo1985

Thanks for putting together this change. It helps MiNiFi to become more secure by protecting the sensitive properties. I have no issues with the approach, only left some cosmetics related comment.

briansolo1985 · 2024-01-05T08:16:43Z

...i-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) {


With introducing this method the previous one could be refactored to

@Override public String getProperty(String key) { return getProperty(key, null); }

briansolo1985 · 2024-01-05T08:24:47Z

...erties-loader/src/main/java/org/apache/nifi/minifi/properties/BootstrapPropertiesLoader.java

+    }
+
+    public static ProtectedBootstrapProperties loadProtectedProperties(File file) {
+        if (file == null || !file.exists() || !file.canRead()) {


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.

briansolo1985 · 2024-01-05T08:43:01Z

...ies-loader/src/main/java/org/apache/nifi/minifi/properties/DuplicateDetectingProperties.java

+        if (existingValue != null) {
+            if (existingValue.toString().equals(value.toString())) {
+                redundantKeys.add(key.toString());
+                return existingValue;


I know it's just a copy paste code, but this return is unnecessary. Practically we always return with value.

briansolo1985 · 2024-01-05T09:21:02Z

...tstrap/src/main/java/org/apache/nifi/minifi/bootstrap/service/MiNiFiExecCommandProvider.java

            .stream()
-            .filter(entry -> ((String) entry.getKey()).startsWith(JAVA_ARG_KEY_PREFIX))
-            .map(entry -> (String) entry.getValue())
+            .filter(entry -> entry.startsWith(JAVA_ARG_KEY_PREFIX))


Lamdba variable should now be called key instead of entry

briansolo1985 · 2024-01-05T09:24:03Z

...st/java/org/apache/nifi/minifi/bootstrap/configuration/ingestors/FileChangeIngestorTest.java

-        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);


Nice refactor

briansolo1985 · 2024-01-05T09:26:50Z

...est/java/org/apache/nifi/minifi/bootstrap/service/GracefulShutdownParameterProviderTest.java

        if (shutdownProperty != null) {
-            properties.setProperty(GRACEFUL_SHUTDOWN_PROP, shutdownProperty);


We could eliminate the if with Optional.ofNullable(shutdownProperty).orElse(DEFAULT_GRACEFUL_SHUTDOWN_VALUE)

briansolo1985 · 2024-01-05T09:29:14Z

...tstrap/src/test/java/org/apache/nifi/minifi/bootstrap/status/reporters/StatusLoggerTest.java

@@ -142,10 +144,7 @@ public void testError() {
    // Exception testing


I know it's not your code, but please remove the comment, as it does not hold any additional or valuable information

briansolo1985 · 2024-01-05T09:30:22Z

...ns/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),


Nice catch!

briansolo1985 · 2024-01-05T09:51:47Z

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>


Thanks for the detailed docs

exceptionfactory

@ferencerdei Just a note that this appears close to completion, but there are a couple conflicts at the moment that require rebasing.

…nifi_bootstrap_conf_properties # Conflicts: # minifi/minifi-assembly/README.md # minifi/minifi-bootstrap/src/test/java/org/apache/nifi/minifi/bootstrap/configuration/ingestors/PullHttpChangeIngestorCommonTest.java # minifi/minifi-bootstrap/src/test/java/org/apache/nifi/minifi/bootstrap/configuration/ingestors/PullHttpChangeIngestorSSLTest.java # minifi/minifi-bootstrap/src/test/java/org/apache/nifi/minifi/bootstrap/configuration/ingestors/PullHttpChangeIngestorTest.java # minifi/minifi-c2/minifi-c2-assembly/README.md

ferencerdei · 2024-01-19T12:56:15Z

Thanks for the review comments, I resolved the requested changes and conflicts as well.

briansolo1985 · 2024-01-22T09:07:26Z

Thanks for the updates. Looked good to me. Merged.

ferencerdei added the minifi Pull requests that updates minifi/c2 codes label Dec 12, 2023

ferencerdei force-pushed the NIFI-12501_encrypt_minifi_bootstrap_conf_properties branch from 5243f63 to 4b1ffa7 Compare December 12, 2023 15:23

exceptionfactory self-requested a review December 12, 2023 16:07

ferencerdei force-pushed the NIFI-12501_encrypt_minifi_bootstrap_conf_properties branch from 4b1ffa7 to 18421f1 Compare December 12, 2023 18:04

exceptionfactory requested changes Dec 13, 2023

View reviewed changes

NIFI-12501 Encrypt MiNiFi bootstrap properties

66f2596

ferencerdei force-pushed the NIFI-12501_encrypt_minifi_bootstrap_conf_properties branch from 18421f1 to 66f2596 Compare December 13, 2023 15:09

ferencerdei commented Dec 13, 2023

View reviewed changes

briansolo1985 requested changes Jan 5, 2024

View reviewed changes

exceptionfactory reviewed Jan 15, 2024

View reviewed changes

ferencerdei added 2 commits January 19, 2024 11:50

NIFI-12501 Apply review comments

d8d101b

ferencerdei requested review from briansolo1985 and exceptionfactory January 19, 2024 12:56

briansolo1985 closed this in 5b9897c Jan 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NIFI-12501 Encrypt MiNiFi bootstrap properties #8151

NIFI-12501 Encrypt MiNiFi bootstrap properties #8151

ferencerdei commented Dec 12, 2023 •

edited

Loading

exceptionfactory left a comment

exceptionfactory Dec 13, 2023

ferencerdei Dec 13, 2023

exceptionfactory Dec 13, 2023

exceptionfactory Dec 13, 2023

ferencerdei Dec 13, 2023

ferencerdei left a comment

ferencerdei Dec 13, 2023

ferencerdei Dec 13, 2023

briansolo1985 left a comment

briansolo1985 Jan 5, 2024

briansolo1985 Jan 5, 2024

briansolo1985 Jan 5, 2024

briansolo1985 Jan 5, 2024

briansolo1985 Jan 5, 2024

briansolo1985 Jan 5, 2024

briansolo1985 Jan 5, 2024

briansolo1985 Jan 5, 2024

briansolo1985 Jan 5, 2024

exceptionfactory left a comment

ferencerdei commented Jan 19, 2024

briansolo1985 commented Jan 22, 2024

		if (shutdownProperty != null) {
		properties.setProperty(GRACEFUL_SHUTDOWN_PROP, shutdownProperty);

		@@ -142,10 +144,7 @@ public void testError() {
		// Exception testing

NIFI-12501 Encrypt MiNiFi bootstrap properties #8151

NIFI-12501 Encrypt MiNiFi bootstrap properties #8151

Conversation

ferencerdei commented Dec 12, 2023 • edited Loading

Summary

Tracking

Issue Tracking

Pull Request Tracking

Pull Request Formatting

Verification

Build

Licensing

Documentation

exceptionfactory left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferencerdei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briansolo1985 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exceptionfactory left a comment

Choose a reason for hiding this comment

ferencerdei commented Jan 19, 2024

briansolo1985 commented Jan 22, 2024

ferencerdei commented Dec 12, 2023 •

edited

Loading