[jwrosewell]

Closes #99.

Problem

Three gaps in the on-premise IP Intelligence engine at rc 6d95eee, the first and third found in an audit and the second while making this draft.

1. A client IP value with a port, such as 203.0.113.1:8080, or an IPv6 address in brackets, such as [2001:db8::1]:8080, was never read. clientIpsFromEvidence trimmed each value and passed it to filter_var with FILTER_VALIDATE_IP, which takes a bare address only. The trim also meant that a value with a space, tab or line ending around the address was read, which the specification refuses. ClientIpTests.php had no case for either.
2. The client IP was not chosen in the specification's order (#99). clientIpsFromEvidence tried query.client-ip and server.client-ip before the other keys, read every Evidence value the Flow Data held (line 528) and not only the keys the engine accepts, and kept the first IPv4 and the first IPv6 address it found, of which line 258 looked up the IPv4 one whenever there was one.
3. Latitude and Longitude numbers were given with every place the conversion produces, for example 18.542741172521133. The only round( in SwigData.php is for weightings.

What the specifications say

The IP Intelligence on-premise page is quoted at specifications c8128ca4. Its client IP sections are the same on main at 6570727d, three lines earlier, as the page there differs only in its results cache note and its rule for reads by name.

Choosing the IP address

Lines 150 to 166 (147 to 163 on main):

The data file lists the client IP keys in priority order, for example true-client-ip-51d, then client-ip-51d, then client-ip, and the Engine accepts each of them under both the query and the server prefix. When the Evidence holds more than one of these keys, the native code uses one value, chosen in this order:

1. The query keys, in the data file's order.
2. Only when no query key supplied an address, the server keys, in the data file's order.

The prefix is decided first, as the Evidence precedence requires, and the data file's order decides between keys under one prefix. So query.client-ip is used ahead of server.true-client-ip-51d. The Property values in the result describe that one address.

Reading the client IP value

Lines 168 to 267. Lines 175 to 183:

A value is read only when all of it is one of these:

1. An IPv4 address in dotted-decimal form, such as 203.0.113.1.
2. An IPv6 address in any of the text forms of RFC 4291 section 2.2, such as 2001:db8::1, 2001:0DB8:0:0:0:0:0:1 or ::ffff:203.0.113.1.
3. An IPv4 address followed by a colon and a port, such as 203.0.113.1:8080.
4. An IPv6 address in square brackets, with or without a colon and a port after the closing bracket, such as [2001:db8::1] or [2001:db8::1]:8080.

The grammar, lines 189 to 194:

value = IPv4address / IPv6address
      / IPv4address ":" port
      / "[" IPv6address "]" [ ":" port ]
port  = 1*DIGIT      ; 0 to 65535

Lines 212 to 220:

A port is read and then dropped. It does not change the Property values. An IPv6 address MUST have brackets when a port is given. Without brackets, the text after the last colon is part of the address, as in 2001:db8::1:80, which is the address 2001:db8::1:80 and has no port.

A value that does not match, in whole, is not read. It is not an error and the Engine uses no part of it. The Engine tries the next key in the order given in choosing the IP address, so a value that is not read under one key does not hide a value under a key that comes later.

The table at lines 224 to 243 lists the values that are not read, and lines 249 to 252 say "The Engine MUST NOT read a value that holds a zone index, in any form, with or without brackets or a port. fe80::1%eth0, fe80::1%1, [fe80::1%eth0]:80 and the percent-encoded form [fe80::1%25eth0] are all refused whole."

The cloud side needs nothing. The IP Intelligence Cloud page, lines 15 to 18, says "This element uses no Evidence, it works on Property values in Element Data in the Flow Data", and the Cloud Request Engine page has no rule for client IP values. PHP's IpIntelligenceCloud and the Cloud Request Engine read no client IP, and send the Evidence to the cloud service as it was given.

Latitude and Longitude

data-model.md lines 53 to 56:

Latitude and Longitude, and the points of Areas, are given to 3 decimal places, as rounding of Azimuth and Declination values describes.

value-types.md lines 97 to 112:

A value converted from Azimuth or Declination MUST be given to the caller rounded to 3 decimal places, whether it is given as a number, as a string or as a point in a WKT shape. For example, a stored Declination of 6751 converts to 18.542741172521133 and is given as 18.543. Where the number is a binary floating point type, it is the value of that type nearest to the rounded decimal.

Both pages are the same on main. The value types page names no rounding for a value exactly half way. The native code writes the text half away from zero (stringBuilder.c lines 164 to 166, at the cxx commit this repository pins), as Rust's round_angle does, and PHP's round does the same. No value converted from a 16 bit Azimuth or Declination is exactly half way at the third place in any case, because 32767 is 7 times 31 times 151, which shares no factor with 180,000 or 360,000.

What the native code does with several keys

All of this is at cxx 7992af70, the commit this repository pins.

• The engine's keys come from IpiGetEvidenceKeys, ipi.c lines 2422 to 2455, which takes the data file's client IP headers in order and writes each one under query. and then server.. With the free data file, every job of the two runs for Choose the client IP address in the order the IP Intelligence specification gives #99 and of run 43548725414 below printed them as query.true-client-ip-51d, server.true-client-ip-51d, query.client-ip-51d, server.client-ip-51d, query.client-ip, server.client-ip.
• Given Evidence, ResultsIpiFromEvidence, lines 2855 to 2893, tries those headers in the same order under the query prefix, and under the server prefix only when nothing was read. setResultsFromEvidence, lines 2791 to 2829, looks up the first value that its parser reads and skips any other.
• That parser, IpAddressParse in ip.c lines 198 to 253, reads the same forms as this draft, with a port, with brackets, with no leading zero, and with nothing but hexadecimal digits, colons and dots in an IPv6 address, so a zone index is refused.

So this draft reads the same keys, in the same order, with the same forms as the native code, and chooses the value the native code would choose if it were given the Evidence.

Choosing in PHP, and why

PHP chooses the value and passes its one address to the native process(const char *ipAddress) (EngineIpi.cpp lines 323 to 337), as it did before, rather than handing the native code every accepted key.

1. PHP cannot hand the native code Evidence. The wrapper offers Evidence only as a subclass of std::map (EvidenceBase.i line 28), and setting a key on it stops PHP. Run 43554906863 built the extension of this draft's head 6a93144 as CI does, from the wrapper that #88 generated with SWIG 4.5.1 and committed, built an engine from the free data file on PHP 8.3, and ran a script under gdb. Its process with the string 116.154.188.222 worked and gave UNICOM and CN. Its next step, set('query.client-ip', '116.154.188.222') on a new FiftyOneDegreesIpiEvidenceIpiSwig, ended in a segmentation fault inside std::map's insert, reached from zim_FiftyOneDegreesIpiMapStringStringSwig_set at line 2459 of ipi_wrap.cxx (job 132325448876). Run 43548725414 had stopped in the same place on the head before the merge, with the wrapper that the runner's SWIG 4.2.0 generated then (job 132305975198). That is the failure the comment in processInternal describes. So the native choice could not be run from PHP to compare with this draft's, and the agreement rests on the code in the previous section and on the tests.
2. The Ip and IpV6 Properties report the chosen address, so PHP needs to know which address was looked up. The results do have getTargetIpAddress() (ResultsIpi.i line 105), but it gives the address itself only through getCopyOfIpAddress(unsigned char copy[], uint32_t size), and IpAddress.i lines 29 to 33 map that buffer for Java alone. So PHP would have to choose the address for these two Properties in any case.
3. The choice is tested without the extension, in the unit suite, which runs on every leg, as well as through the engine with a data file.

What changed and why

Choosing the client IP, in ip-intelligence/src/IpIntelligenceOnPremise.php

• getClientIps and clientIpsFromEvidence take the engine's keys, which initEngineMetaData already read from getKeys(). Only those keys are read, every query key before every server key (CLIENT_IP_PREFIXES), each in the order of the engine's keys.
• The first value that is read is chosen, whatever its family, and only its address is reported, in Ip for an IPv4 or IPv4-mapped address and in IpV6 otherwise.

Reading a client IP value, in the same file

• addressInValue gives the address a value holds, or null when the whole value is not one of the four forms. A value that starts with [ must close the bracket, hold an IPv6 address inside it, and have nothing or a port after it. Any other value must be an address, or an IPv4 address followed by a colon and a port. The port is dropped, so Ip and IpV6 report the address alone and the lookup uses it.
• isIpv4 and isIpv6 keep filter_var, which already refuses a leading zero in an IPv4 part, a second ::, more than eight groups and a group of five digits. Each adds a check of the characters, which refuses a zone index (it follows a %) and anything around the address whatever filter_var makes of it.
• isPort takes one or more digits for a number from 0 to 65535, as the grammar's port rule says.
• A value is no longer trimmed, because the table refuses "A space, tab or line ending before or after the address".

Latitude and Longitude, in ip-intelligence/src/SwigData.php

• The numbers of Latitude and Longitude are rounded with round($value, 3), which gives the double nearest the rounded decimal, whether they are read through getValuesAsWeightedDoubleList (types WeightedDouble and WeightedSingle) or getValueAsDouble (types Double and Single). See number and isAngle.
• Their text, and the points of Areas, are already written to 3 places by the native code, which passes DefaultWktDecimalPlaces (constantsIpi.h lines 43 to 47) whenever it writes a value as text (ResultsIpi.cpp lines 355, 499 and 583). So no text changes.
• The specification ties the rule to how a value is stored, as an Azimuth or a Declination. PHP cannot see that, because ResultsIpi::getPropertyValueType is private (ResultsIpi.hpp line 500) and not in the SWIG interface, and a property's metadata type never names Azimuth or Declination (propertyValueType.c lines 50 to 93). So the two properties are named, as the data model names them. They are the only number properties in .NET's IP Intelligence interface (IIpIntelligenceData.cs lines 211 and 230). Rust reads the stored type and rounds by it (double_text). .NET at main 8b3220b3 gives the number as the native code gives it (WeightedDoubleListSwigWrapper.cs line 51).
• Any other number keeps every place it has, and a Latitude with no value keeps its message.

Before you upgrade

1. Only the client IP keys the engine accepts are read. An address in any other Evidence value, such as header.x-forwarded-for, is no longer looked up. Give the address under one of the engine's keys, such as query.client-ip or server.client-ip.
2. When the Evidence holds more than one client IP key, every query key comes first and the data file's order decides between keys under one prefix, and the first value read is looked up whatever its family. So the address looked up, and with it Ip, IpV6 and every other Property value, can differ from before for such a request. Ip and IpV6 no longer both have a value, because only the chosen address is reported.
3. A client IP value with a space, tab or line ending around the address is no longer read, where before it was read with the space removed. Send the address alone.
4. A client IP value with a port, or an IPv6 address in brackets, is now read, where before it was passed over for the next key's value or for none. So Ip, IpV6 and the other Property values can change for such a request.
5. Latitude and Longitude numbers from the on-premise engine have at most 3 decimal places.

Tests

ClientIpTests.php gives clientIpsFromEvidence the free data file's keys in their getKeys() order (lines 38 to 63). For #99, lines 80 to 197:

• testOnlyTheChosenAddressIsReported replaces the old case that kept the first address of each family, which was the rule Choose the client IP address in the order the IP Intelligence specification gives #99 removes.
• testQueryKeysComeBeforeServerKeys sets two server keys, one of which the data file lists first, ahead of a query key in the Evidence.
• testKeysUnderOnePrefixFollowTheDataFileOrder takes two and three keys under one prefix set in the opposite order, and keys from a data file that lists the headers the other way round.
• testTheSameKeyUnderBothPrefixes takes the query value, and the server value when the query value is not read.
• testTheFirstValueIsUsedWhateverItsFamily puts an IPv6 address under a key ahead of an IPv4 address, under one prefix and across the two.
• testAValueTheEngineDoesNotAcceptIsNotRead takes header.x-forwarded-for, query.ip and cookie.client-ip, and query.client-ip with the keys of a data file that has no client-ip header.

For reading a value, lines 205 to 312:

• testEachAcceptedFormIsRead takes each form at lines 177 to 183, 0.0.0.0 (line 201), 2001:db8::1:80 (line 214), ports 0 and 65535, and an IPv4-mapped address in brackets with a port.
• testEachRefusedValueIsNotRead takes every row of the table, the space row as a leading or trailing space, tab, line feed, and carriage return with line feed, the four zone index forms of lines 250 and 251, 1.02.3.4 (line 199), and the bracket form of the colon, port and IPv4 rows.
• testARefusedValueDoesNotHideALaterKey takes the rule at lines 218 to 220.

IpIntelligenceOnPremiseTests::testTheClientIpIsChosenInTheEngineOrder goes through the engine with a data file and the keys it gives, and checks Ip, IpV6 and that every Property value is the one the chosen address gets when it is given alone.

SwigDataTests.php:

• testLatitudeAndLongitudeAreGivenToThreeDecimalPlaces takes the specification's example (a stored Declination of 6751 gives 18.543), Rust's (a stored Azimuth of 4274 gives 23.478), their negatives, and all four accessor types.
• testOtherNumbersAreNotRounded is a guard and passes before and after the change.

The red and green runs used temporary workflows on Ubuntu that build the IP Intelligence extension with ci/build-project.ps1 and run ip-intelligence/ci/run-unit-tests.ps1, as CI does, with PHPUnit 9.6.34. The two runs for #99 also download the free data file with cxx/data/get-data-files.sh and run IpIntelligenceOnPremiseTests.php with it. Each tmp branch held the commits named plus one commit adding the workflow.

Run	Head	PHP 8.0.30	PHP 8.3.36	PHP 8.5.12
Red 43544955927	4af81a2, which is rc 6d95eee with the tests of 0b0f033 and 0c5f947, where 0c5f947 is c584a02's tests on 0b0f033 (c584a02 itself sits on the first fix). The three test files are the same at 4af81a2, c584a02 and the head	Unit 46 tests, 9 failures. Engine 19 tests, 1 failure, 2 skipped	the same	the same
Green 43544955777	be41a1d, which is 5ca958d, this draft's head before rc was merged	Unit OK (46 tests, 176 assertions). Engine 19 tests, 120 assertions, 2 skipped, none failed	the same	the same
Red, first change only 43536947367	eefe737, which is rc with 0b0f033	Unit 41 tests, 4 failures	the same	the same
Green, first change only 43537066805	c96264b, which is 41ebe76	Unit OK (41 tests, 166 assertions)	the same	the same

The nine unit failures in the red run are the six new ClientIpTests cases for #99 less testTheSameKeyUnderBothPrefixes, which rc already passes, plus the four cases for the first change. The engine failure is testTheClientIpIsChosenInTheEngineOrder. The two skipped engine tests need the engine that PHP builds when it starts, and these runs start PHP with no data file set (requireStartupEngine). Each job of the two runs for #99 also ran a first version of the comparison script after the tests, which could not fail the job. It stopped in the script's own chained call on a returned value that was not held, the failure #92 describes, before it set any Evidence, and run 43548725414 above is the corrected script. All the tmp branches are deleted.

Full build

The head is 6a93144, which merges rc at 70424ea into the branch with a merge commit, bringing in #88 and #89 with no conflict. In the files of this change, the merge brings only #88's new class names in the constructor of IpIntelligenceOnPremise.php, which this change does not use. The red and green runs above tested the change before the merge.

Run 43554873847 built the head 6a93144 with common-ci's full pull request build, with merging turned off, and finished green (the run's last update is 04:35:45 UTC on 14 November). All nine Setting 'PullRequestSha' lines in its job logs read 6a93144, and Configure printed "Left out 5 macOS entries".

On Ubuntu with PHP 8.0, 8.1, 8.2, 8.3, 8.4 and 8.5, the IP Intelligence unit suite gave OK (46 tests, 176 assertions) and its integration suite gave OK (70 tests), with 846 assertions on PHP 8.0 and 842 on the others. The PHP 7.4 job leaves IP Intelligence out because the package needs PHP 8.0 or later, and the Windows job leaves it out because no extension is built on Windows. The draft's own checks are green on the same head.

An earlier full build, run 43547186289, was green on 5ca958d, the head before the merge.

macOS was left out of this draft's full build under the 9 November decision. The rc head is built with macOS before it merges into main.

What remains

1. A property other than Latitude and Longitude stored as an Azimuth or a Declination would not have its number rounded, because the wrapper does not give the stored type. Adding it to the cxx SWIG interface would let PHP round by it, as Rust does, which 51degrees-cxx#175 asks for.
2. This draft releases no package, because fiftyone.ipintelligence has no mirror and is not on Packagist (ci/packages.ps1 line 81).