feat(SLA config): allow to use it on all levels

[kiblik]

Finish of #11300

[dryrunsecurity]

This pull request reveals multiple security concerns including potential race conditions during SLA configuration updates, information disclosure risks through verbose error messages, configuration-based security vulnerabilities that could bypass access controls, and a potential authentication weakness involving hardcoded admin tokens in test environments.

💭 Unconfirmed Findings (4)

Vulnerability	Potential Race Conditions
Description	Multiple files show synchronization risks during SLA configuration updates, with async updating mechanisms that could potentially be exploited or cause service disruption. Specific risks are present in models.py, engagement/views.py, and unittests/test_finding_model.py.

Vulnerability	Information Disclosure Risks
Description	Detailed messages and popovers expose internal system processes and configuration details. Verbose error handling and configuration change messages could leak system internals, identified in files like engagement/views.py, forms.py, templates/dojo/view_eng.html, and test/views.py.

Vulnerability	Configuration-based Security Risks
Description	New setting SLA_CONFIG_ON_NON_PRODUCT_LEVELS could bypass existing access controls. Dynamic configuration loading without strict validation presents potential for unintended configuration behaviors, found in settings/settings.dist.py and models.py.

Vulnerability	Potential Authentication Weakness
Description	Hardcoded admin token used in test environment, which could indicate risky authentication practices if similar patterns exist in production. This was discovered in unittests/test_finding_model.py.

---

All finding details can be found in the DryRun Security Dashboard.

[Maffooch]

Hi @kiblik I think the line of thinking from the previous comment is still valid here

[kiblik]

Hi @kiblik I think the line of thinking from the previous comment is still valid here

Hi @Maffooch,

• regarding documentation/explanation (how does it work), the fields have a description which is accessible as a hint:

• If it is necessary to add a couple of words to the documentation, I can write them down. Do you have a recommendation for which documentation page? Or should I create a new one?
• Regarding performance, I changed the implementation to opt-in mode, so users can decide to enable it. Or can I which it to opt-out?

Are there any other doubts that I should fix or explain?

[kiblik]

@Maffooch / @mtesauro, can I know your opinion, please?

[Maffooch]

The fear here is that we complicate the model, API, documentation, etc. Even if this feature were to be opt in thing from the UI, the code base cannot opt out

[kiblik]

The fear here is that we complicate the model, API, documentation, etc. Even if this feature were to be opt in thing from the UI, the code base cannot opt out

I suppose the adjustment of the model was expected from the beginning (assignment of specific SLA Config needs to be stored somewhere), but this kind of concern has not been raised at the beginning when we were talking about implementation: #10025

I'm willing to scale it down only to Engagement if it would help (even though I still believe there is reason to have it on Test level as well).

Complexity of API? I'm not sure, as there are only two more fields (sla_configuration for Eng and Test) that users can assign. I see that there are new fields (e.g. pro in CommonImportScanSerializer for import and reimport - #12525) that are not usable by non-opensource-users, and they have been added without any complaints.

Documentation? Well, a good product should have good documentation. I'm willing to add it. I just asked where to place it - to keep it as clean as possible - I'm happy to keep it polished and organised.

I agree that adding more and more code to a project makes it harder to maintain. It is the reason regular cleanup of not well-written or not used parts (like Google Sheets or async-eval) should be performed. But I suppose it should not be the reason to stop the development of features that people are happy to use and willing to maintain.

[kiblik]

@Maffooch, did my description help explain why I still believe my idea is reasonable and should be part of the implementation?

[dogboat]

I appreciate all the work and thought that's been put into this. That said, I'm not certain I agree with the need for SLAs at these levels.

One concern I have isn't just about the complexity this adds to the code; I also worry about what this does to the experience for new/inexperienced DD users. DD has a ton of settings already, and layering on additional "this thing overrides this thing overrides this thing" is a lot for someone inexperienced to wrangle with... and forget about. I think for users that Know What They're Doing ™️ this isn't a problem; but for someone with less experience, I could see them changing an SLA, forgetting about it, and being confused as to what's going on. Or imagine yourself setting DD up for the very first time: there's already a ton of decisions to make, and this adds more. All for functionality that I don't think has been really requested since DD's inception.

Even though it's essentially feature-flagged via setting, it is still yet another setting new users have to make a decision about, and in line with other comments already made it does introduce complexity to reasoning about how the program works.

Just my two cents.

[kiblik]

I appreciate all the work and thought that's been put into this. That said, I'm not certain I agree with the need for SLAs at these levels.

One concern I have isn't just about the complexity this adds to the code; I also worry about what this does to the experience for new/inexperienced DD users. DD has a ton of settings already, and layering on additional "this thing overrides this thing overrides this thing" is a lot for someone inexperienced to wrangle with... and forget about. I think for users that Know What They're Doing ™️ this isn't a problem; but for someone with less experience, I could see them changing an SLA, forgetting about it, and being confused as to what's going on. Or imagine yourself setting DD up for the very first time: there's already a ton of decisions to make, and this adds more. All for functionality that I don't think has been really requested since DD's inception.

Even though it's essentially feature-flagged via setting, it is still yet another setting new users have to make a decision about, and in line with other comments already made it does introduce complexity to reasoning about how the program works.

Just my two cents.

Thank you @dogboat for reaction. I'm happy for this kind of feedback.

I agree that DD became quite large, complex and hard to understand for new users. But as you also mentioned, this is opt-in functionality.
From my experience, new users are usually trying to start a minimal setup and then tune the setting. So I'm sure I agree with "another setting new users have to make a decision about". Testing all possible settings for new users is not in place on daily basis in my opinion.

Regarding, "changing an SLA, forgetting about it, and being confused as to what's going on". I can imagine to add a helper (shown when you hover over SLA value) which might describe policy, how the value was evaluated (like "SLA calculated based to Engagement level override").

[blakeaowens]

@kiblik The main concern I have with this feature is performance. Each time an SLA model instance is changed, or a new one is assigned to a Product, all Findings within it have to have their sla_expiration_date re-calculated. Adding more places for a user to set an SLA directly adds to the potential number of finding.save() operations that will need to be completed to ensure all SLA data is always up-to-date.

[mtesauro]

@kiblik I think we've heard from a majority of the DefectDojo moderators/contributors and given the community at large a chance to chime in here. This PR has been open since May so ~2 months of time seems sufficient to gather feedback.

FWIW, I still hold with my initial response to the prior PR here even with the smaller scope of the current PR.

Based on the preponderance of the commenters in this an #11300 those who don't want to add this functionality to DefectDojo have argued:

• Complexity add in both code and a users use of DefectDojo isn't worth this feature add
• Performance concerns especially when changing which SLA applies to which findings
• Lack of a significant desire for this change from the larger DefectDojo community based on feedback here and the prior PR.

We've given amble time for the community to weigh in on this and there's not been a noticeable push from the larger DefectDojo community for this feature add now (or in the prior PR). Those that have commented here have raised concerns about potential negative consequences of the proposed change. So, I think the next action to take is to unfortunately close this PR.

I appreciate your thoughtful replies to the comments on this and the prior PR. You've always done stellar work on DefectDojo and been a positive example to anyone contributing to an open source project. Please know that your professionalism has been noticed beyond just the core moderators to this project.