When AppSec is enabled; unconditionally add http.client_ip (#184)

Author: cataphract

src/security/collection.cpp

@@ -4,17 +4,13 @@
 
 #include <cassert>
 #include <cctype>
-#include <charconv>
 #include <cstring>
-#include <functional>
 #include <string_view>
 #include <unordered_map>
 
 #include "../string_util.h"
-#include "client_ip.h"
 #include "ddwaf_obj.h"
 #include "decode.h"
-#include "library.h"
 #include "util.h"
 
 extern "C" {
@@ -54,7 +50,8 @@ class ReqSerializer {
  public:
   explicit ReqSerializer(dnsec::DdwafMemres &memres) : memres_{memres} {}
 
-  ddwaf_object *serialize(const ngx_http_request_t &request) {
+  ddwaf_object *serialize(const ngx_http_request_t &request,
+                          const std::optional<std::string> &client_ip) {
     dnsec::ddwaf_obj *root = memres_.allocate_objects<dnsec::ddwaf_obj>(1);
     dnsec::ddwaf_map_obj &root_map = root->make_map(6, memres_);
 
@@ -63,7 +60,7 @@ class ReqSerializer {
     set_request_method(request, root_map.at_unchecked(2));
     set_request_headers_nocookies(request, root_map.at_unchecked(3));
     set_request_cookie(request, root_map.at_unchecked(4));
-    set_client_ip(request, root_map.at_unchecked(5));
+    set_client_ip(client_ip, root_map.at_unchecked(5));
 
     return root;
   }
@@ -312,11 +309,8 @@ class ReqSerializer {
     set_value_from_iter(iter, slot);
   }
 
-  void set_client_ip(const ngx_http_request_t &request,
+  void set_client_ip(const std::optional<std::string> &cl_ip,
                      dnsec::ddwaf_obj &slot) {
-    dnsec::ClientIp client_ip{dnsec::Library::custom_ip_header(), request};
-    std::optional<std::string> cl_ip = client_ip.resolve();
-
     slot.set_key(kClientIp);
     if (!cl_ip) {
       slot.make_null();
@@ -381,9 +375,10 @@ class ReqSerializer {
 namespace datadog::nginx::security {
 
 ddwaf_object *collect_request_data(const ngx_http_request_t &request,
+                                   const std::optional<std::string> &client_ip,
                                    DdwafMemres &memres) {
   ReqSerializer rs{memres};
-  return rs.serialize(request);
+  return rs.serialize(request, client_ip);
 }
 
 ddwaf_object *collect_response_data(const ngx_http_request_t &request,

src/security/collection.h

@@ -2,6 +2,8 @@
 
 #include <ddwaf.h>
 
+#include <optional>
+
 #include "ddwaf_memres.h"
 
 extern "C" {
@@ -11,6 +13,7 @@ extern "C" {
 namespace datadog::nginx::security {
 
 ddwaf_object *collect_request_data(const ngx_http_request_t &request,
+                                   const std::optional<std::string> &client_ip,
                                    DdwafMemres &memres);
 ddwaf_object *collect_response_data(const ngx_http_request_t &request,
                                     DdwafMemres &memres);

src/security/context.cpp

@@ -13,6 +13,7 @@
 #include "../ngx_http_datadog_module.h"
 #include "blocking.h"
 #include "body_parse/body_parsing.h"
+#include "client_ip.h"
 #include "collection.h"
 #include "ddwaf_obj.h"
 #include "header_tags.h"
@@ -679,7 +680,12 @@ std::optional<BlockSpecification> Context::run_waf_start(
   static const std::string_view libddwaf_version{ddwaf_get_version()};
   span.set_tag("_dd.appsec.waf.version", libddwaf_version);
 
-  ddwaf_object *data = collect_request_data(req, memres_);
+  dnsec::ClientIp ip_resolver{dnsec::Library::custom_ip_header(), req};
+  auto client_ip = ip_resolver.resolve();
+
+  ddwaf_object *data = collect_request_data(req, client_ip, memres_);
+
+  client_ip_ = std::move(client_ip);
 
   ddwaf_result result;
   auto code =
@@ -1632,6 +1638,7 @@ void Context::do_on_main_log_request(ngx_http_request_t &request,
 
   set_header_tags(has_matches(), request, span);
   report_matches(request, span);
+  report_client_ip(span);
 }
 
 bool Context::has_matches() const noexcept { return !results_.empty(); }
@@ -1645,4 +1652,12 @@ void Context::report_matches(ngx_http_request_t &request, dd::Span &span) {
   results_.clear();
 }
 
+void Context::report_client_ip(dd::Span &span) const {
+  if (!client_ip_) {
+    return;
+  }
+
+  span.set_tag("http.client_ip"sv, *client_ip_);
+}
+
 }  // namespace datadog::nginx::security

src/security/context.h

@@ -98,6 +98,7 @@ class Context {
 
   bool has_matches() const noexcept;
   void report_matches(ngx_http_request_t &request, dd::Span &span);
+  void report_client_ip(dd::Span &span) const;
 
   enum class stage {
     /* Set on on_request_start (NGX_HTTP_ACCESS_PHASE) if there's no thread
@@ -191,6 +192,7 @@ class Context {
   std::vector<OwnedDdwafResult> results_;
   OwnedDdwafContext ctx_{nullptr};
   DdwafMemres memres_;
+  std::optional<std::string> client_ip_;
 
   static inline constexpr std::size_t kMaxFilterData = 40 * 1024;
   static inline constexpr std::size_t kDefaultMaxSavedOutputData = 256 * 1024;

src/security/library.h

@@ -4,7 +4,6 @@
 
 #include <atomic>
 #include <memory>
-#include <string>
 #include <string_view>
 
 #include "../datadog_conf.h"

test/cases/sec_addresses/test_sec_addresses.py

@@ -347,6 +347,24 @@ def test_client_ip_13(self):
             result['triggers'][0]['rule_matches'][0]['parameters'][0]['value'],
             'fe80::1')
 
+    def test_client_ip_meta(self):
+        status, _, _ = self.orch.send_nginx_http_request(
+            '/http', 80, {'cf-connecting-ip': '10.10.10.10'})
+        self.assertEqual(status, 200)
+
+        self.orch.reload_nginx()
+        log_lines = self.orch.sync_service("agent")
+        traces = [
+            json.loads(line) for line in log_lines if line.startswith("[[{")
+        ]
+        found = any(
+            span[0].get("meta", {}).get("http.client_ip") == "10.10.10.10"
+            for trace in traces for span in trace)
+
+        if not found:
+            self.fail("No trace found with http.client_ip=10.10.10.10 in " +
+                      str(traces))
+
     def test_client_ip_prio_1(self):
         result = self.do_request_headers({
             'x-real-ip': '8.8.8.8',