Fix header_rewrite MaxMind geo lookups for GeoIP2/GeoLite2 mmdb databases

plugins/header_rewrite/CMakeLists.txt

@@ -60,6 +60,7 @@ if(BUILD_TESTING)
   target_link_libraries(test_header_rewrite PRIVATE header_rewrite_parser ts::inkevent ts::tscore)
 
   if(maxminddb_FOUND)
+    target_compile_definitions(test_header_rewrite PRIVATE TS_USE_HRW_MAXMINDDB=1)
     target_link_libraries(test_header_rewrite PRIVATE maxminddb::maxminddb)
   endif()
 

plugins/header_rewrite/conditions_geo_maxmind.cc

@@ -51,6 +51,7 @@ MMConditionGeo::initLibrary(const std::string &path)
   if (MMDB_SUCCESS != status) {
     Dbg(pi_dbg_ctl, "Cannot open %s - %s", path.c_str(), MMDB_strerror(status));
     delete gMaxMindDB;
+    gMaxMindDB = nullptr; // avoid leaving a dangling global pointer after delete
     return;
   }
   Dbg(pi_dbg_ctl, "Loaded %s", path.c_str());
@@ -74,48 +75,39 @@ MMConditionGeo::get_geo_string(const sockaddr *addr) const
     return ret;
   }
 
-  MMDB_entry_data_list_s *entry_data_list = nullptr;
   if (!result.found_entry) {
     Dbg(pi_dbg_ctl, "No entry for this IP was found");
     return ret;
   }
 
-  int status = MMDB_get_entry_data_list(&result.entry, &entry_data_list);
-  if (MMDB_SUCCESS != status) {
-    Dbg(pi_dbg_ctl, "Error looking up entry data: %s", MMDB_strerror(status));
-    return ret;
-  }
-
-  if (entry_data_list == nullptr) {
-    Dbg(pi_dbg_ctl, "No data found");
-    return ret;
-  }
+  MMDB_entry_data_s entry_data;
+  int               status;
 
-  const char *field_name;
+  // GeoLite2/GeoIP2/DBIP databases use nested field paths, not flat names.
+  // Use MMDB_get_value() directly on the entry -- no need for the more
+  // expensive MMDB_get_entry_data_list() allocation.
   switch (_geo_qual) {
   case GEO_QUAL_COUNTRY:
-    field_name = "country_code";
+    // "country" -> "iso_code" returns e.g. "US", "KR" (matches old GeoIP backend behavior)
+    status = MMDB_get_value(&result.entry, &entry_data, "country", "iso_code", NULL);
     break;
   case GEO_QUAL_ASN_NAME:
-    field_name = "autonomous_system_organization";
+    status = MMDB_get_value(&result.entry, &entry_data, "autonomous_system_organization", NULL);
     break;
   default:
     Dbg(pi_dbg_ctl, "Unsupported field %d", _geo_qual);
     return ret;
-    break;
   }
 
-  MMDB_entry_data_s entry_data;
-
-  status = MMDB_get_value(&result.entry, &entry_data, field_name, NULL);
   if (MMDB_SUCCESS != status) {
-    Dbg(pi_dbg_ctl, "ERROR on get value asn value: %s", MMDB_strerror(status));
+    Dbg(pi_dbg_ctl, "Error looking up geo string field: %s", MMDB_strerror(status));
     return ret;
   }
-  ret = std::string(entry_data.utf8_string, entry_data.data_size);
 
-  if (nullptr != entry_data_list) {
-    MMDB_free_entry_data_list(entry_data_list);
+  // Validate before access -- entry_data may be uninitialized if the field
+  // exists but has an unexpected type in a third-party database.
+  if (entry_data.has_data && entry_data.type == MMDB_DATA_TYPE_UTF8_STRING) {
+    ret = std::string(entry_data.utf8_string, entry_data.data_size);
   }
 
   return ret;
@@ -139,45 +131,31 @@ MMConditionGeo::get_geo_int(const sockaddr *addr) const
     return ret;
   }
 
-  MMDB_entry_data_list_s *entry_data_list = nullptr;
   if (!result.found_entry) {
     Dbg(pi_dbg_ctl, "No entry for this IP was found");
     return ret;
   }
 
-  int status = MMDB_get_entry_data_list(&result.entry, &entry_data_list);
-  if (MMDB_SUCCESS != status) {
-    Dbg(pi_dbg_ctl, "Error looking up entry data: %s", MMDB_strerror(status));
-    return ret;
-  }
-
-  if (entry_data_list == nullptr) {
-    Dbg(pi_dbg_ctl, "No data found");
-    return ret;
-  }
+  MMDB_entry_data_s entry_data;
+  int               status;
 
-  const char *field_name;
   switch (_geo_qual) {
   case GEO_QUAL_ASN:
-    field_name = "autonomous_system_number";
+    // GeoLite2-ASN / DBIP-ASN store this as a top-level uint32 field
+    status = MMDB_get_value(&result.entry, &entry_data, "autonomous_system_number", NULL);
     break;
   default:
     Dbg(pi_dbg_ctl, "Unsupported field %d", _geo_qual);
     return ret;
-    break;
   }
 
-  MMDB_entry_data_s entry_data;
-
-  status = MMDB_get_value(&result.entry, &entry_data, field_name, NULL);
   if (MMDB_SUCCESS != status) {
-    Dbg(pi_dbg_ctl, "ERROR on get value asn value: %s", MMDB_strerror(status));
+    Dbg(pi_dbg_ctl, "Error looking up geo int field: %s", MMDB_strerror(status));
     return ret;
   }
-  ret = entry_data.uint32;
 
