Settings SHA: The Removal

[Maffooch]

Following the introduction of the settings.dist.py integrity check, it has made contributing to DefectDojo more challenging. As PRs are merged with settings changes for parsers, vulnerability IDs, and new environment variables, everyone else with similar changes will now have conflicts. At times this could impact many people at once, and even multiple times within a singe PR.

Thought the integrity check is a great idea in enforcing that settings.dist.py not be touched, the tradeoff for ease of development is a turnoff for contributors

[sc-8583]

[dryrunsecurity]

DryRun Security Summary

The pull request covers various updates and modifications to the Dojo application's unit tests, GitHub Actions workflow, and configuration files, with potential security implications related to the removal of the integrity check for the settings.dist.py file and the lack of clear guidance on environmental variable handling.

Expand for full summary

Summary:

The code changes in this pull request cover various updates and modifications to the Dojo application's unit tests, GitHub Actions workflow, and configuration files. While the changes do not introduce any obvious security vulnerabilities, there are a few areas that should be reviewed from an application security perspective.

The removal of the test_settings_integrity test case in the unittests/test_utils.py file is not a major security concern, as it was primarily a test for the integrity of the configuration file and not directly related to application security. The removal of unused imports is a common code cleanup practice and does not introduce any security risks.

The changes to the GitHub Actions workflow for merging the master branch into the dev and bugfix branches appear to be a standard process for managing version updates and merging changes between branches. However, it's important to ensure that the version numbers are updated correctly and that the upgrade notes are accurate and complete to help users upgrade their systems safely.

The changes to the dojo/settings/settings.dist.py file, which remove the instructions for calculating the checksum of the file, may reduce the ability to detect unauthorized modifications to the configuration file. While the recommendation to use environmental variables or a separate local_settings.py file for customizations is a good security practice, the overall integrity of the configuration file should be carefully monitored.

The changes to the dojo/settings/settings.py file, which remove the integrity check for the settings.dist.py file, are more concerning from a security perspective. This check was likely in place to ensure that the settings file had not been tampered with, which could have introduced security vulnerabilities. Without this check, the application may be more susceptible to configuration-related security issues if the settings.dist.py file is modified by an attacker. Additionally, the lack of clear guidance on how environmental variables are handled and prioritized over the settings defined in the settings.dist.py file could lead to potential security risks.

Files Changed:

1. unittests/test_utils.py: The changes involve the removal of unused imports and the test_settings_integrity test case, which was a test for the integrity of the settings.dist.py file.

2. .github/workflows/release-3-master-into-dev.yml: The changes to this GitHub Actions workflow are related to the process of merging the master branch into the dev and bugfix branches after a new release. The workflow updates version numbers, generates upgrade notes, and creates pull requests.

3. dojo/settings/settings.dist.py: The changes remove the instructions for calculating the checksum of the settings.dist.py file, which may reduce the ability to detect unauthorized modifications to the configuration file.

4. dojo/settings/settings.py: The changes remove the integrity check for the settings.dist.py file, which could potentially introduce security vulnerabilities if the configuration file is tampered with. The lack of clear guidance on environmental variable handling is also a concern.

Code Analysis

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

Analyzer	Findings
Sensitive Files Analyzer	1 finding

Riskiness

🟢 Risk threshold not exceeded.

View PR in the DryRun Dashboard.

[kiblik]

Q: If HASHCODE_FIELDS_PER_SCANNER, HASHCODE_ALLOWS_NULL_CWE and DEDUPLICATION_ALGORITHM_PER_PARSER would be excluded from settings.dist.py (e.g. they would be part of parsers dojo/X/parser.py: XParser.get_deduplication_algo() + dynamically load them), can you consider to keep hash-checker? It would bring also cleaner labeling of PRs (right now simple change of hashcode fields adds PR label as "change of settings of the whole system" but it is more change of some parser's behavior)

[mtesauro]

Q: If HASHCODE_FIELDS_PER_SCANNER, HASHCODE_ALLOWS_NULL_CWE and DEDUPLICATION_ALGORITHM_PER_PARSER would be excluded from settings.dist.py (e.g. they would be part of parsers dojo/X/parser.py: XParser.get_deduplication_algo() + dynamically load them), can you consider to keep hash-checker? It would bring also cleaner labeling of PRs (right now simple change of hashcode fields adds PR label as "change of settings of the whole system" but it is more change of some parser's behavior)

@kiblik Taking a step back, I'm curious the benefit you thought came from the current sha256sum of settings.dist.py. To me, it was a signal to users that they shouldn't be editing settings.dist.py but using local_settings.py for their deploy customizations.

Your reply focuses more on hash codes so I'm wondering if I missed something about the original intent of adding that sha256sum check.

[kiblik]

Q: If HASHCODE_FIELDS_PER_SCANNER, HASHCODE_ALLOWS_NULL_CWE and DEDUPLICATION_ALGORITHM_PER_PARSER would be excluded from settings.dist.py (e.g. they would be part of parsers dojo/X/parser.py: XParser.get_deduplication_algo() + dynamically load them), can you consider to keep hash-checker? It would bring also cleaner labeling of PRs (right now simple change of hashcode fields adds PR label as "change of settings of the whole system" but it is more change of some parser's behavior)

@kiblik Taking a step back, I'm curious the benefit you though came from the current sha256sum of settings.dist.py. To me, it was a signal to users that they shouldn't be editing settings.dist.py but using local_settings.py for their deploy customizations.

Your reply focuses more on hash codes so I'm wondering if I missed something about the original intent of adding that sha256sum check.

Thank you @mtesauro. Good point, I haven't described my story behind it.
From my observation (but please correct me if I'm wrong), most of the edits of settings.dist.py are connected to changes in parser parameters (HASHCODE_FIELDS_PER_SCANNER, HASHCODE_ALLOWS_NULL_CWE, DEDUPLICATION_ALGORITHM_PER_PARSER). So they are also the biggest reason for merge conflicts.
If these fields were excluded from settings.dist.py, the number of friction points would decrease and the user (developers) experience would increase.
I had an idea of the distribution of the mentioned fields to parsers in the back of my head for a longer period of time but I have not communicated/implemented it until now. I thought it would "fix" this problematic factor.
But if you think that the ratio of changes is different and the number of "other" changes is significantly higher (PRs will still affect each other a lot) I would like to ask whether there is no other way to achieve the original goal (inform the user that direct editing of settings.dist.py is not recommended way of configuring this application).

[manuel-sommer]

What if we just add this information to the documentation?
If things go wrong in personal deployments due to changes of settings.dist.py, they did not read the documentation. Then, we can just refere to the documentation in issues / discussions.

[github-actions]

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

[Maffooch]

There is already a pretty good entry in the documentation warning about the changing settings.dist.py with respect to updates: https://documentation.defectdojo.com/getting_started/configuration/

What scares me about moving deduplication settings out of settings.py is that I fear they may not be configurable without rebuilding images. If that were not an issue, and the task was still to move ~85 dedupe settings to the parser level, it makes me wonder: Is all of that work (and potential migration woes/instability on update) worth it to keep an integrity check?

[Maffooch]

It is sorta humorous to me that removing the integrity check is causing conflicts around the integrity check 😅

[cneill]

I would like to ask whether there is no other way to achieve the original goal (inform the user that direct editing of settings.dist.py is not recommended way of configuring this application).

It seems like the only way to make this less painful would be to auto-generate the checksum at PR merge and/or release time, rather than asking developers to manage it. This would prevent merge conflicts while preserving the original intent, which I think has some value. Today, auto-generation only happens as part of automated merges, e.g. dev -> master, but it could in principle be extended to all PR merges that affect the settings file.

This would require a flag (e.g. DD_DISABLE_SETTINGS_CHECKSUM) that would only disable the "did you change the settings file" check when running unit tests in CI/CD and when developing locally. The original version used DEBUG as an escape valve for this, but that setting has other implications beyond checking the settings hash, so we don't necessarily want to enable it in CI/CD.

[cneill]

I'm not sure we need to announce the removal of this check in our release notes, but just wanted to note that we included it in the release notes in the original PR

[kiblik]

Hi everybody,

• Regarding parser tuning: I mentioned it without any proof. It was just a subjective point of view. I wouldn't be surprised if we would check the statistics, lastest reasons for an update of dojo/settings/settings.dist.py would be VULNERABILITY_URLS (@manuel-sommer, do you know something about that? 😆 Honestly, I'm so happy about these additions). So maybe we should not consider these parameters in this discussion and make decisions independently.
  • I'm still interested in moving values to parsers and loading them more dynamically (@Maffooch, of course with keeping some customization via values like DD_DEDUPLICATION_ALGORITHM_PER_PARSER or in local_settings.py). If I will have time, I might do some tests with this reorg (no promisses).
• If we would be able to add dojo/settings/.settings.dist.py.sha256sum to .gitignore in combination with DD_DISABLE_SETTINGS_CHECKSUM it would be nice and hopefully doable solution.

On the other hand, we might totally turn around perspective: Do not build walls but ask the question "Why are people editing these files"? In my opinion, it might be tightly connected to the quality of documentation and the way how it is written. This is not about blaming somebody. I was also participating on it in some way. If Configuration section in docs starts with the words "The main settings are stored in dojo/settings/settings.dist.py", we can not be surprised that users are ignoring the rest of the page and going directly there.
I would consider rewriting documentation in a way that user will have no motivation even to open any file in dojo/....
How?

1. Write doc: All EnvVars need to be listed in documentation in one page - with listed default value, type, and short description
2. Validator (to be sure that nobody misses point 1): Similar to

   django-DefectDojo/unittests/test_parsers.py

   Lines 30 to 40 in 6d847ab

   	with self.subTest(parser=parser_dir.name, category="docs"):
   	doc_file = os.path.join(basedir, "docs", "content", "en", "integrations", "parsers", category, f"{doc_name}.md")
   	self.assertTrue(
   	os.path.isfile(doc_file),
   	f"Documentation file '{doc_file}' is missing or using different name",
   	)
   	content = Path(doc_file).read_text(encoding="utf-8")
   	self.assertRegex(content, "title:",
   	f"Documentation file '{doc_file}' does not contain a title",
   	)

3. Auto-generator (to simplify point 1): Even step further would be autogenerated documentation from the code. I really like how bitnami generates README files (which describe all HELM values) just from well-written comments in values.yaml files: https://github.com/bitnami/readme-generator-for-helm. Unfortunately, I don't know how we can achieve this in our context (context of EnvVars and docs). But it would be nice 😸
4. Decrease EnvVar scope: It is not possible to do it with all variables but what about migrating (a reasonable amount) to System_settings?
   • I remember a nice try with auditlog(DD_ENABLE_AUDITLOG) and it wasn't successful then 😞 (add system setting to enable/disable audit logging #2068, Fix incorrect implementation of auditlog managment #9002). But after upgrading to 3.0 (Bump django-auditlog from 2.3.0 to 3.0.0 #9945), it might be possible (Dynamic cross-instance register/unregister jazzband/django-auditlog#586, Conditionally disable / enable logging jazzband/django-auditlog#590)

---

Hint (for somebody who will implement a validator):

>>> from dojo.settings import settings
>>> for key, (key_type, default) in settings.env.scheme.items():
...     print(f"Key:{key}\tType:{key_type}\tDefault:{default}")
...
Key:DD_SITE_URL	Type:<class 'str'>	Default:http://localhost:8080
Key:DD_DEBUG	Type:<class 'bool'>	Default:False
Key:DD_TEMPLATE_DEBUG	Type:<class 'bool'>	Default:False
Key:DD_LOG_LEVEL	Type:<class 'str'>	Default:
Key:DD_DJANGO_METRICS_ENABLED	Type:<class 'bool'>	Default:False
Key:DD_LOGIN_REDIRECT_URL	Type:<class 'str'>	Default:/
...