Added a configuration flag for enable request task handler cancelling when client connection closing.

[mosquito]

Related to #6719 #6727. Added a configuration flag for enable request task handler cancelling when client connection closing.

After changes in version 3.8.3, there is no longer any way to enable this behaviour. In our services, we want to handle protocol-level errors, for example for canceling the execution of a heavy query in the DBMS if the user's connection is broken.

Now I created this PR in order to discuss my solution, of course if I did everything well I will add tests changelog, etc.

I guess this breakdown can be solved using the configuration flag that is passed to the Server instance.

Of course AppRunner and SiteRunner can pass this through **kwargs too.

Related issue number #6719

Checklist

• I think the code is well written
• Unit tests for the changes exist
• Documentation reflects the changes
• If you provide code modification, please add yourself to CONTRIBUTORS.txt
  • The format is <Name> <Surname>.
  • Please keep alphabetical order, the file is sorted by names.
• Add a new news fragment into the CHANGES folder
  • name it <issue_id>.<type> for example (588.bugfix)
  • if you don't have an issue_id change it to the pr id after creating the pr
  • ensure type is one of the following:
    • .feature: Signifying a new feature.
    • .bugfix: Signifying a bug fix.
    • .doc: Signifying a documentation improvement.
    • .removal: Signifying a deprecation or removal of public API.
    • .misc: A ticket has been closed, but it is not of interest to users.
  • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

[tzoiker]

I'd rename cancel_when_connection_lost -> cancel_handler_on_connection_lost

[Dreamsorcerer]

Seems simple enough. First though, can we get some tests to validate this behaviour? Then we can be sure not to break it again in future.

Second, how do I use this as a user? Can we pass the argument to run_app()? If so, please also update the docs to include the new argument.

[mosquito]

@Dreamsorcerer I started adding information to the documentation, and found this example this made me think that the current default behaviour is unsafe, and that's what I mean.

I imagined a simpler example that worked well before the change where tasks weren't cancelled, and didn't work so well now.

from aioredis import Redis
from aiohttp import web


async def publish(request: web.Request) -> web.Response:
    redis = await Redis(host='localhost', port=6379)
    await redis.lpush("events", await request.read())
    return web.json_response(data={"status": "success"})


async def subscribe(_: web.Request) -> web.Response:
    redis = await Redis(host='localhost', port=6379)
    _, event = await redis.brpop("events")
    return web.json_response(data={"event": event.decode()})


app = web.Application()
app.router.add_route("POST", "/pub", publish)
app.router.add_route("GET", "/sub", subscribe)
web.run_app(app)

In the previous version, when closing a client connection, an exception was thrown that canceled the connection to the redis too. Now, after the closing the client connection the task is not cancelled and waiting continuously until an event arrives, and will fail only when you try to write to a closed connection.

Thus, in this example, you can arrange a denial of service attack, simply create connections and close them without waiting for completion.

From a user experience point of view, I understand that the default when tasks are not canceled is very intuitive and simple, but from a security point of view, I am scared.

[Dreamsorcerer]

@Dreamsorcerer I started adding information to the documentation, and found this example this made me think that the current default behaviour is unsafe, and that's what I mean.

I don't see how that's related. There is no view/handler in that example.

I imagined a simpler example that worked well before the change where tasks weren't cancelled, and didn't work so well now.

Creating a Redis instance and waiting on an event that might never come, in a handler, just seems crazy. I've not seen any evidence that people are doing anything like this. I think we are still going to have more issues with the old behaviour than the new one (remember that 1 reason we changed it is because every other framework also does this, and they don't seem to be having major issues).

Thus, in this example, you can arrange a denial of service attack, simply create connections and close them without waiting for completion.

DoS is exactly why I'd suggest advanced users look at enabling this option again. It probably won't make a huge difference, but atleast it means an attacker must leave connections open to DoS the server, rather than finding a slow endpoint and briefly opening a connection there many times.

From a user experience point of view, I understand that the default when tasks are not canceled is very intuitive and simple, but from a security point of view, I am scared.

As mentioned, it seems every other framework already does this, and there aren't security warnings in Django, Flask etc. because of it. Ultimately, if someone really wants to DoS the server, this will probably only make it slightly harder, and you'll be best off with Cloudflare protection or something if there is a serious threat.

[mosquito]

Creating a Redis instance and waiting on an event that might never come, in a handler, just seems crazy.

The redis protocol is designed so that it can only process one command at a time, so one way or another you'll have to do it like this, in this example it's just done explicitly, without using connection pooling.

I've not seen any evidence that people are doing anything like this. I think we are still going to have more issues with the old behaviour than the new one (remember that 1 reason we changed it is because every other framework also does this, and they don't seem to be having major issues).

No, after applying my patch, there is no problem, I already checked.

DoS is exactly why I'd suggest advanced users look at enabling this option again. It probably won't make a huge difference, but atleast it means an attacker must leave connections open to DoS the server, rather than finding a slow endpoint and briefly opening a connection there many times.

I already sent the email to @asvetlov with a working exploit.

As mentioned, it seems every other framework already does this, and there aren't security warnings in Django, Flask etc. because of it.

Django and especially Flask are synchronous frameworks, and the task of stopping a workflow is the task of gunicorn or another program that implements a bridge between HTTP and WSGI.

Ultimately, if someone really wants to DoS the server, this will probably only make it slightly harder, and you'll be best off with Cloudflare protection or something if there is a serious threat.

Cloudflare will not protect against this. I think you are confusing DDoS and DoS.

It seems to me that you did not take into account all the negative consequences of disabling this early, especially in a minor patch release.

[Dreamsorcerer]

Creating a Redis instance and waiting on an event that might never come, in a handler, just seems crazy.

The redis protocol is designed so that it can only process one command at a time, so one way or another you'll have to do it like this, in this example it's just done explicitly, without using connection pooling.

Regardless (still inefficient), it's the waiting on an event that may never happen, which is the main issue. That's not code I've seen in realistic applications, there is either some guarantee that an event will happen or a timeout, I've never seen anything that blindly blocks forever in a handler, and most developers would realise that, whereas many don't realise that a handler can be cancelled midway.

I've not seen any evidence that people are doing anything like this. I think we are still going to have more issues with the old behaviour than the new one (remember that 1 reason we changed it is because every other framework also does this, and they don't seem to be having major issues).

No, after applying my patch, there is no problem, I already checked.

I meant more problems in general, not your specific example. So, unless you've tested every developer's code in the world...

I already sent the email to @asvetlov with a working exploit.

You'd be better off sending to @webknjaz. But, it's pretty trivial to come up with an example.

As mentioned, it seems every other framework already does this, and there aren't security warnings in Django, Flask etc. because of it.

Django and especially Flask are synchronous frameworks, and the task of stopping a workflow is the task of gunicorn or another program that implements a bridge between HTTP and WSGI.

Yes, and a quick search suggests that they behave exactly the same as aiohttp does today. They only error when trying to write to the socket (e.g. https://stackoverflow.com/questions/4142012/how-to-find-the-socket-connection-state-in-c).

Can you find a single framework which implements the cancelling behaviour we had before?
The fact that the feature was unique to aiohttp, is the reason it was causing problems for users.

Cloudflare will not protect against this. I think you are confusing DDoS and DoS.

No, I'm just considering that any WAF or similar that can stop an attacker hitting a slow endpoint a large number of times is going to be more effective. If an attacker has resources then they can go full DDoS and it wouldn't make much difference, if they don't have the resources then they'd probably still struggle to take down the server without auto-cancelling (unless you've actually created handlers which block forever on an event that never occurs, in which case you need to rethink your code...).

It seems to me that you did not take into account all the negative consequences of disabling this early, especially in a minor patch release.

I considered it, but the change was already made when I was first getting involved. Unless you can convince @asvetlov or @webknjaz that the default behaviour should be changed back (or provide evidence that the auto-cancelling is actually normal for most frameworks), I'll be leaving it as is.

Anyway, I'll take a look through the new changes in a bit (would be preferable to not force-push, it messes up the diff to look at), and we can hopefully get this feature back in, whether default or not.

[tzoiker]

I considered it, but the change was already made when I was first getting involved. Unless you can convince @asvetlov or @webknjaz that the default behaviour should be changed back (or provide evidence that the auto-cancelling is actually normal for most frameworks), I'll be leaving it as is.

I agree with the argumentation regarding the user expectations due to the other frameworks. The only problem with it is that this behaviour was changed with the patch release. Probably, reverting it back to cancellation on disconnect by default would cause even more damage now.

However, I suggest considering it for the next major release (4.0) if there'll be more evidence in favour.

[mosquito]

Regardless (still inefficient), it's the waiting on an event that may never happen, which is the main issue. That's not code I've seen in realistic applications, there is either some guarantee that an event will happen or a timeout, I've never seen anything that blindly blocks forever in a handler, and most developers would realise that, whereas many don't realise that a handler can be cancelled midway.

Regardless but http protocol has been designed similar. This code will be completely inefficient too:

from aiohttp import web, ClientSession


async def proxy_handler(request: web.Request) -> web.Response:
    session: ClientSession = request.app['client']

    async with session.get(f"/search?q={request.query.get('q')}") as response:
        # This code will wait for response but client connection already closed.
        # Of course the safe way is cancel this task after that.
        payload = await response.read()
    return web.Response(status=response.status, body=payload)


app = web.Application()
app.router.add_route("*", "/", proxy_handler)
app['client'] = ClientSession(base_url="https://stackoverflow.com/")
web.run_app(app)

This still inefficient code that you may not have seen in practice before will work exactly the same. So it will receive the entire page, although the client is no longer waiting for this. In the above example, you will quickly run into a TCPConnector limitation, and your service will no longer serve "good" clients (so thats a classical DoS). Worst of all, you will be downloading kilobytes over the network, and the attacker will be limited to transmitting a few lines of headers.

The HTTP 1.x protocol is not much different from the redis protocol, it is exactly the same text protocol that cannot asynchronously interact in one connection with many requests at once. You will exhaust the connection pool through the fault of the client that mimic a very bad connection (close the connection after headers sent each request). So the CloudFlare wount guard from it on 100%.

[Dreamsorcerer]

You will exhaust the connection pool through the fault of the client that mimic a very bad connection (close the connection after headers sent each request).

Yes, but you can still do that by keeping the connections alive. Even a single machine may be able to exhaust the connections of the average server (as most people don't change the default connection limits).

I agree that any developer deploying a high-performance service should look at enabling this option, but there also needs to be an understanding of how to write safe asyncio code when this option is enabled, and we've already seen that many developers simply don't know how to do that and don't understand the cancelling behaviour. Making it opt-in ensures they've looked at the documentation and understood how things work. Again, unless you can show that other frameworks are doing this as standard, I don't think anyone here will change the default behaviour back.

[mosquito]

@Dreamsorcerer @asvetlov let me sum it up a bit, I think that aiohttp was incredibly close to the protocol, which is its cardinal difference from the competition, and that was great. It was possible to do TLS services using only aiohttp, however, what is being discussed now narrows the scope, and now it is better for us not to do anything without Cloudflare and WAF.

The right way, in my opinion, would be to make it safe by default without relying on CloudFlare. Explaining to users why it's done this way, and how to disable this behavior, and the most important thing that follows (any script kiddie will make you a DoS), it would make the Internet a little more secure.

It is also worth explain the user about try: ... finally: ... about aiojobs, about manual task management, except from the documentation, he is unlikely to find out about it. I understand the argument - "others have done it another one". So let me go and find out from them how protected they are from attacks like this? Maybe we need to fix it there and not break it here? I don't mind doing this in principle, but I should start here.

[Dreamsorcerer]

OK, I've fixed the tests so they fail correctly now (the 2nd test seemed to even pass with both options).

I've renamed the parameter to handler_cancellation to be a little more concise (and being a little more vague should encourage users to check the documentation, I'd rather they didn't think they know what this does without looking at the documentation).

I've also stripped most of the documentation changes, I'm going to do this in more detail in another PR.

I also removed the Gunicorn env var. We tend to avoid using env vars in the library, with that being something for the application to handle. It also seems to me that the person deploying an app would be the one deciding whether the option is enabled or not, when it should be the developer who wrote the app.
So, let's come back to this specific case in another PR which can focus exclusively on how to enable this in a Gunicorn deployment.

[patchback]

Backport to 3.9: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply 38b9ec5 on top of patchback/backports/3.9/38b9ec51e52fdcab11bbe322cc66392c599ca183/pr-7056

Backporting merged PR #7056 into master

1. Ensure you have a local repo clone of your fork. Unless you cloned it
   from the upstream, this would be your origin remote.
2. Make sure you have an upstream repo added as a remote too. In these
   instructions you'll refer to it by the name upstream. If you don't
   have it, here's how you can add it:

   $ git remote add upstream https://github.com/aio-libs/aiohttp.git

3. Ensure you have the latest copy of upstream and prepare a branch
   that will hold the backported code:

   $ git fetch upstream
   $ git checkout -b patchback/backports/3.9/38b9ec51e52fdcab11bbe322cc66392c599ca183/pr-7056 upstream/3.9

4. Now, cherry-pick PR Added a configuration flag for enable request task handler cancelling when client connection closing. #7056 contents into that branch:

   $ git cherry-pick -x 38b9ec51e52fdcab11bbe322cc66392c599ca183

   If it'll yell at you with something like fatal: Commit 38b9ec51e52fdcab11bbe322cc66392c599ca183 is a merge but no -m option was given., add -m 1 as follows intead:

   $ git cherry-pick -m1 -x 38b9ec51e52fdcab11bbe322cc66392c599ca183

5. At this point, you'll probably encounter some merge conflicts. You must
   resolve them in to preserve the patch from PR Added a configuration flag for enable request task handler cancelling when client connection closing. #7056 as close to the
   original as possible.
6. Push this branch to your fork on GitHub:

   $ git push origin patchback/backports/3.9/38b9ec51e52fdcab11bbe322cc66392c599ca183/pr-7056

7. Create a PR, ensure that the CI is green. If it's not — update it so that
   the tests and any other checks pass. This is it!
   Now relax and wait for the maintainers to process your pull request
   when they have some cycles to do reviews. Don't worry — they'll tell you if
   any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.