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

remove mysql leftover #10694

Open
wants to merge 23 commits into
base: bugfix
Choose a base branch
from

Conversation

manuel-sommer
Copy link
Contributor

No description provided.

@github-actions github-actions bot added the settings_changes Needs changes to settings.py based on changes in settings.dist.py included in this PR label Aug 7, 2024
Copy link

dryrunsecurity bot commented Aug 7, 2024

DryRun Security Summary

The pull request includes various updates and improvements to the DefectDojo application, with a focus on enhancing the application's security, including configuration file updates, database configuration changes, refactoring of utility functions, and proper handling of sensitive information.

Expand for full summary

Summary:

The code changes in this pull request cover various updates and improvements to the DefectDojo application, with a focus on enhancing the application's security. The changes include updates to configuration files, refactoring of utility functions, and modifications to database-related settings.

From an application security engineer's perspective, the key highlights are:

  1. Configuration File Updates: The changes to the .settings.dist.py and template-env files address several security-related settings, such as enabling HTTPS, setting appropriate security headers, and securing session and CSRF cookies. These changes help improve the overall security posture of the application.

  2. Database Configuration: The removal of support for MySQL databases and the recommendation to properly encode unsafe characters in the database URL are positive security measures that reduce the application's attack surface and mitigate potential SQL injection vulnerabilities.

  3. Refactoring of Utility Functions: The changes to the dojo/metrics/utils.py and dojo/components/sql_group_concat.py files focus on improving the code quality and maintainability, which indirectly benefits the application's security. However, it's essential to ensure that user input is properly validated and sanitized to prevent potential vulnerabilities, such as SQL injection or cross-site scripting (XSS) attacks.

  4. Sensitive Information Handling: The changes highlight the importance of properly handling and storing sensitive information, such as secret keys and encryption keys, to maintain the application's overall security.

Overall, the code changes in this pull request appear to be focused on improving the security and maintainability of the DefectDojo application. As an application security engineer, I would recommend thoroughly reviewing the changes, testing the application's security, and ensuring that any potential vulnerabilities are addressed.

Files Changed:

  1. dojo/settings/.settings.dist.py.sha256sum: The SHA-256 hash value for the .settings.dist.py file has been updated, indicating a change to the configuration file. It's important to review the changes to the .settings.dist.py file to ensure that they do not introduce any security vulnerabilities or unintended behavior.

  2. dojo/metrics/utils.py: The changes in this file are focused on refactoring the logic for calculating metrics related to findings. While the changes do not introduce any obvious security concerns, it's crucial to ensure that user input is properly validated and sanitized to prevent potential security vulnerabilities.

  3. dojo/components/sql_group_concat.py: The changes in this file update the Sql_GroupConcat class, which is used to perform SQL GROUP_CONCAT operations. It's important to ensure that the class is used securely and that any potential vulnerabilities, such as SQL injection or denial of service, are addressed.

  4. dojo/settings/settings.dist.py: The changes in this file update various Django settings, including several security-related settings. These changes help improve the overall security of the application and should be reviewed carefully to ensure that they align with the application's security requirements and best practices.

  5. dojo/settings/template-env: The changes in this file remove the option to use a MySQL database URL, limiting the database options to only PostgreSQL. This is a positive security measure that reduces the application's attack surface.

Code Analysis

We ran 9 analyzers against 5 files and 1 analyzer had findings. 8 analyzers had no findings.

Analyzer Findings
Configured Codepaths Analyzer 5 findings

Overall Riskiness

🔴 Risk threshold exceeded.

We've notified @mtesauro, @grendel513.

View PR in the DryRun Dashboard.

dojo/metrics/utils.py Outdated Show resolved Hide resolved
@kiblik
Copy link
Contributor

kiblik commented Aug 7, 2024

Maybe the following might be removed as well:

This might be adjusted just not sure how:

@manuel-sommer
Copy link
Contributor Author

I also don't know @kiblik

@Maffooch
Copy link
Contributor

Maffooch commented Aug 7, 2024

I was nervous to remove those things tbh. I did not have the time when this removal work occurred to fully test and ensure there were no breakages. The safe option is to leave them there, but it would be nice to get rid of these leftovers

Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@mtesauro
Copy link
Contributor

mtesauro commented Aug 13, 2024

I was nervous to remove those things tbh. I did not have the time when this removal work occurred to fully test and ensure there were no breakages. The safe option is to leave them there, but it would be nice to get rid of these leftovers

I agree with going slow on these parts of the code that we're not sure about. We have deprecated MySQL and RabbitMQ but I also don't want to make changes that might actually break MySQL (vs removing GHA tests & entries in compose) until the next minor release (2.38.0 / Sept) to give people a bit more time to migrate to PostgreSQL/Redis especially as it seems removing these has a good chance of busting MySQL users.

We keep getting updates to the GH discussion on migrating to Postgres: #9480

Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@manuel-sommer
Copy link
Contributor Author

I rebased this @mtesauro. I guess we can give this a go now.

Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Contributor

github-actions bot commented Nov 1, 2024

Conflicts have been resolved. A maintainer will review the pull request shortly.

Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@mtesauro
Copy link
Contributor

mtesauro commented Nov 29, 2024

@manuel-sommer FYI: As of Feb, we'll have deprecated MySQL for 6 months so we're targeting getting this PR merged in the Feb minor release.

Thanks for keeping this alive 🚀

Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

Copy link
Contributor

github-actions bot commented Dec 2, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Contributor

github-actions bot commented Dec 2, 2024

Conflicts have been resolved. A maintainer will review the pull request shortly.

Copy link
Contributor

github-actions bot commented Dec 3, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Contributor

github-actions bot commented Dec 3, 2024

Conflicts have been resolved. A maintainer will review the pull request shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
settings_changes Needs changes to settings.py based on changes in settings.dist.py included in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants