Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Integrate gosec, a static code vulnerability checker: #29

Merged
merged 5 commits into from
Jan 31, 2019
Merged

Integrate gosec, a static code vulnerability checker: #29

merged 5 commits into from
Jan 31, 2019

Conversation

xolaz
Copy link
Contributor

@xolaz xolaz commented Jan 7, 2019

  • enforced scanner
  • scans any project with .go file, go.mod, go.sum, and Gopkg.lock inside of $GOPATH
  • created symlink to successfully run gosec against $GOPATH projects
  • has tests

gosec: https://github.com/securego/gosec

@xolaz xolaz requested a review from jborrey January 7, 2019 06:31
xolaz
xolaz commented Jan 7, 2019
View reviewed changes
Dockerfile.tests Outdated
RUN bundle install --with test

RUN rm ./spec/docker/docker_spec.rb && rm ./spec/static_analysis/rubocop_spec.rb
RUN rm ./spec/docker/docker_spec.rb
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rm ./spec/static_analysis/rubocop_spec.rb kept failing stating file doesn't exist, I can add it back in if necessary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm yeah that doesn't exist anymore thanks to the new CircleCI flow. Isn't this container built in the tests? How come this was only noticed now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure, it happened as I was building the container locally. Could the image settings be different on circleci?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hit this too. AFAICT we're just not running the in-container tests (and hence not building Dockerfile.tests) in CI, which is why we hadn't noticed the issue.

If anything I feel like the in-container tests are more useful since they're running against the real versions of our dependencies (yarn, go, sift, etc.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the Dockerfile.tests really useful as well. Noting this as an improvement.

jborrey
jborrey reviewed Jan 7, 2019
View reviewed changes
lib/salus/repo.rb
@@ -13,6 +13,8 @@ class Repo
{ handle: :npmrc, filename: '.npmrc' },
# Go
{ handle: :dep_lock, filename: 'Gopkg.lock' },
{ handle: :go_mod, filename: 'go.mod' },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add comments about what these files are?
Helps for the less Go familiar folk.
I should have done the same for the others.

jborrey
jborrey reviewed Jan 7, 2019
View reviewed changes
lib/salus/scanners/gosec.rb Outdated
# - -fmt=json for JSON output
# - gosec only successfully scans repos within $GOPATH, we force the
# go path reference via symlink within Dockerfile & Dockerfile.tests
if ENV['RUNNING_SALUS_TESTS'] == "true"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we avoid this branching if we add /home/repo to GOPATH instead of symlinking it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the digging and testing I did, it looks like we are stuck with this solution until gosec figures how to support go modules (aka until gosec stops requiring we run repo against a $GOPATH). reference: securego/gosec#234 (comment)

jborrey
jborrey reviewed Jan 7, 2019
View reviewed changes
spec/lib/salus/scanners/gosec_spec.rb Outdated
context 'go project with no known vulnerabilities' do
let(:repo) { Salus::Repo.new('spec/fixtures/gosec/safe_goapp') }

it 'should record failure and record the STDOUT from gosec' do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copy-paste error from above

@jborrey
Copy link
Contributor

jborrey commented Jan 7, 2019

Few comments - looking pretty good!

@xolaz xolaz mentioned this pull request Jan 15, 2019
Closed
tutasla
tutasla reviewed Jan 21, 2019
View reviewed changes
lib/salus/scanners/gosec.rb Outdated
if ENV['RUNNING_SALUS_TESTS'] == "true"
shell_return = run_shell("gosec -fmt=json /go/src/repo/#{@repository.path_to_repo}")
else
shell_return = run_shell("gosec -fmt=json /go/src/repo")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to recursively scan from this directory if there are multiple folders within a project (via /go/src/repo/...)? If so, there might be a problem because symlinks seem to be skipped (securego/gosec#203).

My only workaround for recursive scan was to mount a directory under /go/src/ and specify it to salus with -d option. Hope this helps.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will try 👍

@xolaz
Copy link
Contributor Author

xolaz commented Jan 31, 2019

#36 patches failing circle tests

xolaz added 4 commits January 31, 2019 10:20
@xolaz
Integrate gosec, a static code vulnerability checker:
c171e7a 
- default scanner for any project with .go file, go.mod, go.sum, and Gopkg.lock
- create symlink for successfully run gosec against $GOPATH projects
- adds tests for integration
@xolaz
describe gofiles
e9354f5
@xolaz
update spec description
2d6743e
@xolaz
copy projects into $GOPATH instead of symlinking
409a6be
@xolaz xolaz force-pushed the integrate-gosec branch 2 times, most recently from 472e4a0 to 60821cd Compare January 31, 2019 21:26
@xolaz
call #report_failure outside of conditional and after success, levera…
8e3877a 
…ge return of env var for conditional check, remove parens
@xolaz
Copy link
Contributor Author

xolaz commented Jan 31, 2019

circleci integration test is failing because the result includes gosec data:

      "Gosec": {
               "pass_on_raise": false
      },

^^ this will pass once I rebuild docker after the merge and release

nishils
nishils approved these changes Jan 31, 2019
View reviewed changes
Copy link
Contributor

@nishils nishils left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm - would like to see better test cases against gosec so its easier to check what its testing in case we add a tool like trufflehog in the future.

@xolaz
Copy link
Contributor Author

xolaz commented Jan 31, 2019

Test cases around what scanners actually test is a great idea. It does create redundancy but I like the explicit failures it can give us depending on how scanners change as we bump them.

@xolaz xolaz merged commit 785a9ca into master Jan 31, 2019
@xolaz xolaz deleted the integrate-gosec branch February 1, 2019 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jborrey jborrey jborrey left review comments

@as3richa as3richa as3richa left review comments

@tutasla tutasla tutasla left review comments

@nishils nishils nishils approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@xolaz @jborrey @as3richa @tutasla @nishils