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

log: rename http.xff to http.xff_header #7051

Closed

Conversation

catenacyber
Copy link
Contributor

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/4860
but also https://redmine.openinfosecfoundation.org/issues/1369

Describe changes:

  • http : rename http.xff to http.xff_header as it does not have the same meaning as .xff

suricata-verify-pr: 756
OISF/suricata-verify#756

Replaces #6904 with renaming http.xff to http.xff_header

Ticket: 4860

So as to differentiate between .xff which is just one IP,
depending on the configuration first or last, and
.http.xff_header which is the complete header value
@codecov
Copy link

codecov bot commented Feb 22, 2022

Codecov Report

Merging #7051 (01d3df9) into master (b1c0936) will decrease coverage by 2.41%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #7051      +/-   ##
==========================================
- Coverage   77.88%   75.46%   -2.42%     
==========================================
  Files         628      628              
  Lines      185373   185242     -131     
==========================================
- Hits       144374   139791    -4583     
- Misses      40999    45451    +4452     
Flag Coverage Δ
fuzzcorpus 54.48% <100.00%> (-4.02%) ⬇️
suricata-verify 54.44% <ø> (-0.11%) ⬇️
unittests 63.17% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 6389

@jasonish
Copy link
Member

Can you provide samples of the output? As mentioned here, #6904 (comment), I don't think I'm in favour of adding a new field. I'd rather see xff gone at the root, and xff only logged in the http object like here: #6904 (comment) regardless of the code path that caused it to be logged.

This lets us remove a xff at the root, which I think is a better breaking change that renaming http.xff.

@catenacyber
Copy link
Contributor Author

Its a breaking change for some I suppose, but I think its a nice cleanup to get the xff out of the root, and just have it in the http object. But include all addresses.

Do you know where this feature comes from ?
This feature is the xff.deployment configuration option...
I see commit 68f43ff

Maybe I can remove xff from the root, and have another value for the config option xff.deployment being all in addition to reverse and forward

@jasonish
Copy link
Member

Its a breaking change for some I suppose, but I think its a nice cleanup to get the xff out of the root, and just have it in the http object. But include all addresses.

Do you know where this feature comes from ? This feature is the xff.deployment configuration option... I see commit 68f43ff

Maybe I can remove xff from the root, and have another value for the config option xff.deployment being all in addition to reverse and forward

Ok, I see whats going on.

The xff at the root is either the first or last IP in the list based on the setting of mode. I didn't realize that, and perhaps it does have a valid use being different from http.xff. So maybe just move it from root into alert, so its a propertry of the alert object and be done with it?

@catenacyber
Copy link
Contributor Author

So maybe just move it from root into alert, so its a propertry of the alert object and be done with it?

Ok will do

@catenacyber
Copy link
Contributor Author

Replaced by #7148

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

Successfully merging this pull request may close these issues.

3 participants