Skip to content

fix: honor admin.tls.enabled in dual-port mode#92

Merged
christianromeni merged 1 commit into
voidmind-io:mainfrom
martinsotirov:feature/admin-tls-actually-applied
May 20, 2026
Merged

fix: honor admin.tls.enabled in dual-port mode#92
christianromeni merged 1 commit into
voidmind-io:mainfrom
martinsotirov:feature/admin-tls-actually-applied

Conversation

@martinsotirov
Copy link
Copy Markdown
Contributor

Problem

server.admin.tls.{enabled,cert,key} exists in the config schema and is enforced by validate.go, but enabling it has no effect — the admin server is always started in plain HTTP. The Helm chart, the deployment docs, and the struct's own godoc all describe it as a working feature.

Repro with voidllm.yaml:

server:
  proxy:
    port: 9000
  admin:
    port: 9443
    tls:
      enabled: true
      cert: /path/to/cert.pem
      key:  /path/to/key.pem

After startup:

$ curl -k https://localhost:9443/healthz
curl: (35) error:0A00010B:SSL routines::wrong version number
$ curl http://localhost:9443/healthz
{"status":"ok"}        # ← plain HTTP returned 200 despite TLS being configured

docs/deployment/reverse-proxy.md:63 says "VoidLLM supports TLS on the admin port (server.admin.tls) but not on the proxy port" — but the wiring to make that true never landed.

Cause

TLSConfig has been in the schema since 1dea0e1 (v0.0.1) with validation that rejects empty cert/key when enabled (internal/config/validate.go:88-95). However, the only other readers in the repo before this PR are inside validate() itself — cfg.Server.Admin.TLS is never consumed by any listener. The admin goroutine in Application.startListening (internal/app/routes.go:154) unconditionally calls plain Listen(adminAddr), so tls.enabled: true is silently a no-op.

Fix

In dual-port mode, branch on cfg.Server.Admin.TLS.Enabled and pass cert/key to Fiber v3 via ListenConfig:

if adminTLS {
    err := a.adminApp.Listen(adminAddr, fiber.ListenConfig{
        CertFile:    a.cfg.Server.Admin.TLS.Cert,
        CertKeyFile: a.cfg.Server.Admin.TLS.Key,
    })
    // …
}

Cert/key paths appear only in the error log on TLS startup failure; the boot-time INFO log only adds admin_tls=<bool> so ops can confirm state.

Single-port mode (where admin shares the proxy port) does not apply TLS — docs/deployment/reverse-proxy.md already recommends external termination for the proxy. Configuring tls.enabled: true while in single-port mode now emits one WARN at startup ("admin TLS configured but ignored in single-port mode") instead of silently dropping it.

Backward-compatible: tls.enabled: false (the default) is runtime-identical. The only log delta is the new admin_tls=false attribute on the existing "starting servers" line.

Test plan

Added internal/app/admin_tls_test.go with three tests using an in-test self-signed RSA-2048 cert written to t.TempDir():

  • TestAdminTLS_Enabled — admin port accepts TLS, returns 200 on HTTPS /healthz; plain HTTP against the same port fails.
  • TestAdminTLS_Disabled — admin port serves plain HTTP; TLS handshake against the same port fails (regression guard for the default config).
  • TestAdminTLS_SinglePortWarn — single-port mode with tls.enabled: true emits the warn; disabled does not.

Validated locally:

  • go test ./... — all packages green
  • go vet ./... — clean
  • gofmt -l internal/app — clean
  • Manual: curl -k https://localhost:9443/healthz against a running server with TLS enabled returns 200; plain http://… fails handshake

@snyk-io
Copy link
Copy Markdown

snyk-io Bot commented May 14, 2026

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 20, 2026

Codecov Report

❌ Patch coverage is 66.66667% with 8 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/app/routes.go 66.66% 7 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@christianromeni christianromeni merged commit a7ead42 into voidmind-io:main May 20, 2026
7 checks passed
@christianromeni christianromeni mentioned this pull request May 20, 2026
2 tasks
@christianromeni
Copy link
Copy Markdown
Contributor

Sorry for the slow response on this - sat here almost a week. Merged today and released as v0.0.19. Repro, root cause, and tests were all solid. Thanks for the careful write-up.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants