elastic / elastic-package

elastic-package - Command line tool for developing Elastic Integrations
Other
50 stars 116 forks source link

Allowed IP List from allowed_geo_ips.txt is insane #1074

Open colin-stubbs opened 1 year ago

colin-stubbs commented 1 year ago

This should not happen if RFC-1918 or RFC-5737 (IPv4 documentation addresses) or RFC-3849 ( IPv6 documentation addresses) are used.

In fact, none of the IP's here,

https://github.com/elastic/elastic-package/blob/main/internal/fields/_static/allowed_geo_ips.txt

1.128.0.0/11. - owned by Telstra Australia ? What does this have to do with Elastic? 175.16.199.0/24 - AS4837 CHINA UNICOM. What does this have to do with Elastic? 216.160.83.56/29 - AS209 CenturyLink Communications. What does this have to do with Elastic? 81.2.69.142/31 - AS20712 Andrews & Arnold Ltd. What does this have to do with Elastic? 81.2.69.144/31 - AS20712 Andrews & Arnold Ltd. What does this have to do with Elastic? 81.2.69.192/28 - AS20712 Andrews & Arnold Ltd. What does this have to do with Elastic? 89.160.20.112/28 - AS29518 Bredband2 AB. What does this have to do with Elastic? 89.160.20.128/25 - AS29518 Bredband2 AB. What does this have to do with Elastic? 67.43.156.0/24 - Loud Packet Inc. What does this have to do with Elastic? 2a02:cf40::/29 - Christian Ebsen ApS. What does this have to do with Elastic?

None of these should be permitted, at all, ever.

If there's any kind of checks for unsanitised IP's, the elastic-package tool should be enforcing use of RFC-5737/RFC-3849 documentation IP's.

Example error below that lead me to this point,

blah@asdf forcepoint_web % elastic-package test
Run test suite for the package
Run pipeline tests for the package
--- Test results for package: forcepoint_web - START ---
FAILURE DETAILS:
forcepoint_web/logs test-forcepoint-web.json:
[0] parsing field value failed: the IP "192.0.2.68" is not one of the allowed test IPs (see: https://github.com/elastic/elastic-package/blob/main/internal/fields/_static/allowed_geo_ips.txt)
[1] parsing field value failed: the IP "203.0.113.96" is not one of the allowed test IPs (see: https://github.com/elastic/elastic-package/blob/main/internal/fields/_static/allowed_geo_ips.txt)

╭────────────────┬─────────────┬───────────┬──────────────────────────┬─────────────────────────────────────────────────────────────────────────────┬──────────────╮
│ PACKAGE        │ DATA STREAM │ TEST TYPE │ TEST NAME                │ RESULT                                                                      │ TIME ELAPSED │
├────────────────┼─────────────┼───────────┼──────────────────────────┼─────────────────────────────────────────────────────────────────────────────┼──────────────┤
│ forcepoint_web │ logs        │ pipeline  │ test-forcepoint-web.json │ FAIL: test case failed: one or more problems with fields found in documents │   3.202709ms │
╰────────────────┴─────────────┴───────────┴──────────────────────────┴─────────────────────────────────────────────────────────────────────────────┴──────────────╯
--- Test results for package: forcepoint_web - END   ---
Done
Error: one or more test cases failed
blah@asdf forcepoint_web % 
colin-stubbs commented 1 year ago

Right. Kinda makes sense after spending a few more minutes thinking about it. Closed.

colin-stubbs commented 1 year ago

Nope, reopening, this still makes no sense. elastic-package should not be telling users that RFC-5737 IP's are not allowed. GeoIP lookups on those IP's returns no results, so there's no geo fields set, and even if GeoIP enrichment is being attempted in the ingest pipeline, there's still no reason to have elastic-package whinging about the use of those IP's and forcing the integration developer to pick and use a public IP in test logs.

RFC-1918/RFC-5737/RFC-3849 IP's should be in https://github.com/elastic/elastic-package/blob/main/internal/fields/_static/allowed_geo_ips.txt so that this warning isn't raised.

jsoriano commented 1 year ago

Hey @colin-stubbs,

This is not about validity of IPs themselves, it is about the range of IPs included in the GeoIP database embedded in elastic package, these ranges are used only for enrichment purpouses, they are never used or expected to be used in real deployments. You can read more about this in the docs.

We want to avoid having IPs with no results, because they may hide issues with GeoIP enrichment. We want to use a fixed GeoIP database to avoid flakiness when validating these fields. We don't want to distribute full databases, because of possible licensing and size issues. So we use a reduced test database.

Current solution is of course not ideal, it is a compromise solution with some trade-offs, but it is far from "insane" or non sensical.

It could make sense to include IPs of the documentation ranges in the allowed list, but we would need to add GeoIP information to the database we use now. Not sure though how much value this would provide.

PRs are open, we'd be happy to accept a better database than the one we use now, this is just not a priority for us at the moment.

jsoriano commented 1 year ago

@colin-stubbs btw, you can find here the implementation of this check: https://github.com/elastic/elastic-package/blob/5370fcb05d05f29455d3cd6cd63762e9cf696e71/internal/fields/validate.go#L597

It also allows the use of private and broadcast addresses even when they don't have GeoIP information, so RFC-1918 should be covered.

colin-stubbs commented 1 year ago

@jsoriano cheers, I'll work it out and issue a pull request, first glance suggests there should be an isDocumentation() function to allow RFC-5737/3849.

colin-stubbs commented 1 year ago

@jsoriano not sure if you're responsible or can otherwise review/approve a PR here, but I've submitted a PR for this now.