[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium

2020-10-13 Thread GitBox


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 , std::string ) const;
+  bool getString(const std::string , std::string ) 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:
us...@infra.apache.org




[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium

2020-10-13 Thread GitBox


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 
+#include 
+#include 
+
+#include "utils/StringUtils.h"
+
+namespace {
+constexpr std::array 
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(), 
[](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(), [](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 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 = [](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(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





[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium

2020-10-09 Thread GitBox


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:
us...@infra.apache.org




[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium

2020-10-08 Thread GitBox


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:
us...@infra.apache.org




[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium

2020-10-05 Thread GitBox


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 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:
us...@infra.apache.org




[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium

2020-10-05 Thread GitBox


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 , std::string ) const;
+  bool getString(const std::string , std::string ) 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:
us...@infra.apache.org




[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium

2020-10-05 Thread GitBox


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:
us...@infra.apache.org




[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium

2020-10-02 Thread GitBox


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 mergeProperties(std::vector 
left, const std::vector& 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 

[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium

2020-10-02 Thread GitBox


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 mergeProperties(std::vector 
left, const std::vector& 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 

[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium

2020-10-02 Thread GitBox


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 
+
+#include 
+
+#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());
+
+  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();
+  } 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 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 

[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium

2020-10-02 Thread GitBox


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 encrypted_value = 
test_file.getValue(property_name);
+if (!encrypted_value) { return false; }
+
+utils::optional 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:
us...@infra.apache.org




[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium

2020-10-01 Thread GitBox


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:
us...@infra.apache.org




[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium

2020-10-01 Thread GitBox


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 
+
+#include 
+
+#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());
+
+  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();
+  } 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 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:
us...@infra.apache.org




[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium

2020-10-01 Thread GitBox


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:
us...@infra.apache.org




[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium

2020-10-01 Thread GitBox


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:
us...@infra.apache.org




[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium

2020-10-01 Thread GitBox


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 
+#include 
+
+#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 
+
+#include 
+
+#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:
us...@infra.apache.org




[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium

2020-10-01 Thread GitBox


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 
+
+#include 
+
+#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());
+
+  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();
+  } 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:
us...@infra.apache.org




[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium

2020-10-01 Thread GitBox


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 
+#include 
+#include 
+
+#include "utils/StringUtils.h"
+
+namespace {
+constexpr std::array 
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:
us...@infra.apache.org




[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium

2020-10-01 Thread GitBox


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:
us...@infra.apache.org




[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium

2020-10-01 Thread GitBox


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:
us...@infra.apache.org




[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium

2020-09-30 Thread GitBox


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 
+
+#include 
+
+#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());
+
+  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();
+  } 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 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:
us...@infra.apache.org




[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium

2020-09-30 Thread GitBox


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 
+
+#include 
+
+#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());
+
+  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();
+  } 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 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:
us...@infra.apache.org