Add tflint to Terraform Lint, Add trivy workflow
#74
Conversation
There was a problem hiding this comment.
I like the idea of using trivy. I don't like the currently proposed rollout strategy. Per https://aquasecurity.github.io/trivy/v0.19.2/misconfiguration/comparison/tfsec/:
tfsec is designed for Terraform. People who use only Terraform should use tfsec. People who want to scan a wide range of configuration files should use Trivy.
This implies to me that trivy isn't a 1-1 replacement for tfsec, and may flag issues much more broadly than tfsec would. As such, I recommend put trivy in its own independent, new workflow, in parallel with the current tfsec workflow.
A separate workflow has the advantage of a staged rollout, where trivy can be enabled without being required immediately. This gives us a chance identify and address any new failures it reports without scatterblasting the actual work of trivy compliance onto unsuspecting employees who happen to touch the files next.
Once we're happy with Trivy, we can then remove tfsec.
My understanding is that this is outdated. They just announced two days ago that tfsec is deprecated and that everybody should move to trivy. See aquasecurity/tfsec#1994 (forgot to include this URL earlier).
Can definitely add it as a new workflow on If it works the same way as tfsec, existing failures should be fine. It should only be complaining about new failures. We've been having lots of issues with tfsec not catching stuff lately, so I'm hopeful that trivy will solve that. From the local testing I did, it does seem to correctly look into terraform modules and test things there as well, which is what we want. |
I care the most about
It isn't the terraform I'm as worried about. It is the various other things that trivy lints that (afaik) have had little no prior linting. For instance, will it block PRs that touch non-compliant dockerfiles or helm charts? I know it should be ok if that infra is untouched, but I don't yet trust it to be smart enough to handle the various edge cases. I'd like to see it run for a week or two first before becoming blocking. |
Upon further investigation, it doesn't look like tfsec is enforced in |
Does
It does look like requirements for cloud were missed somehow (my fault). I just submitted a PR to add these jobs as required: https://github.com/gravitational/github-terraform/pull/434 Thank you! |
I'm assuming for the Terraform modules: https://github.com/gravitational/teleport/tree/master/examples/aws/terraform. |
jof
force-pushed
the
reed/terraform-lint-v2
branch
from
ba35116 to
edf4063
Compare
|
Test scan against gravitational/teleport: https://github.com/gravitational/teleport/actions/runs/4328529117/jobs/7558312852 |
There was a problem hiding this comment.
It looks like this removes the tfsec job. We can't have tfsec required if we're going to remove it:
I'd recommend we leave tfsec alone in this PR, and keep the changes purely additive. We can come back and remove tfsec later, once repos no longer rely on it. Right now if this merges, there will be a bunch of folks who are blocked -- unable to merge because an expected check isn't running.
Can you also introduce a failure in a run? Our issue with Why do I say this? When I run trivy against teleport, I'm getting a range of high and critical errors in the repo. I know we won't detect latent stuff on PR runs, but I would like to make sure we detect new stuff. Related: should we run a one-off sarif import of issues that trivy flags that are latent in the repo? For context -- it was errors like the above that trivy flags and tfsec doesn't that caused me to pump the breaks in #74 (review). Swapping from tfsec to trivy means this isn' a terraform specific check anymore. I'd recommend putting it in its own workflow file instead of nested under |
jof
force-pushed
the
reed/terraform-lint-v2
branch
4 times, most recently
from
2dc132a to
bb1ea08
Compare
jof
force-pushed
the
reed/terraform-lint-v2
branch
2 times, most recently
from
96e21a6 to
22a4a8a
Compare
|
Renamed as I made some test runs against PRs in gravitational/cloud-terraform and gravitational/teleport. These workflows seem to work, but Trivy flags a whole bunch of latent issues, and also comments about things in PRs that weren't touched in the PR, which is a bit of a behavior change as compared to the I'm thinking that we should keep using a test branch like these testing PRs to see the latent issues, and then iterate on adding ignore lines or fixes to reduce the latent issue backlog. We can also just land in this change which makes a bunch of findings in the Trivy workflow, then work repo by repo to apply the Workflow into the repo, and accept/wontfix the findings it generates. Then, at least, we can flag new issues reliably while still having a backlog of latent issues we can go back and address. |
https://github.com/aquasecurity/tfsec/blob/master/docs/guides/trivy.md Co-authored-by: Jonathan Lassoff <[email protected]>
jof
force-pushed
the
reed/terraform-lint-v2
branch
from
22a4a8a to
b6175a9
Compare
|
I think this is in a mergable state where it sets up the workflow we can start pointing to, without changing downstream users of Once merged, we can then work, repo-by-repo in increasing order of visibility/impact/publicness to add this new workflow, triage the findings and set that state in GitHub, and then once in a good and steady state, set this new workflow as a requirement before merging. |
jof
changed the title
tflint and trivytflint to Terraform Lint, Add trivy workflow
https://github.com/aquasecurity/tfsec/blob/master/docs/guides/trivy.md
tfsec is deprecated: aquasecurity/tfsec#1994