Support no-auth Redis for the embedded auth server and vMCP sessions - #6551
Support no-auth Redis for the embedded auth server and vMCP sessions#6551tgrunnagle wants to merge 5 commits into
Conversation
Redis-backed storage could only connect to Redis/Valkey with a password: a nil ACLUserConfig was a hard error and the password was mandatory. This blocked legitimate no-auth deployments (local/dev, network-isolated, managed tiers provisioned without ACL users). Make no-auth a selectable mode while keeping a misconfigured credential a loud failure rather than a silent downgrade. The split is on presence of the config block: - convertRedisACLConfig: a nil ACLUserConfig now resolves to empty credentials with no error (no-auth). A populated block whose password resolves to empty is still rejected as a misconfiguration. - Emit exactly one startup WARN naming the store on a no-auth resolution, for both the embedded auth server storage and the vMCP Redis session storage (previously an unconditional INFO). - Update RedisRunConfig doc comments to state a nil ACLUserConfig is a valid no-auth configuration. Implements #6550.
- Move the no-auth WARN to after the fallible timeout/TLS conversion steps so it only fires for a config that will actually connect. - Reword the WARN rationale comment to describe a reachable caller path rather than an operator scenario the CRD currently forbids. - Assert the distinctive WARN message substring instead of the generic level=WARN token, so an unrelated WARN cannot skew the count.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6551 +/- ##
=======================================
Coverage 78.70% 78.71%
=======================================
Files 777 777
Lines 76816 76843 +27
=======================================
+ Hits 60459 60486 +27
Misses 16352 16352
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
tgrunnagle
left a comment
There was a problem hiding this comment.
Multi-Agent Consensus Review
Agents consulted: security, test-coverage, general-code-quality
Consensus Summary
| # | Finding | Consensus | Severity | Action |
|---|---|---|---|---|
| 1 | AuthType field never validated against ACLUserConfig presence |
9/10 | MEDIUM | Fix |
| 2 | "Connects to a no-auth server" half of test criterion unverified | 8/10 | MEDIUM | Fix |
| 3 | redisStoreName's Sentinel branch untested |
8/10 | MEDIUM | Fix |
| 4 | Populated ACL config with unset PasswordEnvVar — ambiguous, untested error path |
8/10 | LOW | Fix |
| 5 | vMCP missing "authenticated emits no WARN" counterpart test | 7/10 | LOW | Fix |
| 6 | No-auth WARN/INFO logic duplicated with drifting log schema | 7/10 | MEDIUM | Fix |
Overall
This PR makes no-auth a selectable mode for two Redis consumers — the embedded auth server's storage and vMCP's Redis session storage — while keeping a misconfigured credential a hard error. The core design correctly splits on presence of the ACL config block rather than the resolved-password value, matches issue #6550's acceptance criteria, and leaves the existing authenticated path untouched. The operator CRD path independently still requires ACLUserConfig.PasswordSecretRef, so the stated scope cut holds.
The findings below are all polish/hardening, not design objections. The one worth prioritizing is #1: RedisRunConfig.AuthType is now documented as a mode selector but the conversion code never checks it, so a caller that declares AuthType: "aclUser" with a nil ACLUserConfig silently gets no-auth instead of the pre-PR error — currently safe only because the operator (the sole caller) independently guards ACLUserConfig presence before calling in. The rest are test-depth gaps: no test builds a real client and connects it to an unauthenticated Redis, the new Sentinel-naming branch is untested, and the WARN/INFO logging between the two consumers already uses different structured-log attribute keys for the same concept.
Generated with Claude Code
Addresses #6551 review comments: - MEDIUM config.go (3960523215): reject auth_type "aclUser" with a nil ACLUserConfig in convertRedisRunConfig, restoring fail-loud for declared authenticated intent that carries no credential config - LOW embeddedauthserver.go (3960523247): return the actionable "omit acl_user_config" guidance for a populated ACL block with no password_env_var, instead of resolveEnvVar's generic message - MEDIUM embeddedauthserver_test.go (3960523224): add a miniredis no-auth connect test that builds storage via createStorage and proves a real round-trip against an unauthenticated Redis - MEDIUM embeddedauthserver.go (3960523239): add a Sentinel-mode subtest exercising redisStoreName's "sentinel:<master>" branch
Addresses #6551 review comments: - MEDIUM server.go (3960523264): emit a "store" attribute on the vMCP session-storage WARN/INFO so a log-based alert can match one key across both Redis consumers (the embedded auth server already uses "store") - LOW serve_session_test.go (3960523253): add the "authenticated resolution emits no WARN" counterpart test, mirroring the auth-server package's symmetry
The AuthType / ACLUserConfig doc-comment updates in
pkg/authserver/storage/config.go feed into the generated OpenAPI docs
via the storage.ACLUserRunConfig schema. Run task docs so the committed
docs/server/swagger.{json,yaml} and docs.go match, fixing the Verify
Swagger Documentation CI check.
jhrozek
left a comment
There was a problem hiding this comment.
Went through the no-auth Redis changes for both the embedded auth server and vMCP session storage. The core design is solid — splitting no-auth vs. misconfiguration on whether the ACL config block is present, rather than on the resolved password value, is the right call, and it's well tested on the auth-server side. Left a few non-blocking notes inline, nothing here should hold up the merge.
| // AuthType selects the Redis authentication mode. "aclUser" is the only | ||
| // authenticated mode. Leave it empty, with a nil ACLUserConfig, for a | ||
| // no-auth connection to a Redis/Valkey instance that has no authentication | ||
| // configured. The conversion code does not branch on this field; presence |
There was a problem hiding this comment.
This says the conversion code doesn't branch on AuthType, but it does — convertRedisRunConfig checks AuthType == AuthTypeACLUser to reject a nil ACLUserConfig (embeddedauthserver.go:891-894). Worth fixing so it doesn't mislead the next person who reads it.
| if keyPrefix == "" { | ||
| keyPrefix = "thv:vmcp:session:" | ||
| } | ||
| password := os.Getenv(vmcpconfig.RedisPasswordEnvVar) |
There was a problem hiding this comment.
Unlike the auth server side, there's no way here to tell "password env var was never set" apart from "it was set but resolves to an empty string." The CRD's spec.sessionStorage.passwordRef does express intent, but the operator only uses it to decide whether to inject the env var — that intent doesn't make it down to this function, which just does os.Getenv. An emptied or mis-keyed session Redis secret would silently downgrade to no-auth with just a WARN, which is exactly the case convertRedisACLConfig treats as a hard error on the auth-server side. Not blocking, but might be worth a follow-up — os.LookupEnv would at least let you tell the two cases apart.
|
|
||
| var buf logSyncBuffer | ||
| previous := slog.Default() | ||
| slog.SetDefault(slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelWarn}))) |
There was a problem hiding this comment.
This test is documented as the counterpart to the no-auth WARN test, proving the authenticated path logs INFO instead. But the handler here is set to Level: slog.LevelWarn, which filters out INFO records before they ever reach buf. So this only proves the WARN string is absent — it'd pass identically even if the INFO log line were deleted outright. Might want to drop the level to LevelInfo and assert on the INFO message directly if you want this to actually cover what it claims to.
Summary
The embedded auth server's Redis-backed storage could not connect to a Redis/Valkey instance running without authentication.
convertRedisACLConfigtreated a nilACLUserConfigas a hard error ("acl user config is required") and required the password even when a config was present, so the only supported mode was password auth. That blocked legitimate no-auth deployments — local/dev clusters, network-isolated single-tenant setups, and managed tiers provisioned without ACL users.This PR makes no-auth a selectable mode for the toolhive-owned Redis consumers while keeping a misconfigured credential a loud failure rather than a silent downgrade.
ACLUserConfignow resolves to a no-auth connection.convertRedisACLConfigreturns empty credentials with no error, andconvertRedisRunConfig/createStorageno longer require an ACL block for theredisstorage type.ACLUserConfigis present but itsPasswordEnvVaris unset or resolves to empty, the conversion still fails with actionable guidance — distinguishing an unset credential (no auth intended) from a misconfigured secret (mis-keyed, wrong-namespace, or unsynced). The split is on presence of the config block, not on whether the resolved password happens to be empty.AuthTypeis"aclUser"butACLUserConfigis nil,convertRedisRunConfigfails loudly rather than silently downgrading — an emptyAuthTypewith a nilACLUserConfigremains a valid no-auth configuration.WARNnaming the store on any no-auth resolution, so an unintended downgrade is visible in logs. This covers both the embedded auth server storage (convertRedisRunConfig) and the vMCP Redis session storage (buildSessionDataStorage), which previously logged unconditionally atINFO; it now logsWARNfor no-auth and keepsINFOfor authenticated connections. Both consumers emit a shared"store"log attribute so a single log-based alert can match one key across both.RedisRunConfig.AuthType,ACLUserConfig,convertRedisRunConfig, andconvertRedisACLConfigto state that a nilACLUserConfigis a valid no-auth configuration.Closes #6550
Type of change
Test plan
task test)task lint-fix)All four of the issue's "Suggested tests" acceptance criteria are covered by unit tests:
ACLUserConfigyields empty username/password with no error, and a client built from the resulting config connects to a no-auth server — the connection half is verified end-to-end against a no-authminirediswith a real write/read round-trip (TestCreateStorage_NoAuthRedisConnects), in addition to the struct-level assertion (nil ACL user config resolves to no-auth).ACLUserConfigwhose password is unset or resolves to empty still returns an actionable error (populated ACL config with unset password env var returns actionable error,populated ACL config with empty-resolved password returns error); a declaredauth_type: "aclUser"with a nilACLUserConfigalso errors (aclUser auth type with nil ACL config returns error).WARNnaming the store, for both the embedded auth server storage (TestConvertRedisRunConfig_NoAuthWarns, including a Sentinel-mode subtest that asserts thesentinel:<master>store name) and the vMCP session storage (TestBuildSessionDataStorageRedis_NoAuthWarns); the authenticated path emits noWARNfor either consumer (authenticated resolution emits no WARN,TestBuildSessionDataStorageRedis_AuthenticatedNoWarn).task lint-fixis clean, and both changed packages' test suites pass under-race.API Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.Changes
pkg/authserver/runner/embeddedauthserver.goACLUserConfigresolves to no-auth;auth_type: "aclUser"with a nilACLUserConfigerrors; populated-but-empty/unset password still errors with actionable guidance; newredisStoreNamehelper; one no-auth startupWARNemitted after the fallible timeout/TLS steps; doc comments.pkg/authserver/storage/config.goAuthTypeandACLUserConfigdescribing the no-auth configuration.pkg/vmcp/server/server.gobuildSessionDataStoragelogsWARNnaming the store on empty password (no-auth), keepsINFOwhen authenticated; emits a"store"attribute matching the auth-server WARN so one alert key spans both consumers.pkg/authserver/runner/embeddedauthserver_test.goAuthTypemismatch, populated-but-empty/unset password errors, one-WARNassertions (incl. Sentinel naming), and aminiredisno-auth connect round-trip.pkg/vmcp/server/serve_session_test.goWARNand the authenticated no-WARNcounterpart.Does this introduce a user-facing change?
Yes. Operators can now run the embedded auth server and vMCP Redis session storage against a Redis/Valkey instance with no authentication by omitting the ACL user configuration. A no-auth connection is logged at
WARNat startup, naming the store. A populated ACL config whose password is unset or resolves to empty continues to fail fast.Special notes for reviewers
The operator CRD path (
cmd/thv-operator/pkg/controllerutil/authserver.goand theRedisStorageConfig.ACLUserConfig+kubebuilder:validation:Requiredmarker) still requires an ACL config, so no-auth is not yet selectable end-to-end via the operator CRD. Relaxing that guard (likely gated onAuthType) is intentionally out of scope for this PR per the issue's "Out of scope" notes and is a tracked follow-up. This PR enables no-auth for the runner/proxyrunner path and the vMCP session storage.Generated with Claude Code