Merge pull request #1865 from mrueg/gosec

k8s-ci-robot · web-flow · commit a3a509acdff1 · 2022-10-21T00:37:51.000-07:00
Harden and add gosec linter
diff --git a/.golangci.yml b/.golangci.yml
@@ -8,6 +8,7 @@ linters:
     - gocyclo
     - gofmt
     - goimports
+    - gosec
     - gosimple
     - govet
     - ineffassign
@@ -28,3 +29,7 @@ issues:
     - path: _test\.go
       linters:
         - promlinter
+    # TODO(mrueg) Improve error handling
+    - text: "G104:"
+      linters:
+        - gosec
diff --git a/internal/store/job.go b/internal/store/job.go
@@ -212,7 +212,8 @@ func jobMetricFamilies(allowAnnotationsList, allowLabelsList []string) []generat
 					}
 				}
 
-				for _, condition := range j.Status.Conditions {
+				for _, c := range j.Status.Conditions {
+					condition := c
 					if condition.Type == v1batch.JobFailed {
 						reasonKnown := false
 						for _, reason := range jobFailureReasons {
diff --git a/internal/store/secret.go b/internal/store/secret.go
@@ -33,9 +33,9 @@ import (
 
 var (
 	descSecretAnnotationsName     = "kube_secret_annotations"
-	descSecretAnnotationsHelp     = "Kubernetes annotations converted to Prometheus labels."
+	descSecretAnnotationsHelp     = "Kubernetes annotations converted to Prometheus labels." //nolint:gosec
 	descSecretLabelsName          = "kube_secret_labels"
-	descSecretLabelsHelp          = "Kubernetes labels converted to Prometheus labels."
+	descSecretLabelsHelp          = "Kubernetes labels converted to Prometheus labels." //nolint:gosec
 	descSecretLabelsDefaultLabels = []string{"namespace", "secret"}
 )
 
diff --git a/main.go b/main.go
@@ -20,6 +20,7 @@ import (
 	"context"
 	"fmt"
 	"os"
+	"path/filepath"
 	"strings"
 
 	"github.com/prometheus/common/version"
@@ -74,7 +75,7 @@ func resolveCustomResourceConfig(opts *options.Options) (customresourcestate.Con
 		return yaml.NewDecoder(strings.NewReader(s)), true
 	}
 	if file := opts.CustomResourceConfigFile; file != "" {
-		f, err := os.Open(file)
+		f, err := os.Open(filepath.Clean(file))
 		if err != nil {
 			klog.ErrorS(err, "Custom Resource State Metrics file could not be opened")
 			klog.FlushAndExit(klog.ExitFlushTimeout, 1)
diff --git a/pkg/app/server.go b/pkg/app/server.go
@@ -178,11 +178,17 @@ func RunKubeStateMetrics(ctx context.Context, opts *options.Options, factories .
 
 	telemetryMux := buildTelemetryServer(ksmMetricsRegistry)
 	telemetryListenAddress := net.JoinHostPort(opts.TelemetryHost, strconv.Itoa(opts.TelemetryPort))
-	telemetryServer := http.Server{Handler: telemetryMux, Addr: telemetryListenAddress}
+	telemetryServer := http.Server{
+		Handler:           telemetryMux,
+		Addr:              telemetryListenAddress,
+		ReadHeaderTimeout: 5 * time.Second}
 
 	metricsMux := buildMetricsServer(m, durationVec)
 	metricsServerListenAddress := net.JoinHostPort(opts.Host, strconv.Itoa(opts.Port))
-	metricsServer := http.Server{Handler: metricsMux, Addr: metricsServerListenAddress}
+	metricsServer := http.Server{
+		Handler:           metricsMux,
+		Addr:              metricsServerListenAddress,
+		ReadHeaderTimeout: 5 * time.Second}
 
 	// Run Telemetry server
 	{
diff --git a/pkg/app/server_test.go b/pkg/app/server_test.go
@@ -73,7 +73,10 @@ func BenchmarkKubeStateMetrics(b *testing.B) {
 
 	builder := store.NewBuilder()
 	builder.WithMetrics(reg)
-	builder.WithEnabledResources(options.DefaultResources.AsSlice())
+	err := builder.WithEnabledResources(options.DefaultResources.AsSlice())
+	if err != nil {
+		b.Fatal(err)
+	}
 	builder.WithKubeClient(kubeClient)
 	builder.WithSharding(0, 1)
 	builder.WithContext(ctx)
@@ -118,7 +121,10 @@ func BenchmarkKubeStateMetrics(b *testing.B) {
 
 			b.StopTimer()
 			buf := bytes.Buffer{}
-			buf.ReadFrom(resp.Body)
+			_, err := buf.ReadFrom(resp.Body)
+			if err != nil {
+				b.Fatal(err)
+			}
 			accumulatedContentLength += buf.Len()
 			b.StartTimer()
 		}
@@ -144,7 +150,10 @@ func TestFullScrapeCycle(t *testing.T) {
 	reg := prometheus.NewRegistry()
 	builder := store.NewBuilder()
 	builder.WithMetrics(reg)
-	builder.WithEnabledResources(options.DefaultResources.AsSlice())
+	err = builder.WithEnabledResources(options.DefaultResources.AsSlice())
+	if err != nil {
+		t.Fatal(err)
+	}
 	builder.WithKubeClient(kubeClient)
 	builder.WithNamespaces(options.DefaultNamespaces, "")
 	builder.WithGenerateStoresFunc(builder.DefaultGenerateStoresFunc())
@@ -428,7 +437,10 @@ func TestShardingEquivalenceScrapeCycle(t *testing.T) {
 	reg := prometheus.NewRegistry()
 	unshardedBuilder := store.NewBuilder()
 	unshardedBuilder.WithMetrics(reg)
-	unshardedBuilder.WithEnabledResources(options.DefaultResources.AsSlice())
+	err = unshardedBuilder.WithEnabledResources(options.DefaultResources.AsSlice())
+	if err != nil {
+		t.Fatal(err)
+	}
 	unshardedBuilder.WithKubeClient(kubeClient)
 	unshardedBuilder.WithNamespaces(options.DefaultNamespaces, "")
 	unshardedBuilder.WithFamilyGeneratorFilter(l)
@@ -441,7 +453,10 @@ func TestShardingEquivalenceScrapeCycle(t *testing.T) {
 	regShard1 := prometheus.NewRegistry()
 	shardedBuilder1 := store.NewBuilder()
 	shardedBuilder1.WithMetrics(regShard1)
-	shardedBuilder1.WithEnabledResources(options.DefaultResources.AsSlice())
+	err = shardedBuilder1.WithEnabledResources(options.DefaultResources.AsSlice())
+	if err != nil {
+		t.Fatal(err)
+	}
 	shardedBuilder1.WithKubeClient(kubeClient)
 	shardedBuilder1.WithNamespaces(options.DefaultNamespaces, "")
 	shardedBuilder1.WithFamilyGeneratorFilter(l)
@@ -454,7 +469,10 @@ func TestShardingEquivalenceScrapeCycle(t *testing.T) {
 	regShard2 := prometheus.NewRegistry()
 	shardedBuilder2 := store.NewBuilder()
 	shardedBuilder2.WithMetrics(regShard2)
-	shardedBuilder2.WithEnabledResources(options.DefaultResources.AsSlice())
+	err = shardedBuilder2.WithEnabledResources(options.DefaultResources.AsSlice())
+	if err != nil {
+		t.Fatal(err)
+	}
 	shardedBuilder2.WithKubeClient(kubeClient)
 	shardedBuilder2.WithNamespaces(options.DefaultNamespaces, "")
 	shardedBuilder2.WithFamilyGeneratorFilter(l)
@@ -591,7 +609,11 @@ func TestCustomResourceExtension(t *testing.T) {
 	builder := store.NewBuilder()
 	builder.WithCustomResourceStoreFactories(factories...)
 	builder.WithMetrics(reg)
-	builder.WithEnabledResources(resources)
+	err := builder.WithEnabledResources(resources)
+	if err != nil {
+		t.Fatal(err)
+	}
+
 	builder.WithKubeClient(kubeClient)
 	builder.WithCustomResourceClients(customResourceClients)
 	builder.WithNamespaces(options.DefaultNamespaces, "")
diff --git a/pkg/builder/builder_test.go b/pkg/builder/builder_test.go
@@ -40,9 +40,12 @@ var (
 func TestBuilderWithCustomStore(t *testing.T) {
 	b := builder.NewBuilder()
 	b.WithFamilyGeneratorFilter(generator.NewCompositeFamilyGeneratorFilter())
-	b.WithEnabledResources([]string{"pods"})
-	b.WithGenerateStoresFunc(customStore)
+	err := b.WithEnabledResources([]string{"pods"})
+	if err != nil {
+		t.Fatal(err)
+	}
 
+	b.WithGenerateStoresFunc(customStore)
 	var fStores []*fakeStore
 	for _, stores := range b.BuildStores() {
 		for _, store := range stores {
diff --git a/pkg/metrics_store/metrics_writer_test.go b/pkg/metrics_store/metrics_writer_test.go
@@ -76,7 +76,8 @@ func TestWriteAllWithSingleStore(t *testing.T) {
 			},
 		},
 	}
-	for _, svc := range svcs {
+	for _, s := range svcs {
+		svc := s
 		if err := store.Add(&svc); err != nil {
 			t.Fatal(err)
 		}
@@ -160,7 +161,8 @@ func TestWriteAllWithMultipleStores(t *testing.T) {
 			},
 		},
 	}
-	for _, svc := range svcs1 {
+	for _, s := range svcs1 {
+		svc := s
 		if err := s1.Add(&svc); err != nil {
 			t.Fatal(err)
 		}
@@ -183,7 +185,8 @@ func TestWriteAllWithMultipleStores(t *testing.T) {
 		},
 	}
 	s2 := metricsstore.NewMetricsStore([]string{"Info 1 about services", "Info 2 about services"}, genFunc)
-	for _, svc := range svcs2 {
+	for _, s := range svcs2 {
+		svc := s
 		if err := s2.Add(&svc); err != nil {
 			t.Fatal(err)
 		}
diff --git a/pkg/metricshandler/metrics_handler.go b/pkg/metricshandler/metrics_handler.go
@@ -205,7 +205,10 @@ func (m *MetricsHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 
 	// In case we gzipped the response, we have to close the writer.
 	if closer, ok := writer.(io.Closer); ok {
-		closer.Close()
+		err := closer.Close()
+		if err != nil {
+			klog.ErrorS(err, "Failed to close the writer")
+		}
 	}
 }
 
@@ -226,7 +229,7 @@ func shardingSettingsFromStatefulSet(ss *appsv1.StatefulSet, podName string) (no
 
 func detectNominalFromPod(statefulSetName, podName string) (int32, error) {
 	nominalString := strings.TrimPrefix(podName, statefulSetName+"-")
-	nominal, err := strconv.Atoi(nominalString)
+	nominal, err := strconv.ParseInt(nominalString, 10, 32)
 	if err != nil {
 		return 0, fmt.Errorf("failed to detect shard index for Pod %s of StatefulSet %s, parsed %s: %w", podName, statefulSetName, nominalString, err)
 	}
diff --git a/pkg/options/options.go b/pkg/options/options.go
@@ -79,7 +79,10 @@ func (o *Options) AddFlags() {
 	klogFlags := flag.NewFlagSet("klog", flag.ExitOnError)
 	klog.InitFlags(klogFlags)
 	o.flags.AddGoFlagSet(klogFlags)
-	o.flags.Lookup("logtostderr").Value.Set("true")
+	err := o.flags.Lookup("logtostderr").Value.Set("true")
+	if err != nil {
+		klog.Error(err)
+	}
 	o.flags.Lookup("logtostderr").DefValue = "true"
 	o.flags.Lookup("logtostderr").NoOptDefVal = "true"
 
diff --git a/tests/e2e/main_test.go b/tests/e2e/main_test.go
@@ -24,6 +24,7 @@ import (
 	"log"
 	"os"
 	"path"
+	"path/filepath"
 	"regexp"
 	"sort"
 	"strings"
@@ -165,7 +166,7 @@ func getLabelsDocumentation() (map[string][]string, error) {
 		}
 
 		filePath := path.Join(docPath, file.Name())
-		f, err := os.Open(filePath)
+		f, err := os.Open(filepath.Clean(filePath))
 		if err != nil {
 			return nil, fmt.Errorf("cannot read file %s: %w", filePath, err)
 		}

Original file line number	Diff line number	Diff line change
`@@ -212,7 +212,8 @@ func jobMetricFamilies(allowAnnotationsList, allowLabelsList []string) []generat`
`212`	`212`	`}`
`213`	`213`	`}`
`214`	`214`
`215`		`- for _, condition := range j.Status.Conditions {`
	`215`	`+ for _, c := range j.Status.Conditions {`
	`216`	`+ condition := c`
`216`	`217`	`if condition.Type == v1batch.JobFailed {`
`217`	`218`	`reasonKnown := false`
`218`	`219`	`for _, reason := range jobFailureReasons {`
Original file line number	Diff line number	Diff line change
`@@ -76,7 +76,8 @@ func TestWriteAllWithSingleStore(t *testing.T) {`
`76`	`76`	`},`
`77`	`77`	`},`
`78`	`78`	`}`
`79`		`- for _, svc := range svcs {`
	`79`	`+ for _, s := range svcs {`
	`80`	`+ svc := s`
`80`	`81`	`if err := store.Add(&svc); err != nil {`
`81`	`82`	`t.Fatal(err)`
`82`	`83`	`}`
`@@ -160,7 +161,8 @@ func TestWriteAllWithMultipleStores(t *testing.T) {`
`160`	`161`	`},`
`161`	`162`	`},`
`162`	`163`	`}`
`163`		`- for _, svc := range svcs1 {`
	`164`	`+ for _, s := range svcs1 {`
	`165`	`+ svc := s`
`164`	`166`	`if err := s1.Add(&svc); err != nil {`
`165`	`167`	`t.Fatal(err)`
`166`	`168`	`}`
`@@ -183,7 +185,8 @@ func TestWriteAllWithMultipleStores(t *testing.T) {`
`183`	`185`	`},`
`184`	`186`	`}`
`185`	`187`	`s2 := metricsstore.NewMetricsStore([]string{"Info 1 about services", "Info 2 about services"}, genFunc)`
`186`		`- for _, svc := range svcs2 {`
	`188`	`+ for _, s := range svcs2 {`
	`189`	`+ svc := s`
`187`	`190`	`if err := s2.Add(&svc); err != nil {`
`188`	`191`	`t.Fatal(err)`
`189`	`192`	`}`
Original file line number	Diff line number	Diff line change
`@@ -205,7 +205,10 @@ func (m MetricsHandler) ServeHTTP(w http.ResponseWriter, r http.Request) {`
`205`	`205`
`206`	`206`	`// In case we gzipped the response, we have to close the writer.`
`207`	`207`	`if closer, ok := writer.(io.Closer); ok {`
`208`		`- closer.Close()`
	`208`	`+ err := closer.Close()`
	`209`	`+ if err != nil {`
	`210`	`+ klog.ErrorS(err, "Failed to close the writer")`
	`211`	`+ }`
`209`	`212`	`}`
`210`	`213`	`}`
`211`	`214`
`@@ -226,7 +229,7 @@ func shardingSettingsFromStatefulSet(ss *appsv1.StatefulSet, podName string) (no`
`226`	`229`
`227`	`230`	`func detectNominalFromPod(statefulSetName, podName string) (int32, error) {`
`228`	`231`	`nominalString := strings.TrimPrefix(podName, statefulSetName+"-")`
`229`		`- nominal, err := strconv.Atoi(nominalString)`
	`232`	`+ nominal, err := strconv.ParseInt(nominalString, 10, 32)`
`230`	`233`	`if err != nil {`
`231`	`234`	`return 0, fmt.Errorf("failed to detect shard index for Pod %s of StatefulSet %s, parsed %s: %w", podName, statefulSetName, nominalString, err)`
`232`	`235`	`}`