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

Fix for issue #1074 #1132

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Fix for issue #1074 #1132

wants to merge 5 commits into from

Conversation

colin-stubbs
Copy link

Adds an IsDocumentation function to check if IP's are reserved IP's used for documentation, these should be permitted in sample/test events in integrations.

Adds CloudFlare and Google public DNS server IP addresses, which provides additional completely benign options for valid/real public IP's to be permitted in sample events for integrations.

@elasticmachine
Copy link
Collaborator

elasticmachine commented Feb 2, 2023

❕ Build Aborted

The PR is not allowed to run in the CI yet

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Start Time: 2023-02-20T09:08:44.922+0000

  • Duration: 2 min 22 sec

Steps errors 3

Expand to view the steps failures

Load a resource file from a library
  • Took 0 min 0 sec . View more details here
  • Description: approval-list/elastic/elastic-package.yml
Google Storage Download
  • Took 0 min 0 sec . View more details here
Error signal
  • Took 0 min 0 sec . View more details here
  • Description: githubApiCall: The REST API call https://api.github.com/orgs/elastic/members/colin-stubbs return the message : java.lang.Exception: httpRequest: Failure connecting to the service https://api.github.com/orgs/elastic/members/colin-stubbs : httpRequest: Failure connecting to the service https://api.github.com/orgs/elastic/members/colin-stubbs : Code: 404Error: {"message":"User does not exist or is not a member of the organization","documentation_url":"https://docs.github.com/rest/reference/orgs#check-organization-membership-for-a-user"}

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@jsoriano
Copy link
Member

jsoriano commented Feb 2, 2023

/test

Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @colin-stubbs, thanks for opening this PR! Added a couple of comments about the IPs being included.

@marc-gr could you or someone from your team please take also a look to this change?

2001:4860:4860::8888/128
2001:4860:4860::8844/128
2001:4860:4860::64/128
2001:4860:4860::6464/128
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these IPs included in the current GeoIP database? At least some of them are not related to documentation.

The idea of this list is to have a reference of IP ranges that are included in the GeoIP database, so package developers can know when GeoIP enrichment is working.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeap, from: https://www.maxmind.com/en/geoip-demo

IP Address Country Code Location Network Postal Code Approximate Coordinates* Accuracy Radius (km) ISP Organization Domain Metro Code
2001:4860:4860::8888 US United States,North America 2001:4860:4860::/46   37.751,-97.822 100 Google Google    
2001:4860:4860::8844 US United States,North America 2001:4860:4860::/46   37.751,-97.822 100 Google Google    
2001:4860:4860::64 US United States,North America 2001:4860:4860::/46   37.751,-97.822 100 Google Google    
2001:4860:4860::6464 US United States,North America 2001:4860:4860::/46   37.751,-97.822 100 Google Google    

@@ -648,6 +667,7 @@ func (v *Validator) isAllowedIPValue(s string) bool {

if ip.IsUnspecified() ||
ip.IsPrivate() ||
IsDocumentation(ip) ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am ok with allowing documentation IPs for this purpose, but I would prefer if apart of allowing them, at least any of them are also included in the GeoIP database, so package developers can use them to check that geoip enrichment works.

Copy link
Author

@colin-stubbs colin-stubbs Feb 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jsoriano not possible, the whole point point of documentation addresses is that they are used in place of real addresses in documentation, the test events and sample events that wind up in the integration are a form of documentation and should be permitted. There is no GeoIP data to be associated with them.

For anyone who is generating test events in a lab environment as part of Elastic integration testing, they can/should and likely will use the RFC addresses allocated for this purpose.

They likely will not, and should not, take say... 89.160.20.128/25 - which is a real IP block belonging to someone in Swedend, and use that in their lab environment to generate test events.

Summary: allowing the documentation IP's is important to support test events generated in a lab environment. It's not to support GeoIP lookups in any way.

The documentation IP's are complementary to RFC-1918 private addresses and should be permitted.

Comment on lines 230 to 232
// Following RFC 3849, Section 2. Documentation IPv6 Address Prefix which
// says:
// The prefix allocated for documentation purposes is 2001:DB8::/32
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation looks wrong here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can run make check-static for linting.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make build / make works fine... I'm already using a locally build version of elastic-agent

make lint reports no errors.

make check-static is whinging but unhelpful...

user@box elastic-package % make check-static
go build -ldflags "-X github.com/elastic/elastic-package/internal/version.CommitHash=`git describe --always --long --dirty` -X github.com/elastic/elastic-package/internal/version.BuildTime=`date +%s` -X github.com/elastic/elastic-package/internal/version.Tag=`(git describe --exact-match --tags 2>/dev/null || echo '') | tr -d '\n'`" -o elastic-package
go run golang.org/x/tools/cmd/goimports -local github.com/elastic/elastic-package/ -w .
go run honnef.co/go/tools/cmd/staticcheck ./...
go run github.com/elastic/go-licenser -license Elastic
go mod tidy
cd tools/readme; go run main.go
2023/02/03 12:23:54 generating command doc for benchmark...
2023/02/03 12:23:54 generating command doc for build...
2023/02/03 12:23:54 generating command doc for changelog...
2023/02/03 12:23:54 generating command doc for check...
2023/02/03 12:23:54 generating command doc for clean...
2023/02/03 12:23:54 generating command doc for create...
2023/02/03 12:23:54 generating command doc for dump...
2023/02/03 12:23:54 generating command doc for export...
2023/02/03 12:23:54 generating command doc for format...
2023/02/03 12:23:54 generating command doc for install...
2023/02/03 12:23:54 generating command doc for lint...
2023/02/03 12:23:54 generating command doc for profiles...
2023/02/03 12:23:54 generating command doc for promote...
2023/02/03 12:23:54 generating command doc for publish...
2023/02/03 12:23:54 generating command doc for report...
2023/02/03 12:23:54 generating command doc for service...
2023/02/03 12:23:54 generating command doc for stack...
2023/02/03 12:23:54 generating command doc for status [package]...
2023/02/03 12:23:54 generating command doc for test...
2023/02/03 12:23:54 generating command doc for uninstall...
2023/02/03 12:23:54 generating command doc for version...
README.md successfully written
git update-index --really-refresh
internal/fields/validate.go: needs update
make: *** [check-git-clean] Error 1
user@box elastic-package % make build
go build -ldflags "-X github.com/elastic/elastic-package/internal/version.CommitHash=`git describe --always --long --dirty` -X github.com/elastic/elastic-package/internal/version.BuildTime=`date +%s` -X github.com/elastic/elastic-package/internal/version.Tag=`(git describe --exact-match --tags 2>/dev/null || echo '') | tr -d '\n'`" -o elastic-package
user@box elastic-package % 

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation looks wrong here.

Ewww, tabs in use... I've replaced the spaces and pushed another commit.

@jsoriano jsoriano requested review from marc-gr and a team February 2, 2023 12:24
@jsoriano
Copy link
Member

/test

@sharbuz
Copy link
Contributor

sharbuz commented Jul 10, 2023

Hi everyone,
Please update the branch because we merged an important change to the main branch: #1347

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

Successfully merging this pull request may close these issues.

4 participants