remove class RemoteList c'tor RNG default value

Instead of throwing an exception with --remote-random-hostname, when
no RNG is present during construction, we treat an explicit null RNG
as a choice not to randomize the hosts. To make that choice explicit,
the default value for the RNG is removed, so that callers need to
decide which behavior they want.

Closes #53 in the openvpn3-linux issue tracker.

Signed-off-by: Heiko Hund <heiko@openvpn.net>

openvpn/client/cliopthelper.hpp

@@ -191,8 +191,9 @@ namespace openvpn {
 	    }
 	}
 
-	// validate remote list
+	// validate remote list - don't randomize it at this point
-	remoteList.reset(new RemoteList(options, "", 0, nullptr));
+	RandomAPI::Ptr no_rng;
+	remoteList.reset(new RemoteList(options, "", 0, nullptr, no_rng));
 	{
 	  const RemoteList::Item* ri = remoteList->first_item();
 	  if (ri)

openvpn/client/remotelist.hpp

@@ -416,7 +416,7 @@ namespace openvpn {
 	       const std::string& connection_tag,
 	       const unsigned int flags,
 	       ConnBlockFactory* conn_block_factory,
-	       RandomAPI::Ptr rng_arg = RandomAPI::Ptr())
+	       RandomAPI::Ptr rng_arg)
       : random_hostname(opt.exists("remote-random-hostname"))
       , directives(connection_tag)
       , rng(rng_arg)
@@ -886,7 +886,7 @@ namespace openvpn {
 
     void randomize_host(Item& item)
     {
-      if (!random_hostname)
+      if (!random_hostname || !rng)
 	return;
 
       try
@@ -896,9 +896,6 @@ namespace openvpn {
 	}
       catch (const IP::ip_exception& e)
 	{
-	  if (!rng)
-	    throw remote_list_error("remote-random-hostname without PRNG");
-
 	  // Produce 6 bytes of random prefix data
 	  unsigned char prefix[6];
 	  rng->rand_bytes(prefix, sizeof(prefix));

test/unittests/test_remotelist.cpp

@@ -105,7 +105,8 @@ TEST(RemoteList, CtorRemoteList)
     , nullptr);
   cfg.update_map();
 
-  RemoteList rl(cfg, "", 0, nullptr);
+  RandomAPI::Ptr rng;
+  RemoteList rl(cfg, "", 0, nullptr, rng);
   ASSERT_EQ(rl.defined(), true);
   ASSERT_EQ(rl.size(), 4);
   ASSERT_EQ(rl.get_item(0).server_host, "0.default.invalid");
@@ -132,7 +133,8 @@ TEST(RemoteList, CtorRemoteListConnBlockOnly)
     , nullptr);
   cfg.update_map();
 
-  RemoteList rl(cfg, "", RemoteList::CONN_BLOCK_ONLY, nullptr);
+  RandomAPI::Ptr rng;
+  RemoteList rl(cfg, "", RemoteList::CONN_BLOCK_ONLY, nullptr, rng);
   ASSERT_EQ(rl.defined(), true);
   ASSERT_EQ(rl.size(), 1);
   ASSERT_EQ(rl.get_item(0).server_host, "2.block.invalid");
@@ -143,8 +145,9 @@ TEST(RemoteList, CtorRemoteListEmpty)
   cfg.parse_from_config("", nullptr);
   cfg.update_map();
 
-  ASSERT_THROW(RemoteList(cfg, "", 0, nullptr), option_error);
+  RandomAPI::Ptr rng;
-  RemoteList rl(cfg, "", RemoteList::ALLOW_EMPTY, nullptr);
+  ASSERT_THROW(RemoteList(cfg, "", 0, nullptr, rng), option_error);
+  RemoteList rl(cfg, "", RemoteList::ALLOW_EMPTY, nullptr, rng);
 }
 TEST(RemoteList, CtorRemoteListConnBlockFactory)
 {
@@ -179,14 +182,15 @@ TEST(RemoteList, CtorRemoteListConnBlockFactory)
   cfg.update_map();
   TestConnBlockFactory tcbf;
 
+  RandomAPI::Ptr rng;
   testLog->startCollecting();
-  RemoteList rl1(cfg, "block", 0, &tcbf);
+  RemoteList rl1(cfg, "block", 0, &tcbf, rng);
   std::string output1(testLog->stopCollecting());
   ASSERT_NE(output1.find("TestConnBlock"), std::string::npos);
   ASSERT_EQ(rl1.size(), 2);
 
   testLog->startCollecting();
-  RemoteList rl2(cfg, "block", RemoteList::CONN_BLOCK_OMIT_UNDEF, &tcbf);
+  RemoteList rl2(cfg, "block", RemoteList::CONN_BLOCK_OMIT_UNDEF, &tcbf, rng);
   std::string output2(testLog->stopCollecting());
   ASSERT_NE(output2.find("TestConnBlock"), std::string::npos);
   ASSERT_EQ(rl2.size(), 1);
@@ -204,8 +208,9 @@ TEST(RemoteList, CtorRemoteListWarnUnsupported)
     , nullptr);
   cfg.update_map();
 
+  RandomAPI::Ptr rng;
   testLog->startCollecting();
-  RemoteList rl(cfg, "", RemoteList::WARN_UNSUPPORTED, nullptr);
+  RemoteList rl(cfg, "", RemoteList::WARN_UNSUPPORTED, nullptr, rng);
   std::string output(testLog->stopCollecting());
 
   ASSERT_NE(output.find(" http-proxy "), std::string::npos);
@@ -223,7 +228,8 @@ TEST(RemoteList, CtorRemoteListBlockLimit)
     , nullptr);
   cfg.update_map();
 
-  JY_EXPECT_THROW(RemoteList(cfg, "", 0, nullptr), option_error, "connection_block");
+  RandomAPI::Ptr rng;
+  JY_EXPECT_THROW(RemoteList(cfg, "", 0, nullptr, rng), option_error, "connection_block");
 }
 
 
@@ -240,11 +246,9 @@ TEST(RemoteList, RemoteListPreResolve)
     , nullptr);
   cfg.update_map();
 
-  RemoteList::Ptr rl(new RemoteList(cfg, "", 0, nullptr));
-  rl->set_enable_cache(true);
-
   RandomAPI::Ptr rng(new MTRand(3735928559));
-  rl->set_random(rng);
+  RemoteList::Ptr rl(new RemoteList(cfg, "", 0, nullptr, rng));
+  rl->set_enable_cache(true);
 
   openvpn_io::io_context ioctx;
   SessionStats::Ptr stats(new SessionStats());
@@ -402,7 +406,7 @@ TEST(RemoteList, RemoteRandomHostname)
   rl.next();
   ASSERT_EQ(rl.current_server_host(), "090a0b0c0d0e.3.domain.invalid");
 }
-TEST(RemoteList, RemoteRandomHostnameNoPRNG)
+TEST(RemoteList, RemoteRandomHostnameNoRNG)
 {
   OptionList cfg;
   cfg.parse_from_config(
@@ -411,7 +415,9 @@ TEST(RemoteList, RemoteRandomHostnameNoPRNG)
     , nullptr);
   cfg.update_map();
 
-  ASSERT_THROW(RemoteList(cfg, "", 0, nullptr), RemoteList::remote_list_error);
+  RandomAPI::Ptr no_rng;
+  RemoteList rl(cfg, "", 0, nullptr, no_rng);
+  ASSERT_EQ(rl.current_server_host(), "domain.invalid");
 }