gosec by yuvalk · Pull Request #741 · openshift-kni/performance-addon-operators

yuvalk · 2021-09-28T13:57:13Z

add gosec to verify target
fix CWE-22
fix all CWE-118 issues
fix CWE-276
fix CWE-703
ignore unchecked os.Unsetenv from gosec reports
fix CWE-703
ignore G204 in latency_testing

openshift-ci · 2021-09-28T13:57:15Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yuvalk
To complete the pull request process, please assign schseba after the PR has been reviewed.
You can assign the PR to them by writing /assign @schseba in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

coveralls · 2021-09-28T14:00:27Z

Pull Request Test Coverage Report for Build 2050

12 of 16 (75.0%) changed or added relevant lines in 5 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.1%) to 61.256%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/profilecreator/profilecreator.go	5	9	55.56%

Totals
Change from base Build 2039:	-0.1%
Covered Lines:	1570
Relevant Lines:	2563

💛 - Coveralls

cynepco3hahue · 2021-09-29T10:18:54Z

Probably we need to set GOARCH and compiler variables under our CI.

Golang errors in file: [functests/utils/images]:
  > [line 0 : column 0] - loading files from package "functests/utils/images": could not determine GOARCH and Go compiler

yuvalk · 2021-10-04T09:56:11Z

hack/gosec.sh

+	go get -u github.com/securego/gosec/v2/cmd/gosec
+fi
+
+time GO111MODULE=off gosec -conf gosec.conf.json ./...


I've added this GO111MODULE to make it work under prow
but I dont know WHY it work (without it, we're seeing could not determine GOARCH and Go compiler error )
if anyone can explain...
@fromanirh @cynepco3hahue maybe?

so with the GO111MODULE=on it works in the context of go modules and GO111MODULE=off in the context of GOPATH, so I am assuming something is configured on the PATH level that not configured on the module level(probably because of the CI)

Did you try to run
time GOARCH=<> GOOS=<> gosec -conf gosec.conf.json ./...?

yuvalk · 2021-10-04T09:57:03Z

functests/utils/utils.go

 	var stdout bytes.Buffer
 	var stderr bytes.Buffer
-	cmd := exec.CommandContext(ctx, name, arg...)
+	cmd := exec.CommandContext(ctx, name, arg...) // #nosec G204


@fromanirh I'm not actually sure this is a good,
do you have any idea how to make this from a closed list?

Hardly. The whole purpose of this API is to run arbitrary commands on arbitrary nodes. Having and maintaining a allow list would be quite a big burden. So yes, the main defense here is carefully review HOW we use this API.

this will ensure code is always secured and does not include security vunerabilities. this will be verified by running gosec on all our sources added placeholder for future gosec configs in gosec.conf.json

applying filepath.Clean, will make sure filename is not abused

removed all refernces to loop iterator variable

reducing permissions to 600

explicitly close the file and handle errors. Close is considered unsafe to defer, because it might fail

it seems that, yes, we dont check for errors, but the function always return nil err. this is to avoid CWE-703 on os.Unsetenv

Handle all error cases. for the future, if error is very rare and unlikely to happen on any normal (supported) user case, it's ok to use `panic(err)` in other cases, it's better to propageate error up back to user and logs

This is ignored temporarily as gosec resolver doesnt properly support Call Expressions https://github.com/securego/gosec/blob/master/resolve.go#L70

Ignored as general purpose command execution function

openshift-ci · 2021-11-11T08:08:17Z

@yuvalk: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/ci-index	`45900be`	link	true	`/test ci-index`
ci/prow/ci	`45900be`	link	true	`/test ci`
ci/prow/images	`45900be`	link	true	`/test images`
ci/prow/e2e-gcp-operator-upgrade	`45900be`	link	true	`/test e2e-gcp-operator-upgrade`
ci/prow/e2e-gcp	`45900be`	link	true	`/test e2e-gcp`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-ci · 2022-02-03T02:18:49Z

@yuvalk: PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

yanirq · 2022-04-13T08:06:03Z

@yuvalk please close this PR and open a new one on the NTO repo if needed.

openshift-ci bot requested review from cynepco3hahue and yanirq September 28, 2021 13:57

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 1, 2021

yuvalk force-pushed the gosec branch from b3b1d77 to 6e001d2 Compare October 4, 2021 07:53

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 4, 2021

yuvalk force-pushed the gosec branch 5 times, most recently from 33b1401 to 30e0948 Compare October 4, 2021 09:54

yuvalk commented Oct 4, 2021

View reviewed changes

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 30, 2021

yuvalk added 11 commits November 11, 2021 10:02

add gosec to verify target

ec0e56c

this will ensure code is always secured and does not include security vunerabilities. this will be verified by running gosec on all our sources added placeholder for future gosec configs in gosec.conf.json

fix CWE-22

ed45e68

applying filepath.Clean, will make sure filename is not abused

fix all CWE-118 issues

89412e3

removed all refernces to loop iterator variable

fix CWE-276

5e53083

reducing permissions to 600

fix CWE-703

44fc82b

explicitly close the file and handle errors. Close is considered unsafe to defer, because it might fail

ignore unchecked os.Unsetenv from gosec reports

23ee18a

it seems that, yes, we dont check for errors, but the function always return nil err. this is to avoid CWE-703 on os.Unsetenv

fix CWE-703

bada12a

Handle all error cases. for the future, if error is very rare and unlikely to happen on any normal (supported) user case, it's ok to use `panic(err)` in other cases, it's better to propageate error up back to user and logs

ignore G204 in latency_testing

6159efa

This is ignored temporarily as gosec resolver doesnt properly support Call Expressions https://github.com/securego/gosec/blob/master/resolve.go#L70

ignore G204 in ExecAndLogCommandWithStderr

3f7b7fa

Ignored as general purpose command execution function

WIP - test gosec with GO111MODULE=off

2df0ea1

WIP: go get is deprecated..

45900be

yuvalk force-pushed the gosec branch from 5c8cc1a to 45900be Compare November 11, 2021 08:03

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 11, 2021

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

gosec#741

gosec#741
yuvalk wants to merge 11 commits intoopenshift-kni:masterfrom
yuvalk:gosec

yuvalk commented Sep 28, 2021

Uh oh!

openshift-ci bot commented Sep 28, 2021

Uh oh!

coveralls commented Sep 28, 2021 •

edited

Loading

Uh oh!

cynepco3hahue commented Sep 29, 2021

Uh oh!

yuvalk Oct 4, 2021

Uh oh!

cynepco3hahue Oct 4, 2021

Uh oh!

cynepco3hahue Oct 4, 2021

Uh oh!

yuvalk Oct 4, 2021

Uh oh!

ffromani Oct 4, 2021

Uh oh!

openshift-ci bot commented Nov 11, 2021

Uh oh!

openshift-ci bot commented Feb 3, 2022

Uh oh!

yanirq commented Apr 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments

Conversation

yuvalk commented Sep 28, 2021

Uh oh!

openshift-ci bot commented Sep 28, 2021

Uh oh!

coveralls commented Sep 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 2050

💛 - Coveralls

Uh oh!

cynepco3hahue commented Sep 29, 2021

Uh oh!

yuvalk Oct 4, 2021

Choose a reason for hiding this comment

Uh oh!

cynepco3hahue Oct 4, 2021

Choose a reason for hiding this comment

Uh oh!

cynepco3hahue Oct 4, 2021

Choose a reason for hiding this comment

Uh oh!

yuvalk Oct 4, 2021

Choose a reason for hiding this comment

Uh oh!

ffromani Oct 4, 2021

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Nov 11, 2021

Uh oh!

openshift-ci bot commented Feb 3, 2022

Uh oh!

yanirq commented Apr 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented Sep 28, 2021 •

edited

Loading