-  if (nullptr != entry_data_list) {
-    MMDB_free_entry_data_list(entry_data_list);
+  if (entry_data.has_data && entry_data.type == MMDB_DATA_TYPE_UINT32) {
+    ret = entry_data.uint32;
   }
 
   return ret;

plugins/header_rewrite/header_rewrite_test.cc

@@ -24,9 +24,16 @@
 #include <cstdarg>
 #include <iostream>
 #include <ostream>
+#include <sys/stat.h>
 
 #include "parser.h"
 
+#if TS_USE_HRW_MAXMINDDB
+#include <maxminddb.h>
+#include <arpa/inet.h>
+#include <netdb.h>
+#endif
+
 namespace header_rewrite_ns
 {
 const char PLUGIN_NAME[]     = "TEST_header_rewrite";
@@ -538,12 +545,132 @@ test_tokenizer()
   return errors;
 }
 
+#if TS_USE_HRW_MAXMINDDB
+static bool
+file_exists(const char *path)
+{
+  struct stat st;
+  return stat(path, &st) == 0;
+}
+
+static const char *
+find_country_mmdb()
+{
+  static const char *paths[] = {
+    "/usr/share/GeoIP/GeoLite2-Country.mmdb", "/usr/local/share/GeoIP/GeoLite2-Country.mmdb",
+    "/var/lib/GeoIP/GeoLite2-Country.mmdb",   "/opt/geoip/GeoLite2-Country.mmdb",
+    "/usr/share/GeoIP/GeoIP2-Country.mmdb",   "/usr/share/GeoIP/dbip-country-lite.mmdb",
+  };
+  for (auto *p : paths) {
+    if (file_exists(p)) {
+      return p;
+    }
+  }
+  if (const char *env = getenv("MMDB_COUNTRY_PATH")) {
+    if (file_exists(env)) {
+      return env;
+    }
+  }
+  return nullptr;
+}
+
+int
+test_maxmind_geo()
+{
+  const char *db_path = find_country_mmdb();
+  if (db_path == nullptr) {
+    std::cout << "SKIP: No MaxMind country mmdb found (set MMDB_COUNTRY_PATH to override)" << std::endl;
+    return 0;
+  }
+
+  std::cout << "Testing MaxMind geo lookups with: " << db_path << std::endl;
+
+  int    errors = 0;
+  MMDB_s mmdb;
+  int    status = MMDB_open(db_path, MMDB_MODE_MMAP, &mmdb);
+  if (MMDB_SUCCESS != status) {
+    std::cerr << "Cannot open " << db_path << ": " << MMDB_strerror(status) << std::endl;
+    return 1;
+  }
+
+  // MMDB_lookup_string() returns two independent error codes:
+  //   gai_error  - getaddrinfo() failure (string-to-IP conversion)
+  //   mmdb_error - MMDB lookup failure (database query)
+  // Check both to avoid masking failures.
+  int                  gai_error, mmdb_error;
+  MMDB_lookup_result_s result = MMDB_lookup_string(&mmdb, "8.8.8.8", &gai_error, &mmdb_error);
+
+  if (gai_error != 0) {
+    std::cerr << "getaddrinfo failed for 8.8.8.8: " << gai_strerror(gai_error) << std::endl;
+    MMDB_close(&mmdb);
+    return 1;
+  }
+  if (MMDB_SUCCESS != mmdb_error || !result.found_entry) {
+    std::cerr << "Cannot look up 8.8.8.8 in " << db_path << ": " << MMDB_strerror(mmdb_error) << std::endl;
+    MMDB_close(&mmdb);
+    return 1;
+  }
+
+  MMDB_entry_data_s entry_data;
+
+  // Verify "country" -> "iso_code" path (used by GEO_QUAL_COUNTRY)
+  status = MMDB_get_value(&result.entry, &entry_data, "country", "iso_code", NULL);
+  if (MMDB_SUCCESS != status || !entry_data.has_data || entry_data.type != MMDB_DATA_TYPE_UTF8_STRING) {
+    std::cerr << "FAIL: country/iso_code lookup failed for 8.8.8.8" << std::endl;
+    ++errors;
+  } else {
+    std::string iso(entry_data.utf8_string, entry_data.data_size);
+    if (iso != "US") {
+      std::cerr << "FAIL: expected country iso_code 'US' for 8.8.8.8, got '" << iso << "'" << std::endl;
+      ++errors;
+    } else {
+      std::cout << "  PASS: country/iso_code = " << iso << std::endl;
+    }
+  }
+
+  // Verify "country" -> "names" -> "en" path exists (not used by header_rewrite but validates structure)
+  status = MMDB_get_value(&result.entry, &entry_data, "country", "names", "en", NULL);
+  if (MMDB_SUCCESS != status || !entry_data.has_data || entry_data.type != MMDB_DATA_TYPE_UTF8_STRING) {
+    std::cerr << "FAIL: country/names/en lookup failed for 8.8.8.8" << std::endl;
+    ++errors;
+  } else {
+    std::string name(entry_data.utf8_string, entry_data.data_size);
+    std::cout << "  PASS: country/names/en = " << name << std::endl;
+  }
+
+  // Verify loopback returns no entry
+  result = MMDB_lookup_string(&mmdb, "127.0.0.1", &gai_error, &mmdb_error);
+  if (gai_error != 0) {
+    std::cerr << "FAIL: getaddrinfo failed for 127.0.0.1: " << gai_strerror(gai_error) << std::endl;
+    ++errors;
+  } else if (MMDB_SUCCESS == mmdb_error && result.found_entry) {
+    std::cerr << "FAIL: expected no entry for 127.0.0.1" << std::endl;
+    ++errors;
+  } else {
+    std::cout << "  PASS: 127.0.0.1 correctly returns no entry" << std::endl;
+  }
+
+  MMDB_close(&mmdb);
+
+  if (errors == 0) {
+    std::cout << "MaxMind geo tests passed" << std::endl;
+  }
+  return errors;
+}
+#endif
+
 int
 main()
 {
   if (test_parsing() || test_processing() || test_tokenizer()) {
     return 1;
   }
 
+#if TS_USE_HRW_MAXMINDDB
+  if (test_maxmind_geo()) {
+    return 1;
+  }
+#endif
+
   return 0;
 }