Bug 239334

Summary: mail/milter-greylist: Restore GeoIP support, utilizing libmaxminddb
Product: Ports & Packages Reporter: Harald Schmalzbauer <bugzilla.freebsd>
Component: Individual Port(s)Assignee: freebsd-ports-bugs mailing list <ports-bugs>
Status: Open ---    
Severity: Affects Some People CC: adamw, dewaynegeraghty, m.tsatsenko, zi
Priority: --- Keywords: needs-patch, needs-qa
Version: LatestFlags: bugzilla: maintainer-feedback? (m.tsatsenko)
Hardware: Any   
OS: Any   
URL: https://github.com/mwennrich/milter-greylist-geoip2
Attachments:
Description Flags
Add libmaxminddb (GeoIP2) support to mitler-greylist
none
Add libmaxminddb (GeoIP2) support to mitler-greylist and switch to autoreconf
none
patch
none
the patch m.tsatsenko: maintainer-approval+

Description Harald Schmalzbauer 2019-07-20 12:03:10 UTC
Created attachment 205923 [details]
Add libmaxminddb (GeoIP2) support to mitler-greylist

In r490200, GeoIP support was authoritatively removed.  The reason was that Maxmind no longer provides geolocation data in the legacy format used by net/GeoIP, which milter-greylist uses. (https://svnweb.freebsd.org/ports?view=revision&revision=490211).
While there is a UPDATING record, this action has an unexpected and big impact for users already using GeoIP databases with milter-greylist.  It had to be left to their decision if they want to use outdated data, or abandon GeoIP support at all.  Especially in the milter-greylist case, I can't imagine anybody would consider removing GeoIP support was the correct solution. I guess they would live with outdated data until somebody skilled spends a significant amount of time to implement the successor library net/libmaxminddb, which we already have in the ports tree.

Thankfully, Markus Wennrich did that job.  His decision was to publish a complete fork on github: https://github.com/mwennrich/milter-greylist-geoip2

I simply created a patch from his commit, keeping the existing mail/milter-greylist port using the original sources, and adding support by unconditionally applying the patch enabling support for --with-libmaxminddb.

I'm not sure if it's wise to cary such a big patch containing ./configure modifications, which are done by autoconf.  Maybe time to utilize autoreconf?


Please find attached a workaround for making milter-greylist GeoIP aware again.
I haven't done extensive testing!

In case the maintainer doesn't accept that change, please revert the GeoIP removal from milter-greylist, until anybody else offers a different solution, which doesn't break production setups that badly.

Btw: Any hint appreciated what milter-greylist uses ftp/curl for!?
Comment 1 m.tsatsenko 2019-07-21 22:25:52 UTC
Hello,
Thanks for your efforts, but I do not thing committing such a big sized patch is  a good option. Clearly this should be done using autotools.
Comment 2 Harald Schmalzbauer 2019-07-22 14:59:33 UTC
Hello,

haven't included that change since up to now, there was a configure-patch in place, instead of autoreconf.
I'll add a diff for switching to autoreconf additionally, which reduces files/patch-libmaxminddb-support_from_GH-geoip2-fork significantly and removes files/patch-configure.

I have no idea about yacc and bison, hence no idea about the corresponding hunks in the diff, please check if it's correct what Markus Wennrich provides.

I also don't know if
MAXMINDDB_DESC=         Enable GeoIP1 + GeoIP2 Support
is correct.
I copied from a different port.  I assumed the author is right, but according to https://maxmind.github.io/libmaxminddb/ he isn't, if I interpret the description of "typedef struct MMDB_metadata_s" correctly, where we can read »The binary_format_major_version should always be 2«.  Please correct this before commiting.  I just randomly chose one port listed in https://svnweb.freebsd.org/ports?view=revision&revision=490211, which also needs finxing in case really is incorrect.

Again, if this patch isn't an immediate solution for you, please revert r490200.
The GeoIP option removal badly breaks existing setups, which must not happen for this kind of reason/event/problem (the discontinuance of upstream updates).

Thanks,
-harry
Comment 3 Harald Schmalzbauer 2019-07-22 15:00:34 UTC
Created attachment 205993 [details]
Add libmaxminddb (GeoIP2) support to mitler-greylist and switch to autoreconf
Comment 4 Harald Schmalzbauer 2019-07-22 15:34:49 UTC
find -x . -name Makefile -exec grep -H MAXMINDDB_DESC {} \; reveals that
./dns/gdnsd3 and ./dns/gdnsd2 use this most likely incorrect description.

CCing zi@
Comment 5 m.tsatsenko 2019-07-25 22:43:57 UTC
This patch is much better, I will have a closer look at it. Thanks!
Comment 6 m.tsatsenko 2019-08-17 22:01:07 UTC
Seems fine, but still need to run QA.
By the way did you have a chance to do some run-time testing?
Comment 7 Harald Schmalzbauer 2019-08-18 09:06:33 UTC
(In reply to m.tsatsenko from comment #6)
Yes, I'm running it in production since I posted the diff without attracting attention, utilizing
geoipdb "/usr/local/share/GeoIP/GeoLite2-Country.mmdb".
It's the only change I made in the config.

Not very high traffic on the system: About 200 greylisted not coming back each day, about 100 skipped due to valid SPF or whitelisted, and only 1 or 2 per day really delayed.
But in summary, this GeoIP2 version was invoked about 8000 times during the last 4 weeks.
Comment 8 m.tsatsenko 2019-08-24 01:25:30 UTC
Created attachment 206837 [details]
patch

Please see the patch attached and confirm it works for you.
Comment 9 m.tsatsenko 2019-10-13 22:13:53 UTC
No submitter feedback. 
Good news we do  not really need it.
Comment 10 m.tsatsenko 2019-10-13 22:18:57 UTC
Created attachment 208287 [details]
the patch

- Bring geoip support back
- Do not pass SENDMAIL_FLAGS to the build. It was an old workaround apparently not needed anymore
- Pet portlint
- Bump PORTREVISION

QA
poudriere 113amd64 - OK
portlint - OK
Comment 11 Kubilay Kocak freebsd_committer freebsd_triage 2019-10-13 23:09:20 UTC
@Maintainer

1) Might the OPTION name better be GEOIP as 1) that what it was pre-removal 2) Describes the feature, not the implementation. I believe this is the best UX for users and for any future changes.

2) Can/should the UPDATING entry now be removed? I imagine it should be.
Comment 12 Adam Weinberger freebsd_committer 2019-10-13 23:31:26 UTC
The UPDATING entry applied to a couple dozen ports.
Comment 13 Dewayne 2019-10-14 02:44:45 UTC
(In reply to Kubilay Kocak from comment #11)
Excellent.  My thanks to all for fixing this breakage of production stuff.  :)
Comment 14 m.tsatsenko 2019-10-15 03:21:18 UTC
Preserving original OPTION name makes perfect sense for me. I will update the patch tomorrow