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

Packetbeat HTTP parsing fails on empty status phrase #6176

Closed
adriansr opened this issue Jan 25, 2018 · 6 comments

Comments

@adriansr
Copy link
Member

commented Jan 25, 2018

An HTTP response whose status phrase is empty causes the following error:

2018-01-25T10:50:49Z WARN Failed to understand HTTP response status: 302

For example :

HTTP/1.1 302
Location: http://172.17.0.12:8080/resources/userinfo
Content-Length: 0
Date: Thu, 25 Jan 2018 10:50:48 GMT
Connection: close

The response parser in https://github.com/elastic/beats/blob/master/packetbeat/protos/http/http_parser.go#L224 is expecting the status-phrase to be non-empty.

From this question on discuss.

@mbudnick

This comment has been minimized.

Copy link

commented Jan 25, 2018

The warning tells status:302 so the status seems to be parsed?

See my full log file for more examples: packetbeat.log

Packetbeat and tomcat running together in a docker-container. Maybe there is a problem with docker network because of logged messages "Ignore empty non-FIN packet"?

@adriansr

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2018

@mbudnick It is failing because it expects something else after the status code. Your server status response is in the form:

HTTP/1.1 200
HTTP/1.1 302

When packetbeat is expecting a non-empty status phrase:

HTTP/1.1 200 OK
HTTP/1.1 302 Found

@jreyeshdez

This comment has been minimized.

Copy link
Contributor

commented Jan 26, 2018

Hello @adriansr

I was checking and testing this issue and about to commit it to my fork but it seemed to obvious to me where the issue might be that I'd rather leave the question in this issue before actually make a PR.

In the function parseResponseStatus I would only do the following:
return uint16(statusCode), []byte(http.StatusText(statusCode)), nil
And delete the phrase := s[p+1:] as well as the if-statement check

Let me know what you think about this change.

@adriansr

This comment has been minimized.

Copy link
Member Author

commented Jan 27, 2018

Hi @jreyeshdez

I think it's better to just return the phrase, even if empty, instead of an error.
Also, it will be cool if you can add a test case in http_test.go

@xkoomy

This comment has been minimized.

Copy link

commented Mar 18, 2018

Hi @adriansr
I see an implementation HTTP 1.1 where in status line is no space after status code (i.e. HTTP/1.1 200\r\n). In this case parseResponseStatus still doesn't work even after the patch. We can debate that HTTP standard says the space is mandatory but life looks different. For instance I got this case now during my job and have to workaround it.
Could you consider change parseResponseStatus to support such status lines too?

xkoomy

adriansr added a commit to adriansr/beats that referenced this issue Mar 22, 2018
http: support broken status line
User reports some HTTP servers may respond with a broken status line that's
missing a space between the status code and the optional status phrase. HTTP
clients tested already support this behavior. This patch adjusts the http
parser so that this deviation from the standard is accepted.

Fixes elastic#6176
@adriansr

This comment has been minimized.

Copy link
Member Author

commented Mar 22, 2018

@xkoomy I've submitted a patch for the issue: #6631

ruflin added a commit that referenced this issue Mar 26, 2018
http: support broken status line (#6631)
User reports some HTTP servers may respond with a broken status line that's
missing a space between the status code and the optional status phrase. HTTP
clients tested already support this behavior. This patch adjusts the http
parser so that this deviation from the standard is accepted.

Fixes #6176
adriansr added a commit to adriansr/beats that referenced this issue Apr 3, 2018
http: support broken status line (elastic#6631)
User reports some HTTP servers may respond with a broken status line that's
missing a space between the status code and the optional status phrase. HTTP
clients tested already support this behavior. This patch adjusts the http
parser so that this deviation from the standard is accepted.

Fixes elastic#6176
adriansr added a commit to adriansr/beats that referenced this issue Apr 3, 2018
http: support broken status line (elastic#6631)
User reports some HTTP servers may respond with a broken status line that's
missing a space between the status code and the optional status phrase. HTTP
clients tested already support this behavior. This patch adjusts the http
parser so that this deviation from the standard is accepted.

Fixes elastic#6176
andrewkroh added a commit that referenced this issue Apr 3, 2018
http: support broken status line (#6631) (#6740)
User reports some HTTP servers may respond with a broken status line that's
missing a space between the status code and the optional status phrase. HTTP
clients tested already support this behavior. This patch adjusts the http
parser so that this deviation from the standard is accepted.

Fixes #6176
adriansr added a commit to adriansr/beats that referenced this issue Apr 5, 2018
http: support broken status line (elastic#6631)
User reports some HTTP servers may respond with a broken status line that's
missing a space between the status code and the optional status phrase. HTTP
clients tested already support this behavior. This patch adjusts the http
parser so that this deviation from the standard is accepted.

Fixes elastic#6176
adriansr added a commit to adriansr/beats that referenced this issue Apr 6, 2018
http: support broken status line (elastic#6631)
User reports some HTTP servers may respond with a broken status line that's
missing a space between the status code and the optional status phrase. HTTP
clients tested already support this behavior. This patch adjusts the http
parser so that this deviation from the standard is accepted.

Fixes elastic#6176
andrewkroh added a commit that referenced this issue Apr 6, 2018
http: support broken status line (#6631)
User reports some HTTP servers may respond with a broken status line that's
missing a space between the status code and the optional status phrase. HTTP
clients tested already support this behavior. This patch adjusts the http
parser so that this deviation from the standard is accepted.

Fixes #6176
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.