Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Handle CouchDB HTTP 403 on all routes #56

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

H--o-l
Copy link
Contributor

@H--o-l H--o-l commented Oct 23, 2024

Since CouchDB v3.4.0, there has been a new "Lockout" feature, i.e., a rate limit
on tuples (IP, login) after multiple authentication failures. It's highlighted
in the release note: https://docs.couchdb.org/en/stable/whatsnew/3.4.html#id4
(see the second to last bullet point).

As the following upstream discussion shows, this CouchDB feature adds a new case
of HTTP 403 possible on all routes:
apache/couchdb#5315 (comment)

This commit catches the 403 on all routes. As some routes were already catching
403 for other reasons, the exception message on these routes is changed from
their previous message to "Access forbidden: {reason}" where reason is
either the reason returned by CouchDB in the JSON body of the answer, or if
it doesn't exist, by the message of aiohttp ClientResponseError.

I manually tested a non-stream route with await couchdb.info(), it returns the
following:

> await couchdb.info()
...
aiocouch.exception.UnauthorizedError: Invalid credentials
> await couchdb.info()  # <=== Lockout
...
aiocouch.exception.ForbiddenError: Access forbidden: Account is temporarily
locked due to multiple authentication failures

if not resp.ok:
# Save the reason for later if any
try:
self.failure_reason = (await resp.json())["reason"]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unfamiliar enough with aiohttp streamed request to know if I'm doing something wrong here.

Please give your opinion on it I would be grateful.

@@ -203,12 +217,14 @@ async def close(self) -> None:
await asyncio.sleep(0.250 if has_ssl_conn else 0)

@raises(401, "Invalid credentials")
@raises(403, "Access forbidden: {reason}")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only manually tested this one with a real CouchDB, not the other routes.

@H--o-l H--o-l force-pushed the hoel/403 branch 3 times, most recently from 6f9cf4f to 2b14a0d Compare October 23, 2024 15:26
Comment on lines 164 to 170
if not resp.ok:
# Save the reason for later if any
try:
self.failure_reason = (await resp.json())["reason"]
except:
self.failure_reason = None
resp.raise_for_status()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, my bad. I only quickly checked the documentation and assumed that the message field in the ClientResponseError is the body of the response. So I assumed, that there is no need to pass the response body around.

However, I don't like the indirect setting of failure_reason through the endpoint instances. Instead, I'd extend the ClientResponseError and add a new field reason, which would be set from the response JSON in its constructor. And then throw this exception manually instead of the raise_for_status call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted, I'm on it but I didn't had the time to finish today, I will finish next week. Have a good weekend!

Copy link
Contributor Author

@H--o-l H--o-l Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I assumed, that there is no need to pass the response body around.

I checked the aiohttp v3.9.5 code, and I didn't find a way to add the body reason to the exception.

However, I don't like the indirect setting of failure_reason through the endpoint instances. Instead, I'd extend the ClientResponseError and add a new field reason, which would be set from the response JSON in its constructor. And then throw this exception manually instead of the raise_for_status call.

I updated the PR with your suggestion, thanks.

When raising the exception manually instead of calling aiohttp raise_for_status(), I copied the whole code of raise_for_status() inside aiocouch. I let you see what it gives inside the PR diff but, to be noted that I don't like it, as this upstream code could change without aiocouch being updated. For example, between aiohttp v3.9.5 and aiohttp v3.10.10 that code already changed.
In v3.9.5:

    def raise_for_status(self) -> None:
        if not self.ok:
            # reason should always be not None for a started response
            assert self.reason is not None
            self.release()
            raise ClientResponseError(
                self.request_info,
                self.history,
                status=self.status,
                message=self.reason,
                headers=self.headers,
            )

In v3.10.10:

   def raise_for_status(self) -> None:
        if not self.ok:
            # reason should always be not None for a started response
            assert self.reason is not None

            # If we're in a context we can rely on __aexit__() to release as the
            # exception propagates.
            if not self._in_context:
                self.release()

            raise ClientResponseError(
                self.request_info,
                self.history,
                status=self.status,
                message=self.reason,
                headers=self.headers,
            )

Solutions I see:

  1. Don't copy-paste this code and just manually raise ClientResponseError(); that could let some aiohttp "context" in the wild, ending in a memory leak (I guess).
  2. don't get the reason from the CouchDB body answer and just use the aiohttp.ClientResponseError exception message, but it's really vague.
  3. contribute to aiohttp so the body of the request is set inside aionttp.ClientResponseError(), but I don't have the shoulders or the time to do that.

If you agree with me that the current code of the PR is not maintainable, I think solution 2. is the best in the end. What do you think?

Since CouchDB v3.4.0, there has been a new "Lockout" feature, i.e., a rate limit
on tuples (IP, login) after multiple authentication failures. It's highlighted
in the release note: https://docs.couchdb.org/en/stable/whatsnew/3.4.html#id4
(see the second to last bullet point).

As the following upstream discussion shows, this CouchDB feature adds a new case
of HTTP 403 possible on all routes:
apache/couchdb#5315 (comment)

This commit catches the 403 on all routes. As some routes were already catching
403 for other reasons, the exception message on these routes is changed from
their previous message to `"Access forbidden: {reason}"` where `reason` is
either the `reason` returned by CouchDB in the JSON body of the answer, or if
it doesn't exist, by the `message` of aiohttp ClientResponseError.

I manually tested a non-stream route with `await couchdb.info()`, it returns the
following:

```
> await couchdb.info()
...
aiocouch.exception.UnauthorizedError: Invalid credentials
> await couchdb.info()  # <=== Lockout
...
aiocouch.exception.ForbiddenError: Access forbidden: Account is temporarily
locked due to multiple authentication failures
```

Closes metricq#55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants