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 primarily focuses on updates to the unit tests, configuration management, and release management processes of the DefectDojo application, with minimal impact on the application's security posture.

Expand for full summary

Summary:

The code changes in this pull request primarily focus on updates to the unit tests, configuration management, and release management processes of the DefectDojo application. None of the changes appear to introduce any obvious security vulnerabilities, and the overall impact on the application's security posture is minimal.

The key changes include the removal of unused imports and functionality from the unit tests, simplification of the configuration loading process by removing an integrity check for the settings.dist.py file, and updates to the GitHub Actions workflow responsible for managing the release process. The changes to the settings.dist.py file itself are mostly related to improving the documentation and guidance around customizing the application's settings, which is an important security practice.

While the changes do not directly impact the security of the application, it's important to ensure that the security-related settings in the settings.dist.py file are properly configured and maintained, as they play a crucial role in the overall security of the DefectDojo application. Additionally, the release management process and the generation of upgrade notes are good security practices that help users stay up-to-date with the latest security fixes and changes.

Files Changed:

1. unittests/test_utils.py: The changes remove unused imports and the TestSettings class, which was responsible for verifying the integrity of the settings.dist.py file. This change does not introduce any obvious security concerns.

2. dojo/settings/settings.py: The changes remove the functionality that checked the SHA-256 hash of the settings.dist.py file to ensure its integrity. This removal does not introduce any security vulnerabilities, as long as the application's configuration management and deployment processes are robust.

3. .github/workflows/release-3-master-into-dev.yml: The changes update the version numbers in various files, create a new branch for merging the master branch into the dev and bugfix branches, and generate upgrade notes for major and minor releases. These changes are security-positive, as they help ensure that the application is running the latest version with the latest security fixes.

4. dojo/settings/settings.dist.py: The changes update the comments in the file to provide guidance on customizing the settings, and remove the instructions for calculating and storing the checksum of the file. These changes do not directly impact the security of the application, but they are important for ensuring that the security-related settings in the file are properly configured and maintained.

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

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:/
...

[mtesauro]

Sorry, I'm late to this party, have had some non-DefectDojo stuff keeping me from engaging at my normal level - it seems to me that we should do thing in phases / iteratively.

1. We start with merging this PR as is to remove the check and see how things go
2. We give the removal till the Jan minor version release to see how thing go
3. Re-evaluate what if any next steps are needed.

The irony of having this PR have a merge conflict for the very thing we're wanting to remove isn't lost on me. 🤕

FWIW, I'm going to go ahead and approve this with the understanding that @Maffooch can sort out the merge conflict before it's actually merged.

[mtesauro]

Approved

[github-actions]

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

[valentijnscholten]

DD could help admins by displaying the effective settings/environment on a page. With some care around secrets / obfuscation. Or/additionally they could be displayed on start-up. Hopefully this will help admins before they come to #slack or @github.

Was it ever considered to only apply the check on official releases, or builds made from master and bugfix branches? Leaving out the SHA from the settings.dist.py could disable the check. Or it can be disabled in devmode. The release or merge process could insert the hash for merges to or releases from master/bugfix.