[COST-7998] Restrict prometheus service_address to in-cluster .svc urls (#1050)

* [COST-7998] restrict prometheus service_address to in-cluster .svc urls

* clean up comment

* restrict prometheus service_address to thanos-querier

Author: djnakabaale

internal/collector/prometheus.go

@@ -9,9 +9,11 @@ import (
 	"context"
 	"fmt"
 	"math"
+	"net/url"
 	"os"
 	"path/filepath"
 	"reflect"
+	"strings"
 	"time"
 
 	promapi "github.com/prometheus/client_golang/api"
@@ -89,7 +91,35 @@ func statusHelper(cr *metricscfgv1beta1.MetricsConfig, status int, err error) {
 
 type PrometheusConfigurationSetter func(ps *metricscfgv1beta1.PrometheusSpec, c *PrometheusCollector) error
 
+const (
+	thanosQuerierSvcHost             = "thanos-querier.openshift-monitoring.svc"
+	thanosQuerierSvcClusterLocalHost = "thanos-querier.openshift-monitoring.svc.cluster.local"
+)
+
+// IsAllowedPromSvcAddress reports whether address is an https URL for the
+// OpenShift cluster-monitoring thanos-querier Service.
+func IsAllowedPromSvcAddress(address string) bool {
+	u, err := url.Parse(address)
+	if err != nil || u.Host == "" {
+		return false
+	}
+	if !strings.EqualFold(u.Scheme, "https") {
+		return false
+	}
+	// DNS hostnames are case-insensitive; normalize before exact host checks.
+	switch strings.ToLower(u.Hostname()) {
+	case thanosQuerierSvcHost, thanosQuerierSvcClusterLocalHost:
+		return true
+	default:
+		return false
+	}
+}
+
 func SetPrometheusConfig(ps *metricscfgv1beta1.PrometheusSpec, c *PrometheusCollector) error {
+	// Validate before reading the SA token so a rejected address never loads credentials.
+	if !IsAllowedPromSvcAddress(ps.SvcAddress) {
+		return fmt.Errorf("service_address must be the OpenShift thanos-querier service (%s or %s); got %q", thanosQuerierSvcHost, thanosQuerierSvcClusterLocalHost, ps.SvcAddress)
+	}
 
 	pCfg := &PrometheusConfig{
 		Address: ps.SvcAddress,

internal/collector/prometheus_test.go

@@ -11,6 +11,7 @@ import (
 	"os"
 	"path/filepath"
 	"reflect"
+	"strings"
 	"testing"
 	"time"
 
@@ -25,6 +26,7 @@ import (
 var trueDef = true
 var falseDef = false
 var defaultContextTimeout = 90 * time.Second
+var promSvcAddress = metricscfgv1beta1.DefaultPrometheusSvcAddress
 
 type mappedMockPromResult map[string]*mockPromResult
 type mockPromResult struct {
@@ -558,6 +560,7 @@ func TestGetPromConn(t *testing.T) {
 			statusHelper(cr, statusConfiguration, tt.cfgErr)
 			statusHelper(cr, statusConnection, tt.conErr)
 			cr.Spec.PrometheusConfig.SkipTLSVerification = &trueDef
+			cr.Spec.PrometheusConfig.SvcAddress = promSvcAddress
 			c := &PrometheusCollector{
 				PromConn: tt.con,
 				PromCfg:  tt.cfg,
@@ -575,10 +578,102 @@ func TestGetPromConn(t *testing.T) {
 	}
 }
 
+func TestIsAllowedPrometheusServiceAddress(t *testing.T) {
+	tests := []struct {
+		name    string
+		address string
+		want    bool
+	}{
+		{
+			name:    "default thanos-querier .svc address",
+			address: promSvcAddress,
+			want:    true,
+		},
+		{
+			name:    "thanos-querier with cluster.local suffix",
+			address: "https://thanos-querier.openshift-monitoring.svc.cluster.local:9091",
+			want:    true,
+		},
+		{
+			name:    "thanos-querier without explicit port",
+			address: "https://thanos-querier.openshift-monitoring.svc",
+			want:    true,
+		},
+		{
+			name:    "address with trailing slash",
+			address: "https://thanos-querier.openshift-monitoring.svc:9091/",
+			want:    true,
+		},
+		{
+			name:    "uppercase scheme and hostname are allowed",
+			address: "HTTPS://THANOS-QUERIER.OPENSHIFT-MONITORING.SVC:9091",
+			want:    true,
+		},
+		{
+			name:    "mixed-case .svc.cluster.local hostname is allowed",
+			address: "https://Thanos-Querier.OpenShift-Monitoring.SVC.Cluster.Local:9091",
+			want:    true,
+		},
+		{
+			name:    "custom in-cluster prometheus service rejected",
+			address: "https://prometheus.custom-monitoring.svc:9090",
+			want:    false,
+		},
+		{
+			name:    "evil service in openshift-monitoring rejected",
+			address: "https://evil.openshift-monitoring.svc:9091",
+			want:    false,
+		},
+		{
+			name:    "other service in openshift-monitoring rejected",
+			address: "https://prometheus-k8s.openshift-monitoring.svc:9091",
+			want:    false,
+		},
+		{
+			name:    "CRC route address",
+			address: "https://thanos-querier-openshift-monitoring.apps-crc.testing",
+			want:    false,
+		},
+		{
+			name:    "external malicious URL",
+			address: "https://evil.example.com",
+			want:    false,
+		},
+		{
+			name:    "http scheme rejected",
+			address: "http://thanos-querier.openshift-monitoring.svc:9091",
+			want:    false,
+		},
+		{
+			name:    "empty address",
+			address: "",
+			want:    false,
+		},
+		{
+			name:    "hostname that contains svc but is not thanos-querier",
+			address: "https://evil.svc.attacker.com",
+			want:    false,
+		},
+		{
+			name:    "IP address rejected",
+			address: "https://192.168.1.10:9091",
+			want:    false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := IsAllowedPromSvcAddress(tt.address)
+			if got != tt.want {
+				t.Errorf("IsAllowedPromSvcAddress(%q) = %v, want %v", tt.address, got, tt.want)
+			}
+		})
+	}
+}
+
 func TestSetPrometheusConfig(t *testing.T) {
 	trueDef := true
 	ps := &metricscfgv1beta1.PrometheusSpec{
-		SvcAddress:          "svc-address",
+		SvcAddress:          promSvcAddress,
 		SkipTLSVerification: &trueDef,
 	}
 	secretsPath := "./test_files/test_secrets"
@@ -588,6 +683,7 @@ func TestSetPrometheusConfig(t *testing.T) {
 		basePath    string
 		certKey     bool
 		tokenKey    bool
+		address     string
 		want        *PrometheusConfig
 		wantedError error
 	}{
@@ -597,7 +693,7 @@ func TestSetPrometheusConfig(t *testing.T) {
 			certKey:  true,
 			tokenKey: true,
 			want: &PrometheusConfig{
-				Address:     "svc-address",
+				Address:     promSvcAddress,
 				SkipTLS:     true,
 				BearerToken: config.Secret([]byte("this-is-token-data")),
 				CAFile:      filepath.Join(secretsPath, certKey),
@@ -618,7 +714,7 @@ func TestSetPrometheusConfig(t *testing.T) {
 			certKey:  true,
 			tokenKey: true,
 			want: &PrometheusConfig{
-				Address:     "svc-address",
+				Address:     promSvcAddress,
 				SkipTLS:     true,
 				BearerToken: config.Secret([]byte("this-is-token-data")),
 				CAFile:      filepath.Join(secretsPath, certKey),
@@ -640,6 +736,14 @@ func TestSetPrometheusConfig(t *testing.T) {
 			want:        nil,
 			wantedError: errTest,
 		},
+		{
+			name:        "rejected external service_address does not require token",
+			basePath:    "",
+			tokenKey:    false,
+			address:     "https://evil.example.com",
+			want:        nil,
+			wantedError: errTest,
+		},
 	}
 	for _, tt := range setPrometheusConfigTests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -654,14 +758,21 @@ func TestSetPrometheusConfig(t *testing.T) {
 				os.Remove(filepath.Join(tt.basePath, tokenKey))
 				os.Remove(filepath.Join(tt.basePath, certKey))
 			}()
-			err := SetPrometheusConfig(ps, c)
+			spec := *ps
+			if tt.address != "" {
+				spec.SvcAddress = tt.address
+			}
+			err := SetPrometheusConfig(&spec, c)
 			got := c.PromCfg
 			if tt.wantedError == nil && err != nil {
 				t.Errorf("%s got unexpected error: %v", tt.name, err)
 			}
 			if tt.wantedError != nil && err == nil {
 				t.Errorf("%s expected error, got %v", tt.name, err)
 			}
+			if tt.address != "" && err != nil && !strings.Contains(err.Error(), "thanos-querier") {
+				t.Errorf("%s expected thanos-querier validation error, got %v", tt.name, err)
+			}
 			if got != nil && !reflect.DeepEqual(*got, *tt.want) {
 				t.Errorf("%s got %+v want %+v", tt.name, got, tt.want)
 			}

internal/controller/costmanagementmetricsconfig_controller_test.go

@@ -1338,6 +1338,32 @@ var _ = Describe("MetricsConfigController - CRD Handling", Ordered, func() {
 
 			Expect(fetched.Status.Prometheus.ConfigError).To(ContainSubstring("failed to get token"))
 		})
+		It("rejects external service_address before querying prometheus", func() {
+			resetReconciler(WithSecretOverride(os.Getenv("SECRET_ABSPATH")))
+
+			t := time.Now().UTC().Truncate(1 * time.Hour).Add(-1 * time.Hour)
+			timeRange := promv1.Range{
+				Start: t,
+				End:   t.Add(59*time.Minute + 59*time.Second),
+				Step:  time.Minute,
+			}
+			mockpconn.EXPECT().QueryRange(gomock.Any(), gomock.Any(), timeRange, gomock.Any()).Return(model.Matrix{}, nil, nil).Times(0)
+
+			instCopy.Spec.Upload.UploadToggle = &falseValue
+			instCopy.Spec.PrometheusConfig.SvcAddress = "https://evil.example.com"
+			createObject(ctx, instCopy)
+
+			fetched := &metricscfgv1beta1.MetricsConfig{}
+
+			Eventually(func() bool {
+				_ = k8sClient.Get(ctx, types.NamespacedName{Name: instCopy.Name, Namespace: namespace}, fetched)
+				return fetched.Status.Prometheus.ConfigError != ""
+			}, timeout, interval).Should(BeTrue())
+
+			Expect(fetched.Status.Prometheus.PrometheusConfigured).To(BeFalse())
+			Expect(fetched.Status.Prometheus.ConfigError).To(ContainSubstring("thanos-querier"))
+			Expect(fetched.Status.Prometheus.ConfigError).To(ContainSubstring("evil.example.com"))
+		})
 		It("successfully queried but there was no data", func() {
 			resetReconciler(WithSecretOverride(os.Getenv("SECRET_ABSPATH")))