nnrpd: add blacklistd support

[andkem]

Add support to nnrpd for blocking brute force attacks using blacklistd available on FreeBSD.

This version is slimmed down compared to the patch sent to the inn-workers list. Considering that one nnrpd fork only ever handles one connection, it seemed unnecessary keeping the blacklistd connection around. Not having to keep the connection around simplifies things since all state can be kept inside of the BlacklistReport function.

One thing that I am not entirely sure about is the fact that with this patch, nnrpd will always try to connect to blacklistd if it is compiled with blacklist support. If the user is not running blacklistd, one log line will be generated per connection attempt. Is this fine?

[andkem]

I'll add that the current version of this PR is running in production on Lysator's server at nyheter.lysator.liu.se and seems to be working as expected.

[Julien-Elie]

Thanks for the PR, Andreas!

One thing that I am not entirely sure about is the fact that with this patch, nnrpd will always try to connect to blacklistd if it is compiled with blacklist support. If the user is not running blacklistd, one log line will be generated per connection attempt. Is this fine?

That's a good point indeed. I'm wondering whether that facility should not be explicitly enabled then (a new -B flag to add to nnrpd, so that it tries to connect to blacklistd if built with that support?). This way, error logs won't be generated if blacklistd is installed but not used.

Also, there's something we did not take into account: how the implementation behaves when there are several auth groups?

AuthenticateUser() is called for each auth group, so you may have several login failures reported to blacklistd whereas that is normal:

    while (runame == NULL && i--)
        runame = AuthenticateUser(auth_realms[i], uname, pass, code, errorstr);

Maybe the call to BlacklistReport() should be at the end of PERMlogin() instead of AuthenticateUser()?

For instance, the example in readers.conf should report an error only if all auth realms were unsuccessful:

    auth default {
        auth: "ckpasswd -f <pathdb in inn.conf>/newsusers"
        default: <FAIL>
        default-domain: example.com
    }

    auth shell {
        hosts: *.shell.example.com
        res: ident
        auth: ckpasswd
        default: <FAIL>
        default-domain: shell.example.com
    }

    auth dialup {
        hosts: *.dialup.example.com
        auth: radius
        default: <FAIL>
        default-domain: dialup.example.com
    }

[Julien-Elie]

As you are a FreeBSD expert building from sources, maybe (if that's not too much to ask :)) you could update the AC_MSG_NOTICE() at the beginning of configure.ac to say what FreeBSD packages are needed to build from sources? If of course that applies (I think they are of tbz/tgz form, but I may be wrong) and have different names than the ones listed for Debian (deb) and RedHat (rpm). At least the one for blacklistd could be useful.

Is the test suite still running fine with the patch? (make tests) No need to add glue for the blacklist library to link with nnrpd in the test suite?

#include <string.h> could be dropped (already set in portable/system.h).

[andkem]

Thanks for the PR, Andreas!

One thing that I am not entirely sure about is the fact that with this patch, nnrpd will always try to connect to blacklistd if it is compiled with blacklist support. If the user is not running blacklistd, one log line will be generated per connection attempt. Is this fine?

That's a good point indeed. I'm wondering whether that facility should not be explicitly enabled then (a new -B flag to add to nnrpd, so that it tries to connect to blacklistd if built with that support?). This way, error logs won't be generated if blacklistd is installed but not used.

This was the option I was considering and I'm fine with it. I'll add a -B flag.

Also, there's something we did not take into account: how the implementation behaves when there are several auth groups?

AuthenticateUser() is called for each auth group, so you may have several login failures reported to blacklistd whereas that is normal:

    while (runame == NULL && i--)
        runame = AuthenticateUser(auth_realms[i], uname, pass, code, errorstr);

Maybe the call to BlacklistReport() should be at the end of PERMlogin() instead of AuthenticateUser()?

Sounds good. I'll move the function call to the end of PERMlogin() after the loop is done.

As you are a FreeBSD expert building from sources, maybe (if that's not too much to ask :)) you could update the AC_MSG_NOTICE() at the beginning of configure.ac to say what FreeBSD packages are needed to build from sources? If of course that applies (I think they are of tbz/tgz form, but I may be wrong) and have different names than the ones listed for Debian (deb) and RedHat (rpm). At least the one for blacklistd could be useful.

The packages are in txz format and I can have a look at the message. blacklistd itself doesn't require any package since it is provided by the base FreeBSD system.

Is the test suite still running fine with the patch? (make tests) No need to add glue for the blacklist library to link with nnrpd in the test suite?

make test works without any modifications. I haven't tried looking into why, but can do so if you want me to.

#include <string.h> could be dropped (already set in portable/system.h).

Will do.

[andkem]

I had a look at the dependencies for the configure message, but some of them are provided by the base system in FreeBSD and don't have a package. I'm not sure how to best handle formatting of the message in those cases. Should I simply write "provided by base"?

[Julien-Elie]

This was the option I was considering and I'm fine with it. I'll add a -B flag.

Thanks for the new patch. It sounds good. I'll have a deeper look at it this week-end, and very certainly merge it.

make test works without any modifications

OK, thanks for the confirmation.

The packages are in txz format and I can have a look at the message. blacklistd itself doesn't require any package since it is provided by the base FreeBSD system.

Is the base FreeBSD system providing headers by default? (no -devel packages or things like that?)

I'm not sure how to best handle formatting of the message in those cases. Should I simply write "provided by base"?

It would look a bit messy to add the txz packages at the same level as the others if the majority is provided by base. I would suggest to just add a separate paragraph to say that FreeBSD provides the necessary headers and libraries by default except for the following packages: xxx

[andkem]

Is the base FreeBSD system providing headers by default? (no -devel packages or things like that?)

Headers are provided by default so special devel packages don't exist.

I'm not sure how to best handle formatting of the message in those cases. Should I simply write "provided by base"?

It would look a bit messy to add the txz packages at the same level as the others if the majority is provided by base. I would suggest to just add a separate paragraph to say that FreeBSD provides the necessary headers and libraries by default except for the following packages: xxx

I'll have a look. Since inn is in the ports tree from which all packages are built, I can steal the dependency list from there. When I've been building inn, I've first simply installed the inn package to get all the dependencies (except for compile time dependencies like automake) for building.

[andkem]

I've gone through and collected dependencies: gmake autoconf libtool cyrus-sasl p5-GD p5-MIME-Tools freebsd-uucp krb5 gnupg db18 - BerkeleyDB - package name will change with new versions python3

I configured using the following command line: ./configure --with-perl --with-python --with-bdb --with-bdb-lib=/usr/local/lib/db18 --with-bdb-include=/usr/local/include/db18 --with-blacklist --with-krb5 --with-openssl --with-sasl --with-sasl-include=/usr/local/include --with-sasl-lib=/usr/local/lib --with-sqlite3 --with-sqlite3=/usr/local --with-zlib

The base system comes with OpenSSL, but one could optionally install an OpenSSL version from the package manager if one wants another version of OpenSSL. Then one would have to specify the paths to /usr/local explicitly when running configure.

When building, one needs to use gmake instead of make.

The rest is provided by the base system unless I've missed some option I should have tested.

[andkem]

I had a go at updating the message and pushed it for review.

[Julien-Elie]

Thanks Andreas, I've not forgotten you. I caught the COVID-19 last week and was too much exhausted this week-end to manage to do anything. I'm feeling a bit better now. I'll merge your commits soon.

[andkem]

Ouch! Glad to hear you're feeling better! No worries about the patch, I'm in no rush. Take all the time you need to recover!

[Julien-Elie]

Thanks again for your work Andreas. I've merged it upstream with the usual writing and coding style (and alphabetical sorting).

I've also re-established an option that disappeared (s: was mistakenly changed to s in nnrpd's getopt list). Everything should now be good I think.

[andkem]

Glad it could get merged and thank you for your time reviewing!

[Julien-Elie]

Hi Andreas, I'm running portability tests for the upcoming 2.7.0 release in the GCC C Farm, and I've just found out that gcc300, running NetBSD 9.2, has blacklist.h v1.3 2015/01/23 but without:

#define BLACKLIST_API_ENUM      1
enum {
        BLACKLIST_AUTH_OK = 0,
        BLACKLIST_AUTH_FAIL,
        BLACKLIST_ABUSIVE_BEHAVIOR,
        BLACKLIST_BAD_USER
};

The following straight-forward patch (without adding a separate portable header...) fixes the build. I've not tested it though (I'm only checking that INN builds fine). Do you believe, with your knowledge of blacklistd, it is OK to do that?

--- a/nnrpd/perm.c
+++ b/nnrpd/perm.c
@@ -12,6 +12,14 @@
 #if defined(HAVE_BLACKLIST)
 #    include <blacklist.h>
 #    include <errno.h>
+#    ifndef BLACKLIST_API_ENUM
+enum {
+    BLACKLIST_AUTH_OK = 0,
+    BLACKLIST_AUTH_FAIL,
+    BLACKLIST_ABUSIVE_BEHAVIOR,
+    BLACKLIST_BAD_USER
+};
+#    endif
 #endif

[andkem]

I don't use NetBSD so I can't really say. If they've kept the same return codes as on FreeBSD, then your change should be fine. I'll try to get the NetBSD codebase down and have a look.

[andkem]

From what I can tell, only BLACKLIST_AUTH_OK and BLACKLIST_AUTH_FAIL exist in NetBSD 9. The values for AUTH_OK = 0 and AUTH_FAIL = 1 are the same on both NetBSD 9 and FreeBSD. My suggestion would be to remove BLACKLIST_ABUSIVE_BEHAVIOR and BLACKLIST_BAD_USER from the struct and to only keep the first two.

[Julien-Elie]

Thanks for having checked Andreas! I'll follow your piece of advice, and commit the change soon.