Introduce gosec for Static Application Security Testing (SAST)
#954
Conversation
gardener-robot
added
needs/review
|
Thank you @thiyyakat for your contribution. Before I can start building your PR, a member of the organization must set the required label(s) {'reviewed/ok-to-test'}. Once started, you can check the build status in the PR checks section below. |
thiyyakat
force-pushed
the
enh/add-gosec
branch
from
ccead38 to
9451a92
Compare
rishabh-11
added
the
reviewed/ok-to-test
gardener-robot-ci-3
added
needs/ok-to-test
|
@thiyyakat You need rebase this pull request with latest master branch. Please check. |
thiyyakat
force-pushed
the
enh/add-gosec
branch
from
9451a92 to
b621749
Compare
rishabh-11
added
reviewed/ok-to-test
gardener-robot-ci-3
added
needs/ok-to-test
gardener-robot
added
reviewed/lgtm
gardener-robot-ci-2
added
the
reviewed/ok-to-test
gardener-robot-ci-3
removed
the
reviewed/ok-to-test
gardener-robot
added
the
status/closed
|
|
||
| .PHONY: sast | ||
| sast: $(GOSEC) | ||
| @chmod +xw hack/sast.sh |
There was a problem hiding this comment.
Is this required? When you introduce a new script you already give it execute permissions and then onwards those permissions are preserved and therefore there is no real need to add this as part of the make target.
|
|
||
| .PHONY: sast-report | ||
| sast-report:$(GOSEC) | ||
| @chmod +xw hack/sast.sh |
There was a problem hiding this comment.
same as the comment for sast target.
|
|
||
| if spec.Replicas < 0 { | ||
| allErrs = append(allErrs, field.Required(fldPath.Child("replicas"), "Replicas has to be a whole number")) | ||
| func canConvertIntOrStringToInt32(val *intstr.IntOrString, replicas int) bool { |
There was a problem hiding this comment.
This is a very generic function, can we move elsewhere?
There was a problem hiding this comment.
It's used here only. If we feel like it is required elsewhere, then we'll put it in separate location
| if spec.Strategy.Type != "RollingUpdate" && spec.Strategy.Type != "Recreate" { | ||
| allErrs = append(allErrs, field.Required(fldPath.Child("strategy.type"), "Type can either be RollingUpdate or Recreate")) | ||
| } | ||
| if spec.Strategy.Type == "RollingUpdate" { |
There was a problem hiding this comment.
We should use:
if spec.Strategy.Type == v1alpha1.MachineDeploymentStrategyTypeinstead
| if spec.Strategy.Type != "RollingUpdate" && spec.Strategy.Type != "Recreate" { | ||
| allErrs = append(allErrs, field.Required(fldPath.Child("strategy.type"), "Type can either be RollingUpdate or Recreate")) | ||
| } | ||
| if spec.Strategy.Type == "RollingUpdate" { | ||
| if spec.Strategy.RollingUpdate == nil { |
There was a problem hiding this comment.
why is this check done?
As per the doc string if MaxUnavailable is not specified then By default, a fixed value of 1 is used.
Similarly for MaxSurge its mentioned that By default, a value of 1 is used.
So either the doc string needs correction or this check needs to be removed.
There was a problem hiding this comment.
The doc string is incorrect. If both are zero then we use maxUnavailable as 1.
…dener#954) * Introduce make targets for sast and address security issues. * Address review comments
What this PR does / why we need it:
This PR introduces two make targets:
sastandsast-reportto rungosecfor Static Application Security Testing. Additionally, it also addresses the security vulnerabilities in the MCM repository. It uses the default ruleset of gosec from gardener/gardener as introduced in gardener/gardener#9959.sast-reporttarget has also been added as a dependency for thechecktarget.Which issue(s) this PR fixes:
Partially fixes #948
Special notes for your reviewer:
Tested manually to check if gosec is downloaded and installed to
hack/tools/bin, if not already present, on runningmake sastandmake sast-report.Integration tests passed with provider-aws.
Release note: