API changes

[lytboris]

My goal is to make IP2Location database be available as a data source for FreeRADIUS project (https://github.com/FreeRADIUS/freeradius-server) as an additional module. To make it happen, a lot of changes both in library itself and it's public/private API need to be made. Even though IP2Location library license permits fork, re-license so one can copy DB parser as-is into the FreeRADIUS module, it needs an additional effort to support the forked parser and it does not sound as a good solution for me.

I would like to know if IP2Location project is willing to apply changes I made in this PR (there will be more!) to the upstream version. If so, I will continue my efforts to make this code be usable as an external library for the FreeRADIUS project.

As of code changes as-is made by this PR, unless IP2LOCATION_HIDDEN_INTERNALS is defined, it is mostly no-op. It fixes a bug with loading these fields though:

#define ADDRESSTYPE		0x81000
#define CATEGORY		0x82000
#define DISTRICT		0x83000
#define ASN			0x84000
#define AS			0x85000

These are not defined as bitwise thus will trigger loading of

#define AREACODE		0x01000
#define WEATHERSTATIONCODE	0x02000
#define WEATHERSTATIONNAME	0x04000

into a IP2LocationRecord structure. Fix is trivial - to use a dedicated bit for each of new fields.

CC @alandekok

[ip2location]

@lytboris We are happy to support FreeRADIUS as data source. However, the existing library is being used by several other projects.

Can we know how much changes do you plan to work on the interface? We need to make sure it does not break any existing projects using our libraries.

[lytboris]

@ip2location My idea is to put everything that is under IP2LOCATION_HIDDEN_INTERNALS define into a static/internal use of the library eventually (and delete it from IP2Location.h). I do not have any plans on the functions that are out of IP2LOCATION_HIDDEN_INTERNALS scope for now - it stays stable. To support FreeRADIUS, new API calls will be made so one could benefit from malloc()-free DB lookup or mmap() without RAM cache.

Any existing projects using this library need to be updated at some point if they use seems-to-be internal-only API calls. This could be done by following these steps:

1. Try to build a project with IP2LOCATION_HIDDEN_INTERNALS defined
2. If it fails, update the code to use public-only API calls or raise a PR with explanation why API call must be moved to the public scope.

Sounds a bit scary, but it is not the first project in the World to clean the API up. Take a look on OpenSSL project as a reference. They've made most of the structures opaque at some point in order to make API more stable.

[alandekok]

I'm not clear what the use-cases are.

My goal is to make IP2Location database be available as a data source for FreeRADIUS project

FreeRADIUS usually gets device P addresses in accounting packets, or assigns device IP addresses in authentication packets.

What is the IP2Location database being used for here? Tracking where the IP is located?

Will FreeRADIUS have an IP, and look it up in the DB to get a location? What will FreeRADIUS do with the location information?

One option may be to use the IP2Location database as an external database. i.e. add a wrapper to the IP2Location code, so that it accepts queries via a long-lived socket. FreeRADIUS could then query that.

It's also possible to integrate the API directly into FreeRADIUS, so that the IP2Location functions can be called directly from FreeRADIUS. However, that means making sure that the IP2Location code can be called repeatedly without leaking FDs, memory, etc. I haven't looked to see if that's possible or even if the current API is "safe" to be called from a long-lived application.

I would hope that it's possible to do many queries in a long-lived application. I would also hope that there's no need to massively change the IP2Location API. FreeRADIUS is flexible, and doesn't have stringent requirements on the functions it calls. So long as there are no memory leaks or FD leaks, we should be able to make almost anything work.

[lytboris]

@alandekok, thanks for you comments!

I'm not clear what the use-cases are.

Authorization of remote (VPN) users based on their IP address. The same way web-servers could deny/limit access to the server based on client's IP address.

What is the IP2Location database being used for here? Tracking where the IP is located?

Provides location (country, city, coordinates, etc), ISP information (AS number, etc) for the IP address.

Will FreeRADIUS have an IP, and look it up in the DB to get a location? What will FreeRADIUS do with the location information?

It could deny connection or apply an additional properties for a user.

I haven't considered using IP2location as an external database yet, thanks for the suggestion. I'll take a look on this approach as well. As soon as I fix memory/fd leaks :)

[alandekok]

