MIR: bin:libnginx-mod-http-geoip2 from src:nginx

[Andreas Hasenack]

nginx is already in main, but the binary package libnginx-mod-http-geoip2 comes from another source that is shipped inside src:nginx and hasn't gone through the original nginx MIR.

When it was included[1], the agreement was that it would stay in Universe. We now want it in main, because of the geoip1 legacy deprecation in favor of geoip2 (libmaxminddb), see libmaxminddb's MIR[2].

NOTE: this MIR is *not* a blocker for the libmaxminddb MIR[2], as we are demoting bin:libnginx-mod-http-geoip to universe (see FFe https://bugs.launchpad.net/ubuntu/+source/nginx/+bug/1867150)

1. https://bugs.launchpad.net/ubuntu/+source/nginx/+bug/1825895
2. https://bugs.launchpad.net/ubuntu/+source/libmaxminddb/+bug/1861101

Source: https://github.com/leev/ngx_http_geoip2_module

Availability:
The package must already be in the Ubuntu universe, and must build for the architectures it is designed to work on.
The bin:libnginx-mod-http-geoip2 package is built from src:nginx and exists in universe already:
https://launchpad.net/ubuntu/focal/+package/libnginx-mod-http-geoip2
https://launchpad.net/ubuntu/focal/amd64/libnginx-mod-http-geoip2/1.17.9-0ubuntu1

It's built for amd64, arm64, armhf, ppc64el, s390x

Rationale:
We want to move away from legacy geoip1 code into new geoip2 code. nginx already has a geoip1 module in main (bin:libnginx-mod-http-geoip), and we want to exchange that with the geoip2 module.
The main push to switch to geoip2 comes from the latest stable release of bind9 (9.16.x), which has no support for geoip1, just geoip2, via libmaxminddb, which has an ongoing MIR at https://bugs.launchpad.net/ubuntu/+source/libmaxminddb/+bug/1861101. As a requirement of that MIR, we want to avoid having geoip1 and geoip2 both in main.

Security:
The security history and the current state of security issues in the package must allow us to support the package for at least 9 months (60 for LTS support) without exposing its users to an inappropriate level of security risks. This requires checking of several things that are explained in detail in the subsection Security checks.

http://cve.mitre.org/cve/search_cve_list.html: Search in the National Vulnerability Database using the package as a keyword
- I tried a few keywords: geoip2, "nginx geoip2", "nginx geoip", "ngx_http_geoip2_module", "ngx geoip2", "ngx geoip": all empty, or returning nginx issue, or geoip issues in other products

check OSS security mailing list (feed 'site:www.openwall.com/lists/oss-security <pkgname>' into search engine)
- "site:www.openwall.com/lists/oss-security nginx geoip2": empty
- "site:www.openwall.com/lists/oss-security nginx geoip": empty
- "site:www.openwall.com/lists/oss-security ngx_http_geoip2_module": empty
- "site:www.openwall.com/lists/oss-security ngx geoip2": empty

Ubuntu CVE Tracker
* http://people.ubuntu.com/~ubuntu-security/cve/main.html
- nothing for "geoip", "geoip2" or "nginx" (I checked and the package search is a substring search). Be sure to click "clear filter" between search attempts.

* http://people.ubuntu.com/~ubuntu-security/cve/universe.html
- nothing for "geoip", "geoip2" or "nginx"

* http://people.ubuntu.com/~ubuntu-security/cve/partner.html
- nothing for "geoip", "geoip2" or "nginx" (in fact, there are no packages at all listed there)

* Executables which have the suid or sgid bit set.
- none

* Executables in /sbin, /usr/sbin.
- none, it's an nginx module is a .so file in the modules directory

* Packages which install services / daemons (/etc/init.d/*, /etc/init/*, /lib/systemd/system/*)
- bin:libnginx-mod-http-geoip2 doesn't install a daemon, but nginx of course is one

* Packages which open privileged ports (ports < 1024).
- bin:libnginx-mod-http-geoip2 doesn't, but nginx of course does

* Add-ons and plugins to security-sensitive software (filters, scanners, UI skins, etc)
- this is an add-on, or plugin/module, to nginx

Quality assurance:
* After installing the package it must be possible to make it working with a reasonable effort of configuration and documentation reading.
- the geoip2 module is loaded right after installation:
lrwxrwxrwx 1 root root 55 Mar 12 20:28 /etc/nginx/modules-enabled/50-mod-http-geoip2.conf -> /usr/share/nginx/modules-available/mod-http-geoip2.conf
- To further set it up, nginx configuration must be changed, and a copy of the geoip2 databases must be obtained, either the free one or the subscription one.

* The package must not ask debconf questions higher than medium if it is going to be installed by default. The debconf questions must have reasonable defaults.
- no debconf questions

* There are no long-term outstanding bugs which affect the usability of the program to a major degree. To support a package, we must be reasonably convinced that upstream supports and cares for the package.
  - Ubuntu bugs:
    - src:nginx bugs: https://bugs.launchpad.net/ubuntu/+source/nginx. None about geoip, other than this MIR and the demotion of legacy geoip1
  - Debian bugs:
    - src:nginx bugs: https://bugs.debian.org/cgi-bin/pkgreport.cgi?dist=unstable;package=nginx
    - none about geoip, but note that debian doesn't ship the geoip2 module (it's an ubuntu delta)

* The package is maintained well in Debian/Ubuntu (check out the Debian PTS)
- https://tracker.debian.org/pkg/nginx
- debian doesn't ship the geoip2 module, just geoip(1) (legacy)
- ubuntu's nginx package is ahead of debian
- it looks like it could be better maintained in debian: open security issues in the BTS, bugs with patches still open. Uploads aren't super frequent, but also haven't stalled for toooo long

* The package should not deal with exotic hardware which we cannot support.
- no exotic hardware

* If the package ships a test suite, and there is no obvious reason why it cannot work during build (e. g. it needs root privileges or network access), it should be run during package build, and a failing test suite should fail the build.
- no test suite in the upstream code
- there are many dep8 tests, though:
autopkgtest [06:50:29]: @@@@@@@@@@@@@@@@@@@@ summary
light-simple PASS
core-simple PASS
lua PASS
full-simple PASS
extras-simple PASS
full-module-deps PASS
light-module-deps PASS
core-module-deps PASS
extras-module-deps PASS
- but none about geoip, other than installing most modules. This could be expanded to also install geoip2 perhaps:
$ grep geoip debian/tests/*
debian/tests/control: libnginx-mod-http-geoip,
debian/tests/control: libnginx-mod-http-geoip,
debian/tests/control: libnginx-mod-http-geoip,
debian/tests/control: libnginx-mod-http-geoip,

* The package uses a debian/watch file whenever possible. In cases where this is not possible (e. g. native packages), the package should either provide a debian/README.source file or a debian/watch file (with comments only) providing clear instructions on how to generate the source tar file.
- src:nginx has a d/watch file, but it seems to be out of date, as it looks for versions matching 1.13.*, and focal now has 1.17.9:
$ cat debian/watch
version=3
opts=pgpsigurlmangle=s/$/.asc/ \
https://nginx.org/download/nginx-(1\.13\.\d+)\.tar\.gz

* It is often useful to run lintian --pedantic on the package to spot the most common packaging issues in advance
$ lintian -I --pedantic
W: nginx-common: maintainer-script-should-not-use-dpkg-maintscript-helper postinst:10
W: nginx-common: maintainer-script-should-not-use-dpkg-maintscript-helper postinst:12
W: nginx-common: maintainer-script-should-not-use-dpkg-maintscript-helper postinst:14
W: nginx-common: maintainer-script-should-not-use-dpkg-maintscript-helper ... use --no-tag-display-limit to see all (or pipe to a file/program)
W: nginx source: orig-tarball-missing-upstream-signature nginx_1.17.9.orig.tar.gz
I: nginx source: duplicate-short-description libnginx-mod-http-geoip libnginx-mod-http-geoip2
I: nginx source: duplicate-short-description nginx-core nginx-full
I: libnginx-mod-stream: hardening-no-fortify-functions usr/lib/nginx/modules/ngx_stream_module.so
I: nginx source: out-of-date-standards-version 4.1.3 (released 2017-12-27) (current is 4.5.0)
I: nginx source: public-upstream-key-not-minimal upstream/signing-key.asc has 3 extra signature(s) for keyid 520A9993A1C052F8
I: libnginx-mod-rtmp: spelling-error-in-binary usr/lib/nginx/modules/ngx_rtmp_module.so conection connection
I: libnginx-mod-rtmp: spelling-error-in-binary usr/lib/nginx/modules/ngx_rtmp_module.so shedule schedule
I: libnginx-mod-http-perl: unused-override wrong-section-according-to-package-name libnginx-mod-http-perl => perl
P: nginx source: debug-symbol-migration-possibly-complete --dbgsym-migration='nginx-$(*)-dbg (<< 1.10.1-3~)' (line 184)
P: nginx source: duplicate-in-relation-field in source build-depends: po-debconf, po-debconf
P: nginx source: file-contains-trailing-whitespace debian/changelog (line 1055)
P: nginx source: file-contains-trailing-whitespace debian/changelog (line 1064)
P: nginx source: file-contains-trailing-whitespace debian/changelog (line 144)
P: nginx source: file-contains-trailing-whitespace ... use --no-tag-display-limit to see all (or pipe to a file/program)
P: nginx source: package-uses-old-debhelper-compat-version 10
P: nginx source: rules-requires-root-missing
N: 1 tag overridden (1 error)

- src:nginx could use some love with these lintian issues. Only one seems to be specific to geoip2, and is not serious though

* The package should not rely on obsolete or about to be demoted packages. That currently includes package dependencies on Python2 (without providing Python3 packages), and packages depending on GTK2.
- quite the opposite, the intention of this MIR is to have the nginx geoip module rely on geoip2.

UI standards:
- n/a

Dependencies:
* All binary dependencies (including Recommends:) must be satisfiable in main (i. e. the preferred alternative must be in main). If not, these dependencies need a separate MIR report (this can be a separate bug or another task on the main MIR bug)
- dependencies of libnginx-mod-http-geoip2_1.17.9-0ubuntu1_amd64.deb:
 Depends: nginx-common (= 1.17.9-0ubuntu1), libc6 (>= 2.14), libmaxminddb0 (>= 1.0.2)
- no Recommends
- libmaxminddb0 is being handled in MIR https://bugs.launchpad.net/ubuntu/+source/libmaxminddb/+bug/1861101

Standards compliance:
* The package should meet the FHS and Debian Policy standards. Major violations should be documented and justified. Also, the source packaging should be reasonably easy to understand and maintain.
- speaking specifically about the bin:libnginx-mod-http-geoip2 package, I don't see any violations. It's a standard module package, with a .so, config

Maintenance:
* The package must have an acceptable level of maintenance corresponding to its complexity:
- d/rules is not immediately trivial, but well organized, and uses debhelper

* All packages must have a designated "owning" team, regardless of complexity, which is set as a package bug contact.
- ubuntu server is already subscribed to src:nginx

Background information:
This is an odd mir in the sense that the src:nginx package is already in main, but at a later time another source was added as a third party module (there are a few in the package already, look in debian/modules), and that module wasn't part of the original MIR. So this is a MIR for a binary package, produced by src:nginx.

[Christian Ehrhardt  (paelzer)]

This is a bit of a special review,
the few packaging bits "on top of normal nginx" are just:
- ./debian/libnginx-mod-http-geoip2.nginx
- ./debian/libnginx-mod.conf/mod-http-geoip2.conf
and a few entries in d/rules
- debian/rules:16: http-geoip2 \
- debian/rules:122: --add-dynamic-module=$(MODULESDIR)/http-geoip2
- debian/rules:156: --add-dynamic-module=$(MODULESDIR)/http-geoip2

They all LGTM and don't need a full MIR review "again" for those few lines.
They are a bit complex but that just reflects how things are in nginx until modules can be proper external sources.

The actual source of it is in:
- debian/modules/http-geoip2/*

And that is the new bit as it is an external source and was not security reviewed when nginx was handled.
The source is from:
=> https://github.com/leev/ngx_http_geoip2_module

I re-did the sections that would apply for this code:
- does not FTBFS currently
- does not have a test suite that runs at build time
- no translation present, but none needed for this case (not user visible)?
- not a python package, no extra constraints to consider int hat regard
- symbols tracking not applicable for this kind of code.
- Upstream update history is ok
- promoting this does not seem to cause issues for MOTUs that so far
  maintained the package
- no massive Lintian warnings
- no geoip2 related issues int the build log
- no incautious use of malloc/sprintf (as far as I can check it)
- no use of sudo, gksu, pkexec, or LD_LIBRARY_PATH
- no use of user nobody
- no use of setuid
- no dependency on webkit, qtwebkit, seed or libgoa-*
- no embedded source copies (well this IS the embedded source, but no further ones)

Problems:
- some others have ./debian/modules/watch entries but this one is missing
  add this to reduce the chance to miss updates
- the current release is not packaged (but not too far behind)
- one crasher bug open upstream, but resolved in 3.3

TODOs:
- @Andreas / Teward - would you mind bumping the embedded version to 3.3?
  - that is just a crash fix on DB reload
- @Andreas / Teward - would you mind adding geoip2 to ./debian/modules/watch?
- @Security - Since this is sort of the entry point where things might have more external control to be craftable a security review of just debian/modules/http-geoip2/* is recommended.
- @Security this raises an extra question if we need to set something up that this will be properly CVE tracked - could you outline if that will work well?

Assigning security for now.
Once reviewed the MIR Team can re-check if the other todo's were resolved in the meantime and then I think promotion can happen.

[Seth Arnold (seth-arnold)]

teward, I'm a bit confused on the status of https://bugs.launchpad.net/ubuntu/+source/libmaxminddb/+bug/1861101 and this MIR -- can you let us know if there's a new module nginx module that still needs review because it's desired to move it into main?

Thanks

[Thomas Ward (teward)]

Discussed via IRC, but libmaxminddb is a dependency that needs to be in Main if we're going to get this third-party module (shipped inside src:nginx) binary-included in Main.

The module is still actively reviewed by the third party developer but has not had any code changes to the underlying C modules in 16 months. (https://github.com/leev/ngx_http_geoip2_module) which shows that it's still got attention from its developer.

This MIR still needs to be done for us to enable the geoip2 support in nginx - so it still needs started for the nginx module. libmaxminddb was just the dependency since it needs the maxmind db libs for runtime.

[Steve Beattie (sbeattie)]

I reviewed libnginx-mod-http-geoip2/nginx 1.18.0-6ubuntu4 (aka
http-geoip2 3.3 upstream) as checked into hirsute. This shouldn't
be considered a full audit but rather a quick gauge of maintainability.

libnginx-mod-http-geoip2 is an nginx module registers variables on the
connection based on the precompiled MaxMind database, for use by by
other elements with nginx.

- No CVE history found for this nginx module.
- The only additional dependency here is on the libmaxminddb library.
- pre/post inst/rm scripts attempt to handle the module configuration
  for nginx.
- No init scripts.
- No systemd units.
- No dbus services.
- No binaries, setuid or otherwise.
- No sudo fragments.
- No polkit files.
- No udev rules.
- There does not appear to be any unit or functional tests, and
  autopkgtests do not appear to exercise the module.
- No cron jobs.
- No build warnings or errors on the module compilation. No lintian
  warnings.

- No Processes spawned.
- Most memory/string management is around handling config options as
  well as copying values from the MindMap db. As such, it's got a bunch
  of magic constants that get added and subtracted to things, so it's
  not super clear that it's allocating sizes correctly. But at least
  return values are checked for errors.
- The only File IO is to open and lookup entries in the static MaxMind
  database, and is specified through the module configuration file.
- Logging is handled through the standard ngninx module logging
  interface, and uses format strings correctly.
- No environment variable usage.
- No use of privileged functions.
- No use of cryptography / random number sources etc?
- No use of temp files.
- Networking is handled by nginx proper, the only bits related to
  networking are getting the ipv4/ipv6 connection information from the
  request, and looks okay.
- No use of WebKit.
- No use of PolicyKit.

- No Coverity of cppcheck results reported for the module.

The module is relatively small and well contained. Issues like
https://github.com/leev/ngx_http_geoip2_module/issues/90 are mildly
concerning, but the only thing really under an attacker control is the
ipv4 or ipv6 address being looked up, so the threat surface is small.

Security team ACK for promoting libnginx-mod-http-geoip2 to main.
One question I have is whether this MIR is limited strictly to
libnginx-mod-http-geoip2, or is libnginx-mod-stream-geoip2 covered
as well? If the latter, nothing different turned up while examining it
in the course of looking at libnginx-mod-http-geoip2.

[Thomas Ward (teward)]

Steve Beattie:

This MIR is strictly for the http-geoip2 module and not stream-geoip2. I have not slated stream modules for Main inclusion as it is not something that we have tested in depth, therefore the stream configs and modules unless they are already in Main should be considered Universe modules at this time. `stream` is essentially doing TCP/UDP listeners and forwarding to other things. Which is not available in the default package configuration as we have now, and requires users to edit nginx.conf.

I'm working on making the ability to apply different configurations *simpler* but that's a long-term project since it radically overhauls the packaging.

So, for the purposes of this MIR, only focus on the http-geoip2 module and not the stream-geoip2 module - stream-geoip2 can still be kept in Universe.

[Christian Ehrhardt  (paelzer)]

Thanks Steve,
thereby almost everything seems to be ready now.

@Teward when you add the dependency pulling libnginx-mod-http-geoip2 (from nginx-core I guess) into main just add the minimal TODOs we've had in comment #1 (small version bump to 3.3 and adding ./debian/modules/watch).

Once done that should show up in component mismatches and be ready for promotion then.

[Thomas Ward (teward)]

paelzer:

I think someone has failed hardcore on part 2 of that comment - there's already a ./debian/modules/watch/http-geoip2 watch file containing the following in Hirsute:

version=4
opts="dversionmangle=s/v//,filenamemangle=s%(?:.*?)?v?(\d[\d.]*)\.tar\.gz%libnginx-mod-http-geoip2-$1.tar.gz%" \
    https://github.com/leev/ngx_http_geoip2_module/tags \
    (?:.*?/)?v?(\d[\d.]*)\.tar\.gz debian debian/ngxmod uupdate http-geoip2

[Thomas Ward (teward)]

Note that all the watches seem to be organized by ./debian/modules/watch/[MODULENAME] and match items in ./debian/modules/[MODULENAME]. That is inherited from Debian.

[Christian Ehrhardt  (paelzer)]

> I think someone has failed hardcore on part 2 of that comment - there's
> already a ./debian/modules/watch/http-geoip2 watch file containing the
> following in Hirsute:

Absolutely right, it is fine by now - sorry for missing it earlier.

And to complete - also the second TODO is covered:
 151 [ Ondřej Surý ]
 152 * http-geoip2: Add ngx_http_geoip2_module 3.3
 153
 154 -- Ondřej Nový <email address hidden> Fri, 05 Jun 2020 18:28:40 +0200

Therefore yeah - as just discussed on IRC - I think this is good to go.
Thanks for the clarifications!

[Christian Ehrhardt  (paelzer)]

The change that pulls this into main is in https://launchpad.net/ubuntu/+source/nginx/1.18.0-6ubuntu7 which is in hirsute-proposed now.
The only deps are (as expected)
 Depends: nginx-common (= 1.18.0-6ubuntu7), libc6 (>= 2.33), libmaxminddb0 (>= 1.0.2)
All of them are in main already.

Therefore this is ready for promotion to main - setting state to fix committed.

[Launchpad Janitor (janitor)]

This bug was fixed in the package nginx - 1.18.0-6ubuntu8

---------------
nginx (1.18.0-6ubuntu8) hirsute; urgency=medium

  * d/modules/control: Remove Lua module from definitions
  * d/tests/:
    - control: Remove Lua test, remove dependencies on any test which
      request libnginx-mod-http-lua as it's gone.
    - lua: Remove the lua test entirely.

 -- Thomas Ward <email address hidden> Wed, 10 Mar 2021 10:50:43 -0500