Enable gosec as a linter and resolve potential security issues (#1259)

* Enable gosec as a linter and resolve potential security issues Signed-off-by: Saswata Mukherjee <[email protected]> * Bump read and readHeader timeout Signed-off-by: Saswata Mukherjee <[email protected]> --------- Signed-off-by: Saswata Mukherjee <[email protected]>
stolostron · Oct 17, 2023 · f7668a0 · f7668a0
1 parent 1d6732a
commit f7668a0
Show file tree

Hide file tree

Showing 17 changed files with 43 additions and 33 deletions.
diff --git a/.golangci.yml b/.golangci.yml
@@ -41,6 +41,7 @@ linters:
     - gofmt
     - goimports
     - gosimple
+    - gosec
     - govet
     - ineffassign
     - misspell

diff --git a/collectors/metrics/cmd/metrics-collector/main.go b/collectors/metrics/cmd/metrics-collector/main.go
@@ -7,7 +7,6 @@ import (
 	"errors"
 	"fmt"
 	stdlog "log"
-	"net"
 	"net/http"
 	"net/url"
 	"os"
@@ -362,21 +361,24 @@ func (o *Options) Run() error {
 			return worker.Reconfigure(*cfg)
 		})
 		handlers.Handle("/federate", serveLastMetrics(o.Logger, worker))
-		l, err := net.Listen("tcp", o.Listen)
-		if err != nil {
-			return fmt.Errorf("failed to listen: %w", err)
+		s := http.Server{
+			Addr:              o.Listen,
+			Handler:           handlers,
+			ReadHeaderTimeout: 5 * time.Second,
+			ReadTimeout:       15 * time.Second,
+			WriteTimeout:      12 * time.Minute,
 		}
 
 		{
 			// Run the HTTP server.
 			g.Add(func() error {
-				if err := http.Serve(l, handlers); err != nil && err != http.ErrServerClosed {
+				if err := s.ListenAndServe(); err != nil && err != http.ErrServerClosed {
 					logger.Log(o.Logger, logger.Error, "msg", "server exited unexpectedly", "err", err)
 					return err
 				}
 				return nil
 			}, func(error) {
-				err := l.Close()
+				err := s.Shutdown(context.Background())
 				if err != nil {
 					logger.Log(o.Logger, logger.Error, "msg", "failed to close listener", "err", err)
 				}

diff --git a/collectors/metrics/pkg/forwarder/forwarder.go b/collectors/metrics/pkg/forwarder/forwarder.go
@@ -122,7 +122,7 @@ func CreateFromClient(cfg Config, metrics *workerMetrics, interval time.Duration
 		fromTransport.TLSClientConfig.RootCAs = pool
 	} else {
 		if fromTransport.TLSClientConfig == nil {
-			/* #nosec G402*/
+			// #nosec G402 -- Only used if no TLS config is provided.
 			fromTransport.TLSClientConfig = &tls.Config{
 				MinVersion:         tls.VersionTLS12,
 				InsecureSkipVerify: true,

diff --git a/operators/endpointmetrics/controllers/observabilityendpoint/ocp_monitoring_config.go b/operators/endpointmetrics/controllers/observabilityendpoint/ocp_monitoring_config.go
@@ -21,8 +21,8 @@ import (
 )
 
 const (
-	hubAmAccessorSecretName        = "observability-alertmanager-accessor" // #nosec
-	hubAmAccessorSecretKey         = "token"
+	hubAmAccessorSecretName        = "observability-alertmanager-accessor" // #nosec G101 -- Not a hardcoded credential.
+	hubAmAccessorSecretKey         = "token"                               // #nosec G101 -- Not a hardcoded credential.
 	hubAmRouterCASecretName        = "hub-alertmanager-router-ca"
 	hubAmRouterCASecretKey         = "service-ca.crt"
 	clusterMonitoringConfigName    = "cluster-monitoring-config"

diff --git a/operators/endpointmetrics/pkg/util/lease.go b/operators/endpointmetrics/pkg/util/lease.go
@@ -15,7 +15,7 @@ import (
 )
 
 const (
-	leaseName = "observability-controller"
+	leaseName = "observability-controller" // #nosec G101 -- Not a hardcoded credential.
 )
 
 var (

diff --git a/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go b/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go
@@ -561,7 +561,6 @@ func (r *PlacementRuleReconciler) SetupWithManager(mgr ctrl.Manager) error {
 					"obsAddonName",
 					e.Object.GetName(),
 				)
-				/* #nosec */
 				if err := removePostponeDeleteAnnotationForManifestwork(c, e.Object.GetNamespace()); err != nil {
 					log.Error(err, "postpone delete annotation for manifestwork could not be removed")
 					return false
@@ -775,7 +774,6 @@ func (r *PlacementRuleReconciler) SetupWithManager(mgr ctrl.Manager) error {
 			if e.Object.GetName() == config.AlertmanagerAccessorSAName &&
 				e.Object.GetNamespace() == config.GetDefaultNamespace() {
 				// wait 10s for access_token of alertmanager and generate the secret that contains the access_token
-				/* #nosec */
 				if err := wait.Poll(2*time.Second, 10*time.Second, func() (bool, error) {
 					var err error
 					log.Info("generate amAccessorTokenSecret for alertmanager access serviceaccount CREATE")

diff --git a/operators/multiclusterobservability/controllers/placementrule/role.go b/operators/multiclusterobservability/controllers/placementrule/role.go
@@ -20,7 +20,7 @@ import (
 )
 
 const (
-	addonName          = "observability-controller"
+	addonName          = "observability-controller" // #nosec G101 -- Not a hardcoded credential.
 	resRoleName        = "endpoint-observability-res-role"
 	resRoleBindingName = "endpoint-observability-res-rolebinding"
 	mcoRoleName        = "endpoint-observability-mco-role"

diff --git a/operators/multiclusterobservability/pkg/certificates/cert_agent.go b/operators/multiclusterobservability/pkg/certificates/cert_agent.go
@@ -12,7 +12,7 @@ import (
 )
 
 const (
-	addonName = "observability-controller"
+	addonName = "observability-controller" // #nosec G101 -- Not a hardcoded credential.
 	agentName = "observability"
 )
 

diff --git a/operators/multiclusterobservability/pkg/config/config.go b/operators/multiclusterobservability/pkg/config/config.go
@@ -71,15 +71,13 @@ const (
 	GrafanaCN        = "grafana"
 	ManagedClusterOU = "acm"
 
-	GrafanaRouteName       = "grafana"
-	GrafanaServiceName     = "grafana"
-	GrafanaOauthClientName = "grafana-proxy-client"
-	/* #nosec */
-	GrafanaOauthClientSecret = "grafana-proxy-client"
-
-	AlertmanagerAccessorSAName = "observability-alertmanager-accessor"
-	/* #nosec */
-	AlertmanagerAccessorSecretName = "observability-alertmanager-accessor"
+	GrafanaRouteName         = "grafana"
+	GrafanaServiceName       = "grafana"
+	GrafanaOauthClientName   = "grafana-proxy-client" // #nosec G101 -- Not a hardcoded credential.
+	GrafanaOauthClientSecret = "grafana-proxy-client" // #nosec G101 -- Not a hardcoded credential.
+
+	AlertmanagerAccessorSAName     = "observability-alertmanager-accessor"
+	AlertmanagerAccessorSecretName = "observability-alertmanager-accessor" // #nosec G101 -- Not a hardcoded credential.
 	AlertmanagerServiceName        = "alertmanager"
 	AlertmanagerRouteName          = "alertmanager"
 	AlertmanagerRouteBYOCAName     = "alertmanager-byo-ca"
@@ -136,8 +134,8 @@ const (
 	MemcachedExporterImgTag  = "v0.9.0"
 
 	GrafanaImgKey               = "grafana"
-	GrafanaDashboardLoaderName  = "grafana-dashboard-loader"
-	GrafanaDashboardLoaderKey   = "grafana_dashboard_loader"
+	GrafanaDashboardLoaderName  = "grafana-dashboard-loader" // #nosec G101 -- Not a hardcoded credential.
+	GrafanaDashboardLoaderKey   = "grafana_dashboard_loader" // #nosec G101 -- Not a hardcoded credential.
 	GrafanaCustomDashboardLabel = "grafana-custom-dashboard"
 
 	AlertManagerImgName           = "prometheus-alertmanager"

diff --git a/operators/multiclusterobservability/pkg/rendering/renderer_proxy.go b/operators/multiclusterobservability/pkg/rendering/renderer_proxy.go
@@ -125,6 +125,7 @@ func (r *MCORenderer) renderProxySecret(res *resource.Resource,
 		return nil, err
 	}
 
+	// #nosec G101 -- Not a hardcoded credential.
 	if u.GetName() == "rbac-proxy-cookie-secret" {
 		obj := util.GetK8sObj(u.GetKind())
 		err = runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, obj)

diff --git a/operators/multiclusterobservability/pkg/util/clustermanagementaddon.go b/operators/multiclusterobservability/pkg/util/clustermanagementaddon.go
@@ -18,7 +18,7 @@ import (
 )
 
 const (
-	ObservabilityController       = "observability-controller"
+	ObservabilityController       = "observability-controller" // #nosec G101 -- Not a hardcoded credential.
 	AddonGroup                    = "addon.open-cluster-management.io"
 	AddonDeploymentConfigResource = "addondeploymentconfigs"
 	grafanaLink                   = "/d/2b679d600f3b9e7676a7c5ac3643d448/acm-clusters-overview"

diff --git a/operators/multiclusterobservability/pkg/util/managedclusteraddon.go b/operators/multiclusterobservability/pkg/util/managedclusteraddon.go
@@ -18,7 +18,7 @@ import (
 )
 
 const (
-	ManagedClusterAddonName = "observability-controller"
+	ManagedClusterAddonName = "observability-controller" // #nosec G101 -- Not a hardcoded credential.
 )
 
 var (

diff --git a/operators/pkg/config/config.go b/operators/pkg/config/config.go
@@ -6,7 +6,7 @@ package config
 const (
 	ClusterNameKey                  = "cluster-name"
 	HubInfoSecretName               = "hub-info-secret"
-	HubInfoSecretKey                = "hub-info.yaml" // #nosec
+	HubInfoSecretKey                = "hub-info.yaml" // #nosec G101 -- Not a hardcoded credential.
 	ObservatoriumAPIRemoteWritePath = "/api/metrics/v1/default/api/v1/receive"
 	AnnotationSkipCreation          = "skip-creation-if-exist"
 

diff --git a/proxy/cmd/main.go b/proxy/cmd/main.go
@@ -7,6 +7,7 @@ import (
 	"flag"
 	"net/http"
 	"os"
+	"time"
 
 	"github.com/spf13/pflag"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
@@ -84,8 +85,17 @@ func main() {
 	go util.ScheduleManagedClusterLabelAllowlistResync(kubeClient)
 	go util.CleanExpiredProjectInfoJob(24 * 60 * 60)
 
-	http.HandleFunc("/", proxy.HandleRequestAndRedirect)
-	if err := http.ListenAndServe(cfg.listenAddress, nil); err != nil {
+	handlers := http.NewServeMux()
+	handlers.HandleFunc("/", proxy.HandleRequestAndRedirect)
+	s := http.Server{
+		Addr:              cfg.listenAddress,
+		Handler:           handlers,
+		ReadHeaderTimeout: 5 * time.Second,
+		ReadTimeout:       15 * time.Second,
+		WriteTimeout:      12 * time.Minute,
+	}
+
+	if err := s.ListenAndServe(); err != nil {
 		klog.Fatalf("failed to ListenAndServe: %v", err)
 	}
 }
diff --git a/tests/pkg/utils/mco_dashboard.go b/tests/pkg/utils/mco_dashboard.go
@@ -31,7 +31,7 @@ func ContainDashboard(opt TestOptions, title string) (error, bool) {
 	client := &http.Client{}
 	if os.Getenv("IS_KIND_ENV") != "true" {
 		tr := &http.Transport{
-			// #nosec
+			// #nosec G402 -- Used in test.
 			TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
 		}
 

diff --git a/tests/pkg/utils/mco_deploy.go b/tests/pkg/utils/mco_deploy.go
@@ -29,7 +29,7 @@ const (
 	MCO_NAMESPACE                 = "open-cluster-management-observability"
 	MCO_ADDON_NAMESPACE           = "open-cluster-management-addon-observability"
 	MCO_PULL_SECRET_NAME          = "multiclusterhub-operator-pull-secret"
-	OBJ_SECRET_NAME               = "thanos-object-storage" // #nosec
+	OBJ_SECRET_NAME               = "thanos-object-storage" // #nosec G101 -- Not a hardcoded credential.
 	MCO_GROUP                     = "observability.open-cluster-management.io"
 	OCM_WORK_GROUP                = "work.open-cluster-management.io"
 	OCM_CLUSTER_GROUP             = "cluster.open-cluster-management.io"

diff --git a/tests/pkg/utils/mco_metric.go b/tests/pkg/utils/mco_metric.go
@@ -37,7 +37,7 @@ func ContainManagedClusterMetric(opt TestOptions, query string, matchedLabels []
 	client := &http.Client{}
 	if os.Getenv("IS_KIND_ENV") != "true" {
 		tr := &http.Transport{
-			/* #nosec */
+			// #nosec G402 -- Only used in test.
 			TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
 		}
-Original file line number
+Diff line change
@@ Expand Up / @@ -15,7 +15,7 @@ import ( @@
     )
     const (
-    	leaseName = "observability-controller"
+    	leaseName = "observability-controller" // #nosec G101 -- Not a hardcoded credential.
     )
     var (
@@ Expand Down @@