Overall response timeout. #1450
Comments
florimondmanca
added
concurrency
|
@Granitosaurus Hi! This is an interesting topic. To be honest, I'm not sure yet if/how we would be adding security knobs like these. Security folks tend to be paranoid 😄 and I'm not sure if they wouldn't just like to put things like slow lorry defense measures in place themselves, rather than relying on the underlying HTTP lib to do it for them. There's also a case to be made to include some minimal support built-in, but then we need to discuss opt-in vs opt-out; if opt-out then what is the default; etc. Luckily, HTTPX already enables people to implement these measures themselves without having to do advanced concurrency. I was pinged on the From my POV, the best bet for folks might be to write a custom transport that wraps the stream and checks against slow sending of bytes. Something like this: import time
from typing import Iterator
import httpcore
import httpx
class TooSlowError(Exception):
pass
class SlowGuardTransport(httpcore.SyncHTTPTransport):
def __init__(self, transport: httpcore.SyncHTTPTransport, min_rate: str) -> None:
self._transport = transport
value, unit = min_rate.split("/")
self._min_rate = float(value) / {"second": 1, "minute": 60}[unit]
def _wrap(self, stream: Iterator[bytes]) -> Iterator[bytes]:
last_chunk_date = time.time()
for chunk in stream:
# Compute current recv rate.
# TODO: Maybe use a rolling average to reduce false positives.
now = time.time()
elapsed = now - last_chunk_date
rate = len(chunk) / elapsed
if rate <= self._min_rate:
raise TooSlowError(f"Server is sending data too slowly: {rate:.2f} < {self._min_rate:.2f}")
last_chunk_date = now
yield chunk
def request(self, *args, **kwargs):
status_code, headers, stream, ext = self._transport.request(*args, **kwargs)
return status_code, headers, self._wrap(stream), ext
transport = httpcore.SyncConnectionPool() # See docs for kwargs, or wait for upcoming `httpx.HTTPTransport()`.
transport = SlowGuardTransport(transport, min_rate="10/second")
with httpx.Client(transport=transport) as client:
# 20/second: OK
response = client.get("http://httpbin.org/drip?duration=5&numbytes=100&code=200")
print(response)
# 1/second: fails with `TooSlowError`
# 'TooSlowError: Server is sending data too slowly: 0.93 < 10.00'
response = client.get("http://httpbin.org/drip?duration=5&numbytes=5&code=200")I'm not sure yet whether something like this would benefit from being built-in, with what API, or if it's okay for folks to just copy-paste and tweak a bit of this code using custom transports. (The Transport API is here exactly for enabling this kind of high-precision behaviors.) |
florimondmanca
added
security
|
Heya, thanks for raising this. I'd potentially be a bit cautious about labelling this under "attacks"/"security", since controlling the server and attacking the client isn't a very typical scenario, but yes I do think there's scope for a few more resource limits, which I've so far only noted in passing. pause - I'm going to follow up on this more in a moment, but first lunch is up... |
florimondmanca
removed
the
security
|
@tomchristie As far as security scenarios go, slowloris seems like a typical one to take into account to me. Eg one is building a service that uses HTTPX to interact with some other server-side service that you can't trust to not be compromised and controlled by malicious agent. Clearly that's entering paranoia land, but one thing I've learnt is dealing with security is a lot about dealing with paranoia. 😆 Anyway, I've dropped the "security" label for now as I've realized it might come off as "here's a security breach we found in HTTPX" whereas that's not what this issue is about (which might be what you were getting at). |
|
It's just that slowlorris makes sense as a client-side attack on the server, allowing attackers to DDOS a server. Whereas in the converse case, where the server has been owned by the attacker, it's an odd scenario to envisage an "attack" that is "let's make the clients using this service really slow". Maybe that's a thing that's happened(?), but I've never heard of it. You'd more typically see simply this kind of issue on the client side framed as "resource limits to ensure graceful degradation vs. overloaded servers". Anyways, I think it's valid either ways around, and I think a useful guideline for considering resource limits is to think about it in the same way as services like Heroku treat resource limiting. The cases that seem like likely candidates to me might be:
|
|
Yeah I did put "attack" in quotes because it's not really an attack but it serves a very similar function. I think those 3 proposals by @tomchristie would be more than enough to deal with this! |
|
I think about this problem a lot. I've written a lot of code which accepts an untrusted URL from a user and attempts to fetch that URL - a few examples:
For all of these I really appreciate being able to use an HTTP client that makes it easy to "safely" consume a malicious URL. I want to be able to set aggressive timeouts AND limit the number of bytes retrieved too (see #1392 for a previous question I've had around limiting the number of bytes). The attacks I'm worried about here are deliberate denial-of-service attacks - users providing malicious URLs that exhaust my server resources. The three limits Tom suggests above are exactly what I'm after. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
We need to reframe this issue more specifically around the types of resource limiting that we'd still like to add, but I think it's worth us continuing to track this. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
tomchristie
changed the title
|
Okay, I've retitled this issue so that it's just about addressing the overall request timeout. In the comment at #1450 I also mentioned some other resource limiting that we might consider, but let's treat any conversations about that separately. For this one particular case, I can see us adding the following... timeout = httpx.Timeout(response=60.0)
client = httpx.Client(timeout=timeout)I'd expect that we'd want to implement the timeout behaviour at the Would we want a response timeout to be on or off by default? |
tomchristie
added
enhancement
|
I like this proposal, it's good to be able to limit the resources you can expend on a task (no so much for security as for coping with the amazing variety of brokenness you encounter on the Internet). My sense is that the response timeout should probably be off by default as it's really hard to guess what a good timeout would be, especially for HTTP. Someone might be getting some massive document over a slow connection, and any "default to on" behavior might break existing code. I think giving people a mechanism they can activate and specify values appropriate to their situation is enough. One default-related thing I do think is worth discussing is what is the meaning of
You could argue that for backwards compatibility that this means "10 second idle timeout for all operations, no overall response timeout", or you could argue "least surprise" and say it means a 10 second value for all timeouts, and thus a max block time of 10 seconds. I'm not sure which way to go on this one, as I don't know how much code out there is expecting the timeout to be only an idle timeout because they read the documentation carefully, and how much code out there made the same mistake we did in dnspython by assuming it was an overall request timeout :). |
|
One other thing: I'm definitely not arguing for any removal of the existing idle timeouts. For most things I've written involving stream connections, I've ended up with both an idle timeout and a total lifetime, because they both have useful purposes. |
|
Oh that's a really good point that I'd not considered... I'm not sure either. From an API perspective I'd assume it to include the response timeout yup. |
|
Hello everyone! |
That'd be a really neat feature, yep. I'd also like us to have this. Couple of different things here...
Here's our recently updated not quite live yet timeout docs. Currently we support # This looks like a 'total' timeout, but it's actually a network timeout.
httpx.get('http://example.com/api/v1/example', timeout=10.0)If we switched to enforcing an explicit style everywhere, then we could ensure it's always more clear. Eg... timeout = httpx.Timeout(network=10.0)
httpx.get('http://example.com/api/v1/example', timeout=timeout)timeout = httpx.Timeout(network=10.0, total=60.0)
httpx.get('http://example.com/api/v1/example', timeout=timeout)timeout = httpx.Timeout(total=60.0)
httpx.get('http://example.com/api/v1/example', timeout=timeout) |
|
Should the overall timeout include the pool timeout? If yes the implementation is easy, at least for the async client; just wrap I took a stab at implementing this but got stuck because there is not a single method or code block to guard with |
|
Thanks for mentioning #!/usr/bin/env python3
import asyncio
import httpx
import time
async def download():
t = time.perf_counter()
try:
async with httpx.AsyncClient(timeout=1.0) as client:
response = await asyncio.wait_for(client.get('https://httpbin.org/delay/3'), timeout=1.0)
print(response.content.decode())
except httpx.TimeoutException as e:
print(f'HTTPX Timeout: {e}')
except asyncio.TimeoutError as e:
print(f'Asyncio Timeout: {e}')
print(f'{time.perf_counter() - t:.3f} s')
asyncio.run(download()) |
|
@falkoschindler you don't need |
|
@gsakkis You're right, the other URL "http://httpbin.org/drip" is even better for testing the total timeout. But even with "https://httpbin.org/delay/3" the normal httpx timeout takes around 1.4s instead of 1.0s, while |
|
Yep. Using async primitives for timeouts is the sensible approach for the I'd suggest that trio has the neater design here, because it gets that timeouts make sense as context blocks. Implementing total-request-timeouts would mostly be beneficial for the sync case, since there's no cancellation semantics available to us with threaded code.
I think the timeout would need to be implemented as two separate blocks...
Also useful reading: https://stackoverflow.com/questions/21965484/timeout-for-python-requests-get-entire-response |
|
Well said Tom! And yeah, it's indeed much easier with trio as you just scope your whole task under a cancelation timeout and don't worry about the details, and you don't have to have each individual thing the blocks figure out how much time it has left. It's the sync case for httpx that's the open ticket for me. The idea you propose would work for my purposes. |
|
Python 3.11 comes with I have used it to prevent servers doing a "reverse slowloris" (unintentionally I assume) in a webhook delivery system at work, where a few slow servers would respond slower than the timeout (300+ vs 5 seconds), and would eventually slow the system so much down it was unable to keep up with the stream of new events. |
|
A total timeout would be a great addition to the timeout class, particularly for the sync api. Besides I would like to carefully suggest that an additional timeout check for reading each chunk might be a good idea in iter_raw(). |
|
I found that httpx will be easily hang even I set the timeout parameter... After debugging, I found that the program will be paused at Stack trace: |
|
I think it's should be resolved in encode/httpcore#936 |
@gsakkis I think the library can't provide an API to cover all the use cases users may have. However, HTTPX has a great design that allows you to easily customize parts of your client by simply overriding the necessary components. If you want to set a timeout for all read operations, you can simply inherit from the Response class and override the appropriate method, like so: import typing
import httpx
import asyncio
import time
class ResponseWithTimeout(httpx.Response):
def __init__(self, *args, **kwargs):
self.timeout = kwargs.pop("read_total", None)
super().__init__(*args, **kwargs)
async def aiter_raw(
self, chunk_size: typing.Optional[int] = None
) -> typing.AsyncIterator[bytes]:
async_iter = super().aiter_raw(chunk_size)
while True:
t1 = time.monotonic()
try:
chunk = await asyncio.wait_for(async_iter.__anext__(), self.timeout)
except StopAsyncIteration:
break
t2 = time.monotonic()
elapsed = t2 - t1
if self.timeout is not None:
self.timeout -= elapsed
if self.timeout <= 0:
raise asyncio.TimeoutError()
yield chunkWhen you have your own import httpx
import asyncio
from httpx._transports.default import AsyncResponseStream
class AsyncTransportWithTimeout(httpx.AsyncBaseTransport):
def __init__(self, next_transport: httpx.AsyncHTTPTransport):
self.next_transport = next_transport
async def handle_async_request(self, request: httpx.Request) -> httpx.Response:
response = await self.next_transport.handle_async_request(request)
return ResponseWithTimeout(
status_code=response.status_code,
headers=response.headers,
stream=AsyncResponseStream(response.stream),
extensions=response.extensions,
read_total=request.extensions.get("timeout", {}).get("read_total", None),
)Now you can use that transport in cases where you want control over the total time the read operation takes, like this: async def main():
async with httpx.AsyncClient(
transport=AsyncTransportWithTimeout(httpx.AsyncHTTPTransport())
) as client:
response = await client.get("https://example.org", extensions={"timeout": {"read_total": 2}}) # 2 second total read
print(response.text)
asyncio.run(main())Also note that I've written the transport to accept |
@karpetrosyan apologies for the belated response and many thanks for the elaborate answer! Sadly even if my use case is possible to implement (it's not clear how the sample code above can be extended to enforce a |
|
@gsakkis Appreciate the suggestion, tho we won't support that, no. Perhaps the trace functionality is useful here? |
master.Currently both async and sync client requests are succeptible to
low-and-slow"attacks" unless explicitly handled. In other words when request is being streamed as long as 1 byte every 2 seconds (or aprox.) is being generated the connection will not close or timeout with none of the current available settings.It's possible for the server to serve single html page almost idefinitely and hang the client thread.
Maybe
httpxshould introduce some optional or even default handlers for this? Maybe it's already possible to hook something like this in easily?To reproduce
The above code snippet will take 30+ seconds to complete (though this could be practically infinity) disregarding any timeout settings. The only way to avoid this is to explitictly wrap everything in either
asyncio.wait_for()for async code and for sync code seems to be much more complicated (?).The text was updated successfully, but these errors were encountered: