You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2020/09/29 20:00:41 UTC
[GitHub] [nifi-minifi-cpp] fgerlits opened a new pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
fgerlits opened a new pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914
https://issues.apache.org/jira/browse/MINIFICPP-1323
Encrypt sensitive properties in the minifi.properties file. The change has four parts:
* introduce a new dependency: libsodium
* wrap libsodium functions in `EncryptionUtils`
* implement a new command-line tool called `encrypt-config` which can encrypt the sensitive properties
* decrypt the sensitive properties during startup.
Please also review the wiki page https://cwiki.apache.org/confluence/display/MINIFI/Encrypt+sensitive+configuration+properties as part of this pull request.
---
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [x] Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [x] Has your PR been rebased against the latest commit within the target branch (typically main)?
- [ ] Is your initial contribution a single, squashed commit?
### For code changes:
- [x] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [x] If applicable, have you updated the LICENSE file?
- [x] If applicable, have you updated the NOTICE file?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r501824449
##########
File path: encrypt-config/ConfigFile.cpp
##########
@@ -0,0 +1,171 @@
+/**
+ * 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.
+ */
+
+#include "ConfigFile.h"
+
+#include <algorithm>
+#include <fstream>
+#include <utility>
+
+#include "utils/StringUtils.h"
+
+namespace {
+constexpr std::array<const char*, 2> DEFAULT_SENSITIVE_PROPERTIES{"nifi.security.client.pass.phrase",
+ "nifi.rest.api.password"};
+constexpr const char* ADDITIONAL_SENSITIVE_PROPS_PROPERTY_NAME = "nifi.sensitive.props.additional.keys";
+} // namespace
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace encrypt_config {
+
+ConfigLine::ConfigLine(std::string line) : line_(line) {
+ line = utils::StringUtils::trim(line);
+ if (line.empty() || line[0] == '#') { return; }
+
+ size_t index_of_first_equals_sign = line.find('=');
+ if (index_of_first_equals_sign == std::string::npos) { return; }
+
+ std::string key = utils::StringUtils::trim(line.substr(0, index_of_first_equals_sign));
+ if (key.empty()) { return; }
+
+ key_ = key;
+ value_ = utils::StringUtils::trim(line.substr(index_of_first_equals_sign + 1));
+}
+
+ConfigLine::ConfigLine(std::string key, std::string value)
+ : line_{utils::StringUtils::join_pack(key, "=", value)}, key_{std::move(key)}, value_{std::move(value)} {
+}
+
+void ConfigLine::updateValue(const std::string& value) {
+ auto pos = line_.find('=');
+ if (pos != std::string::npos) {
+ line_.replace(pos + 1, std::string::npos, value);
+ value_ = value;
+ } else {
+ throw std::invalid_argument{"Cannot update value in config line: it does not contain an = sign!"};
+ }
+}
+
+ConfigFile::ConfigFile(std::istream& input_stream) {
+ std::string line;
+ while (std::getline(input_stream, line)) {
+ config_lines_.push_back(ConfigLine{line});
+ }
+}
+
+ConfigFile::Lines::const_iterator ConfigFile::findKey(const std::string& key) const {
+ return std::find_if(config_lines_.cbegin(), config_lines_.cend(), [&key](const ConfigLine& config_line) {
+ return config_line.getKey() == key;
+ });
+}
+
+ConfigFile::Lines::iterator ConfigFile::findKey(const std::string& key) {
+ return std::find_if(config_lines_.begin(), config_lines_.end(), [&key](const ConfigLine& config_line) {
+ return config_line.getKey() == key;
+ });
+}
+
+bool ConfigFile::hasValue(const std::string& key) const {
+ const auto it = findKey(key);
+ return (it != config_lines_.end());
+}
+
+utils::optional<std::string> ConfigFile::getValue(const std::string& key) const {
+ const auto it = findKey(key);
+ if (it != config_lines_.end()) {
+ return it->getValue();
+ } else {
+ return utils::nullopt;
+ }
+}
+
+void ConfigFile::update(const std::string& key, const std::string& value) {
+ auto it = findKey(key);
+ if (it != config_lines_.end()) {
+ it->updateValue(value);
+ } else {
+ throw std::invalid_argument{"Key " + key + " not found in the config file!"};
+ }
+}
+
+void ConfigFile::insertAfter(const std::string& after_key, const std::string& key, const std::string& value) {
+ auto it = findKey(after_key);
+ if (it != config_lines_.end()) {
+ ++it;
+ config_lines_.emplace(it, key, value);
+ } else {
+ throw std::invalid_argument{"Key " + after_key + " not found in the config file!"};
+ }
+}
+
+void ConfigFile::append(const std::string& key, const std::string& value) {
+ config_lines_.emplace_back(key, value);
+}
+
+int ConfigFile::erase(const std::string& key) {
+ auto has_this_key = [&key](const ConfigLine& line) { return line.getKey() == key; };
+ auto new_end = std::remove_if(config_lines_.begin(), config_lines_.end(), has_this_key);
+ auto num_removed = std::distance(new_end, config_lines_.end());
+ config_lines_.erase(new_end, config_lines_.end());
+ return gsl::narrow<int>(num_removed);
+}
+
+void ConfigFile::writeTo(const std::string& file_path) const {
+ std::ofstream file{file_path};
Review comment:
`std::ofstream` doesn't throw on error, just sets error bits by default. This hides errors and causes me as a user to not realize that despite the success output, nothing has changes because I was not running as admin.
Luckily its error reporting behavior can be configured.
```suggestion
std::ofstream file{file_path};
file.exceptions(std::ios::failbit | std::ios::badbit);
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r498117414
##########
File path: encrypt-config/ConfigFileEncryptor.cpp
##########
@@ -0,0 +1,64 @@
+/**
+ * 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.
+ */
+
+#include "ConfigFileEncryptor.h"
+
+#include <iostream>
+#include <string>
+
+#include "utils/StringUtils.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace encrypt_config {
+
+int encryptSensitivePropertiesInFile(ConfigFile& config_file, const utils::crypto::Bytes& encryption_key) {
Review comment:
An unsigned type like std::size_t or uint32_t may be more appropriate in this case.
##########
File path: encrypt-config/EncryptConfig.cpp
##########
@@ -0,0 +1,150 @@
+/**
+ * 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.
+ */
+
+#include "EncryptConfig.h"
+
+#include <sodium.h>
+
+#include <stdexcept>
+
+#include "ConfigFile.h"
+#include "ConfigFileEncryptor.h"
+#include "cxxopts.hpp"
+#include "utils/file/FileUtils.h"
+#include "utils/OptionalUtils.h"
+
+namespace {
Review comment:
Blank lines could be added after the start and before the end of namespace https://google.github.io/styleguide/cppguide.html#Vertical_Whitespace
##########
File path: encrypt-config/EncryptConfig.cpp
##########
@@ -0,0 +1,150 @@
+/**
+ * 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.
+ */
+
+#include "EncryptConfig.h"
+
+#include <sodium.h>
+
+#include <stdexcept>
+
+#include "ConfigFile.h"
+#include "ConfigFileEncryptor.h"
+#include "cxxopts.hpp"
+#include "utils/file/FileUtils.h"
+#include "utils/OptionalUtils.h"
+
+namespace {
+constexpr const char* CONF_DIRECTORY_NAME = "conf";
+constexpr const char* BOOTSTRAP_FILE_NAME = "bootstrap.conf";
+constexpr const char* MINIFI_PROPERTIES_FILE_NAME = "minifi.properties";
+constexpr const char* ENCRYPTION_KEY_PROPERTY_NAME = "nifi.bootstrap.sensitive.key";
+} // namespace
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace encrypt_config {
+
+EncryptConfig::EncryptConfig(int argc, char* argv[]) : minifi_home_(parseMinifiHomeFromTheOptions(argc, argv)) {
+ if (sodium_init() < 0) {
+ throw std::runtime_error{"Could not initialize the libsodium library!"};
+ }
+}
+
+std::string EncryptConfig::parseMinifiHomeFromTheOptions(int argc, char* argv[]) {
+ cxxopts::Options options("encrypt-config", "Encrypt sensitive minifi properties");
+ options.add_options()
+ ("h,help", "Shows help")
+ ("m,minifi-home", "The MINIFI_HOME directory", cxxopts::value<std::string>());
+
+ auto parse_result = options.parse(argc, argv);
+
+ if (parse_result.count("help")) {
+ std::cout << options.help() << '\n';
+ std::exit(0);
+ }
+
+ if (parse_result.count("minifi-home")) {
+ return parse_result["minifi-home"].as<std::string>();
+ } else {
+ throw std::runtime_error{"Required parameter missing: --minifi-home"};
+ }
+}
+
+void EncryptConfig::encryptSensitiveProperties() const {
+ utils::crypto::Bytes encryption_key = getEncryptionKey();
+ encryptSensitiveProperties(encryption_key);
Review comment:
This could be merged to a single line.
##########
File path: encrypt-config/ConfigFile.cpp
##########
@@ -0,0 +1,166 @@
+/**
+ * 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.
+ */
+
+#include "ConfigFile.h"
+
+#include <algorithm>
+#include <fstream>
+#include <utility>
+
+#include "utils/StringUtils.h"
+
+namespace {
+constexpr std::array<const char*, 2> DEFAULT_SENSITIVE_PROPERTIES{"nifi.security.client.pass.phrase",
+ "nifi.rest.api.password"};
+constexpr const char* ADDITIONAL_SENSITIVE_PROPS_PROPERTY_NAME = "nifi.sensitive.props.additional.keys";
+} // namespace
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace encrypt_config {
+
+ConfigLine::ConfigLine(std::string line) : line_(line) {
+ line = utils::StringUtils::trim(line);
Review comment:
Maybe we could store the trimmed line instead. I saw you had a comment about leaving the original formatting, and blank lines, but in case of a property file, what's the benefit of keeping the leading and trailing whitespaces?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r504036753
##########
File path: libminifi/include/properties/Properties.h
##########
@@ -65,7 +65,7 @@ class Properties {
* @param value value in which to place the map's stored property value
* @returns true if found, false otherwise.
*/
- bool get(const std::string &key, std::string &value) const;
+ bool getString(const std::string &key, std::string &value) const;
Review comment:
I think that by naming both functions in the parent and child classes `get()`, we would be setting a trap for our future selves, and for anyone who will work on this code. I agree `getString()` is not a great name, but I would prefer to rename it to anything other than `get()`. How about `value()`, `getProperty()`, `getPropertyValue()` or something similar?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r498308693
##########
File path: cmake/BundledLibSodium.cmake
##########
@@ -0,0 +1,95 @@
+# 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.
+
+function(use_bundled_libsodium SOURCE_DIR BINARY_DIR)
+ message("Using bundled libsodium")
+
+ # Define patch step
+ if (WIN32)
+ set(PC "${Patch_EXECUTABLE}" -p1 -i "${SOURCE_DIR}/thirdparty/libsodium/libsodium.patch")
+ endif()
+
+ # Define byproduct
+ if (WIN32)
+ set(BYPRODUCT "lib/sodium.lib")
+ else()
+ set(BYPRODUCT "lib/libsodium.a")
+ endif()
+
+ # Set build options
+ set(LIBSODIUM_BIN_DIR "${BINARY_DIR}/thirdparty/libsodium-install" CACHE STRING "" FORCE)
+
+ if (WIN32)
+ set(LIBSODIUM_CMAKE_ARGS ${PASSTHROUGH_CMAKE_ARGS}
+ "-DCMAKE_INSTALL_PREFIX=${LIBSODIUM_BIN_DIR}"
+ "-DSODIUM_LIBRARY_MINIMAL=1")
+ endif()
+
+ # Build project
+ set(LIBSODIUM_URL https://download.libsodium.org/libsodium/releases/libsodium-1.0.18.tar.gz)
+ set(LIBSODIUM_URL_HASH "SHA256=6f504490b342a4f8a4c4a02fc9b866cbef8622d5df4e5452b46be121e46636c1")
+
+ if (WIN32)
+ ExternalProject_Add(
+ libsodium-external
+ URL ${LIBSODIUM_URL}
+ URL_HASH ${LIBSODIUM_URL_HASH}
+ SOURCE_DIR "${BINARY_DIR}/thirdparty/libsodium-src"
+ LIST_SEPARATOR % # This is needed for passing semicolon-separated lists
+ CMAKE_ARGS ${LIBSODIUM_CMAKE_ARGS}
+ PATCH_COMMAND ${PC}
+ BUILD_BYPRODUCTS "${LIBSODIUM_BIN_DIR}/${BYPRODUCT}"
+ EXCLUDE_FROM_ALL TRUE
+ )
+ else()
+ set(CONFIGURE_COMMAND ./configure --disable-pie --enable-minimal "--prefix=${LIBSODIUM_BIN_DIR}")
+
+ ExternalProject_Add(
+ libsodium-external
+ URL ${LIBSODIUM_URL}
+ URL_HASH ${LIBSODIUM_URL_HASH}
+ BUILD_IN_SOURCE true
+ SOURCE_DIR "${BINARY_DIR}/thirdparty/libsodium-src"
+ BUILD_COMMAND make
+ CMAKE_COMMAND ""
+ UPDATE_COMMAND ""
+ INSTALL_COMMAND make install
+ BUILD_BYPRODUCTS "${LIBSODIUM_BIN_DIR}/${BYPRODUCT}"
+ CONFIGURE_COMMAND "${CONFIGURE_COMMAND}"
+ PATCH_COMMAND ""
+ STEP_TARGETS build
+ EXCLUDE_FROM_ALL TRUE
+ )
+ endif()
+
+ # Set variables
+ set(LIBSODIUM_FOUND "YES" CACHE STRING "" FORCE)
+ set(LIBSODIUM_INCLUDE_DIRS "${LIBSODIUM_BIN_DIR}/include" CACHE STRING "" FORCE)
+ set(LIBSODIUM_LIBRARIES "${LIBSODIUM_BIN_DIR}/${BYPRODUCT}" CACHE STRING "" FORCE)
+
+ # Set exported variables for FindPackage.cmake
+ set(PASSTHROUGH_VARIABLES ${PASSTHROUGH_VARIABLES} "-DEXPORTED_LIBSODIUM_INCLUDE_DIRS=${LIBSODIUM_INCLUDE_DIRS}" CACHE STRING "" FORCE)
+ set(PASSTHROUGH_VARIABLES ${PASSTHROUGH_VARIABLES} "-DEXPORTED_LIBSODIUM_LIBRARIES=${LIBSODIUM_LIBRARIES}" CACHE STRING "" FORCE)
+
+ # Create imported targets
+ file(MAKE_DIRECTORY ${LIBSODIUM_INCLUDE_DIRS})
+
+ add_library(libsodium STATIC IMPORTED)
+ set_target_properties(libsodium PROPERTIES IMPORTED_LOCATION "${LIBSODIUM_LIBRARIES}")
+ add_dependencies(libsodium libsodium-external)
+ set_property(TARGET libsodium APPEND PROPERTY INTERFACE_INCLUDE_DIRECTORIES "${LIBSODIUM_INCLUDE_DIRS}")
Review comment:
Both of these require CMake >= 3.11 (ticket: https://gitlab.kitware.com/cmake/cmake/-/issues/15689, PR: https://gitlab.kitware.com/cmake/cmake/-/merge_requests/1264).
Do you think it's worth bumping the minimum CMake version for this? Ubuntu 18.04 comes with cmake 3.10 by default.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r498055943
##########
File path: libminifi/include/properties/Properties.h
##########
@@ -65,6 +67,13 @@ class Properties {
*/
bool get(const std::string &key, std::string &value) const;
+ /**
+ * Returns the config value.
+ * @param key key to look up
+ * @returns the value if found, otherwise nullopt
+ */
+ utils::optional<std::string> get(const std::string &key) const;
Review comment:
👍
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r498836072
##########
File path: encrypt-config/tests/ConfigFileTests.cpp
##########
@@ -0,0 +1,224 @@
+/**
+ * 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.
+ */
+
+#include "ConfigFile.h"
+
+#include "gsl/gsl-lite.hpp"
+
+#include "TestBase.h"
+#include "utils/file/FileUtils.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace encrypt_config {
+
+class ConfigFileTestAccessor {
+ public:
+ static std::vector<std::string> mergeProperties(std::vector<std::string> left, const std::vector<std::string>& right) {
+ return ConfigFile::mergeProperties(left, right);
+ }
+};
+
+} // namespace encrypt_config
+} // namespace minifi
+} // namespace nifi
+} // namespace apache
+} // namespace org
+
+using org::apache::nifi::minifi::encrypt_config::ConfigFile;
+using org::apache::nifi::minifi::encrypt_config::ConfigFileTestAccessor;
+using org::apache::nifi::minifi::encrypt_config::ConfigLine;
+
+TEST_CASE("ConfigLine can be constructed from a line", "[encrypt-config][constructor]") {
+ auto line_is_parsed_correctly = [](const std::string& line, const std::string& expected_key, const std::string& expected_value) {
+ ConfigLine config_line{line};
+ return config_line.getKey() == expected_key && config_line.getValue() == expected_value;
+ };
+
+ REQUIRE(line_is_parsed_correctly("", "", ""));
+ REQUIRE(line_is_parsed_correctly(" \t \r", "", ""));
+ REQUIRE(line_is_parsed_correctly("#disabled.setting=foo", "", ""));
+ REQUIRE(line_is_parsed_correctly("some line without an equals sign", "", ""));
+ REQUIRE(line_is_parsed_correctly("=value_without_key", "", ""));
+ REQUIRE(line_is_parsed_correctly("\t =value_without_key", "", ""));
+
+ REQUIRE(line_is_parsed_correctly("nifi.some.key=", "nifi.some.key", ""));
+ REQUIRE(line_is_parsed_correctly("nifi.some.key=some_value", "nifi.some.key", "some_value"));
+ REQUIRE(line_is_parsed_correctly("nifi.some.key = some_value", "nifi.some.key", "some_value"));
+ REQUIRE(line_is_parsed_correctly("\tnifi.some.key\t=\tsome_value", "nifi.some.key", "some_value"));
+ REQUIRE(line_is_parsed_correctly("nifi.some.key=some_value \r", "nifi.some.key", "some_value"));
+ REQUIRE(line_is_parsed_correctly("nifi.some.key=some value", "nifi.some.key", "some value"));
+ REQUIRE(line_is_parsed_correctly("nifi.some.key=value=with=equals=signs=", "nifi.some.key", "value=with=equals=signs="));
+}
+
+TEST_CASE("ConfigLine can be constructed from a key-value pair", "[encrypt-config][constructor]") {
+ auto can_construct_from_kv = [](const std::string& key, const std::string& value, const std::string& expected_line) {
+ ConfigLine config_line{key, value};
+ return config_line.getLine() == expected_line;
+ };
+
+ REQUIRE(can_construct_from_kv("nifi.some.key", "", "nifi.some.key="));
+ REQUIRE(can_construct_from_kv("nifi.some.key", "some_value", "nifi.some.key=some_value"));
+}
+
+TEST_CASE("ConfigLine can update the value", "[encrypt-config][updateValue]") {
+ auto can_update_value = [](const std::string& original_line, const std::string& new_value, const std::string& expected_line) {
+ ConfigLine config_line{original_line};
+ config_line.updateValue(new_value);
+ return config_line.getLine() == expected_line;
+ };
+
+ REQUIRE(can_update_value("nifi.some.key=some_value", "new_value", "nifi.some.key=new_value"));
+ REQUIRE(can_update_value("nifi.some.key=", "new_value", "nifi.some.key=new_value"));
+ REQUIRE(can_update_value("nifi.some.key=some_value", "", "nifi.some.key="));
+ REQUIRE(can_update_value("nifi.some.key=some_value", "very_long_new_value", "nifi.some.key=very_long_new_value"));
+
+ // whitespace is preserved in the key, but not in the value
+ REQUIRE(can_update_value("nifi.some.key= some_value", "some_value", "nifi.some.key=some_value"));
+ REQUIRE(can_update_value("nifi.some.key = some_value ", "some_value", "nifi.some.key =some_value"));
+ REQUIRE(can_update_value(" nifi.some.key=some_value", "some_value", " nifi.some.key=some_value"));
+ REQUIRE(can_update_value(" nifi.some.key =\tsome_value\r", "some_value", " nifi.some.key =some_value"));
+}
+
+TEST_CASE("ConfigFile creates an empty object from a nonexistent file", "[encrypt-config][constructor]") {
+ ConfigFile test_file{std::ifstream{"resources/nonexistent-minifi.properties"}};
+ REQUIRE(test_file.size() == 0);
Review comment:
In the case of the `bootstrap.conf` file, I wanted to handle the "file does not exist" case and the "file exists but does not contain the `nifi.bootstrap.sensitive.key` property" case in the same way.
If the `minifi.properties` file either does not exist or is empty, the user will get an error message (EncryptConfig.cpp:135).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r499626690
##########
File path: main/MiNiFiMain.cpp
##########
@@ -208,6 +238,10 @@ int main(int argc, char **argv) {
configure->setHome(minifiHome);
configure->loadConfigureFile(DEFAULT_NIFI_PROPERTIES_FILE);
+ if (containsEncryptedProperties(*configure)) {
+ decryptSensitiveProperties(*configure, minifiHome, *logger);
Review comment:
fixed in https://github.com/apache/nifi-minifi-cpp/pull/914/commits/10259562e26b6be5c8dbd6ba200118dc43f0d1e5
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r498090718
##########
File path: main/MiNiFiMain.cpp
##########
@@ -208,6 +238,10 @@ int main(int argc, char **argv) {
configure->setHome(minifiHome);
configure->loadConfigureFile(DEFAULT_NIFI_PROPERTIES_FILE);
+ if (containsEncryptedProperties(*configure)) {
+ decryptSensitiveProperties(*configure, minifiHome, *logger);
Review comment:
as I see, we update the `configure` object in `decryptSensitiveProperties `, one possible problem with this, is that on a flow update (from a C2 agent) the configurations file is persisted by default (to store the new flow url) this could inadvertently leak the decrypted values
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r498366948
##########
File path: cmake/BundledLibSodium.cmake
##########
@@ -0,0 +1,95 @@
+# 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.
+
+function(use_bundled_libsodium SOURCE_DIR BINARY_DIR)
+ message("Using bundled libsodium")
+
+ # Define patch step
+ if (WIN32)
+ set(PC "${Patch_EXECUTABLE}" -p1 -i "${SOURCE_DIR}/thirdparty/libsodium/libsodium.patch")
+ endif()
+
+ # Define byproduct
+ if (WIN32)
+ set(BYPRODUCT "lib/sodium.lib")
+ else()
+ set(BYPRODUCT "lib/libsodium.a")
+ endif()
+
+ # Set build options
+ set(LIBSODIUM_BIN_DIR "${BINARY_DIR}/thirdparty/libsodium-install" CACHE STRING "" FORCE)
+
+ if (WIN32)
+ set(LIBSODIUM_CMAKE_ARGS ${PASSTHROUGH_CMAKE_ARGS}
+ "-DCMAKE_INSTALL_PREFIX=${LIBSODIUM_BIN_DIR}"
+ "-DSODIUM_LIBRARY_MINIMAL=1")
+ endif()
+
+ # Build project
+ set(LIBSODIUM_URL https://download.libsodium.org/libsodium/releases/libsodium-1.0.18.tar.gz)
+ set(LIBSODIUM_URL_HASH "SHA256=6f504490b342a4f8a4c4a02fc9b866cbef8622d5df4e5452b46be121e46636c1")
+
+ if (WIN32)
+ ExternalProject_Add(
+ libsodium-external
+ URL ${LIBSODIUM_URL}
+ URL_HASH ${LIBSODIUM_URL_HASH}
+ SOURCE_DIR "${BINARY_DIR}/thirdparty/libsodium-src"
+ LIST_SEPARATOR % # This is needed for passing semicolon-separated lists
+ CMAKE_ARGS ${LIBSODIUM_CMAKE_ARGS}
+ PATCH_COMMAND ${PC}
+ BUILD_BYPRODUCTS "${LIBSODIUM_BIN_DIR}/${BYPRODUCT}"
+ EXCLUDE_FROM_ALL TRUE
+ )
+ else()
+ set(CONFIGURE_COMMAND ./configure --disable-pie --enable-minimal "--prefix=${LIBSODIUM_BIN_DIR}")
+
+ ExternalProject_Add(
+ libsodium-external
+ URL ${LIBSODIUM_URL}
+ URL_HASH ${LIBSODIUM_URL_HASH}
+ BUILD_IN_SOURCE true
+ SOURCE_DIR "${BINARY_DIR}/thirdparty/libsodium-src"
+ BUILD_COMMAND make
+ CMAKE_COMMAND ""
+ UPDATE_COMMAND ""
+ INSTALL_COMMAND make install
+ BUILD_BYPRODUCTS "${LIBSODIUM_BIN_DIR}/${BYPRODUCT}"
+ CONFIGURE_COMMAND "${CONFIGURE_COMMAND}"
+ PATCH_COMMAND ""
+ STEP_TARGETS build
+ EXCLUDE_FROM_ALL TRUE
+ )
+ endif()
+
+ # Set variables
+ set(LIBSODIUM_FOUND "YES" CACHE STRING "" FORCE)
+ set(LIBSODIUM_INCLUDE_DIRS "${LIBSODIUM_BIN_DIR}/include" CACHE STRING "" FORCE)
+ set(LIBSODIUM_LIBRARIES "${LIBSODIUM_BIN_DIR}/${BYPRODUCT}" CACHE STRING "" FORCE)
+
+ # Set exported variables for FindPackage.cmake
+ set(PASSTHROUGH_VARIABLES ${PASSTHROUGH_VARIABLES} "-DEXPORTED_LIBSODIUM_INCLUDE_DIRS=${LIBSODIUM_INCLUDE_DIRS}" CACHE STRING "" FORCE)
+ set(PASSTHROUGH_VARIABLES ${PASSTHROUGH_VARIABLES} "-DEXPORTED_LIBSODIUM_LIBRARIES=${LIBSODIUM_LIBRARIES}" CACHE STRING "" FORCE)
+
+ # Create imported targets
+ file(MAKE_DIRECTORY ${LIBSODIUM_INCLUDE_DIRS})
+
+ add_library(libsodium STATIC IMPORTED)
+ set_target_properties(libsodium PROPERTIES IMPORTED_LOCATION "${LIBSODIUM_LIBRARIES}")
+ add_dependencies(libsodium libsodium-external)
+ set_property(TARGET libsodium APPEND PROPERTY INTERFACE_INCLUDE_DIRECTORIES "${LIBSODIUM_INCLUDE_DIRS}")
Review comment:
:disappointed:
No, it's not yet worth bumping the required cmake version IMO, thanks for the references.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r497542525
##########
File path: encrypt-config/EncryptConfig.cpp
##########
@@ -0,0 +1,150 @@
+/**
+ * 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.
+ */
+
+#include "EncryptConfig.h"
+
+#include <sodium.h>
+
+#include <stdexcept>
+
+#include "ConfigFile.h"
+#include "ConfigFileEncryptor.h"
+#include "cxxopts.hpp"
+#include "utils/file/FileUtils.h"
+#include "utils/OptionalUtils.h"
+
+namespace {
+constexpr const char* CONF_DIRECTORY_NAME = "conf";
+constexpr const char* BOOTSTRAP_FILE_NAME = "bootstrap.conf";
+constexpr const char* MINIFI_PROPERTIES_FILE_NAME = "minifi.properties";
+constexpr const char* ENCRYPTION_KEY_PROPERTY_NAME = "nifi.bootstrap.sensitive.key";
+} // namespace
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace encrypt_config {
+
+EncryptConfig::EncryptConfig(int argc, char* argv[]) : minifi_home_(parseMinifiHomeFromTheOptions(argc, argv)) {
+ if (sodium_init() < 0) {
+ throw std::runtime_error{"Could not initialize the libsodium library!"};
+ }
+}
+
+std::string EncryptConfig::parseMinifiHomeFromTheOptions(int argc, char* argv[]) {
+ cxxopts::Options options("encrypt-config", "Encrypt sensitive minifi properties");
+ options.add_options()
+ ("h,help", "Shows help")
+ ("m,minifi-home", "The MINIFI_HOME directory", cxxopts::value<std::string>());
+
+ auto parse_result = options.parse(argc, argv);
+
+ if (parse_result.count("help")) {
+ std::cout << options.help() << '\n';
+ std::exit(0);
+ }
+
+ if (parse_result.count("minifi-home")) {
+ return parse_result["minifi-home"].as<std::string>();
+ } else {
+ throw std::runtime_error{"Required parameter missing: --minifi-home"};
+ }
+}
+
+void EncryptConfig::encryptSensitiveProperties() const {
+ utils::crypto::Bytes encryption_key = getEncryptionKey();
+ encryptSensitiveProperties(encryption_key);
+}
+
+std::string EncryptConfig::bootstrapFilePath() const {
+ return utils::file::FileUtils::concat_path(
+ utils::file::FileUtils::concat_path(minifi_home_, CONF_DIRECTORY_NAME),
+ BOOTSTRAP_FILE_NAME);
+}
+
+std::string EncryptConfig::propertiesFilePath() const {
+ return utils::file::FileUtils::concat_path(
+ utils::file::FileUtils::concat_path(minifi_home_, CONF_DIRECTORY_NAME),
+ MINIFI_PROPERTIES_FILE_NAME);
+}
+
+utils::crypto::Bytes EncryptConfig::getEncryptionKey() const {
+ encrypt_config::ConfigFile bootstrap_file{std::ifstream{bootstrapFilePath()}};
+ utils::optional<std::string> key_from_bootstrap_file = bootstrap_file.getValue(ENCRYPTION_KEY_PROPERTY_NAME);
+
+ if (key_from_bootstrap_file && !key_from_bootstrap_file->empty()) {
+ std::string binary_key = hexDecodeAndValidateKey(*key_from_bootstrap_file);
+ std::cout << "Using the existing encryption key found in " << bootstrapFilePath() << '\n';
+ return utils::crypto::stringToBytes(binary_key);
+ } else {
+ std::cout << "Generating a new encryption key...\n";
+ utils::crypto::Bytes encryption_key = utils::crypto::generateKey();
+ writeEncryptionKeyToBootstrapFile(encryption_key);
+ std::cout << "Wrote the new encryption key to " << bootstrapFilePath() << '\n';
+ return encryption_key;
+ }
+}
+
+std::string EncryptConfig::hexDecodeAndValidateKey(const std::string& key) const {
+ std::string binary_key = utils::StringUtils::from_hex(key);
Review comment:
I'm not sure what the expected behavior would be, but be aware that `from_hex` simply skips non-hex characters in the string, so `from_hex("aT_bZa9") == "\xab\xa9"`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r498809970
##########
File path: encrypt-config/tests/ConfigFileEncryptorTests.cpp
##########
@@ -0,0 +1,128 @@
+/**
+ * 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.
+ */
+
+#include "ConfigFileEncryptor.h"
+
+#include "TestBase.h"
+#include "utils/RegexUtils.h"
+
+using org::apache::nifi::minifi::encrypt_config::ConfigFile;
+using org::apache::nifi::minifi::encrypt_config::encryptSensitivePropertiesInFile;
+
+namespace {
+size_t base64_length(size_t unencoded_length) {
+ return (unencoded_length + 2) / 3 * 4;
+}
+
+bool check_encryption(const ConfigFile& test_file, const std::string& property_name, size_t original_value_length) {
+ utils::optional<std::string> encrypted_value = test_file.getValue(property_name);
+ if (!encrypted_value) { return false; }
+
+ utils::optional<std::string> encryption_type = test_file.getValue(property_name + ".protected");
+ if (!encryption_type || *encryption_type != utils::crypto::EncryptionType::name()) { return false; }
+
+ auto length = base64_length(utils::crypto::EncryptionType::nonceLength()) +
+ utils::crypto::EncryptionType::separator().size() +
+ base64_length(original_value_length + utils::crypto::EncryptionType::macLength());
+ return utils::Regex::matchesFullInput("[0-9A-Za-z/+=|]{" + std::to_string(length) + "}", *encrypted_value);
+}
+} // namespace
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace encrypt_config {
+
+// NOTE(fgerlits): these ==/!= operators are in the test file on purpose, and should not be part of production code,
+// as they take a varying amount of time depending on which character in the line differs, so they would open up
+// our code to timing attacks. If you need == in production code, make sure to compare all pairs of chars/lines.
+bool operator==(const ConfigLine& left, const ConfigLine& right) { return left.getLine() == right.getLine(); }
+bool operator!=(const ConfigLine& left, const ConfigLine& right) { return !(left == right); }
+bool operator==(const ConfigFile& left, const ConfigFile& right) { return left.config_lines_ == right.config_lines_; }
+bool operator!=(const ConfigFile& left, const ConfigFile& right) { return !(left == right); }
Review comment:
ConfigFile contains the plaintext initially, and it gets encrypted later. I don't think there is a risk now, but we want to do the right thing in case this gets exposed to users later. I think this is more about principles: if a string can contain sensitive information, remember not to compare it using the default `==`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r497392257
##########
File path: CMakeLists.txt
##########
@@ -245,6 +245,12 @@ if (NOT OPENSSL_OFF)
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -DOPENSSL_SUPPORT")
endif()
+# libsodium
+include(BundledLibSodium)
+use_bundled_libsodium("${CMAKE_CURRENT_SOURCE_DIR}" "${CMAKE_CURRENT_BINARY_DIR}")
+set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DSODIUM_STATIC=1")
+set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -DSODIUM_STATIC=1")
Review comment:
This should move to `use_bundled_libsodium` as `target_compile_definitions`. If it's required on user code, then it should be public or interface.
```suggestion
```
##########
File path: cmake/BundledLibSodium.cmake
##########
@@ -0,0 +1,95 @@
+# 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.
+
+function(use_bundled_libsodium SOURCE_DIR BINARY_DIR)
+ message("Using bundled libsodium")
+
+ # Define patch step
+ if (WIN32)
+ set(PC "${Patch_EXECUTABLE}" -p1 -i "${SOURCE_DIR}/thirdparty/libsodium/libsodium.patch")
+ endif()
+
+ # Define byproduct
+ if (WIN32)
+ set(BYPRODUCT "lib/sodium.lib")
+ else()
+ set(BYPRODUCT "lib/libsodium.a")
+ endif()
+
+ # Set build options
+ set(LIBSODIUM_BIN_DIR "${BINARY_DIR}/thirdparty/libsodium-install" CACHE STRING "" FORCE)
+
+ if (WIN32)
+ set(LIBSODIUM_CMAKE_ARGS ${PASSTHROUGH_CMAKE_ARGS}
+ "-DCMAKE_INSTALL_PREFIX=${LIBSODIUM_BIN_DIR}"
+ "-DSODIUM_LIBRARY_MINIMAL=1")
+ endif()
+
+ # Build project
+ set(LIBSODIUM_URL https://download.libsodium.org/libsodium/releases/libsodium-1.0.18.tar.gz)
+ set(LIBSODIUM_URL_HASH "SHA256=6f504490b342a4f8a4c4a02fc9b866cbef8622d5df4e5452b46be121e46636c1")
+
+ if (WIN32)
+ ExternalProject_Add(
+ libsodium-external
+ URL ${LIBSODIUM_URL}
+ URL_HASH ${LIBSODIUM_URL_HASH}
+ SOURCE_DIR "${BINARY_DIR}/thirdparty/libsodium-src"
+ LIST_SEPARATOR % # This is needed for passing semicolon-separated lists
+ CMAKE_ARGS ${LIBSODIUM_CMAKE_ARGS}
+ PATCH_COMMAND ${PC}
+ BUILD_BYPRODUCTS "${LIBSODIUM_BIN_DIR}/${BYPRODUCT}"
+ EXCLUDE_FROM_ALL TRUE
+ )
+ else()
+ set(CONFIGURE_COMMAND ./configure --disable-pie --enable-minimal "--prefix=${LIBSODIUM_BIN_DIR}")
+
+ ExternalProject_Add(
+ libsodium-external
+ URL ${LIBSODIUM_URL}
+ URL_HASH ${LIBSODIUM_URL_HASH}
+ BUILD_IN_SOURCE true
+ SOURCE_DIR "${BINARY_DIR}/thirdparty/libsodium-src"
+ BUILD_COMMAND make
+ CMAKE_COMMAND ""
+ UPDATE_COMMAND ""
+ INSTALL_COMMAND make install
+ BUILD_BYPRODUCTS "${LIBSODIUM_BIN_DIR}/${BYPRODUCT}"
+ CONFIGURE_COMMAND "${CONFIGURE_COMMAND}"
+ PATCH_COMMAND ""
+ STEP_TARGETS build
+ EXCLUDE_FROM_ALL TRUE
+ )
+ endif()
+
+ # Set variables
+ set(LIBSODIUM_FOUND "YES" CACHE STRING "" FORCE)
+ set(LIBSODIUM_INCLUDE_DIRS "${LIBSODIUM_BIN_DIR}/include" CACHE STRING "" FORCE)
+ set(LIBSODIUM_LIBRARIES "${LIBSODIUM_BIN_DIR}/${BYPRODUCT}" CACHE STRING "" FORCE)
+
+ # Set exported variables for FindPackage.cmake
+ set(PASSTHROUGH_VARIABLES ${PASSTHROUGH_VARIABLES} "-DEXPORTED_LIBSODIUM_INCLUDE_DIRS=${LIBSODIUM_INCLUDE_DIRS}" CACHE STRING "" FORCE)
+ set(PASSTHROUGH_VARIABLES ${PASSTHROUGH_VARIABLES} "-DEXPORTED_LIBSODIUM_LIBRARIES=${LIBSODIUM_LIBRARIES}" CACHE STRING "" FORCE)
+
+ # Create imported targets
+ file(MAKE_DIRECTORY ${LIBSODIUM_INCLUDE_DIRS})
+
+ add_library(libsodium STATIC IMPORTED)
+ set_target_properties(libsodium PROPERTIES IMPORTED_LOCATION "${LIBSODIUM_LIBRARIES}")
+ add_dependencies(libsodium libsodium-external)
+ set_property(TARGET libsodium APPEND PROPERTY INTERFACE_INCLUDE_DIRECTORIES "${LIBSODIUM_INCLUDE_DIRS}")
Review comment:
```suggestion
target_include_directories(libsodium INTERFACE "${LIBSODIUM_INCLUDE_DIRS}")
target_compile_definitions(libsodium INTERFACE -DSODIUM_STATIC=1)
```
(the first line is a shorter, more readable equivalent)
##########
File path: encrypt-config/EncryptConfig.cpp
##########
@@ -0,0 +1,150 @@
+/**
+ * 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.
+ */
+
+#include "EncryptConfig.h"
+
+#include <sodium.h>
+
+#include <stdexcept>
+
+#include "ConfigFile.h"
+#include "ConfigFileEncryptor.h"
+#include "cxxopts.hpp"
+#include "utils/file/FileUtils.h"
+#include "utils/OptionalUtils.h"
+
+namespace {
+constexpr const char* CONF_DIRECTORY_NAME = "conf";
+constexpr const char* BOOTSTRAP_FILE_NAME = "bootstrap.conf";
+constexpr const char* MINIFI_PROPERTIES_FILE_NAME = "minifi.properties";
+constexpr const char* ENCRYPTION_KEY_PROPERTY_NAME = "nifi.bootstrap.sensitive.key";
+} // namespace
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace encrypt_config {
+
+EncryptConfig::EncryptConfig(int argc, char* argv[]) : minifi_home_(parseMinifiHomeFromTheOptions(argc, argv)) {
+ if (sodium_init() < 0) {
+ throw std::runtime_error{"Could not initialize the libsodium library!"};
+ }
+}
+
+std::string EncryptConfig::parseMinifiHomeFromTheOptions(int argc, char* argv[]) {
+ cxxopts::Options options("encrypt-config", "Encrypt sensitive minifi properties");
+ options.add_options()
+ ("h,help", "Shows help")
+ ("m,minifi-home", "The MINIFI_HOME directory", cxxopts::value<std::string>());
+
+ auto parse_result = options.parse(argc, argv);
+
+ if (parse_result.count("help")) {
+ std::cout << options.help() << '\n';
+ std::exit(0);
+ }
+
+ if (parse_result.count("minifi-home")) {
+ return parse_result["minifi-home"].as<std::string>();
+ } else {
+ throw std::runtime_error{"Required parameter missing: --minifi-home"};
+ }
+}
+
+void EncryptConfig::encryptSensitiveProperties() const {
+ utils::crypto::Bytes encryption_key = getEncryptionKey();
+ encryptSensitiveProperties(encryption_key);
+}
+
+std::string EncryptConfig::bootstrapFilePath() const {
+ return utils::file::FileUtils::concat_path(
+ utils::file::FileUtils::concat_path(minifi_home_, CONF_DIRECTORY_NAME),
+ BOOTSTRAP_FILE_NAME);
+}
+
+std::string EncryptConfig::propertiesFilePath() const {
+ return utils::file::FileUtils::concat_path(
+ utils::file::FileUtils::concat_path(minifi_home_, CONF_DIRECTORY_NAME),
+ MINIFI_PROPERTIES_FILE_NAME);
+}
+
+utils::crypto::Bytes EncryptConfig::getEncryptionKey() const {
+ encrypt_config::ConfigFile bootstrap_file{std::ifstream{bootstrapFilePath()}};
+ utils::optional<std::string> key_from_bootstrap_file = bootstrap_file.getValue(ENCRYPTION_KEY_PROPERTY_NAME);
+
+ if (key_from_bootstrap_file && !key_from_bootstrap_file->empty()) {
+ std::string binary_key = hexDecodeAndValidateKey(*key_from_bootstrap_file);
+ std::cout << "Using the existing encryption key found in " << bootstrapFilePath() << '\n';
+ return utils::crypto::stringToBytes(binary_key);
+ } else {
+ std::cout << "Generating a new encryption key...\n";
+ utils::crypto::Bytes encryption_key = utils::crypto::generateKey();
+ writeEncryptionKeyToBootstrapFile(encryption_key);
+ std::cout << "Wrote the new encryption key to " << bootstrapFilePath() << '\n';
+ return encryption_key;
+ }
+}
+
+std::string EncryptConfig::hexDecodeAndValidateKey(const std::string& key) const {
+ std::string binary_key = utils::StringUtils::from_hex(key);
Review comment:
This is something that should get a comment above the line so that future readers are aware of this decision.
##########
File path: encrypt-config/CMakeLists.txt
##########
@@ -0,0 +1,25 @@
+#
+# 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.
+
+file(GLOB ENCRYPT_CONFIG_FILES "*.cpp")
+add_executable(encrypt-config "${ENCRYPT_CONFIG_FILES}")
+target_include_directories(encrypt-config PRIVATE ../libminifi/include ../thirdparty/cxxopts/include)
+target_wholearchive_library(encrypt-config minifi)
+target_link_libraries(encrypt-config cxxopts)
Review comment:
Not sure why encrypt-config compiles since it's missing a libsodium dependency, therefore it shouldn't have the necessary include paths. Anyway, I suggest adding libsodium here.
```suggestion
target_link_libraries(encrypt-config cxxopts libsodium)
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r498584923
##########
File path: encrypt-config/ConfigFile.cpp
##########
@@ -0,0 +1,166 @@
+/**
+ * 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.
+ */
+
+#include "ConfigFile.h"
+
+#include <algorithm>
+#include <fstream>
+#include <utility>
+
+#include "utils/StringUtils.h"
+
+namespace {
+constexpr std::array<const char*, 2> DEFAULT_SENSITIVE_PROPERTIES{"nifi.security.client.pass.phrase",
Review comment:
You can find all of the properties in `libminifi/include/properties/Configure.h`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r499683494
##########
File path: libminifi/include/properties/Properties.h
##########
@@ -65,7 +65,7 @@ class Properties {
* @param value value in which to place the map's stored property value
* @returns true if found, false otherwise.
*/
- bool get(const std::string &key, std::string &value) const;
+ bool getString(const std::string &key, std::string &value) const;
Review comment:
Instead of renaming this, the updated `Configure` should use base class qualified function call. The `String` part of `getString` is redundant information in context of `Properties`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r497656887
##########
File path: encrypt-config/EncryptConfig.cpp
##########
@@ -0,0 +1,150 @@
+/**
+ * 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.
+ */
+
+#include "EncryptConfig.h"
+
+#include <sodium.h>
+
+#include <stdexcept>
+
+#include "ConfigFile.h"
+#include "ConfigFileEncryptor.h"
+#include "cxxopts.hpp"
+#include "utils/file/FileUtils.h"
+#include "utils/OptionalUtils.h"
+
+namespace {
+constexpr const char* CONF_DIRECTORY_NAME = "conf";
+constexpr const char* BOOTSTRAP_FILE_NAME = "bootstrap.conf";
+constexpr const char* MINIFI_PROPERTIES_FILE_NAME = "minifi.properties";
+constexpr const char* ENCRYPTION_KEY_PROPERTY_NAME = "nifi.bootstrap.sensitive.key";
+} // namespace
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace encrypt_config {
+
+EncryptConfig::EncryptConfig(int argc, char* argv[]) : minifi_home_(parseMinifiHomeFromTheOptions(argc, argv)) {
+ if (sodium_init() < 0) {
+ throw std::runtime_error{"Could not initialize the libsodium library!"};
+ }
+}
+
+std::string EncryptConfig::parseMinifiHomeFromTheOptions(int argc, char* argv[]) {
+ cxxopts::Options options("encrypt-config", "Encrypt sensitive minifi properties");
+ options.add_options()
+ ("h,help", "Shows help")
+ ("m,minifi-home", "The MINIFI_HOME directory", cxxopts::value<std::string>());
+
+ auto parse_result = options.parse(argc, argv);
+
+ if (parse_result.count("help")) {
+ std::cout << options.help() << '\n';
+ std::exit(0);
+ }
+
+ if (parse_result.count("minifi-home")) {
+ return parse_result["minifi-home"].as<std::string>();
+ } else {
+ throw std::runtime_error{"Required parameter missing: --minifi-home"};
+ }
+}
+
+void EncryptConfig::encryptSensitiveProperties() const {
+ utils::crypto::Bytes encryption_key = getEncryptionKey();
+ encryptSensitiveProperties(encryption_key);
+}
+
+std::string EncryptConfig::bootstrapFilePath() const {
+ return utils::file::FileUtils::concat_path(
+ utils::file::FileUtils::concat_path(minifi_home_, CONF_DIRECTORY_NAME),
+ BOOTSTRAP_FILE_NAME);
+}
+
+std::string EncryptConfig::propertiesFilePath() const {
+ return utils::file::FileUtils::concat_path(
+ utils::file::FileUtils::concat_path(minifi_home_, CONF_DIRECTORY_NAME),
+ MINIFI_PROPERTIES_FILE_NAME);
+}
+
+utils::crypto::Bytes EncryptConfig::getEncryptionKey() const {
+ encrypt_config::ConfigFile bootstrap_file{std::ifstream{bootstrapFilePath()}};
+ utils::optional<std::string> key_from_bootstrap_file = bootstrap_file.getValue(ENCRYPTION_KEY_PROPERTY_NAME);
+
+ if (key_from_bootstrap_file && !key_from_bootstrap_file->empty()) {
+ std::string binary_key = hexDecodeAndValidateKey(*key_from_bootstrap_file);
+ std::cout << "Using the existing encryption key found in " << bootstrapFilePath() << '\n';
+ return utils::crypto::stringToBytes(binary_key);
+ } else {
+ std::cout << "Generating a new encryption key...\n";
+ utils::crypto::Bytes encryption_key = utils::crypto::generateKey();
+ writeEncryptionKeyToBootstrapFile(encryption_key);
+ std::cout << "Wrote the new encryption key to " << bootstrapFilePath() << '\n';
+ return encryption_key;
+ }
+}
+
+std::string EncryptConfig::hexDecodeAndValidateKey(const std::string& key) const {
+ std::string binary_key = utils::StringUtils::from_hex(key);
Review comment:
I didn't know that, but I don't think it is a problem. It may even be useful, if we want to use an externally-generated key, to be able to format it using separators, eg. "aa411f289c91685e-f9d5a9e5a4fad939-3ff4c7a78ab97848-4323488caed7a9ab".
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r501848713
##########
File path: encrypt-config/CMakeLists.txt
##########
@@ -0,0 +1,25 @@
+#
+# 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.
+
+file(GLOB ENCRYPT_CONFIG_FILES "*.cpp")
+add_executable(encrypt-config "${ENCRYPT_CONFIG_FILES}")
+target_include_directories(encrypt-config PRIVATE ../libminifi/include ../thirdparty/cxxopts/include)
+target_wholearchive_library(encrypt-config minifi)
+target_link_libraries(encrypt-config cxxopts libsodium)
+set_target_properties(encrypt-config PROPERTIES OUTPUT_NAME encrypt-config)
+install(TARGETS encrypt-config RUNTIME DESTINATION bin COMPONENT bin)
Review comment:
yes, I will, thank you!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r504012960
##########
File path: encrypt-config/ConfigFile.cpp
##########
@@ -0,0 +1,171 @@
+/**
+ * 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.
+ */
+
+#include "ConfigFile.h"
+
+#include <algorithm>
+#include <fstream>
+#include <utility>
+
+#include "utils/StringUtils.h"
+
+namespace {
+constexpr std::array<const char*, 2> DEFAULT_SENSITIVE_PROPERTIES{"nifi.security.client.pass.phrase",
+ "nifi.rest.api.password"};
+constexpr const char* ADDITIONAL_SENSITIVE_PROPS_PROPERTY_NAME = "nifi.sensitive.props.additional.keys";
+} // namespace
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace encrypt_config {
+
+ConfigLine::ConfigLine(std::string line) : line_(line) {
+ line = utils::StringUtils::trim(line);
+ if (line.empty() || line[0] == '#') { return; }
+
+ size_t index_of_first_equals_sign = line.find('=');
+ if (index_of_first_equals_sign == std::string::npos) { return; }
+
+ std::string key = utils::StringUtils::trim(line.substr(0, index_of_first_equals_sign));
+ if (key.empty()) { return; }
+
+ key_ = key;
+ value_ = utils::StringUtils::trim(line.substr(index_of_first_equals_sign + 1));
+}
+
+ConfigLine::ConfigLine(std::string key, std::string value)
+ : line_{utils::StringUtils::join_pack(key, "=", value)}, key_{std::move(key)}, value_{std::move(value)} {
+}
+
+void ConfigLine::updateValue(const std::string& value) {
+ auto pos = line_.find('=');
+ if (pos != std::string::npos) {
+ line_.replace(pos + 1, std::string::npos, value);
+ value_ = value;
+ } else {
+ throw std::invalid_argument{"Cannot update value in config line: it does not contain an = sign!"};
+ }
+}
+
+ConfigFile::ConfigFile(std::istream& input_stream) {
+ std::string line;
+ while (std::getline(input_stream, line)) {
+ config_lines_.push_back(ConfigLine{line});
+ }
+}
+
+ConfigFile::Lines::const_iterator ConfigFile::findKey(const std::string& key) const {
+ return std::find_if(config_lines_.cbegin(), config_lines_.cend(), [&key](const ConfigLine& config_line) {
+ return config_line.getKey() == key;
+ });
+}
+
+ConfigFile::Lines::iterator ConfigFile::findKey(const std::string& key) {
+ return std::find_if(config_lines_.begin(), config_lines_.end(), [&key](const ConfigLine& config_line) {
+ return config_line.getKey() == key;
+ });
+}
+
+bool ConfigFile::hasValue(const std::string& key) const {
+ const auto it = findKey(key);
+ return (it != config_lines_.end());
+}
+
+utils::optional<std::string> ConfigFile::getValue(const std::string& key) const {
+ const auto it = findKey(key);
+ if (it != config_lines_.end()) {
+ return it->getValue();
+ } else {
+ return utils::nullopt;
+ }
+}
+
+void ConfigFile::update(const std::string& key, const std::string& value) {
+ auto it = findKey(key);
+ if (it != config_lines_.end()) {
+ it->updateValue(value);
+ } else {
+ throw std::invalid_argument{"Key " + key + " not found in the config file!"};
+ }
+}
+
+void ConfigFile::insertAfter(const std::string& after_key, const std::string& key, const std::string& value) {
+ auto it = findKey(after_key);
+ if (it != config_lines_.end()) {
+ ++it;
+ config_lines_.emplace(it, key, value);
+ } else {
+ throw std::invalid_argument{"Key " + after_key + " not found in the config file!"};
+ }
+}
+
+void ConfigFile::append(const std::string& key, const std::string& value) {
+ config_lines_.emplace_back(key, value);
+}
+
+int ConfigFile::erase(const std::string& key) {
+ auto has_this_key = [&key](const ConfigLine& line) { return line.getKey() == key; };
+ auto new_end = std::remove_if(config_lines_.begin(), config_lines_.end(), has_this_key);
+ auto num_removed = std::distance(new_end, config_lines_.end());
+ config_lines_.erase(new_end, config_lines_.end());
+ return gsl::narrow<int>(num_removed);
+}
+
+void ConfigFile::writeTo(const std::string& file_path) const {
+ std::ofstream file{file_path};
Review comment:
done: https://github.com/apache/nifi-minifi-cpp/pull/914/commits/ec7277ca5be16565d25738d23f898d7e46608c5c
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r498101379
##########
File path: main/MiNiFiMain.cpp
##########
@@ -208,6 +238,10 @@ int main(int argc, char **argv) {
configure->setHome(minifiHome);
configure->loadConfigureFile(DEFAULT_NIFI_PROPERTIES_FILE);
+ if (containsEncryptedProperties(*configure)) {
+ decryptSensitiveProperties(*configure, minifiHome, *logger);
Review comment:
Ouch. Yes, that is a serious problem.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r498356448
##########
File path: encrypt-config/EncryptConfig.cpp
##########
@@ -0,0 +1,150 @@
+/**
+ * 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.
+ */
+
+#include "EncryptConfig.h"
+
+#include <sodium.h>
+
+#include <stdexcept>
+
+#include "ConfigFile.h"
+#include "ConfigFileEncryptor.h"
+#include "cxxopts.hpp"
+#include "utils/file/FileUtils.h"
+#include "utils/OptionalUtils.h"
+
+namespace {
+constexpr const char* CONF_DIRECTORY_NAME = "conf";
+constexpr const char* BOOTSTRAP_FILE_NAME = "bootstrap.conf";
+constexpr const char* MINIFI_PROPERTIES_FILE_NAME = "minifi.properties";
+constexpr const char* ENCRYPTION_KEY_PROPERTY_NAME = "nifi.bootstrap.sensitive.key";
+} // namespace
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace encrypt_config {
+
+EncryptConfig::EncryptConfig(int argc, char* argv[]) : minifi_home_(parseMinifiHomeFromTheOptions(argc, argv)) {
+ if (sodium_init() < 0) {
+ throw std::runtime_error{"Could not initialize the libsodium library!"};
+ }
+}
+
+std::string EncryptConfig::parseMinifiHomeFromTheOptions(int argc, char* argv[]) {
+ cxxopts::Options options("encrypt-config", "Encrypt sensitive minifi properties");
+ options.add_options()
+ ("h,help", "Shows help")
+ ("m,minifi-home", "The MINIFI_HOME directory", cxxopts::value<std::string>());
+
+ auto parse_result = options.parse(argc, argv);
+
+ if (parse_result.count("help")) {
+ std::cout << options.help() << '\n';
+ std::exit(0);
+ }
+
+ if (parse_result.count("minifi-home")) {
+ return parse_result["minifi-home"].as<std::string>();
+ } else {
+ throw std::runtime_error{"Required parameter missing: --minifi-home"};
+ }
+}
+
+void EncryptConfig::encryptSensitiveProperties() const {
+ utils::crypto::Bytes encryption_key = getEncryptionKey();
+ encryptSensitiveProperties(encryption_key);
+}
+
+std::string EncryptConfig::bootstrapFilePath() const {
+ return utils::file::FileUtils::concat_path(
+ utils::file::FileUtils::concat_path(minifi_home_, CONF_DIRECTORY_NAME),
+ BOOTSTRAP_FILE_NAME);
+}
+
+std::string EncryptConfig::propertiesFilePath() const {
+ return utils::file::FileUtils::concat_path(
+ utils::file::FileUtils::concat_path(minifi_home_, CONF_DIRECTORY_NAME),
+ MINIFI_PROPERTIES_FILE_NAME);
+}
+
+utils::crypto::Bytes EncryptConfig::getEncryptionKey() const {
+ encrypt_config::ConfigFile bootstrap_file{std::ifstream{bootstrapFilePath()}};
+ utils::optional<std::string> key_from_bootstrap_file = bootstrap_file.getValue(ENCRYPTION_KEY_PROPERTY_NAME);
+
+ if (key_from_bootstrap_file && !key_from_bootstrap_file->empty()) {
+ std::string binary_key = hexDecodeAndValidateKey(*key_from_bootstrap_file);
+ std::cout << "Using the existing encryption key found in " << bootstrapFilePath() << '\n';
+ return utils::crypto::stringToBytes(binary_key);
+ } else {
+ std::cout << "Generating a new encryption key...\n";
+ utils::crypto::Bytes encryption_key = utils::crypto::generateKey();
+ writeEncryptionKeyToBootstrapFile(encryption_key);
+ std::cout << "Wrote the new encryption key to " << bootstrapFilePath() << '\n';
+ return encryption_key;
+ }
+}
+
+std::string EncryptConfig::hexDecodeAndValidateKey(const std::string& key) const {
+ std::string binary_key = utils::StringUtils::from_hex(key);
Review comment:
I have added a comment.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r498290405
##########
File path: encrypt-config/ConfigFile.cpp
##########
@@ -0,0 +1,166 @@
+/**
+ * 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.
+ */
+
+#include "ConfigFile.h"
+
+#include <algorithm>
+#include <fstream>
+#include <utility>
+
+#include "utils/StringUtils.h"
+
+namespace {
+constexpr std::array<const char*, 2> DEFAULT_SENSITIVE_PROPERTIES{"nifi.security.client.pass.phrase",
+ "nifi.rest.api.password"};
+constexpr const char* ADDITIONAL_SENSITIVE_PROPS_PROPERTY_NAME = "nifi.sensitive.props.additional.keys";
+} // namespace
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace encrypt_config {
+
+ConfigLine::ConfigLine(std::string line) : line_(line) {
+ line = utils::StringUtils::trim(line);
Review comment:
I feel fairly strongly that we should only update the properties we are encrypting, and not touch the rest of the configuration file. If I run the script and it says "encrypted 1 property", then the diff between the old and new file should be something like
```
-nifi.rest.api.password=password123
+nifi.rest.api.password=iARpRfcyawNeEkEYLcLfatLUABuf9Nq3||OKHK6goZgGPskfKBow7CARSwdhyExPr1xEzE
+nifi.rest.api.password.protected=xsalsa20poly1305
```
and nothing else.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r508418996
##########
File path: libminifi/include/properties/Properties.h
##########
@@ -65,7 +65,7 @@ class Properties {
* @param value value in which to place the map's stored property value
* @returns true if found, false otherwise.
*/
- bool get(const std::string &key, std::string &value) const;
+ bool getString(const std::string &key, std::string &value) const;
Review comment:
After discussion, we agreed to leave it as is, because all of the alternatives were suboptimal.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r501827638
##########
File path: encrypt-config/CMakeLists.txt
##########
@@ -0,0 +1,25 @@
+#
+# 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.
+
+file(GLOB ENCRYPT_CONFIG_FILES "*.cpp")
+add_executable(encrypt-config "${ENCRYPT_CONFIG_FILES}")
+target_include_directories(encrypt-config PRIVATE ../libminifi/include ../thirdparty/cxxopts/include)
+target_wholearchive_library(encrypt-config minifi)
+target_link_libraries(encrypt-config cxxopts libsodium)
+set_target_properties(encrypt-config PROPERTIES OUTPUT_NAME encrypt-config)
+install(TARGETS encrypt-config RUNTIME DESTINATION bin COMPONENT bin)
Review comment:
Multi configuration generators (visual studio) don't save the last build type in the cache, so cpack apparently has no way of knowing that it has to get the binary from the Debug/RelWithDebInfo directory and defaults to Release (the first one AFAIK). Not sure why the issue only happens with `install(TARGETS)`, but the fix is to append pass `-C <configuration>` to cpack.
Could you add this to `win_build_vs.bat`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r498356767
##########
File path: encrypt-config/CMakeLists.txt
##########
@@ -0,0 +1,25 @@
+#
+# 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.
+
+file(GLOB ENCRYPT_CONFIG_FILES "*.cpp")
+add_executable(encrypt-config "${ENCRYPT_CONFIG_FILES}")
+target_include_directories(encrypt-config PRIVATE ../libminifi/include ../thirdparty/cxxopts/include)
+target_wholearchive_library(encrypt-config minifi)
+target_link_libraries(encrypt-config cxxopts)
Review comment:
fixed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r497542525
##########
File path: encrypt-config/EncryptConfig.cpp
##########
@@ -0,0 +1,150 @@
+/**
+ * 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.
+ */
+
+#include "EncryptConfig.h"
+
+#include <sodium.h>
+
+#include <stdexcept>
+
+#include "ConfigFile.h"
+#include "ConfigFileEncryptor.h"
+#include "cxxopts.hpp"
+#include "utils/file/FileUtils.h"
+#include "utils/OptionalUtils.h"
+
+namespace {
+constexpr const char* CONF_DIRECTORY_NAME = "conf";
+constexpr const char* BOOTSTRAP_FILE_NAME = "bootstrap.conf";
+constexpr const char* MINIFI_PROPERTIES_FILE_NAME = "minifi.properties";
+constexpr const char* ENCRYPTION_KEY_PROPERTY_NAME = "nifi.bootstrap.sensitive.key";
+} // namespace
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace encrypt_config {
+
+EncryptConfig::EncryptConfig(int argc, char* argv[]) : minifi_home_(parseMinifiHomeFromTheOptions(argc, argv)) {
+ if (sodium_init() < 0) {
+ throw std::runtime_error{"Could not initialize the libsodium library!"};
+ }
+}
+
+std::string EncryptConfig::parseMinifiHomeFromTheOptions(int argc, char* argv[]) {
+ cxxopts::Options options("encrypt-config", "Encrypt sensitive minifi properties");
+ options.add_options()
+ ("h,help", "Shows help")
+ ("m,minifi-home", "The MINIFI_HOME directory", cxxopts::value<std::string>());
+
+ auto parse_result = options.parse(argc, argv);
+
+ if (parse_result.count("help")) {
+ std::cout << options.help() << '\n';
+ std::exit(0);
+ }
+
+ if (parse_result.count("minifi-home")) {
+ return parse_result["minifi-home"].as<std::string>();
+ } else {
+ throw std::runtime_error{"Required parameter missing: --minifi-home"};
+ }
+}
+
+void EncryptConfig::encryptSensitiveProperties() const {
+ utils::crypto::Bytes encryption_key = getEncryptionKey();
+ encryptSensitiveProperties(encryption_key);
+}
+
+std::string EncryptConfig::bootstrapFilePath() const {
+ return utils::file::FileUtils::concat_path(
+ utils::file::FileUtils::concat_path(minifi_home_, CONF_DIRECTORY_NAME),
+ BOOTSTRAP_FILE_NAME);
+}
+
+std::string EncryptConfig::propertiesFilePath() const {
+ return utils::file::FileUtils::concat_path(
+ utils::file::FileUtils::concat_path(minifi_home_, CONF_DIRECTORY_NAME),
+ MINIFI_PROPERTIES_FILE_NAME);
+}
+
+utils::crypto::Bytes EncryptConfig::getEncryptionKey() const {
+ encrypt_config::ConfigFile bootstrap_file{std::ifstream{bootstrapFilePath()}};
+ utils::optional<std::string> key_from_bootstrap_file = bootstrap_file.getValue(ENCRYPTION_KEY_PROPERTY_NAME);
+
+ if (key_from_bootstrap_file && !key_from_bootstrap_file->empty()) {
+ std::string binary_key = hexDecodeAndValidateKey(*key_from_bootstrap_file);
+ std::cout << "Using the existing encryption key found in " << bootstrapFilePath() << '\n';
+ return utils::crypto::stringToBytes(binary_key);
+ } else {
+ std::cout << "Generating a new encryption key...\n";
+ utils::crypto::Bytes encryption_key = utils::crypto::generateKey();
+ writeEncryptionKeyToBootstrapFile(encryption_key);
+ std::cout << "Wrote the new encryption key to " << bootstrapFilePath() << '\n';
+ return encryption_key;
+ }
+}
+
+std::string EncryptConfig::hexDecodeAndValidateKey(const std::string& key) const {
+ std::string binary_key = utils::StringUtils::from_hex(key);
Review comment:
I'm not sure what the expected behavior would be, but be aware that `from_hex` simply skips non-hex characters in the string, so `from_hex("aT_bZa9Y") == "\xab\xa9"`, this might not be desirable
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r498101379
##########
File path: main/MiNiFiMain.cpp
##########
@@ -208,6 +238,10 @@ int main(int argc, char **argv) {
configure->setHome(minifiHome);
configure->loadConfigureFile(DEFAULT_NIFI_PROPERTIES_FILE);
+ if (containsEncryptedProperties(*configure)) {
+ decryptSensitiveProperties(*configure, minifiHome, *logger);
Review comment:
Ouch. Yes, that is a serious problem.
EDIT: as discussed, persisting the `Configure` object doesn't work at the moment, due to a bug: new properties are added to the `minifi.properties` file, but existing and modified properties are not updated. So the decrypted sensitive properties cannot be leaked right now.
I think the best long-term solution would be not to update the sensitive values in the `Configure` object, but store the key instead, and decrypt the sensitive values on the fly in the getter function.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r498830986
##########
File path: encrypt-config/EncryptConfig.cpp
##########
@@ -0,0 +1,153 @@
+/**
+ * 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.
+ */
+
+#include "EncryptConfig.h"
+
+#include <sodium.h>
+
+#include <stdexcept>
+
+#include "ConfigFile.h"
+#include "ConfigFileEncryptor.h"
+#include "cxxopts.hpp"
+#include "utils/file/FileUtils.h"
+#include "utils/OptionalUtils.h"
+
+namespace {
+
+constexpr const char* CONF_DIRECTORY_NAME = "conf";
+constexpr const char* BOOTSTRAP_FILE_NAME = "bootstrap.conf";
+constexpr const char* MINIFI_PROPERTIES_FILE_NAME = "minifi.properties";
+constexpr const char* ENCRYPTION_KEY_PROPERTY_NAME = "nifi.bootstrap.sensitive.key";
+
+} // namespace
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace encrypt_config {
+
+EncryptConfig::EncryptConfig(int argc, char* argv[]) : minifi_home_(parseMinifiHomeFromTheOptions(argc, argv)) {
+ if (sodium_init() < 0) {
+ throw std::runtime_error{"Could not initialize the libsodium library!"};
+ }
+}
+
+std::string EncryptConfig::parseMinifiHomeFromTheOptions(int argc, char* argv[]) {
+ cxxopts::Options options("encrypt-config", "Encrypt sensitive minifi properties");
+ options.add_options()
+ ("h,help", "Shows help")
+ ("m,minifi-home", "The MINIFI_HOME directory", cxxopts::value<std::string>());
+
+ auto parse_result = options.parse(argc, argv);
+
+ if (parse_result.count("help")) {
+ std::cout << options.help() << '\n';
+ std::exit(0);
+ }
+
+ if (parse_result.count("minifi-home")) {
+ return parse_result["minifi-home"].as<std::string>();
+ } else {
+ throw std::runtime_error{"Required parameter missing: --minifi-home"};
+ }
+}
+
+void EncryptConfig::encryptSensitiveProperties() const {
+ utils::crypto::Bytes encryption_key = getEncryptionKey();
+ encryptSensitiveProperties(encryption_key);
+}
+
+std::string EncryptConfig::bootstrapFilePath() const {
+ return utils::file::concat_path(
+ utils::file::concat_path(minifi_home_, CONF_DIRECTORY_NAME),
+ BOOTSTRAP_FILE_NAME);
+}
+
+std::string EncryptConfig::propertiesFilePath() const {
+ return utils::file::concat_path(
+ utils::file::concat_path(minifi_home_, CONF_DIRECTORY_NAME),
+ MINIFI_PROPERTIES_FILE_NAME);
+}
+
+utils::crypto::Bytes EncryptConfig::getEncryptionKey() const {
+ encrypt_config::ConfigFile bootstrap_file{std::ifstream{bootstrapFilePath()}};
+ utils::optional<std::string> key_from_bootstrap_file = bootstrap_file.getValue(ENCRYPTION_KEY_PROPERTY_NAME);
+
+ if (key_from_bootstrap_file && !key_from_bootstrap_file->empty()) {
+ std::string binary_key = hexDecodeAndValidateKey(*key_from_bootstrap_file);
+ std::cout << "Using the existing encryption key found in " << bootstrapFilePath() << '\n';
+ return utils::crypto::stringToBytes(binary_key);
+ } else {
+ std::cout << "Generating a new encryption key...\n";
+ utils::crypto::Bytes encryption_key = utils::crypto::generateKey();
+ writeEncryptionKeyToBootstrapFile(encryption_key);
+ std::cout << "Wrote the new encryption key to " << bootstrapFilePath() << '\n';
+ return encryption_key;
+ }
+}
+
+std::string EncryptConfig::hexDecodeAndValidateKey(const std::string& key) const {
+ // Note: from_hex() allows [and skips] non-hex characters
+ std::string binary_key = utils::StringUtils::from_hex(key);
+ if (binary_key.size() == utils::crypto::EncryptionType::keyLength()) {
+ return binary_key;
+ } else {
+ std::stringstream error;
+ error << "The encryption key " << ENCRYPTION_KEY_PROPERTY_NAME << " in the bootstrap file\n"
+ << " " << bootstrapFilePath() << '\n'
+ << "is invalid; delete it to generate a new key.";
+ throw std::runtime_error{error.str()};
+ }
+}
+
+void EncryptConfig::writeEncryptionKeyToBootstrapFile(const utils::crypto::Bytes& encryption_key) const {
+ std::string key_encoded = utils::StringUtils::to_hex(utils::crypto::bytesToString(encryption_key));
+ encrypt_config::ConfigFile bootstrap_file{std::ifstream{bootstrapFilePath()}};
+
+ if (bootstrap_file.getValue(ENCRYPTION_KEY_PROPERTY_NAME)) {
Review comment:
I have added `hasValue`, although I'm not sure if the better readability compensates for the increased amount of code.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r498719002
##########
File path: encrypt-config/EncryptConfig.cpp
##########
@@ -0,0 +1,153 @@
+/**
+ * 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.
+ */
+
+#include "EncryptConfig.h"
+
+#include <sodium.h>
+
+#include <stdexcept>
+
+#include "ConfigFile.h"
+#include "ConfigFileEncryptor.h"
+#include "cxxopts.hpp"
+#include "utils/file/FileUtils.h"
+#include "utils/OptionalUtils.h"
+
+namespace {
+
+constexpr const char* CONF_DIRECTORY_NAME = "conf";
+constexpr const char* BOOTSTRAP_FILE_NAME = "bootstrap.conf";
+constexpr const char* MINIFI_PROPERTIES_FILE_NAME = "minifi.properties";
+constexpr const char* ENCRYPTION_KEY_PROPERTY_NAME = "nifi.bootstrap.sensitive.key";
+
+} // namespace
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace encrypt_config {
+
+EncryptConfig::EncryptConfig(int argc, char* argv[]) : minifi_home_(parseMinifiHomeFromTheOptions(argc, argv)) {
+ if (sodium_init() < 0) {
+ throw std::runtime_error{"Could not initialize the libsodium library!"};
+ }
+}
+
+std::string EncryptConfig::parseMinifiHomeFromTheOptions(int argc, char* argv[]) {
+ cxxopts::Options options("encrypt-config", "Encrypt sensitive minifi properties");
+ options.add_options()
+ ("h,help", "Shows help")
+ ("m,minifi-home", "The MINIFI_HOME directory", cxxopts::value<std::string>());
+
+ auto parse_result = options.parse(argc, argv);
+
+ if (parse_result.count("help")) {
+ std::cout << options.help() << '\n';
+ std::exit(0);
+ }
+
+ if (parse_result.count("minifi-home")) {
+ return parse_result["minifi-home"].as<std::string>();
+ } else {
+ throw std::runtime_error{"Required parameter missing: --minifi-home"};
+ }
+}
+
+void EncryptConfig::encryptSensitiveProperties() const {
+ utils::crypto::Bytes encryption_key = getEncryptionKey();
+ encryptSensitiveProperties(encryption_key);
+}
+
+std::string EncryptConfig::bootstrapFilePath() const {
+ return utils::file::concat_path(
+ utils::file::concat_path(minifi_home_, CONF_DIRECTORY_NAME),
+ BOOTSTRAP_FILE_NAME);
+}
+
+std::string EncryptConfig::propertiesFilePath() const {
+ return utils::file::concat_path(
+ utils::file::concat_path(minifi_home_, CONF_DIRECTORY_NAME),
+ MINIFI_PROPERTIES_FILE_NAME);
+}
+
+utils::crypto::Bytes EncryptConfig::getEncryptionKey() const {
+ encrypt_config::ConfigFile bootstrap_file{std::ifstream{bootstrapFilePath()}};
+ utils::optional<std::string> key_from_bootstrap_file = bootstrap_file.getValue(ENCRYPTION_KEY_PROPERTY_NAME);
+
+ if (key_from_bootstrap_file && !key_from_bootstrap_file->empty()) {
+ std::string binary_key = hexDecodeAndValidateKey(*key_from_bootstrap_file);
+ std::cout << "Using the existing encryption key found in " << bootstrapFilePath() << '\n';
+ return utils::crypto::stringToBytes(binary_key);
+ } else {
+ std::cout << "Generating a new encryption key...\n";
+ utils::crypto::Bytes encryption_key = utils::crypto::generateKey();
+ writeEncryptionKeyToBootstrapFile(encryption_key);
+ std::cout << "Wrote the new encryption key to " << bootstrapFilePath() << '\n';
+ return encryption_key;
+ }
+}
+
+std::string EncryptConfig::hexDecodeAndValidateKey(const std::string& key) const {
+ // Note: from_hex() allows [and skips] non-hex characters
+ std::string binary_key = utils::StringUtils::from_hex(key);
+ if (binary_key.size() == utils::crypto::EncryptionType::keyLength()) {
+ return binary_key;
+ } else {
+ std::stringstream error;
+ error << "The encryption key " << ENCRYPTION_KEY_PROPERTY_NAME << " in the bootstrap file\n"
+ << " " << bootstrapFilePath() << '\n'
+ << "is invalid; delete it to generate a new key.";
+ throw std::runtime_error{error.str()};
+ }
+}
+
+void EncryptConfig::writeEncryptionKeyToBootstrapFile(const utils::crypto::Bytes& encryption_key) const {
+ std::string key_encoded = utils::StringUtils::to_hex(utils::crypto::bytesToString(encryption_key));
+ encrypt_config::ConfigFile bootstrap_file{std::ifstream{bootstrapFilePath()}};
+
+ if (bootstrap_file.getValue(ENCRYPTION_KEY_PROPERTY_NAME)) {
Review comment:
`contains`/`present`/`hasKey` in `ConfigFile` would be nice to increase readability.
##########
File path: encrypt-config/tests/ConfigFileTests.cpp
##########
@@ -0,0 +1,224 @@
+/**
+ * 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.
+ */
+
+#include "ConfigFile.h"
+
+#include "gsl/gsl-lite.hpp"
+
+#include "TestBase.h"
+#include "utils/file/FileUtils.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace encrypt_config {
+
+class ConfigFileTestAccessor {
+ public:
+ static std::vector<std::string> mergeProperties(std::vector<std::string> left, const std::vector<std::string>& right) {
+ return ConfigFile::mergeProperties(left, right);
+ }
+};
+
+} // namespace encrypt_config
+} // namespace minifi
+} // namespace nifi
+} // namespace apache
+} // namespace org
+
+using org::apache::nifi::minifi::encrypt_config::ConfigFile;
+using org::apache::nifi::minifi::encrypt_config::ConfigFileTestAccessor;
+using org::apache::nifi::minifi::encrypt_config::ConfigLine;
+
+TEST_CASE("ConfigLine can be constructed from a line", "[encrypt-config][constructor]") {
+ auto line_is_parsed_correctly = [](const std::string& line, const std::string& expected_key, const std::string& expected_value) {
+ ConfigLine config_line{line};
+ return config_line.getKey() == expected_key && config_line.getValue() == expected_value;
+ };
+
+ REQUIRE(line_is_parsed_correctly("", "", ""));
+ REQUIRE(line_is_parsed_correctly(" \t \r", "", ""));
+ REQUIRE(line_is_parsed_correctly("#disabled.setting=foo", "", ""));
+ REQUIRE(line_is_parsed_correctly("some line without an equals sign", "", ""));
+ REQUIRE(line_is_parsed_correctly("=value_without_key", "", ""));
+ REQUIRE(line_is_parsed_correctly("\t =value_without_key", "", ""));
+
+ REQUIRE(line_is_parsed_correctly("nifi.some.key=", "nifi.some.key", ""));
+ REQUIRE(line_is_parsed_correctly("nifi.some.key=some_value", "nifi.some.key", "some_value"));
+ REQUIRE(line_is_parsed_correctly("nifi.some.key = some_value", "nifi.some.key", "some_value"));
+ REQUIRE(line_is_parsed_correctly("\tnifi.some.key\t=\tsome_value", "nifi.some.key", "some_value"));
+ REQUIRE(line_is_parsed_correctly("nifi.some.key=some_value \r", "nifi.some.key", "some_value"));
+ REQUIRE(line_is_parsed_correctly("nifi.some.key=some value", "nifi.some.key", "some value"));
+ REQUIRE(line_is_parsed_correctly("nifi.some.key=value=with=equals=signs=", "nifi.some.key", "value=with=equals=signs="));
+}
+
+TEST_CASE("ConfigLine can be constructed from a key-value pair", "[encrypt-config][constructor]") {
+ auto can_construct_from_kv = [](const std::string& key, const std::string& value, const std::string& expected_line) {
+ ConfigLine config_line{key, value};
+ return config_line.getLine() == expected_line;
+ };
+
+ REQUIRE(can_construct_from_kv("nifi.some.key", "", "nifi.some.key="));
+ REQUIRE(can_construct_from_kv("nifi.some.key", "some_value", "nifi.some.key=some_value"));
+}
+
+TEST_CASE("ConfigLine can update the value", "[encrypt-config][updateValue]") {
+ auto can_update_value = [](const std::string& original_line, const std::string& new_value, const std::string& expected_line) {
+ ConfigLine config_line{original_line};
+ config_line.updateValue(new_value);
+ return config_line.getLine() == expected_line;
+ };
+
+ REQUIRE(can_update_value("nifi.some.key=some_value", "new_value", "nifi.some.key=new_value"));
+ REQUIRE(can_update_value("nifi.some.key=", "new_value", "nifi.some.key=new_value"));
+ REQUIRE(can_update_value("nifi.some.key=some_value", "", "nifi.some.key="));
+ REQUIRE(can_update_value("nifi.some.key=some_value", "very_long_new_value", "nifi.some.key=very_long_new_value"));
+
+ // whitespace is preserved in the key, but not in the value
+ REQUIRE(can_update_value("nifi.some.key= some_value", "some_value", "nifi.some.key=some_value"));
+ REQUIRE(can_update_value("nifi.some.key = some_value ", "some_value", "nifi.some.key =some_value"));
+ REQUIRE(can_update_value(" nifi.some.key=some_value", "some_value", " nifi.some.key=some_value"));
+ REQUIRE(can_update_value(" nifi.some.key =\tsome_value\r", "some_value", " nifi.some.key =some_value"));
+}
+
+TEST_CASE("ConfigFile creates an empty object from a nonexistent file", "[encrypt-config][constructor]") {
+ ConfigFile test_file{std::ifstream{"resources/nonexistent-minifi.properties"}};
+ REQUIRE(test_file.size() == 0);
Review comment:
Shouldn't it throw? I mean why would you want to encrypt something that's not there? It's probably a user error and we shouldn't silently do nothing IMO.
##########
File path: encrypt-config/tests/ConfigFileTests.cpp
##########
@@ -0,0 +1,224 @@
+/**
+ * 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.
+ */
+
+#include "ConfigFile.h"
+
+#include "gsl/gsl-lite.hpp"
+
+#include "TestBase.h"
+#include "utils/file/FileUtils.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace encrypt_config {
+
+class ConfigFileTestAccessor {
+ public:
+ static std::vector<std::string> mergeProperties(std::vector<std::string> left, const std::vector<std::string>& right) {
+ return ConfigFile::mergeProperties(left, right);
+ }
+};
+
+} // namespace encrypt_config
+} // namespace minifi
+} // namespace nifi
+} // namespace apache
+} // namespace org
+
+using org::apache::nifi::minifi::encrypt_config::ConfigFile;
+using org::apache::nifi::minifi::encrypt_config::ConfigFileTestAccessor;
+using org::apache::nifi::minifi::encrypt_config::ConfigLine;
+
+TEST_CASE("ConfigLine can be constructed from a line", "[encrypt-config][constructor]") {
+ auto line_is_parsed_correctly = [](const std::string& line, const std::string& expected_key, const std::string& expected_value) {
+ ConfigLine config_line{line};
+ return config_line.getKey() == expected_key && config_line.getValue() == expected_value;
+ };
+
+ REQUIRE(line_is_parsed_correctly("", "", ""));
+ REQUIRE(line_is_parsed_correctly(" \t \r", "", ""));
+ REQUIRE(line_is_parsed_correctly("#disabled.setting=foo", "", ""));
+ REQUIRE(line_is_parsed_correctly("some line without an equals sign", "", ""));
+ REQUIRE(line_is_parsed_correctly("=value_without_key", "", ""));
+ REQUIRE(line_is_parsed_correctly("\t =value_without_key", "", ""));
+
+ REQUIRE(line_is_parsed_correctly("nifi.some.key=", "nifi.some.key", ""));
+ REQUIRE(line_is_parsed_correctly("nifi.some.key=some_value", "nifi.some.key", "some_value"));
+ REQUIRE(line_is_parsed_correctly("nifi.some.key = some_value", "nifi.some.key", "some_value"));
+ REQUIRE(line_is_parsed_correctly("\tnifi.some.key\t=\tsome_value", "nifi.some.key", "some_value"));
+ REQUIRE(line_is_parsed_correctly("nifi.some.key=some_value \r", "nifi.some.key", "some_value"));
+ REQUIRE(line_is_parsed_correctly("nifi.some.key=some value", "nifi.some.key", "some value"));
+ REQUIRE(line_is_parsed_correctly("nifi.some.key=value=with=equals=signs=", "nifi.some.key", "value=with=equals=signs="));
+}
+
+TEST_CASE("ConfigLine can be constructed from a key-value pair", "[encrypt-config][constructor]") {
+ auto can_construct_from_kv = [](const std::string& key, const std::string& value, const std::string& expected_line) {
+ ConfigLine config_line{key, value};
+ return config_line.getLine() == expected_line;
+ };
+
+ REQUIRE(can_construct_from_kv("nifi.some.key", "", "nifi.some.key="));
+ REQUIRE(can_construct_from_kv("nifi.some.key", "some_value", "nifi.some.key=some_value"));
+}
+
+TEST_CASE("ConfigLine can update the value", "[encrypt-config][updateValue]") {
+ auto can_update_value = [](const std::string& original_line, const std::string& new_value, const std::string& expected_line) {
+ ConfigLine config_line{original_line};
+ config_line.updateValue(new_value);
+ return config_line.getLine() == expected_line;
+ };
+
+ REQUIRE(can_update_value("nifi.some.key=some_value", "new_value", "nifi.some.key=new_value"));
+ REQUIRE(can_update_value("nifi.some.key=", "new_value", "nifi.some.key=new_value"));
+ REQUIRE(can_update_value("nifi.some.key=some_value", "", "nifi.some.key="));
+ REQUIRE(can_update_value("nifi.some.key=some_value", "very_long_new_value", "nifi.some.key=very_long_new_value"));
+
+ // whitespace is preserved in the key, but not in the value
+ REQUIRE(can_update_value("nifi.some.key= some_value", "some_value", "nifi.some.key=some_value"));
+ REQUIRE(can_update_value("nifi.some.key = some_value ", "some_value", "nifi.some.key =some_value"));
+ REQUIRE(can_update_value(" nifi.some.key=some_value", "some_value", " nifi.some.key=some_value"));
+ REQUIRE(can_update_value(" nifi.some.key =\tsome_value\r", "some_value", " nifi.some.key =some_value"));
+}
+
+TEST_CASE("ConfigFile creates an empty object from a nonexistent file", "[encrypt-config][constructor]") {
+ ConfigFile test_file{std::ifstream{"resources/nonexistent-minifi.properties"}};
+ REQUIRE(test_file.size() == 0);
+}
+
+TEST_CASE("ConfigFile can parse a simple config file", "[encrypt-config][constructor]") {
+ ConfigFile test_file{std::ifstream{"resources/minifi.properties"}};
+ REQUIRE(test_file.size() == 101);
+}
+
+TEST_CASE("ConfigFile can read empty properties correctly", "[encrypt-config][constructor]") {
+ ConfigFile test_file{std::ifstream{"resources/with-additional-sensitive-props.minifi.properties"}};
+ REQUIRE(test_file.size() == 103);
+
+ auto empty_property = test_file.getValue("nifi.security.need.ClientAuth");
+ REQUIRE(empty_property);
+ REQUIRE(empty_property->empty());
+
+ auto whitespace_property = test_file.getValue("nifi.security.client.certificate"); // value = " \t\r"
+ REQUIRE(whitespace_property);
+ REQUIRE(whitespace_property->empty());
+}
+
+TEST_CASE("ConfigFile can find the value for a key", "[encrypt-config][getValue]") {
+ ConfigFile test_file{std::ifstream{"resources/minifi.properties"}};
+
+ SECTION("valid key") {
+ REQUIRE(test_file.getValue("nifi.bored.yield.duration") == utils::optional<std::string>{"10 millis"});
+ }
+
+ SECTION("nonexistent key") {
+ REQUIRE(test_file.getValue("nifi.bored.panda") == utils::nullopt);
+ }
+}
+
+TEST_CASE("ConfigFile can update the value for a key", "[encrypt-config][update]") {
+ ConfigFile test_file{std::ifstream{"resources/minifi.properties"}};
+
+ SECTION("valid key") {
+ test_file.update("nifi.bored.yield.duration", "20 millis");
+ REQUIRE(test_file.getValue("nifi.bored.yield.duration") == utils::optional<std::string>{"20 millis"});
+ }
+
+ SECTION("nonexistent key") {
+ REQUIRE_THROWS(test_file.update("nifi.bored.panda", "cat video"));
+ }
+}
+
+TEST_CASE("ConfigFile can add a new setting after an existing setting", "[encrypt-config][insertAfter]") {
+ ConfigFile test_file{std::ifstream{"resources/minifi.properties"}};
+
+ SECTION("valid key") {
+ test_file.insertAfter("nifi.rest.api.password", "nifi.rest.api.password.protected", "XChaCha20-Poly1305");
+ REQUIRE(test_file.size() == 102);
+ REQUIRE(test_file.getValue("nifi.rest.api.password.protected") == utils::optional<std::string>{"XChaCha20-Poly1305"});
Review comment:
Not a huge deal, but now it's salsa, not chacha :dancers:
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r499760951
##########
File path: libminifi/include/properties/Properties.h
##########
@@ -65,7 +65,7 @@ class Properties {
* @param value value in which to place the map's stored property value
* @returns true if found, false otherwise.
*/
- bool get(const std::string &key, std::string &value) const;
+ bool getString(const std::string &key, std::string &value) const;
Review comment:
This way it is easier to see which function is getting called. We don't need `get()` to be virtual, and hiding a non-virtual function isn't nice. Also, Properties has a `getInt()` method, so `getString()` makes sense.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r499687851
##########
File path: libminifi/src/Decryptor.cpp
##########
@@ -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.
+ */
+#include "properties/Decryptor.h"
+
+#include "properties/Properties.h"
+#include "utils/OptionalUtils.h"
+#include "utils/StringUtils.h"
+
+namespace {
+
+#ifdef WIN32
+constexpr const char* DEFAULT_NIFI_BOOTSTRAP_FILE = "\\conf\\bootstrap.conf";
+#else
+constexpr const char* DEFAULT_NIFI_BOOTSTRAP_FILE = "./conf/bootstrap.conf";
+#endif // WIN32
+
+constexpr const char* CONFIG_ENCRYPTION_KEY_PROPERTY_NAME = "nifi.bootstrap.sensitive.key";
+
+} // namespace
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+
+utils::optional<minifi::Decryptor> Decryptor::create(const std::string& minifi_home) {
+ minifi::Properties bootstrap_conf;
+ bootstrap_conf.setHome(minifi_home);
+ bootstrap_conf.loadConfigureFile(DEFAULT_NIFI_BOOTSTRAP_FILE);
+ return bootstrap_conf.getString(CONFIG_ENCRYPTION_KEY_PROPERTY_NAME)
+ | utils::map([](const std::string& encryption_key_hex) { return utils::StringUtils::from_hex(encryption_key_hex); })
+ | utils::map([](const std::string& encryption_key) { return utils::crypto::stringToBytes(encryption_key); })
Review comment:
Sadly overloaded functions (like `from_hex`) and constructor calls (like `Decryptor`) are not [`Callable`](https://en.cppreference.com/w/cpp/named_req/Callable), but function pointers are, so this line can be simplified:
```suggestion
| utils::map(&utils::crypto::stringToBytes)
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r501824449
##########
File path: encrypt-config/ConfigFile.cpp
##########
@@ -0,0 +1,171 @@
+/**
+ * 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.
+ */
+
+#include "ConfigFile.h"
+
+#include <algorithm>
+#include <fstream>
+#include <utility>
+
+#include "utils/StringUtils.h"
+
+namespace {
+constexpr std::array<const char*, 2> DEFAULT_SENSITIVE_PROPERTIES{"nifi.security.client.pass.phrase",
+ "nifi.rest.api.password"};
+constexpr const char* ADDITIONAL_SENSITIVE_PROPS_PROPERTY_NAME = "nifi.sensitive.props.additional.keys";
+} // namespace
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace encrypt_config {
+
+ConfigLine::ConfigLine(std::string line) : line_(line) {
+ line = utils::StringUtils::trim(line);
+ if (line.empty() || line[0] == '#') { return; }
+
+ size_t index_of_first_equals_sign = line.find('=');
+ if (index_of_first_equals_sign == std::string::npos) { return; }
+
+ std::string key = utils::StringUtils::trim(line.substr(0, index_of_first_equals_sign));
+ if (key.empty()) { return; }
+
+ key_ = key;
+ value_ = utils::StringUtils::trim(line.substr(index_of_first_equals_sign + 1));
+}
+
+ConfigLine::ConfigLine(std::string key, std::string value)
+ : line_{utils::StringUtils::join_pack(key, "=", value)}, key_{std::move(key)}, value_{std::move(value)} {
+}
+
+void ConfigLine::updateValue(const std::string& value) {
+ auto pos = line_.find('=');
+ if (pos != std::string::npos) {
+ line_.replace(pos + 1, std::string::npos, value);
+ value_ = value;
+ } else {
+ throw std::invalid_argument{"Cannot update value in config line: it does not contain an = sign!"};
+ }
+}
+
+ConfigFile::ConfigFile(std::istream& input_stream) {
+ std::string line;
+ while (std::getline(input_stream, line)) {
+ config_lines_.push_back(ConfigLine{line});
+ }
+}
+
+ConfigFile::Lines::const_iterator ConfigFile::findKey(const std::string& key) const {
+ return std::find_if(config_lines_.cbegin(), config_lines_.cend(), [&key](const ConfigLine& config_line) {
+ return config_line.getKey() == key;
+ });
+}
+
+ConfigFile::Lines::iterator ConfigFile::findKey(const std::string& key) {
+ return std::find_if(config_lines_.begin(), config_lines_.end(), [&key](const ConfigLine& config_line) {
+ return config_line.getKey() == key;
+ });
+}
+
+bool ConfigFile::hasValue(const std::string& key) const {
+ const auto it = findKey(key);
+ return (it != config_lines_.end());
+}
+
+utils::optional<std::string> ConfigFile::getValue(const std::string& key) const {
+ const auto it = findKey(key);
+ if (it != config_lines_.end()) {
+ return it->getValue();
+ } else {
+ return utils::nullopt;
+ }
+}
+
+void ConfigFile::update(const std::string& key, const std::string& value) {
+ auto it = findKey(key);
+ if (it != config_lines_.end()) {
+ it->updateValue(value);
+ } else {
+ throw std::invalid_argument{"Key " + key + " not found in the config file!"};
+ }
+}
+
+void ConfigFile::insertAfter(const std::string& after_key, const std::string& key, const std::string& value) {
+ auto it = findKey(after_key);
+ if (it != config_lines_.end()) {
+ ++it;
+ config_lines_.emplace(it, key, value);
+ } else {
+ throw std::invalid_argument{"Key " + after_key + " not found in the config file!"};
+ }
+}
+
+void ConfigFile::append(const std::string& key, const std::string& value) {
+ config_lines_.emplace_back(key, value);
+}
+
+int ConfigFile::erase(const std::string& key) {
+ auto has_this_key = [&key](const ConfigLine& line) { return line.getKey() == key; };
+ auto new_end = std::remove_if(config_lines_.begin(), config_lines_.end(), has_this_key);
+ auto num_removed = std::distance(new_end, config_lines_.end());
+ config_lines_.erase(new_end, config_lines_.end());
+ return gsl::narrow<int>(num_removed);
+}
+
+void ConfigFile::writeTo(const std::string& file_path) const {
+ std::ofstream file{file_path};
Review comment:
`std::ofstream` doesn't throw on error, just sets error bits by default. This hides errors and causes me as a user to not realize that despite the success output, nothing has changed because I was not running as admin.
Luckily its error reporting behavior can be configured.
```suggestion
std::ofstream file{file_path};
file.exceptions(std::ios::failbit | std::ios::badbit);
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r498290976
##########
File path: encrypt-config/EncryptConfig.cpp
##########
@@ -0,0 +1,150 @@
+/**
+ * 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.
+ */
+
+#include "EncryptConfig.h"
+
+#include <sodium.h>
+
+#include <stdexcept>
+
+#include "ConfigFile.h"
+#include "ConfigFileEncryptor.h"
+#include "cxxopts.hpp"
+#include "utils/file/FileUtils.h"
+#include "utils/OptionalUtils.h"
+
+namespace {
+constexpr const char* CONF_DIRECTORY_NAME = "conf";
+constexpr const char* BOOTSTRAP_FILE_NAME = "bootstrap.conf";
+constexpr const char* MINIFI_PROPERTIES_FILE_NAME = "minifi.properties";
+constexpr const char* ENCRYPTION_KEY_PROPERTY_NAME = "nifi.bootstrap.sensitive.key";
+} // namespace
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace encrypt_config {
+
+EncryptConfig::EncryptConfig(int argc, char* argv[]) : minifi_home_(parseMinifiHomeFromTheOptions(argc, argv)) {
+ if (sodium_init() < 0) {
+ throw std::runtime_error{"Could not initialize the libsodium library!"};
+ }
+}
+
+std::string EncryptConfig::parseMinifiHomeFromTheOptions(int argc, char* argv[]) {
+ cxxopts::Options options("encrypt-config", "Encrypt sensitive minifi properties");
+ options.add_options()
+ ("h,help", "Shows help")
+ ("m,minifi-home", "The MINIFI_HOME directory", cxxopts::value<std::string>());
+
+ auto parse_result = options.parse(argc, argv);
+
+ if (parse_result.count("help")) {
+ std::cout << options.help() << '\n';
+ std::exit(0);
+ }
+
+ if (parse_result.count("minifi-home")) {
+ return parse_result["minifi-home"].as<std::string>();
+ } else {
+ throw std::runtime_error{"Required parameter missing: --minifi-home"};
+ }
+}
+
+void EncryptConfig::encryptSensitiveProperties() const {
+ utils::crypto::Bytes encryption_key = getEncryptionKey();
+ encryptSensitiveProperties(encryption_key);
Review comment:
I prefer one line to do one thing, and the compiler will optimize it away in any case.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r498309792
##########
File path: CMakeLists.txt
##########
@@ -245,6 +245,12 @@ if (NOT OPENSSL_OFF)
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -DOPENSSL_SUPPORT")
endif()
+# libsodium
+include(BundledLibSodium)
+use_bundled_libsodium("${CMAKE_CURRENT_SOURCE_DIR}" "${CMAKE_CURRENT_BINARY_DIR}")
+set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DSODIUM_STATIC=1")
+set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -DSODIUM_STATIC=1")
Review comment:
requires CMake >= 3.11, see below
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r498804163
##########
File path: encrypt-config/tests/ConfigFileEncryptorTests.cpp
##########
@@ -0,0 +1,128 @@
+/**
+ * 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.
+ */
+
+#include "ConfigFileEncryptor.h"
+
+#include "TestBase.h"
+#include "utils/RegexUtils.h"
+
+using org::apache::nifi::minifi::encrypt_config::ConfigFile;
+using org::apache::nifi::minifi::encrypt_config::encryptSensitivePropertiesInFile;
+
+namespace {
+size_t base64_length(size_t unencoded_length) {
+ return (unencoded_length + 2) / 3 * 4;
+}
+
+bool check_encryption(const ConfigFile& test_file, const std::string& property_name, size_t original_value_length) {
+ utils::optional<std::string> encrypted_value = test_file.getValue(property_name);
+ if (!encrypted_value) { return false; }
+
+ utils::optional<std::string> encryption_type = test_file.getValue(property_name + ".protected");
+ if (!encryption_type || *encryption_type != utils::crypto::EncryptionType::name()) { return false; }
+
+ auto length = base64_length(utils::crypto::EncryptionType::nonceLength()) +
+ utils::crypto::EncryptionType::separator().size() +
+ base64_length(original_value_length + utils::crypto::EncryptionType::macLength());
+ return utils::Regex::matchesFullInput("[0-9A-Za-z/+=|]{" + std::to_string(length) + "}", *encrypted_value);
+}
+} // namespace
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace encrypt_config {
+
+// NOTE(fgerlits): these ==/!= operators are in the test file on purpose, and should not be part of production code,
+// as they take a varying amount of time depending on which character in the line differs, so they would open up
+// our code to timing attacks. If you need == in production code, make sure to compare all pairs of chars/lines.
+bool operator==(const ConfigLine& left, const ConfigLine& right) { return left.getLine() == right.getLine(); }
+bool operator!=(const ConfigLine& left, const ConfigLine& right) { return !(left == right); }
+bool operator==(const ConfigFile& left, const ConfigFile& right) { return left.config_lines_ == right.config_lines_; }
+bool operator!=(const ConfigFile& left, const ConfigFile& right) { return !(left == right); }
Review comment:
Is this a real risk? `ConfigLine` and `ConfigFile` contain the properties file with the sensitive properties in ciphertext, so it's not sensitive IMO.
I'm not asking this to be moved back, just want to know how real the risk is.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r497656887
##########
File path: encrypt-config/EncryptConfig.cpp
##########
@@ -0,0 +1,150 @@
+/**
+ * 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.
+ */
+
+#include "EncryptConfig.h"
+
+#include <sodium.h>
+
+#include <stdexcept>
+
+#include "ConfigFile.h"
+#include "ConfigFileEncryptor.h"
+#include "cxxopts.hpp"
+#include "utils/file/FileUtils.h"
+#include "utils/OptionalUtils.h"
+
+namespace {
+constexpr const char* CONF_DIRECTORY_NAME = "conf";
+constexpr const char* BOOTSTRAP_FILE_NAME = "bootstrap.conf";
+constexpr const char* MINIFI_PROPERTIES_FILE_NAME = "minifi.properties";
+constexpr const char* ENCRYPTION_KEY_PROPERTY_NAME = "nifi.bootstrap.sensitive.key";
+} // namespace
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace encrypt_config {
+
+EncryptConfig::EncryptConfig(int argc, char* argv[]) : minifi_home_(parseMinifiHomeFromTheOptions(argc, argv)) {
+ if (sodium_init() < 0) {
+ throw std::runtime_error{"Could not initialize the libsodium library!"};
+ }
+}
+
+std::string EncryptConfig::parseMinifiHomeFromTheOptions(int argc, char* argv[]) {
+ cxxopts::Options options("encrypt-config", "Encrypt sensitive minifi properties");
+ options.add_options()
+ ("h,help", "Shows help")
+ ("m,minifi-home", "The MINIFI_HOME directory", cxxopts::value<std::string>());
+
+ auto parse_result = options.parse(argc, argv);
+
+ if (parse_result.count("help")) {
+ std::cout << options.help() << '\n';
+ std::exit(0);
+ }
+
+ if (parse_result.count("minifi-home")) {
+ return parse_result["minifi-home"].as<std::string>();
+ } else {
+ throw std::runtime_error{"Required parameter missing: --minifi-home"};
+ }
+}
+
+void EncryptConfig::encryptSensitiveProperties() const {
+ utils::crypto::Bytes encryption_key = getEncryptionKey();
+ encryptSensitiveProperties(encryption_key);
+}
+
+std::string EncryptConfig::bootstrapFilePath() const {
+ return utils::file::FileUtils::concat_path(
+ utils::file::FileUtils::concat_path(minifi_home_, CONF_DIRECTORY_NAME),
+ BOOTSTRAP_FILE_NAME);
+}
+
+std::string EncryptConfig::propertiesFilePath() const {
+ return utils::file::FileUtils::concat_path(
+ utils::file::FileUtils::concat_path(minifi_home_, CONF_DIRECTORY_NAME),
+ MINIFI_PROPERTIES_FILE_NAME);
+}
+
+utils::crypto::Bytes EncryptConfig::getEncryptionKey() const {
+ encrypt_config::ConfigFile bootstrap_file{std::ifstream{bootstrapFilePath()}};
+ utils::optional<std::string> key_from_bootstrap_file = bootstrap_file.getValue(ENCRYPTION_KEY_PROPERTY_NAME);
+
+ if (key_from_bootstrap_file && !key_from_bootstrap_file->empty()) {
+ std::string binary_key = hexDecodeAndValidateKey(*key_from_bootstrap_file);
+ std::cout << "Using the existing encryption key found in " << bootstrapFilePath() << '\n';
+ return utils::crypto::stringToBytes(binary_key);
+ } else {
+ std::cout << "Generating a new encryption key...\n";
+ utils::crypto::Bytes encryption_key = utils::crypto::generateKey();
+ writeEncryptionKeyToBootstrapFile(encryption_key);
+ std::cout << "Wrote the new encryption key to " << bootstrapFilePath() << '\n';
+ return encryption_key;
+ }
+}
+
+std::string EncryptConfig::hexDecodeAndValidateKey(const std::string& key) const {
+ std::string binary_key = utils::StringUtils::from_hex(key);
Review comment:
I think that's fine. It may even be useful, if we want to use an externally-generated key, to be able to format it using separators, eg. "aa411f289c91685e-f9d5a9e5a4fad939-3ff4c7a78ab97848-4323488caed7a9ab".
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r501673502
##########
File path: libminifi/include/properties/Properties.h
##########
@@ -65,7 +65,7 @@ class Properties {
* @param value value in which to place the map's stored property value
* @returns true if found, false otherwise.
*/
- bool get(const std::string &key, std::string &value) const;
+ bool getString(const std::string &key, std::string &value) const;
Review comment:
> We don't need get() to be virtual, and hiding a non-virtual function isn't nice.
I don't think hiding is a big issue. What I don't like is that a change in a derived class forces the parent class to change in an undesirable way.
> Properties has a getInt() method, so getString() makes sense.
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Ro-equivalent
Also, getInt isn't too important, just a convenience overload from the times before optional and map. `Properties` is a string key-string value store, so I as a user shouldn't need to say `getString` when retrieving a value, because I already said that by using the `Properties` class.
##########
File path: encrypt-config/ConfigFile.cpp
##########
@@ -0,0 +1,171 @@
+/**
+ * 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.
+ */
+
+#include "ConfigFile.h"
+
+#include <algorithm>
+#include <fstream>
+#include <utility>
+
+#include "utils/StringUtils.h"
+
+namespace {
+constexpr std::array<const char*, 2> DEFAULT_SENSITIVE_PROPERTIES{"nifi.security.client.pass.phrase",
+ "nifi.rest.api.password"};
+constexpr const char* ADDITIONAL_SENSITIVE_PROPS_PROPERTY_NAME = "nifi.sensitive.props.additional.keys";
+} // namespace
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace encrypt_config {
+
+ConfigLine::ConfigLine(std::string line) : line_(line) {
+ line = utils::StringUtils::trim(line);
+ if (line.empty() || line[0] == '#') { return; }
+
+ size_t index_of_first_equals_sign = line.find('=');
+ if (index_of_first_equals_sign == std::string::npos) { return; }
+
+ std::string key = utils::StringUtils::trim(line.substr(0, index_of_first_equals_sign));
+ if (key.empty()) { return; }
+
+ key_ = key;
+ value_ = utils::StringUtils::trim(line.substr(index_of_first_equals_sign + 1));
+}
+
+ConfigLine::ConfigLine(std::string key, std::string value)
+ : line_{utils::StringUtils::join_pack(key, "=", value)}, key_{std::move(key)}, value_{std::move(value)} {
+}
+
+void ConfigLine::updateValue(const std::string& value) {
+ auto pos = line_.find('=');
+ if (pos != std::string::npos) {
+ line_.replace(pos + 1, std::string::npos, value);
+ value_ = value;
+ } else {
+ throw std::invalid_argument{"Cannot update value in config line: it does not contain an = sign!"};
+ }
+}
+
+ConfigFile::ConfigFile(std::istream& input_stream) {
+ std::string line;
+ while (std::getline(input_stream, line)) {
+ config_lines_.push_back(ConfigLine{line});
+ }
+}
+
+ConfigFile::Lines::const_iterator ConfigFile::findKey(const std::string& key) const {
+ return std::find_if(config_lines_.cbegin(), config_lines_.cend(), [&key](const ConfigLine& config_line) {
+ return config_line.getKey() == key;
+ });
+}
+
+ConfigFile::Lines::iterator ConfigFile::findKey(const std::string& key) {
+ return std::find_if(config_lines_.begin(), config_lines_.end(), [&key](const ConfigLine& config_line) {
+ return config_line.getKey() == key;
+ });
+}
+
+bool ConfigFile::hasValue(const std::string& key) const {
+ const auto it = findKey(key);
+ return (it != config_lines_.end());
+}
+
+utils::optional<std::string> ConfigFile::getValue(const std::string& key) const {
+ const auto it = findKey(key);
+ if (it != config_lines_.end()) {
+ return it->getValue();
+ } else {
+ return utils::nullopt;
+ }
+}
+
+void ConfigFile::update(const std::string& key, const std::string& value) {
+ auto it = findKey(key);
+ if (it != config_lines_.end()) {
+ it->updateValue(value);
+ } else {
+ throw std::invalid_argument{"Key " + key + " not found in the config file!"};
+ }
+}
+
+void ConfigFile::insertAfter(const std::string& after_key, const std::string& key, const std::string& value) {
+ auto it = findKey(after_key);
+ if (it != config_lines_.end()) {
+ ++it;
+ config_lines_.emplace(it, key, value);
+ } else {
+ throw std::invalid_argument{"Key " + after_key + " not found in the config file!"};
+ }
+}
+
+void ConfigFile::append(const std::string& key, const std::string& value) {
+ config_lines_.emplace_back(key, value);
+}
+
+int ConfigFile::erase(const std::string& key) {
+ auto has_this_key = [&key](const ConfigLine& line) { return line.getKey() == key; };
+ auto new_end = std::remove_if(config_lines_.begin(), config_lines_.end(), has_this_key);
+ auto num_removed = std::distance(new_end, config_lines_.end());
+ config_lines_.erase(new_end, config_lines_.end());
+ return gsl::narrow<int>(num_removed);
+}
+
+void ConfigFile::writeTo(const std::string& file_path) const {
+ std::ofstream file{file_path};
Review comment:
`std::ofstream` doesn't throw on error, just sets error bits by default. This hides errors and causes me as a user to not realize that despite the success output, nothing has changes because I was not running as admin.
Luckily its error reporting behavior can be configured.
```suggestion
std::ofstream file{file_path};
file.exceptions(std::ios::failbit | std::ios::badbit);
```
##########
File path: encrypt-config/ConfigFile.cpp
##########
@@ -0,0 +1,171 @@
+/**
+ * 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.
+ */
+
+#include "ConfigFile.h"
+
+#include <algorithm>
+#include <fstream>
+#include <utility>
+
+#include "utils/StringUtils.h"
+
+namespace {
+constexpr std::array<const char*, 2> DEFAULT_SENSITIVE_PROPERTIES{"nifi.security.client.pass.phrase",
+ "nifi.rest.api.password"};
+constexpr const char* ADDITIONAL_SENSITIVE_PROPS_PROPERTY_NAME = "nifi.sensitive.props.additional.keys";
+} // namespace
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace encrypt_config {
+
+ConfigLine::ConfigLine(std::string line) : line_(line) {
+ line = utils::StringUtils::trim(line);
+ if (line.empty() || line[0] == '#') { return; }
+
+ size_t index_of_first_equals_sign = line.find('=');
+ if (index_of_first_equals_sign == std::string::npos) { return; }
+
+ std::string key = utils::StringUtils::trim(line.substr(0, index_of_first_equals_sign));
+ if (key.empty()) { return; }
+
+ key_ = key;
+ value_ = utils::StringUtils::trim(line.substr(index_of_first_equals_sign + 1));
+}
+
+ConfigLine::ConfigLine(std::string key, std::string value)
+ : line_{utils::StringUtils::join_pack(key, "=", value)}, key_{std::move(key)}, value_{std::move(value)} {
+}
+
+void ConfigLine::updateValue(const std::string& value) {
+ auto pos = line_.find('=');
+ if (pos != std::string::npos) {
+ line_.replace(pos + 1, std::string::npos, value);
+ value_ = value;
+ } else {
+ throw std::invalid_argument{"Cannot update value in config line: it does not contain an = sign!"};
+ }
+}
+
+ConfigFile::ConfigFile(std::istream& input_stream) {
+ std::string line;
+ while (std::getline(input_stream, line)) {
+ config_lines_.push_back(ConfigLine{line});
+ }
+}
+
+ConfigFile::Lines::const_iterator ConfigFile::findKey(const std::string& key) const {
+ return std::find_if(config_lines_.cbegin(), config_lines_.cend(), [&key](const ConfigLine& config_line) {
+ return config_line.getKey() == key;
+ });
+}
+
+ConfigFile::Lines::iterator ConfigFile::findKey(const std::string& key) {
+ return std::find_if(config_lines_.begin(), config_lines_.end(), [&key](const ConfigLine& config_line) {
+ return config_line.getKey() == key;
+ });
+}
+
+bool ConfigFile::hasValue(const std::string& key) const {
+ const auto it = findKey(key);
+ return (it != config_lines_.end());
+}
+
+utils::optional<std::string> ConfigFile::getValue(const std::string& key) const {
+ const auto it = findKey(key);
+ if (it != config_lines_.end()) {
+ return it->getValue();
+ } else {
+ return utils::nullopt;
+ }
+}
+
+void ConfigFile::update(const std::string& key, const std::string& value) {
+ auto it = findKey(key);
+ if (it != config_lines_.end()) {
+ it->updateValue(value);
+ } else {
+ throw std::invalid_argument{"Key " + key + " not found in the config file!"};
+ }
+}
+
+void ConfigFile::insertAfter(const std::string& after_key, const std::string& key, const std::string& value) {
+ auto it = findKey(after_key);
+ if (it != config_lines_.end()) {
+ ++it;
+ config_lines_.emplace(it, key, value);
+ } else {
+ throw std::invalid_argument{"Key " + after_key + " not found in the config file!"};
+ }
+}
+
+void ConfigFile::append(const std::string& key, const std::string& value) {
+ config_lines_.emplace_back(key, value);
+}
+
+int ConfigFile::erase(const std::string& key) {
+ auto has_this_key = [&key](const ConfigLine& line) { return line.getKey() == key; };
+ auto new_end = std::remove_if(config_lines_.begin(), config_lines_.end(), has_this_key);
+ auto num_removed = std::distance(new_end, config_lines_.end());
+ config_lines_.erase(new_end, config_lines_.end());
+ return gsl::narrow<int>(num_removed);
+}
+
+void ConfigFile::writeTo(const std::string& file_path) const {
+ std::ofstream file{file_path};
Review comment:
`std::ofstream` doesn't throw on error, just sets error bits by default. This hides errors and causes me as a user to not realize that despite the success output, nothing has changed because I was not running as admin.
Luckily its error reporting behavior can be configured.
```suggestion
std::ofstream file{file_path};
file.exceptions(std::ios::failbit | std::ios::badbit);
```
##########
File path: encrypt-config/CMakeLists.txt
##########
@@ -0,0 +1,25 @@
+#
+# 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.
+
+file(GLOB ENCRYPT_CONFIG_FILES "*.cpp")
+add_executable(encrypt-config "${ENCRYPT_CONFIG_FILES}")
+target_include_directories(encrypt-config PRIVATE ../libminifi/include ../thirdparty/cxxopts/include)
+target_wholearchive_library(encrypt-config minifi)
+target_link_libraries(encrypt-config cxxopts libsodium)
+set_target_properties(encrypt-config PROPERTIES OUTPUT_NAME encrypt-config)
+install(TARGETS encrypt-config RUNTIME DESTINATION bin COMPONENT bin)
Review comment:
Multi configuration generators (visual studio) don't save the last build type in the cache, so cpack apparently has no way of knowing that it has to get the binary from the Debug/RelWithDebInfo directory and defaults to Release (the first one AFAIK). Not sure why the issue only happens with `install(TARGETS)`, but the fix is to append pass `-C <configuration>` to cpack.
Could you add this to `win_build_vs.bat`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r499760640
##########
File path: libminifi/src/Decryptor.cpp
##########
@@ -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.
+ */
+#include "properties/Decryptor.h"
+
+#include "properties/Properties.h"
+#include "utils/OptionalUtils.h"
+#include "utils/StringUtils.h"
+
+namespace {
+
+#ifdef WIN32
+constexpr const char* DEFAULT_NIFI_BOOTSTRAP_FILE = "\\conf\\bootstrap.conf";
+#else
+constexpr const char* DEFAULT_NIFI_BOOTSTRAP_FILE = "./conf/bootstrap.conf";
+#endif // WIN32
+
+constexpr const char* CONFIG_ENCRYPTION_KEY_PROPERTY_NAME = "nifi.bootstrap.sensitive.key";
+
+} // namespace
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+
+utils::optional<minifi::Decryptor> Decryptor::create(const std::string& minifi_home) {
+ minifi::Properties bootstrap_conf;
+ bootstrap_conf.setHome(minifi_home);
+ bootstrap_conf.loadConfigureFile(DEFAULT_NIFI_BOOTSTRAP_FILE);
+ return bootstrap_conf.getString(CONFIG_ENCRYPTION_KEY_PROPERTY_NAME)
+ | utils::map([](const std::string& encryption_key_hex) { return utils::StringUtils::from_hex(encryption_key_hex); })
+ | utils::map([](const std::string& encryption_key) { return utils::crypto::stringToBytes(encryption_key); })
Review comment:
done: https://github.com/apache/nifi-minifi-cpp/pull/914/commits/54f405621368410d9ac27947d455eb345dc641fe
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r498290578
##########
File path: encrypt-config/ConfigFileEncryptor.cpp
##########
@@ -0,0 +1,64 @@
+/**
+ * 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.
+ */
+
+#include "ConfigFileEncryptor.h"
+
+#include <iostream>
+#include <string>
+
+#include "utils/StringUtils.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace encrypt_config {
+
+int encryptSensitivePropertiesInFile(ConfigFile& config_file, const utils::crypto::Bytes& encryption_key) {
Review comment:
I am not totally convinced: to me, `int` means "a smallish integer, don't care too much which type", which is what I meant here; but I have changed it to `uint32_t`.
##########
File path: encrypt-config/EncryptConfig.cpp
##########
@@ -0,0 +1,150 @@
+/**
+ * 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.
+ */
+
+#include "EncryptConfig.h"
+
+#include <sodium.h>
+
+#include <stdexcept>
+
+#include "ConfigFile.h"
+#include "ConfigFileEncryptor.h"
+#include "cxxopts.hpp"
+#include "utils/file/FileUtils.h"
+#include "utils/OptionalUtils.h"
+
+namespace {
Review comment:
That reference says "do _not_ use blank lines in general, but you can use them in certain cases", and it's not clear this is one of those cases, but OK.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r498300651
##########
File path: encrypt-config/ConfigFileEncryptor.cpp
##########
@@ -0,0 +1,64 @@
+/**
+ * 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.
+ */
+
+#include "ConfigFileEncryptor.h"
+
+#include <iostream>
+#include <string>
+
+#include "utils/StringUtils.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace encrypt_config {
+
+int encryptSensitivePropertiesInFile(ConfigFile& config_file, const utils::crypto::Bytes& encryption_key) {
Review comment:
Thanks! If a return value is an `int` it usually suggests to me that it must be some kind of error code usually from legacy C code. An unsigned integer type would suggest some count result, like the one we have here. Maybe this is just my thinking.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r508332045
##########
File path: encrypt-config/ConfigFileEncryptor.cpp
##########
@@ -0,0 +1,64 @@
+/**
+ * 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.
+ */
+
+#include "ConfigFileEncryptor.h"
+
+#include <iostream>
+#include <string>
+
+#include "utils/StringUtils.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace encrypt_config {
+
+uint32_t encryptSensitivePropertiesInFile(ConfigFile& config_file, const utils::crypto::Bytes& encryption_key) {
+ int num_properties_encrypted = 0;
+
+ for (const auto& property_key : config_file.getSensitiveProperties()) {
+ utils::optional<std::string> property_value = config_file.getValue(property_key);
+ if (!property_value) { continue; }
+
+ std::string encryption_type_key = property_key + ".protected";
+ utils::optional<std::string> encryption_type = config_file.getValue(encryption_type_key);
+
+ if (!encryption_type || encryption_type->empty() || *encryption_type == "plaintext") {
Review comment:
what if the user accidentally tries to encrypt a file that already contains encrypted values with a different encryption key, should they get a warning?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r501848713
##########
File path: encrypt-config/CMakeLists.txt
##########
@@ -0,0 +1,25 @@
+#
+# 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.
+
+file(GLOB ENCRYPT_CONFIG_FILES "*.cpp")
+add_executable(encrypt-config "${ENCRYPT_CONFIG_FILES}")
+target_include_directories(encrypt-config PRIVATE ../libminifi/include ../thirdparty/cxxopts/include)
+target_wholearchive_library(encrypt-config minifi)
+target_link_libraries(encrypt-config cxxopts libsodium)
+set_target_properties(encrypt-config PROPERTIES OUTPUT_NAME encrypt-config)
+install(TARGETS encrypt-config RUNTIME DESTINATION bin COMPONENT bin)
Review comment:
yes, I will, thank you!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r498296412
##########
File path: encrypt-config/ConfigFile.cpp
##########
@@ -0,0 +1,166 @@
+/**
+ * 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.
+ */
+
+#include "ConfigFile.h"
+
+#include <algorithm>
+#include <fstream>
+#include <utility>
+
+#include "utils/StringUtils.h"
+
+namespace {
+constexpr std::array<const char*, 2> DEFAULT_SENSITIVE_PROPERTIES{"nifi.security.client.pass.phrase",
+ "nifi.rest.api.password"};
+constexpr const char* ADDITIONAL_SENSITIVE_PROPS_PROPERTY_NAME = "nifi.sensitive.props.additional.keys";
+} // namespace
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace encrypt_config {
+
+ConfigLine::ConfigLine(std::string line) : line_(line) {
+ line = utils::StringUtils::trim(line);
Review comment:
I understand, it definitely removes any clutter from the diff file.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r498090718
##########
File path: main/MiNiFiMain.cpp
##########
@@ -208,6 +238,10 @@ int main(int argc, char **argv) {
configure->setHome(minifiHome);
configure->loadConfigureFile(DEFAULT_NIFI_PROPERTIES_FILE);
+ if (containsEncryptedProperties(*configure)) {
+ decryptSensitiveProperties(*configure, minifiHome, *logger);
Review comment:
as I see, we update the `configure` object in `decryptSensitiveProperties `, one possible problem with this, is that on a flow update (from a C2 agent) the configurations file is persisted by default (to store the new flow url) this could inadvertently leak the decrypted values
see [here](https://github.com/apache/nifi-minifi-cpp/blob/da90d98b79c5590844b28ea81ce80f08765f48c9/libminifi/src/c2/C2Agent.cpp#L720)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r501673502
##########
File path: libminifi/include/properties/Properties.h
##########
@@ -65,7 +65,7 @@ class Properties {
* @param value value in which to place the map's stored property value
* @returns true if found, false otherwise.
*/
- bool get(const std::string &key, std::string &value) const;
+ bool getString(const std::string &key, std::string &value) const;
Review comment:
> We don't need get() to be virtual, and hiding a non-virtual function isn't nice.
I don't think hiding is a big issue. What I don't like is that a change in a derived class forces the parent class to change in an undesirable way.
> Properties has a getInt() method, so getString() makes sense.
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Ro-equivalent
Also, getInt isn't too important, just a convenience overload from the times before optional and map. `Properties` is a string key-string value store, so I as a user shouldn't need to say `getString` when retrieving a value, because I already said that by using the `Properties` class.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] alopresto commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r498583289
##########
File path: encrypt-config/ConfigFile.cpp
##########
@@ -0,0 +1,166 @@
+/**
+ * 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.
+ */
+
+#include "ConfigFile.h"
+
+#include <algorithm>
+#include <fstream>
+#include <utility>
+
+#include "utils/StringUtils.h"
+
+namespace {
+constexpr std::array<const char*, 2> DEFAULT_SENSITIVE_PROPERTIES{"nifi.security.client.pass.phrase",
Review comment:
Is there a canonical enumeration of the default properties somewhere?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] arpadboda closed pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
arpadboda closed pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r498302716
##########
File path: encrypt-config/EncryptConfig.cpp
##########
@@ -0,0 +1,150 @@
+/**
+ * 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.
+ */
+
+#include "EncryptConfig.h"
+
+#include <sodium.h>
+
+#include <stdexcept>
+
+#include "ConfigFile.h"
+#include "ConfigFileEncryptor.h"
+#include "cxxopts.hpp"
+#include "utils/file/FileUtils.h"
+#include "utils/OptionalUtils.h"
+
+namespace {
Review comment:
Thanks, I was referring to the last use case, where this could help with the readability a bit.
`Blank lines immediately inside a declaration of a namespace or block of namespaces may help readability by visually separating the load-bearing content from the (largely non-semantic) organizational wrapper.`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r498836361
##########
File path: encrypt-config/tests/ConfigFileTests.cpp
##########
@@ -0,0 +1,224 @@
+/**
+ * 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.
+ */
+
+#include "ConfigFile.h"
+
+#include "gsl/gsl-lite.hpp"
+
+#include "TestBase.h"
+#include "utils/file/FileUtils.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace encrypt_config {
+
+class ConfigFileTestAccessor {
+ public:
+ static std::vector<std::string> mergeProperties(std::vector<std::string> left, const std::vector<std::string>& right) {
+ return ConfigFile::mergeProperties(left, right);
+ }
+};
+
+} // namespace encrypt_config
+} // namespace minifi
+} // namespace nifi
+} // namespace apache
+} // namespace org
+
+using org::apache::nifi::minifi::encrypt_config::ConfigFile;
+using org::apache::nifi::minifi::encrypt_config::ConfigFileTestAccessor;
+using org::apache::nifi::minifi::encrypt_config::ConfigLine;
+
+TEST_CASE("ConfigLine can be constructed from a line", "[encrypt-config][constructor]") {
+ auto line_is_parsed_correctly = [](const std::string& line, const std::string& expected_key, const std::string& expected_value) {
+ ConfigLine config_line{line};
+ return config_line.getKey() == expected_key && config_line.getValue() == expected_value;
+ };
+
+ REQUIRE(line_is_parsed_correctly("", "", ""));
+ REQUIRE(line_is_parsed_correctly(" \t \r", "", ""));
+ REQUIRE(line_is_parsed_correctly("#disabled.setting=foo", "", ""));
+ REQUIRE(line_is_parsed_correctly("some line without an equals sign", "", ""));
+ REQUIRE(line_is_parsed_correctly("=value_without_key", "", ""));
+ REQUIRE(line_is_parsed_correctly("\t =value_without_key", "", ""));
+
+ REQUIRE(line_is_parsed_correctly("nifi.some.key=", "nifi.some.key", ""));
+ REQUIRE(line_is_parsed_correctly("nifi.some.key=some_value", "nifi.some.key", "some_value"));
+ REQUIRE(line_is_parsed_correctly("nifi.some.key = some_value", "nifi.some.key", "some_value"));
+ REQUIRE(line_is_parsed_correctly("\tnifi.some.key\t=\tsome_value", "nifi.some.key", "some_value"));
+ REQUIRE(line_is_parsed_correctly("nifi.some.key=some_value \r", "nifi.some.key", "some_value"));
+ REQUIRE(line_is_parsed_correctly("nifi.some.key=some value", "nifi.some.key", "some value"));
+ REQUIRE(line_is_parsed_correctly("nifi.some.key=value=with=equals=signs=", "nifi.some.key", "value=with=equals=signs="));
+}
+
+TEST_CASE("ConfigLine can be constructed from a key-value pair", "[encrypt-config][constructor]") {
+ auto can_construct_from_kv = [](const std::string& key, const std::string& value, const std::string& expected_line) {
+ ConfigLine config_line{key, value};
+ return config_line.getLine() == expected_line;
+ };
+
+ REQUIRE(can_construct_from_kv("nifi.some.key", "", "nifi.some.key="));
+ REQUIRE(can_construct_from_kv("nifi.some.key", "some_value", "nifi.some.key=some_value"));
+}
+
+TEST_CASE("ConfigLine can update the value", "[encrypt-config][updateValue]") {
+ auto can_update_value = [](const std::string& original_line, const std::string& new_value, const std::string& expected_line) {
+ ConfigLine config_line{original_line};
+ config_line.updateValue(new_value);
+ return config_line.getLine() == expected_line;
+ };
+
+ REQUIRE(can_update_value("nifi.some.key=some_value", "new_value", "nifi.some.key=new_value"));
+ REQUIRE(can_update_value("nifi.some.key=", "new_value", "nifi.some.key=new_value"));
+ REQUIRE(can_update_value("nifi.some.key=some_value", "", "nifi.some.key="));
+ REQUIRE(can_update_value("nifi.some.key=some_value", "very_long_new_value", "nifi.some.key=very_long_new_value"));
+
+ // whitespace is preserved in the key, but not in the value
+ REQUIRE(can_update_value("nifi.some.key= some_value", "some_value", "nifi.some.key=some_value"));
+ REQUIRE(can_update_value("nifi.some.key = some_value ", "some_value", "nifi.some.key =some_value"));
+ REQUIRE(can_update_value(" nifi.some.key=some_value", "some_value", " nifi.some.key=some_value"));
+ REQUIRE(can_update_value(" nifi.some.key =\tsome_value\r", "some_value", " nifi.some.key =some_value"));
+}
+
+TEST_CASE("ConfigFile creates an empty object from a nonexistent file", "[encrypt-config][constructor]") {
+ ConfigFile test_file{std::ifstream{"resources/nonexistent-minifi.properties"}};
+ REQUIRE(test_file.size() == 0);
+}
+
+TEST_CASE("ConfigFile can parse a simple config file", "[encrypt-config][constructor]") {
+ ConfigFile test_file{std::ifstream{"resources/minifi.properties"}};
+ REQUIRE(test_file.size() == 101);
+}
+
+TEST_CASE("ConfigFile can read empty properties correctly", "[encrypt-config][constructor]") {
+ ConfigFile test_file{std::ifstream{"resources/with-additional-sensitive-props.minifi.properties"}};
+ REQUIRE(test_file.size() == 103);
+
+ auto empty_property = test_file.getValue("nifi.security.need.ClientAuth");
+ REQUIRE(empty_property);
+ REQUIRE(empty_property->empty());
+
+ auto whitespace_property = test_file.getValue("nifi.security.client.certificate"); // value = " \t\r"
+ REQUIRE(whitespace_property);
+ REQUIRE(whitespace_property->empty());
+}
+
+TEST_CASE("ConfigFile can find the value for a key", "[encrypt-config][getValue]") {
+ ConfigFile test_file{std::ifstream{"resources/minifi.properties"}};
+
+ SECTION("valid key") {
+ REQUIRE(test_file.getValue("nifi.bored.yield.duration") == utils::optional<std::string>{"10 millis"});
+ }
+
+ SECTION("nonexistent key") {
+ REQUIRE(test_file.getValue("nifi.bored.panda") == utils::nullopt);
+ }
+}
+
+TEST_CASE("ConfigFile can update the value for a key", "[encrypt-config][update]") {
+ ConfigFile test_file{std::ifstream{"resources/minifi.properties"}};
+
+ SECTION("valid key") {
+ test_file.update("nifi.bored.yield.duration", "20 millis");
+ REQUIRE(test_file.getValue("nifi.bored.yield.duration") == utils::optional<std::string>{"20 millis"});
+ }
+
+ SECTION("nonexistent key") {
+ REQUIRE_THROWS(test_file.update("nifi.bored.panda", "cat video"));
+ }
+}
+
+TEST_CASE("ConfigFile can add a new setting after an existing setting", "[encrypt-config][insertAfter]") {
+ ConfigFile test_file{std::ifstream{"resources/minifi.properties"}};
+
+ SECTION("valid key") {
+ test_file.insertAfter("nifi.rest.api.password", "nifi.rest.api.password.protected", "XChaCha20-Poly1305");
+ REQUIRE(test_file.size() == 102);
+ REQUIRE(test_file.getValue("nifi.rest.api.password.protected") == utils::optional<std::string>{"XChaCha20-Poly1305"});
Review comment:
fixed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #914:
URL: https://github.com/apache/nifi-minifi-cpp/pull/914#discussion_r498300651
##########
File path: encrypt-config/ConfigFileEncryptor.cpp
##########
@@ -0,0 +1,64 @@
+/**
+ * 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.
+ */
+
+#include "ConfigFileEncryptor.h"
+
+#include <iostream>
+#include <string>
+
+#include "utils/StringUtils.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace encrypt_config {
+
+int encryptSensitivePropertiesInFile(ConfigFile& config_file, const utils::crypto::Bytes& encryption_key) {
Review comment:
Thanks! If a return value is an `int` it usually suggests me that it must be some kind of error code usually from legacy C code. An unsigned integer type would suggest some count result, like the one we have here. Maybe this is just my thinking.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org