@lytboris The main thing for FreeRADIUS is that it may do millions of API calls over a period of days to weeks.

If the IP2Location API is "safe" in that context, then FreeRADIUS shouldn't care about public / private APIs. It just makes API calls, and things work.

If the IP2Location API isn't safe to be run for millions of API calls, then that has to be fixed before it can be used either in FreeRADIUS, or via a DB / socket API.

[lytboris]

@alandekok

If the IP2Location API isn't safe to be run for millions of API calls, then that has to be fixed before it can be used either in FreeRADIUS, or via a DB / socket API.

Yep, this is exactly why we're chatting here :) First I am up to updating the library so it can stand the load, then I'll take a look on new rlm_ module.

[pbiering]

Potentially one can take a look how I implemented this in ipv6calc for mod_ipv6calc used by Apache (there is a LRU cache inside): https://github.com/pbiering/ipv6calc/tree/master/mod_ipv6calc Also using the library of ipv6calc one would be somehow database agnostic as it is supporting GeoIP, DBIP and IP2Location.

[ip2location]

IP2Location should be able to handle millions of API calls without memory leak as it is being used by other projects. Otherwise, we can fix it easily even if we detected a memory leak.

I think a wrapper option between FreeRadius > IP2Location is a better option because changes of interface will requires us to discuss with other projects up-front.

[lytboris]

I think a wrapper option between FreeRadius > IP2Location is a better option because changes of interface will requires us to discuss with other projects up-front.

I do understand you concerns. But let me outline two major points:

1. this PR, being merged as-is, does make any changes into current API (at least that was my goal)
2. all functions that are marked as to-be-removed in future from public API do not seem to be a public API at all as they are dealing with internal structure of database/structures.

These changes will help to build a better wrapper anyway. At least, ALL define will not be exported :)

[ip2location]

@pbiering Is this PR merge OK for your existing project? We are alright with it if it does not break.

[pbiering]

@pbiering Is this PR merge OK for your existing project? We are alright with it if it does not break.

I would assume it's still compatible, but I would prefer having the version number bumped to e.g. 8.7.0 The RPM version of "ipv6calc" is anyhow compiled in dynamic link mode, so I would assume the change would not harm at all.

[lytboris]

The RPM version of "ipv6calc" is anyhow compiled in dynamic link mode, so I would assume the change would not harm at all.

Can you please try compiling it with IP2LOCATION_HIDDEN_INTERNALS set in CFLAGS?

[lytboris]

but I would prefer having the version number bumped to e.g. 8.7.0

So be it!

[pbiering]

Can you please try compiling it with IP2LOCATION_HIDDEN_INTERNALS set in CFLAGS?

Tried, causing issues, stops related to "typedef IP2Location" change during configure.

conftest.c: In function 'test':
conftest.c:62:44: error: invalid use of incomplete typedef 'IP2Location' {aka 'struct _IP2Location'}
   62 |                                         loc.ipv6_database_address = 1;
      |                                            ^
conftest.c: At top level:
conftest.c:60:45: error: storage size of 'loc' isn't known
   60 |                                 IP2Location loc;
      |                                             ^~~
configure:6101: $? = 1

BTW: you can test yourself "interactive" compilation of "ipv6calc":

CFLAGS="-D IP2LOCATION_HIDDEN_INTERNALS" ./configure --enable-ip2location --with-ip2location-headers=/path/to/IP2Location-C-Library/libIP2Location

[lytboris]

Tried, causing issues, stops related to "typedef IP2Location" change during configure.

Will take a look at it.

[pbiering]

any update? I plan to release a new version of ipv6calc soon and can extend the configure.ac and other code if required to support the upcoming changes.

[lytboris]

any update? I plan to release a new version of ipv6calc soon and can extend the configure.ac and other code if required to support the upcoming changes.

Yeah, I as playing with configure.ac and other code in order to support the new version. It's not done and I do not have enough time for proper patching and testing. But I can make a draft PR into your project so you can reuse it.

[pbiering]

Tested "ipv6calc" with "IP2Location" variants incl. upcoming 8.7.0, found no incompatibility issue anymore

[lytboris]

@ip2location I guess this PR should be OK to be merged. If anything needs to be changed - just let me know.

And one more PR coming with up to 30-40% speed boost on massive queries :)