Merge pull request #1301 from majestrate/ensure-key-backup-bug-squashed-2020-06-08

add regression test for key backup bug

include/llarp.hpp

@@ -107,6 +107,7 @@ namespace llarp
     std::string configfile;
     std::unique_ptr<std::promise<void>> closeWaiter;
   };
+
 }  // namespace llarp
 
 #endif

llarp/config/config.cpp

@@ -171,13 +171,13 @@ namespace llarp
     conf.defineOption<int>("network", "hops", false, HopsDefault, [this](int arg) {
       if (arg < 1 or arg > 8)
         throw std::invalid_argument("[endpoint]:hops must be >= 1 and <= 8");
-      m_hops = arg;
+      m_Hops = arg;
     });
 
     conf.defineOption<int>("network", "paths", false, PathsDefault, [this](int arg) {
       if (arg < 1 or arg > 8)
         throw std::invalid_argument("[endpoint]:paths must be >= 1 and <= 8");
-      m_paths = arg;
+      m_Paths = arg;
     });
 
     conf.defineOption<std::string>("network", "exit-node", false, "", [this](std::string arg) {
@@ -407,8 +407,15 @@ namespace llarp
 
     conf.defineOption<std::string>(
         "bootstrap", "add-node", false, true, "", [this](std::string arg) {
-          // TODO: validate as router fs path
+          if (arg.empty())
+          {
+            throw std::invalid_argument("cannot use empty filename as bootstrap");
+          }
           routers.emplace_back(std::move(arg));
+          if (not fs::exists(routers.back()))
+          {
+            throw std::invalid_argument("file does not exist: " + arg);
+          }
         });
   }
 
@@ -444,7 +451,7 @@ namespace llarp
   }
 
   bool
-  Config::Load(const char* fname, bool isRelay, fs::path defaultDataDir)
+  Config::Load(const fs::path fname, bool isRelay, fs::path defaultDataDir)
   {
     try
     {

llarp/config/config.hpp

@@ -72,11 +72,11 @@ namespace llarp
     std::string m_ifname;
     std::string m_ifaddr;
 
-    std::string m_keyfile;
+    std::optional<fs::path> m_keyfile;
     std::string m_endpointType;
     bool m_reachable = false;
-    int m_hops = -1;
-    int m_paths = -1;
+    std::optional<int> m_Hops;
+    std::optional<int> m_Paths;
     bool m_AllowExit = false;
     std::set<RouterID> m_snodeBlacklist;
     std::optional<service::Address> m_exitNode;
@@ -152,7 +152,9 @@ namespace llarp
 
   struct BootstrapConfig
   {
-    std::vector<std::string> routers;
+    std::vector<fs::path> routers;
+    /// for unit tests
+    bool skipBootstrap = false;
     void
     defineConfigOptions(ConfigDefinition& conf, const ConfigGenParameters& params);
   };
@@ -192,7 +194,7 @@ namespace llarp
 
     // Load a config from the given file
     bool
-    Load(const char* fname, bool isRelay, fs::path defaultDataDir);
+    Load(const fs::path fname, bool isRelay, fs::path defaultDataDir);
 
     /// Load (initialize) a default config.
     ///
@@ -224,4 +226,12 @@ namespace llarp
 
 }  // namespace llarp
 
+struct llarp_config
+{
+  llarp::Config impl;
+  llarp_config() = default;
+
+  explicit llarp_config(const llarp_config* other);
+};
+
 #endif

llarp/config/ini.cpp

@@ -12,11 +12,10 @@
 namespace llarp
 {
   bool
-  ConfigParser::LoadFile(std::string_view fname)
+  ConfigParser::LoadFile(const fs::path fname)
   {
-    std::string name{fname};
     {
-      std::ifstream f(name, std::ios::in | std::ios::binary);
+      std::ifstream f(fname, std::ios::in | std::ios::binary);
       if (!f.is_open())
         return false;
       f.seekg(0, std::ios::end);
@@ -26,7 +25,7 @@ namespace llarp
         return false;
       f.read(m_Data.data(), m_Data.size());
     }
-    m_FileName = name;
+    m_FileName = fname;
     return Parse();
   }
 
@@ -35,7 +34,6 @@ namespace llarp
   {
     m_Data.resize(str.size());
     std::copy(str.begin(), str.end(), m_Data.begin());
-    m_FileName = "<anonymous string>";
     return Parse();
   }
 

llarp/config/ini.hpp

@@ -7,6 +7,7 @@
 #include <memory>
 #include <unordered_map>
 #include <vector>
+#include <util/fs.hpp>
 
 namespace llarp
 {
@@ -22,7 +23,7 @@ namespace llarp
     /// return true on success
     /// return false on error
     bool
-    LoadFile(std::string_view fname);
+    LoadFile(const fs::path fname);
 
     /// load from string
     /// return true on success
@@ -45,7 +46,7 @@ namespace llarp
 
     std::vector<char> m_Data;
     Config_impl_t m_Config;
-    std::string m_FileName;
+    fs::path m_FileName;
   };
 
 }  // namespace llarp

llarp/context.cpp

@@ -234,15 +234,9 @@ struct llarp_main
   std::shared_ptr<llarp::Context> ctx;
 };
 
-struct llarp_config
+llarp_config::llarp_config(const llarp_config* other) : impl(other->impl)
 {
-  llarp::Config impl;
-  llarp_config() = default;
-
-  llarp_config(const llarp_config* other) : impl(other->impl)
-  {
-  }
-};
+}
 
 namespace llarp
 {

llarp/ev/ev_libuv.cpp

@@ -750,7 +750,9 @@ namespace libuv
     loop->process_timer_queue();
     loop->process_cancel_queue();
     loop->FlushLogic();
-    llarp::LogContext::Instance().logStream->Tick(loop->time_now());
+    auto& log = llarp::LogContext::Instance();
+    if (log.logStream)
+      log.logStream->Tick(loop->time_now());
   }
 
   Loop::Loop(size_t queue_size)

llarp/router/router.cpp

@@ -442,8 +442,10 @@ namespace llarp
       // TODO: use constant
       fs::path defaultBootstrapFile = conf->router.m_dataDir / "bootstrap.signed";
       if (fs::exists(defaultBootstrapFile))
+      {
         configRouters.push_back(defaultBootstrapFile);
-      else
+      }
+      else if (not conf->bootstrap.skipBootstrap)
       {
         LogError("No bootstrap files specified in config file, and the default");
         LogError("bootstrap file ", defaultBootstrapFile, " does not exist.");

llarp/service/endpoint.cpp

@@ -45,11 +45,11 @@ namespace llarp
     bool
     Endpoint::Configure(const NetworkConfig& conf, [[maybe_unused]] const DnsConfig& dnsConf)
     {
-      if (conf.m_paths > 0)
-        numPaths = conf.m_paths;
+      if (conf.m_Paths.has_value())
+        numPaths = *conf.m_Paths;
 
-      if (conf.m_hops)
-        numHops = conf.m_hops;
+      if (conf.m_Hops.has_value())
+        numHops = *conf.m_Hops;
 
       return m_state->Configure(conf);
     }
@@ -387,7 +387,6 @@ namespace llarp
     bool
     Endpoint::LoadKeyFile()
     {
-      LogWarn("LoadKeyFile()");
       const auto& keyfile = m_state->m_Keyfile;
       if (!keyfile.empty())
       {

llarp/service/endpoint_state.cpp

@@ -13,7 +13,8 @@ namespace llarp
     bool
     EndpointState::Configure(const NetworkConfig& conf)
     {
-      m_Keyfile = conf.m_keyfile;
+      if (conf.m_keyfile.has_value())
+        m_Keyfile = *conf.m_keyfile;
       m_SnodeBlacklist = conf.m_snodeBlacklist;
       m_ExitEnabled = conf.m_AllowExit;
       m_ExitNode = conf.m_exitNode;

llarp/util/logging/logger.cpp

@@ -90,6 +90,12 @@ namespace llarp
     }
   }
 
+  LogLevel
+  GetLogLevel()
+  {
+    return LogContext::Instance().curLevel;
+  }
+
   void
   LogContext::ImmediateFlush()
   {
@@ -168,6 +174,19 @@ namespace llarp
     }
   }
 
+  LogSilencer::LogSilencer() : LogSilencer(LogContext::Instance())
+  {
+  }
+
+  LogSilencer::LogSilencer(LogContext& ctx) : parent(ctx), stream(std::move(ctx.logStream))
+  {
+  }
+
+  LogSilencer::~LogSilencer()
+  {
+    parent.logStream = std::move(stream);
+  }
+
 }  // namespace llarp
 
 extern "C"

llarp/util/logging/logger.hpp

@@ -60,9 +60,25 @@ namespace llarp
         std::shared_ptr<thread::ThreadPool> threadpool);
   };
 
+  /// RAII type to turn logging off
+  /// logging is suppressed as long as the silencer is in scope
+  struct LogSilencer
+  {
+    LogSilencer();
+    ~LogSilencer();
+    explicit LogSilencer(LogContext& ctx);
+
+   private:
+    LogContext& parent;
+    ILogStream_ptr stream;
+  };
+
   void
   SetLogLevel(LogLevel lvl);
 
