976 - Introduce Security Scans for Go Packages #1053

BradleyBoutcher · 2019-12-19T19:52:59Z

What does this PR do (include background context, if relevant)?

A new bash script which runs gosec on our packages

We only flag issues of high severity with 'medium' or 'high' confidence by Gosec
Gosec only scans directories modified by checking the Git diff first. If the branch is master, it scans the entire project. This way we save time in our pipeline while developing.
Finally, the reports are exported as xml and parsed using Junit.

What ticket does this PR close?

Connected to #976 and #988

Where should the reviewer start?

bin/build_security_scan

What is the status of the manual tests?

NA

Links to open issues for related automated integration and unit tests

NA

Links to open issues for related documentation (in READMEs, docs, etc)

NA

hughsaunders

This looks great, thanks :D

Looking at the pipeline in blueocean, it struck me that the scanners (secretless image, go packages) could be parallelised as neither depends on the result of the other. That also groups them into a higher level security stage.

I noticed in the log that the xml doc is empty (apart from the xml declaration), is that the expected behaviour when there are no issues? Have you introduced a deliberate security issue to check it shows up in the Jenkins Test Results view?

BradleyBoutcher · 2019-12-20T18:35:24Z

@hughsaunders Thanks for the review! Addressing your comments in order:

It absolutely can be parallelized, I'll make a few changes and re-submit it.
So far I've tested locally and made sure that issues are detected and output correctly, but currently there are none in our repository that meet the criteria for an issue. When there's no issues, the log should be empty.
On a related note, I realized there is an issue. Since we're scanning the git diff, we won't see which files are changed in that commit since technically nothing has been changed locally. I'll have to make a change there for sure.

izgeri

Looking forward to getting this integrated :) left a few comments - I think this could be simplified a bit by combining the scripts and the naming could be clearer

It would also be nice to clean up the pipeline stages for the tests in the Jenkinsfile - they're a little inconsistently labeled right now and it looks odd in blue ocean. But that's optional.

izgeri · 2019-12-20T19:57:18Z

Jenkinsfile

@@ -31,6 +31,13 @@ pipeline {
      }
    }

+    stage('Scan Go Packages') {
+      steps {
+        sh "./bin/build_security_scan -s High -c 'Medium' -b ${env.BRANCH_NAME}"


Can the filename be changed to run_gosec or similar? It would be clearer what it's actually doing

Looking at the other bin/ files, it looks like we use the 'check_' format for file names. So for golint, it's check_style. How about for this we use 'check_security'?

is it your intention that the file becomes a general script to run security checks, so that when we have another we'd like to run we also do it from this file?

I'm still not sure I like check_security - security means a lot of different things to different people, so if we can find a way to be more clear I think that'd be good

bin/build_security_scan

izgeri · 2019-12-20T19:59:58Z

bin/security_scan

+    echo "directories regardless of what has been modified locally."
+    echo " "
+    echo "Format:"
+    echo "security_scan [arguments]"


Can this be integrated into the other file? I'm not sure I see the need for two separate files in bin to run this. If I'm missing something, please lmk

Since we wanted to run it within a docker container, I thought it would be easiest to have two separate files. I'm not very familiar with bash, but I assume that's the easiest way to run a large script like this inside a docker run command? Also, I thought it would be useful to have the script be able to run without using a docker container.

ok, so there are two scripts because one is meant to be run in a docker container (which docker container?) and the other one actually runs the the container?

I feel like there might be some patterns for how to name and organize these that would make their intended use more clear, but I'm not close enough to the code lately to remember exactly what to point you to. @sgnn7 @jonahx do you have any examples of how we've done something like this in other cases? in particular, I'm concerned that if I look at the bin/ dir, it's not going to be obvious which script I should run to do a gosec scan if we have two with similar names. I don't think this is the first time we've had to think about something like this.

Currently I have them named as such:

check_security: performs the gosec scan with whatever parameters you need

run_check_security: creates a docker container with secretless mounted as a volume, and runs check_security on the volume
run_gosec_container may be clearer for the second one, and maybe something like run_gosec for the first?

Jenkinsfile

sgnn7

@BradleyBoutcher just some minor nits

bin/check_security

bin/run_check_security

diverdane

Looks good. Just a couple of minor suggestions.

bin/check_security

bin/run_check_security

bin/check_security

Comments addressed

izgeri

@BradleyBoutcher what do you think about check_golang_security for the one that runs in the pipeline and run_gosec for the local runner? those still sound similar enough, though - each script should have comments at the top of the file or usage instructions that are clear about the purpose of each file

A new bash script which runs gosec on our packages We only flag issues of high severity with 'medium' or 'high' confidence by Gosec Gosec only scans directories modified by checking the Git diff first. If the branch is master, it scans the entire project. This way we save time in our pipeline while developing. Finally, the reports are exported as xml and parsed using Junit.

izgeri · 2020-01-03T18:25:50Z

bin/run_gosec

@@ -0,0 +1,69 @@
+#!/usr/bin/env bash


as a follow-on to this PR, does it make sense to include this as a bash-lib script? cc @hughsaunders

diverdane

LGTM

BradleyBoutcher force-pushed the 976-security-scan branch 2 times, most recently from 2fe89c9 to adb87d1 Compare December 19, 2019 20:08

hughsaunders reviewed Dec 20, 2019

View reviewed changes

izgeri previously requested changes Dec 20, 2019

View reviewed changes

BradleyBoutcher force-pushed the 976-security-scan branch 14 times, most recently from 003aead to b5fb744 Compare January 2, 2020 19:08

BradleyBoutcher self-assigned this Jan 2, 2020

sgnn7 previously requested changes Jan 2, 2020

View reviewed changes

bin/check_security Outdated Show resolved Hide resolved

bin/check_security Outdated Show resolved Hide resolved

bin/check_security Outdated Show resolved Hide resolved

bin/check_security Outdated Show resolved Hide resolved

bin/run_check_security Outdated Show resolved Hide resolved

BradleyBoutcher force-pushed the 976-security-scan branch 2 times, most recently from f49d4ed to 4e6ba7a Compare January 2, 2020 19:49

BradleyBoutcher requested review from sgnn7 and izgeri January 2, 2020 19:50

diverdane reviewed Jan 2, 2020

View reviewed changes

bin/check_security Outdated Show resolved Hide resolved

bin/run_check_security Outdated Show resolved Hide resolved

bin/check_security Outdated Show resolved Hide resolved

bin/check_security Outdated Show resolved Hide resolved

bin/check_security Outdated Show resolved Hide resolved

BradleyBoutcher force-pushed the 976-security-scan branch 2 times, most recently from c3d9105 to 75053ab Compare January 3, 2020 15:59

BradleyBoutcher force-pushed the 976-security-scan branch from 75053ab to af26632 Compare January 3, 2020 16:42

izgeri reviewed Jan 3, 2020

View reviewed changes

BradleyBoutcher force-pushed the 976-security-scan branch 5 times, most recently from c7515d0 to 7eec834 Compare January 3, 2020 18:05

BradleyBoutcher force-pushed the 976-security-scan branch from 38bf4e2 to aacac91 Compare January 3, 2020 18:16

Merge branch 'master' into 976-security-scan

2493e1e

izgeri reviewed Jan 3, 2020

View reviewed changes

Merge branch 'master' into 976-security-scan

6f34f03

diverdane approved these changes Jan 3, 2020

View reviewed changes

BradleyBoutcher merged commit 2dfd040 into master Jan 3, 2020

BradleyBoutcher deleted the 976-security-scan branch January 3, 2020 21:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

976 - Introduce Security Scans for Go Packages #1053

976 - Introduce Security Scans for Go Packages #1053

BradleyBoutcher commented Dec 19, 2019

hughsaunders left a comment

BradleyBoutcher commented Dec 20, 2019

izgeri left a comment

izgeri Dec 20, 2019

BradleyBoutcher Jan 2, 2020

izgeri Jan 3, 2020

izgeri Dec 20, 2019

BradleyBoutcher Jan 2, 2020 •

edited

izgeri Jan 3, 2020

BradleyBoutcher Jan 3, 2020 •

edited

sgnn7 left a comment

diverdane left a comment

izgeri left a comment

izgeri Jan 3, 2020

diverdane left a comment

976 - Introduce Security Scans for Go Packages #1053

976 - Introduce Security Scans for Go Packages #1053

Conversation

BradleyBoutcher commented Dec 19, 2019

What does this PR do (include background context, if relevant)?

What ticket does this PR close?

Where should the reviewer start?

What is the status of the manual tests?

Links to open issues for related automated integration and unit tests

Links to open issues for related documentation (in READMEs, docs, etc)

hughsaunders left a comment

Choose a reason for hiding this comment

BradleyBoutcher commented Dec 20, 2019

izgeri left a comment

Choose a reason for hiding this comment

izgeri Dec 20, 2019

Choose a reason for hiding this comment

BradleyBoutcher Jan 2, 2020

Choose a reason for hiding this comment

izgeri Jan 3, 2020

Choose a reason for hiding this comment

izgeri Dec 20, 2019

Choose a reason for hiding this comment

BradleyBoutcher Jan 2, 2020 • edited

Choose a reason for hiding this comment

izgeri Jan 3, 2020

Choose a reason for hiding this comment

BradleyBoutcher Jan 3, 2020 • edited

Choose a reason for hiding this comment

sgnn7 left a comment

Choose a reason for hiding this comment

diverdane left a comment

Choose a reason for hiding this comment

izgeri left a comment

Choose a reason for hiding this comment

izgeri Jan 3, 2020

Choose a reason for hiding this comment

diverdane left a comment

Choose a reason for hiding this comment

BradleyBoutcher Jan 2, 2020 •

edited

BradleyBoutcher Jan 3, 2020 •

edited