Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allowed IP List from allowed_geo_ips.txt is insane #1074

Open
colin-stubbs opened this issue Dec 16, 2022 · 6 comments
Open

Allowed IP List from allowed_geo_ips.txt is insane #1074

colin-stubbs opened this issue Dec 16, 2022 · 6 comments

Comments

@colin-stubbs
Copy link

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 colin-stubbs closed this as not planned Won't fix, can't repro, duplicate, stale Dec 16, 2022
@colin-stubbs
Copy link
Author

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

@colin-stubbs colin-stubbs reopened this Dec 16, 2022
@colin-stubbs
Copy link
Author

colin-stubbs commented Dec 16, 2022

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
Copy link
Member

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
Copy link
Member

@colin-stubbs btw, you can find here the implementation of this check:

func (v *Validator) isAllowedIPValue(s string) bool {

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
Copy link
Author

@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 added a commit to colin-stubbs/elastic-package that referenced this issue Feb 2, 2023
Add IsDocumentation function to test for RFC 5737/3849 IP usage
@colin-stubbs
Copy link
Author

colin-stubbs commented Feb 2, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants