Update: Add support for x_forwarded_for headers in apaches access logs by redcinelli · Pull Request #3251 · elastic/integrations

redcinelli · 2022-05-02T12:52:11Z

What does this PR do?

Tldr;
Enable support for apache logs with the x_forwarded_for header.

Heavily inspired by the work done in this PR
Addressing this Enhancement Request.

Grok pattern has been updated to match logs starting with a list of IP, storing it in apache.access.remote_ip.
This pattern is heavily inspired by the nginx integration.

A painless scrip extract the first public ip, and store the result in source.address.

network.forwarded_ip field is being filled by source.ip, as it seems to be the perfect fit.

Checklist

I have reviewed tips for building integrations and this pull request is aligned with them.
I have verified that all data streams collect metrics or logs.
I have added an entry to my package's changelog.yml file.
I have verified that Kibana version constraints are current according to guidelines.

Author's Checklist

Do we want to support x_forward_for ?
Lots of noise in the test files, let's double check it is really backward compatible.
Simplify the Grok pattern, verify the Grok pattern
I am allowed to modify the httpd.conf ?

How to test this PR locally

Following the steps in the documentation

elastic-package stack down && elastic-package build && elastic-package stack up -d -v --services=elasticsearch && eval "$(elastic-package stack shellinit)" && elastic-package test pipeline --generate

Related issues

None

Screenshots

None

elasticmachine · 2022-05-02T13:05:49Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-08-26T12:12:53.719+0000
Duration: 16 min 6 sec

Test stats 🧪

Test	Results
Failed	0
Passed	44
Skipped	0
Total	44

🤖 GitHub comments

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

/test : Re-trigger the build.

elasticmachine · 2022-05-02T13:05:59Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	100.0% (`2/2`)	💚
Files	50.0% (`2/4`)	👎 -47.11
Classes	50.0% (`2/4`)	👎 -47.11
Methods	70.0% (`28/40`)	👎 -19.623
Lines	70.234% (`210/299`)	👎 -21.514
Conditionals	100.0% (`0/0`)	💚

redcinelli · 2022-05-16T14:01:00Z

I believe my pr to be good for review 🤞

At least open for discussion.

Btw seeing the whole mess the .json files are should I first do a pr to "update" the test then rebase this pr ?

packages/apache/data_stream/access/elasticsearch/ingest_pipeline/default.yml

packages/apache/data_stream/access/fields/fields.yml

packages/apache/data_stream/access/elasticsearch/ingest_pipeline/default.yml

packages/apache/data_stream/access/_dev/test/pipeline/test-access-vhost.log-expected.json

redcinelli · 2022-05-17T13:37:29Z

@andrewkroh I believe it is worth taking a look again ^^

Thanks a lot for helping me.

andrewkroh

I was reviewing this from technical perspective on the code/config in the pipeline. I didn't really consider the big picture with respect to ECS mappings, the field meanings, and whether this is a breaking change.

Taking a step back and looking at the approach here, this looks like a change in behavior w.r.t. to how source is populated. IMO the source.address should be the IP (or hostname) where traffic came from (the last hop). That is %{c}h or %h or (barring any changes from things like mod_remoteip) in mod_log_config format. This would be the same source address observed if we looked at a packet capture.

The ECS network.forwarded_ip would be populated only when X-Forwarded-For is used because this field is only used when the source address represents a proxy. I think the first address in the X-Forwarded-For list would always be used to fill network.forwarded_ip.

I think the new apache.access.remote_ip_list field should be the x-forwarded-for list plus the remote hostname (%h value). This would mean that all the hops from the client through the last proxy are accounted for in the list. And given the reference info you pointed me to, perhaps the name of this field is better as apache.access.remote_addresses (data type keyword) so that we can take the raw values without caring if they are IPs.

In order to have both %h and %{X-Forwarded-For}i available in the log I think we should prescribe formats in the docs that it will accept. We want to continue accepting the current format as is and we also want to allow X-Forwarded-For to be added by users. So perhaps we should allow another header to be tacked on to the end like:

%h %l %u %t \"%r\" %>s %b \"%{Referer}i\" \"%{User-Agent}i\" X-Forwarded-For=\"%{X-Forwarded-For}i\"

packages/apache/data_stream/access/elasticsearch/ingest_pipeline/default.yml

redcinelli · 2022-05-18T07:48:17Z

Hi Andrew, so 2 new commits:

e3f15b9 to address your suggestions (null checks, weird try/catch ...)
4abfdbb to try to address the bigger picture

packages/apache/data_stream/access/elasticsearch/ingest_pipeline/default.yml

andrewkroh

Overall LGTM, but I think there is one thing missing -- docs. Users need to know how to configure there HTTPD service to output one of the formats the integration expects. Can you take a look at the readme (should be in packages/apache/_dev/...) and see if you can add a blurb about the accepted formats.

andrewkroh

The doc additions look good. The package needs to be build to update the generated readme file (e.g. elastic-package build). Then there is a conflict with the changelog and version that need resolved.

packages/apache/_dev/build/docs/README.md

andrewkroh

LGTM.

This will require a review from @elastic/obs-service-integrations.

redcinelli · 2022-07-18T08:55:38Z

@elastic/obs-service-integrations Hi guys ? have you had the time to look at this PR ?

I'll fix the conflict asap

This Pr is heavily inpired by the work done in [this PR](elastic/beats#4417) It is adressing [this ER](elastic/enhancements#14402). `Grok` pattern has been updated to match logs starting with a list of IP adresses and store all those ip in `apache.access.remote_ip`. This pattern is heavily insipred by the one in the nginx integration. I also decided to fill a new field `network.forwarded_ip` as it seems to be the perfect fit.

It want the expression ot be as lean as possible. I removed the ?: non capturing group - we want to capture it - It was ignored somehow I removed the %{NOTSPACE:source.address} match : - I am not sure why it is was there in the first place

As x-forwarded-for mostly send ip, grok pattern is updated, to match only ip. To improve searchability remote_ip type is set to ip

apache.access.remote_adresses contain all IP/HOST the request rebounded of X-Forwarded-For is optional, should be tacked a the end of the logs network.forwarded_ip exist only when X-Forwarded_for is found Merge 2 similar pattern in GROK processor

Co-authored-by: Andrew Kroh <andrew.kroh@elastic.co>

packages/apache/data_stream/access/elasticsearch/ingest_pipeline/default.yml

ishleenk17 · 2022-08-19T13:35:29Z

packages/apache/data_stream/access/fields/ecs.yml

 - external: ecs
  name: message
+- external: ecs
+  name: network.forwarded_ip


We are setting the value of network.forwarded_ip in the ingest pipeline.
Do we need to use the ecs version of this here ?

I believe we have to.

If I do not I get this issue

FAILURE DETAILS: apache/access test-access-basic.log: [0] field "network.forwarded_ip" is undefined

while running this command elastic-package build && elastic-package stack up -d -v --services=elasticsearch && eval "$(elastic-package stack shellinit)" && elastic-package test pipeline --generate

Let's move it to fields.yml then as assignment is happening under ingest pipeline

done in ddaed22

We decided to undo ddaed22

packages/apache/data_stream/access/_dev/test/pipeline/test-access-basic.log

ishleenk17

Looks good !

redcinelli added the >enhancement label May 2, 2022

redcinelli marked this pull request as ready for review May 16, 2022 14:37

redcinelli requested a review from a team as a code owner May 16, 2022 14:37

andrewkroh reviewed May 16, 2022

View reviewed changes

redcinelli commented May 17, 2022

View reviewed changes

packages/apache/data_stream/access/_dev/test/pipeline/test-access-vhost.log-expected.json Outdated Show resolved Hide resolved

andrewkroh reviewed May 17, 2022

View reviewed changes

andrewkroh reviewed May 22, 2022

View reviewed changes

packages/apache/data_stream/access/elasticsearch/ingest_pipeline/default.yml Outdated Show resolved Hide resolved

andrewkroh reviewed Jun 2, 2022

View reviewed changes

andrewkroh reviewed Jun 3, 2022

View reviewed changes

packages/apache/_dev/build/docs/README.md Outdated Show resolved Hide resolved

andrewkroh approved these changes Jun 7, 2022

View reviewed changes

andrewkroh added the Team:Service-Integrations Label for the Observability Service Integrations team label Jun 15, 2022

redcinelli and others added 13 commits July 18, 2022 11:29

Update: Pr link in the changelogs

0058ab5

Update: Painless check property exist before referencing it

28ed558

Update: simplify grok expression

dbbe9a6

It want the expression ot be as lean as possible. I removed the ?: non capturing group - we want to capture it - It was ignored somehow I removed the %{NOTSPACE:source.address} match : - I am not sure why it is was there in the first place

Update: x-forwarded-for match only ip

986bd02

As x-forwarded-for mostly send ip, grok pattern is updated, to match only ip. To improve searchability remote_ip type is set to ip

Update: migrate ip adress string analysis to CIDR

5904f15

Fix: support for vhost at the begining of the log

16579c1

Update: refactor try/catch, ctx can't be null, remove redundancy

96f1e32

Update: Updated format for apache logs

ac90936

apache.access.remote_adresses contain all IP/HOST the request rebounded of X-Forwarded-For is optional, should be tacked a the end of the logs network.forwarded_ip exist only when X-Forwarded_for is found Merge 2 similar pattern in GROK processor

Update: Fill client.ip instead of source.address

3e5c9f8

Update: Add some documentation to explain supported format

73ecf83

Update packages/apache/_dev/build/docs/README.md

15d0614

Co-authored-by: Andrew Kroh <andrew.kroh@elastic.co>

Update: Build integration to generate Md doc

1047981

Update: manifest.yml

499515f

lalit-satapathy requested a review from ishleenk17 July 25, 2022 13:10

redcinelli added 2 commits July 29, 2022 10:51

Merge branch 'main' into apache/support_x_forwarded_for_headers

ded37f9

Fix: issue during merge

2c91c78

ishleenk17 reviewed Aug 19, 2022

View reviewed changes

Merge branch 'main' into apache/support_x_forwarded_for_headers

3c9d133

ishleenk17 reviewed Aug 29, 2022

View reviewed changes

ishleenk17 approved these changes Aug 29, 2022

View reviewed changes

redcinelli merged commit 176203b into elastic:main Aug 29, 2022

Conversation

redcinelli commented May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Checklist

Author's Checklist

How to test this PR locally

Related issues

Screenshots

Uh oh!

elasticmachine commented May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

Uh oh!

elasticmachine commented May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🌐 Coverage report

Uh oh!

redcinelli commented May 16, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

redcinelli commented May 17, 2022

Uh oh!

andrewkroh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

redcinelli commented May 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

andrewkroh left a comment

Choose a reason for hiding this comment

Uh oh!

andrewkroh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andrewkroh left a comment

Choose a reason for hiding this comment

Uh oh!

redcinelli commented Jul 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ishleenk17 Aug 19, 2022

Choose a reason for hiding this comment

Uh oh!

redcinelli Aug 22, 2022

Choose a reason for hiding this comment

Uh oh!

ishleenk17 Aug 23, 2022

Choose a reason for hiding this comment

Uh oh!

redcinelli Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

redcinelli Aug 26, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ishleenk17 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

redcinelli commented May 2, 2022 •

edited

Loading

elasticmachine commented May 2, 2022 •

edited

Loading

elasticmachine commented May 2, 2022 •

edited

Loading

redcinelli commented May 18, 2022 •

edited

Loading

redcinelli commented Jul 18, 2022 •

edited

Loading

redcinelli Aug 23, 2022 •

edited

Loading