Skip to content

Commit

Permalink
fix defer unsafe Close vulnerability
Browse files Browse the repository at this point in the history
Signed-off-by: Subbarao Meduri <[email protected]>
  • Loading branch information
subbarao-meduri committed Oct 19, 2023
1 parent bc2dc3d commit 68a9947
Show file tree
Hide file tree
Showing 9 changed files with 62 additions and 40 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ func NewHypershiftTransformer(l log.Logger, c client.Client, labels map[string]s
//clusters := map[string]string{}
hClient := c
if hClient == nil {
if os.Getenv("UNIT_TEST") != "true" {
if _, ok := os.LookupEnv("UNIT_TEST"); !ok {
config, err := clientcmd.BuildConfigFromFlags("", "")
if err != nil {
return nil, errors.New("failed to create the kube config")
Expand Down
3 changes: 1 addition & 2 deletions collectors/metrics/pkg/metricsclient/metricsclient.go
Original file line number Diff line number Diff line change
Expand Up @@ -366,8 +366,7 @@ func withCancel(ctx context.Context, client *http.Client, req *http.Request, fn
}

func MTLSTransport(logger log.Logger, caCertFile, tlsCrtFile, tlsKeyFile string) (*http.Transport, error) {
testMode := os.Getenv("UNIT_TEST") != ""
if testMode {
if _, ok := os.LookupEnv("UNIT_TEST"); !ok {
caCertFile = "../../testdata/tls/ca.crt"
tlsKeyFile = "../../testdata/tls/tls.key"
tlsCrtFile = "../../testdata/tls/tls.crt"
Expand Down
18 changes: 10 additions & 8 deletions loaders/dashboards/pkg/util/grafana_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,14 +61,16 @@ func SetRequest(method string, url string, body io.Reader, retry int) ([]byte, i
resp, err = getHTTPClient().Do(req)
}

if resp != nil {
defer resp.Body.Close()
respBody, err := io.ReadAll(resp.Body)
if err != nil {
klog.Info("failed to parse response body ", "error ", err)
}
return respBody, resp.StatusCode
} else {
if resp == nil {
return nil, http.StatusNotFound
}

respBody, err := io.ReadAll(resp.Body)
if err != nil {
klog.Info("failed to parse response body ", "error ", err)
resp.Body.Close()

Check warning

Code scanning / SonarCloud

Errors unhandled. See more on SonarCloud Warning

Errors unhandled. See more on SonarCloud
return nil, resp.StatusCode
}
resp.Body.Close()

Check warning

Code scanning / SonarCloud

Errors unhandled. See more on SonarCloud Warning

Errors unhandled. See more on SonarCloud
return respBody, resp.StatusCode
}
2 changes: 1 addition & 1 deletion operators/endpointmetrics/pkg/util/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ const (

// GetOrCreateOCPClient get an existing hub client or create new one if it doesn't exist.
func GetOrCreateHubClient(renew bool) (client.Client, error) {
if os.Getenv("UNIT_TEST") == "true" {
if _, ok := os.LookupEnv("UNIT_TEST"); !ok {
return hubClient, nil
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
corev1 "k8s.io/api/core/v1"
rbacv1 "k8s.io/api/rbac/v1"
storev1 "k8s.io/api/storage/v1"
"k8s.io/apimachinery/pkg/api/errors"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/meta"
"k8s.io/apimachinery/pkg/api/resource"
Expand Down Expand Up @@ -72,6 +71,7 @@ var (
isReceiveStorageSizeChanged = false
isStoreStorageSizeChanged = false
isLegacyResourceRemoved = false
lastLogTime = time.Now()
)

// MultiClusterObservabilityReconciler reconciles a MultiClusterObservability object
Expand Down Expand Up @@ -170,6 +170,14 @@ func (r *MultiClusterObservabilityReconciler) Reconcile(ctx context.Context, req
mchCrdExists := r.CRDMap[config.MCHCrdName]
// requeue after 10 seconds if the mch crd exists and image image manifests map is empty
if mchCrdExists && len(config.GetImageManifests()) == 0 {
currentTime := time.Now()

// Log the message if it has been longer than 5 minutes since the last log
if currentTime.Sub(lastLogTime) > 5*time.Minute {
reqLogger.Info("Waiting for the mch CR to be ready", "mchCrdExists", mchCrdExists, "imageManifests", len(config.GetImageManifests()))
lastLogTime = currentTime
}

// if the mch CR is not ready, then requeue the request after 10s
return ctrl.Result{RequeueAfter: 10 * time.Second}, nil
}
Expand Down Expand Up @@ -321,8 +329,7 @@ func (r *MultiClusterObservabilityReconciler) Reconcile(ctx context.Context, req
}
}

if os.Getenv("UNIT_TEST") != "true" && !isLegacyResourceRemoved {
isLegacyResourceRemoved = true
if _, ok := os.LookupEnv("UNIT_TEST"); !ok && !isLegacyResourceRemoved {
// Delete PrometheusRule from openshift-monitoring namespace
if err := r.deleteSpecificPrometheusRule(ctx); err != nil {
reqLogger.Error(err, "Failed to delete the specific PrometheusRule in the openshift-monitoring namespace")
Expand All @@ -333,6 +340,7 @@ func (r *MultiClusterObservabilityReconciler) Reconcile(ctx context.Context, req
reqLogger.Error(err, "Failed to delete service monitor in the openshift-monitoring namespace")
return ctrl.Result{}, err
}
isLegacyResourceRemoved = true
}

//update status
Expand Down Expand Up @@ -626,19 +634,19 @@ func GenerateAlertmanagerRoute(
log.Info("BYO CA/Certificate found for the Route of Alertmanager, will using BYO CA/certificate for the Route of Alertmanager")
amRouteCA, ok := amRouteBYOCaSrt.Data["tls.crt"]
if !ok {
return &ctrl.Result{}, cerr.New("Invalid BYO CA for the Route of Alertmanager")
return &ctrl.Result{}, cerr.New("invalid BYO CA for the Route of Alertmanager")
}
amGateway.Spec.TLS.CACertificate = string(amRouteCA)

amRouteCert, ok := amRouteBYOCertSrt.Data["tls.crt"]
if !ok {
return &ctrl.Result{}, cerr.New("Invalid BYO Certificate for the Route of Alertmanager")
return &ctrl.Result{}, cerr.New("invalid BYO Certificate for the Route of Alertmanager")
}
amGateway.Spec.TLS.Certificate = string(amRouteCert)

amRouteCertKey, ok := amRouteBYOCertSrt.Data["tls.key"]
if !ok {
return &ctrl.Result{}, cerr.New("Invalid BYO Certificate Key for the Route of Alertmanager")
return &ctrl.Result{}, cerr.New("invalid BYO Certificate Key for the Route of Alertmanager")
}
amGateway.Spec.TLS.Key = string(amRouteCertKey)
}
Expand Down Expand Up @@ -720,19 +728,19 @@ func GenerateProxyRoute(
log.Info("BYO CA/Certificate found for the Route of Proxy, will using BYO CA/certificate for the Route of Proxy")
proxyRouteCA, ok := proxyRouteBYOCaSrt.Data["tls.crt"]
if !ok {
return &ctrl.Result{}, cerr.New("Invalid BYO CA for the Route of Proxy")
return &ctrl.Result{}, cerr.New("invalid BYO CA for the Route of Proxy")
}
proxyGateway.Spec.TLS.CACertificate = string(proxyRouteCA)

proxyRouteCert, ok := proxyRouteBYOCertSrt.Data["tls.crt"]
if !ok {
return &ctrl.Result{}, cerr.New("Invalid BYO Certificate for the Route of Proxy")
return &ctrl.Result{}, cerr.New("invalid BYO Certificate for the Route of Proxy")
}
proxyGateway.Spec.TLS.Certificate = string(proxyRouteCert)

proxyRouteCertKey, ok := proxyRouteBYOCertSrt.Data["tls.key"]
if !ok {
return &ctrl.Result{}, cerr.New("Invalid BYO Certificate Key for the Route of Proxy")
return &ctrl.Result{}, cerr.New("invalid BYO Certificate Key for the Route of Proxy")
}
proxyGateway.Spec.TLS.Key = string(proxyRouteCertKey)
}
Expand Down Expand Up @@ -826,7 +834,7 @@ func (r *MultiClusterObservabilityReconciler) ensureOpenShiftNamespaceLabel(ctx
}

err := r.Client.Get(ctx, types.NamespacedName{Name: resNS}, existingNs)
if err != nil || errors.IsNotFound(err) {
if err != nil || apierrors.IsNotFound(err) {
log.Error(err, fmt.Sprintf("Failed to find namespace for Multicluster Operator: %s", resNS))
return reconcile.Result{Requeue: true}, err
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -468,7 +468,7 @@ func newAPISpec(c client.Client, mco *mcov1beta2.MultiClusterObservability) (obs
data, ok := storageSecret.Data[storageConfig.Key]
if !ok {
log.Error(err, "Invalid key in secret", "name", storageConfig.Name, "key", storageConfig.Key)
return apiSpec, fmt.Errorf("Invalid key %s in secret %s", storageConfig.Key, storageConfig.Name)
return apiSpec, fmt.Errorf("invalid key %s in secret %s", storageConfig.Key, storageConfig.Name)
}
ep := &mcoutil.RemoteWriteEndpointWithSecret{}
err = yaml.Unmarshal(data, ep)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (

"gopkg.in/yaml.v2"
corev1 "k8s.io/api/core/v1"
v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
Expand All @@ -20,9 +19,8 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client/fake"

mcoshared "github.com/stolostron/multicluster-observability-operator/operators/multiclusterobservability/api/shared"
oashared "github.com/stolostron/multicluster-observability-operator/operators/multiclusterobservability/api/shared"
mcov1beta2 "github.com/stolostron/multicluster-observability-operator/operators/multiclusterobservability/api/v1beta2"
"github.com/stolostron/multicluster-observability-operator/operators/multiclusterobservability/pkg/config"

mcoconfig "github.com/stolostron/multicluster-observability-operator/operators/multiclusterobservability/pkg/config"
mcoutil "github.com/stolostron/multicluster-observability-operator/operators/multiclusterobservability/pkg/util"
observatoriumv1alpha1 "github.com/stolostron/observatorium-operator/api/v1alpha1"
Expand All @@ -34,8 +32,8 @@ var (

func TestNewVolumeClaimTemplate(t *testing.T) {
vct := newVolumeClaimTemplate("10Gi", "test")
if vct.Spec.AccessModes[0] != v1.ReadWriteOnce ||
vct.Spec.Resources.Requests[v1.ResourceStorage] != resource.MustParse("10Gi") {
if vct.Spec.AccessModes[0] != corev1.ReadWriteOnce ||
vct.Spec.Resources.Requests[corev1.ResourceStorage] != resource.MustParse("10Gi") {
t.Errorf("Failed to newVolumeClaimTemplate")
}
}
Expand Down Expand Up @@ -81,7 +79,7 @@ func TestNewDefaultObservatoriumSpec(t *testing.T) {
writeStorageS := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "write_name",
Namespace: config.GetDefaultNamespace(),
Namespace: mcoconfig.GetDefaultNamespace(),
},
Type: "Opaque",
Data: map[string][]byte{
Expand Down Expand Up @@ -120,7 +118,7 @@ func TestNewDefaultObservatoriumSpec(t *testing.T) {
endpointS := &corev1.Secret{}
err := cl.Get(context.TODO(), types.NamespacedName{
Name: endpointsConfigName,
Namespace: config.GetDefaultNamespace(),
Namespace: mcoconfig.GetDefaultNamespace(),
}, endpointS)
if err != nil {
t.Errorf("Failed to get endpoint config secret due to %v", err)
Expand Down Expand Up @@ -214,7 +212,7 @@ func TestGetTLSSecretMountPath(t *testing.T) {
testCaseList := []struct {
name string
secret *corev1.Secret
storeConfig *oashared.PreConfiguredStorage
storeConfig *mcoshared.PreConfiguredStorage
expected string
}{

Expand All @@ -223,7 +221,7 @@ func TestGetTLSSecretMountPath(t *testing.T) {
&corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "test",
Namespace: config.GetDefaultNamespace(),
Namespace: mcoconfig.GetDefaultNamespace(),
},
Type: "Opaque",
Data: map[string][]byte{
Expand All @@ -234,7 +232,7 @@ config:
`),
},
},
&oashared.PreConfiguredStorage{
&mcoshared.PreConfiguredStorage{
Key: "thanos.yaml",
Name: "test",
},
Expand All @@ -245,7 +243,7 @@ config:
&corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "test-1",
Namespace: config.GetDefaultNamespace(),
Namespace: mcoconfig.GetDefaultNamespace(),
},
Type: "Opaque",
Data: map[string][]byte{
Expand All @@ -263,7 +261,7 @@ config:
`),
},
},
&oashared.PreConfiguredStorage{
&mcoshared.PreConfiguredStorage{
Key: "thanos.yaml",
Name: "test-1",
},
Expand All @@ -274,7 +272,7 @@ config:
&corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "test-2",
Namespace: config.GetDefaultNamespace(),
Namespace: mcoconfig.GetDefaultNamespace(),
},
Type: "Opaque",
Data: map[string][]byte{
Expand All @@ -292,7 +290,7 @@ config:
`),
},
},
&oashared.PreConfiguredStorage{
&mcoshared.PreConfiguredStorage{
Key: "thanos.yaml",
Name: "test-2",
},
Expand All @@ -307,6 +305,9 @@ config:
t.Errorf("failed to create object storage secret, due to %v", err)
}
path, err := getTLSSecretMountPath(client, c.storeConfig)
if err != nil {
t.Errorf("failed to get tls secret mount path, due to %v", err)
}
if path != c.expected {
t.Errorf("case (%v) output: (%v) is not the expected: (%v)", c.name, path, c.expected)
}
Expand Down
12 changes: 10 additions & 2 deletions proxy/pkg/util/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -427,14 +427,18 @@ func FetchUserProjectList(token string, url string) []string {
writeError(fmt.Sprintf("failed to send http request: %v", err))
return []string{}
}
defer resp.Body.Close()

var projects projectv1.ProjectList
err = json.NewDecoder(resp.Body).Decode(&projects)
if err != nil {
klog.Errorf("failed to decode response json body: %v", err)
resp.Body.Close()

Check warning

Code scanning / SonarCloud

Errors unhandled. See more on SonarCloud Warning

Errors unhandled. See more on SonarCloud
return []string{}
}
err = resp.Body.Close()
if err != nil {
klog.Errorf("failed to close response body: %v", err)
}

projectList := make([]string, len(projects.Items))
for idx, p := range projects.Items {
Expand All @@ -451,14 +455,18 @@ func GetUserName(token string, url string) string {
writeError(fmt.Sprintf("failed to send http request: %v", err))
return ""
}
defer resp.Body.Close()

user := userv1.User{}
err = json.NewDecoder(resp.Body).Decode(&user)
if err != nil {
klog.Errorf("failed to decode response json body: %v", err)
resp.Body.Close()

Check warning

Code scanning / SonarCloud

Errors unhandled. See more on SonarCloud Warning

Errors unhandled. See more on SonarCloud
return ""
}
err = resp.Body.Close()
if err != nil {
klog.Errorf("failed to close response body: %v", err)
}

return user.Name
}
Expand Down
6 changes: 5 additions & 1 deletion tests/pkg/utils/mco_pods.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,12 +65,16 @@ func GetPodLogs(
klog.Errorf("Failed to get logs for %s/%s in namespace %s due to %v", podName, containerName, namespace, err)
return "", err
}
defer podLogs.Close()
buf := new(bytes.Buffer)
_, err = io.Copy(buf, podLogs)
if err != nil {
klog.Errorf("Failed to copy pod logs to buffer due to %v", err)
podLogs.Close()

Check warning

Code scanning / SonarCloud

Errors unhandled. See more on SonarCloud Warning test

Errors unhandled. See more on SonarCloud
return "", err
}
err = podLogs.Close()
if err != nil {
klog.Errorf("Failed to close pod logs due to %v", err)
}
return buf.String(), nil
}

0 comments on commit 68a9947

Please sign in to comment.