Integrate IPFire Location (new implementation for the geoip keyword)

[mtremer]

This is a draft of a module that includes IPFire Location in suricata. The "geoip" keyword allows rule authors to detect (with a certain degree of confidence) where the source/destination host is located on the internet.

This module is loosely based on the existing GeoIP module which remains untouched. This module has been tested on suricata 7 and can be easily backported to earlier versions. It already has the advantage to support both, IPv6 and IPv4, whereas the first is lacking in the existing module. I also believe that libloc is substantially faster in looking up any IP addresses from the database.

I do not know what happens if someone tries to compile suricata with support for both, libloc and libgeoip. Potentially this should be checked in configure.

This module should also be extended to offer support for ASN matching (https://redmine.openinfosecfoundation.org/issues/4649).

I am looking forward to any feedback.

[jasonish]

Some initial thoughts:

• It uses the "geoip" keyword which is also used by GeoIP2.. While not a big issue I think this means that configure.ac should only allow one.
• As it uses the "geoip" keyword, some unit tests to make sure it can parse the same rule option would be good.

Also important before merge, but may want to wait on more code review, etc:

• suricata-verify tests
• (nice to have) examples in out github ci tests, one for latest ubuntu and latest centos, the testing with suricata-verify comes part of our basic testing
• if there are incompatibilities then some discussion in our documentation about them so users can know what to expect

Thanks.

[mtremer]

Thank you for taking some time to look at this.

Some initial thoughts:

• It uses the "geoip" keyword which is also used by GeoIP2.. While not a big issue I think this means that configure.ac should only allow one.

I thought this is the only way we would take advantage of already existing rules. There is no point for rule writers to distinguish between what databases are being used.

• As it uses the "geoip" keyword, some unit tests to make sure it can parse the same rule option would be good.

Agreed. I didn't take time to figure out how unit tests work, yet.

Also important before merge, but may want to wait on more code review, etc:

• suricata-verify tests

What would these do? Is there some documentation you can point me to?

• (nice to have) examples in out github ci tests, one for latest ubuntu and latest centos, the testing with suricata-verify comes part of our basic testing

Would we be able to support building some third-party software, since the library is not part of any major distribution, yet. We have our own repositories which can be added for Debian. So for there is little reason to add libloc to major distributions because not that much software is using it, but this is obviously going to change with suricata becoming another user now.

• if there are incompatibilities then some discussion in our documentation about them so users can know what to expect

I am not aware of anything. In fact, IPFire Location has a superset of features because IPv6 is supported.

[jasonish]

Thank you for taking some time to look at this.

Some initial thoughts:

• It uses the "geoip" keyword which is also used by GeoIP2.. While not a big issue I think this means that configure.ac should only allow one.

I thought this is the only way we would take advantage of already existing rules. There is no point for rule writers to distinguish between what databases are being used.

Yeah, its fine to use the geoip keyword. Just ./configure should only allow one.

• As it uses the "geoip" keyword, some unit tests to make sure it can parse the same rule option would be good.

Agreed. I didn't take time to figure out how unit tests work, yet.

Also important before merge, but may want to wait on more code review, etc:

• suricata-verify tests

What would these do? Is there some documentation you can point me to?

Here's our existing suricata-verify test for geoip. Its very light-weight, but gives us some indication geoip is working. Due to the database formats it would probably need to be a little different for libloc.

• (nice to have) examples in out github ci tests, one for latest ubuntu and latest centos, the testing with suricata-verify comes part of our basic testing

Would we be able to support building some third-party software, since the library is not part of any major distribution, yet. We have our own repositories which can be added for Debian. So for there is little reason to add libloc to major distributions because not that much software is using it, but this is obviously going to change with suricata becoming another user now.

Yes, its pretty flexible.. We use a PPA to get cocci in one of the builds. Can easily build from source as well. Do you plan a copr repo for RHEL-like systems?

[mtremer]

• (nice to have) examples in out github ci tests, one for latest ubuntu and latest centos, the testing with suricata-verify comes part of our basic testing

Would we be able to support building some third-party software, since the library is not part of any major distribution, yet. We have our own repositories which can be added for Debian. So for there is little reason to add libloc to major distributions because not that much software is using it, but this is obviously going to change with suricata becoming another user now.

Yes, its pretty flexible.. We use a PPA to get cocci in one of the builds. Can easily build from source as well. Do you plan a copr repo for RHEL-like systems?

We do not have any unofficial RPM packages, yet. Ideally we want this package to become parts of the distributions as quickly as possible and we do not want to maintain too many out-of-tree packages, simply because we do not have enough distribution diversity in our team to support all of them. We would be happy if people would join us and package libloc for their favourite distribution.

[jasonish]

@mtremer Any plans to support cities? Also, is there any info on how locations are acquired?

[mtremer]

@mtremer Any plans to support cities?

No, there is not support planned for cities because there is little need for it and absolutely no reliable data that we can use.

Also, is there any info on how locations are acquired?

Yes, the code that collects all information is part of the source repository:

https://git.ipfire.org/?p=location/libloc.git;a=blob;f=src/python/location-importer.in;h=bee91868f66e3ea60c5146a1e41be713bbbe4142;hb=HEAD
https://git.ipfire.org/?p=location/libloc.git;a=blob;f=src/python/importer.py;h=dee36ed9dfacb2c6403ec4d53d30cc122c784ec1;hb=HEAD

The basis is WHOIS information paired with the global routing table and some manual data:

https://git.ipfire.org/?p=location/location-database.git;a=tree;f=overrides;h=db120d468920336d74a9c3868f81349009b8a337;hb=HEAD

[suricata-qa]

Warning: no commits in this PR have specified the following ticket(s):

• 4649 - https://redmine.openinfosecfoundation.org/issues/4649

Please update the commit(s) and submit a new PR.

[mtremer]

I have updated the commit message.

[jufajardini]

I have updated the commit message.

Thanks! Considering it also needs rebase, could you please submit a new PR, when you have the time?

[mtremer]

I have updated the commit message.

Thanks! Considering it also needs rebase, could you please submit a new PR, when you have the time?

Thank you. I appreciate your response very much, but I have been spending a lot of time to resubmit the same PR again and again with only small changes. The requested changes to the commit message could have easily been added with the merge. I have already rebased this multiple times just to see the PR lying around for a long time.

Is there anything else that needs to be done to get this finally merged so that I can combine things into one job, please?

[jasonish]

Is there anything else that needs to be done to get this finally merged so that I can combine things into one job, please?

I think now it comes down to tests to prove that it works as expected, done with Suricata-Verify. This is even more important with this feature over something new as its re-using an existing rule keyword, so it has to behave very close, if not identical to the existing support provided by libmaxminddb, which I also means adding some more tests for our geoip support which is really low on tests at the moment. Basically it needs a champion to finish off the tedious details of adding a new feature. A build in ~/.github/workflows/builds.yml that uses this feature would also be good, as then it will be continuously verified against Suricata-Verify.

[victorjulien]

Closing due to inactivity. If you're interested in picking this back up, please open a new PR addressing the comments. Thanks!