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(settings): Add integrity checker #10212

Merged
merged 5 commits into from
May 20, 2024

Conversation

kiblik
Copy link
Contributor

@kiblik kiblik commented May 15, 2024

In many cases, users are changing dojo/settings/settings.dist.py file (they expect it is the way how the application should be configured).

This PR adds a checker that does not allow to start app if the change is detected. The error message points the user to documentation.

It is allowed only in debug mode when changes are expected.
Unittest which helps you in debug mode was added as well.

Copy link

dryrunsecurity bot commented May 15, 2024

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

DryRun Security Status Findings
Configured Codepaths Analyzer 0 findings
Sensitive Files Analyzer 1 finding
AppSec Analyzer 0 findings
Authn/Authz Analyzer 0 findings
Secrets Analyzer 0 findings

Note

🟢 Risk threshold not exceeded.

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 integrity of the DefectDojo application's configuration settings. The key changes include:

  1. Integrity Checks for settings.dist.py: The application now performs an integrity check on the settings.dist.py file, which is the default configuration file. If the file has been modified, the application will not start until the changes have been reverted. This is a positive security measure to prevent accidental or malicious changes to the core configuration.

  2. Recommendation to Use Environment Variables or local_settings.py: The documentation and code changes emphasize that any customizations to the configuration should be done through environment variables or the local_settings.py file, rather than directly modifying the settings.dist.py file. This is a recommended best practice for managing application configurations securely.

  3. Security-related Configuration Updates: The changes update various security-related settings, such as CSRF, session, and content security settings. These updates help improve the overall security posture of the DefectDojo application.

  4. Support for Additional Authentication Providers: The changes add support for various social login providers, which can improve the flexibility and security of the authentication process, depending on how these integrations are implemented.

  5. Deduplication and Hashcode Configuration: The changes update the configuration related to deduplication and hashcode handling for different vulnerability scanners. This is an important security-related feature, as it can impact the accuracy and effectiveness of the vulnerability management process.

Overall, these code changes demonstrate a strong focus on enhancing the security and maintainability of the DefectDojo application's configuration settings, which is a crucial aspect of application security.

Files Changed:

  1. docs/content/en/getting_started/upgrading/2.35.md: This file has been updated to document the new integrity check feature for the settings.dist.py file and the recommended approach for customizing the application's configuration.

  2. dojo/settings/.settings.dist.py.sha256sum: This file contains the SHA-256 hash of the settings.dist.py file, which is used to verify the integrity of the default configuration file.

  3. docker-compose.override.debug.yml: This file contains configuration changes for a development or debugging environment, including the use of environment variables for debugging and email configuration.

  4. dojo/settings/settings.dist.py: This file has been updated with numerous configuration changes, including security-related settings, authentication provider support, and deduplication and hashcode configuration.

  5. dojo/settings/settings.py: This file now includes code to check the integrity of the settings.dist.py file by verifying its SHA-256 hash.

  6. unittests/test_utils.py: This file has been updated to include a new test case that verifies the integrity of the settings.dist.py file by checking its SHA-256 hash.

Powered by DryRun Security

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

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.

Good idea. Just a few small suggestions. Given that people probably won't know about this if they're editing settings.dist.py as part of a PR, and we don't call this out explicitly for people who might be editing this on their instance, could you add a comment about this at the top of that file?

  • Explain that this file shouldn't be edited for modifying a running instance (you can use the same message you used in the ValueError)
  • Inform developers that they will need to update the hash in .settings.dist.py.sha256sum if they are modifying this file as part of a PR and explain how to do so (so they don't have to wait for a test failure to know about this)

dojo/settings/settings.py Outdated Show resolved Hide resolved
unittests/test_utils.py Outdated Show resolved Hide resolved
kiblik and others added 2 commits May 16, 2024 19:11
Co-authored-by: Charles Neill <1749665+cneill@users.noreply.github.com>
@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 May 16, 2024
@kiblik
Copy link
Contributor Author

kiblik commented May 16, 2024

Good idea. Just a few small suggestions. Given that people probably won't know about this if they're editing settings.dist.py as part of a PR, and we don't call this out explicitly for people who might be editing this on their instance, could you add a comment about this at the top of that file?

  • Explain that this file shouldn't be edited for modifying a running instance (you can use the same message you used in the ValueError)
  • Inform developers that they will need to update the hash in .settings.dist.py.sha256sum if they are modifying this file as part of a PR and explain how to do so (so they don't have to wait for a test failure to know about this)

Thank you for your feedback. I agree.
I'm just not sure I understand where you would like to place these notes. Both on the top of the settings.dist.py? I'm not sure it is the best place because it increases the chances that also for non-developers will edit settings.dist.py and update the checksum.

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.

The 2 comment blocks you added to settings.dist.py look good to me. Should resolve any confusion about who should and should not edit that file. Any thoughts @Maffooch @mtesauro ?

@mtesauro
Copy link
Contributor

The 2 comment blocks you added to settings.dist.py look good to me. Should resolve any confusion about who should and should not edit that file. Any thoughts @Maffooch @mtesauro ?

@cneill I'm good with the updates that @kiblik made in settings.dist.py 👍

@cneill
Copy link
Collaborator

cneill commented May 16, 2024

Sorry to ask for changes after approving, but since this will immediately break anyone who has modified their settings.dist.py file, would you mind adding a "breaking change" notice to the 2.35.0 release notes?

@github-actions github-actions bot added the docs label May 17, 2024
@kiblik
Copy link
Contributor Author

kiblik commented May 17, 2024

Sorry to ask for changes after approving, but since this will immediately break anyone who has modified their settings.dist.py file, would you mind adding a "breaking change" notice to the 2.35.0 release notes?

No problem. @cneill, can you check the wording?

@kiblik kiblik requested a review from cneill May 17, 2024 12:30
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.

Just a few small suggestions. Thanks for adding this

docs/content/en/getting_started/upgrading/2.35.md Outdated Show resolved Hide resolved
Co-authored-by: Charles Neill <1749665+cneill@users.noreply.github.com>
@Maffooch Maffooch merged commit f6f44b9 into DefectDojo:dev May 20, 2024
123 checks passed
@kiblik kiblik deleted the settings_integrity branch May 20, 2024 16:21
@quirinziessler
Copy link
Contributor

quirinziessler commented May 21, 2024

@kiblik how extensive have you tested this? I am not able to build even I enabled the Debug mode.

❯ docker/setEnv.sh debug Now using 'debug' configuration. ❯ ./dc-build.sh Checking docker compose version Supported docker compose version Building docker compose [+] Building 0.7s (35/39) docker:colima => [uwsgi internal] load build definition from Dockerfile.django-debian 0.0s => => transferring dockerfile: 4.81kB 0.0s => [uwsgi internal] load .dockerignore 0.0s => => transferring context: 98B 0.0s => [nginx internal] load metadata for docker.io/library/python:3.11.4-slim-bullseye@sha256:40319d0a897896e746edf877783ef3 0.0s => [nginx base 1/1] FROM docker.io/library/python:3.11.4-slim-bullseye@sha256:40319d0a897896e746edf877783ef39685d44e90e1e 0.0s => [uwsgi internal] load build context 0.2s => => transferring context: 2.19MB 0.2s => CACHED [nginx build 1/4] WORKDIR /app 0.0s => CACHED [uwsgi django 2/12] RUN apt-get -y update && mkdir -p /usr/share/man/man1 /usr/share/man/man7 && apt-get 0.0s => CACHED [nginx build 2/4] RUN apt-get -y update && apt-get -y install --no-install-recommends gcc build-ess 0.0s => CACHED [uwsgi build 3/4] COPY requirements.txt ./ 0.0s => CACHED [uwsgi build 4/4] RUN CPUCOUNT=1 pip3 wheel --wheel-dir=/tmp/wheels -r ./requirements.txt 0.0s => CACHED [uwsgi django 3/12] COPY --from=build /tmp/wheels /tmp/wheels 0.0s => CACHED [uwsgi django 4/12] COPY requirements.txt ./ 0.0s => CACHED [uwsgi django 5/12] RUN export PYCURL_SSL_LIBRARY=openssl && pip3 install --no-cache-dir --no-index -- 0.0s => CACHED [uwsgi django 6/12] COPY docker/entrypoint-celery-beat.sh docker/entrypoint-celery-worker.sh docker/entr 0.0s => CACHED [uwsgi django 7/12] COPY wsgi.py manage.py docker/unit-tests.sh ./ 0.0s => CACHED [uwsgi django 8/12] COPY dojo/ ./dojo/ 0.0s => CACHED [uwsgi django 9/12] COPY docker/extra_fixtures/* /app/dojo/fixtures/ 0.0s => CACHED [uwsgi django 10/12] COPY tests/ ./tests/ 0.0s => CACHED [uwsgi django 11/12] RUN rm -f /readme.txt && rm -f dojo/fixtures/readme.txt && mkdir -p dojo/migrations 0.0s => CACHED [uwsgi django 12/12] RUN addgroup --gid 1337 defectdojo && adduser --system --no-create-home --disabled 0.0s => [uwsgi] exporting to image 0.0s => => exporting layers 0.0s => => writing image sha256:970d0ef2eab758b9b7cb5801c1e21805628d2da39f8242d7c2d049e369106c5d 0.0s => => naming to docker.io/defectdojo/defectdojo-django:latest 0.0s => [nginx internal] load build definition from Dockerfile.nginx-debian 0.0s => => transferring dockerfile: 3.46kB 0.0s => [nginx internal] load .dockerignore 0.0s => => transferring context: 98B 0.0s => [nginx internal] load metadata for docker.io/library/nginx:1.26.0-alpine@sha256:ef587d1eb99e991291c582bfb74f27db27f7ca 0.0s => [nginx internal] load build context 0.1s => => transferring context: 263.65kB 0.1s => CACHED [nginx stage-3 1/5] FROM docker.io/library/nginx:1.26.0-alpine@sha256:ef587d1eb99e991291c582bfb74f27db27f7ca2c0 0.0s => CACHED [nginx build 3/4] COPY requirements.txt ./ 0.0s => CACHED [nginx build 4/4] RUN CPUCOUNT=1 pip3 wheel --wheel-dir=/tmp/wheels -r ./requirements.txt 0.0s => CACHED [nginx collectstatic 1/7] RUN apt-get -y update && apt-get -y install --no-install-recommends apt-transport 0.0s => CACHED [nginx collectstatic 2/7] RUN pip3 install --no-cache-dir --no-index --find-links=/tmp/wheels -r ./require 0.0s => CACHED [nginx collectstatic 3/7] COPY components/ ./components/ 0.0s => CACHED [nginx collectstatic 4/7] RUN cd components && yarn 0.0s => CACHED [nginx collectstatic 5/7] COPY manage.py ./ 0.0s => CACHED [nginx collectstatic 6/7] COPY dojo/ ./dojo/ 0.0s => ERROR [nginx collectstatic 7/7] RUN env DD_SECRET_KEY='.' python3 manage.py collectstatic --noinput && true 0.2s => [nginx collectstatic 7/7] RUN env DD_SECRET_KEY='.' python3 manage.py collectstatic --noinput && true: 0.210 Traceback (most recent call last): 0.210 File "/app/manage.py", line 10, in <module> 0.210 execute_from_command_line(sys.argv) 0.210 File "/usr/local/lib/python3.11/site-packages/django/core/management/__init__.py", line 446, in execute_from_command_line 0.210 utility.execute() 0.210 File "/usr/local/lib/python3.11/site-packages/django/core/management/__init__.py", line 386, in execute 0.210 settings.INSTALLED_APPS 0.210 File "/usr/local/lib/python3.11/site-packages/django/conf/__init__.py", line 92, in __getattr__ 0.210 self._setup(name) 0.210 File "/usr/local/lib/python3.11/site-packages/django/conf/__init__.py", line 79, in _setup 0.210 self._wrapped = Settings(settings_module) 0.210 ^^^^^^^^^^^^^^^^^^^^^^^^^ 0.210 File "/usr/local/lib/python3.11/site-packages/django/conf/__init__.py", line 190, in __init__ 0.210 mod = importlib.import_module(self.SETTINGS_MODULE) 0.210 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 0.210 File "/usr/local/lib/python3.11/importlib/__init__.py", line 126, in import_module 0.210 return _bootstrap._gcd_import(name[level:], package, level) 0.210 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 0.210 File "<frozen importlib._bootstrap>", line 1204, in _gcd_import 0.210 File "<frozen importlib._bootstrap>", line 1176, in _find_and_load 0.210 File "<frozen importlib._bootstrap>", line 1147, in _find_and_load_unlocked 0.210 File "<frozen importlib._bootstrap>", line 690, in _load_unlocked 0.210 File "<frozen importlib._bootstrap_external>", line 940, in exec_module 0.210 File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed 0.210 File "/app/dojo/settings/settings.py", line 23, in <module> 0.210 raise ValueError(msg) 0.210 ValueError: Change of 'settings.dist.py' file was detected. It is not allowed to edit this file. Any customization of variables need to be done via environmental variables or in 'local_settings.py'. For more information check https://documentation.defectdojo.com/getting_started/configuration/ failed to solve: process "/bin/sh -c env DD_SECRET_KEY='.' python3 manage.py collectstatic --noinput && true" did not complete successfully: exit code: 1

@kiblik
Copy link
Contributor Author

kiblik commented May 21, 2024

@kiblik how extensive have you tested this? I am not able to build even I enabled the Debug mode.

I have tested the local change of settings.dist.py (with a prebuilt image) and I tested the build in GitHub workflow. You are right that I forgot about the local ./dc-build.sh option with manage.py collectstatic.

I fixed this issue now: #10241

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

Successfully merging this pull request may close these issues.

None yet

6 participants