haproxytech / haproxy-lua-http

Simple Lua HTTP helper && client for use with HAProxy.
Apache License 2.0
56 stars 23 forks source link

Incorrect handling of HEAD requests with Content-Length > 0 #4

Closed TimWolla closed 4 years ago

TimWolla commented 4 years ago

Example configuration:

global
    lua-prepend-path /redacted/?.lua
    lua-load test.lua

frontend test_fe
    mode http
    bind *:8080
    http-request lua.test
    use_backend test_be

backend test_be
    mode http
    server example example.com:80
local http = require('http')

core.register_action("test", {"http-req"},function ()
    local r, err = http.head {
        url = "http://93.184.216.34/"
    }

    if err ~= nil then
        core.Info("err: " .. err)
    else
        core.Info(r.content)
    end
end)

Now making a request to localhost:8080 will log the following:

[info] 053/235929 (28384) : err: http.head: Receive error (content): connection closed.

RFC 7230#3.3.2 says:

A server MAY send a Content-Length header field in a response to a HEAD request (Section 4.3.2 of [RFC7231]); a server MUST NOT send Content-Length in such a response unless its field-value equals the decimal number of octets that would have been sent in the payload body of a response if the same request had used the GET method.

and that's what the example.com server is doing: It's sending a non-zero Content-Length in response to the HEAD request, leading to haproxy-lua-http falling over.

anezirovic commented 4 years ago

Hey Tim, I'll investigate this more thoroughly, but from the quick glance of the code, we don't specifically handle responses to HEAD requests, and this specific error is returned by underlying socket:receive("*a")

Aside from that, we do need some light testing harness for this library, maybe something like httpbin.org or similar.

TimWolla commented 4 years ago

Hey Tim, I'll investigate this more thoroughly, but from the quick glance of the code, we don't specifically handle responses to HEAD requests, and this specific error is returned by underlying socket:receive("*a")

Yes, that's what I concluded as well. I didn't create a PR, because I did have the time to investigate the proper solution.

Aside from that, we do need some light testing harness for this library, maybe something like httpbin.org or similar.

I suggest VTest as with HAProxy itself. I plan to use that for my haproxy-auth-request as well, when I get around to it.

TimWolla commented 4 years ago

@anezirovic You might be interested in this: https://github.com/TimWolla/haproxy-auth-request/blob/master/.github/workflows/vtest.yml

Maybe the HAProxy installation and VTest installation steps could be spun out into an includeable action within either the haproxytech or the haproxy organization (I would prefer the latter). Documentation is here: https://help.github.com/en/actions/building-actions

TimWolla commented 4 years ago

Do you have an update to share / did you get around to working on this, yet?

anezirovic commented 4 years ago

Hey Tim, the issue was rather trivial, forgot to push the fix long time ago. We just need to be sure not to consume response body (if any) for HEAD requests. request.content attribute will be explicitly set to nil as well (so if you use core.Info(), please guard it with tostring()).

I've never used HEAD in my haproxy Lua code, I feel embarrassed.

If this works for you, we will close the issue.

TimWolla commented 4 years ago

so if you use core.Info(), please guard it with tostring()

I don't. This was just my my simple reproducer. In fact I don't care about the body at all (that's why I've been using a HEAD request).

If this works for you, we will close the issue.

Yes, this is looking good on the system where I've been seeing the issue. You can close this (and label as “fixed” or whatever).