refactor: Use error code in make_address() calls (#3044)

Function `ip::make_address()` throws an exception on an invalid IP.
Refactor to a better error handling without exceptions.

docs/config-description.md

@@ -230,7 +230,7 @@ This document provides a list of all available Clio configuration properties in
 - **Required**: False
 - **Type**: string
 - **Default value**: None
-- **Constraints**: None
+- **Constraints**: The value must be a valid IP address.
 - **Description**: The list of IP addresses to whitelist for DOS protection.
 
 ### dos_guard.max_fetches
@@ -342,7 +342,7 @@ This document provides a list of all available Clio configuration properties in
 - **Required**: True
 - **Type**: string
 - **Default value**: None
-- **Constraints**: None
+- **Constraints**: The value must be a valid IP address.
 - **Description**: List of proxy ip addresses. When Clio receives a request from proxy it will use `Forwarded` value (if any) as client ip. When this option is used together with `server.proxy.tokens` Clio will identify proxy by ip or by token.
 
 ### server.proxy.tokens.[]

src/app/ClioApplication.cpp

@@ -92,9 +92,14 @@ ClioApplication::run(bool const useNgWebServer)
     boost::asio::io_context ioc{threads};
 
     // Rate limiter, to prevent abuse
-    auto whitelistHandler = web::dosguard::WhitelistHandler{config_};
+    auto whitelistHandler = web::dosguard::WhitelistHandler::create(config_);
+    if (not whitelistHandler.has_value()) {
+        LOG(util::LogService::fatal()) << whitelistHandler.error();
+        return EXIT_FAILURE;
+    }
+
     auto const dosguardWeights = web::dosguard::Weights::make(config_);
-    auto dosGuard = web::dosguard::DOSGuard{config_, whitelistHandler, dosguardWeights};
+    auto dosGuard = web::dosguard::DOSGuard{config_, *whitelistHandler, dosguardWeights};
     auto sweepHandler = web::dosguard::IntervalSweepHandler{config_, ioc, dosGuard};
 
     auto cache = data::LedgerCache{};
@@ -222,10 +227,15 @@ ClioApplication::run(bool const useNgWebServer)
         config_, backend, rpcEngine, etl, dosGuard
     );
 
-    auto const httpServer = web::makeHttpServer(config_, ioc, dosGuard, handler, cache);
+    auto const expectedHttpServer = web::makeHttpServer(config_, ioc, dosGuard, handler, cache);
+    if (not expectedHttpServer.has_value()) {
+        LOG(util::LogService::fatal()) << expectedHttpServer.error();
+        return EXIT_FAILURE;
+    }
+
     appStopper_.setOnStop(
         Stopper::makeOnStopCallback(
-            *httpServer,
+            **expectedHttpServer,
             *balancer,
             *etl,
             *subscriptions,

src/util/config/ConfigDefinition.cpp

@@ -312,7 +312,8 @@ getClioConfig()
          {"num_markers",
           ConfigValue{ConfigType::Integer}.optional().withConstraint(gValidateNumMarkers)},
 
-         {"dos_guard.whitelist.[]", Array{ConfigValue{ConfigType::String}.optional()}},
+         {"dos_guard.whitelist.[]",
+          Array{ConfigValue{ConfigType::String}.optional().withConstraint(gValidateIp)}},
          {"dos_guard.max_fetches",
           ConfigValue{ConfigType::Integer}.defaultValue(1000'000u).withConstraint(gValidateUint32)},
          {"dos_guard.max_connections",
@@ -361,7 +362,8 @@ getClioConfig()
          {"server.ws_max_sending_queue_size",
           ConfigValue{ConfigType::Integer}.defaultValue(1500).withConstraint(gValidateUint32)},
          {"server.__ng_web_server", ConfigValue{ConfigType::Boolean}.defaultValue(false)},
-         {"server.proxy.ips.[]", Array{ConfigValue{ConfigType::String}}},
+         {"server.proxy.ips.[]",
+          Array{ConfigValue{ConfigType::String}.withConstraint(gValidateIp)}},
          {"server.proxy.tokens.[]", Array{ConfigValue{ConfigType::String}}},
 
          {"prometheus.enabled", ConfigValue{ConfigType::Boolean}.defaultValue(true)},

src/web/Server.hpp

@@ -375,7 +375,7 @@ using HttpServer = Server<HttpSession, SslHttpSession, HandlerType>;
  * @return The server instance
  */
 template <typename HandlerType>
-static std::shared_ptr<HttpServer<HandlerType>>
+static std::expected<std::shared_ptr<HttpServer<HandlerType>>, std::string>
 makeHttpServer(
     util::config::ClioConfigDefinition const& config,
     boost::asio::io_context& ioc,
@@ -388,19 +388,24 @@ makeHttpServer(
 
     auto expectedSslContext = ng::impl::makeServerSslContext(config);
     if (not expectedSslContext) {
-        LOG(log.error()) << "Failed to create SSL context: " << expectedSslContext.error();
-        return nullptr;
+        return std::unexpected(
+            fmt::format("Failed to create SSL context: {}", expectedSslContext.error())
+        );
     }
 
     auto const serverConfig = config.getObject("server");
-    auto const address = boost::asio::ip::make_address(serverConfig.get<std::string>("ip"));
+
+    auto const ipFromConfig = serverConfig.get<std::string>("ip");
+    boost::system::error_code ec;
+    auto const address = boost::asio::ip::make_address(ipFromConfig, ec);
+    if (ec.failed())
+        return std::unexpected(fmt::format("Invalid 'server.ip' config value: {}", ipFromConfig));
+
     auto const port = serverConfig.get<unsigned short>("port");
 
     auto expectedAdminVerification = makeAdminVerificationStrategy(config);
-    if (not expectedAdminVerification.has_value()) {
-        LOG(log.error()) << expectedAdminVerification.error();
-        throw std::logic_error{expectedAdminVerification.error()};
-    }
+    if (not expectedAdminVerification.has_value())
+        return std::unexpected(expectedAdminVerification.error());
 
     // If the transactions number is 200 per ledger, A client which subscribes everything will send
     // 400+ feeds for each ledger. we allow user delay 3 ledgers by default

src/web/dosguard/WhitelistHandler.cpp

@@ -8,32 +8,48 @@
 #include <algorithm>
 #include <functional>
 #include <regex>
-#include <stdexcept>
 #include <string>
 #include <string_view>
-#include <unordered_map>
 #include <unordered_set>
+#include <utility>
 #include <vector>
 
 namespace web::dosguard {
 
-void
+WhitelistHandler::WhitelistHandler(Whitelist whitelist) : whitelist_(std::move(whitelist))
+{
+}
+
+std::expected<void, std::string>
 Whitelist::add(std::string_view net)
 {
     using namespace boost::asio;
 
     if (not isMask(net)) {
-        ips_.push_back(ip::make_address(net));
-        return;
+        boost::system::error_code ec;
+        auto const ip = ip::make_address(net, ec);
+        if (ec.failed())
+            return std::unexpected{fmt::format("Malformed whitelist ip address: {}. ", net)};
+        ips_.push_back(ip);
+        return {};
     }
 
     if (isV4(net)) {
-        subnetsV4_.push_back(ip::make_network_v4(net));
+        boost::system::error_code ec;
+        auto const net4 = ip::make_network_v4(net, ec);
+        if (ec.failed())
+            return std::unexpected{fmt::format("Malformed network: {}. ", net)};
+        subnetsV4_.push_back(net4);
     } else if (isV6(net)) {
-        subnetsV6_.push_back(ip::make_network_v6(net));
+        boost::system::error_code ec;
+        auto const net6 = ip::make_network_v6(net, ec);
+        if (ec.failed())
+            return std::unexpected{fmt::format("Malformed network: {}. ", net)};
+        subnetsV6_.push_back(net6);
     } else {
-        throw std::runtime_error(fmt::format("malformed network: {}", net.data()));
+        return std::unexpected{fmt::format("Malformed network: {}. ", net)};
     }
+    return {};
 }
 
 bool
@@ -41,7 +57,11 @@ Whitelist::isWhiteListed(std::string_view ip) const
 {
     using namespace boost::asio;
 
-    auto const addr = ip::make_address(ip);
+    boost::system::error_code ec;
+    auto const addr = ip::make_address(ip, ec);
+    if (ec.failed())
+        return false;
+
     if (std::ranges::find(ips_, addr) != std::end(ips_))
         return true;
 
@@ -95,7 +115,7 @@ Whitelist::isV6(std::string_view net)
 bool
 Whitelist::isMask(std::string_view net)
 {
-    return net.find('/') != std::string_view::npos;
+    return net.contains('/');
 }
 
 }  // namespace web::dosguard

src/web/dosguard/WhitelistHandler.hpp

@@ -14,11 +14,11 @@
 #include <fmt/format.h>
 
 #include <algorithm>
-#include <regex>
+#include <optional>
 #include <string>
 #include <string_view>
-#include <unordered_map>
 #include <unordered_set>
+#include <utility>
 #include <vector>
 
 namespace web::dosguard {
@@ -36,16 +36,15 @@ public:
      * @brief Add network address to whitelist.
      *
      * @param net Network part of the ip address
-     * @throws std::runtime::error when the network address is not valid
+     * @return void on success, or an error string if the address is not valid
      */
-    void
+    std::expected<void, std::string>
     add(std::string_view net);
 
     /**
      * @brief Checks to see if ip address is whitelisted.
      *
      * @param ip IP address
-     * @throws std::runtime::error when the network address is not valid
      * @return true if the given IP is whitelisted; false otherwise
      */
     bool
@@ -76,21 +75,38 @@ class WhitelistHandler : public WhitelistHandlerInterface {
 
 public:
     /**
-     * @brief Adds all whitelisted IPs and masks from the given config.
+     * @brief Constructs a WhitelistHandler from an already-built Whitelist.
+     *
+     * @param whitelist The whitelist to use
+     */
+    explicit WhitelistHandler(Whitelist whitelist);
+
+    /**
+     * @brief Creates a WhitelistHandler by loading all whitelisted IPs and masks from config.
      *
      * @param config The Clio config to use
      * @param resolver The resolver to use for hostname resolution
+     * @return The WhitelistHandler on success, or an error string if any whitelist entry is invalid
      */
     template <SomeResolver HostnameResolverType = Resolver>
-    WhitelistHandler(
-        util::config::ClioConfigDefinition const& config,
-        HostnameResolverType&& resolver = {}
-    )
+    static std::expected<WhitelistHandler, std::string>
+    create(util::config::ClioConfigDefinition const& config, HostnameResolverType&& resolver = {})
     {
         std::unordered_set<std::string> const arr =
             getWhitelist(config, std::forward<HostnameResolverType>(resolver));
-        for (auto const& net : arr)
-            whitelist_.add(net);
+        Whitelist whitelist;
+        std::optional<std::string> errors;
+        for (auto const& net : arr) {
+            if (auto result = whitelist.add(net); !result.has_value()) {
+                if (!errors.has_value())
+                    errors.emplace();
+                errors->append(std::move(result).error());
+            }
+        }
+        if (errors.has_value()) {
+            return std::unexpected{std::move(errors).value()};
+        }
+        return WhitelistHandler(std::move(whitelist));
     }
 
     /**

tests/unit/rpc/RPCEngineTests.cpp

@@ -81,7 +81,9 @@ struct RPCEngineTest : util::prometheus::WithPrometheus,
 
     util::TagDecoratorFactory tagFactory{cfg};
     WorkQueue queue = WorkQueue::makeWorkQueue(cfg);
-    web::dosguard::WhitelistHandler whitelistHandler{cfg};
+    web::dosguard::WhitelistHandler whitelistHandler{
+        web::dosguard::WhitelistHandler::create(cfg).value()
+    };
     web::dosguard::Weights weights{1, {}};
     web::dosguard::DOSGuard dosGuard{cfg, whitelistHandler, weights};
     std::shared_ptr<MockHandlerProvider> handlerProvider = std::make_shared<MockHandlerProvider>();

tests/unit/web/ServerTests.cpp

@@ -150,13 +150,15 @@ struct WebServerTest : public virtual ::testing::Test {
     boost::asio::io_context ctxSync;
     std::string const port = std::to_string(tests::util::generateFreePort());
     ClioConfigDefinition cfg{getParseServerConfig(generateJSONWithDynamicPort(port))};
-    dosguard::WhitelistHandler whitelistHandler{cfg};
+    dosguard::WhitelistHandler whitelistHandler{dosguard::WhitelistHandler::create(cfg).value()};
     dosguard::Weights dosguardWeights{1, {}};
     dosguard::DOSGuard dosGuard{cfg, whitelistHandler, dosguardWeights};
     dosguard::IntervalSweepHandler sweepHandler{cfg, ctxSync, dosGuard};
 
     ClioConfigDefinition cfgOverload{getParseServerConfig(generateJSONDataOverload(port))};
-    dosguard::WhitelistHandler whitelistHandlerOverload{cfgOverload};
+    dosguard::WhitelistHandler whitelistHandlerOverload{
+        dosguard::WhitelistHandler::create(cfgOverload).value()
+    };
     dosguard::DOSGuard dosGuardOverload{cfgOverload, whitelistHandlerOverload, dosguardWeights};
     dosguard::IntervalSweepHandler sweepHandlerOverload{cfgOverload, ctxSync, dosGuardOverload};
     // this ctx is for http server
@@ -216,11 +218,26 @@ makeServerSync(
     std::reference_wrapper<data::LedgerCacheInterface const> cache
 )
 {
-    return web::makeHttpServer(config, ioc, dosGuard, handler, cache);
+    auto result = web::makeHttpServer(config, ioc, dosGuard, handler, cache);
+    [&]() { ASSERT_TRUE(result.has_value()); }();
+    return std::move(result).value();
 }
 
 }  // namespace
 
+TEST_F(WebServerTest, InvalidIpAddress)
+{
+    auto jsonConfig = generateJSONWithDynamicPort(port);
+    jsonConfig.as_object()["server"].as_object()["ip"] = "not-an-ip";
+
+    auto cache = MockLedgerCache();
+    auto const e = std::make_shared<EchoExecutor>();
+    auto const result =
+        web::makeHttpServer(getParseServerConfig(jsonConfig), ctx, dosGuard, e, cache);
+    ASSERT_FALSE(result.has_value());
+    EXPECT_THAT(result.error(), testing::HasSubstr("Invalid 'server.ip' config value"));
+}
+
 TEST_F(WebServerTest, Http)
 {
     auto cache = MockLedgerCache();
@@ -294,8 +311,9 @@ TEST_F(WebServerTest, IncompleteSslConfig)
     jsonConfig.as_object()["ssl_key_file"] = sslKeyFile.path;
 
     auto cache = MockLedgerCache();
-    auto const server = makeServerSync(getParseServerConfig(jsonConfig), ctx, dosGuard, e, cache);
-    EXPECT_EQ(server, nullptr);
+    auto const result =
+        web::makeHttpServer(getParseServerConfig(jsonConfig), ctx, dosGuard, e, cache);
+    EXPECT_FALSE(result.has_value());
 }
 
 TEST_F(WebServerTest, WrongSslConfig)
@@ -307,8 +325,9 @@ TEST_F(WebServerTest, WrongSslConfig)
     jsonConfig.as_object()["ssl_cert_file"] = "wrong_path";
 
     auto cache = MockLedgerCache();
-    auto const server = makeServerSync(getParseServerConfig(jsonConfig), ctx, dosGuard, e, cache);
-    EXPECT_EQ(server, nullptr);
+    auto const result =
+        web::makeHttpServer(getParseServerConfig(jsonConfig), ctx, dosGuard, e, cache);
+    EXPECT_FALSE(result.has_value());
 }
 
 TEST_F(WebServerTest, Https)
@@ -694,9 +713,8 @@ TEST_F(WebServerTest, AdminErrorCfgTestBothAdminPasswordAndLocalAdminSet)
     )};
 
     MockLedgerCache cache;
-    EXPECT_THROW(
-        web::makeHttpServer(serverConfig, ctx, dosGuardOverload, e, cache), std::logic_error
-    );
+    auto const result = web::makeHttpServer(serverConfig, ctx, dosGuardOverload, e, cache);
+    EXPECT_FALSE(result.has_value());
 }
 
 TEST_F(WebServerTest, AdminErrorCfgTestBothAdminPasswordAndLocalAdminFalse)
@@ -719,9 +737,8 @@ TEST_F(WebServerTest, AdminErrorCfgTestBothAdminPasswordAndLocalAdminFalse)
     )};
 
     MockLedgerCache cache;
-    EXPECT_THROW(
-        web::makeHttpServer(serverConfig, ctx, dosGuardOverload, e, cache), std::logic_error
-    );
+    auto const result = web::makeHttpServer(serverConfig, ctx, dosGuardOverload, e, cache);
+    EXPECT_FALSE(result.has_value());
 }
 
 struct WebServerPrometheusTest : util::prometheus::WithPrometheus, WebServerTest {};

tests/unit/web/dosguard/WhitelistHandlerTests.cpp

@@ -11,6 +11,7 @@
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
 
+#include <expected>
 #include <string>
 #include <string_view>
 #include <vector>
@@ -62,7 +63,9 @@ TEST_F(WhitelistHandlerTest, TestWhiteListIPV4)
     ClioConfigDefinition const cfg{
         getParseWhitelistHandlerConfig(boost::json::parse(kJSON_DATA_IP_V4))
     };
-    WhitelistHandler const whitelistHandler{cfg, mockResolver};
+    auto const result = WhitelistHandler::create(cfg, mockResolver);
+    ASSERT_TRUE(result.has_value());
+    auto const& whitelistHandler = *result;
 
     EXPECT_TRUE(whitelistHandler.isWhiteListed("192.168.1.10"));
     EXPECT_FALSE(whitelistHandler.isWhiteListed("193.168.0.123"));
@@ -86,7 +89,9 @@ TEST_F(WhitelistHandlerTest, TestWhiteListResolvesHostname)
     ClioConfigDefinition const cfg{
         getParseWhitelistHandlerConfig(boost::json::parse(kJSON_DATA_IP_V4))
     };
-    WhitelistHandler const whitelistHandler{cfg};
+    auto const result = WhitelistHandler::create(cfg);
+    ASSERT_TRUE(result.has_value());
+    auto const& whitelistHandler = *result;
 
     EXPECT_TRUE(whitelistHandler.isWhiteListed("127.0.0.1"));
     EXPECT_FALSE(whitelistHandler.isWhiteListed("193.168.0.123"));
@@ -110,10 +115,69 @@ TEST_F(WhitelistHandlerTest, TestWhiteListIPV6)
     ClioConfigDefinition const cfg{
         getParseWhitelistHandlerConfig(boost::json::parse(kJSON_DATA_IP_V6))
     };
-    WhitelistHandler const whitelistHandler{cfg};
+    auto const result = WhitelistHandler::create(cfg);
+    ASSERT_TRUE(result.has_value());
+    auto const& whitelistHandler = *result;
 
     EXPECT_TRUE(whitelistHandler.isWhiteListed("2002:1dd8:85a7:0000:0000:8a6e:0000:1111"));
     EXPECT_FALSE(whitelistHandler.isWhiteListed("2002:1dd8:85a7:1101:0000:8a6e:0000:1111"));
     EXPECT_TRUE(whitelistHandler.isWhiteListed("2001:0db8:85a3:0000:0000:8a2e:0000:0000"));
     EXPECT_TRUE(whitelistHandler.isWhiteListed("2001:0db8:85a3:0000:1111:8a2e:0370:7334"));
 }
+
+struct WhitelistTest : public virtual ::testing::Test {};
+
+TEST_F(WhitelistTest, AddValidIPV4)
+{
+    Whitelist whitelist;
+    EXPECT_TRUE(whitelist.add("1.2.3.4").has_value());
+}
+
+TEST_F(WhitelistTest, AddInvalidIP)
+{
+    Whitelist whitelist;
+    auto const result = whitelist.add("not-an-ip");
+    ASSERT_FALSE(result.has_value());
+    EXPECT_THAT(result.error(), testing::HasSubstr("not-an-ip"));
+}
+
+TEST_F(WhitelistTest, AddInvalidNetwork)
+{
+    Whitelist whitelist;
+    auto const result = whitelist.add("not-a-net/24");
+    ASSERT_FALSE(result.has_value());
+    EXPECT_THAT(result.error(), testing::HasSubstr("not-a-net/24"));
+}
+
+TEST_F(WhitelistTest, IsWhiteListedWithInvalidIP)
+{
+    Whitelist whitelist;
+    EXPECT_FALSE(whitelist.isWhiteListed("not-an-ip"));
+}
+
+TEST_F(WhitelistHandlerTest, CreateWithInvalidIPFails)
+{
+    struct MockResolver {
+        MOCK_METHOD(std::vector<std::string>, resolve, (std::string_view, std::string_view));
+        MOCK_METHOD(std::vector<std::string>, resolve, (std::string_view));
+    };
+
+    static constexpr auto kJSON = R"JSON(
+        {
+            "dos_guard": {
+                "whitelist": ["not-an-ip"]
+            }
+        }
+    )JSON";
+
+    testing::StrictMock<MockResolver> mockResolver;
+    EXPECT_CALL(mockResolver, resolve(testing::_))
+        .WillOnce([](auto hostname) -> std::vector<std::string> {
+            return {std::string{hostname}};
+        });
+
+    ClioConfigDefinition const cfg{getParseWhitelistHandlerConfig(boost::json::parse(kJSON))};
+    auto const result = WhitelistHandler::create(cfg, mockResolver);
+    ASSERT_FALSE(result.has_value());
+    EXPECT_THAT(result.error(), testing::HasSubstr("not-an-ip"));
+}