haproxytech / haproxy-lua-http

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

Fix multipart/form-data boundary parsing #12

Closed lastmikoi closed 3 years ago

lastmikoi commented 3 years ago

I've spent several hours troubleshooting an issue where I couldn't get a simple plain python3 script to emulate a cURL request against haproxy-lua-acme, powered by haproxy-lua-http.

As it turns out, cURL uses boundaries like this:

----------------------e45e387473048941

while python's httplib uses boundaries like this:

6bc4c0b431e13fda6748fe624a1002c9

However, those boundaries are all prefixed by --, as per https://tools.ietf.org/html/rfc7578#section-4.1, to separate parts. On cURL-style boundaries, M.request.parse_multipart does fine, but it chokes on python's dash-less boundaries and mixes up the boundary prefix and the boundary itself.

This PR aims to fix that. I've applied that patch to my httpd.lua and I've been able to do the same request using cURL as python3.

I am no Lua developper by any stretch of the imagination so do not hesitate to point out issues, even if the fix is small.

anezirovic commented 3 years ago

Hello there, nice catch!

In theory it should not matter since we only need to match that entire "boundary" appears[1] in the separating line, however, the code handling it is probably to fragile so we need to refactor it anyway.

Is it too much to ask you for short steps for reproducing the issue. While your patch probably solves the issue, we might have other problems, so I'll investigate further.

[1] Per rfc2046 NOTE TO IMPLEMENTORS: Boundary string comparisons must compare the boundary value with the beginning of each candidate line. An exact match of the entire candidate line is not required; it is sufficient that the boundary appear in its entirety following the CRLF.

lastmikoi commented 3 years ago

I sadly haven't managed to simplify much the steps for reproducing, I've compared the two following behaviors:

$ curl -XPOST http://127.0.0.1:9011/acme/order -F 'account_key=@account.key.pem' -F 'domain_key=@domain_key.pem' -F 'domain=test.example.com'  -v

and

import requests
response = requests.post(
    'http://127.0.0.1:9011/acme/order',
    files={
        'account_key': ('account.key.pem', open('account.key.pem', 'rb'), 'application/octet-stream'),
        'domain_key': ('domain.key.pem', open('domain.key.pem', 'rb'), 'application/octet-stream'),
        'domain': (None, 'test.example.com', None),
    },
)
print(response.__dict__)

against an instance of the latest haproxy-lua-acme service on CentOS7 & haproxy 2.2 (package provided by IUS). Due to my limited knowledge of Lua and its haproxy integration, I'm afraid I'm not able to go one level deeper and test against a clean service without any lua-acme coupling.

Note: both account.key.pem and domain.key.pem were generated using openssl genpkey, but any non-empty file should do

anezirovic commented 3 years ago

That's good enough, you're using requests on the python side, which is the thing I need to know.

I'll be busy to handle it this week, probably early next week. I guess your use case is covered for now, but we'll keep this issue open until the fix is made (either your pull request, something more generic, or combination of the two).

Thanks!

anezirovic commented 3 years ago

This is fixed in the latest master, it got slightly complicated, but your PR (correctly checking for double dash) is an important part of it. Thanks again, hope it works now for python clients.