Fix header_rewrite MaxMind geo lookups for GeoIP2/GeoLite2 mmdb databases#12948
Open
bryancall wants to merge 3 commits intoapache:masterfrom
Open
Fix header_rewrite MaxMind geo lookups for GeoIP2/GeoLite2 mmdb databases#12948bryancall wants to merge 3 commits intoapache:masterfrom
bryancall wants to merge 3 commits intoapache:masterfrom
Conversation
The MaxMind geo lookup code used incorrect field paths that don't match the structure of standard GeoLite2 and DBIP mmdb databases. Country lookups used a flat "country_code" field that doesn't exist; the correct path is "country" -> "names" -> "en" for country name and "country" -> "iso_code" for the ISO code. Changes: - Fix GEO_QUAL_COUNTRY to use nested path "country/names/en" - Add missing GEO_QUAL_COUNTRY_ISO support using "country/iso_code" - Add has_data and type checks before accessing entry data - Fix memory leak: set gMaxMindDB to nullptr after delete on open failure - Ensure entry_data_list is freed on all code paths Verified with DBIP Country Lite and ASN Lite databases using the same MMDB_get_value paths. Matches the field paths used by maxmind_acl plugin. Fixes: apache#11812
Contributor
Author
|
[approve ci autest 1] |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the header_rewrite plugin’s MaxMind (MMDB) geo lookup implementation to better match the field layout used by standard GeoLite2 / DBIP databases, and improves cleanup/error-handling around lookups and initialization.
Changes:
- Adjust MMDB lookup paths for geo string fields and add handling for
GEO_QUAL_COUNTRY_ISOin the string lookup function. - Improve robustness by validating
entry_databefore reading values and ensuring MMDB entry data lists are freed on more return paths. - Fix a retry reliability issue by nulling
gMaxMindDBafterdeleteonMMDB_open()failure.
You can also share your feedback on Copilot code review. Take the survey.
- GEO_QUAL_COUNTRY now returns iso_code ("KR") instead of English
name ("South Korea"), matching the GeoIP backend behavior
- Remove dead GEO_QUAL_COUNTRY_ISO case from get_geo_string() since
the dispatcher routes it to get_geo_int()
- Remove unnecessary MMDB_get_entry_data_list() allocation -- the
MMDB_get_value() API works directly from result.entry
bryancall
changed the title
Adds a test that verifies the MMDB lookup paths used by the geo conditions (country/iso_code, country/names/en) work correctly against a real MaxMind or DBIP database. The test skips gracefully if no MMDB file is available, and can be pointed at a specific file via MMDB_COUNTRY_PATH environment variable.
zwoop
reviewed
Mar 9, 2026
| int gai_error, mmdb_error; | ||
| MMDB_lookup_result_s result = MMDB_lookup_string(&mmdb, "8.8.8.8", &gai_error, &mmdb_error); | ||
|
|
||
| if (MMDB_SUCCESS != mmdb_error || !result.found_entry) { |
Contributor
There was a problem hiding this comment.
bike shed, but do we not want to use the gai_error here (and in the other place) ? Should it be in the error message? I don't know what it contains, but we collect it and never use it.
zwoop
approved these changes
Mar 9, 2026
Contributor
zwoop
left a comment
zwoop
left a comment
There was a problem hiding this comment.
Looks good. I made a comment, but it's no big deal.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The header_rewrite plugin's MaxMind geo lookup code used incorrect MMDB
field paths that don't match the structure of standard MaxMind GeoIP2
databases (GeoLite2, GeoIP2, DBIP). All geo lookups (
%{GEO:COUNTRY},%{GEO:ASN},%{GEO:ASN-NAME}) returned(unknown)or-1evenwhen the mmdb file loaded successfully. The code queried flat field names
like
"country_code"that don't exist -- these databases use nestedstructures like
country -> iso_code.Changes
GEO_QUAL_COUNTRYfrom flat"country_code"to nested"country", "iso_code"path, returningthe ISO country code (e.g. "KR", "US") consistent with the old GeoIP
backend's
GeoIP_country_code_by_ipnum()behaviorinitLibrary-- setgMaxMindDB = nullptrafterdeleteonMMDB_openfailure sosubsequent calls can retry instead of seeing a dangling pointer
entry_data.has_dataandentry_data.typebefore accessing entry values to avoid readinguninitialized data
MMDB_get_entry_data_listallocation -- theMMDB_get_value()API works directly fromresult.entry, so theexpensive list allocation/traversal was unnecessary overhead on every
geo lookup
Testing
Country Lite and ASN Lite databases on eris (ASAN build)
COUNTRY: KRCOUNTRY: USASN-NAME: KINX,ASN: 9286(unknown)/-1mmdblookupconfirms paths match database structureFixes: #11812