Bug 239334 - mail/milter-greylist: Restore GeoIP support, utilizing libmaxminddb
Summary: mail/milter-greylist: Restore GeoIP support, utilizing libmaxminddb
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: freebsd-ports-bugs (Nobody)
URL: https://github.com/mwennrich/milter-g...
Keywords: needs-patch, needs-qa
Depends on:
Reported: 2019-07-20 12:03 UTC by Harald Schmalzbauer
Modified: 2021-01-07 10:32 UTC (History)
7 users (show)

See Also:
bugzilla: maintainer-feedback? (m.tsatsenko)

Add libmaxminddb (GeoIP2) support to mitler-greylist (129.02 KB, patch)
2019-07-20 12:03 UTC, Harald Schmalzbauer
no flags Details | Diff
Add libmaxminddb (GeoIP2) support to mitler-greylist and switch to autoreconf (8.84 KB, patch)
2019-07-22 15:00 UTC, Harald Schmalzbauer
no flags Details | Diff
patch (9.89 KB, patch)
2019-08-24 01:25 UTC, m.tsatsenko
no flags Details | Diff
the patch (9.92 KB, patch)
2019-10-13 22:18 UTC, m.tsatsenko
m.tsatsenko: maintainer-approval+
Details | Diff
the patch (9.90 KB, patch)
2020-03-15 21:52 UTC, m.tsatsenko
m.tsatsenko: maintainer-approval-
Details | Diff
the patch (2.24 KB, patch)
2020-07-31 22:10 UTC, m.tsatsenko
m.tsatsenko: maintainer-approval+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
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
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

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).

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]

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

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

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
Comment 15 Tobias C. Berner freebsd_committer 2020-03-08 17:24:34 UTC
(In reply to m.tsatsenko from comment #14)
ping? :)
Comment 16 m.tsatsenko 2020-03-15 21:52:22 UTC
Created attachment 212437 [details]
the patch

- Preserve original GEOIP OPTION name. No functional changes

121amd64 still builds OK
Comment 17 Bernard Spil freebsd_committer 2020-07-11 20:42:47 UTC
Comment on attachment 212437 [details]
the patch

This patch seems to be travelling back in time (PORTREVISION going from 5 to 4!)

Please check and provide an updated patch.
Comment 18 Markus Wennrich 2020-07-28 16:28:28 UTC
The GeoIP2 patch has been merged upstream

> http://ftp.espci.fr/pub/milter-greylist/milter-greylist-4.6.3.tgz
> ChangeLog since 4.6.2
>        Add support for GeoIP2 (Markus Wennrich)
>        Build fixes for conflicting ns_type in SPF and NSupdate code
>        Quiet build warnings
>        Missing bits to make rawfrom usable (Marcus Schopen)
>        Fix crash when GeoIP for IPv6 is not configured (Paul Howarth)
>        Report queueId for maxpeek overflow warnings (Attila Bruncsak)
>        Sendmail access.db usage documentation (Johann Klasek)
Comment 19 m.tsatsenko 2020-07-28 22:40:31 UTC
That means extra patches are not needed anymore. I was trying to make them obsolete, but wasnt able to without uploading a new one.
I will have a look at the latest release this weekend.
Comment 20 m.tsatsenko 2020-07-31 22:10:39 UTC
Created attachment 216919 [details]
the patch

- Update to 4.6.3 and bring back geoip support
- Add SmapAssassin and DRAC support
- Do not pass sendmail flags from make.conf. This is a part of very old workaround which is not needed anymore. Also it is know to cause build issues in some cases. 

portlint - No errors
poudrierre 121amd64 - OK
Comment 21 commit-hook freebsd_committer 2021-01-07 10:05:18 UTC
A commit references this bug:

Author: brnrd
Date: Thu Jan  7 10:04:50 UTC 2021
New revision: 560697
URL: https://svnweb.freebsd.org/changeset/ports/560697

  mail/milter-greylist: Update to 4.6.3

  PR:		239334
  Submitted by:	<m tsatsenko gmail com> (maintainer)