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

Allow setting --max-fd argument to uwsgi to stop it from getting OOMKilled in Kubernetes #10384

Merged
merged 11 commits into from
Jul 12, 2024

Conversation

tmablunar
Copy link
Contributor

Description

Reopening #9564

This PR fixes the issue described in issue #9562 regarding uWSGI that under some circumstances will take up an unnecessary amount of resources on a kubernetes node leading to the pod getting OOMKilled.

We introduce the possibility to set the --max-fd argument when starting up uWSGI to mitigate this issue.

Test results

I have tested the fix on a kubernetes cluster where it prevented the pod from getting OOMKilled. For more information see #9562.

Documentation

It is not clear to me where the documentation should be updated.

Copy link

dryrunsecurity bot commented Jun 11, 2024

Hi there 👋, @DryRunSecurity here, below is a summary of our analysis and findings.

DryRun Security Status Findings
Server-Side Request Forgery Analyzer 0 findings
Configured Codepaths Analyzer 7 findings
IDOR Analyzer 0 findings
Sensitive Files Analyzer 0 findings
SQL Injection Analyzer 0 findings
Authn/Authz Analyzer 0 findings
Secrets Analyzer 0 findings

Note

🔴 Risk threshold exceeded. Adding a reviewer if one is configured in .dryrunsecurity.yaml.

notification list: @mtesauro @grendel513

Change Summary (click to expand)

The following is a summary of changes in this pull request made by me, your security buddy 🤖. Note that this summary is auto-generated and not meant to be a definitive list of security issues but rather a helpful summary from a security perspective.

Summary:

The code changes in this pull request focus on improving the security and reliability of the DefectDojo application, a popular open-source application security management tool. The key changes include:

  1. uWSGI Process Configuration: The docker/entrypoint-uwsgi.sh file introduces a new environment variable DD_UWSGI_MAX_FD that allows setting the maximum number of file descriptors for the uWSGI process. This is a positive change as it enables better control and optimization of the uWSGI process's resource usage, which is important for preventing potential denial-of-service attacks or resource exhaustion issues.

  2. Monitoring and Observability: The changes in the helm/defectdojo/templates/configmap.yaml and helm/defectdojo/values.yaml files include configuration options for enabling Django and Nginx metrics, as well as setting the HTTP authentication user for the metrics endpoint. This improved monitoring and visibility can help detect and respond to potential security incidents.

  3. Transport Encryption for Redis: If the application is configured to use Redis as the Celery broker, the pull request includes an option to enable transport encryption for the Redis connection. This is a good security practice, as it helps to protect the confidentiality and integrity of the communication between the application and the Redis server.

Overall, the changes in this pull request appear to be focused on improving the security and reliability of the DefectDojo application, which is a positive step from an application security perspective. However, it's important to ensure that the new configuration parameters, such as the DD_UWSGI_MAX_FD variable, are properly validated and sanitized to prevent potential issues.

Files Changed:

  1. docker/entrypoint-uwsgi.sh: This file introduces a new environment variable DD_UWSGI_MAX_FD that can be used to set the maximum number of file descriptors for the uWSGI process, which is a positive change for improving resource management and preventing potential denial-of-service attacks.

  2. helm/defectdojo/templates/configmap.yaml: The changes in this file include the introduction of a new configuration parameter DD_UWSGI_MAX_FD, as well as options for enabling Django and Nginx metrics and setting the HTTP authentication user for the metrics endpoint. These changes improve the monitoring and observability of the application, which is an important security practice.

  3. helm/defectdojo/values.yaml: The changes in this file update the Django configuration to include a commented-out line to set the max_fd (maximum number of file descriptors) for the Django application, and enable the Nginx Prometheus exporter sidecar for improved monitoring and observability. These changes can have a positive impact on the application's security posture.

Powered by DryRun Security

@tmablunar tmablunar mentioned this pull request Jun 11, 2024
3 tasks
@tmablunar
Copy link
Contributor Author

I can see that the tests are failing due to changes being made to files that that I (or @hoeg) is not allowed to alter. How should I handle this?

@mtesauro
Copy link
Contributor

It's OK to have a failed test only for this one GH Action:
image

Basically, we use that to alert reviewers that one of the files that 'important' to DefectDojo has been modified by someone who isn't a reviewer/core contributor. Those are files that have historically had bugs in them, require migrations or other things the reviewers need to keep in mind. It's sort of a 'heads up' to reviewers. Thanks for asking 👍

However, the k8s tests do need to pass - I'm going to re-kick them now but a trick you can use in the future is to close and them re-open the PR which will re-run the tests since you likely don't have permission to re-kick tests.

@tmablunar tmablunar closed this Jun 17, 2024
@tmablunar tmablunar reopened this Jun 17, 2024
@tmablunar tmablunar closed this Jun 17, 2024
@tmablunar tmablunar reopened this Jun 17, 2024
@tmablunar
Copy link
Contributor Author

It's OK to have a failed test only for this one GH Action: image

Basically, we use that to alert reviewers that one of the files that 'important' to DefectDojo has been modified by someone who isn't a reviewer/core contributor. Those are files that have historically had bugs in them, require migrations or other things the reviewers need to keep in mind. It's sort of a 'heads up' to reviewers. Thanks for asking 👍

However, the k8s tests do need to pass - I'm going to re-kick them now but a trick you can use in the future is to close and them re-open the PR which will re-run the tests since you likely don't have permission to re-kick tests.

It does not seem like the k8s tests has been run even after closing and reopening the PR. At least the status is not shown. Could you re-kick them again or advise how to get them run?

@tmablunar tmablunar requested a review from cneill June 21, 2024 08:38
Copy link
Collaborator

@cneill cneill left a comment

Choose a reason for hiding this comment

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

This should be good to go after a quick typo fix

helm/defectdojo/values.yaml Outdated Show resolved Hide resolved
Co-authored-by: Charles Neill <1749665+cneill@users.noreply.github.com>
Copy link
Contributor

@mtesauro mtesauro left a comment

Choose a reason for hiding this comment

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

Approved

@mtesauro mtesauro merged commit f71dd87 into DefectDojo:bugfix Jul 12, 2024
124 of 125 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants