feat(security): add require_tls config to enforce HTTPS connections by nader-ziada · Pull Request #954 · containers/kubernetes-mcp-server

nader-ziada · 2026-03-23T21:05:11Z

Add require_tls configuration option that enforces TLS for all server and client connections when enabled

Changes:

All TLS configs now enforce MinVersion TLS 1.2
if require_tls is true
- Server refuses to start without TLS certificates configured if require_tls is true
- Outbound URLs (Kiali, authorization_url) must use HTTPS

Add require_tls configuration option that enforces TLS for all server and client connections when enabled Signed-off-by: Nader Ziada <nziada@redhat.com>

Cali0707

Overall this looks solid! A few things I would want to think about (not necessarily blocking)

What happens on SIGHUP - I don't think we would revalidate the tls bits with the enforce tls flag, but am not sure
Is it worth adding defence in depth somehow by adding a roundtripper that double checks the url the connection is going to is https? Or perhaps having a constructor for creating new http clients that adds a roundtripper to do such?

Cali0707 · 2026-03-23T21:30:14Z

+	// Validate URL scheme when require_tls is enabled
+	if config.RequireTLSFromContext(ctx) {
+		if err := config.ValidateURLRequiresTLS(cfg.Url, "Kiali URL"); err != nil {
+			return nil, err
+		}
+	}


will remembering to check any outgoing url on contributed toolsets be a requirement going forwards now?

No, toolset contributors don't need to manually validate URLs. The TLSEnforcingTransport handles this automatically at runtime, but toolset contributors need to use the IsRequireTLS function from the config provider to wrap their HTTP client when making HTTP requests

Kiali is currently the only toolset that makes direct outbound HTTP requests to an external service

Cali0707 · 2026-03-23T21:44:24Z

