client.py does not handle exceptions from httplib

[gcoxmoz]

CentOS7 box with python2.7 (I KNOW, I'm working on it RIGHT NOW).

On VERY rare occasions, my code gets an exception passed up through your library:

  File "/usr/lib/python2.7/site-packages/duo_client/auth.py", line 29, in check
    return self.json_api_call('GET', '/auth/v2/check', {})
  File "/usr/lib/python2.7/site-packages/duo_client/client.py", line 382, in json_api_call
    (response, data) = self.api_call(method, path, params)
  File "/usr/lib/python2.7/site-packages/duo_client/client.py", line 269, in api_call
    return self._make_request(method, uri, body, encoded_headers)
  File "/usr/lib/python2.7/site-packages/duo_client/client.py", line 338, in _make_request
    conn, method, uri, body, headers)
  File "/usr/lib/python2.7/site-packages/duo_client/client.py", line 351, in _attempt_single_request
    response = conn.getresponse()
  File "/usr/lib64/python2.7/httplib.py", line 1128, in getresponse
    response.begin()
  File "/usr/lib64/python2.7/httplib.py", line 453, in begin
    version, status, reason = self._read_status()
  File "/usr/lib64/python2.7/httplib.py", line 417, in _read_status
    raise BadStatusLine(line)
BadStatusLine: ''

I'd say, in 6 months, I've gotten about 4 of these (out of thousands of positive responses). I imagine it's a web burp in production.

This is mostly a question of "is this YOUR problem or MINE?" I see there's very few uses of try in the library, and none around conn.getresponse(), so I'm unsure if you consider it a design feature that your library is propagating httplib exceptions up to users' code, or if I've hit a super rare edge that you would handle, but nobody ran across in development.

Thanks!

[AaronAtDuo]

@gcoxmoz Thanks for bringing this up. Overall, I would say the lack of try blocks is our intended design, since as a pure client, there's nothing particularly useful we can do when a request goes wrong. Is there something you have in mind that you'd like to see us do with this type of error?

[gcoxmoz]

https://duo.com/docs/authapi#/check basically says an API should expect 200 or 401 on a check call. Since you didn't get that, the exception bubbled all the way through your code, to my code, as a confusing "BadStatusLine".

My thought here is that, since I'm not writing http-level code / you're providing the python API and you might change your http library underpinnings, it's very difficult for me to do something better in my own scripting than 'catch all exceptions', without doing a vast amount of study and following your library down through the next layer, now and in the future. That is, it feels like it's weird for you to ever raise 'BadStatusLine' (ok, technically "allow to raise through you"), and I don't know what else you might raise from that library (or others), and if it'll ever change.

Ultimately the question, I think, is "should duo_client_python encapsulate its own sublibraries?" Both from laziness and neatness, I think so. The API docs are super short and only show cases of 201/400, and obviously the web can fail you on occasion, which means the libraries can fail us. But I'm two levels removed from httplib here.

I think the elegant approach here would be to catch 'BadStatusLine' in _attempt_single_request (and other errors you might get in testing/bug reports over time) and re-wrap / re-raise them as a duo_client_python defined Exception, so that I have a known Exception to catch. Basically an exception that says "the duo_client API failed to get a valid response from the REST API, you need to handle it." That way I know what exception type to capture (because it's defined by you), and you aren't leaking someone else's exceptions.

[AaronAtDuo]

@gcoxmoz Thanks for your thoughts on this. We use this library internally at Duo, so I'll run your ideas past some of our internal folks and see what they think.