fgerlits commented on a change in pull request #886: URL: https://github.com/apache/nifi-minifi-cpp/pull/886#discussion_r492618142
########## File path: encrypt-config/CMakeLists.txt ########## @@ -0,0 +1,30 @@ +# +# 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. +# + +cmake_minimum_required(VERSION 3.7) +project(encrypt-config) Review comment: I have removed these 3 lines ########## File path: encrypt-config/CMakeLists.txt ########## @@ -0,0 +1,30 @@ +# +# 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. +# + +cmake_minimum_required(VERSION 3.7) +project(encrypt-config) +set(VERSION, "0.1") + +file(GLOB ENCRYPT_CONFIG_FILES "*.cpp") +add_executable(encrypt-config "${ENCRYPT_CONFIG_FILES}") +include_directories(../libminifi/include ../thirdparty/cxxopts/include) Review comment: If I change it to `target_include_directories`, I get an error message which says ``` Determining if the pthread_create exist failed with the following output: Change Dir: /home/fgerlits/src/minifi/build/CMakeFiles/CMakeTmp Run Build Command:"/usr/bin/make" "cmTC_c8414/fast" /usr/bin/make -f CMakeFiles/cmTC_c8414.dir/build.make CMakeFiles/cmTC_c8414.dir/build make[1]: Entering directory '/home/fgerlits/src/minifi/build/CMakeFiles/CMakeTmp' Building C object CMakeFiles/cmTC_c8414.dir/CheckSymbolExists.c.o /usr/bin/cc -o CMakeFiles/cmTC_c8414.dir/CheckSymbolExists.c.o -c /home/fgerlits/src/minifi/build/CMakeFiles/CMakeTmp/CheckSymbolExists.c Linking C executable cmTC_c8414 /usr/bin/cmake -E cmake_link_script CMakeFiles/cmTC_c8414.dir/link.txt --verbose=1 /usr/bin/cc -rdynamic CMakeFiles/cmTC_c8414.dir/CheckSymbolExists.c.o -o cmTC_c8414 CMakeFiles/cmTC_c8414.dir/CheckSymbolExists.c.o: In function `main': CheckSymbolExists.c:(.text+0x1b): undefined reference to `pthread_create' collect2: error: ld returned 1 exit status CMakeFiles/cmTC_c8414.dir/build.make:97: recipe for target 'cmTC_c8414' failed make[1]: *** [cmTC_c8414] Error 1 make[1]: Leaving directory '/home/fgerlits/src/minifi/build/CMakeFiles/CMakeTmp' Makefile:126: recipe for target 'cmTC_c8414/fast' failed make: *** [cmTC_c8414/fast] Error 2 File /home/fgerlits/src/minifi/build/CMakeFiles/CMakeTmp/CheckSymbolExists.c: /* */ #include <pthread.h> int main(int argc, char** argv) { (void)argv; #ifndef pthread_create return ((int*)(&pthread_create))[argc]; #else (void)argc; return 0; #endif } ``` If I simply delete this line, then the includes are missing from the compile options. I have tried various ways of telling CMake that `encrypt-config` depends on `minifi` and `cxxopts` and should inherit their include and link flags, but none of them worked. I agree that the CMake build structure should be cleared up, but I suggest we do that separately from this pull request. ########## File path: encrypt-config/tests/CMakeLists.txt ########## @@ -0,0 +1,44 @@ +# +# 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_TESTS "*.cpp") + +file(GLOB ENCRYPT_CONFIG_SOURCES "${CMAKE_SOURCE_DIR}/encrypt-config/*.cpp") Review comment: Can you suggest an alternative? I don't want to list each file, because then we would need to modify the CMakeLists.txt file every time we add a new .cpp file, which is easy to forget. Note that globs like this are used all over minifi. If we want to clear it up, I think we should do it in a separate pull request. ########## File path: encrypt-config/tests/CMakeLists.txt ########## @@ -0,0 +1,44 @@ +# +# 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_TESTS "*.cpp") + +file(GLOB ENCRYPT_CONFIG_SOURCES "${CMAKE_SOURCE_DIR}/encrypt-config/*.cpp") +list(REMOVE_ITEM ENCRYPT_CONFIG_SOURCES "${CMAKE_SOURCE_DIR}/encrypt-config/EncryptConfigMain.cpp") + +set(ENCRYPT_CONFIG_TEST_COUNT 0) +foreach(testfile ${ENCRYPT_CONFIG_TESTS}) + get_filename_component(testfilename "${testfile}" NAME_WE) + add_executable(${testfilename} "${testfile}" ${ENCRYPT_CONFIG_SOURCES}) + + target_include_directories(${testfilename} BEFORE PRIVATE "${CMAKE_SOURCE_DIR}/thirdparty/catch") + target_include_directories(${testfilename} BEFORE PRIVATE "${CMAKE_SOURCE_DIR}/encrypt-config") + target_include_directories(${testfilename} BEFORE PRIVATE "${CMAKE_SOURCE_DIR}/libminifi/include") + target_include_directories(${testfilename} BEFORE PRIVATE "${CMAKE_SOURCE_DIR}/libminifi/test") + target_include_directories(${testfilename} BEFORE PRIVATE "${CMAKE_SOURCE_DIR}/thirdparty/cxxopts/include") Review comment: Same as above: simply removing these lines doesn't work; I'm sure there is a better way, but I haven't been able to find it. A clear-up could be done as part of a separate pull request. Note that `cmake/BuildTests.cmake` looks like this, too. ########## File path: encrypt-config/EncryptConfig.cpp ########## @@ -0,0 +1,146 @@ +/** + * 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 "cxxopts.hpp" + +#include <stdexcept> + +#include "ConfigFile.h" +#include "utils/file/FileUtils.h" +#include "utils/OptionalUtils.h" + +namespace { +const char* CONF_DIRECTORY_NAME = "conf"; +const char* BOOTSTRAP_FILE_NAME = "bootstrap.conf"; +const char* MINIFI_PROPERTIES_FILE_NAME = "minifi.properties"; +const char* ENCRYPTION_KEY_PROPERTY_NAME = "nifi.bootstrap.sensitive.key"; +const size_t ENCRYPTION_KEY_SIZE = 32; Review comment: fixed ########## File path: encrypt-config/ConfigFile.h ########## @@ -0,0 +1,85 @@ +/** + * 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> + +#include "utils/EncryptionUtils.h" +#include "utils/OptionalUtils.h" + +namespace org { +namespace apache { +namespace nifi { +namespace minifi { +namespace encrypt_config { + +class ConfigLine { +public: + ConfigLine(std::string line); + ConfigLine(const std::string& key, const std::string& value); + + void updateValue(const std::string& value); + + std::string line_; + std::string key_; + std::string value_; +}; + +inline bool operator==(const ConfigLine& left, const ConfigLine& right) { + return left.line_ == right.line_; +} + +class ConfigFile { +public: + ConfigFile(const std::string& file_path); + + utils::optional<std::string> getValue(const std::string& key) const; + void update(const std::string& key, const std::string& value); + void insertAfter(const std::string& after_key, const std::string& key, const std::string& value); + void append(const std::string& key, const std::string& value); Review comment: We use plain `update` in most places, once we use "`update` or `append`", and once we use "`update` or `insertAfter`". I can replace the one "`update` or `append`" call with a `set()`; I could also replace all `update()` calls with `set()` -- but I'm not convinced either option would improve the code. If you feel strongly about this, I will do it (tell me which of the above options you would prefer). ########## File path: encrypt-config/ConfigFile.h ########## @@ -0,0 +1,85 @@ +/** + * 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> + +#include "utils/EncryptionUtils.h" +#include "utils/OptionalUtils.h" + +namespace org { +namespace apache { +namespace nifi { +namespace minifi { +namespace encrypt_config { + +class ConfigLine { +public: + ConfigLine(std::string line); + ConfigLine(const std::string& key, const std::string& value); + + void updateValue(const std::string& value); + + std::string line_; + std::string key_; + std::string value_; +}; + +inline bool operator==(const ConfigLine& left, const ConfigLine& right) { + return left.line_ == right.line_; Review comment: This is used in the tests only, to verify that the input file is not modified at all in a certain use case. I don't want to allow `encrypt-config` to reformat the config file when it is not supposed to make any changes. ########## File path: encrypt-config/ConfigFile.h ########## @@ -0,0 +1,85 @@ +/** + * 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> + +#include "utils/EncryptionUtils.h" +#include "utils/OptionalUtils.h" + +namespace org { +namespace apache { +namespace nifi { +namespace minifi { +namespace encrypt_config { + +class ConfigLine { +public: + ConfigLine(std::string line); + ConfigLine(const std::string& key, const std::string& value); + + void updateValue(const std::string& value); + + std::string line_; + std::string key_; + std::string value_; +}; + +inline bool operator==(const ConfigLine& left, const ConfigLine& right) { + return left.line_ == right.line_; +} + +class ConfigFile { +public: + ConfigFile(const std::string& file_path); + + utils::optional<std::string> getValue(const std::string& key) const; + void update(const std::string& key, const std::string& value); + void insertAfter(const std::string& after_key, const std::string& key, const std::string& value); + void append(const std::string& key, const std::string& value); + int erase(const std::string& key); + + void writeTo(const std::string& file_path) const; + + size_t size() const { return config_lines_.size(); } + + int encryptSensitiveProperties(const utils::crypto::Bytes& encryption_key); Review comment: Done. I have also moved `decryptSensitiveProperties()` from `Configuration` to `Decryptor` as that violated the same principle even more. ########## File path: encrypt-config/ConfigFile.cpp ########## @@ -0,0 +1,195 @@ +/** + * 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 <fstream> + +#include "utils/StringUtils.h" + +namespace { +const std::array<const char*, 2> DEFAULT_SENSITIVE_PROPERTIES{"nifi.security.client.pass.phrase", + "nifi.rest.api.password"}; +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(const std::string& key, const std::string& value) + : line_{key + "=" + value}, key_{key}, value_{value} { Review comment: done ########## File path: encrypt-config/ConfigFile.cpp ########## @@ -0,0 +1,195 @@ +/** + * 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 <fstream> + +#include "utils/StringUtils.h" + +namespace { +const std::array<const char*, 2> DEFAULT_SENSITIVE_PROPERTIES{"nifi.security.client.pass.phrase", + "nifi.rest.api.password"}; +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(const std::string& key, const std::string& value) + : line_{key + "=" + value}, key_{key}, value_{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(const std::string& file_path) { + std::ifstream file{file_path}; Review comment: done ---------------------------------------------------------------- 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]
