nifi-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #914: MINIFICPP-1323 Encrypt sensitive properties using libsodium
Date Fri, 02 Oct 2020 12:54:43 GMT

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



Mime
View raw message