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

doc/userguide: more eve http upgrade notes #9166

Closed
wants to merge 1 commit into from

Conversation

jasonish
Copy link
Member

@jasonish jasonish commented Jul 6, 2023

Add more information with a examples of how the changes to EVE HTTP logging may affect users.

Add more information with a examples of how the changes to EVE HTTP
logging may affect users.
Comment on lines +100 to +107
"http": {
"hostname": "suricata.io",
"http_method": "GET",
"protocol": "HTTP/1/1",
"response_headers": [
{ "name": "Server", "value": "nginx" }
]
}
Copy link
Member Author

@jasonish jasonish Jul 6, 2023

Choose a reason for hiding this comment

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

To me this reinforces why we need to do this and I don't see a sane way to maintain compatibility. Lets take an arbitrary header name "Foo" that could exist in the request and response.

If we do:

"http": {
  "foo": "value",
}

was the server or response?

Further, what should we do about a header that appears multiple times. We then end up with:

"http": {
  "foo": ["value"],
}

(Elastic does fine with those arrays)

This may lead to one wanting:

"http": {
  "request_headers": {
    "foo": ["one", "two", "three"]
  }
}

but this conflicts with our existing request_headers and response_headers that are already arrays. We also can't make this objects for dump-all-headers as Elastic doesn't like arbitrary field growth, or certain chars in the keys.

As I mentioned over in https://redmine.openinfosecfoundation.org/issues/6173, we could make sure the "normalized" header names allowed in custom all have distinct names that don't overwrite any existing field name.. We should also prefix them with request_ and response_.

So for the custom: [Server] example I give, this would mean logging like:

"http": {
  "request_server": "nginx",
  ...
}

(could also be "request_server": ["nginx"] to handle multiple occurrences of a header.

This does feel like a bit of a hack, and will still break many reports etc. that will need to be updated for the new field names. But perhaps a little easier to adapt to than the array approach for 7.

Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure libhtp won't allow key duplication in a single direction:

User-Agent: abc
User-Agent: def

will end up as

User-Agent: abc, def

Where abc, def is a single string.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is rather Suricata doing a join(",") on libhtp's headers table

Copy link
Member Author

Choose a reason for hiding this comment

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

So duplicate headers would be handled differently between http 1 and 2?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question, I think http2 has the same join(",") normalization at some point

But that needs to be tested ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

So looks like detection works on normalized with join(",") but logging logs only the last value 😨

@catenacyber
Copy link
Contributor

Is this about @regit 's ticket ?

@catenacyber catenacyber added the typo/doc update No code change : only doc or typo fixes label Jul 7, 2023
@jasonish
Copy link
Member Author

jasonish commented Jul 7, 2023

Is this about @regit 's ticket ?

Yeah, but add more info just to make possible breakage more clear.

@jasonish
Copy link
Member Author

jasonish commented Jul 7, 2023

Replaced by #9176.

@jasonish jasonish closed this Jul 7, 2023
@jasonish jasonish deleted the eve-http-upgrade-notes/v1 branch March 14, 2024 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
typo/doc update No code change : only doc or typo fixes
3 participants