Keep the fetcher off private address space by default - #2127
Conversation
The IP address filter was the only check that looked at the address the fetcher actually connects to, and both of its keys shipped commented out, so the interceptor was never installed at library defaults. The regex exclusion list in the archetypes matched literal localhost and four dotted-quad ranges and nothing else: link-local, CGNAT, IPv6 unique-local and the abbreviated and integer IPv4 forms the resolver maps to 127.0.0.1 all passed it. - crawler-default.yaml ships http.filter.ipaddress.exclude enabled (loopback, RFC1918, link-local, CGNAT, IPv6 unique-local); a fetched page decides which hosts the fetcher connects to, and these ranges host unauthenticated services a public index must not leak into. Crawling an intranet needs an explicit opt-out, documented in the comment next to the key - the archetype default-regex-filters.txt (and its opensearch/solr copies) gains rules for 169.254.0.0/16, 100.64.0.0/10, 0.0.0.0/8, IPv6 fc00::/7 and fe80::/10, the abbreviated loopback forms (127.1) and integer IPv4 hosts (2130706433), and says in its comment that a regex list judges the bytes of the URL only, so the IP filter stays the authoritative check
rzo1
left a comment
There was a problem hiding this comment.
Currently I am short on time, so first round AI-based review)
Enabling http.filter.ipaddress.exclude by default is correct, and the new NOTE saying the post-DNS filter is the authoritative check belongs in this file. One regex is wrong; see line 45.
Unrelated change. This PR also swaps the skip-suffix regex in core/src/test/resources/default-regex-filters.txt for a much longer list and rewrites a comment. RegexFilterTest and URLFilterBoltTest both load that file. Aligning it with the archetype copy is fine, but it does not belong to #2080. Please split it out, or explain in the description why it has to be here.
Test style. DefaultRegexFiltersPrivateRangeTest.java has imports out of order (java.nio.charset before java.net) and several inline fully-qualified names producing over-long lines. CI runs the format check (-Dskip.format.code=false), so please import Utils, ConfUtils, IPFilterRules and InetAddress properly.
Worth putting in the description so nobody chases it: the JVM resolver rejects hex (0x7f000001 gives UnknownHostException) and does not treat leading zeros as octal (0177.0.0.1 gives 177.0.0.1), so the \d{1,10} rule closes the only numeric form that actually works.
Note this overlaps with #2119, which also touches the http.filter.ipaddress.exclude example. Worth agreeing which PR owns the shipped value.
| # 169.254.0.0/16 (link-local, e.g. cloud instance metadata services) | ||
| -^https?://(?:169\.254(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){2})(?::\d+)?(?:/|$) | ||
| # 100.64.0.0/10 (carrier-grade NAT) | ||
| -^https?://(?:100\.(?:6[0-4]|[7-9][0-9]|1[01][0-9]|12[0-7])(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){2})(?::\d+)?(?:/|$) |
There was a problem hiding this comment.
100.64.0.0/10 is 100.64.0.0 to 100.127.255.255, i.e. second octet 64 to 127.
The alternation is 6[0-4]|[7-9][0-9]|1[01][0-9]|12[0-7], which blocks 60 to 64 (60 to 63 are public space) and misses 65 to 69 entirely. The test only checks 100.64.0.1 and 100.127.255.254, so neither end of the mistake is covered.
| -^https?://(?:100\.(?:6[0-4]|[7-9][0-9]|1[01][0-9]|12[0-7])(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){2})(?::\d+)?(?:/|$) | |
| -^https?://(?:100\.(?:6[4-9]|[7-9][0-9]|1[01][0-9]|12[0-7])(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){2})(?::\d+)?(?:/|$) |
Please apply to all four copies of the file and add 100.65.0.1 and 100.63.0.1 to otherNonRoutableRangesAreRejected.
| # 0.0.0.0/8 ("this network") | ||
| -^https?://(?:0(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){1,3})(?::\d+)?(?:/|$) | ||
| # IPv6 unique-local (fc00::/7) and link-local (fe80::/10) addresses | ||
| -^https?://\[(?:f[cd][0-9a-f]{2}|fe[89ab][0-9a-f]):[0-9a-f:]+\](?::\d+)?(?:/|$) |
There was a problem hiding this comment.
Two gaps here.
Case. RegexURLFilter does not compile with CASE_INSENSITIVE (the suffix rule at the top of the file carries its own (?i)), so [FD00::1] and [FE80::1] pass. Same for LOCALHOST on line 29.
Loopback spellings. Both of these reach loopback through the JVM resolver while matching neither rule:
InetAddress.getByName("::ffff:127.0.0.1") -> /127.0.0.1
InetAddress.getByName("0:0:0:0:0:0:0:1") -> /0:0:0:0:0:0:0:1 (== ::1)
| -^https?://\[(?:f[cd][0-9a-f]{2}|fe[89ab][0-9a-f]):[0-9a-f:]+\](?::\d+)?(?:/|$) | |
| -^(?i)https?://\[(?:f[cd][0-9a-f]{2}|fe[89ab][0-9a-f]):[0-9a-f:]+\](?::\d+)?(?:/|$) | |
| # loopback reached through an IPv4-mapped or fully expanded IPv6 literal | |
| -^(?i)https?://\[(?:::ffff:127\.|(?:0*:){7}0*1\]) |
isLoopbackAddress() catches all of these after DNS now that the IP filter is on by default, so this is defence in depth, but catching them earlier is what this file is for.
| # http://2130706433/ | ||
| # http://[::1]/ | ||
| -^https?://(?:localhost|127(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){3}|\[::1\])(?::\d+)?(?:/|$) | ||
| -^https?://(?:localhost|127(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){0,3}|\[::1\])(?::\d+)?(?:/|$) |
There was a problem hiding this comment.
Case-sensitive, so LOCALHOST passes. Worth (?i) here too, same as the IPv6 rule on line 49.
| # 169.254.0.0/16 (link-local, e.g. cloud instance metadata services) | ||
| -^https?://(?:169\.254(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){2})(?::\d+)?(?:/|$) | ||
| # 100.64.0.0/10 (carrier-grade NAT) | ||
| -^https?://(?:100\.(?:6[0-4]|[7-9][0-9]|1[01][0-9]|12[0-7])(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){2})(?::\d+)?(?:/|$) |
There was a problem hiding this comment.
Same CGNAT bug as in the archetype copy; see the comment there.
| # 0.0.0.0/8 ("this network") | ||
| -^https?://(?:0(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){1,3})(?::\d+)?(?:/|$) | ||
| # IPv6 unique-local (fc00::/7) and link-local (fe80::/10) addresses | ||
| -^https?://\[(?:f[cd][0-9a-f]{2}|fe[89ab][0-9a-f]):[0-9a-f:]+\](?::\d+)?(?:/|$) |
There was a problem hiding this comment.
Same IPv6 gaps as in the archetype copy; see the comment there.
| # The exclude list is enabled by default: a fetched page decides which hosts | ||
| # the fetcher connects to, and loopback, private and link-local ranges host | ||
| # unauthenticated services (e.g. cloud instance metadata) which a public | ||
| # index must not leak into. Set http.filter.ipaddress.include to any value |
There was a problem hiding this comment.
This advice does not work as written.
Set http.filter.ipaddress.include to any value (e.g. "0.0.0.0/0") and leave the exclude list empty
Exclude wins over include in IPFilterRules.accept, so setting only include changes nothing: the shipped exclude list on line 169 still applies. The operator has to write http.filter.ipaddress.exclude: "" explicitly. Please say that.
Also, fd00::/8 here only covers the locally-assigned half of ULA; fc00::/7 is the full range. Same point as in #2119.
|
@abhinav-phi CI is red here, could you have a look please? Two Checkstyle violations, both the import order I flagged in the review: Reproduce with |
Fixes #2080.
The connection-time IP filter was the only check that looked at the address the fetcher actually connects to, and both of its keys shipped commented out in
crawler-default.yaml, so at defaults the interceptor was never installed. The regex exclusion list in the archetypes matched literallocalhostand four dotted-quad ranges and nothing else — link-local, CGNAT, IPv6 unique-local and the abbreviated and integer IPv4 forms the JVM resolver maps to 127.0.0.1 all passed it, andInetAddress.getByNamemaps127.1and2130706433to loopback without any DNS control.crawler-default.yamlshipshttp.filter.ipaddress.excludeenabled:localhost,sitelocal,linklocal,100.64.0.0/10,fd00::/8. A fetched page decides which hosts the fetcher connects to, and these ranges host unauthenticated services (e.g. cloud instance metadata at 169.254.169.254, reachable whatever DNS name a link used, since the check runs on the resolved socket address) which a public index must not leak intodefault-regex-filters.txt(and its opensearch/solr copies) gains rules for 169.254.0.0/16, 100.64.0.0/10, 0.0.0.0/8, IPv6fc00::/7andfe80::/10, abbreviated loopback (127.1) and integer IPv4 hosts (2130706433), and states that a regex list judges the bytes of the URL only — it can never enforce where a host name resolves, so the IP filter stays the authoritative checkRelease note needed: enabling the exclude list by default changes behaviour for anyone crawling an intranet or a loopback service; the way to opt out is documented next to the key.