Parse port suffixed client IP evidence and report parse failures accurately - #311
Parse port suffixed client IP evidence and report parse failures accurately#311Jamesr51d wants to merge 2 commits into
Conversation
…rately Hosting front ends commonly supply the visitor address with a port suffix, 85.118.2.126:53169 or [2001:db8::9]:443. IpiOnPremiseEngine passed the raw string to the native engine and used a strict IPAddress.TryParse for the echo properties, so such evidence broke the location lookup and returned null ip and ipv6. Adds IpEvidenceValue.TryParse to FiftyOne.IpIntelligence.Shared, accepting bare, port suffixed and bracketed forms. ProcessEngine now normalises whitelisted evidence values before handing them to the native engine and uses the tolerant parser for the echo selection. An unparseable query.client-ip falls through to server.client-ip and then to any other parseable whitelisted evidence. When evidence was provided but unusable the Ip and IpV6 NoValueMessage now says the IP was not valid instead of claiming no evidence was supplied. Root cause behind 51Degrees/cloud#208, 51Degrees/cloud#209 and 51Degrees/Website#856. Replaces the element level fix proposed in 51Degrees/cloud#210.
justadreamer
left a comment
justadreamer
left a comment
There was a problem hiding this comment.
Review with Fable (4 lenses + adversarial verification), plus a local build and dotnet test run. The core fix is sound: port-suffixed IPs now populate ip/ipv6 and drive the lookup, the misleading "not supplied" message is corrected, it builds clean, and the 22 new IpEvidenceValueTests pass.
Two things worth addressing:
- (main) The
IPAddress.TryParse+ToString()rewrite launders legacy inet_aton IPv4 forms into confident wrong lookups - a regression vsmain. See the inline suggestion onIpEvidenceValue.cs(strict dotted-quad gate; verified with 7 added rejection tests). - (major) The query-blocks-server fix is incomplete at the lookup layer for unparseable evidence, and the fall-through test only asserts the echo so it masks this. Inline comment on
IpiOnPremiseEngine.cs.
Plus one minor mirror-bug on the "not valid" message. Details inline.
| if (IPAddress.TryParse(candidate, out address)) | ||
| { | ||
| return true; | ||
| } | ||
| // IPv4 with a port suffix has exactly one colon. | ||
| var colon = candidate.IndexOf(':'); | ||
| if (colon > 0 && | ||
| colon == candidate.LastIndexOf(':') && | ||
| IsPortSuffix(candidate, colon) && | ||
| IPAddress.TryParse(candidate.Substring(0, colon), out address)) | ||
| { | ||
| return true; | ||
| } | ||
| address = null; | ||
| return false; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// True when the value from <paramref name="colonIndex"/> onward is | ||
| /// a colon followed by a decimal port number. | ||
| /// </summary> | ||
| private static bool IsPortSuffix(string value, int colonIndex) |
There was a problem hiding this comment.
IPAddress.TryParse accepts legacy inet_aton IPv4 notations that a real client address never uses and that the native engine rejects: single integer (12345 -> 0.0.48.57), fewer than four parts (85.118.2 -> 85.118.0.2), hex/octal components (0x7f.0.0.1, 192.168.015.1 -> 192.168.13.1, octal). Since ProcessEngine now rewrites evidence to parsedValue.ToString() before the native lookup, these are silently laundered into confident wrong addresses for both the lookup and the echoed ip/ipv6 (reproduced live: server.client-ip=85.118.2 -> Ip=85.118.0.2, Country=United Kingdom; on main this produced no result). Gating IPv4 results on a canonical round-trip closes it while leaving IPv6 untouched. Verified: 22 existing + 7 new IpEvidenceValueTests pass, and the port-suffix happy path is unchanged.
| if (IPAddress.TryParse(candidate, out address)) | |
| { | |
| return true; | |
| } | |
| // IPv4 with a port suffix has exactly one colon. | |
| var colon = candidate.IndexOf(':'); | |
| if (colon > 0 && | |
| colon == candidate.LastIndexOf(':') && | |
| IsPortSuffix(candidate, colon) && | |
| IPAddress.TryParse(candidate.Substring(0, colon), out address)) | |
| { | |
| return true; | |
| } | |
| address = null; | |
| return false; | |
| } | |
| /// <summary> | |
| /// True when the value from <paramref name="colonIndex"/> onward is | |
| /// a colon followed by a decimal port number. | |
| /// </summary> | |
| private static bool IsPortSuffix(string value, int colonIndex) | |
| if (TryParseStrict(candidate, out address)) | |
| { | |
| return true; | |
| } | |
| // IPv4 with a port suffix has exactly one colon. | |
| var colon = candidate.IndexOf(':'); | |
| if (colon > 0 && | |
| colon == candidate.LastIndexOf(':') && | |
| IsPortSuffix(candidate, colon) && | |
| TryParseStrict(candidate.Substring(0, colon), out address)) | |
| { | |
| return true; | |
| } | |
| address = null; | |
| return false; | |
| } | |
| /// <summary> | |
| /// Parse a bare address but reject the legacy IPv4 notations | |
| /// (octal or hex components, or fewer than four parts) that | |
| /// IPAddress.TryParse accepts. A real client address is always a | |
| /// canonical dotted quad, and the native engine only accepts that | |
| /// form, so a value like "85.118.2" or "192.168.015.1" must fail | |
| /// here rather than be silently rewritten to a different address. | |
| /// IPv6 is parsed as-is. | |
| /// </summary> | |
| private static bool TryParseStrict(string value, out IPAddress address) | |
| { | |
| if (IPAddress.TryParse(value, out address) == false) | |
| { | |
| return false; | |
| } | |
| if (address.AddressFamily == | |
| System.Net.Sockets.AddressFamily.InterNetwork && | |
| address.ToString() != value) | |
| { | |
| // A canonical dotted quad round-trips; a legacy form does not. | |
| address = null; | |
| return false; | |
| } | |
| return true; | |
| } | |
| /// <summary> | |
| /// True when the value from <paramref name="colonIndex"/> onward is | |
| /// a colon followed by a decimal port number. | |
| /// </summary> | |
| private static bool IsPortSuffix(string value, int colonIndex) |
There was a problem hiding this comment.
Agreed and applied. Rebuilt the helper around the native parser itself. The native parser reads one address from the start of the value and stops at the first break character, comma, space, slash, closing bracket, newline, or a colon once the address is known to be IPv4. So port suffixes, forwarded chains (first entry) and CIDR ranges (prefix) already parse natively and never needed rewriting. TryParse now mirrors that first token read, with your strict IPv4 gate and a rejected zone index as the only divergences, and the evidence passes to the native engine verbatim.
| [DataRow("85.118.2.126:banana")] | ||
| [DataRow("85.118.2.126:99999")] | ||
| [DataRow("85.118.2.126:")] | ||
| [DataRow("300.1.2.3:80")] |
There was a problem hiding this comment.
Pin the strict rejection so the legacy IPAddress.TryParse forms can't creep back in (pairs with the TryParseStrict suggestion; all pass locally).
| [DataRow("300.1.2.3:80")] | |
| [DataRow("300.1.2.3:80")] | |
| [DataRow("85.118.2")] | |
| [DataRow("85.118.2:53169")] | |
| [DataRow("12345")] | |
| [DataRow("12345:80")] | |
| [DataRow("192.168.015.1")] | |
| [DataRow("192.168.015.001:53169")] | |
| [DataRow("0x7f.0.0.1")] |
There was a problem hiding this comment.
Added, plus other rows: a malformed suffix after the break character no longer invalidates the address it follows (85.118.2.126:banana parses as 85.118.2.126, matching what the native parser does), a chain parses as its first entry only, and a CIDR range parses as its prefix.
| // "[2001:db8::9]:443"). The native engine expects | ||
| // a bare address, so pass the parsed form when the | ||
| // value is readable and the raw string otherwise. | ||
| if (IpEvidenceValue.TryParse(value, out var parsedValue)) |
There was a problem hiding this comment.
The lookup-layer fix is incomplete for a genuinely unparseable (non-port) query.client-ip. When TryParse fails the raw value is still added to relevantEvidence, and the native engine aborts on the query-prefix pass before it ever tries server.client-ip. Reproduced live: {query.client-ip="not-an-ip", server.client-ip="85.118.2.126"} -> Country empty, whereas server.client-ip alone -> "United Kingdom". So the query-blocks-server problem is fixed only at the echo layer.
Process_UnparseableQueryEvidence_FallsThroughToServer asserts only data.Ip (the echo), so it passes while the actual lookup stays broken - the test name overpromises. Consider skipping values that don't parse when building relevantEvidence here, and adding a Country assert to that test.
There was a problem hiding this comment.
Fixed. Values TryParse() rejects are now skipped instead of passed raw, so query.client-ip=not-an-ip falls through to server.client-ip at the lookup layer too. Process_UnparseableQueryEvidence_FallsThroughToServer now asserts Country as well as the echo. Note a plain per entry parse would have left a hole, a chain like "not-an-ip, 1.2.3.4" has a parseable second entry but the native parser only reads the first, so it still aborts. The first token mirror closes that, pinned by Process_ChainWithUnparseableFirstEntry_FallsThroughToServer.
| } | ||
|
|
||
| (ipData as IpDataOnPremise).SetEchoIp(echoV4, echoV6); | ||
| (ipData as IpDataOnPremise).SetEchoIp(echoV4, echoV6, ipEvidenceProvided); |
There was a problem hiding this comment.
ipEvidenceProvided is set for any non-whitespace filtered evidence, but the echo is parsed with the stricter IpEvidenceValue.TryParse. For forms the native parser accepts but the .NET parser doesn't (forwarded chain "85.118.2.126, 10.0.0.1", CIDR "1.2.3.4/24"), Country resolves yet the echo reports "The IP supplied as evidence was not valid." - contradicting the populated location properties in the same FlowData. It's the mirror of the bug this PR fixes. Deriving evidenceProvided from IP-shaped evidence (or from whether the native lookup produced a result) would keep the message honest.
There was a problem hiding this comment.
Agreed. Helper should mirror native parser. The echo and the native lookup now read the same first token, so a chain echoes the entry the lookup used and a CIDR range echoes the prefix address the native parser stops at. The flag passed to SetEchoIp is now provided-and-none-usable, derived from the same parse that gates the native evidence, so "The IP supplied as evidence was not valid." only appears when the native lookup got nothing at all. I preferred this over checking whether the native results contain values because an in range miss on a valid address should not flip the echo message.
…Pv4 forms Address review comments. IpEvidenceValue.TryParse now reads evidence exactly the way the native parser does. It takes the single address at the start of the value and stops at the first break character, comma, space, slash, closing bracket, newline, or a colon once the address is known to be IPv4. That makes port suffixes, forwarded chains, first entry, and CIDR ranges, prefix address, parse without any rewriting, because the native engine already handles those forms itself. The evidence handed to the native engine is the raw trimmed value. A value TryParse rejects is one the native parser would reject too, and since a native parse failure aborts the whole evidence walk before lower priority evidence is tried, such values are skipped. This also covers a chain whose first entry is unparseable, which a per entry parse would wrongly keep. IPv4 results are gated on a canonical dotted quad round trip, one deliberate divergence from the native parser, so legacy inet_aton forms, 85.118.2, 192.168.015.1, 0x7f.0.0.1, fail instead of being reinterpreted as a different address. A zone index is rejected to match the native treatment of the percent character. The echo uses the same parse, so the echoed ip and ipv6 always agree with the address the native lookup used, including the prefix of a CIDR range. The not valid message only fires when evidence was provided and none of it was usable. The engine tests pin the chain, CIDR, fall through and message behaviour against the real engine, and the fall through test asserts Country as well as the echo.
2 participants
@-