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] fgerlits commented on a change in pull request #886: MINIFICPP-1323 Encrypt sensitive properties
Date Wed, 23 Sep 2020 04:20:44 GMT

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



Mime
View raw message