+  LogLevel
+  GetLogLevel();
+
   /** internal */
   template <typename... TArgs>
   inline static void
@@ -75,7 +91,7 @@ namespace llarp
 /* nop out logging for hive mode for now */
 #ifndef LOKINET_HIVE
     auto& log = LogContext::Instance();
-    if (log.curLevel > lvl)
+    if (log.curLevel > lvl || log.logStream == nullptr)
       return;
     std::stringstream ss;
     LogAppend(ss, std::forward<TArgs>(args)...);

test/CMakeLists.txt

@@ -69,6 +69,7 @@ add_subdirectory(Catch2)
 add_executable(catchAll
   nodedb/test_nodedb.cpp
   path/test_path.cpp
+  regress/2020-06-08-key-backup-bug.cpp
   util/test_llarp_util_bits.cpp
   util/test_llarp_util_printer.cpp
   util/test_llarp_util_str.cpp

test/main.cpp

@@ -1,5 +1,7 @@
 #include <gtest/gtest.h>
 
+#include <util/logging/logger.hpp>
+
 #ifdef _WIN32
 #include <winsock2.h>
 int
@@ -8,7 +10,7 @@ startWinsock()
   WSADATA wsockd;
   int err;
   err = ::WSAStartup(MAKEWORD(2, 2), &wsockd);
-  if(err)
+  if (err)
   {
     perror("Failed to start Windows Sockets");
     return err;
@@ -20,8 +22,9 @@ startWinsock()
 int
 main(int argc, char** argv)
 {
+  llarp::LogSilencer shutup;
 #ifdef _WIN32
-  if(startWinsock())
+  if (startWinsock())
     return -1;
 #endif
 

test/regress/2020-06-08-key-backup-bug.cpp

@@ -0,0 +1,74 @@
+#include <llarp.h>
+#include <llarp.hpp>
+#include <config/config.hpp>
+#include <router/abstractrouter.hpp>
+#include <service/context.hpp>
+#include <catch2/catch.hpp>
+
+/// make a llarp_main* with 1 endpoint that specifies a keyfile
+static llarp_main*
+make_context(std::optional<fs::path> keyfile)
+{
+  auto config = llarp_default_config();
+  config->impl.network.m_endpointType = "null";
+  config->impl.network.m_keyfile = keyfile;
+  config->impl.bootstrap.skipBootstrap = true;
+  auto ptr = llarp_main_init_from_config(config, false);
+  llarp_config_free(config);
+  return ptr;
+}
+
+/// test that we dont back up all keys when self.signed is missing or invalid as client
+TEST_CASE("key backup bug regression test", "[regress]")
+{
+  // kill logging, this code is noisy
+  llarp::LogSilencer shutup;
+  // test 2 explicitly provided keyfiles, empty keyfile and no keyfile
+  for (std::optional<fs::path> path : {std::optional<fs::path>{"regress-1.private"},
+                                       std::optional<fs::path>{"regress-2.private"},
+                                       std::optional<fs::path>{""},
+                                       {std::nullopt}})
+  {
+    llarp::service::Address endpointAddress{};
+    // try 10 start up and shut downs and see if our key changes or not
+    for (size_t index = 0; index < 10; index++)
+    {
+      auto context = make_context(path);
+      REQUIRE(llarp_main_setup(context, false) == 0);
+      auto ctx = llarp::Context::Get(context);
+      ctx->CallSafe([ctx, index, &endpointAddress, &path]() {
+        auto ep = ctx->router->hiddenServiceContext().GetDefault();
+        REQUIRE(ep != nullptr);
+        if (index == 0)
+        {
+          REQUIRE(endpointAddress.IsZero());
+          // first iteration, we are getting our identity that we start with
+          endpointAddress = ep->GetIdentity().pub.Addr();
+          REQUIRE(not endpointAddress.IsZero());
+        }
+        else
+        {
+          REQUIRE(not endpointAddress.IsZero());
+          if (path.has_value() and not path->empty())
+          {
+            // we have a keyfile provided
+            // after the first iteration we expect the keys to stay the same
+            REQUIRE(endpointAddress == ep->GetIdentity().pub.Addr());
+          }
+          else
+          {
+            // we want the keys to shift because no keyfile was provided
+            REQUIRE(endpointAddress != ep->GetIdentity().pub.Addr());
+          }
+        }
+        // close the router "later" so llarp_main_run exits
+        ctx->CloseAsync();
+      });
+      REQUIRE(llarp_main_run(context, llarp_main_runtime_opts{}) == 0);
+      llarp_main_free(context);
+    }
+    // remove keys if provied
+    if (path.has_value() and not path->empty())
+      fs::remove(*path);
+  }
+}