Adds generic sbt node snyk workflow

[SHession]

What does this change?

This PR aims to add a generic Snyk workflow which could be used across multiple projects.

The workflow uses the snyk monitor --all-projects command, which does a reasonable job of identifying dependency manifests within a project and can be used in cases where it is able to identify all of them successfully (this isn't always the case).

I have added a DEBUG input which optionally adds the -d argument to the Snyk command, providing more helpful logging.

How to test

It should be possible to point other repo's workflows at this one and confirm it works as expected. One example might be this PR.

How can we measure success?

We can avoid duplication where possible and benefit from a single point of change across the projects which consume this workflow.

[mchv]

You should likely use Java 11rather than 8

[SHession]

Happy to make that change. Out of interest, what affect would you expect this to have on our results from Snyk results / SBT dependencies?

[mchv]

I am not certain why we need to setup java in the first place for snyk monitor, however if we do we need to set it up, it seems we should use the LTS java version we have moved to.

[SHession]

It's a good point, I've made that change now. In regards to the setup, I'll add some more context to the description but essentially using the Snyk setup action, we are responsible for setting up the environment in which our runner performs the Snyk CLI checks. So in this case we are setting up a Node and Java environment so we can check the dependencies of SBT and Node projects.

[jfsoul]

if there's a quick way to make this configurable (with a default of 11) that would be nice

[mchv]

@SHession I have some questions:

• Could you explain the benefits over using existing provided snyk actions?
• Should we uploading Snyk scan results to GitHub Code Scanning? Using --sarif-file-output Snyk CLI flag and the official GitHub SARIF upload action, we could upload Snyk scan results to the GitHub Code Scanning.

[SHession]

@mchv, thanks for the questions. There is important context missing from my description so I will make sure to update that and also add a comment to the top of the reusable workflow.

1. The benefit of using the setup action was around complexity with projects featuring multiple languages. Whilst implementing a Snyk workflow for a project using Node and Scala actions, we found that CLI was producing odd results. For example, in some cases different language actions would check the same manifests (i.e. a Scala check, checking only Node dependencies). The conclusion we drew was that the individual actions are designed to be used in isolation and not composed with others. Each language specific action will attempt to setup the necessary environment and then run the Snyk commands. This resulted in a solution like this. The version of the workflow featured in this PR attempts to setup the environment for out typical projects (SBT and Node) and run Snyk once for all languages and manifests.

2. That's an interesting idea and one I had not considered. I have not worked with GitHub Code Scanning before. My suggestion would be to explore that in another PR after this one is working as intended but I would be interested to learn more about GitHub Code Scanning.

[mchv]

Thanks @SHession this is helpful and that look sensible! Regarding the idea for 2, just mentioning @jfsoul and @markjamesbutler for reference as I know their team was looking at improving reporting of issues.

[jfsoul]

@mchv do we have a "Github Advanced Security" license? I can't currently see organisation overview of security, which code-scanning would feed into, so from that point of view we're not yet getting a benefit (at the org level) of using code scanning.

However, I know the dotcom team enabled uploads to code scanning, and I think there are some advantages in terms of visibility in co-locating security vulnerability information with your code, and in a tool we use every day like Github. We can look into potentially making it a recommendation later as @SHession suggested.

[jfsoul]

I see in https://github.com/guardian/workflow-frontend/pull/356/files we're setting this up to be called on push to any branch. I think the guidance in the past has been to run synk test for branches and synk monitor only on main, so that the continuously monitored application in snyk is main.

[SHession]

@jfsoul, this is a good point. Our workflows diverged from that recommendation due to failing checks against PRs which were nothing to do with the content of the PR. As many of our projects have outstanding vulnerabilities which need resolving, the test function would always provide a failing result. This was unhelpful and indicated an issue to developers unfamiliar with this process. As it represented a problem with overall codebase and not the PR specifically we felt it wasn't relevant to have as a PR check.

We therefore thought it would be preferable to exclude the test check and only run monitor, the results of which can be reviewed in the Snyk dashboard. It would be interesting to know how other teams addressed these concerns.

[jfsoul]

After discussing offline I realise now that workflow-frontend will actually only be running this workflow on pushes to main - we've just commented that restriction out for testing. I do think there's value in snyk test for at least some teams, but I think this is an area that needs more thought before we can form a solid recommendation.

[jfsoul]

I suppose this isn't just Node, so we might want to rename. Although I think at least in terms of clarity it might be preferable to separate the 2 actions?

[SHession]

What would you recommend we rename it to? My intention was to create a generic Snyk workflow which would match the general setup of our projects i.e. SBT with Node. If a project has one language then the existing Snyk actions could be used, unless we also wanted to create our own versions for reusability.

[jfsoul]

Sorry, I misread this initially so my comment isn't really valid.

[jfsoul]

Maybe something like Simple Snyk monitor for SBT + Node?

[jfsoul]

I haven't used --all-projects before, I wonder whether it'll be a bit prescriptive for more complex projects that might want to be specific about which manifests to monitor.

Relatedly, there's potentially other CLI flags we'd want to consider supporting like --org --target-reference etc.

[SHession]

all-projects has been an interesting one. As you mention it's not applicable in all cases such as flexible-content. My aim was to make a simple and generic case which would be useable across most projects. In cases where they differ, a manual workflow could be created as an alternative. My assumption is, you are trying to conceive of a way to cover the cases this wouldn't be able to cope with? If so, I think this is a good idea, but would consider this to still be a sensible starting point.

[jfsoul]

Just minor comments about name and Java version configurability. Thanks for doing this, as discussed let's see how this works in practice and iterate!

[SHession]

I have integrated @jorgeazevedo changes into the PR and made a few minor tweaks based off the comments in this PR. I have also noticed the debug flag is no longer resulting in useful output from the CLI. I suspect this is an issue with the latest version of the Snyk package, as I have confirmed this by tested locally with the current and an older version. It seems to have been introduced with this PR, I can raise an issue if it persists.

EDIT: Investigating the difference between composite actions and reusable workflows, they do appear to be very similar. We could change this to a reusable action in a new repo if we thought it was a better fit.

[jorgeazevedo]

tested in security hq and it works fine @SHession , thanks for bringing my changes in as well!