From 6b7027ff41eff6aeba3ab3de644a0c2fa4ee567f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20R=C3=BCger?= Date: Mon, 17 Oct 2022 00:48:44 +0200 Subject: [PATCH] Harden and add gosec linter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .golangci.yml | 5 ++++ internal/store/job.go | 3 +- internal/store/secret.go | 4 +-- main.go | 3 +- pkg/app/server.go | 10 +++++-- pkg/app/server_test.go | 36 +++++++++++++++++++----- pkg/builder/builder_test.go | 7 +++-- pkg/metrics_store/metrics_writer_test.go | 9 ++++-- pkg/metricshandler/metrics_handler.go | 7 +++-- pkg/options/options.go | 5 +++- tests/e2e/main_test.go | 3 +- 11 files changed, 70 insertions(+), 22 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index a0b8723e19..b10ab5c6f3 100644 --- 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 index cd878c7e08..391fd24b3b 100644 --- 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 index fee2a2c8a3..74bca4327f 100644 --- 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 index a0e6c97a45..4bf0d3fb1f 100644 --- 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 index a4a20234c2..75d08617fd 100644 --- 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 index cba7e27fe3..a2353a49af 100644 --- 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 index 9b420d47d0..7189cb1c66 100644 --- 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 index ff758a6894..1c3f66fa1a 100644 --- 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 index d46afa2651..b1a88f051d 100644 --- 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 index f87b459c0c..3bd07e9ab8 100644 --- 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 index 130d34085d..d01fb99f26 100644 --- 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) }