awslabs / threat-composer

A simple threat modeling tool to help humans to reduce time-to-value when threat modeling
https://awslabs.github.io/threat-composer/
Apache License 2.0
468 stars 54 forks source link

[FEATURE] Ability to record the status of threats #126

Closed arivra closed 2 months ago

arivra commented 3 months ago

Describe the feature

I would like to be able to capture the status of any threats that are documented within threat composer

Use Case

We would like to track threats that are identified during modeling. To do this, we want to bring these threats to Defect Dojo (it's like an ASPM or defect repository) as defects of a product. Our idea is to make a Defect Dojo parser that reads the threat-composer json and creates as many defects as there are detected threats. The mapping would be more or less:

To know if a threat, and therefore a defect, has been mitigated (or not) we need its status.

Proposed Solution

Add a new "Status" field against each threat with the following values:

Other Information

I don't think there should be a direct relationship between the status of the mitigations and the status of the threat. It could be that even if the mitigations were implemented, the entire threat would not be covered.

Acknowledgements

dboyd13 commented 2 months ago

The following is a combined proposal for both the ask for Mitigation status (#61) and Threat status (#126) given the interplay between them.


Ask: @arivra for your feedback (and anyone else reading this) on if the below proposal would address your use case.


Broad design philosophy and decisions

We propose the tool will:

  1. Focus on capturing outcome states for threats (risks) and mitigations (actions). In our experience, the combination of interim states, their associated business logic and risk management making process is highly customer specific.
  2. Have static system defined statuses to ensure portability of exports (.tc.json) between deployments, and to maximise the usefulness of any associated 'Insight dashboard' widgets that would be created.
  3. Allow for a degree of organisational customisation of the status, support change of the status "title" and "description" shown in the UI. Note this is applicable only for self-hosted and deployed environments.
  4. Have human readable (english only) values for the static system defined statuses that are human readable (english only) - example: threatIdentified, such that it useful for diffs/PRs for those that use version control methods (which we encourage)
  5. Not track (or of keep an audit log of) status changes. Given the client-side nature of the tool, we do not see this tool as a viable alternative for a workflow tracking or ITSM tool. Additionally, we need to be frugal with our data storage requirements within browser storage, and ensure .tc.json diffs do not become "noisy".

Threat status

Mental model

Proposed statuses

Status in .tc.json export (constant) UI Label (can be changed in build) UI Description (can be changed in build) Comments
threatIdentified "Indentified" (default) - Other possible values you may want in your build could include: "Open", "Active" etc "Potential threat has been identified" All newly added threats get this status
threatResolved "Resolved" (default) - Other possible values you may want in your build include: "Mitigated", "Complete" etc "All agreed risk response actions (mitigation, transfer, avoidance, risk acceptance) have been completed aligned to our risk tolerance" As noted above there is always amount of residual risk acceptance (unless one avoids the risk) - hence we will not include a seperate status for "Risk Accept" it will form part of "Resolved"
threatResolvedNotUseful "Not Useful" (default) - Other possible values you may want in your build could include: "False positive" etc "This threat is invalid / not applicable to our use system" We consider "Resolved" and "NotUseful" as both being outcome state - so both will include the term "Resolved" in their system defined code threatResolved and threatResolvedNotUseful respectively.

UI placement

Screenshot 2024-08-02 at 9 06 35 AM

Business logic

Dashboard

Mitigation status

Mental model

Proposed statuses

Status in .tc.json export (constant) UI Label (can be changed in build) UI Description (can be changed in build) Comments
mitigationIdentified "Indentified" (default) - Other possible values you may want in your build could include: "Idea", "Planned", "Backlog" "Mitigation has been identified" All newly added mitigations get this status
mitigationInProgress “In-Progress" (default) - Other possible values you may want in your build include: "Doing" "Mitigation is in-progress"
mitigationResolved “Resolved" (default) - Other possible values you may want in your build could include: "Implemented", "Complete","Accepted" etc “Mitigation has been completed to an agreed scoped” We do not have the notion of "Partially Resolved" as we see that as" Resolved" where the mitigation has been completed to an agreed scope. Meaning if partial... that was agreed. We capture the outcome only.
mitigationResolvedAbandoned "Abandoned" (default) - Other possible values you may want in your build could include: "Won't do" etc "Will not be implementing this mitigation"

UI placement

Screenshot 2024-08-01 at 4 59 29 PM

Business logic

Dashboard

climbertjh2 commented 2 months ago

Overall, this proposal seems reasonable to me.

One comment is that I do think that the dashboard could flag threats that are marked Resolved (either Resolved or NotUseful) when one more of the mitigations associated with the threats is/are still in Identified or InProgress (i.e. not all associations are in either Resolved or Abandoned). My logic here is that if there is a mitigation identified - then it should be dispatched/considered/reasoned-about enough to get it to Resolved or Abandoned before treating the threat as Resolved.

As another consideration - you could consider noting in the dashboard any mitigations that are not Abandoned and associated with threats that are marked NotUseful. This might point to work or potential work that could be avoided (save time, money). I realize that this is probably an UNcommon situation but figured I'd mention it.

arivra commented 2 months ago

Thank you so much @dboyd13

If I have understood correctly, the proposal you indicate would include a dict with "key": "Status" as a new element in the "metadata" list and with three possible "value": threatIdentified, threatResolved or threatResolvedNotUseful.

  "threats": [
    {
      "id": "7b49bdbd-25e9-446f-b44a-46fa2807e182",
      "numericId": 20,
      "displayOrder": 20,
      "metadata": [
        {
          "key": "Priority",
          "value": "Low"
        },
        {
          "key": "STRIDE",
          "value": [
            "I"
          ]
        },
        {
          "key": "Status",
          "value": "threatIdentified"
        }
      ],
...
    },
    {
      "id": "782adefc-b480-4e8f-83ee-64f6d680df0a",
      "numericId": 19,
      "displayOrder": 19,
      "metadata": [
        {
          "key": "Priority",
          "value": "Medium"
        },
        {
          "key": "STRIDE",
          "value": [
            "D"
          ]
        },
        {
          "key": "Status",
          "value": "threatResolved"
        }
      ],
...    
    },
    {
      "id": "a0f523b0-bbc0-4e6a-9064-500af1f3836e",
      "numericId": 18,
      "displayOrder": 18,
      "metadata": [
        {
          "key": "STRIDE",
          "value": [
            "T"
          ]
        },
        {
          "key": "Priority",
          "value": "Low"
        },
        {
          "key": "Status",
          "value": "threatResolvedNotUseful"
        }
      ],
...
    }

It would perfectly cover what was requested, except for the accepted risk part, but it is not a problem because I could cover it by putting a label that would be taken into account when "Status" had the value "threatResolved", and I think that when it is put in, the justification in "Comments" would be filled in. I understand what you say in:

As noted above there is always amount of residual risk acceptance (unless one avoids the risk) - hence we will not include a separate status for "Risk Accept" it will form part of "Resolved"

but what I am looking for is precisely for those grey threats and for accountability.

Also, what I understand is that there would be constants for the labels that could change, so based on the initial proposal it could be:

export const OPTIONS = [ { label: 'Active', value: 'threatIdentified' }, { label: 'Mitigated', value: 'threatResolved' }, { label: 'False Positive', value: 'threatResolvedNotUseful' }, ];

That's right, isn't it?

dboyd13 commented 2 months ago

Thanks @arivra

If I have understood correctly, the proposal you indicate would include a dict with "key": "Status" as a new element in the "metadata" list and with three possible "value": threatIdentified, threatResolved or threatResolvedNotUseful.

The possible values you've listed is correct. What we have not decided on is if the status should be a key/value pair under metadata (as you've described) or it's own key called status one level up (at the same level as id, metadata etc). We are leaning towards the latter, any feedback on this?

It would perfectly cover what was requested, except for the accepted risk part, but it is not a problem because I could cover it by putting a label that would be taken into account when "Status" had the value "threatResolved", and I think that when it is put in, the justification in "Comments" would be filled in.

Acknowledged, and agree that tagging (or label as you referred to it) together with commentary within the comments field is a suitable way forward that should work in your scenario, and other users with custom needs.

Also, what I understand is that there would be constants for the labels that could change, so based on the initial proposal it could be: export const OPTIONS = [ { label: 'Active', value: 'threatIdentified' }, { label: 'Mitigated', value: 'threatResolved' }, { label: 'False Positive', value: 'threatResolvedNotUseful' }, ];

Yes, there will be constants for status within the .tc.json exports (e.g. threatIndentified), and there will be default labels and descriptions for the UI (e.g. "label":"Identified", "description": "Mitigation has been identified") that could be changed in source code for those that self-deploy.

Overall my impression is that you are satisfied with this proposal, is that correct?

dboyd13 commented 2 months ago

Thanks @climbertjh2

One comment is that I do think that the dashboard could flag threats that are marked Resolved (either Resolved or NotUseful) when one more of the mitigations associated with the threats is/are still in Identified or InProgress (i.e. not all associations are in either Resolved or Abandoned). My logic here is that if there is a mitigation identified - then it should be dispatched/considered/reasoned-about enough to get it to Resolved or Abandoned before treating the threat as Resolved.

As another consideration - you could consider noting in the dashboard any mitigations that are not Abandoned and associated with threats that are marked NotUseful. This might point to work or potential work that could be avoided (save time, money). I realize that this is probably an UNcommon situation but figured I'd mention it.

Both good suggestions. Thank you.

arivra commented 2 months ago

The possible values you've listed is correct. What we have not decided on is if the status should be a key/value pair under metadata (as you've described) or it's own key called status one level up (at the same level as id, metadata etc). We are leaning towards the latter, any feedback on this?

Both options are good, although removing it from the 'metadata' saves us a click to open the metadata dropdown, so it is probably better to leave it out of 'metadata'.

Overall my impression is that you are satisfied with this proposal, is that correct?

Yes, that's correct, thank you @dboyd13 !

github-actions[bot] commented 2 months ago

:tada: This issue has been resolved in version 1.0.57 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: