izar / pytm

A Pythonic framework for threat modeling
Other
861 stars 161 forks source link

Add Assumption to Elements #245

Closed Dakes closed 3 weeks ago

Dakes commented 1 month ago

Fixes #244 This adds an Assumption class to Elements, as described in the Issue #244 by @raphaelahrens

It allows someone to set a list of assumptions for elements, which also allow (optionally) to exclude a list of SIDs.

All Findings, that match one of the SIDs, will not be included in the findings list. Instead there is a new list excluded_findings on the TM class. This allows the excluded threats to be shown in a separate section in the report.

To demonstrate this, I also added this feature to the reference tm.py threat model and added a new section to the advanced template.

I am not sure about the html files in the docs folder. I noticed there are make commands to update them, but they weren't updated in along while so I wasn't sure, whether to include them. But just let me know, if that was wrong and I will remove them from the Pull request.

izar commented 1 month ago

This looks great, thanks! Can you please add tests to validate the new class ?

raphaelahrens commented 1 month ago

@izar does this PR replace the excluded flag or do you see a value in having both?

izar commented 1 month ago

@raphaelahrens really good question. I like this one because of the flexibility and cleanliess of the approach. Do you think we need both? Do you see a case to be made of one instead of the other?

raphaelahrens commented 1 month ago

@raphaelahrens really good question. I like this one because of the flexibility and cleanliess of the approach. Do you think we need both? Do you see a case to be made of one instead of the other?

Funny that you throw the ball back to me :laughing:. I think this solution makes the --exclude flag less useful, maybe useless.

--exclude is an all or nothing approach, and then the question is when do you need to exclude a whole threat. The only thing for which it might be useful is when a threat is useless or at least the user considers it invalid.

Maybe in the case with AC22 it was useful, to exclude it while it was still in there. But for pytm it was much better to create an issue and complain, than just to exclude it.

And as always there is backwards compatibility.

izar commented 1 month ago

I think --exclude is still useful, as in cases where a mitigation is in place that invalidates a class of threats.

On Tue, May 28, 2024 at 2:36 PM Raphael Ahrens @.***> wrote:

@raphaelahrens https://github.com/raphaelahrens really good question. I like this one because of the flexibility and cleanliess of the approach. Do you think we need both? Do you see a case to be made of one instead of the other?

Funny that you throw the ball back to me 😆. I think this solution makes the --exclude flag less useful, maybe useless.

--exclude is an all or nothing approach, and then the question is when do you need to exclude a whole threat. The only thing for which it might become useful is when a threat is useless or you consider it invalid.

Maybe in the case with AC22 it was useful, to exclude it while it was still in there. But for pytm it was much better to create an issue and complain, than just to exclude it.

And as always there is backwards compatibility.

— Reply to this email directly, view it on GitHub https://github.com/izar/pytm/pull/245#issuecomment-2135884606, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC2BAPKJS5KPNRPF2GTYW3ZETFENAVCNFSM6AAAAABINFRW46VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMZVHA4DINRQGY . You are receiving this because you were mentioned.Message ID: @.***>

Dakes commented 1 month ago

tm.assumptions is currently a list of strings. Would it make sense to change it to be a list of Assumption? I think it would make sense, but would also break backwards compatibility. But it should be possible to make varAssumptions, to automatically create Assumptions from strings and return an Assumptions list. This would keep backwards compatibility. At least for the threat model code. Templates would still need adjusting.

raphaelahrens commented 1 month ago

Mhh the logic of tm.assumptions would with the current logic not benefit from the change to a list[Assumption]. Since there is no threat for the whole TM. For this to make sense it would need to integrate into the logic of --exclude Or add extras logic for the TM.assuptions.

Dakes commented 1 month ago

I now changed the Assumptions on the TM class to use the Assumption class in a way, that is 100% backwards compatible to existing threat models. It also will exclude threats globally, if it includes SIDs to exclude. (But still list them in the Excluded Section)