Introduce gosec for Static Application Security Testing (SAST) (gar…

…dener#954)

* Introduce make targets for sast and address security issues.

* Address review comments

.ci/check

@@ -47,4 +47,7 @@ gofmt -s -l -w ${PACKAGES_DIRS}
 ## Execute lint checks.
 echo "Running golangci-lint..."
 # golangci-lint can't be run from outside the directory
-(cd ${SOURCE_PATH} && golangci-lint run -c .golangci.yaml --timeout 10m)
+(cd ${SOURCE_PATH} && golangci-lint run -c .golangci.yaml --timeout 10m)
+
+# Run Static Application Security Testing (SAST) using gosec
+make sast-report

.gitignore

@@ -31,3 +31,6 @@ tags
 
 # dont check in tool binariess
 hack/tools/bin/
+
+# gosec
+gosec-report.sarif

Makefile

@@ -170,7 +170,16 @@ generate: $(VGOPATH) $(DEEPCOPY_GEN) $(DEFAULTER_GEN) $(CONVERSION_GEN) $(OPENAP
 	@./hack/generate-code
 	@./hack/api-reference/generate-spec-doc.sh
 
-
 .PHONY: add-license-headers
 add-license-headers: $(GO_ADD_LICENSE)
 	@./hack/add_license_headers.sh
+
+.PHONY: sast
+sast: $(GOSEC)
+	@chmod +xw hack/sast.sh
+	@./hack/sast.sh
+
+.PHONY: sast-report
+sast-report:$(GOSEC)
+	@chmod +xw hack/sast.sh
+	@./hack/sast.sh --gosec-report true

cmd/machine-controller-manager/app/controllermanager.go

@@ -364,7 +364,7 @@ func startHTTP(s *options.MCMServer) {
 	handlers.UpdateHealth(true)
 	mux.HandleFunc("/healthz", handlers.Healthz)
 
-	server := &http.Server{
+	server := &http.Server{ // #nosec  G112 (CWE-400) -- Only used for metrics, profiling and health checks.
 		Addr:    net.JoinHostPort(s.Address, strconv.Itoa(int(s.Port))),
 		Handler: mux,
 	}

hack/sast.sh

@@ -0,0 +1,44 @@
+#!/usr/bin/env bash
+#
+# SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and Gardener contributors
+#
+# SPDX-License-Identifier: Apache-2.0
+
+set -e
+
+root_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )/.." &> /dev/null && pwd )"
+
+gosec_report="false"
+gosec_report_parse_flags=""
+
+parse_flags() {
+  while test $# -gt 1; do
+    case "$1" in
+      --gosec-report)
+        shift; gosec_report="$1"
+        ;;
+      *)
+        echo "Unknown argument: $1"
+        exit 1
+        ;;
+    esac
+    shift
+  done
+}
+
+parse_flags "$@"
+
+echo "> Running gosec"
+gosec --version
+if [[ "$gosec_report" != "false" ]]; then
+  echo "Exporting report to $root_dir/gosec-report.sarif"
+  gosec_report_parse_flags="-track-suppressions -fmt=sarif -out=gosec-report.sarif -stdout"
+fi
+
+# MCM uses code-generators https://github.com/kubernetes/code-generator which create lots of G103 (CWE-242:
+# Use of unsafe calls should be audited) & G104 (CWE-703: Errors unhandled) errors.
+# However, those generators are best-pratice in Kubernetes environment and their results are tested well.
+# Thus, generated code is excluded from gosec scan.
+# Nested go modules are not supported by gosec (see https://github.com/securego/gosec/issues/501), so the ./hack folder
+# is excluded too. It does not contain productive code anyway.
+gosec -exclude-generated -exclude-dir=hack $gosec_report_parse_flags ./...

hack/tools.mk

@@ -16,6 +16,7 @@ GEN_CRD_API_REFERENCE_DOCS ?= $(TOOLS_BIN_DIR)/gen-crd-api-reference-docs
 ADDLICENSE ?= $(TOOLS_BIN_DIR)/addlicense
 GOIMPORTS ?= $(TOOLS_BIN_DIR)/goimports
 GOLANGCI_LINT ?= $(TOOLS_BIN_DIR)/golangci-lint
+GOSEC ?= $(TOOLS_BIN_DIR)/gosec
 
 ## Tool Versions
 CODE_GENERATOR_VERSION ?= v0.31.0
@@ -25,6 +26,7 @@ GEN_CRD_API_REFERENCE_DOCS_VERSION ?= v0.3.0
 ADDLICENSE_VERSION ?= v1.1.1
 GOIMPORTS_VERSION ?= v0.13.0
 GOLANGCI_LINT_VERSION ?= v1.60.3
+GOSEC_VERSION ?= v2.21.4
 
 
 # default tool versions
@@ -69,4 +71,7 @@ $(GOIMPORTS):
 	GOBIN=$(abspath $(TOOLS_BIN_DIR)) go install golang.org/x/tools/cmd/goimports@$(GOIMPORTS_VERSION)
 
 $(GOLANGCI_LINT): $(TOOLS_BIN_DIR)
-	GOBIN=$(abspath $(TOOLS_BIN_DIR)) go install github.com/golangci/golangci-lint/cmd/golangci-lint@$(GOLANGCI_LINT_VERSION)
+	GOBIN=$(abspath $(TOOLS_BIN_DIR)) go install github.com/golangci/golangci-lint/cmd/golangci-lint@$(GOLANGCI_LINT_VERSION)
+
+$(GOSEC):
+	GOSEC_VERSION=$(GOSEC_VERSION) bash $(abspath $(TOOLS_DIR))/install-gosec.sh

hack/tools/install-gosec.sh

@@ -0,0 +1,43 @@
+#!/usr/bin/env bash
+#
+# SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and Gardener contributors
+#
+# SPDX-License-Identifier: Apache-2.0
+
+set -e
+
+echo "> Installing gosec"
+
+TOOLS_BIN_DIR=${TOOLS_BIN_DIR:-$(dirname $0)/bin}
+
+platform=$(uname -s | tr '[:upper:]' '[:lower:]')
+version=$GOSEC_VERSION
+echo "gosec version:$GOSEC_VERSION"
+case $(uname -m) in
+  aarch64 | arm64)
+    arch="arm64"
+    ;;
+  x86_64)
+    arch="amd64"
+    ;;
+  *)
+    echo "Unknown architecture"
+    exit -1
+    ;;
+esac
+
+archive_name="gosec_${version#v}_${platform}_${arch}"
+file_name="${archive_name}.tar.gz"
+
+temp_dir="$(mktemp -d)"
+function cleanup {
+  rm -rf "${temp_dir}"
+}
+trap cleanup EXIT ERR INT TERM
+echo "Downloading from: https://github.com/securego/gosec/releases/download/${version}/${file_name}"
+curl -L -o ${temp_dir}/${file_name} "https://github.com/securego/gosec/releases/download/${version}/${file_name}"
+
+
+tar -xzm -C "${temp_dir}" -f "${temp_dir}/${file_name}"
+mv "${temp_dir}/gosec" $TOOLS_BIN_DIR
+chmod +x $TOOLS_BIN_DIR/gosec

pkg/apis/machine/validation/machinedeployment.go

@@ -2,13 +2,14 @@
 //
 // SPDX-License-Identifier: Apache-2.0
 
-
 // Package validation is used to validate all the machine CRD objects
 package validation
 
 import (
 	"github.com/gardener/machine-controller-manager/pkg/apis/machine"
+	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/apimachinery/pkg/util/validation/field"
+	"math"
 )
 
 // ValidateMachineDeployment and returns a list of errors.
@@ -22,24 +23,49 @@ func internalValidateMachineDeployment(machineDeployment *machine.MachineDeploym
 	return allErrs
 }
 
-func validateMachineDeploymentSpec(spec *machine.MachineDeploymentSpec, fldPath *field.Path) field.ErrorList {
-	allErrs := field.ErrorList{}
-
-	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 {
+	intVal, err := intstr.GetScaledValueFromIntOrPercent(val, replicas, true)
+	if err != nil {
+		return false
+	}
+	if intVal < math.MinInt32 || intVal > math.MaxInt32 {
+		return false
 	}
+	return true
+}
 
+func validateUpdateStrategy(spec *machine.MachineDeploymentSpec, fldPath *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
 	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 {
+			allErrs = append(allErrs, field.Required(fldPath.Child("strategy.rollingUpdate"), "RollingUpdate parameter cannot be nil for rolling update strategy"))
+		} else {
+			if !canConvertIntOrStringToInt32(spec.Strategy.RollingUpdate.MaxUnavailable, int(spec.Replicas)) {
+				allErrs = append(allErrs, field.Required(fldPath.Child("strategy.rollingUpdate.maxUnavailable"), "unable to convert maxUnavailable to int32"))
+			}
+			if !canConvertIntOrStringToInt32(spec.Strategy.RollingUpdate.MaxSurge, int(spec.Replicas)) {
+				allErrs = append(allErrs, field.Required(fldPath.Child("strategy.rollingUpdate.maxSurge"), "unable to convert maxSurge to int32"))
+			}
+		}
+	}
+	return allErrs
+}
 
+func validateMachineDeploymentSpec(spec *machine.MachineDeploymentSpec, fldPath *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
+	if spec.Replicas < 0 {
+		allErrs = append(allErrs, field.Required(fldPath.Child("replicas"), "Replicas has to be a whole number"))
+	}
+	allErrs = append(allErrs, validateUpdateStrategy(spec, fldPath)...)
 	for k, v := range spec.Selector.MatchLabels {
 		if spec.Template.Labels[k] != v {
 			allErrs = append(allErrs, field.Required(fldPath.Child("selector.matchLabels"), "is not matching with spec.template.metadata.labels"))
 			break
 		}
 	}
-
 	allErrs = append(allErrs, validateClassReference(&spec.Template.Spec.Class, field.NewPath("spec.template.spec.class"))...)
 	return allErrs
 }

pkg/controller/controller_utils.go

@@ -903,8 +903,11 @@ func ComputeHash(template *v1alpha1.MachineTemplateSpec, collisionCount *int32)
 	// Add collisionCount in the hash if it exists.
 	if collisionCount != nil {
 		collisionCountBytes := make([]byte, 8)
-		binary.LittleEndian.PutUint32(collisionCountBytes, uint32(*collisionCount))
-		machineTemplateSpecHasher.Write(collisionCountBytes)
+		binary.LittleEndian.PutUint32(collisionCountBytes, uint32(*collisionCount)) // #nosec G115 (CWE-190) -- collisionCount cannot be negative
+		_, err := machineTemplateSpecHasher.Write(collisionCountBytes)
+		if err != nil {
+			klog.Warningf("Unable to write collision count: %v", err)
+		}
 	}
 
 	return machineTemplateSpecHasher.Sum32()

pkg/controller/deployment_machineset_util.go

@@ -129,6 +129,7 @@ func calculateMachineSetStatus(is *v1alpha1.MachineSet, filteredMachines []*v1al
 	// Update the FailedMachines field only if we see new failures
 	// Clear FailedMachines if ready replicas equals total replicas,
 	// which means the machineset doesn't have any machine objects which are in any failed state
+	// #nosec G115 -- number of machines will not exceed MaxInt32
 	if len(failedMachines) > 0 {
 		newStatus.FailedMachines = &failedMachines
 	} else if int32(readyReplicasCount) == is.Status.Replicas {
@@ -149,10 +150,10 @@ func calculateMachineSetStatus(is *v1alpha1.MachineSet, filteredMachines []*v1al
 		RemoveCondition(&newStatus, v1alpha1.MachineSetReplicaFailure)
 	}
 
-	newStatus.Replicas = int32(len(filteredMachines))
-	newStatus.FullyLabeledReplicas = int32(fullyLabeledReplicasCount)
-	newStatus.ReadyReplicas = int32(readyReplicasCount)
-	newStatus.AvailableReplicas = int32(availableReplicasCount)
+	newStatus.Replicas = int32(len(filteredMachines))                 // #nosec  G115 (CWE-190) -- number of machines will not exceed MaxInt32
+	newStatus.FullyLabeledReplicas = int32(fullyLabeledReplicasCount) // #nosec  G115 (CWE-190) -- number of machines will not exceed MaxInt32
+	newStatus.ReadyReplicas = int32(readyReplicasCount)               // #nosec  G115 (CWE-190) -- number of machines will not exceed MaxInt32
+	newStatus.AvailableReplicas = int32(availableReplicasCount)       // #nosec  G115 (CWE-190) -- number of machines will not exceed MaxInt32
 	newStatus.LastOperation.LastUpdateTime = metav1.Now()
 	return newStatus
 }

pkg/controller/deployment_rolling.go

@@ -230,7 +230,7 @@ func (dc *controller) cleanupUnhealthyReplicas(ctx context.Context, oldISs []*v1
 			continue
 		}
 
-		scaledDownCount := int32(integer.IntMin(int(maxCleanupCount-totalScaledDown), int((targetIS.Spec.Replicas)-targetIS.Status.AvailableReplicas)))
+		scaledDownCount := int32(integer.IntMin(int(maxCleanupCount-totalScaledDown), int((targetIS.Spec.Replicas)-targetIS.Status.AvailableReplicas))) // #nosec  G115 (CWE-190) -- obtained from MachineDeployment.Spec.Replicas is of type int32, which is already validated
 		newReplicasCount := (targetIS.Spec.Replicas) - scaledDownCount
 		if newReplicasCount > (targetIS.Spec.Replicas) {
 			return nil, 0, fmt.Errorf("when cleaning up unhealthy replicas, got invalid request to scale down %s %d -> %d", targetIS.Name, (targetIS.Spec.Replicas), newReplicasCount)
@@ -274,7 +274,7 @@ func (dc *controller) scaleDownOldMachineSetsForRollingUpdate(ctx context.Contex
 			continue
 		}
 		// Scale down.
-		scaleDownCount := int32(integer.IntMin(int((targetIS.Spec.Replicas)), int(totalScaleDownCount-totalScaledDown)))
+		scaleDownCount := int32(integer.IntMin(int((targetIS.Spec.Replicas)), int(totalScaleDownCount-totalScaledDown))) // #nosec  G115 (CWE-190) -- obtained from MachineDeployment.Spec.Replicas is of type int32 which is already validated
 		newReplicasCount := (targetIS.Spec.Replicas) - scaleDownCount
 		if newReplicasCount > (targetIS.Spec.Replicas) {
 			return 0, fmt.Errorf("when scaling down old IS, got invalid request to scale down %s %d -> %d", targetIS.Name, (targetIS.Spec.Replicas), newReplicasCount)

pkg/controller/deployment_sync.go

@@ -578,7 +578,7 @@ func (dc *controller) cleanupMachineDeployment(ctx context.Context, oldISs []*v1
 	}
 	cleanableISes := FilterMachineSets(oldISs, aliveFilter)
 
-	diff := int32(len(cleanableISes)) - *deployment.Spec.RevisionHistoryLimit
+	diff := int32(len(cleanableISes)) - *deployment.Spec.RevisionHistoryLimit // #nosec G115 (CWE-190) -- number will never reach MaxInt32, and len() cannot be negative
 	if diff <= 0 {
 		return nil
 	}

pkg/controller/deployment_util.go

@@ -592,7 +592,7 @@ func getIntFromAnnotation(is *v1alpha1.MachineSet, annotationKey string) (int32,
 		klog.V(2).Infof("Cannot convert the value %q with annotation key %q for the machine set %q", annotationValue, annotationKey, is.Name)
 		return int32(0), false
 	}
-	return int32(intValue), true
+	return int32(intValue), true // #nosec G109, G115 -- value obtained from MachineDeployment.Spec.Replicas which is of type int32 and is already validated
 }
 
 // SetReplicasAnnotations sets the desiredReplicas and maxReplicas into the annotations
@@ -1092,15 +1092,15 @@ func NewISNewReplicas(deployment *v1alpha1.MachineDeployment, allISs []*v1alpha1
 		// Find the total number of machines
 		// currentMachineCount := GetReplicaCountForMachineSets(allISs)
 		currentMachineCount := GetActualReplicaCountForMachineSets(allISs)
-		maxTotalMachines := (deployment.Spec.Replicas) + int32(maxSurge)
+		maxTotalMachines := (deployment.Spec.Replicas) + int32(maxSurge) // #nosec G115 (CWE-190) -- value already validated
 		if currentMachineCount >= maxTotalMachines {
 			// Cannot scale up.
 			return (newIS.Spec.Replicas), nil
 		}
 		// Scale up.
 		scaleUpCount := maxTotalMachines - currentMachineCount
 		// Do not exceed the number of desired replicas.
-		scaleUpCount = int32(integer.IntMin(int(scaleUpCount), int((deployment.Spec.Replicas)-(newIS.Spec.Replicas))))
+		scaleUpCount = int32(integer.IntMin(int(scaleUpCount), int((deployment.Spec.Replicas)-(newIS.Spec.Replicas)))) // #nosec G115 (CWE-190) -- Obtained from replicas and maxSurge, both of which are validated.
 		return (newIS.Spec.Replicas) + scaleUpCount, nil
 	case v1alpha1.RecreateMachineDeploymentStrategyType:
 		return (deployment.Spec.Replicas), nil
@@ -1122,6 +1122,7 @@ func IsSaturated(deployment *v1alpha1.MachineDeployment, is *v1alpha1.MachineSet
 	if err != nil {
 		return false
 	}
+	// #nosec G109, G115 -- obtained from MachineDeployement.Spec.Replicas, which is already validated
 	return (is.Spec.Replicas) == (deployment.Spec.Replicas) &&
 		int32(desired) == (deployment.Spec.Replicas) &&
 		is.Status.AvailableReplicas == (deployment.Spec.Replicas)
@@ -1179,8 +1180,7 @@ func ResolveFenceposts(maxSurge, maxUnavailable *intstrutil.IntOrString, desired
 		// theory that surge might not work due to quota.
 		unavailable = 1
 	}
-
-	return int32(surge), int32(unavailable), nil
+	return int32(surge), int32(unavailable), nil // #nosec G115 (CWE-190) -- surge and unavailable values already validated
 }
 
 // statusUpdateRequired checks for if status update is required comparing two MachineDeployment statuses

pkg/controller/machine_safety.go

@@ -238,8 +238,7 @@ func (c *controller) checkAndFreezeORUnfreezeMachineSets(ctx context.Context) er
 					klog.Error("SafetyController: Error while trying to GET surge value - ", err)
 					return err
 				}
-
-				higherThreshold = machineDeployment.Spec.Replicas + int32(surge) + c.safetyOptions.SafetyUp
+				higherThreshold = machineDeployment.Spec.Replicas + int32(surge) + c.safetyOptions.SafetyUp // #nosec G115 (CWE-190) -- value already validated
 				lowerThreshold = higherThreshold - c.safetyOptions.SafetyDown
 			}
 		}