From 3ef5dc296500cbb5a4d5a9429b363fd80acc5af6 Mon Sep 17 00:00:00 2001 From: Hubert Parzych Date: Mon, 29 Jan 2024 07:32:49 +0100 Subject: [PATCH 1/4] refactor: fix a typo in error message --- libservice/src/Aggregator.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libservice/src/Aggregator.cpp b/libservice/src/Aggregator.cpp index 02a56116..d01caa90 100644 --- a/libservice/src/Aggregator.cpp +++ b/libservice/src/Aggregator.cpp @@ -16,7 +16,7 @@ static std::string getEndpoint(const std::string& host, const std::string& url) static bool isIpv4ClientExternal(const IpAddressChecker& ipChecker, const std::string& addr) { in_addr_t clientAddrBinary; if (inet_pton(AF_INET, addr.c_str(), &clientAddrBinary) != 1) { - throw std::runtime_error("Cannot parse IPv4 client address: {}" + addr); + throw std::runtime_error("Can't parse IPv4 client address: " + addr); } return ipChecker.isV4AddressExternal(clientAddrBinary); } @@ -24,7 +24,7 @@ static bool isIpv4ClientExternal(const IpAddressChecker& ipChecker, const std::s static bool isIpv6ClientExternal(const IpAddressChecker& ipChecker, const std::string& addr) { in6_addr clientAddrBinary{}; if (inet_pton(AF_INET6, addr.c_str(), &clientAddrBinary) != 1) { - throw std::runtime_error("Cannot parse IPv6 client address: {}" + addr); + throw std::runtime_error("Can't parse IPv6 client address: " + addr); } return ipChecker.isV6AddressExternal(clientAddrBinary); } From 368bb5f29b86a72c8fb799371df26482a3e0aa5f Mon Sep 17 00:00:00 2001 From: Hubert Parzych Date: Mon, 29 Jan 2024 07:42:28 +0100 Subject: [PATCH 2/4] fix: Remove using src address metadata in case of X-Forwarded-For --- libservice/src/Aggregator.cpp | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/libservice/src/Aggregator.cpp b/libservice/src/Aggregator.cpp index d01caa90..eb689ff7 100644 --- a/libservice/src/Aggregator.cpp +++ b/libservice/src/Aggregator.cpp @@ -16,7 +16,7 @@ static std::string getEndpoint(const std::string& host, const std::string& url) static bool isIpv4ClientExternal(const IpAddressChecker& ipChecker, const std::string& addr) { in_addr_t clientAddrBinary; if (inet_pton(AF_INET, addr.c_str(), &clientAddrBinary) != 1) { - throw std::runtime_error("Can't parse IPv4 client address: " + addr); + return true; } return ipChecker.isV4AddressExternal(clientAddrBinary); } @@ -24,28 +24,35 @@ static bool isIpv4ClientExternal(const IpAddressChecker& ipChecker, const std::s static bool isIpv6ClientExternal(const IpAddressChecker& ipChecker, const std::string& addr) { in6_addr clientAddrBinary{}; if (inet_pton(AF_INET6, addr.c_str(), &clientAddrBinary) != 1) { - throw std::runtime_error("Can't parse IPv6 client address: " + addr); + return true; } return ipChecker.isV6AddressExternal(clientAddrBinary); } +static bool isClientExternal(const IpAddressChecker& ipChecker, const std::string& addr) { + bool isPossiblyIpv6{addr.find(':') != std::string::npos}; + return isPossiblyIpv6 ? isIpv6ClientExternal(ipChecker, addr) : isIpv4ClientExternal(ipChecker, addr); +} + static bool isClientExternal(const IpAddressChecker& ipChecker, const std::string& addr, bool isIpV6) { return isIpV6 ? isIpv6ClientExternal(ipChecker, addr) : isIpv4ClientExternal(ipChecker, addr); } static void incrementServiceClientsNumber( const IpAddressChecker& ipChecker, Service& service, const httpparser::HttpRequest& request, const DiscoverySessionMeta& meta) { - std::string clientAddr; + bool isExternal{false}; if (!request.xForwardedFor.empty()) { - clientAddr = request.xForwardedFor.front(); + isExternal = isClientExternal(ipChecker, request.xForwardedFor.front()); } else if (discoverySessionFlagsIsIPv4(meta.flags)) { - clientAddr = ipv4ToString(meta.sourceIPData); + isExternal = isClientExternal(ipChecker, ipv4ToString(meta.sourceIPData), false); } else if (discoverySessionFlagsIsIPv6(meta.flags)) { - clientAddr = ipv6ToString(meta.sourceIPData); + isExternal = isClientExternal(ipChecker, ipv6ToString(meta.sourceIPData), true); + } else { + return; } try { - if (isClientExternal(ipChecker, clientAddr, discoverySessionFlagsIsIPv6(meta.flags))) { + if (isExternal) { ++service.externalClientsNumber; } else { ++service.internalClientsNumber; From f7a517dba38f1ca5c4547b4fc23afeb49c90fc3a Mon Sep 17 00:00:00 2001 From: Hubert Parzych Date: Thu, 1 Feb 2024 08:24:33 +0100 Subject: [PATCH 3/4] fix: Ignore request if X-Forwarded-For couldn't be parsed --- libservice/src/Aggregator.cpp | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/libservice/src/Aggregator.cpp b/libservice/src/Aggregator.cpp index eb689ff7..6d77affa 100644 --- a/libservice/src/Aggregator.cpp +++ b/libservice/src/Aggregator.cpp @@ -13,29 +13,30 @@ static std::string getEndpoint(const std::string& host, const std::string& url) return host + url; } -static bool isIpv4ClientExternal(const IpAddressChecker& ipChecker, const std::string& addr) { +static std::optional isIpv4ClientExternal(const IpAddressChecker& ipChecker, const std::string& addr) { in_addr_t clientAddrBinary; if (inet_pton(AF_INET, addr.c_str(), &clientAddrBinary) != 1) { - return true; + return std::nullopt; } return ipChecker.isV4AddressExternal(clientAddrBinary); } -static bool isIpv6ClientExternal(const IpAddressChecker& ipChecker, const std::string& addr) { +static std::optional isIpv6ClientExternal(const IpAddressChecker& ipChecker, const std::string& addr) { in6_addr clientAddrBinary{}; if (inet_pton(AF_INET6, addr.c_str(), &clientAddrBinary) != 1) { - return true; + return std::nullopt; } return ipChecker.isV6AddressExternal(clientAddrBinary); } -static bool isClientExternal(const IpAddressChecker& ipChecker, const std::string& addr) { - bool isPossiblyIpv6{addr.find(':') != std::string::npos}; - return isPossiblyIpv6 ? isIpv6ClientExternal(ipChecker, addr) : isIpv4ClientExternal(ipChecker, addr); +static bool isClientExternal(const IpAddressChecker& ipChecker, const std::string& addr, bool isIpv6) { + auto isExternal = isIpv6 ? isIpv6ClientExternal(ipChecker, addr) : isIpv4ClientExternal(ipChecker, addr); + return isExternal.value_or(false); } -static bool isClientExternal(const IpAddressChecker& ipChecker, const std::string& addr, bool isIpV6) { - return isIpV6 ? isIpv6ClientExternal(ipChecker, addr) : isIpv4ClientExternal(ipChecker, addr); +static bool isClientExternal(const IpAddressChecker& ipChecker, const std::string& addr) { + bool isPossiblyIpv6{std::count(addr.begin(), addr.end(), ':') >= 2}; + return isClientExternal(ipChecker, addr, isPossiblyIpv6); } static void incrementServiceClientsNumber( @@ -51,15 +52,10 @@ static void incrementServiceClientsNumber( return; } - try { - if (isExternal) { - ++service.externalClientsNumber; - } else { - ++service.internalClientsNumber; - } - } catch (const std::runtime_error& e) { - LOG_TRACE(e.what()); - return; + if (isExternal) { + ++service.externalClientsNumber; + } else { + ++service.internalClientsNumber; } } From 7293f1f14d04b42c10d517cb85f8e80628a3ddf2 Mon Sep 17 00:00:00 2001 From: "Parzych, Hubert" Date: Thu, 1 Feb 2024 15:06:44 +0100 Subject: [PATCH 4/4] fix: Add error handling and logging --- libservice/src/Aggregator.cpp | 35 +++++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/libservice/src/Aggregator.cpp b/libservice/src/Aggregator.cpp index c3d00b22..fcb767c0 100644 --- a/libservice/src/Aggregator.cpp +++ b/libservice/src/Aggregator.cpp @@ -28,25 +28,24 @@ static std::string getEndpoint(const std::string& host, const std::string& url) return host + url; } -static std::optional isIpv4ClientExternal(const IpAddressChecker& ipChecker, const std::string& addr) { +static bool isIpv4ClientExternal(const IpAddressChecker& ipChecker, const std::string& addr) { in_addr_t clientAddrBinary; if (inet_pton(AF_INET, addr.c_str(), &clientAddrBinary) != 1) { - return std::nullopt; + throw std::runtime_error("Couldn't parse IPv4 client address"); } return ipChecker.isV4AddressExternal(clientAddrBinary); } -static std::optional isIpv6ClientExternal(const IpAddressChecker& ipChecker, const std::string& addr) { +static bool isIpv6ClientExternal(const IpAddressChecker& ipChecker, const std::string& addr) { in6_addr clientAddrBinary{}; if (inet_pton(AF_INET6, addr.c_str(), &clientAddrBinary) != 1) { - return std::nullopt; + throw std::runtime_error("Couldn't parse IPv6 client address"); } return ipChecker.isV6AddressExternal(clientAddrBinary); } static bool isClientExternal(const IpAddressChecker& ipChecker, const std::string& addr, bool isIpv6) { - auto isExternal = isIpv6 ? isIpv6ClientExternal(ipChecker, addr) : isIpv4ClientExternal(ipChecker, addr); - return isExternal.value_or(false); + return isIpv6 ? isIpv6ClientExternal(ipChecker, addr) : isIpv4ClientExternal(ipChecker, addr); } static bool isClientExternal(const IpAddressChecker& ipChecker, const std::string& addr) { @@ -57,14 +56,22 @@ static bool isClientExternal(const IpAddressChecker& ipChecker, const std::strin static void incrementServiceClientsNumber( const IpAddressChecker& ipChecker, Service& service, const httpparser::HttpRequest& request, const DiscoverySessionMeta& meta) { bool isExternal{false}; - if (!request.xForwardedFor.empty()) { - isExternal = isClientExternal(ipChecker, request.xForwardedFor.front()); - } else if (discoverySessionFlagsIsIPv4(meta.flags)) { - isExternal = isClientExternal(ipChecker, ipv4ToString(meta.sourceIPData), false); - } else if (discoverySessionFlagsIsIPv6(meta.flags)) { - isExternal = isClientExternal(ipChecker, ipv6ToString(meta.sourceIPData), true); - } else { - return; + std::string clientAddr; + try { + if (!request.xForwardedFor.empty()) { + clientAddr = request.xForwardedFor.front(); + isExternal = isClientExternal(ipChecker, clientAddr); + } else if (discoverySessionFlagsIsIPv4(meta.flags)) { + clientAddr = ipv4ToString(meta.sourceIPData); + isExternal = isClientExternal(ipChecker, clientAddr, false); + } else if (discoverySessionFlagsIsIPv6(meta.flags)) { + clientAddr = ipv6ToString(meta.sourceIPData); + isExternal = isClientExternal(ipChecker, clientAddr, true); + } else { + return; + } + } catch (const std::runtime_error& e) { + LOG_TRACE("Couldn't determine if the client is external: {} (client address: {})", e.what(), clientAddr); } if (isExternal) {