Disconnection when ICAP headers given as bytes

[tomers]

I experienced various issues due to missing space, caused by an offending commit #88f6dda00e4e49faa346db2e279b0cc8b05360d3.

88f6dda.

[netom]

Hi @tomers, header formatting should now be fixed by 1dd32c8. Thank you and sorry for the mishap.

[tomers]

Thanks so much for the very fast response!
Unfortunately, the missing space was not the culprit of the issue. I still get disconnections. I will investigate this further.

172.18.0.17 - - [01/Feb/2022 12:29:44] "b'REQMOD icap://icap-verifier.proxy:14591/request ICAP/1.0'" b'200' b'-'
----------------------------------------
Exception happened during processing of request from ('172.18.0.17', 37596)
Traceback (most recent call last):
  File "/usr/local/lib/python3.8/socketserver.py", line 683, in process_request_thread
    self.finish_request(request, client_address)
  File "/usr/local/lib/python3.8/socketserver.py", line 360, in finish_request
    self.RequestHandlerClass(request, client_address, self)
  File "/usr/local/lib/python3.8/socketserver.py", line 747, in __init__
    self.handle()
  File "/usr/local/lib/python3.8/site-packages/pyicap.py", line 470, in handle
    self.handle_one_request()
  File "/usr/local/lib/python3.8/site-packages/pyicap.py", line 504, in handle_one_request
    self.raw_requestline = self.rfile.readline(65537)
  File "/usr/local/lib/python3.8/socket.py", line 669, in readinto
    return self._sock.recv_into(b)
ConnectionResetError: [Errno 104] Connection reset by peer
----------------------------------------
172.18.0.17 - - [01/Feb/2022 12:30:15] "b'OPTIONS icap://icap-verifier.proxy:14591/request ICAP/1.0'" b'200' b'-'

If I may ask what commit 88f6dda tried to achieve? Does that change means icap_headers are expected to contain string values instead of bytes? If so, why aren't they also converted when they are matched against connection and close/keep-alive?

[tomers]

@netom , I found the issue. It is with conversions of headers which are already in bytes format.
Earlier code assumed icap_headers contained bytes, thus it converted that dict's key and values to byte strings.
However, you probably wanted to use string there, so you converted the resulting string to bytes. This is wrong, because it breaks existing code base which define ICAP headers as byte strings:

>>> k = b'connection'
>>> v = b'close'
>>> icap_header_str = b''
>>> icap_header_str += "{}: {}\r\n".format(k, v).encode()
>>> icap_header_str
b"b'connection': b'close'\r\n"

If you want to add support for ICAP headers given as strings, you should keep backward-compatibility, and retain support for headers given as bytes. You should convert to bytes conditionally, so as to not convert byte string to bytes.

[netom]

Eh, this just shows how badly this project needs proper automated tests :)

But of course I'd like to clean up this particular issue first. If you have any suggestions or - God forbid - a pull request you're thinking about please don't hold yourself back :)

[sstamoulis]

Opened PR #42 that fixes the issue for me.

[Mirraz]

@netom Can PR suggested by @sstamoulis be merged anytime soon?