From 5bb5ed30bf4f11cd72b299cae98e630a72a460e7 Mon Sep 17 00:00:00 2001 From: Saswata Mukherjee Date: Tue, 17 Oct 2023 20:19:57 +0530 Subject: [PATCH 1/2] Enable gosec as a linter and resolve potential security issues Signed-off-by: Saswata Mukherjee --- .golangci.yml | 1 + .../metrics/cmd/metrics-collector/main.go | 14 +++++++------ collectors/metrics/pkg/forwarder/forwarder.go | 2 +- .../ocp_monitoring_config.go | 4 ++-- operators/endpointmetrics/pkg/util/lease.go | 2 +- .../placementrule/placementrule_controller.go | 2 -- .../controllers/placementrule/role.go | 2 +- .../pkg/certificates/cert_agent.go | 2 +- .../pkg/config/config.go | 20 +++++++++---------- .../pkg/rendering/renderer_proxy.go | 1 + .../pkg/util/clustermanagementaddon.go | 2 +- .../pkg/util/managedclusteraddon.go | 2 +- operators/pkg/config/config.go | 2 +- proxy/cmd/main.go | 14 +++++++++++-- tests/pkg/utils/mco_dashboard.go | 2 +- tests/pkg/utils/mco_deploy.go | 2 +- tests/pkg/utils/mco_metric.go | 2 +- 17 files changed, 43 insertions(+), 33 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 06ea4a0e9..e8108bb2d 100644 --- 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 index dd1d7b392..e5c730f3a 100644 --- 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: 1 * time.Second, + ReadTimeout: 5 * 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 index d12ceb57a..8def2ffca 100644 --- 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 index df65626a1..349fe037c 100644 --- 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 index 2e2f4d947..37a0ab833 100644 --- 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 index 9bb87cfaa..bf9e602fc 100644 --- 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 index 05bea1ea7..b4219fd98 100644 --- 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 index 1665d361d..a3b314002 100644 --- 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 index ef8530706..3aa86dbc4 100644 --- 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 index 1abfa86bb..8a5341df4 100644 --- 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 index ba52d8229..4fbaded5c 100644 --- 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 index 428577d07..9cc66473e 100644 --- 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 index 9b3424f7f..58e9a3aa6 100644 --- 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 index 2fbb350be..82501dfa6 100644 --- 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: 1 * time.Second, + ReadTimeout: 5 * 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 index c2d84661a..c3a753b2c 100644 --- 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 index 66c923267..5bc6391c4 100644 --- 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 index 9a4884789..d2db02ff1 100644 --- 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}, } From 173025a7d16f749b8a0061baafb4785aefc66334 Mon Sep 17 00:00:00 2001 From: Saswata Mukherjee Date: Tue, 17 Oct 2023 20:46:14 +0530 Subject: [PATCH 2/2] Bump read and readHeader timeout Signed-off-by: Saswata Mukherjee --- collectors/metrics/cmd/metrics-collector/main.go | 4 ++-- proxy/cmd/main.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/collectors/metrics/cmd/metrics-collector/main.go b/collectors/metrics/cmd/metrics-collector/main.go index e5c730f3a..341be5075 100644 --- a/collectors/metrics/cmd/metrics-collector/main.go +++ b/collectors/metrics/cmd/metrics-collector/main.go @@ -364,8 +364,8 @@ func (o *Options) Run() error { s := http.Server{ Addr: o.Listen, Handler: handlers, - ReadHeaderTimeout: 1 * time.Second, - ReadTimeout: 5 * time.Second, + ReadHeaderTimeout: 5 * time.Second, + ReadTimeout: 15 * time.Second, WriteTimeout: 12 * time.Minute, } diff --git a/proxy/cmd/main.go b/proxy/cmd/main.go index 82501dfa6..caabe50b2 100644 --- a/proxy/cmd/main.go +++ b/proxy/cmd/main.go @@ -90,8 +90,8 @@ func main() { s := http.Server{ Addr: cfg.listenAddress, Handler: handlers, - ReadHeaderTimeout: 1 * time.Second, - ReadTimeout: 5 * time.Second, + ReadHeaderTimeout: 5 * time.Second, + ReadTimeout: 15 * time.Second, WriteTimeout: 12 * time.Minute, }