Skip to content

Add operator support for Redis session storage TLS - #6767

Open
peppescg wants to merge 9 commits into
mainfrom
redis-tls-session-followup
Open

peppescg wants to merge 9 commits into
mainfrom
redis-tls-session-followup

Conversation

@peppescg

@peppescg peppescg commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #6731 (merged), which added Redis TLS for session storage at the runtime level: proxy runner, vMCP and both rate limiters. Operator-managed workloads still had no way to request TLS, and turning it off went unnoticed. This PR completes the feature requested in #6730.

  • Operator support. MCPServer, MCPRemoteProxy and VirtualMCPServer can now set spec.sessionStorage.tls. Without it, the redisconfig.TLSConfig added in Add TLS to Redis session storage #6731 was only reachable through hand-written RunConfig or vMCP YAML. The field reuses the existing RedisTLSConfig type from the embedded auth server's Redis storage:
    • An empty object verifies the server certificate against the system roots.
    • caCertSecretRef mounts a private CA from a Secret (read-only, subPath, same pattern as the auth server's Redis CA).
    • insecureSkipVerify is an opt-out, for testing only.
    • The operator writes these settings into the RunConfig (scaling_config.session_redis.tls) and the vMCP config (sessionStorage.tls).
    • Switching the referenced CA Secret rolls the Deployment.
    • CEL rules reject tls with a non-redis provider, insecureSkipVerify together with caCertSecretRef, a caCertSecretRef with an empty name or key, and spec.config.sessionStorage.tls on a VirtualMCPServer (the converter discards it, so accepting it would silently leave the connection plaintext).
    • The CA volume drift check compares against the Deployment after any podTemplateSpec patch, so a patch that overrides the volume is not reported as drift on every reconcile.
  • Startup warning. Leaving TLS off is still allowed so existing deployments keep working, but it should be visible. The Redis session store constructors (used by the proxy runner and vMCP) now log one startup WARN if TLS is not set or certificate verification is disabled, whether or not a password is configured, since session data crosses the network in plaintext either way. The password is never logged.
  • Tests.
    • A Runner.Run-level wiring test.
    • More precise rate-limiter assertions.
    • An e2e spec running standalone vMCP against a TLS-only, password-protected Redis container.
    • The redistls fixture moved to test/testkit, where shared test fixtures live, and now uses a single certificate generator.

Closes #6730

Type of change

  • Bug fix
  • New feature
  • Refactoring (no behavior change)
  • Dependency update
  • Documentation
  • Other (describe):

Test plan

  • Unit tests (task test): full repository with -race, exit 0. The first run hit an unrelated flake in pkg/ignore (TestGetOverlayPaths, which passes in isolation); the rerun was clean
  • E2E tests (TOOLHIVE_SKIP_DESKTOP_CHECK=1 LABEL_FILTER=infra task test-e2e): 3/3 passed. The new spec "stores sessions over verified TLS with password authentication":
    • starts redis:7-alpine with only a TLS port and requirepass;
    • runs thv vmcp serve with sessionStorage.tls.caCertFile and THV_SESSION_REDIS_PASSWORD;
    • opens a Legacy session;
    • checks with redis-cli --tls that the session key exists.
  • Linting (task lint-fix, task lint): 0 issues
  • Manual testing (described below)

Manual testing and other checks:

  • task license-check: pass.
  • Operator code generation (operator-generate, operator-manifests, crdref-gen) and task docs: rerunning them produces no diff.
  • Operator envtest integration (operator-test-integration): all 12 suites passed. One unrelated timeout in the MCPToolConfig deletion spec passed on rerun. This includes the new CEL specs:
    • tls with provider memory is rejected;
    • insecureSkipVerify with caCertSecretRef is rejected;
    • an empty tls is accepted;
    • a CA reference is accepted.
  • After the review fixes: task lint-fix 0 issues; task test exit 0; operator-test-integration passed except one spec (MCPRemoteProxy AuthServerRef … externalAuthConfigRef only … without Failed phase), a race where the phase is checked before the MCPOIDCConfig is validated, which passed 8/8 when rerun alone. The two new CEL specs (empty caCertSecretRef name/key, spec.config.sessionStorage.tls) pass. The new podTemplateSpec drift test fails against the previous drift check and passes with the fix.
  • After the follow-up review: the sessionStorage.tls admission table (7 cases) passes on MCPServer, MCPRemoteProxy and VirtualMCPServer at both v1beta1 and v1alpha1 (envtest); task test exit 0; go test -race -count=20 -run TestIntegration ./pkg/authserver/ clean. The MCPOIDCConfig suite has a flaky deletion-protection spec that also fails 2/3 on clean main.

API Compatibility

  • This PR does not break the v1beta1 API, OR the api-break-allowed label is applied and the migration guidance is described above.

spec.sessionStorage.tls is a new optional field. Three of the CEL rules only constrain this new field. The fourth rejects spec.config.sessionStorage.tls on VirtualMCPServer, a field added in #6731 that is not in any release yet. No existing object becomes invalid. On the shared RedisTLSConfig type, only the insecureSkipVerify and caCertSecretRef descriptions change.

Changes

File Change
cmd/thv-operator/api/v1beta1/mcpserver_types.go SessionStorageConfig.TLS *RedisTLSConfig (optional) and three CEL rules
cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go Description-only: insecureSkipVerify is for testing (prefer caCertSecretRef); the doc notes that a missing CA Secret keeps the pod in ContainerCreating
cmd/thv-operator/api/v1beta1/virtualmcpserver_types.go CEL rule rejecting spec.config.sessionStorage.tls
cmd/thv-operator/pkg/controllerutil/session_redis_tls.go Maps CRD TLS to redisconfig.TLSConfig; builds the CA Secret volume and mount; detects volume drift against the desired volumes
cmd/thv-operator/pkg/controllerutil/authserver.go Auth server Redis CA volumes use the shared Secret-file volume helper
cmd/thv-operator/controllers/mcpserver_*, mcpremoteproxy_*, virtualmcpserver_deployment.go Populate RunConfig TLS; mount the CA in the proxy runner and vMCP Deployments; roll the Deployment when the CA Secret switches (the remote proxy compares against the podTemplateSpec-patched Deployment)
cmd/thv-operator/pkg/vmcpconfig/converter.go Sets vMCP sessionStorage.tls from spec.sessionStorage.tls
pkg/transport/session/redis_config.go, storage_redis.go, session_data_storage_redis.go Startup WARN from the session store constructors when TLS is not verified
pkg/runner/runner.go, pkg/vmcp/server/server.go store attribute on the runner's Redis INFO log; doc updates
pkg/vmcp/config/config.go Doc: on Kubernetes, use spec.sessionStorage.tls
test/testkit/redistls/ Moved from test/helpers/redistls; on-disk CA and IP-only server certificates from a single generator
Tests Operator controllers/controllerutil/converter/runconfig, envtest CEL, pkg/runner (Run-level), pkg/ratelimit, pkg/vmcp/ratelimit/factory, pkg/vmcp/server (warning), e2e test/e2e/vmcp_infra_features_test.go
pkg/authserver/server/provider.go Sets fosite's audience strategy explicitly and uses a fail-closed JWKS fetcher, to fix a data race (see notes)
cmd/thv-operator/test-integration/testutil/session_storage_tls.go Shared table of sessionStorage.tls admission cases
Generated / docs deepcopy, CRD manifests (4 CRDs), docs/operator/crd-api.md, cmd/vmcp/README.md, docs/arch/13-vmcp-scalability.md

Does this introduce a user-facing change?

Yes. Operator-managed workloads can enable TLS for Redis session storage, and the same settings apply to Redis-backed rate limiting:

spec:
  sessionStorage:
    provider: redis
    address: redis.example.com:6380
    passwordRef: {name: redis, key: password}
    tls:
      caCertSecretRef: {name: redis-ca, key: ca.crt}   # omit for system roots

If Redis session storage runs without TLS, or with insecureSkipVerify, the proxy runner and vMCP log a startup warning. On a VirtualMCPServer, spec.config.sessionStorage.tls is now rejected; use spec.sessionStorage.tls.

Special notes for reviewers

  • Behavior changes for existing deployments:

    • Omitting tls keeps the current behavior. The only visible change is the new startup WARN when TLS is not verified, with or without a password.
    • Adding tls, or switching the referenced CA Secret, rolls the Deployment.
    • A missing CA Secret or key leaves the pod in ContainerCreating, the same as for the embedded auth server's Redis CA.
    • The CA is mounted with subPath, so rotating the Secret's content needs a pod restart.
  • Where the CEL rules live: on SessionStorageConfig, not on the shared RedisTLSConfig. That type is also used by MCPExternalAuthConfig Redis storage, and a type-level rule could start rejecting existing auth-server resources.

  • spec.config.sessionStorage.tls on VirtualMCPServer: the converter overwrites spec.config.sessionStorage from spec.sessionStorage, so a value there was silently discarded. A CEL rule now rejects it. The field was added in Add TLS to Redis session storage #6731, which is not in a release yet, so no stored objects are affected.

  • Rate limiter: it always shares the session storage settings, so only the session stores emit the warning.

  • Unrelated CI fix included on request: pkg/authserver tests failed under -race (also on main). fosite's GetAudienceStrategy and GetJWKSFetcherStrategy assign their defaults to the shared Config on first use, and the embedded auth server left both unset, so concurrent token requests raced on that write. The fix sets the audience strategy to fosite's default, like the scope strategy and secrets hasher already are. The JWKS fetcher is never used, because DCR rejects jwks_uri and storage drops it, so instead of fosite's default (a ristretto cache whose goroutines are never closed) it is a fetcher that rejects the client with invalid_client. This fails closed if a client with a jwks_uri ever reached client authentication. Behavior for supported clients is unchanged. Reproduced locally with go test -race -count=10 -run TestIntegration ./pkg/authserver/; with the fix, 20 and 40 repetitions were clean.

  • Not addressed here:

    • The proxy runner decodes its RunConfig without rejecting unknown fields, so a runner image older than Add TLS to Redis session storage #6731 ignores tls. Making decoding strict would break operator/runner version skew for every field, not just this one.
    • Runner.Run does not close the Redis session store when a later startup step fails. This predates the PR (Add TLS to Redis session storage #6731) and belongs in a separate fix.
  • Follow-ups:

    • TLS for the operator-wide defaultRedis fallback (TOOLHIVE_DEFAULT_REDIS_*). It needs Helm values and a CA mount.
    • mTLS client certificates. redisconn.TLSConfig supports them, but neither CRD type exposes them yet.
    • CA rotation without a restart.
  • Size: about 300 non-generated production lines across 19 files, which is over the 10-file guideline. It is split into commits:

    1. move the test fixture;
    2. add the startup warning;
    3. add the operator integration (10 production files);
    4. add the tests;
    5. make the e2e certificates readable;
    6. address review findings (drift check with podTemplateSpec, two CEL rules, warning moved into the store constructors, shared volume helper);
    7. fix the fosite strategy data race in the embedded auth server;
    8. replace the eager JWKS fetcher with a fail-closed one;
    9. run the shared sessionStorage.tls admission cases on every kind and served version (review suggestion).

    Reviewing commit by commit should be easier.

🤖 Generated with Claude Code

peppescg and others added 4 commits October 9, 2026 11:56
Shared test fixtures live under test/testkit. Move the redistls
helper added for Redis session storage TLS there and update its
importers; the fixture itself is unchanged.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Giuseppe Scuglia <peppescg@gmail.com>
Session storage now supports TLS, but omitting it keeps the previous
plaintext connection so existing deployments are unaffected. Make
that choice visible: when a Redis password is configured, the proxy
runner and vMCP session stores log one startup WARN if TLS is not
set or certificate verification is disabled. The connection is still
allowed and the password is never logged.

The message also notes that the operator-wide default Redis
(TOOLHIVE_DEFAULT_REDIS_ADDR) cannot enable TLS yet. The rate
limiter always shares these settings, so it does not warn again.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Giuseppe Scuglia <peppescg@gmail.com>
#6731 lets the proxy runner and vMCP connect to Redis session
storage over TLS, but only through hand-written RunConfig or vMCP
YAML. Operator-managed workloads had no way to request it.

Add spec.sessionStorage.tls to MCPServer, MCPRemoteProxy and
VirtualMCPServer. It reuses the existing RedisTLSConfig type from
the embedded auth server's Redis storage:

- An empty object verifies the server certificate against the
  system roots.
- caCertSecretRef mounts a private CA from a Secret at
  /etc/toolhive/session-redis-tls/ca.crt, read-only and via
  subPath, as for the auth server's Redis CA.
- insecureSkipVerify disables verification, for testing only.

The operator writes these settings into the RunConfig
(scaling_config.session_redis.tls) and the vMCP config
(sessionStorage.tls). On VirtualMCPServer the operator value replaces
any spec.config.sessionStorage.tls, like the other session storage
fields. Switching the referenced CA Secret rolls the Deployment.

CEL rules on SessionStorageConfig reject tls with a non-redis
provider, and insecureSkipVerify combined with caCertSecretRef. They
sit on SessionStorageConfig rather than on the shared type so
existing auth server resources are unaffected. The shared
insecureSkipVerify description now recommends caCertSecretRef
instead.

Refs #6730

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Giuseppe Scuglia <peppescg@gmail.com>
Add coverage for the Redis session storage TLS paths that were not
exercised end to end:

- Runner.Run test showing the session store client takes its TLS
  settings from the RunConfig. Without TLS it does not handshake. With
  TLS, the server certificate and the hostname from the configured
  address are verified. A trusted CA connects.
- The rate limiter and the vMCP limiter factory now also cover the
  omitted-TLS case against a TLS-only server, and match the
  certificate-verification error precisely.
- An e2e spec that runs standalone vMCP against a TLS-only,
  password-protected Redis container. It opens a session and checks
  over TLS that the session was stored.

The redistls fixture gains on-disk certificates with an IP-only server
certificate. A real Redis server uses them, and they make a hostname
mismatch observable.

Refs #6730

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Giuseppe Scuglia <peppescg@gmail.com>
@github-actions github-actions Bot added the size/XL Extra large PR: 1000+ lines changed label Oct 9, 2026
@codecov

codecov Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.87179% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.46%. Comparing base (183978d) to head (ec0c140).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
test/testkit/redistls/redistls.go 89.06% 7 Missing ⚠️
...v-operator/pkg/controllerutil/session_redis_tls.go 97.72% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6767      +/-   ##
==========================================
+ Coverage   79.40%   79.46%   +0.05%     
==========================================
  Files         808      810       +2     
  Lines       82022    82147     +125     
==========================================
+ Hits        65133    65281     +148     
+ Misses      16884    16861      -23     
  Partials        5        5              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Sanskarzz
Sanskarzz self-requested a review October 9, 2026 11:05
The TLS-only Redis e2e spec bind-mounts a generated CA, certificate and
key into the Redis container. Only the key was made world-readable, so
on Linux hosts, where the container's redis user does not own the
mounted files, Redis could not read the 0600 certificate and CA, failed
to configure TLS and exited before answering PING. Docker Desktop on
macOS does not enforce file ownership on bind mounts, which hid this
locally.

Make all three files readable inside the private test directory.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Giuseppe Scuglia <peppescg@gmail.com>
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Oct 9, 2026
The session storage CA drift check compared the live Deployment with
the raw desired volume. On an MCPRemoteProxy whose podTemplateSpec
patches that volume, every reconcile saw drift and issued a no-op
update. The check now compares live volumes with the desired ones, and
the remote proxy uses the rebuilt, patched Deployment when a
podTemplateSpec is set.

Reject configurations that would silently misbehave:

- spec.config.sessionStorage.tls on a VirtualMCPServer was accepted
  but discarded by the converter, leaving the connection plaintext.
- A caCertSecretRef with an empty name or key passed admission but
  produced a volume the Deployment API rejects on every reconcile.

The insecure-transport warning moves into the Redis session store
constructors so every caller gets it, and it now also fires without a
password because session data still crosses the network in plaintext.
Its text no longer names configuration keys that only apply to some
callers.

Also share one Secret-file volume helper between the session store and
the embedded auth server's Redis CAs, build the redistls fixture on a
single certificate generator, and drop a runner test case that only
exercised crypto/tls and go-redis.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Giuseppe Scuglia <peppescg@gmail.com>
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Oct 9, 2026
@peppescg peppescg self-assigned this Oct 9, 2026
fosite's GetAudienceStrategy and GetJWKSFetcherStrategy assign their
default to the shared Config on first use. The embedded auth server
left both unset, so concurrent token requests raced on that write. The
race detector flagged it in the pkg/authserver integration tests, which
fail on main as well.

Set both to fosite's defaults when building the Config, as is already
done for the scope strategy and secrets hasher. Behavior is unchanged;
the getters simply no longer write.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Giuseppe Scuglia <peppescg@gmail.com>
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Oct 9, 2026

@Sanskarzz Sanskarzz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for completing the operator integration for #6730 after #6731. I reviewed head ca46e64685d85e43ef143b0b078cd856b7624583, tracing the API fields through configuration conversion, Secret projection, and deployment updates for MCPServer, MCPRemoteProxy, and VirtualMCPServer.

I found no blocking issues in the changes reviewed:

  • The shared SessionStorageConfig.TLS field is wired into all three runtime configuration paths. An omitted field preserves plaintext behavior, while tls: {} enables certificate verification using system roots. A custom CA is passed as a mounted file, and TLS configuration or handshake failures do not fall back to plaintext.
  • The CA Secret projection and deployment update handling account for changes to the Secret name or key, including the remote proxy PodTemplate path. The documented restart requirement for changes to the contents of the same Secret matches the subPath mount and startup-time CA loading.
  • The session storage CEL rules reject TLS with the memory provider, incomplete CA references, and the combination of a custom CA with insecureSkipVerify. Rejecting VirtualMCPServer's nested spec.config.sessionStorage.tls also prevents a setting from being accepted and then discarded by the converter.
  • The startup warnings preserve existing connection behavior without logging the Redis password. The shared certificate fixture, runtime wiring tests, controller tests, and real Redis TLS e2e test provide useful coverage. The e2e test checks that a legacy session is actually persisted in Redis.

Optional, non-blocking test suggestion: the new TLS admission cases exercise VirtualMCPServer. Could we extend the table to MCPServer and MCPRemoteProxy, covering both served API versions? The generated schemas currently contain the same rules across all three resources, so I do not see a current correctness issue; this would protect that consistency against future schema changes.

Operator-wide defaultRedis TLS remains a separate follow-up, as agreed in #6731. The eager initialization of the two Fosite strategies also preserves their existing defaults while avoiding lazy writes to shared configuration.

Verification: source and generated-schema inspection, a clean git diff --check, and review of the reported CI results. I did not rerun the test suites locally. With the current passing checks, I consider this ready to merge; the additional admission coverage can be a follow-up.

peppescg and others added 2 commits October 10, 2026 11:56
The previous commit set fosite's JWKS fetcher to its default to avoid a
lazy write to the shared Config. That fetcher is never used: DCR rejects
jwks_uri for private_key_jwt and storage drops it, so client keys are
always inline. Creating it eagerly only started a ristretto cache with
goroutines that are never closed, for every auth server instance.

Use a fetcher that rejects the client instead. It still keeps fosite's
getter from writing, starts nothing, and fails closed rather than
fetching an arbitrary URL with an unhardened client if a client with a
jwks_uri ever reached client authentication.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Giuseppe Scuglia <peppescg@gmail.com>
The sessionStorage.tls CEL rules live on the shared SessionStorageConfig
type, but admission tests only covered VirtualMCPServer at v1beta1. A
schema change could drop a rule from one kind or one served version
without any test noticing.

Run one shared table of TLS admission cases against MCPServer,
MCPRemoteProxy and VirtualMCPServer at both v1beta1 and v1alpha1.

Also document that workloads using the operator-wide defaultRedis
fallback always log the plaintext warning, and that vMCP's
no-authentication warning is separate from it.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Giuseppe Scuglia <peppescg@gmail.com>
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Oct 10, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XL Extra large PR: 1000+ lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TLS configuration for VirtualMCPServer Redis session storage

2 participants