Skip to content

CNTRLPLANE-3973: Make sure unencrypted metrics bind to localhost only, and remove dead code #6350

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vincentdephily wants to merge 2 commits into
base: main
Choose a base branch
from
+32 −31
Open

CNTRLPLANE-3973: Make sure unencrypted metrics bind to localhost only, and remove dead code #6350

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ed259b
Make sure metrics bind to localhost, and remove dead TLS code
vincentdephily Jul 30, 2026
f6a6dec
Deprecate unused `--tls-min-version` and `--tls-cipher-suites` for MC…
vincentdephily Jul 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions cmd/machine-config-controller/start.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,6 @@ var (
templates string
promMetricsListenAddress string
resourceLockNamespace string
tlsCipherSuites []string
tlsMinVersion string
streamsCache string
}
)
Expand All @@ -57,10 +55,12 @@ func init() {
rootCmd.AddCommand(startCmd)
startCmd.PersistentFlags().StringVar(&startOpts.kubeconfig, "kubeconfig", "", "Kubeconfig file to access a remote cluster (testing only)")
startCmd.PersistentFlags().StringVar(&startOpts.resourceLockNamespace, "resourcelock-namespace", metav1.NamespaceSystem, "Path to the template files used for creating MachineConfig objects")
startCmd.PersistentFlags().StringVar(&startOpts.promMetricsListenAddress, "metrics-listen-address", "127.0.0.1:8797", "Listen address for prometheus metrics listener")
startCmd.PersistentFlags().StringSliceVar(&startOpts.tlsCipherSuites, "tls-cipher-suites", nil, "Comma-separated list of cipher suites for the metrics server")
startCmd.PersistentFlags().StringVar(&startOpts.tlsMinVersion, "tls-min-version", "VersionTLS12", "Minimum TLS version supported for the metrics server")
startCmd.PersistentFlags().StringVar(&startOpts.promMetricsListenAddress, "metrics-listen-address", ctrlcommon.DefaultMetricsBindAddress, "Listen address for prometheus metrics listener")
startCmd.PersistentFlags().StringVar(&startOpts.streamsCache, "streams-cache", "/var/cache/mcc", "Directory to use as cache for streams discovery")
startCmd.PersistentFlags().StringSlice("tls-cipher-suites", nil, "")
startCmd.PersistentFlags().String("tls-min-version", "", "")
_ = startCmd.PersistentFlags().MarkDeprecated("tls-cipher-suites", "always using the APIServer setting")
_ = startCmd.PersistentFlags().MarkDeprecated("tls-min-version", "always using the APIServer setting")
}

func runStartCmd(_ *cobra.Command, _ []string) {
Expand Down Expand Up @@ -148,7 +148,7 @@ func runStartCmd(_ *cobra.Command, _ []string) {
}
}

go ctrlcommon.StartMetricsListener(startOpts.promMetricsListenAddress, ctx.Done(), ctrlcommon.RegisterMCCMetrics, startOpts.tlsMinVersion, startOpts.tlsCipherSuites)
go ctrlcommon.StartMetricsListener(startOpts.promMetricsListenAddress, ctx.Done(), ctrlcommon.RegisterMCCMetrics)

controllers := createControllers(ctrlctx, inspectionCache, inspectorFactory)
draincontroller := drain.New(
Expand Down
12 changes: 6 additions & 6 deletions cmd/machine-config-daemon/start.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,6 @@ var (
kubeletHealthzEnabled bool
kubeletHealthzEndpoint string
promMetricsURL string
tlsCipherSuites []string
tlsMinVersion string
}
)

Expand All @@ -58,9 +56,11 @@ func init() {
startCmd.PersistentFlags().BoolVar(&startOpts.skipReboot, "skip-reboot", false, "Skips reboot after a sync, applies only in once-from")
startCmd.PersistentFlags().BoolVar(&startOpts.kubeletHealthzEnabled, "kubelet-healthz-enabled", true, "kubelet healthz endpoint monitoring")
startCmd.PersistentFlags().StringVar(&startOpts.kubeletHealthzEndpoint, "kubelet-healthz-endpoint", "http://localhost:10248/healthz", "healthz endpoint to check health")
startCmd.PersistentFlags().StringVar(&startOpts.promMetricsURL, "metrics-url", "127.0.0.1:8797", "URL for prometheus metrics listener")
startCmd.PersistentFlags().StringSliceVar(&startOpts.tlsCipherSuites, "tls-cipher-suites", nil, "Comma-separated list of cipher suites for the metrics server")
startCmd.PersistentFlags().StringVar(&startOpts.tlsMinVersion, "tls-min-version", "VersionTLS12", "Minimum TLS version supported for the metrics server")
startCmd.PersistentFlags().StringVar(&startOpts.promMetricsURL, "metrics-url", ctrlcommon.DefaultMetricsBindAddress, "URL for prometheus metrics listener")
startCmd.PersistentFlags().StringSlice("tls-cipher-suites", nil, "")
startCmd.PersistentFlags().String("tls-min-version", "", "")
_ = startCmd.PersistentFlags().MarkDeprecated("tls-cipher-suites", "always using the APIServer setting")
_ = startCmd.PersistentFlags().MarkDeprecated("tls-min-version", "always using the APIServer setting")
}

//nolint:gocritic
Expand Down Expand Up @@ -181,7 +181,7 @@ func runStartCmd(_ *cobra.Command, _ []string) {
}

// Start local metrics listener
go ctrlcommon.StartMetricsListener(startOpts.promMetricsURL, stopCh, daemon.RegisterMCDMetrics, startOpts.tlsMinVersion, startOpts.tlsCipherSuites)
go ctrlcommon.StartMetricsListener(startOpts.promMetricsURL, stopCh, daemon.RegisterMCDMetrics)

ctrlctx := ctrlcommon.CreateControllerContext(ctx, cb)

Expand Down
2 changes: 0 additions & 2 deletions manifests/machineconfigcontroller/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@ spec:
- "--resourcelock-namespace={{.TargetNamespace}}"
- "--v={{.LogLevel}}"
- "--payload-version={{.ReleaseVersion}}"
- "--tls-cipher-suites={{join .TLSCipherSuites ","}}"
- "--tls-min-version={{.TLSMinVersion}}"
- "--streams-cache=/var/cache/mcc"
resources:
requests:
Expand Down
2 changes: 0 additions & 2 deletions manifests/machineconfigdaemon/daemonset.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@ spec:
- "start"
- "--payload-version={{.ReleaseVersion}}"
- "--v={{.LogLevel}}"
- "--tls-cipher-suites={{join .TLSCipherSuites ","}}"
- "--tls-min-version={{.TLSMinVersion}}"
resources:
requests:
cpu: 20m
Expand Down
35 changes: 20 additions & 15 deletions pkg/controller/common/metrics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ package common

import (
"context"
"crypto/tls"
"fmt"
"net"
"net/http"

"github.com/prometheus/client_golang/prometheus"
Expand All @@ -12,8 +12,8 @@ import (
)

const (
// DefaultBindAddress is the port for the metrics listener
DefaultBindAddress = ":8797"
// DefaultMetricsBindAddress is the port for the metrics listener
DefaultMetricsBindAddress = "127.0.0.1:8797"
)

// MCC Metrics
Expand Down Expand Up @@ -135,10 +135,22 @@ func RegisterMetrics(metrics []prometheus.Collector) error {
return nil
}

// StartMetricsListener is metrics listener via http on localhost
func StartMetricsListener(addr string, stopCh <-chan struct{}, registerFunc func() error, tlsMinVersion string, tlsCipherSuites []string) {
// StartMetricsListener starts the prometheus metrics listener
//
// It is unencrypted and should bind to localhost, with kube-rbac-proxy exposing a TLS endpoint outside localhost
func StartMetricsListener(addr string, stopCh <-chan struct{}, registerFunc func() error) {
if addr == "" {
addr = DefaultBindAddress
addr = DefaultMetricsBindAddress
}

host, _, err := net.SplitHostPort(addr)
if err != nil {
klog.Errorf("invalid metrics listen address %q: %v", addr, err)
return
}
if ip := net.ParseIP(host); ip == nil || !ip.IsLoopback() {
klog.Errorf("metrics is listening on %q, it should listen on localhost and be exposed via a TLS proxy", addr)
Comment on lines +146 to +152

@coderabbitai coderabbitai Bot Jul 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Avoid logging raw rejected listener addresses.

addr is operator-configurable; invalid values may contain internal hostnames and are written directly to logs. Redact the address or log only the validation failure.

As per coding guidelines, “Flag logging that may expose … internal hostnames.”

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/controller/common/metrics.go` around lines 146 - 152, Update the
validation logging around net.SplitHostPort and the loopback check to avoid
including the operator-configured addr value, which may expose internal
hostnames. Log only generic validation-failure messages or a redacted
representation, while preserving the existing early return and localhost
enforcement behavior.

Source: Coding guidelines

return
}

klog.Info("Registering Prometheus metrics")
Expand All @@ -148,17 +160,10 @@ func StartMetricsListener(addr string, stopCh <-chan struct{}, registerFunc func
return
}

// Get TLS config from provided settings, or use defaults
tlsConfig := GetGoTLSConfig(tlsMinVersion, tlsCipherSuites)

klog.Infof("Starting metrics listener on %s with TLS min version: %s", addr, tlsMinVersion)
klog.Infof("Starting metrics listener on %s", addr)
mux := http.NewServeMux()
mux.Handle("/metrics", promhttp.Handler())
s := http.Server{
TLSConfig: tlsConfig,
TLSNextProto: make(map[string]func(*http.Server, *tls.Conn, http.Handler)),
Addr: addr,
Handler: mux}
s := http.Server{Addr: addr, Handler: mux}
Comment on lines +163 to +166

@coderabbitai coderabbitai Bot Jul 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Bound request and shutdown time.

Without ReadHeaderTimeout, a same-pod client can retain connections while sending headers indefinitely; Shutdown(context.Background()) can then also wait forever. Configure server deadlines and use a bounded shutdown context.

As per path instructions, “context.Context for cancellation and timeouts.”

🧰 Tools
🪛 ast-grep (0.45.0)

[warning] 165-165: This http.Server is constructed without a ReadTimeout. Without a read timeout, a slow or malicious client can hold connections open indefinitely (e.g. a Slowloris attack), exhausting server resources and causing a denial of service. Set ReadTimeout (and ideally ReadHeaderTimeout, WriteTimeout, and IdleTimeout) on the http.Server to bound how long the server waits while reading a request.
Context: http.Server{Addr: addr, Handler: mux}
Note: [CWE-400] Uncontrolled Resource Consumption.

(http-server-missing-read-timeout-go)

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/controller/common/metrics.go` around lines 163 - 166, Update the HTTP
server setup in the metrics listener to configure appropriate request deadlines,
including ReadHeaderTimeout, and replace the unbounded
Shutdown(context.Background()) call with a context.Context that has a finite
timeout. Ensure shutdown uses that bounded context and is properly canceled.

Sources: Path instructions, Linters/SAST tools


go func() {
if err := s.ListenAndServe(); err != nil && err != http.ErrServerClosed {
Expand Down