szaszm commented on a change in pull request #886: URL: https://github.com/apache/nifi-minifi-cpp/pull/886#discussion_r493682636
########## File path: encrypt-config/tests/ConfigFileTests.cpp ########## @@ -0,0 +1,219 @@ +/** + * 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; +using org::apache::nifi::minifi::utils::file::FileUtils; + +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")); Review comment: I would add update test cases for lines with whitespaces around the key and value. If keeping lines unchanged is a design goal, the extent of change in case of update is an important detail. ########## File path: encrypt-config/tests/ConfigFileEncryptorTests.cpp ########## @@ -0,0 +1,106 @@ +/** + * 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 != "XChaCha20-Poly1305") { return false; } Review comment: ```suggestion namespace cryptoutils = org::apache::nifi::minifi::utils::crypto; if (!encryption_type || *encryption_type != cryptoutils::EncryptionType::name()) { return false; } ``` ########## File path: libminifi/include/utils/EncryptionUtils.h ########## @@ -0,0 +1,105 @@ +/** + * 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. + */ +#pragma once + +#include <string> +#include <vector> + +struct evp_aead_st; +typedef struct evp_aead_st EVP_AEAD; + +namespace org { +namespace apache { +namespace nifi { +namespace minifi { +namespace utils { +namespace crypto { + +using Bytes = std::vector<unsigned char>; Review comment: The key and the payload should be `gsl::span<const char>` and `gsl::span<char>` to be able to use them with any kind of storage and not only `std::string`/`std::vector`. ########## File path: libminifi/include/utils/EncryptionUtils.h ########## @@ -0,0 +1,105 @@ +/** + * 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. + */ +#pragma once + +#include <string> +#include <vector> + +struct evp_aead_st; +typedef struct evp_aead_st EVP_AEAD; + +namespace org { +namespace apache { +namespace nifi { +namespace minifi { +namespace utils { +namespace crypto { + +using Bytes = std::vector<unsigned char>; + +Bytes stringToBytes(const std::string& text); + +std::string bytesToString(const Bytes& bytes); + +Bytes randomBytes(size_t num_bytes); + +struct EncryptionType { + static const EVP_AEAD* cipher(); + static std::string name(); + static size_t nonceLength(); +}; Review comment: Why are these all `static`? I know that currently only one encryption type is supported, but I think it should either be non-static so that it's more general, or be called `XChaCha20_Poly1305_EncryptionType` so that it doesn't give the impression of a general class. ########## File path: libminifi/include/properties/Decryptor.h ########## @@ -0,0 +1,55 @@ +/** + * 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. + */ +#pragma once + +#include <string> + +#include "utils/EncryptionUtils.h" +#include "utils/OptionalUtils.h" + +namespace org { +namespace apache { +namespace nifi { +namespace minifi { + +class Configure; + +class Decryptor { + public: + explicit Decryptor(const utils::crypto::Bytes& encryption_key); + static bool isEncrypted(const utils::optional<std::string>& encryption_type); + std::string decrypt(const std::string& encrypted_text, const std::string& aad) const; + void decryptSensitiveProperties(Configure& configure) const; + + private: + const utils::crypto::Bytes encryption_key_; +}; + +inline Decryptor::Decryptor(const utils::crypto::Bytes& encryption_key) : encryption_key_(encryption_key) {} + +inline bool Decryptor::isEncrypted(const utils::optional<std::string>& encryption_type) { Review comment: I think this name is misleading, because the question it answer is not whether the argument is encrypted, but rather whether it's a valid encryption type string. I suggest calling it `isValidEncryptionType` or similar, move it near `utils::crypto::EncryptionType` and make it take a string rather than an optional string. (Move the optional validity check to the usage site) later: After looking at the test case, I realize that this is checking that it valid and it's not plaintext, so the encryption type is encrypted. I maintain that since it's checking a property of an encryption type string, it should be near the EncryptionType class. But not sure how to resolve the naming confusion, maybe you have a better idea? ########## File path: main/MiNiFiMain.cpp ########## @@ -53,13 +53,41 @@ #include "core/FlowConfiguration.h" #include "core/ConfigurationFactory.h" #include "core/RepositoryFactory.h" +#include "properties/Decryptor.h" #include "utils/file/PathUtils.h" #include "utils/file/FileUtils.h" #include "utils/Environment.h" #include "FlowController.h" #include "AgentDocs.h" #include "MainHelper.h" +namespace { +#ifdef OPENSSL_SUPPORT +bool containsEncryptedProperties(const minifi::Configure& minifi_properties) { + const auto is_encrypted_property_marker = [&minifi_properties](const std::string& property_name) { + return utils::StringUtils::endsWith(property_name, ".protected") && + minifi::Decryptor::isEncrypted(minifi_properties.get(property_name)); + }; + const auto property_names = minifi_properties.getConfiguredKeys(); + return std::any_of(property_names.begin(), property_names.end(), is_encrypted_property_marker); +} + +void decryptSensitiveProperties(minifi::Configure& minifi_properties, const std::string& minifi_home, logging::Logger& logger) { Review comment: Both with this and encrypt-config: I generally like functions that take a value and return a value better than functions that take a reference to some mutable memory and modify it in place. ---------------------------------------------------------------- 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: [email protected]
