3 show a summary of the attack graph for a consequence in the web UI

[panositi]

No description provided.

[kenmeacham]

I've checked out the branch and tested SSM. All seems generally fine with the new functionality. I confirmed that you can get a different attack path on different consequences and that the results are cached. Also, results are cleared when running risk calc, so that's good. No major issues that I can see.

For the code itself, I've been through the code changes and made comments on various files. Nothing major - generally a bt of tidying up, removing commented out code, fixing copyright headers, etc.

I have not looked at the attack path algorithm code in great detail, but quick scan looks OK.

[scp93ch]

SonarLint highlights many cases of defined but unused variables and methods which should be removed.
There are unused imports which should be removed (e.g. in LogicalExpression.java).
There is also quite a lot of unnecessary commented out code so please see what could be deleted there.
It would be nice to have more consistent formatting in the new code at least (so for example many cases of "for(" rather than "for (" (the later is to be used surely). Perhaps you can run the new files through a basic code formatter?

[scp93ch]

Minor points:

• ThreatsPanel.js line 46 has some weird white-space characters at the start of the line
• the button for "Calculate Attack Path" is prefixed by an icon which does not fit the style of any other button so I suggest it is removed

More serious:

• From the server logs I can see that the attack path calculated is the current risk one and not shortest path.

The default risk calculation mode is the future risk so if we are to have just one button for the attack path then we should be consistent and the attack graph calculation should also be future risk.
I suppose for now, we actually need two buttons, one for current and one for future, which is not ideal. We need to revisit the idea of having a toggle button for current/future risk mode.

Not sure why we're not calculating the shortest path tree?

[panositi]

The exposed API for shortest path takes two parameters:

• allPaths
• normalOperations

The UI button for "Calculate Attack Path" invokes the shortest path calculation with allPaths=TRUE and normalOperations=FALSE.

Internally the AttackTree object is initialised with futureRisk=FALSE mode. This parameter should be either exposed to the API and a button consequently, or detect automatically the last valid risk calculation mode.

As a temporary fix futureRisk will be fixed to FUTURE mode.

[scp93ch]

Following further team discussions, the following strategy was agreed:

• The U/I client should store and (ideally) display the core#riskCalculationMode value ('CURRENT' or 'FUTURE') from the risk calculation response message.
• When the user presses the button to get a threat graph for a selected misbehaviour, the U/I client should use the saved value for core#riskCalculationMode as an argument. That way it is asking for a threat graph corresponding to the displayed likelihood levels, by using the same risk calculation mode that led to those displayed likelihoods.
• When the service receives the request for a threat graph calculation, it should check if there is a saved risk calculation, and if so, whether it has the requested risk calculation mode. If not, it should return an error (possible a 400) along with a textual message explaining that a saved risk calculation is a prerequisite.
• The attack graph endpoint should be renamed to "threatgraph".
• The web client should request the shortest paths, not all paths.

To avoid the situation where the web UI gets a threat graph calculation error:

• The U/I client buttons for starting a risk calculation should by default use the option to save the results.
• The attack graph button should be disabled if there is no saved risk calculation (i.e. if the risk calculation button is not green). Ideally, the disabled button would have a tooltip explaining that a saved risk calculation was required.

It is not necessary to change the RiskCalculator behaviour: we do not need to clear the results of any previous risk calculation from the triple store before loading the model for a new calculation.

[mike1813]

I suggest we also store (though not display) the timestamp for the last saved risk calculation in the U/I, and include this as an argument. That way if there is a saved risk calculation at the service, system-modeller can check if it is the same one the client is using.

This could be done instead of checking the risk calculation mode. If the timestamp doesn't match, then we return the error, as described by Stephen. If the timestamp does match, then the risk calculation mode should also match.

The advantage this brings is that it detects the case where a different client runs a saved risk calculation after the one that requested the attack graph analysis. If this other client changed risk calculation inputs (e.g., control selection or coverage, or assumed TW levels), then the attack graph wouldn't be correct for the displayed likelihood data.

The timestamp we would need is a property of the system model graph (http://purl.org/dc/terms/modified). It is added in the risk calculator. This is already included in the ModelDB class as variable 'modified', but it is not set until the very end of the risk calc, after the results have been returned to the client. That should be an easy fix, though.

This extension could be done separately, if desired. It should be a minor change on what @scp93ch proposed above.

[mike1813]

Just checked - it seems (based on code comments) that http://purl.org/dc/terms/modified is not deserialized correctly by the ModelDB class. I still think the timestamp should be used, but it should be added in a separate update, after any issues within ModelDB have been checked and fixed.

[scp93ch]

The new code still has some issues but it's well worth getting it merged in as it's a great new feature. 👍