+	}
+	// Validate outbound URLs when require_tls is enabled
+	if m.StaticConfig.RequireTLS {
+		if err := config.ValidateURLRequiresTLS(m.StaticConfig.AuthorizationURL, "authorization_url"); err != nil {


maybe check m.StaticConfig.ServerURL and m.StaticConfig.SSEBaseURL as well? do we also need to validate the urls set within the kubeconfig file?

Also this would be more of a stretch but should we be validating the issuer urls in the token exchange config that a toolset can optionally return?

added validation for ServerURL and SSEBaseURL

token exchange issuer urls from toolsets: handled at runtime, TargetTokenExchangeConfig has a RequireTLS func() bool field, and HTTPClient() wraps the transport with TLSEnforcingTransport, so any non-https request just gets blocked at runtime. no explicit url validation needed

Cali0707 · 2026-03-23T21:47:34Z

+
+func (c *StaticConfig) IsRequireTLS() bool {
+	return c.RequireTLS
+}


maybe I missed this but I didn't see this consumed anywhere - it seems like we are only use the context accessor for this?

Context accessor is needed during TOML parsing (config object doesn't exist yet), func is used after config is built for runtime HTTP client wrapping.

manusa

Thanks for looking into this.
I think Calum made a lot of good points.

For me the most important ones are:

currently only validating certain outbound urls: kiali is covered now, but maybe others aren't. We should look into solutions for this (providing an HTTP client with enforced HTTPS consumption for toolset implementers, or providing a e NewRequireTLSClient() helper to enforce HTTPS in HTTP clients created by implementers, or provided HTTP enforced roundtripper, or anything along those lines.
Validation after SIGHUP reload: we definitely need to move the validation to a different stage or at least revalidate on config changes

manusa · 2026-03-24T10:27:13Z

+	if err != nil {
+		return fmt.Errorf("invalid %s: %w", fieldName, err)
+	}
+	if u.Scheme != "https" {


I'm not sure if some urls might attempt to connect to a non-http protocol such as a web socket (wss://

added wss since the only two protocols currently used in the codebase are http and ws so didn't want any other ones

nader-ziada · 2026-03-24T17:19:20Z

@Cali0707 @manusa

comments addressed by adding two-layer tls enforcement:

layer 1 (config-time): validates known config urls at startup/reload for fail-fast feedback
layer 2 (runtime): TLSEnforcingTransport wraps http clients and rejects non-https at the transport level, catches all outbound calls regardless of where the url came from (dynamic, toolset-specific, whatever we might miss in layer 1)
sighup: transport uses func() bool reading from Configuration.IsRequireTLS() per-request, so reloads take effect immediately, no re-validation needed, non-https just gets blocked if require_tls is true

Layer 1: Config-time URL validation for fail-fast feedback Layer 2: TLSEnforcingTransport wrapper that blocks non-HTTPS requests at runtime Signed-off-by: Nader Ziada <nziada@redhat.com>

Cali0707 · 2026-03-24T19:48:39Z


+	// RequireTLS is a function that returns whether TLS is required for outbound connections.
+	// When set and returns true, HTTP requests to non-HTTPS endpoints will be rejected.
+	RequireTLS func() bool `toml:"-"`


are we setting this anywhere where we are creating these configs?

this is a cli flag and a config to be added in the toml, not sure what you mean? I didn't set it be default

I believe that the problem is that this field IS NOT explicitly populated in the codebase, since this is a function and non-serializable field, it's effectively useless.

yeah removed it

and improve error message Signed-off-by: Nader Ziada <nziada@redhat.com>

manusa · 2026-03-25T11:07:22Z

+// IsRequireTLS returns whether TLS is required for outbound connections.
+// This method exists to support dynamic config reload via SIGHUP - it reads
+// from the current StaticConfig rather than being a promoted method bound
+// to a potentially stale config reference.
+func (c *Configuration) IsRequireTLS() bool {
+	return c.RequireTLS
+}


I think this is not needed since StaticConfig.IsRequireTLS should already be promoted.

manusa · 2026-03-25T11:08:51Z

+
+func (t *TLSEnforcingTransport) RoundTrip(req *http.Request) (*http.Response, error) {
+	if t.RequireTLS != nil && t.RequireTLS() && !isSecureHTTPScheme(req.URL.Scheme) {
+		klog.V(0).Infof("require_tls: blocked request to %s", req.URL.Host)


I'm not sure, but maybe the log level (0) is too low and might clutter the server logs. Are we sure 0 is the right choice here?

change to v(1) to be similar to authorization.go

Signed-off-by: Nader Ziada <nziada@redhat.com>

matzew · 2026-03-26T15:32:38Z

 	}

+	// Validate URL scheme when require_tls is enabled
+	if config.RequireTLSFromContext(ctx) {


shuld htis check on cfg.Insecure ?

i haven't been assuming that require_tls also means should reject Insecure=true because the admin explicitly sets Insecure=true knowing what it means

also let me know if you think this is not a good assumption

Agree that admins should know, but the value of require_tls is catching mis-configurations. Like someone copies a dev config with insecure = true to production, adds require_tls = true, and assumes they're covered. Perhaps some startup error/validation saves those from a false sense of security?

added error if insecure = true

matzew · 2026-03-26T15:37:49Z


+	transport.TLSClientConfig = tlsConfig
+
 	c.client = &http.Client{


the other outbound clients (well-known proxy, OIDC, Kiali) are wrapped with TLSEnforcingTransport. Should this one be wrapped too for consistency?

I think the reason was that the TokenURL is typed directly into a config file by an admin. If they write http://internal-keycloak:8080/, that's an explicit choice, not an accident. But i can add it if you don't think that's a good reason

For consistency I'd wrap this one too, better safe than sorry on these layered security thing and catching those for te admins/users?

looked into this more and the HTTPClient() code path is actually not used today since no provider implements TokenExchangeProvider, so the type assertion in ExchangeTokenInContext always fails and we never reach it

matzew · 2026-03-26T15:42:11Z

+
+func (t *TLSEnforcingTransport) RoundTrip(req *http.Request) (*http.Response, error) {
+	if t.RequireTLS != nil && t.RequireTLS() && !isSecureHTTPScheme(req.URL.Scheme) {
+		klog.V(1).Infof("require_tls: blocked request to %s", req.URL.Host)


like here, I guess there are more areas where we could do the same thing: when backend TLS handshakes fails (e.g. expired certs, protocol mismatches etc), they show up as generic HTTP errors in the logs.

Might be worth a follow-up to surface those more clearly for debugging.

Signed-off-by: Nader Ziada <nziada@redhat.com>

manusa

(deleted)

Dismissing stale review — re-reviewing with full context of existing discussions.

manusa

LGTM, thx!

feat(security): add require_tls config to enforce HTTPS connections

31457e9

Add require_tls configuration option that enforces TLS for all server and client connections when enabled Signed-off-by: Nader Ziada <nziada@redhat.com>

Cali0707 reviewed Mar 23, 2026

View reviewed changes

manusa requested changes Mar 24, 2026

View reviewed changes

add two-layer TLS enforcement for require_tls config

7a6ccfd

Layer 1: Config-time URL validation for fail-fast feedback Layer 2: TLSEnforcingTransport wrapper that blocks non-HTTPS requests at runtime Signed-off-by: Nader Ziada <nziada@redhat.com>

nader-ziada force-pushed the require-tls branch from 747c353 to 7a6ccfd Compare March 24, 2026 17:24

Cali0707 reviewed Mar 24, 2026

View reviewed changes

Comment thread pkg/config/tls.go Outdated

Cali0707 reviewed Mar 24, 2026

View reviewed changes

Comment thread pkg/config/tls.go Outdated

Cali0707 reviewed Mar 24, 2026

View reviewed changes

unify URL validation to use isSecureHTTPScheme for all checks

1a111fe

and improve error message Signed-off-by: Nader Ziada <nziada@redhat.com>

manusa self-requested a review March 25, 2026 10:50

manusa reviewed Mar 25, 2026

View reviewed changes

cleanup unused code

5d16bb5

Signed-off-by: Nader Ziada <nziada@redhat.com>

manusa requested a review from Cali0707 March 26, 2026 02:57

matzew reviewed Mar 26, 2026

View reviewed changes

error if require_tls is true and Insecure is true

d679cec

Signed-off-by: Nader Ziada <nziada@redhat.com>

github-actions Bot added the toolset/kiali label Mar 26, 2026

manusa previously requested changes Mar 30, 2026

View reviewed changes

manusa self-requested a review March 30, 2026 14:12

manusa added this to the 0.1.0 milestone Mar 30, 2026

manusa approved these changes Mar 30, 2026

View reviewed changes

manusa merged commit beed6f9 into containers:main Mar 30, 2026
9 checks passed

manusa mentioned this pull request Mar 30, 2026

test(security): add require_tls validation and enforcement tests #990

Merged

BrewTestBot mentioned this pull request Apr 1, 2026

kubernetes-mcp-server 0.0.60 Homebrew/homebrew-core#275560

Merged


		transport.TLSClientConfig = tlsConfig

		c.client = &http.Client{

Uh oh!

Conversation

nader-ziada commented Mar 23, 2026

Uh oh!

Cali0707 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manusa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nader-ziada commented Mar 24, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manusa left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manusa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

manusa left a comment •

edited

Loading