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

Stop rate-limiting asset-signed-url requests #2786

Merged
merged 1 commit into from Oct 25, 2022

Conversation

jschaul
Copy link
Member

@jschaul jschaul commented Oct 20, 2022

After not using the wire client for some time, it can easily happen that many conversations have many assets that should be downloaded. We may wish to be more lenient on asset download (well, getting signed URLs to download assets) requests. See https://wearezeta.atlassian.net/browse/SQCORE-1372 and https://wearezeta.atlassian.net/browse/SQSERVICES-1763

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@jschaul jschaul temporarily deployed to cachix October 20, 2022 16:05 Inactive
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Oct 20, 2022
@tlebon
Copy link
Contributor

tlebon commented Oct 21, 2022

@jschaul i have created a ticket for the FE side of things if you would like to attach https://wearezeta.atlassian.net/browse/SQSERVICES-1763

@akshaymankar
Copy link
Member

Have the downsides/risks of this been considered? If so, can you please write them here or link it?

@jschaul
Copy link
Member Author

jschaul commented Oct 24, 2022

I updated the description to include links to the two related issues.

Have the downsides/risks of this been considered? If so, can you please write them here or link it?

The only risk I can see here is a bigger amount of requests going to cargohold for getting an asset link, or deleting an asset, and a bigger amount of requests going to AWS for this. As none of these requests involve streaming of actual asset payloads (that's only /assets without a trailing slash), the only potential risk I see is a form of denial-of-service attack on cargohold (though you would need to create really a lot of requests for this - all the while being authenticated as a Wire user, allowing us to see where this comes from). We already have a few other unlimited API endpoints (/access), which have a higher potential of causing issues for users were it to be denial-of-service-attacked.
@akshaymankar do you see any other risks here?

…hat many conversations have many assets that should be downloaded. We may wish to be more lenient on asset download (well, getting signed URLs to download assets) requests.
@jschaul jschaul force-pushed the dont-rate-limit-asset-downloads branch from 41b9ff1 to eb5a4eb Compare October 24, 2022 11:37
@jschaul jschaul temporarily deployed to cachix October 24, 2022 11:37 Inactive
@akshaymankar
Copy link
Member

Sounds good, I don't see any other threats, just wanted to make sure we leave some context here in case we need to look into this in future.

@jschaul jschaul merged commit 3825ed1 into develop Oct 25, 2022
@jschaul jschaul deleted the dont-rate-limit-asset-downloads branch October 25, 2022 12:35
jschaul added a commit that referenced this pull request Mar 28, 2023
… too (#3138)

Fix a rate-limit exemption whereby authenticated endpoints did not get the unlimited_requests_endpoint, if set, applied. This is a concern for the webapp and calls to /assets, which can happen in larger numbers on initial loading. A previous change in [this PR](#2786) had no effect. 

This PR also increases default rate limits, to compensate for [new ingress controller chart](#3140 default topologyAwareRouting.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants