Harden and add gosec linter

Remediate: G104: Errors unhandled. G109: Potential Integer overflow made by strconv.Atoi result conversion to int16/32 G112: Potential Slowloris Attack because ReadHeaderTimeout is not configured in the http.Server G304: Potential file inclusion via variable G601: Implicit memory aliasing in for loop. Signed-off-by: Manuel Rüger <[email protected]>
pawcykca · Oct 20, 2022 · 6b7027f · 6b7027f
1 parent d4fdcda
commit 6b7027f
Show file tree

Hide file tree

Showing 11 changed files with 70 additions and 22 deletions.
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)
 		}