✨ SAST Check for TFSec #1981
✨ SAST Check for TFSec #1981
Conversation
naveensrinivasan
requested review from
azeemshaikh38,
justaugustus and
laurentsimon
as code owners
tfsec is the standard for validating terraform code security. It was requested in the Scorecard meeting 6/2/2022. Also github recommends these tools for code scanning https://github.blog/2021-07-28-new-code-scanning-integrations-open-source-security-tools/ Signed-off-by: naveensrinivasan <[email protected]>
naveensrinivasan
force-pushed
the
naveen/feat/tfsec
branch
from
1c6cfd3 to
d4dec7f
Compare
|
I am aware of there is an existing PR for SAST #1487 , but it has been WIP for almost 6 months. The community is looking for tools like these and it is important that we provide them with these tools. |
Codecov Report
@@ Coverage Diff @@
## main #1981 +/- ##
==========================================
+ Coverage 51.15% 51.22% +0.06%
==========================================
Files 85 85
Lines 6986 7030 +44
==========================================
+ Hits 3574 3601 +27
- Misses 3180 3195 +15
- Partials 232 234 +2 |
| @@ -25,7 +25,7 @@ import ( | |||
| // CheckSAST is the registered name for SAST. | |||
| const CheckSAST = "SAST" | |||
|
|
|||
| var sastTools = map[string]bool{"github-code-scanning": true, "lgtm-com": true, "sonarcloud": true} | |||
| var sastTools = map[string]bool{"github-code-scanning": true, "lgtm-com": true, "sonarcloud": true, "tfsec": true} | |||
There was a problem hiding this comment.
this is fine on the condition that the repo only contains these sort of files, ie it's not both a development repo and a deployment repo.
@nadgowdas can you confirm the above is correct? Could you also provide us with some example repos with terraform files? Is there a way to differentiate between the two "kinds" of repos?
There was a problem hiding this comment.
Sorr I don't understand. Why can't it have both?
There was a problem hiding this comment.
it may have both, but it won't be reflected in the check results in the current implementation. The check uses an OR to pass, see
Line 162 in d4dec7f
There was a problem hiding this comment.
Yes, I agree with @laurentsimon
I think we should have a pre-scanner that parses code from given repo and identifies applicable checks. E.g. if repo has .tf code, then we select tfsec to be applicable. If code contains both app code (e.g. .go and .tf, although unlikely) then we should select checks applicable for both types.
Also, if repo contains .yaml/helm charts then we could have provision to run tools like kubelinter.
I general, it can be made pluggable for any "kind" of artifact in the code repository. Thoughts ?
There was a problem hiding this comment.
makes sense to me. I suggested a similar approach in #1981 (comment): would be great if we can use the language API (https://docs.github.com/en/rest/repos/repos#list-repository-languages). If not we'll have to list files ourselves for .tf, etc.
Note: #1979 is adding support for language API query in the repo client.
fyi @aidenwang9867 (author of the PR above)
| @@ -25,7 +25,7 @@ import ( | |||
| // CheckSAST is the registered name for SAST. | |||
| const CheckSAST = "SAST" | |||
|
|
|||
| var sastTools = map[string]bool{"github-code-scanning": true, "lgtm-com": true, "sonarcloud": true} | |||
There was a problem hiding this comment.
this assumes that the tool is installed an a GH App. Is this how it works? I thought it was a CLI users need to run. if so, we may need to wait for #1487 to land.
Wdut?
There was a problem hiding this comment.
It is a GH Action. So will it be an issue? I don't think so.
There was a problem hiding this comment.
sorry I missed something when reading the PR. You can remove tfsec from the list. This list is only for GH apps.
The code tfsecCheckDefinitions() takes care of declaration of GH actions.
| @@ -25,7 +25,7 @@ import ( | |||
| // CheckSAST is the registered name for SAST. | |||
| const CheckSAST = "SAST" | |||
|
|
|||
| var sastTools = map[string]bool{"github-code-scanning": true, "lgtm-com": true, "sonarcloud": true} | |||
There was a problem hiding this comment.
sorry I missed something when reading the PR. You can remove tfsec from the list. This list is only for GH apps.
The code tfsecCheckDefinitions() takes care of declaration of GH actions.
| @@ -49,10 +49,16 @@ func SAST(c *checker.CheckRequest) checker.CheckResult { | |||
| return checker.CreateRuntimeErrorResult(CheckSAST, codeQlErr) | |||
| } | |||
|
|
|||
| tfsecScore, tfsecErr := tfsecCheckDefinitions(c) | |||
There was a problem hiding this comment.
if tfsecCheckDefinitions() succeeds, we're returning early. If repos with such files never have other type of code and don't need to have codeQL enabled, the code works. If it's possible they have both terraform code and other programming languages code, I think this will create false positive, ie high score even though dependabot is not enabled.
Wdut?
Is there a way to tweak the logic to accommodate both scenarios?
There was a problem hiding this comment.
Would using the language API help in any way https://docs.github.com/en/rest/repos/repos#list-repository-languages?
There was a problem hiding this comment.
I think that's a great idea to use that API 👍
|
Stale pull request message |
tfsecis the standard for validating terraform code security.It was requested in the Scorecard meeting on 6/2/2022.
Also, GitHub recommends these tools for code scanning https://github.blog/2021-07-28-new-code-scanning-integrations-open-source-security-tools/
Signed-off-by: naveensrinivasan [email protected]