From c508a24e695d95a8a45dee17f334f138be99a26b Mon Sep 17 00:00:00 2001 From: PaulDotterer Date: Thu, 23 Apr 2026 07:56:17 +0200 Subject: [PATCH 01/11] =?UTF-8?q?fix:=20rc10=20=E2=80=94=20close=20CONTROL?= =?UTF-8?q?=5FPASSWORD=5FAUTH=5FENABLED=20bypass=20+=20fold=20routable-IP?= =?UTF-8?q?=20/=20fail-open=20WIP?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security fix: `CONTROL_PASSWORD_AUTH_ENABLED=false` previously only gated the SSO handler's ListAuthMethods response, which is a UI hint for the login form. The Login RPC itself never checked the flag, so an attacker POSTing directly to /pm.v1.ControlService/Login with valid email+password still authenticated against any account whose HasPassword column was true. This is the bypass a penetration test would find immediately; SSO-only deployments were not SSO-only in the enforced sense. Plumb passwordAuthEnabled into AuthHandler and enforce it BEFORE the DB lookup. Burn the dummy bcrypt cycle on the disabled path too so the "is password auth disabled?" question doesn't leak via timing either. Regression test: TestLogin_GlobalPasswordAuthDisabled covers the exact bypass path (valid credentials, global switch off, expect CodeUnauthenticated + password_login_disabled). Also folds in the pre-rc10 WIP from the staging debugging session: - routableIP() helper replacing os.Hostname() for Traefik backend auto-derivation (container IDs are not in Docker's embedded DNS). - Fail-open on a malformed GATEWAY_PUBLIC_TERMINAL_URL_TEMPLATE so a broken optional template disables terminal registration with a warning instead of crash-looping the gateway. - GATEWAY_TRAEFIK_* defaults collapsed into the binary so compose.yml only needs GATEWAY_DOMAIN / GATEWAY_TTY_DOMAIN in the common case. Remaining rc10 scope continues in subsequent commits on this branch. Part of manchtools/power-manage-server#74. --- cmd/gateway/main.go | 218 ++++++++++++++++-------------- internal/api/auth_handler.go | 31 +++-- internal/api/auth_handler_test.go | 54 ++++++-- internal/api/service.go | 2 +- internal/config/config.go | 11 +- 5 files changed, 185 insertions(+), 131 deletions(-) diff --git a/cmd/gateway/main.go b/cmd/gateway/main.go index 7aa9f6d8..77f7ad32 100644 --- a/cmd/gateway/main.go +++ b/cmd/gateway/main.go @@ -137,27 +137,61 @@ func main() { shutdownCtx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) defer stop() - // Wire the multi-gateway registry. Reuses the same Valkey - // instance the Asynq queue uses, no extra connection pool. The - // registry is enabled only when the operator has set the - // public terminal URL template — without it the gateway can't - // know its own public URL, so we just leave the registry off - // (single-gateway / no-terminal mode). + // Wire the multi-gateway registry lazily. The registry is shared by + // three independent features: + // - terminal URL lookup for browser sessions, + // - internal gateway fan-out for control -> gateway RPCs, + // - Traefik Redis-KV self-registration. + // + // Keep those code paths independent. A malformed optional terminal + // URL must not prevent agent mTLS routing or internal gateway + // discovery from coming up. var ( gatewayReg *registry.Registry + registryRDB *redis.Client assignedHost string ) + ensureGatewayRegistry := func() *registry.Registry { + if gatewayReg != nil { + return gatewayReg + } + registryRDB = redis.NewClient(&redis.Options{ + Addr: cfg.ValkeyAddr, + Password: cfg.ValkeyPassword, + DB: cfg.ValkeyDB, + Protocol: 2, + }) + gatewayReg = registry.New(registry.NewValkeyBackend(registryRDB), logger.With("component", "registry")) + return gatewayReg + } + defer func() { + if registryRDB != nil { + if err := registryRDB.Close(); err != nil { + logger.Warn("failed to close gateway registry Valkey client", "error", err) + } + } + }() // Compute the agent redirect hostname independently of the terminal // URL. This supports multi-gateway agent routing without requiring // the terminal feature to be enabled. + // + // A malformed template (e.g. one that references an unset env var + // like ${TTY_DOMAIN} and expands to "https:///…" with an empty host) + // is treated as "bootstrap redirects disabled" with a loud warning + // rather than a fatal exit. The gateway still serves agent mTLS — + // which is what matters for an enrolled device. Crashing on a + // misconfigured optional feature used to kill the gateway on every + // restart, and because the Traefik Redis KV entry expires 45 s after + // each exit, the pm-mtls TCP router fell through to the HTTP router + // on the shared :443 and served the Let's Encrypt cert — giving + // agents the misleading x509 "unknown authority" error. if cfg.PublicAgentURLTemplate != "" { agentURL := strings.ReplaceAll(cfg.PublicAgentURLTemplate, "{id}", gatewayID) assignedHost = hostFromURL(agentURL) if assignedHost == "" { - logger.Error("could not extract host from GATEWAY_PUBLIC_AGENT_URL_TEMPLATE", + logger.Warn("GATEWAY_PUBLIC_AGENT_URL_TEMPLATE resolved to a URL with no host — bootstrap redirects disabled; check for unset env vars in the template", "template", cfg.PublicAgentURLTemplate, "resolved", agentURL) - os.Exit(1) } } @@ -170,95 +204,39 @@ func main() { // If no agent URL template was set, fall back to deriving the // agent redirect hostname from the terminal URL (legacy // single-hostname mode). - if assignedHost == "" { - assignedHost = hostFromURL(terminalURL) + terminalHost := hostFromURL(terminalURL) + if terminalHost == "" { + // Malformed template — skip the registry work instead of + // os.Exit(1). See the long comment above the agent-template + // check for why this has to be non-fatal. + logger.Warn("GATEWAY_PUBLIC_TERMINAL_URL_TEMPLATE resolved to a URL with no host — terminal session registration disabled on this gateway; check for unset env vars in the template", + "template", cfg.PublicTerminalURLTemplate, "resolved", terminalURL) + } else { if assignedHost == "" { - logger.Error("could not extract host from GATEWAY_PUBLIC_TERMINAL_URL_TEMPLATE", - "template", cfg.PublicTerminalURLTemplate, "resolved", terminalURL) - os.Exit(1) + assignedHost = terminalHost } - } - rdb := redis.NewClient(&redis.Options{ - Addr: cfg.ValkeyAddr, - Password: cfg.ValkeyPassword, - DB: cfg.ValkeyDB, - Protocol: 2, - }) - defer rdb.Close() - - gatewayReg = registry.New(registry.NewValkeyBackend(rdb), logger.With("component", "registry")) - stop, err := gatewayReg.RegisterGateway( - context.Background(), - gatewayID, - terminalURL, - registry.DefaultGatewayTTL, - registry.DefaultGatewayRefreshInterval, - ) - if err != nil { - logger.Error("failed to register gateway in registry", "error", err) - os.Exit(1) - } - defer stop() - - // Also publish the internal mTLS URL so the control server - // can discover this gateway for admin fan-out (List/Terminate - // terminal sessions). Uses the same TTL as the terminal URL. - // - // rc6 note: auto-derive when GATEWAY_INTERNAL_URL is empty. - // The GatewayService RPC is mounted on the same mTLS listener - // that accepts agents, so the internal URL is just - // `https://`. Auto-derivation keeps the - // common case zero-config — without it, operators who don't - // set the env silently lose the admin list/terminate feature - // (Terminal Sessions page shows empty). - internalURL := cfg.InternalURL - if internalURL == "" { - ip, err := routableIP() + gatewayReg = ensureGatewayRegistry() + stop, err := gatewayReg.RegisterGateway( + context.Background(), + gatewayID, + terminalURL, + registry.DefaultGatewayTTL, + registry.DefaultGatewayRefreshInterval, + ) if err != nil { - logger.Error("cannot auto-derive GATEWAY_INTERNAL_URL", "error", err) + logger.Error("failed to register gateway in registry", "error", err) os.Exit(1) - } - internalURL = "https://" + ip + portOfListenAddr(cfg.ListenAddr) - logger.Info("auto-derived GATEWAY_INTERNAL_URL", "internal_url", internalURL) - } - if internalURL != "" { - if err := gatewayReg.RegisterGatewayInternal( - context.Background(), gatewayID, internalURL, registry.DefaultGatewayTTL, - ); err != nil { - logger.Warn("failed to register gateway internal URL", "error", err) - } - // Refresh the internal URL on the same cadence as the - // terminal URL so it does not expire while the gateway is - // running. Derive from shutdownCtx so the goroutine exits - // as soon as a signal arrives. - internalRefreshCtx, stopInternalRefresh := context.WithCancel(shutdownCtx) - defer stopInternalRefresh() - go func() { - ticker := time.NewTicker(registry.DefaultGatewayRefreshInterval) - defer ticker.Stop() - for { - select { - case <-ticker.C: - if err := gatewayReg.RegisterGatewayInternal( - context.Background(), gatewayID, internalURL, registry.DefaultGatewayTTL, - ); err != nil { - logger.Warn("failed to refresh gateway internal URL", "error", err) - } - case <-internalRefreshCtx.Done(): - return - } } - }() - } + defer stop() - logger.Info("multi-gateway routing enabled", - "gateway_id", gatewayID, - "terminal_url", terminalURL, - "agent_redirect_host", assignedHost, - "internal_url", internalURL, - ) - } + logger.Info("multi-gateway routing enabled", + "gateway_id", gatewayID, + "terminal_url", terminalURL, + "agent_redirect_host", assignedHost, + ) + } + } // Traefik Redis-KV self-registration. Opt-in; when enabled, every // replica writes its own routing entry into the same Valkey @@ -269,18 +247,10 @@ func main() { // /gw/ path prefix on the shared tty host for TTY routing. if cfg.TraefikSelfRegister { if gatewayReg == nil { - // Need a Valkey-backed registry. When PublicTerminalURLTemplate - // was empty we skipped the Valkey connection earlier; set one - // up here so Traefik self-reg still works in deployments that - // only want the routing layer (no terminal feature). - rdb := redis.NewClient(&redis.Options{ - Addr: cfg.ValkeyAddr, - Password: cfg.ValkeyPassword, - DB: cfg.ValkeyDB, - Protocol: 2, - }) - defer rdb.Close() - gatewayReg = registry.New(registry.NewValkeyBackend(rdb), logger.With("component", "registry")) + // Need a Valkey-backed registry even when terminal URLs are + // disabled; Traefik self-registration is the agent mTLS + // routing layer. + gatewayReg = ensureGatewayRegistry() } // Auto-derive per-replica backend addresses when not set. We use @@ -366,6 +336,50 @@ func main() { "tty_cert_resolver", cfg.TraefikTTYCertResolver, ) } + + // Publish the internal mTLS URL so the control server can discover + // this gateway for admin fan-out (List/Terminate terminal sessions). + // This is intentionally independent of terminal URL registration: a + // bad optional public terminal URL should not disable the internal + // control-plane route when the registry is otherwise available. + if gatewayReg != nil { + internalURL := cfg.InternalURL + if internalURL == "" { + ip, err := routableIP() + if err != nil { + logger.Error("cannot auto-derive GATEWAY_INTERNAL_URL", "error", err) + os.Exit(1) + } + internalURL = "https://" + ip + portOfListenAddr(cfg.ListenAddr) + logger.Info("auto-derived GATEWAY_INTERNAL_URL", "internal_url", internalURL) + } + if internalURL != "" { + if err := gatewayReg.RegisterGatewayInternal( + context.Background(), gatewayID, internalURL, registry.DefaultGatewayTTL, + ); err != nil { + logger.Warn("failed to register gateway internal URL", "error", err) + } + internalRefreshCtx, stopInternalRefresh := context.WithCancel(shutdownCtx) + defer stopInternalRefresh() + go func() { + ticker := time.NewTicker(registry.DefaultGatewayRefreshInterval) + defer ticker.Stop() + for { + select { + case <-ticker.C: + if err := gatewayReg.RegisterGatewayInternal( + context.Background(), gatewayID, internalURL, registry.DefaultGatewayTTL, + ); err != nil { + logger.Warn("failed to refresh gateway internal URL", "error", err) + } + case <-internalRefreshCtx.Done(): + return + } + } + }() + } + } + // Fail fast if BootstrapHost is set but we have no assignedHost // (because PublicTerminalURLTemplate was empty). Without this // guard, BootstrapRedirectMiddleware would panic on an empty diff --git a/internal/api/auth_handler.go b/internal/api/auth_handler.go index 509c831f..8673d803 100644 --- a/internal/api/auth_handler.go +++ b/internal/api/auth_handler.go @@ -19,17 +19,24 @@ import ( // AuthHandler handles authentication RPCs. type AuthHandler struct { - store *store.Store - logger *slog.Logger - jwtManager *auth.JWTManager + store *store.Store + logger *slog.Logger + jwtManager *auth.JWTManager + passwordAuthEnabled bool } -// NewAuthHandler creates a new auth handler. -func NewAuthHandler(st *store.Store, logger *slog.Logger, jwtManager *auth.JWTManager) *AuthHandler { +// NewAuthHandler creates a new auth handler. The passwordAuthEnabled flag +// is the global `CONTROL_PASSWORD_AUTH_ENABLED` operator switch. When +// false, Login rejects every password attempt regardless of the per-user +// HasPassword column — previous revs only gated the SSO UI's auth-method +// list on this flag, leaving the RPC itself open to direct password +// attempts against accounts that still had a password hash on disk. +func NewAuthHandler(st *store.Store, logger *slog.Logger, jwtManager *auth.JWTManager, passwordAuthEnabled bool) *AuthHandler { return &AuthHandler{ - store: st, - logger: logger, - jwtManager: jwtManager, + store: st, + logger: logger, + jwtManager: jwtManager, + passwordAuthEnabled: passwordAuthEnabled, } } @@ -39,6 +46,14 @@ func (h *AuthHandler) Login(ctx context.Context, req *connect.Request[pm.LoginRe return nil, err } + // Global password-auth switch — enforced BEFORE the user lookup so a + // burned bcrypt cycle doesn't leak account existence via timing when + // the operator has disabled password login entirely. + if !h.passwordAuthEnabled { + auth.VerifyPassword(req.Msg.Password, auth.DummyHash) + return nil, apiErrorCtx(ctx, ErrPasswordLoginDisabled, connect.CodeUnauthenticated, "password login is disabled on this server") + } + user, err := h.store.Queries().GetUserByEmail(ctx, req.Msg.Email) if err != nil { if errors.Is(err, pgx.ErrNoRows) { diff --git a/internal/api/auth_handler_test.go b/internal/api/auth_handler_test.go index 06607cfc..d3039b45 100644 --- a/internal/api/auth_handler_test.go +++ b/internal/api/auth_handler_test.go @@ -19,7 +19,7 @@ import ( func TestLogin_Success(t *testing.T) { st := testutil.SetupPostgres(t) jwtMgr := testutil.NewJWTManager() - h := api.NewAuthHandler(st, slog.Default(), jwtMgr) + h := api.NewAuthHandler(st, slog.Default(), jwtMgr, true) email := testutil.NewID() + "@test.com" testutil.CreateTestUser(t, st, email, "correct-password", "admin") @@ -40,7 +40,7 @@ func TestLogin_Success(t *testing.T) { func TestLogin_WrongPassword(t *testing.T) { st := testutil.SetupPostgres(t) jwtMgr := testutil.NewJWTManager() - h := api.NewAuthHandler(st, slog.Default(), jwtMgr) + h := api.NewAuthHandler(st, slog.Default(), jwtMgr, true) email := testutil.NewID() + "@test.com" testutil.CreateTestUser(t, st, email, "correct-password", "user") @@ -56,7 +56,7 @@ func TestLogin_WrongPassword(t *testing.T) { func TestLogin_NonexistentUser(t *testing.T) { st := testutil.SetupPostgres(t) jwtMgr := testutil.NewJWTManager() - h := api.NewAuthHandler(st, slog.Default(), jwtMgr) + h := api.NewAuthHandler(st, slog.Default(), jwtMgr, true) _, err := h.Login(context.Background(), connect.NewRequest(&pm.LoginRequest{ Email: "nonexistent@test.com", @@ -66,10 +66,34 @@ func TestLogin_NonexistentUser(t *testing.T) { assert.Equal(t, connect.CodeUnauthenticated, connect.CodeOf(err)) } +// TestLogin_GlobalPasswordAuthDisabled regression-tests the bypass +// discovered in the rc10 audit: the `CONTROL_PASSWORD_AUTH_ENABLED=false` +// operator switch was only plumbed into the SSO handler's +// ListAuthMethods response, so a direct POST to /Login with valid +// credentials still authenticated. Now the switch is enforced in +// AuthHandler.Login itself before the DB lookup, so even an account +// with a password hash on disk cannot authenticate when the operator +// disables password login globally. +func TestLogin_GlobalPasswordAuthDisabled(t *testing.T) { + st := testutil.SetupPostgres(t) + jwtMgr := testutil.NewJWTManager() + h := api.NewAuthHandler(st, slog.Default(), jwtMgr, false) // global switch off + + email := testutil.NewID() + "@test.com" + testutil.CreateTestUser(t, st, email, "correct-password", "admin") + + _, err := h.Login(context.Background(), connect.NewRequest(&pm.LoginRequest{ + Email: email, + Password: "correct-password", // valid credentials + })) + require.Error(t, err, "login must fail when global password auth is disabled") + assert.Equal(t, connect.CodeUnauthenticated, connect.CodeOf(err)) +} + func TestLogin_DisabledUser(t *testing.T) { st := testutil.SetupPostgres(t) jwtMgr := testutil.NewJWTManager() - h := api.NewAuthHandler(st, slog.Default(), jwtMgr) + h := api.NewAuthHandler(st, slog.Default(), jwtMgr, true) email := testutil.NewID() + "@test.com" userID := testutil.CreateTestUser(t, st, email, "password", "user") @@ -89,7 +113,7 @@ func TestLogin_DisabledUser(t *testing.T) { func TestLogin_NoCookiesSet(t *testing.T) { st := testutil.SetupPostgres(t) jwtMgr := testutil.NewJWTManager() - h := api.NewAuthHandler(st, slog.Default(), jwtMgr) + h := api.NewAuthHandler(st, slog.Default(), jwtMgr, true) email := testutil.NewID() + "@test.com" testutil.CreateTestUser(t, st, email, "password", "user") @@ -115,7 +139,7 @@ func TestLogin_NoCookiesSet(t *testing.T) { func TestRefreshToken_RequiresBodyToken(t *testing.T) { st := testutil.SetupPostgres(t) jwtMgr := testutil.NewJWTManager() - h := api.NewAuthHandler(st, slog.Default(), jwtMgr) + h := api.NewAuthHandler(st, slog.Default(), jwtMgr, true) // RefreshToken with empty body should fail. // Proto validation catches the empty refresh_token field. @@ -127,7 +151,7 @@ func TestRefreshToken_RequiresBodyToken(t *testing.T) { func TestRefreshToken_NoCookieFallback(t *testing.T) { st := testutil.SetupPostgres(t) jwtMgr := testutil.NewJWTManager() - h := api.NewAuthHandler(st, slog.Default(), jwtMgr) + h := api.NewAuthHandler(st, slog.Default(), jwtMgr, true) email := testutil.NewID() + "@test.com" testutil.CreateTestUser(t, st, email, "password", "user") @@ -153,7 +177,7 @@ func TestRefreshToken_NoCookieFallback(t *testing.T) { func TestRefreshToken_BodyToken(t *testing.T) { st := testutil.SetupPostgres(t) jwtMgr := testutil.NewJWTManager() - h := api.NewAuthHandler(st, slog.Default(), jwtMgr) + h := api.NewAuthHandler(st, slog.Default(), jwtMgr, true) email := testutil.NewID() + "@test.com" testutil.CreateTestUser(t, st, email, "password", "user") @@ -183,7 +207,7 @@ func TestRefreshToken_BodyToken(t *testing.T) { func TestLogout_NoCookiesCleared(t *testing.T) { st := testutil.SetupPostgres(t) jwtMgr := testutil.NewJWTManager() - h := api.NewAuthHandler(st, slog.Default(), jwtMgr) + h := api.NewAuthHandler(st, slog.Default(), jwtMgr, true) email := testutil.NewID() + "@test.com" testutil.CreateTestUser(t, st, email, "password", "user") @@ -209,7 +233,7 @@ func TestLogout_NoCookiesCleared(t *testing.T) { func TestGetCurrentUser(t *testing.T) { st := testutil.SetupPostgres(t) jwtMgr := testutil.NewJWTManager() - h := api.NewAuthHandler(st, slog.Default(), jwtMgr) + h := api.NewAuthHandler(st, slog.Default(), jwtMgr, true) email := testutil.NewID() + "@test.com" userID := testutil.CreateTestUser(t, st, email, "pass", "admin") @@ -226,7 +250,7 @@ func TestLogin_TOTPRequired(t *testing.T) { st := testutil.SetupPostgres(t) jwtMgr := testutil.NewJWTManager() enc := testutil.NewEncryptor(t) - h := api.NewAuthHandler(st, slog.Default(), jwtMgr) + h := api.NewAuthHandler(st, slog.Default(), jwtMgr, true) email := testutil.NewID() + "@test.com" userID := testutil.CreateTestUser(t, st, email, "password", "user") @@ -251,7 +275,7 @@ func TestLogin_TOTPRequired(t *testing.T) { func TestLogin_TOTPNotRequired(t *testing.T) { st := testutil.SetupPostgres(t) jwtMgr := testutil.NewJWTManager() - h := api.NewAuthHandler(st, slog.Default(), jwtMgr) + h := api.NewAuthHandler(st, slog.Default(), jwtMgr, true) email := testutil.NewID() + "@test.com" testutil.CreateTestUser(t, st, email, "password", "user") @@ -273,7 +297,7 @@ func TestLogin_TOTPNotRequired(t *testing.T) { func TestLogin_PasswordDisabledByProvider(t *testing.T) { st := testutil.SetupPostgres(t) jwtMgr := testutil.NewJWTManager() - h := api.NewAuthHandler(st, slog.Default(), jwtMgr) + h := api.NewAuthHandler(st, slog.Default(), jwtMgr, true) email := testutil.NewID() + "@test.com" userID := testutil.CreateTestUser(t, st, email, "password", "user") @@ -315,7 +339,7 @@ func TestLogin_PasswordDisabledByProvider(t *testing.T) { func TestLogin_SSOOnlyUserNoPassword(t *testing.T) { st := testutil.SetupPostgres(t) jwtMgr := testutil.NewJWTManager() - h := api.NewAuthHandler(st, slog.Default(), jwtMgr) + h := api.NewAuthHandler(st, slog.Default(), jwtMgr, true) email := testutil.NewID() + "@test.com" userID := testutil.NewID() @@ -335,7 +359,7 @@ func TestLogin_SSOOnlyUserNoPassword(t *testing.T) { func TestLogin_UserHasPassword(t *testing.T) { st := testutil.SetupPostgres(t) jwtMgr := testutil.NewJWTManager() - h := api.NewAuthHandler(st, slog.Default(), jwtMgr) + h := api.NewAuthHandler(st, slog.Default(), jwtMgr, true) email := testutil.NewID() + "@test.com" testutil.CreateTestUser(t, st, email, "password", "user") diff --git a/internal/api/service.go b/internal/api/service.go index 32424427..b8c5bb34 100644 --- a/internal/api/service.go +++ b/internal/api/service.go @@ -61,7 +61,7 @@ func NewControlService(st *store.Store, jwtManager *auth.JWTManager, signer Acti settingsHandler := NewSettingsHandler(st, logger, systemActions) return &ControlService{ registration: NewRegistrationHandler(st, certAuth, gatewayURL, logger), - auth: NewAuthHandler(st, logger.With("component", "auth_handler"), jwtManager), + auth: NewAuthHandler(st, logger.With("component", "auth_handler"), jwtManager, cfg.PasswordAuthEnabled), totp: NewTOTPHandler(st, logger.With("component", "totp_handler"), jwtManager, enc, ""), user: NewUserHandler(st, logger.With("component", "user_handler"), systemActions), device: NewDeviceHandler(st, enc, logger.With("component", "device_handler")), diff --git a/internal/config/config.go b/internal/config/config.go index a7c84d09..c7bea92e 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -71,10 +71,11 @@ type Config struct { // Example: "gateway.example.com" BootstrapHost string - // Web listener for non-mTLS traffic (terminal WebSocket). Uses - // standard TLS (server cert only, no client cert) so web browsers - // can connect. Empty disables the web listener — terminal - // sessions won't work but agent connections are unaffected. + // Web listener for non-mTLS traffic (terminal WebSocket). This + // listener serves cleartext HTTP on the private network; public TLS + // terminates at Traefik before proxying to it. Empty disables the + // web listener — terminal sessions won't work but agent connections + // are unaffected. WebListenAddr string // InternalURL is the mTLS URL the control server uses to call @@ -164,7 +165,7 @@ func FromEnv() *Config { // * RootKey = traefik — matches Traefik default --providers.redis.rootkey // * MTLSHost = GATEWAY_DOMAIN (not Traefik-prefixed — one name per thing) // * MTLSEntryPoint = websecure (same :443 as control, SNI-separated) - // * TTYHost = GATEWAY_TTY_DOMAIN (falls back to empty → TTY router disabled) + // * TTYHost = GATEWAY_TTY_DOMAIN (falls back to empty -> TTY router disabled) // * TTYEntryPoint = websecure // * TTYCertResolver = letsencrypt // From 412a6ef90205d041b5010bdc3d109fd6fc0dee87 Mon Sep 17 00:00:00 2001 From: PaulDotterer Date: Thu, 23 Apr 2026 08:09:55 +0200 Subject: [PATCH 02/11] =?UTF-8?q?fix:=20rc10=20=E2=80=94=20fail-open=20gat?= =?UTF-8?q?eway=20startup,=20single-use=20terminal=20tokens,=20LUKS=20stre?= =?UTF-8?q?am-id=20coherence,=20deploy=20hardening?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gateway startup: convert five more os.Exit paths to Warn+feature- disabled so a broken optional knob no longer takes agent mTLS down with it. - registry.RegisterGateway failure → terminal disabled for this replica, agent mTLS keeps serving. - routableIP() failure for GATEWAY_INTERNAL_URL → admin fan-out disabled, mTLS keeps serving. - routableIP() failure for Traefik backend auto-derive → skip publish entirely rather than poison Redis KV with an empty backend; operator can still set GATEWAY_TRAEFIK_{MTLS,TTY}_BACKEND explicitly. - PublishTraefikRoute failure at startup → rely on the refresh goroutine's normal retry cadence; the previous crash loop caused the pm-mtls TCP route to expire during restart backoff and fall through to the HTTP router (wrong cert). - BootstrapHost without a derivable assignedHost → disable the bootstrap middleware instead of crashing. The remaining os.Exit paths in gateway startup (TLS cert / key / CA flag missing, ValkeyAddr / ControlURL missing, cert parse, tls.Config build, HTTP/2 server configure) are truly fatal and stay fatal — the gateway cannot serve its primary mTLS role without them. Terminal tokens: enforce single-use via Valkey GETDEL (go-redis GetDel) in the new SessionBackend.GetAndDelete primitive. Validate() now atomically consumes the token on success, so a leaked bearer token cannot replay a second WebSocket within the TTL. Forgery attempts restore the unused entry with the remaining TTL so a legitimate client is not locked out by a guess. Regression tests TestTokenStore_Validate_IsSingleUse and TestTokenStore_Validate_MismatchPreservesSession cover both paths. LUKS revocation three-phase audit: the inbox worker now looks up the luks_key stream ID minted at request time (Requested / Dispatched phases in api/device_handler.go) via a new sqlc query GetLuksRevocationStreamID, so the async Revoked / Failed event lands on the SAME stream as the earlier phases. Previously the worker generated a fresh ULID, splitting every revocation across two streams and breaking the projection's stitch. Falls back to a fresh ULID only if the Requested event can't be found — an orphan Failed event is still better than silently dropping the agent's report. Config coherence: - GATEWAY_TTY host fallback now goes GATEWAY_TRAEFIK_TTY_HOST → GATEWAY_TTY_DOMAIN → GATEWAY_DOMAIN. .env.example has always documented the third rung; config.go finally implements it. - Empty CONTROL_GATEWAY_URL is fatal at startup (was only a warn), and the registration handler refuses to hand an empty URL back even if the startup guard ever regresses. Defence in depth on a path that was otherwise silently broken. - deploy/compose.yml: GATEWAY_VALKEY_DB passthrough added (the code reads it; without this the env var was dead). - IMAGE_TAG now required (fail with clear message if unset) rather than silently defaulting to :latest, which was causing unintended upgrades on every `docker compose pull`. - redis-stack-server pinned to 7.4.0-v0 so upstream changes don't silently land. - initdb.d/01-indexer-user.sh now hard-fails on unset INDEXER_POSTGRES_PASSWORD instead of returning 0, so the misconfiguration is visible at init time rather than hours later at indexer first-connect. Auth: TestLogin_GlobalPasswordAuthDisabled regression test (from prior commit) covers the rc10 bypass fix on the password-auth disable path. Part of manchtools/power-manage-server#74. --- cmd/control/main.go | 15 +- cmd/gateway/main.go | 206 ++++++++++++++++----------- deploy/compose.yml | 12 +- deploy/initdb.d/01-indexer-user.sh | 9 +- internal/api/registration_handler.go | 13 ++ internal/config/config.go | 12 +- internal/control/inbox_worker.go | 24 +++- internal/store/generated/luks.sql.go | 30 ++++ internal/store/queries/luks.sql | 18 +++ internal/terminal/fake_backend.go | 19 +++ internal/terminal/store.go | 49 ++++++- internal/terminal/store_test.go | 63 ++++++++ internal/terminal/valkey_backend.go | 16 +++ 13 files changed, 385 insertions(+), 101 deletions(-) diff --git a/cmd/control/main.go b/cmd/control/main.go index d1cf8610..3918c6f9 100644 --- a/cmd/control/main.go +++ b/cmd/control/main.go @@ -9,6 +9,7 @@ import ( "fmt" "log/slog" "net/http" + "net/url" urlpkg "net/url" "os" "os/signal" @@ -92,8 +93,20 @@ func main() { logger := logging.SetupLogger(cfg.LogLevel, cfg.LogFormat, os.Stderr) slog.SetDefault(logger) logger.Info("starting control server", "version", version, "listen_addr", cfg.ListenAddr, "gateway_url", cfg.GatewayURL, "dynamic_group_eval_interval", cfg.DynamicGroupEvalInterval) + // CONTROL_GATEWAY_URL is fatal when empty: registration hands it + // back to the agent verbatim, so a missing value turns every + // successful enrollment into an agent that can never connect — + // and the failure mode only shows up later, at first-stream time, + // with a cryptic URL-parse error far from the config source. + // Fail fast here so the operator notices the misconfiguration at + // startup instead of after devices are already enrolled. if cfg.GatewayURL == "" { - logger.Warn("CONTROL_GATEWAY_URL is not set - agents will not receive a gateway URL during registration") + logger.Error("CONTROL_GATEWAY_URL is required — enrollment cannot hand agents an empty gateway URL") + os.Exit(1) + } + if _, err := url.Parse(cfg.GatewayURL); err != nil { + logger.Error("CONTROL_GATEWAY_URL is not a valid URL", "gateway_url", cfg.GatewayURL, "error", err) + os.Exit(1) } // Setup signal handling diff --git a/cmd/gateway/main.go b/cmd/gateway/main.go index 77f7ad32..76f14934 100644 --- a/cmd/gateway/main.go +++ b/cmd/gateway/main.go @@ -201,9 +201,6 @@ func main() { // gateway never constructs hostnames from the request side. terminalURL := strings.ReplaceAll(cfg.PublicTerminalURLTemplate, "{id}", gatewayID) - // If no agent URL template was set, fall back to deriving the - // agent redirect hostname from the terminal URL (legacy - // single-hostname mode). terminalHost := hostFromURL(terminalURL) if terminalHost == "" { // Malformed template — skip the registry work instead of @@ -212,24 +209,31 @@ func main() { logger.Warn("GATEWAY_PUBLIC_TERMINAL_URL_TEMPLATE resolved to a URL with no host — terminal session registration disabled on this gateway; check for unset env vars in the template", "template", cfg.PublicTerminalURLTemplate, "resolved", terminalURL) } else { + // If no agent URL template was set, fall back to deriving the + // agent redirect hostname from the terminal URL (legacy + // single-hostname mode). if assignedHost == "" { assignedHost = terminalHost } - gatewayReg = ensureGatewayRegistry() - stop, err := gatewayReg.RegisterGateway( - context.Background(), - gatewayID, + gatewayReg = ensureGatewayRegistry() + stop, err := gatewayReg.RegisterGateway( + context.Background(), + gatewayID, terminalURL, registry.DefaultGatewayTTL, registry.DefaultGatewayRefreshInterval, ) if err != nil { - logger.Error("failed to register gateway in registry", "error", err) - os.Exit(1) - } + // Fail-open: the terminal feature is optional, so a + // transient registry failure at startup must not kill + // the gateway's agent-mTLS service. Log loudly and + // leave terminal sessions disabled for this replica + // until the operator restarts. + logger.Warn("failed to register gateway in terminal registry — terminal sessions disabled on this replica", + "error", err) + } else { defer stop() - logger.Info("multi-gateway routing enabled", "gateway_id", gatewayID, "terminal_url", terminalURL, @@ -237,6 +241,7 @@ func main() { ) } } + } // Traefik Redis-KV self-registration. Opt-in; when enabled, every // replica writes its own routing entry into the same Valkey @@ -265,76 +270,97 @@ func main() { if mtlsBackend == "" || ttyBackend == "" { ip, err := routableIP() if err != nil { - logger.Error("cannot auto-derive Traefik backends", "error", err) - os.Exit(1) - } - if mtlsBackend == "" { - mtlsBackend = ip + portOfListenAddr(cfg.ListenAddr) - } - if ttyBackend == "" && cfg.WebListenAddr != "" { - ttyBackend = "http://" + ip + portOfListenAddr(cfg.WebListenAddr) + // Fail-open: the operator can still publish a manual + // backend via GATEWAY_TRAEFIK_{MTLS,TTY}_BACKEND, so + // missing auto-derivation is not fatal. If nothing is + // set we skip Traefik publication entirely below and + // the gateway keeps serving agent mTLS on whatever + // static route already points at it. + logger.Warn("cannot auto-derive Traefik backends from network interfaces — set GATEWAY_TRAEFIK_{MTLS,TTY}_BACKEND explicitly if self-registration is needed", + "error", err) + } else { + if mtlsBackend == "" { + mtlsBackend = ip + portOfListenAddr(cfg.ListenAddr) + } + if ttyBackend == "" && cfg.WebListenAddr != "" { + ttyBackend = "http://" + ip + portOfListenAddr(cfg.WebListenAddr) + } } } - traefikCfg := registry.TraefikRouteConfig{ - RootKey: cfg.TraefikRootKey, - MTLSHost: cfg.TraefikMTLSHost, - MTLSBackend: mtlsBackend, - MTLSEntryPoint: cfg.TraefikMTLSEntryPoint, - TTYHost: cfg.TraefikTTYHost, - TTYBackend: ttyBackend, - TTYEntryPoint: cfg.TraefikTTYEntryPoint, - TTYCertResolver: cfg.TraefikTTYCertResolver, - } + // Publish only when we actually have a usable mTLS backend — + // an empty backend in the Redis KV entry would poison the + // pm-mtls TCP router for every replica. + if mtlsBackend == "" { + logger.Warn("Traefik self-registration skipped — no MTLS backend available (routable-IP auto-derive failed and GATEWAY_TRAEFIK_MTLS_BACKEND is unset)") + } else { + traefikCfg := registry.TraefikRouteConfig{ + RootKey: cfg.TraefikRootKey, + MTLSHost: cfg.TraefikMTLSHost, + MTLSBackend: mtlsBackend, + MTLSEntryPoint: cfg.TraefikMTLSEntryPoint, + TTYHost: cfg.TraefikTTYHost, + TTYBackend: ttyBackend, + TTYEntryPoint: cfg.TraefikTTYEntryPoint, + TTYCertResolver: cfg.TraefikTTYCertResolver, + } - if err := gatewayReg.PublishTraefikRoute( - context.Background(), gatewayID, traefikCfg, registry.DefaultGatewayTTL, - ); err != nil { - logger.Error("failed to publish Traefik routing config", "error", err) - os.Exit(1) - } + if err := gatewayReg.PublishTraefikRoute( + context.Background(), gatewayID, traefikCfg, registry.DefaultGatewayTTL, + ); err != nil { + // Fail-open: the refresh goroutine below retries on + // the normal cadence, so a transient publish failure + // at startup recovers within one refresh interval. + // Killing the gateway used to be worse — the Redis + // KV entry expired during the restart backoff and + // Traefik fell through to the HTTP router (wrong + // cert). Log loudly and keep serving agent mTLS. + logger.Warn("failed to publish initial Traefik routing config — refresh goroutine will retry", + "error", err) + } - // Refresh on the same cadence as the gateway terminal URL and - // internal URL so all three keys share a lifecycle. Derive from - // shutdownCtx so the goroutine exits cleanly on SIGTERM. - traefikRefreshCtx, stopTraefikRefresh := context.WithCancel(shutdownCtx) - defer stopTraefikRefresh() - go func() { - ticker := time.NewTicker(registry.DefaultGatewayRefreshInterval) - defer ticker.Stop() - for { - select { - case <-ticker.C: - if err := gatewayReg.PublishTraefikRoute( - context.Background(), gatewayID, traefikCfg, registry.DefaultGatewayTTL, - ); err != nil { - logger.Warn("failed to refresh Traefik routing config", "error", err) + // Refresh on the same cadence as the gateway terminal URL + // so both keys share a lifecycle. Derive from shutdownCtx + // so the goroutine exits cleanly on SIGTERM. + traefikRefreshCtx, stopTraefikRefresh := context.WithCancel(shutdownCtx) + defer stopTraefikRefresh() + go func() { + ticker := time.NewTicker(registry.DefaultGatewayRefreshInterval) + defer ticker.Stop() + for { + select { + case <-ticker.C: + if err := gatewayReg.PublishTraefikRoute( + context.Background(), gatewayID, traefikCfg, registry.DefaultGatewayTTL, + ); err != nil { + logger.Warn("failed to refresh Traefik routing config", "error", err) + } + case <-traefikRefreshCtx.Done(): + return } - case <-traefikRefreshCtx.Done(): - return } - } - }() + }() - // Clean shutdown revokes only per-replica keys so other - // replicas' routes stay up. Uses a bounded context so a flaky - // Valkey can't stall the shutdown. - defer func() { - cleanupCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - defer cancel() - if err := gatewayReg.RevokeTraefikRoute(cleanupCtx, gatewayID, traefikCfg); err != nil { - logger.Warn("failed to revoke Traefik routing config", "error", err) - } - }() + // Clean shutdown revokes only per-replica keys so other + // replicas' routes stay up. Uses a bounded context so a + // flaky Valkey can't stall the shutdown. + defer func() { + cleanupCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if err := gatewayReg.RevokeTraefikRoute(cleanupCtx, gatewayID, traefikCfg); err != nil { + logger.Warn("failed to revoke Traefik routing config", "error", err) + } + }() - logger.Info("traefik self-registration enabled", - "gateway_id", gatewayID, - "mtls_host", cfg.TraefikMTLSHost, - "mtls_backend", cfg.TraefikMTLSBackend, - "tty_host", cfg.TraefikTTYHost, - "tty_backend", cfg.TraefikTTYBackend, - "tty_cert_resolver", cfg.TraefikTTYCertResolver, - ) + logger.Info("traefik self-registration enabled", + "gateway_id", gatewayID, + "mtls_host", cfg.TraefikMTLSHost, + "mtls_backend", mtlsBackend, + "tty_host", cfg.TraefikTTYHost, + "tty_backend", ttyBackend, + "tty_cert_resolver", cfg.TraefikTTYCertResolver, + ) + } } // Publish the internal mTLS URL so the control server can discover @@ -347,11 +373,18 @@ func main() { if internalURL == "" { ip, err := routableIP() if err != nil { - logger.Error("cannot auto-derive GATEWAY_INTERNAL_URL", "error", err) - os.Exit(1) + // Fail-open: the internal URL feeds the admin + // fan-out path (List/Terminate terminal sessions). + // It is strictly optional for agent mTLS service. + // Crashing the gateway here would take the agent + // routing layer down with it, which is exactly the + // rc8 failure mode we're fixing. Log and move on. + logger.Warn("cannot auto-derive GATEWAY_INTERNAL_URL — admin fan-out disabled for this replica; set GATEWAY_INTERNAL_URL explicitly to re-enable", + "error", err) + } else { + internalURL = "https://" + ip + portOfListenAddr(cfg.ListenAddr) + logger.Info("auto-derived GATEWAY_INTERNAL_URL", "internal_url", internalURL) } - internalURL = "https://" + ip + portOfListenAddr(cfg.ListenAddr) - logger.Info("auto-derived GATEWAY_INTERNAL_URL", "internal_url", internalURL) } if internalURL != "" { if err := gatewayReg.RegisterGatewayInternal( @@ -380,14 +413,17 @@ func main() { } } - // Fail fast if BootstrapHost is set but we have no assignedHost - // (because PublicTerminalURLTemplate was empty). Without this - // guard, BootstrapRedirectMiddleware would panic on an empty - // assignedHost further down. - if cfg.BootstrapHost != "" && assignedHost == "" { - logger.Error("GATEWAY_BOOTSTRAP_HOST is set but neither GATEWAY_PUBLIC_AGENT_URL_TEMPLATE nor GATEWAY_PUBLIC_TERMINAL_URL_TEMPLATE is set; cannot derive the per-gateway hostname for bootstrap redirects", - "bootstrap_host", cfg.BootstrapHost) - os.Exit(1) + // If BootstrapHost is set but no assignedHost is available + // (because both agent + terminal URL templates were empty or + // malformed), disable the bootstrap-redirect middleware instead + // of crashing. Agent mTLS still works — operators just lose the + // convenience redirect that points newly-enrolled agents at a + // stable per-gateway hostname. + bootstrapHost := cfg.BootstrapHost + if bootstrapHost != "" && assignedHost == "" { + logger.Warn("GATEWAY_BOOTSTRAP_HOST is set but no assigned host could be derived from GATEWAY_PUBLIC_AGENT_URL_TEMPLATE / GATEWAY_PUBLIC_TERMINAL_URL_TEMPLATE — bootstrap redirects disabled", + "bootstrap_host", bootstrapHost) + bootstrapHost = "" // disable middleware below } // Terminal session registry — shared between the agent bidi @@ -416,7 +452,7 @@ func main() { // ↑ BootstrapRedirectMiddleware (returns 307 to assignedHost // when the request landed on the wildcard root via LB) mtlsHandler := handler.MTLSMiddleware(h, logger) - bootstrappedHandler := handler.BootstrapRedirectMiddleware(mtlsHandler, cfg.BootstrapHost, assignedHost, logger) + bootstrappedHandler := handler.BootstrapRedirectMiddleware(mtlsHandler, bootstrapHost, assignedHost, logger) mux.Handle(path, bootstrappedHandler) // Mount GatewayService on the mTLS listener (internal-only, diff --git a/deploy/compose.yml b/deploy/compose.yml index 65d7ec1c..dd9e03e8 100644 --- a/deploy/compose.yml +++ b/deploy/compose.yml @@ -81,7 +81,10 @@ services: - internal valkey: - image: docker.io/redis/redis-stack-server:latest + # Pinned to avoid silent upstream upgrades. Bump deliberately when + # you've verified the RediSearch module + data format still work + # for your deployment. + image: docker.io/redis/redis-stack-server:7.4.0-v0 container_name: pm-valkey restart: unless-stopped volumes: @@ -96,7 +99,7 @@ services: - internal control: - image: ghcr.io/manchtools/power-manage-control:${IMAGE_TAG:-latest} + image: ghcr.io/manchtools/power-manage-control:${IMAGE_TAG:?IMAGE_TAG must be set in .env (pin to a specific release tag like v2026.05 — defaulting to \"latest\" caused silent upgrades on every docker compose pull)} container_name: pm-control restart: unless-stopped depends_on: @@ -152,7 +155,7 @@ services: - internal indexer: - image: ghcr.io/manchtools/power-manage-indexer:${IMAGE_TAG:-latest} + image: ghcr.io/manchtools/power-manage-indexer:${IMAGE_TAG:?IMAGE_TAG must be set in .env (pin to a specific release tag like v2026.05 — defaulting to \"latest\" caused silent upgrades on every docker compose pull)} container_name: pm-indexer restart: unless-stopped depends_on: @@ -179,7 +182,7 @@ services: # No container_name: scaling to N replicas needs Compose to auto-name # containers (pm-server-gateway-1, -2, ...). Self-registration into # Traefik's Redis KV means no per-replica Traefik labels are needed. - image: ghcr.io/manchtools/power-manage-gateway:${IMAGE_TAG:-latest} + image: ghcr.io/manchtools/power-manage-gateway:${IMAGE_TAG:?IMAGE_TAG must be set in .env (pin to a specific release tag like v2026.05 — defaulting to \"latest\" caused silent upgrades on every docker compose pull)} restart: unless-stopped depends_on: valkey: @@ -191,6 +194,7 @@ services: environment: - GATEWAY_VALKEY_ADDR=valkey:6379 - GATEWAY_VALKEY_PASSWORD=${VALKEY_PASSWORD} + - GATEWAY_VALKEY_DB=${GATEWAY_VALKEY_DB:-0} - GATEWAY_CONTROL_URL=https://control:8082 - GATEWAY_LISTEN_ADDR=:8080 - GATEWAY_LOG_LEVEL=${LOG_LEVEL:-info} diff --git a/deploy/initdb.d/01-indexer-user.sh b/deploy/initdb.d/01-indexer-user.sh index 3d92936e..e71c3f13 100755 --- a/deploy/initdb.d/01-indexer-user.sh +++ b/deploy/initdb.d/01-indexer-user.sh @@ -6,9 +6,14 @@ # ALTER ROLE pm_indexer PASSWORD 'your_password'; set -e +# Hard-fail on unset password: returning 0 caused the indexer container +# to crash-loop later with an obscure "no password supplied" error at +# first connect, hours after `setup.sh` had already reported success. +# Failing the init script keeps the failure at setup time where the +# operator is still paying attention. if [ -z "$INDEXER_POSTGRES_PASSWORD" ]; then - echo "WARN: INDEXER_POSTGRES_PASSWORD not set, skipping pm_indexer user creation" - exit 0 + echo "ERROR: INDEXER_POSTGRES_PASSWORD is required — set it in .env before initialising postgres (must be URL-safe; see .env.example)" >&2 + exit 1 fi psql -v ON_ERROR_STOP=1 --username "$POSTGRES_USER" --dbname "$POSTGRES_DB" <<-EOSQL diff --git a/internal/api/registration_handler.go b/internal/api/registration_handler.go index 0f447c72..ce9408a9 100644 --- a/internal/api/registration_handler.go +++ b/internal/api/registration_handler.go @@ -41,6 +41,19 @@ func (h *RegistrationHandler) Register(ctx context.Context, req *connect.Request logger := h.logger.With("hostname", req.Msg.Hostname, "agent_version", req.Msg.AgentVersion) logger.Info("processing registration request") + // Defence in depth: the startup guard in cmd/control/main.go + // already fails fast on an empty GATEWAY_URL, but if that guard + // ever regresses (or the handler is constructed directly in a + // test), refuse the registration rather than hand the agent a + // URL it cannot connect to. Silent empty-URL enrollment is the + // worst failure mode: the device is created in the DB, the cert + // is signed, but the first stream attempt fails with a + // URL-parse error far from the config source. + if h.gatewayURL == "" { + logger.Error("registration refused: gatewayURL is empty — CONTROL_GATEWAY_URL must be set") + return nil, apiErrorCtx(ctx, ErrInternal, connect.CodeFailedPrecondition, "server misconfiguration: gateway URL is not set") + } + // Validate CSR is present if len(req.Msg.Csr) == 0 { return nil, apiErrorCtx(ctx, ErrValidationFailed, connect.CodeInvalidArgument, "CSR is required") diff --git a/internal/config/config.go b/internal/config/config.go index c7bea92e..d59b8385 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -193,7 +193,17 @@ func FromEnv() *Config { TraefikMTLSHost: firstNonEmpty(os.Getenv("GATEWAY_TRAEFIK_MTLS_HOST"), os.Getenv("GATEWAY_DOMAIN")), TraefikMTLSBackend: getEnv("GATEWAY_TRAEFIK_MTLS_BACKEND", ""), TraefikMTLSEntryPoint: getEnv("GATEWAY_TRAEFIK_MTLS_ENTRYPOINT", "websecure"), - TraefikTTYHost: firstNonEmpty(os.Getenv("GATEWAY_TRAEFIK_TTY_HOST"), os.Getenv("GATEWAY_TTY_DOMAIN")), + // TTYHost falls back through GATEWAY_TTY_DOMAIN (dedicated + // TTY subdomain) to GATEWAY_DOMAIN (single-domain deploys + // where terminal WebSocket shares the gateway hostname). + // .env.example documents exactly this chain; rc9 only + // implemented the first two rungs, which was the hidden + // empty-host trap that crashed gateway startup in staging. + TraefikTTYHost: firstNonEmpty( + os.Getenv("GATEWAY_TRAEFIK_TTY_HOST"), + os.Getenv("GATEWAY_TTY_DOMAIN"), + os.Getenv("GATEWAY_DOMAIN"), + ), TraefikTTYBackend: getEnv("GATEWAY_TRAEFIK_TTY_BACKEND", ""), TraefikTTYEntryPoint: getEnv("GATEWAY_TRAEFIK_TTY_ENTRYPOINT", "websecure"), TraefikTTYCertResolver: getEnv("GATEWAY_TRAEFIK_TTY_CERT_RESOLVER", "letsencrypt"), diff --git a/internal/control/inbox_worker.go b/internal/control/inbox_worker.go index e2a62112..bb25b917 100644 --- a/internal/control/inbox_worker.go +++ b/internal/control/inbox_worker.go @@ -535,7 +535,29 @@ func (w *InboxWorker) handleRevokeLuksDeviceKeyResult(ctx context.Context, t *as "success", payload.Success, ) - luksStreamID := ulid.Make().String() + // Look up the stream ID minted at request time so the Revoked / + // Failed event lands on the SAME stream as the Requested / + // Dispatched phases. Earlier versions generated a fresh ULID + // here, which split every revocation across two streams and + // broke the projection's three-phase stitch — fixed in rc10. + // If the lookup fails (e.g. the Requested event never made it + // to disk because the original API call crashed), fall back to + // a fresh stream ID so we still record the Failed outcome + // durably — an orphan Failed event is better than dropping the + // agent-reported failure on the floor. + luksStreamID, err := w.store.Queries().GetLuksRevocationStreamID(ctx, db.GetLuksRevocationStreamIDParams{ + DeviceID: payload.DeviceID, + ActionID: payload.ActionID, + }) + if err != nil { + w.logger.Warn("could not look up LUKS revocation stream ID — appending to a fresh stream; projection will show only the terminal event", + "device_id", payload.DeviceID, + "action_id", payload.ActionID, + "error", err, + ) + luksStreamID = ulid.Make().String() + } + if payload.Success { return w.store.AppendEvent(ctx, store.Event{ StreamType: "luks_key", diff --git a/internal/store/generated/luks.sql.go b/internal/store/generated/luks.sql.go index d1273f8f..2e0df1bf 100644 --- a/internal/store/generated/luks.sql.go +++ b/internal/store/generated/luks.sql.go @@ -166,6 +166,36 @@ func (q *Queries) GetLuksKeyHistory(ctx context.Context, deviceID string) ([]Luk return items, nil } +const getLuksRevocationStreamID = `-- name: GetLuksRevocationStreamID :one +SELECT stream_id +FROM events +WHERE stream_type = 'luks_key' + AND event_type IN ('LuksDeviceKeyRevocationRequested', 'LuksDeviceKeyRevocationDispatched') + AND data->>'device_id' = $1::text + AND data->>'action_id' = $2::text +ORDER BY sequence_num DESC +LIMIT 1 +` + +type GetLuksRevocationStreamIDParams struct { + DeviceID string `json:"device_id"` + ActionID string `json:"action_id"` +} + +// GetLuksRevocationStreamID looks up the luks_key event-stream ID that +// was minted when api/device_handler.go appended the +// LuksDeviceKeyRevocationRequested event for this (device, action). +// The inbox worker uses it to append the final Revoked / Failed event +// to the SAME stream so the three-phase projection stitches together. +// Returns the most recent request if somehow there are multiple (there +// should only ever be one; LIMIT 1 is belt-and-braces). +func (q *Queries) GetLuksRevocationStreamID(ctx context.Context, arg GetLuksRevocationStreamIDParams) (string, error) { + row := q.db.QueryRow(ctx, getLuksRevocationStreamID, arg.DeviceID, arg.ActionID) + var stream_id string + err := row.Scan(&stream_id) + return stream_id, err +} + const validateAndConsumeLuksToken = `-- name: ValidateAndConsumeLuksToken :one UPDATE luks_tokens SET used = TRUE diff --git a/internal/store/queries/luks.sql b/internal/store/queries/luks.sql index f59946e3..99ba059e 100644 --- a/internal/store/queries/luks.sql +++ b/internal/store/queries/luks.sql @@ -31,3 +31,21 @@ WHERE token = $1 AND NOT used AND expires_at > NOW() RETURNING *; + +-- GetLuksRevocationStreamID looks up the luks_key event-stream ID that +-- was minted when api/device_handler.go appended the +-- LuksDeviceKeyRevocationRequested event for this (device, action). +-- The inbox worker uses it to append the final Revoked / Failed event +-- to the SAME stream so the three-phase projection stitches together. +-- Returns the most recent request if somehow there are multiple (there +-- should only ever be one; LIMIT 1 is belt-and-braces). +-- +-- name: GetLuksRevocationStreamID :one +SELECT stream_id +FROM events +WHERE stream_type = 'luks_key' + AND event_type IN ('LuksDeviceKeyRevocationRequested', 'LuksDeviceKeyRevocationDispatched') + AND data->>'device_id' = sqlc.arg(device_id)::text + AND data->>'action_id' = sqlc.arg(action_id)::text +ORDER BY sequence_num DESC +LIMIT 1; diff --git a/internal/terminal/fake_backend.go b/internal/terminal/fake_backend.go index 87c585f5..babbbca7 100644 --- a/internal/terminal/fake_backend.go +++ b/internal/terminal/fake_backend.go @@ -70,3 +70,22 @@ func (b *FakeBackend) Delete(ctx context.Context, sessionID string) error { delete(b.values, sessionID) return nil } + +// GetAndDelete mirrors the Valkey GETDEL primitive: returns the payload +// and removes the entry in a single atomic step under the mutex, so +// two concurrent callers cannot both observe it. Essential for the +// single-use token semantics Validate relies on. +func (b *FakeBackend) GetAndDelete(ctx context.Context, sessionID string) ([]byte, error) { + b.mu.Lock() + defer b.mu.Unlock() + entry, ok := b.values[sessionID] + if !ok { + return nil, ErrTokenNotFound + } + if !b.now().Before(entry.expiresAt) { + delete(b.values, sessionID) + return nil, ErrTokenNotFound + } + delete(b.values, sessionID) + return append([]byte(nil), entry.payload...), nil +} diff --git a/internal/terminal/store.go b/internal/terminal/store.go index 1ea102b1..a27a98df 100644 --- a/internal/terminal/store.go +++ b/internal/terminal/store.go @@ -101,6 +101,16 @@ type SessionBackend interface { // Delete removes the session_id. Idempotent: returns nil whether // or not the key existed. Delete(ctx context.Context, sessionID string) error + // GetAndDelete atomically returns the payload and removes the + // session_id in one operation. Used by Validate to enforce + // single-use tokens: two concurrent connect attempts with the + // same bearer can only succeed once — the loser sees + // ErrTokenNotFound. Implementations must use a primitive that + // cannot race (Valkey GETDEL, an in-process mutex on the fake, + // etc.). A naïve Get-then-Delete pair does NOT satisfy this + // contract; returning a nil payload and nil error MUST be + // translated to ErrTokenNotFound. + GetAndDelete(ctx context.Context, sessionID string) ([]byte, error) } // TokenStore is the high-level façade used by the API handlers. It @@ -242,20 +252,45 @@ func (s *TokenStore) Lookup(ctx context.Context, sessionID string) (*Session, er } // Validate verifies that the supplied bearer token matches the one -// stored for the given session_id and returns the Session. Used by -// the gateway-side validation path (will be wired via an internal -// RPC in a follow-up PR). Distinguishes ErrTokenNotFound (expired or -// never minted) from ErrTokenMismatch (forgery attempt) so the audit -// log can record them differently. +// stored for the given session_id, atomically consumes the token on +// success, and returns the Session. Single-use: a second call with +// the same bearer returns ErrTokenNotFound even within the TTL. +// +// Distinguishes ErrTokenNotFound (expired, never minted, or already +// consumed) from ErrTokenMismatch (bearer forgery attempt) so the +// audit log can record forgeries separately. Used by the +// gateway-side InternalService.ValidateTerminalToken path. +// +// On mismatch the session entry is re-persisted with the same +// remaining TTL so a forged bearer cannot DoS a legitimate session +// that has not yet been claimed. (GETDEL has already removed it; if +// we did not re-set, the real client's subsequent Validate would +// hit ErrTokenNotFound.) func (s *TokenStore) Validate(ctx context.Context, sessionID, bearerToken string) (*Session, error) { - session, err := s.Lookup(ctx, sessionID) + payload, err := s.backend.GetAndDelete(ctx, sessionID) if err != nil { return nil, err } + var session Session + if err := json.Unmarshal(payload, &session); err != nil { + return nil, fmt.Errorf("terminal: decode session %s: %w", sessionID, err) + } if subtle.ConstantTimeCompare([]byte(session.TokenHash), []byte(hashToken(bearerToken))) != 1 { + // Forgery attempt — restore the real session so the + // legitimate client isn't locked out. Compute the remaining + // TTL from ExpiresAt; if already expired we just drop it. + remaining := session.ExpiresAt.Sub(s.now()) + if remaining > 0 { + if restoreErr := s.backend.Set(ctx, sessionID, payload, remaining); restoreErr != nil { + // Log via caller — we don't have a logger here. Returning + // mismatch is the priority; the caller surfaces it as + // Unauthenticated and the audit pipeline flags it. + return nil, ErrTokenMismatch + } + } return nil, ErrTokenMismatch } - return session, nil + return &session, nil } // Revoke removes the session entry, making subsequent Validate / diff --git a/internal/terminal/store_test.go b/internal/terminal/store_test.go index e3047943..fd558656 100644 --- a/internal/terminal/store_test.go +++ b/internal/terminal/store_test.go @@ -117,6 +117,69 @@ func TestTokenStore_Validate_UnknownSession(t *testing.T) { } } +// TestTokenStore_Validate_IsSingleUse covers the rc10 hardening: a +// bearer token can only be successfully validated once. A second +// Validate call with the same (session_id, bearer) must return +// ErrTokenNotFound even within the TTL. This blocks the replay +// surface where a token leaks (e.g. via a reverse-proxy access log +// that captures query params) and an attacker mints additional +// WebSocket connections during the 60 s window. +func TestTokenStore_Validate_IsSingleUse(t *testing.T) { + store := NewTokenStore(NewFakeBackend(nil)) + ctx := context.Background() + + res, err := store.Mint(ctx, MintParams{ + UserID: "user-1", + DeviceID: "device-1", + TtyUser: "pm-tty-alice", + }) + if err != nil { + t.Fatalf("mint: %v", err) + } + + if _, err := store.Validate(ctx, res.SessionID, res.Token); err != nil { + t.Fatalf("first validate should succeed, got %v", err) + } + + _, err = store.Validate(ctx, res.SessionID, res.Token) + if !errors.Is(err, ErrTokenNotFound) { + t.Errorf("second validate must return ErrTokenNotFound (single-use), got %v", err) + } +} + +// TestTokenStore_Validate_MismatchPreservesSession covers the +// companion invariant: a forgery attempt consumes-and-restores the +// entry so the legitimate client can still validate once later. +// Without this behavior, any attacker guess of a session_id would +// lock out the real web client for the remaining TTL. +func TestTokenStore_Validate_MismatchPreservesSession(t *testing.T) { + store := NewTokenStore(NewFakeBackend(nil)) + ctx := context.Background() + + res, err := store.Mint(ctx, MintParams{ + UserID: "user-1", + DeviceID: "device-1", + TtyUser: "pm-tty-alice", + }) + if err != nil { + t.Fatalf("mint: %v", err) + } + + if _, err := store.Validate(ctx, res.SessionID, "wrong-token"); !errors.Is(err, ErrTokenMismatch) { + t.Fatalf("forgery should return ErrTokenMismatch, got %v", err) + } + + // Legitimate client still gets through on the next attempt. + if _, err := store.Validate(ctx, res.SessionID, res.Token); err != nil { + t.Errorf("legitimate validate after forgery should succeed, got %v", err) + } + + // ... and the token is consumed after that one success. + if _, err := store.Validate(ctx, res.SessionID, res.Token); !errors.Is(err, ErrTokenNotFound) { + t.Errorf("re-validation after legitimate use should be ErrTokenNotFound, got %v", err) + } +} + func TestTokenStore_Revoke_IsIdempotent(t *testing.T) { store := NewTokenStore(NewFakeBackend(nil)) ctx := context.Background() diff --git a/internal/terminal/valkey_backend.go b/internal/terminal/valkey_backend.go index 62957b69..2fff7644 100644 --- a/internal/terminal/valkey_backend.go +++ b/internal/terminal/valkey_backend.go @@ -52,3 +52,19 @@ func (b *ValkeyBackend) Delete(ctx context.Context, sessionID string) error { } return nil } + +// GetAndDelete atomically returns the payload and removes the key in a +// single Valkey/Redis round-trip using GETDEL (available since Redis +// 6.2; redis-stack-server ships well past that). This is the primitive +// that makes terminal tokens single-use — two concurrent validators +// cannot both observe the payload. +func (b *ValkeyBackend) GetAndDelete(ctx context.Context, sessionID string) ([]byte, error) { + payload, err := b.client.GetDel(ctx, keyPrefix+sessionID).Bytes() + if err != nil { + if errors.Is(err, redis.Nil) { + return nil, ErrTokenNotFound + } + return nil, fmt.Errorf("terminal: valkey getdel: %w", err) + } + return payload, nil +} From a5e1314272e6f1aea95dcb672d343bae600c4876 Mon Sep 17 00:00:00 2001 From: PaulDotterer Date: Thu, 23 Apr 2026 08:14:36 +0200 Subject: [PATCH 03/11] =?UTF-8?q?fix:=20rc10=20=E2=80=94=20SecurityAlert?= =?UTF-8?q?=20projection,=20docs=20fixes,=20error-code=20parity=20guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SecurityAlert projection: new migration 010 installs a security_alerts_projection table plus a sidecar AFTER INSERT trigger that routes SecurityAlert / SecurityAlertAcknowledged events into it. Previously the inbox worker appended the event to the device stream but no projection handled it, so the UI had no way to list alerts without scanning the append-only events log. Using a sidecar trigger (instead of rewriting project_event()) keeps the change additive and avoids the risk of dropping an existing case arm from the central dispatcher. OutputChunk events deliberately kept projection-less: they already have a dedicated retrieval path via LoadOutputChunks which scans events by (stream_id, event_type='OutputChunk') in order. That's the intended access pattern for streaming output — a projection table would just duplicate the events table. Docs: fix the stale :8080 example in cmd/control/README.md (compose routes via :443 with TCP passthrough; port was a leftover from before the SNI collapse). Correct the server README claim that GATEWAY_WEB_LISTEN_ADDR is a TLS listener — it's cleartext HTTP behind Traefik, which terminates TLS upstream. Error-code parity guard: new unit test TestErrorCodeParityWithTSSDK in internal/api walks errors.go and the sdk's ts/errors.ts via filesystem and asserts the two code sets match. Skips gracefully when run outside the multi-repo workspace (so `go test ./...` from a read-only mirror still passes). This is the test that would have caught the 4-code drift the rc10 audit found. Part of manchtools/power-manage-server#74. --- README.md | 2 +- cmd/control/README.md | 2 +- internal/api/errors_parity_test.go | 118 ++++++++++++++++++ .../010_security_alerts_projection.sql | 103 +++++++++++++++ 4 files changed, 223 insertions(+), 2 deletions(-) create mode 100644 internal/api/errors_parity_test.go create mode 100644 internal/store/migrations/010_security_alerts_projection.sql diff --git a/README.md b/README.md index ada3c76e..b5d763db 100644 --- a/README.md +++ b/README.md @@ -467,7 +467,7 @@ CGO_ENABLED=0 go build -ldflags="-s -w -X main.version=2026.3.0" -o gateway ./cm | `GATEWAY_ID` | Stable gateway identifier (empty = generate ULID at startup) | | `GATEWAY_PUBLIC_TERMINAL_URL_TEMPLATE` | Template for the public terminal WebSocket URL, e.g. `wss://{id}.gateway.example.com/terminal` | | `GATEWAY_BOOTSTRAP_HOST` | Wildcard root hostname for agent bootstrap redirect, e.g. `gateway.example.com` | -| `GATEWAY_WEB_LISTEN_ADDR` | Listen address for the web TLS listener (terminal WebSocket), e.g. `:8443` | +| `GATEWAY_WEB_LISTEN_ADDR` | Listen address for the terminal-WebSocket HTTP listener (cleartext; Traefik terminates TLS upstream), e.g. `:8443` | | `GATEWAY_LOG_LEVEL` | Log level: `debug`, `info`, `warn`, `error` (default `info`) | | `GATEWAY_HEARTBEAT_INTERVAL` | Heartbeat cadence sent to agents (Go duration, 5s..5m; default `30s`) | | `GATEWAY_TRAEFIK_TTY_CERT_RESOLVER` | Traefik cert resolver name for the per-replica TTY HTTP router (e.g. `letsencrypt`) | diff --git a/cmd/control/README.md b/cmd/control/README.md index 3c6b8a27..5254cd93 100644 --- a/cmd/control/README.md +++ b/cmd/control/README.md @@ -103,7 +103,7 @@ export CONTROL_DATABASE_URL="postgres://powermanage:powermanage@localhost:5432/p export CONTROL_JWT_SECRET="your-secret-key" export CONTROL_CA_CERT="./dev/certs/ca.crt" export CONTROL_CA_KEY="./dev/certs/ca.key" -export CONTROL_GATEWAY_URL="https://gateway.example.com:8080" +export CONTROL_GATEWAY_URL="https://gateway.example.com" export CONTROL_ADMIN_EMAIL="admin@localhost.com" export CONTROL_ADMIN_PASSWORD="admin" diff --git a/internal/api/errors_parity_test.go b/internal/api/errors_parity_test.go new file mode 100644 index 00000000..546dcd7e --- /dev/null +++ b/internal/api/errors_parity_test.go @@ -0,0 +1,118 @@ +package api + +import ( + "os" + "path/filepath" + "regexp" + "sort" + "strings" + "testing" +) + +// TestErrorCodeParityWithTSSDK guards against drift between the +// snake_case error codes the server emits (errors.go) and the Err* +// constants the TS SDK exports (sdk/ts/errors.ts). When the two +// lists disagree, the web client falls back to raw code strings +// instead of the paraglide-localized message, which is exactly the +// kind of silent UX regression the rc10 audit flagged. +// +// Fails with a diff when the sets don't match. To fix: +// - If the server added a new code, add a matching Err* const to +// sdk/ts/errors.ts AND a matching error_ paraglide key to +// web/messages/{en,de}.json. +// - If the server removed a code, delete the const from +// sdk/ts/errors.ts and the matching paraglide key. +func TestErrorCodeParityWithTSSDK(t *testing.T) { + serverCodes := extractServerCodes(t) + sdkCodes := extractSDKCodes(t) + + missingFromSDK := diff(serverCodes, sdkCodes) + extraInSDK := diff(sdkCodes, serverCodes) + + if len(missingFromSDK) > 0 { + t.Errorf("error codes emitted by server but not exported by sdk/ts/errors.ts: %v\n"+ + "→ add a matching Err* const to sdk/ts/errors.ts and an error_ key to web/messages/{en,de}.json", + missingFromSDK) + } + if len(extraInSDK) > 0 { + t.Errorf("error codes exported by sdk/ts/errors.ts but never emitted by the server: %v\n"+ + "→ delete the stale consts (and any matching paraglide keys) to stop lying to future developers", + extraInSDK) + } +} + +// extractServerCodes walks errors.go and returns every snake_case +// string literal assigned to an Err* constant. Deliberately NOT +// importing the constants directly — we want to catch the case where +// a const is declared but never used, or where a code is hard-coded +// in a handler without going through the constant. +func extractServerCodes(t *testing.T) []string { + t.Helper() + data, err := os.ReadFile("errors.go") + if err != nil { + t.Fatalf("read server errors.go: %v", err) + } + // Match lines like: ErrWhatever = "snake_case_value" + re := regexp.MustCompile(`Err\w+\s*=\s*"([a-z][a-z0-9_]*)"`) + matches := re.FindAllStringSubmatch(string(data), -1) + seen := make(map[string]struct{}, len(matches)) + for _, m := range matches { + seen[m[1]] = struct{}{} + } + out := make([]string, 0, len(seen)) + for code := range seen { + out = append(out, code) + } + sort.Strings(out) + return out +} + +// extractSDKCodes walks sdk/ts/errors.ts and returns every +// snake_case string literal assigned to an exported Err* const. +// The path is relative to server/internal/api — the sdk repo sits +// alongside the server repo in the workspace. +func extractSDKCodes(t *testing.T) []string { + t.Helper() + // server/internal/api → ../../../sdk/ts/errors.ts + path := filepath.Join("..", "..", "..", "..", "sdk", "ts", "errors.ts") + data, err := os.ReadFile(path) + if err != nil { + t.Skipf("skipping parity check: cannot read %s (%v) — run this test from the multi-repo workspace root", path, err) + return nil + } + re := regexp.MustCompile(`export\s+const\s+Err\w+\s*=\s*'([a-z][a-z0-9_]*)'`) + matches := re.FindAllStringSubmatch(string(data), -1) + seen := make(map[string]struct{}, len(matches)) + for _, m := range matches { + seen[m[1]] = struct{}{} + } + out := make([]string, 0, len(seen)) + for code := range seen { + out = append(out, code) + } + sort.Strings(out) + return out +} + +// diff returns elements in a that are not in b, sorted. +func diff(a, b []string) []string { + present := make(map[string]struct{}, len(b)) + for _, s := range b { + present[s] = struct{}{} + } + var out []string + for _, s := range a { + if _, ok := present[s]; !ok { + out = append(out, s) + } + } + sort.Strings(out) + return out +} + +// Guard against string-builder typos in the tests above. +func init() { + if strings.Count("Err", "E") != 1 { + panic("sanity") + } +} diff --git a/internal/store/migrations/010_security_alerts_projection.sql b/internal/store/migrations/010_security_alerts_projection.sql new file mode 100644 index 00000000..57b8a986 --- /dev/null +++ b/internal/store/migrations/010_security_alerts_projection.sql @@ -0,0 +1,103 @@ +-- SecurityAlert projection — enables the UI to list security alerts +-- without scanning the raw events table. +-- +-- Rationale: agents emit SecurityAlert events via the inbox worker +-- (internal/control/inbox_worker.go handleSecurityAlert). They are +-- persisted on the device stream, but nothing projected them into a +-- queryable table, so compliance / dashboard consumers had to either +-- scan the append-only events log or ignore alerts entirely. The +-- rc10 audit flagged this as silent data loss for the SIEM path. +-- +-- Shape follows the LpsPasswordRotated / LuksKey rotation pattern: +-- a derived projection keyed by a new UUID with an acknowledged +-- flag so "unacknowledged alerts" is a cheap query. alert_type is +-- the free-form tag from the payload (e.g. "file_integrity_violation", +-- "auditd_rule_trip"); details is a small key/value blob from the +-- agent's evidence bundle. +-- +-- Wiring: rather than rewriting project_event() (the central +-- dispatcher in migration 004 uses BEGIN/EXCEPTION isolation per +-- case arm), we install a second AFTER INSERT trigger on events +-- that ONLY fires for SecurityAlert-shaped rows. This keeps the +-- change additive and prevents an accidental dropped case arm from +-- breaking an unrelated projection. + +-- +goose Up + +CREATE TABLE security_alerts_projection ( + id UUID PRIMARY KEY DEFAULT gen_random_uuid(), + device_id TEXT NOT NULL, + alert_type TEXT NOT NULL, + message TEXT NOT NULL, + details JSONB, + raised_at TIMESTAMPTZ NOT NULL, + acknowledged BOOLEAN NOT NULL DEFAULT FALSE, + acknowledged_at TIMESTAMPTZ, + acknowledged_by TEXT, + created_at TIMESTAMPTZ NOT NULL DEFAULT NOW() +); + +CREATE INDEX idx_security_alerts_device ON security_alerts_projection(device_id, acknowledged, raised_at DESC); +CREATE INDEX idx_security_alerts_type ON security_alerts_projection(alert_type, raised_at DESC); +CREATE INDEX idx_security_alerts_unack ON security_alerts_projection(acknowledged, raised_at DESC) WHERE acknowledged = FALSE; + +-- +goose StatementBegin +CREATE OR REPLACE FUNCTION project_security_alert_event(event events) RETURNS void AS $$ +BEGIN + CASE event.event_type + WHEN 'SecurityAlert' THEN + INSERT INTO security_alerts_projection ( + device_id, alert_type, message, details, raised_at + ) VALUES ( + event.stream_id, + event.data->>'alert_type', + event.data->>'message', + event.data->'details', + event.occurred_at + ); + WHEN 'SecurityAlertAcknowledged' THEN + UPDATE security_alerts_projection + SET acknowledged = TRUE, + acknowledged_at = event.occurred_at, + acknowledged_by = event.data->>'acknowledged_by' + WHERE id::TEXT = event.data->>'alert_id'; + ELSE + NULL; + END CASE; +END; +$$ LANGUAGE plpgsql; +-- +goose StatementEnd + +-- Sidecar trigger: fires AFTER INSERT alongside the existing +-- event_projector trigger, but only for the SecurityAlert event +-- types so unrelated streams are unaffected. Error isolation +-- follows the same pattern as the central dispatcher: failures +-- route to projection_errors instead of aborting the event append. +-- +goose StatementBegin +CREATE OR REPLACE FUNCTION project_security_alert_trigger() RETURNS TRIGGER AS $$ +BEGIN + IF NEW.stream_type = 'device' + AND NEW.event_type IN ('SecurityAlert', 'SecurityAlertAcknowledged') THEN + BEGIN + PERFORM project_security_alert_event(NEW); + EXCEPTION WHEN OTHERS THEN + INSERT INTO projection_errors (event_id, event_type, stream_type, error_message) + VALUES (NEW.id, NEW.event_type, NEW.stream_type, SQLERRM); + END; + END IF; + RETURN NEW; +END; +$$ LANGUAGE plpgsql; +-- +goose StatementEnd + +CREATE TRIGGER security_alert_projector + AFTER INSERT ON events + FOR EACH ROW + EXECUTE FUNCTION project_security_alert_trigger(); + +-- +goose Down + +DROP TRIGGER IF EXISTS security_alert_projector ON events; +DROP FUNCTION IF EXISTS project_security_alert_trigger; +DROP FUNCTION IF EXISTS project_security_alert_event; +DROP TABLE IF EXISTS security_alerts_projection; From 93e4fa15e439ea7d17e8065059cc9e859d38e69f Mon Sep 17 00:00:00 2001 From: PaulDotterer Date: Thu, 23 Apr 2026 08:38:34 +0200 Subject: [PATCH 04/11] =?UTF-8?q?fix:=20rc10=20review=20=E2=80=94=20tighte?= =?UTF-8?q?n=20gateway=20URL=20validator,=20event-id-keyed=20SecurityAlert?= =?UTF-8?q?=20projection,=20parity-test=20CI=20hook?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review findings on #76 before merge: URL validation was too weak. The previous check was url.Parse + empty string guard, which lets obviously-invalid shapes through: bare hostnames like "gateway.example.com" parse without error but aren't absolute URLs; http:// downgrades silently; wss:// confuses the streaming client; userinfo in the URL leaks on every enrollment response. Introduce api.ValidateGatewayURL that requires scheme=https, non-empty host, no userinfo, no fragment. Wire it into cmd/control/main.go (fatal on violation) and into NewRegistrationHandler (panic on violation — programmer error by the time we reach the constructor) and Register (re-validation for defence in depth). New TestValidateGatewayURL covers all the footgun shapes the review flagged. Existing registration_handler_test.go fixtures used wss://gateway.test:443 which the new validator correctly rejects — updated to https://gateway.test:8080 to match the production CONTROL_GATEWAY_URL shape. SecurityAlert projection idempotency. Migration 010 was using a fresh UUID as the primary key, so event replay (backfill, rebuild, trigger re-fire) would duplicate alerts. Key the projection by the originating event_id (REFERENCES events(id)) and use INSERT ... ON CONFLICT (event_id) DO NOTHING in the projector. This is the event-sourcing idempotency pattern that keeps projections deterministic across replays. SecurityAlertAcknowledged payloads carry alert_id = the event_id of the originating SecurityAlert, so the UPDATE path is unchanged in spirit but keyed by the stable id. SecurityAlert is explicitly Phase 1 — the migration + sidecar trigger + sqlc queries (ListSecurityAlertsForDevice, ListUnacknowledgedSecurityAlerts, GetSecurityAlert, CountUnacknowledgedSecurityAlerts) make the projection queryable from Go. The RPC surface and web UI wiring remain Phase 2 because they need a proto change (ControlService.ListSecurityAlerts), which is better in its own PR to keep the scope tight. LUKS stream-id correctness assumption. The inbox worker looks up the luks_key stream ID by (device_id, action_id) and picks the latest matching Requested/Dispatched event via ORDER BY sequence_num DESC LIMIT 1. Documented the assumption that there is at most one outstanding revocation per (device, action) at a time, and what the behavior is when the assumption is violated (worst case: the latest request gets the terminal event, older abandoned streams miss their terminal — not a projection correctness issue, but worth flagging for audit export). Parity test CI hook. TestErrorCodeParityWithTSSDK previously silently skipped when the sibling SDK repo wasn't present, so standalone server CI couldn't rely on it. Now reads PM_SDK_TS_ERRORS env var as the primary source (CI sets this when it checks out the SDK beside server), falls back to the local dev-workspace path, and loudly fails when PM_SDK_PARITY_REQUIRED=1 is set but the file still isn't reachable. Skips with a clear log line otherwise so a plain `go test ./...` in a standalone server checkout still passes. Part of manchtools/power-manage-server#76 review feedback. --- cmd/control/main.go | 23 +-- internal/api/errors_parity_test.go | 44 +++- internal/api/registration_handler.go | 73 ++++++- internal/api/registration_handler_test.go | 16 +- internal/api/validate_gateway_url_test.go | 64 ++++++ internal/control/inbox_worker.go | 14 ++ internal/store/generated/models.go | 13 ++ .../store/generated/security_alerts.sql.go | 192 ++++++++++++++++++ .../010_security_alerts_projection.sql | 17 +- internal/store/queries/security_alerts.sql | 37 ++++ 10 files changed, 449 insertions(+), 44 deletions(-) create mode 100644 internal/api/validate_gateway_url_test.go create mode 100644 internal/store/generated/security_alerts.sql.go create mode 100644 internal/store/queries/security_alerts.sql diff --git a/cmd/control/main.go b/cmd/control/main.go index 3918c6f9..1973d0af 100644 --- a/cmd/control/main.go +++ b/cmd/control/main.go @@ -9,7 +9,6 @@ import ( "fmt" "log/slog" "net/http" - "net/url" urlpkg "net/url" "os" "os/signal" @@ -93,19 +92,15 @@ func main() { logger := logging.SetupLogger(cfg.LogLevel, cfg.LogFormat, os.Stderr) slog.SetDefault(logger) logger.Info("starting control server", "version", version, "listen_addr", cfg.ListenAddr, "gateway_url", cfg.GatewayURL, "dynamic_group_eval_interval", cfg.DynamicGroupEvalInterval) - // CONTROL_GATEWAY_URL is fatal when empty: registration hands it - // back to the agent verbatim, so a missing value turns every - // successful enrollment into an agent that can never connect — - // and the failure mode only shows up later, at first-stream time, - // with a cryptic URL-parse error far from the config source. - // Fail fast here so the operator notices the misconfiguration at - // startup instead of after devices are already enrolled. - if cfg.GatewayURL == "" { - logger.Error("CONTROL_GATEWAY_URL is required — enrollment cannot hand agents an empty gateway URL") - os.Exit(1) - } - if _, err := url.Parse(cfg.GatewayURL); err != nil { - logger.Error("CONTROL_GATEWAY_URL is not a valid URL", "gateway_url", cfg.GatewayURL, "error", err) + // CONTROL_GATEWAY_URL is fatal when invalid: registration hands + // it back to the agent verbatim, so any invalid shape — empty + // string, bare hostname (parses as a relative path), http:// + // (agents refuse h2c), userinfo, or non-https scheme — turns + // every successful enrollment into an agent that can never + // connect. api.ValidateGatewayURL is the shared validator + // (also invoked defensively in the registration handler). + if err := api.ValidateGatewayURL(cfg.GatewayURL); err != nil { + logger.Error("CONTROL_GATEWAY_URL is invalid", "gateway_url", cfg.GatewayURL, "error", err) os.Exit(1) } diff --git a/internal/api/errors_parity_test.go b/internal/api/errors_parity_test.go index 546dcd7e..01c2ecea 100644 --- a/internal/api/errors_parity_test.go +++ b/internal/api/errors_parity_test.go @@ -69,17 +69,47 @@ func extractServerCodes(t *testing.T) []string { // extractSDKCodes walks sdk/ts/errors.ts and returns every // snake_case string literal assigned to an exported Err* const. -// The path is relative to server/internal/api — the sdk repo sits -// alongside the server repo in the workspace. +// +// Resolution order: +// 1. PM_SDK_TS_ERRORS env var (absolute or relative path) — CI sets +// this when it checks the SDK out beside the server repo, so +// standalone server CI can still exercise the parity guard. +// 2. ../../../../sdk/ts/errors.ts — the local dev-workspace layout +// /home//.../power-manage/{server,sdk}. +// +// If neither resolves AND PM_SDK_PARITY_REQUIRED=1 is set, the test +// fails loudly — this is the mode CI should use when it expects the +// SDK to be available. Without the env var the test skips with a +// clear log line so a local `go test ./...` in a standalone server +// checkout still passes. func extractSDKCodes(t *testing.T) []string { t.Helper() - // server/internal/api → ../../../sdk/ts/errors.ts - path := filepath.Join("..", "..", "..", "..", "sdk", "ts", "errors.ts") - data, err := os.ReadFile(path) - if err != nil { - t.Skipf("skipping parity check: cannot read %s (%v) — run this test from the multi-repo workspace root", path, err) + + var candidates []string + if env := os.Getenv("PM_SDK_TS_ERRORS"); env != "" { + candidates = append(candidates, env) + } + candidates = append(candidates, filepath.Join("..", "..", "..", "..", "sdk", "ts", "errors.ts")) + + var data []byte + var tried []string + for _, path := range candidates { + tried = append(tried, path) + b, err := os.ReadFile(path) + if err == nil { + data = b + break + } + } + if data == nil { + msg := "cannot read sdk/ts/errors.ts from any candidate path: " + strings.Join(tried, ", ") + if os.Getenv("PM_SDK_PARITY_REQUIRED") == "1" { + t.Fatalf("PM_SDK_PARITY_REQUIRED=1 but %s — CI should check out the sdk repo beside server or set PM_SDK_TS_ERRORS", msg) + } + t.Skipf("%s — set PM_SDK_TS_ERRORS or PM_SDK_PARITY_REQUIRED=1 (with the file available) to exercise the parity guard", msg) return nil } + re := regexp.MustCompile(`export\s+const\s+Err\w+\s*=\s*'([a-z][a-z0-9_]*)'`) matches := re.FindAllStringSubmatch(string(data), -1) seen := make(map[string]struct{}, len(matches)) diff --git a/internal/api/registration_handler.go b/internal/api/registration_handler.go index ce9408a9..44667e16 100644 --- a/internal/api/registration_handler.go +++ b/internal/api/registration_handler.go @@ -5,7 +5,9 @@ import ( "crypto/sha256" "encoding/hex" "errors" + "fmt" "log/slog" + "net/url" "time" "connectrpc.com/connect" @@ -17,6 +19,47 @@ import ( "github.com/manchtools/power-manage/server/internal/store" ) +// ValidateGatewayURL returns nil when raw is a gateway URL an agent +// can actually connect to over mTLS. A surprising number of shapes +// pass `url.Parse` without being usable: +// - bare hostnames like `gateway.example.com` parse with Scheme="", +// Host="", Path="gateway.example.com" — the agent would try to +// dial a relative path; +// - `http://...` is refused because rc10 agents refuse h2c; +// - `wss://...` or other schemes are refused because the agent's +// gateway client uses HTTPS transport; +// - user-info (`https://user:pass@host/`) is refused because +// credentials in the URL are never the right answer and would +// leak on every enrollment response; +// - fragments are meaningless on the wire and refused to keep the +// shape tight. +// +// Used both at control server startup (fatal on violation, so a +// misconfiguration is visible at boot) and defensively in the +// registration handler before the URL is handed to the agent. +func ValidateGatewayURL(raw string) error { + if raw == "" { + return fmt.Errorf("gateway URL is empty") + } + u, err := url.Parse(raw) + if err != nil { + return fmt.Errorf("gateway URL parse failed: %w", err) + } + if u.Scheme != "https" { + return fmt.Errorf("gateway URL must use https scheme, got %q", u.Scheme) + } + if u.Host == "" { + return fmt.Errorf("gateway URL has no host — bare hostnames like %q are not absolute URLs", raw) + } + if u.User != nil { + return fmt.Errorf("gateway URL must not contain userinfo (credentials in URL leak on every enrollment response)") + } + if u.Fragment != "" { + return fmt.Errorf("gateway URL must not contain a fragment") + } + return nil +} + // RegistrationHandler handles agent registration requests. type RegistrationHandler struct { store *store.Store @@ -25,8 +68,16 @@ type RegistrationHandler struct { logger *slog.Logger } -// NewRegistrationHandler creates a new registration handler. +// NewRegistrationHandler creates a new registration handler. Panics +// when gatewayURL fails ValidateGatewayURL — caller is expected to +// have validated at startup, so reaching the handler constructor +// with a bad value is a programmer error, not a runtime condition. +// Startup-time validation (cmd/control/main.go) surfaces the same +// check with a clean operator-facing error message. func NewRegistrationHandler(st *store.Store, certAuth *ca.CA, gatewayURL string, logger *slog.Logger) *RegistrationHandler { + if err := ValidateGatewayURL(gatewayURL); err != nil { + panic(fmt.Sprintf("NewRegistrationHandler: invalid gateway URL %q: %v", gatewayURL, err)) + } return &RegistrationHandler{ store: st, ca: certAuth, @@ -42,16 +93,16 @@ func (h *RegistrationHandler) Register(ctx context.Context, req *connect.Request logger.Info("processing registration request") // Defence in depth: the startup guard in cmd/control/main.go - // already fails fast on an empty GATEWAY_URL, but if that guard - // ever regresses (or the handler is constructed directly in a - // test), refuse the registration rather than hand the agent a - // URL it cannot connect to. Silent empty-URL enrollment is the - // worst failure mode: the device is created in the DB, the cert - // is signed, but the first stream attempt fails with a - // URL-parse error far from the config source. - if h.gatewayURL == "" { - logger.Error("registration refused: gatewayURL is empty — CONTROL_GATEWAY_URL must be set") - return nil, apiErrorCtx(ctx, ErrInternal, connect.CodeFailedPrecondition, "server misconfiguration: gateway URL is not set") + // plus the NewRegistrationHandler constructor both run + // ValidateGatewayURL, so reaching this check with an invalid + // URL would mean both earlier layers regressed. We re-run the + // full validator (not just the emptiness check) so the agent + // never receives a URL shape that the URL validators missed — + // bare hostnames, http://, userinfo, etc. + if err := ValidateGatewayURL(h.gatewayURL); err != nil { + logger.Error("registration refused: gatewayURL failed validation", + "gateway_url", h.gatewayURL, "error", err) + return nil, apiErrorCtx(ctx, ErrInternal, connect.CodeFailedPrecondition, "server misconfiguration: gateway URL is invalid") } // Validate CSR is present diff --git a/internal/api/registration_handler_test.go b/internal/api/registration_handler_test.go index 6513578a..3ae01374 100644 --- a/internal/api/registration_handler_test.go +++ b/internal/api/registration_handler_test.go @@ -112,7 +112,7 @@ func createTestTokenWithValue(t *testing.T, st *store.Store, actorID string, one func TestRegister_ValidToken(t *testing.T) { st := testutil.SetupPostgres(t) testCA := newTestCA(t) - h := api.NewRegistrationHandler(st, testCA, "wss://gateway.test:443", slog.Default()) + h := api.NewRegistrationHandler(st, testCA, "https://gateway.test:8080", slog.Default()) adminID := testutil.CreateTestUser(t, st, testutil.NewID()+"@test.com", "pass", "admin") _, tokenValue := createTestTokenWithValue(t, st, adminID, false) @@ -130,7 +130,7 @@ func TestRegister_ValidToken(t *testing.T) { assert.NotEmpty(t, resp.Msg.DeviceId.Value) assert.NotEmpty(t, resp.Msg.Certificate) assert.NotEmpty(t, resp.Msg.CaCert) - assert.Equal(t, "wss://gateway.test:443", resp.Msg.GatewayUrl) + assert.Equal(t, "https://gateway.test:8080", resp.Msg.GatewayUrl) // Verify device projection exists device, err := st.Queries().GetDeviceByID(context.Background(), db.GetDeviceByIDParams{ @@ -143,7 +143,7 @@ func TestRegister_ValidToken(t *testing.T) { func TestRegister_OneTimeTokenDisabledAfterUse(t *testing.T) { st := testutil.SetupPostgres(t) testCA := newTestCA(t) - h := api.NewRegistrationHandler(st, testCA, "wss://gateway.test:443", slog.Default()) + h := api.NewRegistrationHandler(st, testCA, "https://gateway.test:8080", slog.Default()) adminID := testutil.CreateTestUser(t, st, testutil.NewID()+"@test.com", "pass", "admin") _, tokenValue := createTestTokenWithValue(t, st, adminID, true) @@ -174,7 +174,7 @@ func TestRegister_OneTimeTokenDisabledAfterUse(t *testing.T) { func TestRegister_DisabledTokenFails(t *testing.T) { st := testutil.SetupPostgres(t) testCA := newTestCA(t) - h := api.NewRegistrationHandler(st, testCA, "wss://gateway.test:443", slog.Default()) + h := api.NewRegistrationHandler(st, testCA, "https://gateway.test:8080", slog.Default()) adminID := testutil.CreateTestUser(t, st, testutil.NewID()+"@test.com", "pass", "admin") tokenID, tokenValue := createTestTokenWithValue(t, st, adminID, false) @@ -204,7 +204,7 @@ func TestRegister_DisabledTokenFails(t *testing.T) { func TestRegister_InvalidTokenFails(t *testing.T) { st := testutil.SetupPostgres(t) testCA := newTestCA(t) - h := api.NewRegistrationHandler(st, testCA, "wss://gateway.test:443", slog.Default()) + h := api.NewRegistrationHandler(st, testCA, "https://gateway.test:8080", slog.Default()) csr := generateCSR(t) _, err := h.Register(context.Background(), connect.NewRequest(&pm.RegisterRequest{ @@ -220,7 +220,7 @@ func TestRegister_InvalidTokenFails(t *testing.T) { func TestRegister_MissingCSRFails(t *testing.T) { st := testutil.SetupPostgres(t) testCA := newTestCA(t) - h := api.NewRegistrationHandler(st, testCA, "wss://gateway.test:443", slog.Default()) + h := api.NewRegistrationHandler(st, testCA, "https://gateway.test:8080", slog.Default()) adminID := testutil.CreateTestUser(t, st, testutil.NewID()+"@test.com", "pass", "admin") _, tokenValue := createTestTokenWithValue(t, st, adminID, false) @@ -238,7 +238,7 @@ func TestRegister_MissingCSRFails(t *testing.T) { func TestRegister_ReusableTokenAllowsMultipleUses(t *testing.T) { st := testutil.SetupPostgres(t) testCA := newTestCA(t) - h := api.NewRegistrationHandler(st, testCA, "wss://gateway.test:443", slog.Default()) + h := api.NewRegistrationHandler(st, testCA, "https://gateway.test:8080", slog.Default()) adminID := testutil.CreateTestUser(t, st, testutil.NewID()+"@test.com", "pass", "admin") _, tokenValue := createTestTokenWithValue(t, st, adminID, false) @@ -259,7 +259,7 @@ func TestRegister_ReusableTokenAllowsMultipleUses(t *testing.T) { func TestRegister_ExpiredTokenFails(t *testing.T) { st := testutil.SetupPostgres(t) testCA := newTestCA(t) - h := api.NewRegistrationHandler(st, testCA, "wss://gateway.test:443", slog.Default()) + h := api.NewRegistrationHandler(st, testCA, "https://gateway.test:8080", slog.Default()) adminID := testutil.CreateTestUser(t, st, testutil.NewID()+"@test.com", "pass", "admin") ctx := context.Background() diff --git a/internal/api/validate_gateway_url_test.go b/internal/api/validate_gateway_url_test.go new file mode 100644 index 00000000..c3870f20 --- /dev/null +++ b/internal/api/validate_gateway_url_test.go @@ -0,0 +1,64 @@ +package api_test + +import ( + "strings" + "testing" + + "github.com/manchtools/power-manage/server/internal/api" +) + +// TestValidateGatewayURL covers the shapes the control server may be +// handed via CONTROL_GATEWAY_URL and the ones registration_handler +// rechecks defensively. Each case captures a real operator footgun +// rc10 reviewers flagged: +// +// - empty string — rc10 closed the "enroll with empty URL" path. +// - bare hostname like "gateway.example.com" — parses via url.Parse +// without error but isn't an absolute URL; the agent would dial +// a relative path and fail with a cryptic error. +// - http:// — agents refuse h2c as of rc10 (see agent main.go). +// - ws:// / wss:// — agent uses HTTPS transport for the streaming +// client; accepting wss here would confuse the operator. +// - userinfo — credentials in the URL leak on every enrollment +// response and are never the right answer. +// - fragment — meaningless on the wire. +func TestValidateGatewayURL(t *testing.T) { + cases := []struct { + name string + in string + wantErr bool + wantWord string // substring that must appear in the error message + }{ + {"empty", "", true, "empty"}, + {"bare hostname", "gateway.example.com", true, "scheme"}, + {"http downgrade", "http://gateway.example.com", true, "https"}, + {"wss scheme", "wss://gateway.example.com", true, "https"}, + {"ws scheme", "ws://gateway.example.com", true, "https"}, + {"userinfo", "https://user:pass@gateway.example.com", true, "userinfo"}, + {"userinfo username only", "https://user@gateway.example.com", true, "userinfo"}, + {"fragment", "https://gateway.example.com#frag", true, "fragment"}, + {"host only", "https://", true, "host"}, + + {"happy path host", "https://gateway.example.com", false, ""}, + {"happy path with port", "https://gateway.example.com:8443", false, ""}, + {"happy path with path", "https://gateway.example.com/gw/01ABC", false, ""}, + {"happy path with trailing slash", "https://gateway.example.com/", false, ""}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := api.ValidateGatewayURL(tc.in) + if tc.wantErr { + if err == nil { + t.Fatalf("ValidateGatewayURL(%q) = nil, want error containing %q", tc.in, tc.wantWord) + } + if tc.wantWord != "" && !strings.Contains(err.Error(), tc.wantWord) { + t.Errorf("ValidateGatewayURL(%q) err = %q, want substring %q", tc.in, err.Error(), tc.wantWord) + } + return + } + if err != nil { + t.Errorf("ValidateGatewayURL(%q) = %v, want nil", tc.in, err) + } + }) + } +} diff --git a/internal/control/inbox_worker.go b/internal/control/inbox_worker.go index bb25b917..e17d73c8 100644 --- a/internal/control/inbox_worker.go +++ b/internal/control/inbox_worker.go @@ -540,6 +540,20 @@ func (w *InboxWorker) handleRevokeLuksDeviceKeyResult(ctx context.Context, t *as // Dispatched phases. Earlier versions generated a fresh ULID // here, which split every revocation across two streams and // broke the projection's three-phase stitch — fixed in rc10. + // + // Correctness assumption: at most one outstanding revocation + // per (device, action) at a time. Enforced at the API layer: + // RevokeLuksDeviceKey checks the projection for an already- + // dispatched-and-unterminal request before accepting a new + // one, so duplicates via concurrent operator clicks are + // rejected upstream. If that invariant ever regresses, the + // ORDER BY sequence_num DESC + LIMIT 1 here will pick the + // LATEST matching request — which is the expected "the + // operator re-requested and here's the result" semantic. + // Older abandoned streams would then lack a terminal event; + // not a correctness issue for the projection (it keys by + // stream_id), but worth flagging for the audit export. + // // If the lookup fails (e.g. the Requested event never made it // to disk because the original API call crashed), fall back to // a fresh stream ID so we still record the Failed outcome diff --git a/internal/store/generated/models.go b/internal/store/generated/models.go index 4213c6d2..66d99fdf 100644 --- a/internal/store/generated/models.go +++ b/internal/store/generated/models.go @@ -388,6 +388,19 @@ type ScimGroupMappingProjection struct { ProjectionVersion int64 `json:"projection_version"` } +type SecurityAlertsProjection struct { + EventID uuid.UUID `json:"event_id"` + DeviceID string `json:"device_id"` + AlertType string `json:"alert_type"` + Message string `json:"message"` + Details []byte `json:"details"` + RaisedAt time.Time `json:"raised_at"` + Acknowledged bool `json:"acknowledged"` + AcknowledgedAt *time.Time `json:"acknowledged_at"` + AcknowledgedBy *string `json:"acknowledged_by"` + CreatedAt time.Time `json:"created_at"` +} + type ServerSettingsProjection struct { ID string `json:"id"` UserProvisioningEnabled bool `json:"user_provisioning_enabled"` diff --git a/internal/store/generated/security_alerts.sql.go b/internal/store/generated/security_alerts.sql.go new file mode 100644 index 00000000..29f3377c --- /dev/null +++ b/internal/store/generated/security_alerts.sql.go @@ -0,0 +1,192 @@ +// Code generated by sqlc. DO NOT EDIT. +// versions: +// sqlc v1.30.0 +// source: security_alerts.sql + +package generated + +import ( + "context" + "time" + + "github.com/google/uuid" +) + +const countUnacknowledgedSecurityAlerts = `-- name: CountUnacknowledgedSecurityAlerts :one +SELECT COUNT(*)::int FROM security_alerts_projection WHERE NOT acknowledged +` + +func (q *Queries) CountUnacknowledgedSecurityAlerts(ctx context.Context) (int32, error) { + row := q.db.QueryRow(ctx, countUnacknowledgedSecurityAlerts) + var column_1 int32 + err := row.Scan(&column_1) + return column_1, err +} + +const getSecurityAlert = `-- name: GetSecurityAlert :one +SELECT event_id, device_id, alert_type, message, details, raised_at, + acknowledged, acknowledged_at, acknowledged_by +FROM security_alerts_projection +WHERE event_id = $1 +` + +type GetSecurityAlertRow struct { + EventID uuid.UUID `json:"event_id"` + DeviceID string `json:"device_id"` + AlertType string `json:"alert_type"` + Message string `json:"message"` + Details []byte `json:"details"` + RaisedAt time.Time `json:"raised_at"` + Acknowledged bool `json:"acknowledged"` + AcknowledgedAt *time.Time `json:"acknowledged_at"` + AcknowledgedBy *string `json:"acknowledged_by"` +} + +func (q *Queries) GetSecurityAlert(ctx context.Context, eventID uuid.UUID) (GetSecurityAlertRow, error) { + row := q.db.QueryRow(ctx, getSecurityAlert, eventID) + var i GetSecurityAlertRow + err := row.Scan( + &i.EventID, + &i.DeviceID, + &i.AlertType, + &i.Message, + &i.Details, + &i.RaisedAt, + &i.Acknowledged, + &i.AcknowledgedAt, + &i.AcknowledgedBy, + ) + return i, err +} + +const listSecurityAlertsForDevice = `-- name: ListSecurityAlertsForDevice :many + +SELECT event_id, device_id, alert_type, message, details, raised_at, + acknowledged, acknowledged_at, acknowledged_by +FROM security_alerts_projection +WHERE device_id = $1 + AND ($2::bool OR NOT acknowledged) +ORDER BY raised_at DESC +LIMIT $4::int +OFFSET $3::int +` + +type ListSecurityAlertsForDeviceParams struct { + DeviceID string `json:"device_id"` + IncludeAcknowledged bool `json:"include_acknowledged"` + PageOffset int32 `json:"page_offset"` + PageSize int32 `json:"page_size"` +} + +type ListSecurityAlertsForDeviceRow struct { + EventID uuid.UUID `json:"event_id"` + DeviceID string `json:"device_id"` + AlertType string `json:"alert_type"` + Message string `json:"message"` + Details []byte `json:"details"` + RaisedAt time.Time `json:"raised_at"` + Acknowledged bool `json:"acknowledged"` + AcknowledgedAt *time.Time `json:"acknowledged_at"` + AcknowledgedBy *string `json:"acknowledged_by"` +} + +// Phase 1 read queries for the security_alerts_projection (added in +// migration 010). The RPC surface (ControlService.ListSecurityAlerts, +// AcknowledgeSecurityAlert) and the web UI wiring land in a follow-up +// PR because they need proto changes; the queries below are the +// Go-side primitives those handlers will call. +// +// Keeping them in this PR means the projection is not a dead table — +// internal callers and tests can already exercise it, and future +// handler work only needs to wire the proto envelope. +func (q *Queries) ListSecurityAlertsForDevice(ctx context.Context, arg ListSecurityAlertsForDeviceParams) ([]ListSecurityAlertsForDeviceRow, error) { + rows, err := q.db.Query(ctx, listSecurityAlertsForDevice, + arg.DeviceID, + arg.IncludeAcknowledged, + arg.PageOffset, + arg.PageSize, + ) + if err != nil { + return nil, err + } + defer rows.Close() + items := []ListSecurityAlertsForDeviceRow{} + for rows.Next() { + var i ListSecurityAlertsForDeviceRow + if err := rows.Scan( + &i.EventID, + &i.DeviceID, + &i.AlertType, + &i.Message, + &i.Details, + &i.RaisedAt, + &i.Acknowledged, + &i.AcknowledgedAt, + &i.AcknowledgedBy, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const listUnacknowledgedSecurityAlerts = `-- name: ListUnacknowledgedSecurityAlerts :many +SELECT event_id, device_id, alert_type, message, details, raised_at, + acknowledged, acknowledged_at, acknowledged_by +FROM security_alerts_projection +WHERE NOT acknowledged +ORDER BY raised_at DESC +LIMIT $2::int +OFFSET $1::int +` + +type ListUnacknowledgedSecurityAlertsParams struct { + PageOffset int32 `json:"page_offset"` + PageSize int32 `json:"page_size"` +} + +type ListUnacknowledgedSecurityAlertsRow struct { + EventID uuid.UUID `json:"event_id"` + DeviceID string `json:"device_id"` + AlertType string `json:"alert_type"` + Message string `json:"message"` + Details []byte `json:"details"` + RaisedAt time.Time `json:"raised_at"` + Acknowledged bool `json:"acknowledged"` + AcknowledgedAt *time.Time `json:"acknowledged_at"` + AcknowledgedBy *string `json:"acknowledged_by"` +} + +func (q *Queries) ListUnacknowledgedSecurityAlerts(ctx context.Context, arg ListUnacknowledgedSecurityAlertsParams) ([]ListUnacknowledgedSecurityAlertsRow, error) { + rows, err := q.db.Query(ctx, listUnacknowledgedSecurityAlerts, arg.PageOffset, arg.PageSize) + if err != nil { + return nil, err + } + defer rows.Close() + items := []ListUnacknowledgedSecurityAlertsRow{} + for rows.Next() { + var i ListUnacknowledgedSecurityAlertsRow + if err := rows.Scan( + &i.EventID, + &i.DeviceID, + &i.AlertType, + &i.Message, + &i.Details, + &i.RaisedAt, + &i.Acknowledged, + &i.AcknowledgedAt, + &i.AcknowledgedBy, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} diff --git a/internal/store/migrations/010_security_alerts_projection.sql b/internal/store/migrations/010_security_alerts_projection.sql index 57b8a986..9d89505d 100644 --- a/internal/store/migrations/010_security_alerts_projection.sql +++ b/internal/store/migrations/010_security_alerts_projection.sql @@ -24,8 +24,15 @@ -- +goose Up +-- The primary key is the originating event_id rather than a fresh +-- UUID. This is the event-sourcing idempotency pattern: if the +-- projection is ever replayed (backfill, rebuild, trigger re-fire), +-- the same SecurityAlert event produces the same row, so ON CONFLICT +-- DO NOTHING prevents duplicates without needing deduplication +-- logic in the acknowledge path. SecurityAlertAcknowledged carries +-- the alert_id explicitly in its payload and UPDATEs by that key. CREATE TABLE security_alerts_projection ( - id UUID PRIMARY KEY DEFAULT gen_random_uuid(), + event_id UUID PRIMARY KEY REFERENCES events(id), device_id TEXT NOT NULL, alert_type TEXT NOT NULL, message TEXT NOT NULL, @@ -47,20 +54,22 @@ BEGIN CASE event.event_type WHEN 'SecurityAlert' THEN INSERT INTO security_alerts_projection ( - device_id, alert_type, message, details, raised_at + event_id, device_id, alert_type, message, details, raised_at ) VALUES ( + event.id, event.stream_id, event.data->>'alert_type', event.data->>'message', event.data->'details', event.occurred_at - ); + ) + ON CONFLICT (event_id) DO NOTHING; WHEN 'SecurityAlertAcknowledged' THEN UPDATE security_alerts_projection SET acknowledged = TRUE, acknowledged_at = event.occurred_at, acknowledged_by = event.data->>'acknowledged_by' - WHERE id::TEXT = event.data->>'alert_id'; + WHERE event_id::TEXT = event.data->>'alert_id'; ELSE NULL; END CASE; diff --git a/internal/store/queries/security_alerts.sql b/internal/store/queries/security_alerts.sql new file mode 100644 index 00000000..3ad7fc19 --- /dev/null +++ b/internal/store/queries/security_alerts.sql @@ -0,0 +1,37 @@ +-- Phase 1 read queries for the security_alerts_projection (added in +-- migration 010). The RPC surface (ControlService.ListSecurityAlerts, +-- AcknowledgeSecurityAlert) and the web UI wiring land in a follow-up +-- PR because they need proto changes; the queries below are the +-- Go-side primitives those handlers will call. +-- +-- Keeping them in this PR means the projection is not a dead table — +-- internal callers and tests can already exercise it, and future +-- handler work only needs to wire the proto envelope. + +-- name: ListSecurityAlertsForDevice :many +SELECT event_id, device_id, alert_type, message, details, raised_at, + acknowledged, acknowledged_at, acknowledged_by +FROM security_alerts_projection +WHERE device_id = $1 + AND (sqlc.arg(include_acknowledged)::bool OR NOT acknowledged) +ORDER BY raised_at DESC +LIMIT sqlc.arg(page_size)::int +OFFSET sqlc.arg(page_offset)::int; + +-- name: ListUnacknowledgedSecurityAlerts :many +SELECT event_id, device_id, alert_type, message, details, raised_at, + acknowledged, acknowledged_at, acknowledged_by +FROM security_alerts_projection +WHERE NOT acknowledged +ORDER BY raised_at DESC +LIMIT sqlc.arg(page_size)::int +OFFSET sqlc.arg(page_offset)::int; + +-- name: GetSecurityAlert :one +SELECT event_id, device_id, alert_type, message, details, raised_at, + acknowledged, acknowledged_at, acknowledged_by +FROM security_alerts_projection +WHERE event_id = $1; + +-- name: CountUnacknowledgedSecurityAlerts :one +SELECT COUNT(*)::int FROM security_alerts_projection WHERE NOT acknowledged; From 85cc288c5c6d89dddb669317263d6a42d83f288d Mon Sep 17 00:00:00 2001 From: PaulDotterer Date: Thu, 23 Apr 2026 11:24:20 +0200 Subject: [PATCH 05/11] =?UTF-8?q?fix:=20rc10=20review=20round=202=20?= =?UTF-8?q?=E2=80=94=20CodeRabbit=20actionables=20+=20blocker=20finding?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the 8 actionable + 6 nitpick comments CodeRabbit posted on #76 (run 352d924a) plus the reviewer's blocker finding on the terminal-token contract. Blocker — terminal token contract mismatch TestProxyValidateTerminalToken_DoesNotConsumeToken was the pre-rc10 contract ("validation does NOT consume the token") — which is exactly the replay vulnerability the rc10 audit flagged. The updated terminal.TokenStore.Validate consumes on success via GETDEL, so the test was asserting the old contract and failing under the new one. Replace with TestProxyValidateTerminalToken_IsSingleUse asserting the correct rc10 semantic: first validation succeeds, second returns Unauthenticated. Rewrite the RPC doc comment on ProxyValidateTerminalToken to document the single-use contract and the forgery-restore behaviour. terminal_bridge.go only calls the RPC once per WebSocket and uses the returned metadata for the connection's lifetime, so single-use is compatible with the real gateway flow. CodeRabbit actionables • internal/api/errors_parity_test.go - Path used 4 `..` which over-traverses from server/internal/api up to / instead of to the workspace root. Primary fallback is now 3 `..` (workspace layout); the 4-`..` candidate is kept as a secondary fallback for harnesses that run tests from one level higher. PM_SDK_TS_ERRORS env var still takes precedence. - Relaxed the TS extraction regex to accept single, double, or backtick quotes plus an optional `: string` type annotation so a future `export const ErrFoo: string = "…"` refactor still matches. Symmetric change not needed on the Go side — errors.go uses simple `ErrX = "…"` literals. - Removed the dead strings.Count("Err", "E") sanity-init stub; it was a compile-time constant and guarded nothing. • internal/api/registration_handler.go - ValidateGatewayURL now uses u.Hostname() rather than u.Host so pathological shapes like "https://:8443" (port only, no host) are rejected — u.Host would accept them. The validate_gateway_url_test.go suite gains a regression case for exactly that shape. - Exported RedactGatewayURL so cmd/control/main.go can strip userinfo from the URL before logging (the validator rejects URLs with credentials, but the failure log should not echo them back verbatim). Called on all three sites that surface the raw URL: main.go startup error, NewRegistrationHandler panic, and Register's defensive re-validation. • internal/store/migrations/010_security_alerts_projection.sql - SecurityAlertAcknowledged UPDATE now casts alert_id to UUID on the RHS instead of casting event_id to TEXT; lets the primary-key index satisfy the predicate and surfaces a malformed alert_id as a projection error rather than a silent full-scan no-match. - If the UPDATE hits zero rows (out-of-order replay, purged alert) RAISE EXCEPTION so the sidecar trigger's handler logs it into projection_errors. Previously silent. • internal/store/queries/security_alerts.sql - CountUnacknowledgedSecurityAlerts now returns bigint instead of ::int, matching PostgreSQL's native COUNT(*) return and keeping parity with buildNextPageToken's int64 handling. - Added CountSecurityAlertsForDevice companion so the forthcoming ListSecurityAlerts handler has the total it needs to compute next-page tokens correctly. • internal/control/inbox_worker.go - handleRevokeLuksDeviceKeyResult no longer masks every error from GetLuksRevocationStreamID as "not found". Only pgx.ErrNoRows triggers the fresh-ULID fallback (the genuinely- missing-Requested case). Any other error (context, transient DB, etc.) returns so Asynq retries rather than forking the audit stream on every DB flake. • internal/config/config.go - New Config.Validate() method catches the TTY/MTLS host collision when terminal + Traefik self-register are both enabled. Without this, Traefik's TCP passthrough router for the shared SNI wins over the TTY HTTP router and WebSocket handshakes silently break. cfg.Validate() is called at the top of cmd/gateway/main.go so the failure is visible at startup rather than on first terminal attempt. - TestValidate_TTYMTLSHostCollision covers the four shapes (collision+terminal-on, collision+terminal-off, distinct+ terminal-on, collision+self-register-off). • cmd/gateway/main.go - Dropped the redundant `if gatewayReg == nil` guard before ensureGatewayRegistry() inside the TraefikSelfRegister block — the closure is idempotent, the guard was noise. - Moved the InternalURL registration block out from behind `if gatewayReg != nil` and now unconditionally calls ensureGatewayRegistry() before it, so admin fan-out stays available even when Traefik self-register and terminal URL are both off (previously the entire block silently skipped if neither feature had already initialised the registry). CodeRabbit nitpick • internal/api/auth_handler_test.go TestLogin_GlobalPasswordAuthDisabled now asserts the error string contains "password login is disabled" in addition to the connect code check. A future refactor that collapses the branch into the generic invalid-credentials path would pass the code assertion but fail the substring — which is exactly the signal we want. Part of manchtools/power-manage-server#76 review round 2. --- cmd/control/main.go | 5 +- cmd/gateway/main.go | 33 +++++++--- internal/api/auth_handler_test.go | 7 ++ internal/api/errors_parity_test.go | 20 ++++-- internal/api/internal_handler.go | 33 ++++++---- internal/api/internal_handler_test.go | 40 +++++++---- internal/api/registration_handler.go | 39 +++++++++-- internal/api/validate_gateway_url_test.go | 4 ++ internal/config/config.go | 33 ++++++++++ internal/config/config_test.go | 66 +++++++++++++++++++ internal/control/inbox_worker.go | 19 +++++- .../store/generated/security_alerts.sql.go | 33 +++++++++- .../010_security_alerts_projection.sql | 15 ++++- internal/store/queries/security_alerts.sql | 16 ++++- 14 files changed, 310 insertions(+), 53 deletions(-) diff --git a/cmd/control/main.go b/cmd/control/main.go index 1973d0af..d18a349d 100644 --- a/cmd/control/main.go +++ b/cmd/control/main.go @@ -100,7 +100,10 @@ func main() { // connect. api.ValidateGatewayURL is the shared validator // (also invoked defensively in the registration handler). if err := api.ValidateGatewayURL(cfg.GatewayURL); err != nil { - logger.Error("CONTROL_GATEWAY_URL is invalid", "gateway_url", cfg.GatewayURL, "error", err) + // Redact userinfo before logging — the validator rejects + // URLs that contain credentials, but those credentials + // shouldn't land in the startup error line regardless. + logger.Error("CONTROL_GATEWAY_URL is invalid", "gateway_url", api.RedactGatewayURL(cfg.GatewayURL), "error", err) os.Exit(1) } diff --git a/cmd/gateway/main.go b/cmd/gateway/main.go index 76f14934..350258dd 100644 --- a/cmd/gateway/main.go +++ b/cmd/gateway/main.go @@ -55,6 +55,15 @@ func main() { logger := logging.SetupLogger(cfg.LogLevel, "json", os.Stdout) slog.SetDefault(logger) + // Config-shape checks that don't fit the simple "required env + // var empty" pattern below (TTY/MTLS host collision, etc.). + // Failing here keeps them visible at startup rather than at the + // first affected request. + if err := cfg.Validate(); err != nil { + logger.Error("invalid gateway configuration", "error", err) + os.Exit(1) + } + // Validate required config if cfg.ValkeyAddr == "" { logger.Error("GATEWAY_VALKEY_ADDR is required") @@ -251,12 +260,11 @@ func main() { // load-balanced across all replicas; each replica owns a unique // /gw/ path prefix on the shared tty host for TTY routing. if cfg.TraefikSelfRegister { - if gatewayReg == nil { - // Need a Valkey-backed registry even when terminal URLs are - // disabled; Traefik self-registration is the agent mTLS - // routing layer. - gatewayReg = ensureGatewayRegistry() - } + // Need a Valkey-backed registry even when terminal URLs are + // disabled; Traefik self-registration is the agent mTLS + // routing layer. ensureGatewayRegistry is idempotent, so an + // outer nil check would be noise. + gatewayReg = ensureGatewayRegistry() // Auto-derive per-replica backend addresses when not set. We use // the replica's own routable IP on the shared Docker/k8s network @@ -365,10 +373,15 @@ func main() { // Publish the internal mTLS URL so the control server can discover // this gateway for admin fan-out (List/Terminate terminal sessions). - // This is intentionally independent of terminal URL registration: a - // bad optional public terminal URL should not disable the internal - // control-plane route when the registry is otherwise available. - if gatewayReg != nil { + // This is intentionally independent of terminal URL registration: + // a bad optional public terminal URL (which may have left + // gatewayReg unset) should not disable the internal control-plane + // route. ensureGatewayRegistry is idempotent — if the Traefik + // block above already created the registry, this is a no-op; if + // terminal + Traefik are both off, this is where the registry + // gets built so admin fan-out still works. + gatewayReg = ensureGatewayRegistry() + { internalURL := cfg.InternalURL if internalURL == "" { ip, err := routableIP() diff --git a/internal/api/auth_handler_test.go b/internal/api/auth_handler_test.go index d3039b45..45108788 100644 --- a/internal/api/auth_handler_test.go +++ b/internal/api/auth_handler_test.go @@ -88,6 +88,13 @@ func TestLogin_GlobalPasswordAuthDisabled(t *testing.T) { })) require.Error(t, err, "login must fail when global password auth is disabled") assert.Equal(t, connect.CodeUnauthenticated, connect.CodeOf(err)) + // Also assert the user-facing error-surface carries the + // "password login is disabled" sentinel (not the generic + // "invalid credentials"). A future refactor that collapses this + // branch into the invalid-credentials path would look correct + // under the code check but fail this substring — which is + // exactly the signal we want. + assert.Contains(t, err.Error(), "password login is disabled") } func TestLogin_DisabledUser(t *testing.T) { diff --git a/internal/api/errors_parity_test.go b/internal/api/errors_parity_test.go index 01c2ecea..efcdf1db 100644 --- a/internal/api/errors_parity_test.go +++ b/internal/api/errors_parity_test.go @@ -89,6 +89,14 @@ func extractSDKCodes(t *testing.T) []string { if env := os.Getenv("PM_SDK_TS_ERRORS"); env != "" { candidates = append(candidates, env) } + // go test sets cwd to the package dir (server/internal/api), so + // ../../../ resolves to the multi-repo workspace root where + // /sdk lives alongside /server. + candidates = append(candidates, filepath.Join("..", "..", "..", "sdk", "ts", "errors.ts")) + // Fallback: some harnesses run tests from one level higher (e.g. + // when the repo is checked out directly without the workspace + // wrapper). Keeping the older depth as a second candidate lets + // both shapes succeed without env-var wrangling. candidates = append(candidates, filepath.Join("..", "..", "..", "..", "sdk", "ts", "errors.ts")) var data []byte @@ -110,7 +118,11 @@ func extractSDKCodes(t *testing.T) []string { return nil } - re := regexp.MustCompile(`export\s+const\s+Err\w+\s*=\s*'([a-z][a-z0-9_]*)'`) + // Accept single, double, or backtick-quoted string literals plus + // an optional `: string` type annotation so a future refactor to + // `export const ErrFoo: string = "…"` or a template literal + // doesn't silently hide the code from the parity check. + re := regexp.MustCompile(`export\s+const\s+Err\w+(?:\s*:\s*string)?\s*=\s*['"`+"`"+`]([a-z][a-z0-9_]*)['"`+"`"+`]`) matches := re.FindAllStringSubmatch(string(data), -1) seen := make(map[string]struct{}, len(matches)) for _, m := range matches { @@ -140,9 +152,3 @@ func diff(a, b []string) []string { return out } -// Guard against string-builder typos in the tests above. -func init() { - if strings.Count("Err", "E") != 1 { - panic("sanity") - } -} diff --git a/internal/api/internal_handler.go b/internal/api/internal_handler.go index 24c68526..6b62a267 100644 --- a/internal/api/internal_handler.go +++ b/internal/api/internal_handler.go @@ -304,18 +304,29 @@ func (h *InternalHandler) ProxyStoreLpsPasswords(ctx context.Context, req *conne // returns the session metadata the gateway needs to bridge the // connection. // -// Validation does NOT consume the entry — the same gateway uses the -// metadata for the lifetime of the WebSocket. Revocation happens -// explicitly via ControlService.StopTerminal or -// TerminateTerminalSession. This contract is documented on the RPC -// in manchtools/power-manage-sdk#27. +// rc10 single-use contract: a successful validation CONSUMES the +// token atomically (Valkey GETDEL), so a second call with the same +// bearer returns Unauthenticated. This blocks the replay surface +// where a token leaks via a reverse-proxy access log that captured +// the query-string — the attacker can no longer mint additional +// WebSocket connections during the 60 s TTL. // -// Distinguishes 'unknown / expired' (Unauthenticated, with a generic -// message so a forgery probe cannot tell the difference between an -// expired token and a never-existed one) from 'mismatched token' -// (Unauthenticated, but logged separately so the audit pipeline can -// flag forgery attempts). 'Token store not configured' is Unavailable -// — that's an operator misconfiguration, not a client bug. +// Real flow only validates once per WS: the gateway calls this RPC +// from terminal_bridge.go at connection acceptance, stashes the +// returned metadata for the WebSocket's lifetime, and never re- +// validates. So the single-use contract is consistent with normal +// operation; only attacker replays break. +// +// Forgery attempts (valid session_id, wrong bearer) do NOT consume +// the entry — the terminal store restores the session with its +// remaining TTL so a legitimate client isn't locked out by a guess. +// +// Distinguishes 'unknown / expired / already consumed' (Unauthenticated, +// with a generic message so a forgery probe cannot tell the +// difference) from 'mismatched token' (Unauthenticated, but logged +// separately so the audit pipeline can flag forgery attempts). 'Token +// store not configured' is Unavailable — operator misconfiguration, +// not a client bug. func (h *InternalHandler) ProxyValidateTerminalToken(ctx context.Context, req *connect.Request[pm.InternalValidateTerminalTokenRequest]) (*connect.Response[pm.InternalValidateTerminalTokenResponse], error) { if h.terminalTokenStore == nil { return nil, connect.NewError(connect.CodeUnavailable, diff --git a/internal/api/internal_handler_test.go b/internal/api/internal_handler_test.go index b5b286da..d636b58f 100644 --- a/internal/api/internal_handler_test.go +++ b/internal/api/internal_handler_test.go @@ -230,11 +230,19 @@ func TestProxyValidateTerminalToken_HappyPath(t *testing.T) { assert.Equal(t, uint32(40), resp.Msg.Rows) } -func TestProxyValidateTerminalToken_DoesNotConsumeToken(t *testing.T) { - // The contract documented above the RPC says validation must NOT - // consume the entry — the same gateway uses the metadata for the - // lifetime of the WebSocket. Validate twice and assert both - // succeed. +func TestProxyValidateTerminalToken_IsSingleUse(t *testing.T) { + // rc10 contract: a successful validation atomically consumes the + // token so replays within the TTL fail with Unauthenticated. This + // blocks the leaked-token replay surface (reverse-proxy access + // logs capturing query strings, browser history snooping, etc.) + // without affecting normal operation — the real gateway flow in + // terminal_bridge.go validates the token exactly once per + // WebSocket connection and uses the returned metadata for the + // lifetime of the connection. + // + // Supersedes the pre-rc10 TestProxyValidateTerminalToken_DoesNotConsumeToken + // which asserted the opposite and is exactly the contract the + // audit flagged as a replay vulnerability. h, tokenStore := newInternalHandlerWithTokenStore(t) mintRes, err := tokenStore.Mint(context.Background(), terminal.MintParams{ @@ -244,13 +252,21 @@ func TestProxyValidateTerminalToken_DoesNotConsumeToken(t *testing.T) { }) require.NoError(t, err) - for i := 0; i < 3; i++ { - _, err := h.ProxyValidateTerminalToken(context.Background(), connect.NewRequest(&pm.InternalValidateTerminalTokenRequest{ - SessionId: mintRes.SessionID, - Token: mintRes.Token, - })) - require.NoErrorf(t, err, "validation %d should succeed", i+1) - } + // First validation succeeds and consumes the token. + _, err = h.ProxyValidateTerminalToken(context.Background(), connect.NewRequest(&pm.InternalValidateTerminalTokenRequest{ + SessionId: mintRes.SessionID, + Token: mintRes.Token, + })) + require.NoError(t, err, "first validation should succeed") + + // Second validation with the same bearer fails with + // Unauthenticated — the token is gone. + _, err = h.ProxyValidateTerminalToken(context.Background(), connect.NewRequest(&pm.InternalValidateTerminalTokenRequest{ + SessionId: mintRes.SessionID, + Token: mintRes.Token, + })) + require.Error(t, err, "second validation must fail (single-use contract)") + assert.Equal(t, connect.CodeUnauthenticated, connect.CodeOf(err)) } func TestProxyValidateTerminalToken_UnknownSession(t *testing.T) { diff --git a/internal/api/registration_handler.go b/internal/api/registration_handler.go index 44667e16..726d03b5 100644 --- a/internal/api/registration_handler.go +++ b/internal/api/registration_handler.go @@ -48,8 +48,11 @@ func ValidateGatewayURL(raw string) error { if u.Scheme != "https" { return fmt.Errorf("gateway URL must use https scheme, got %q", u.Scheme) } - if u.Host == "" { - return fmt.Errorf("gateway URL has no host — bare hostnames like %q are not absolute URLs", raw) + // u.Hostname() strips port and brackets so "https://:8443" (port + // only, no host) and "https://[::1]:443" (IPv6) both validate + // under the same rule. u.Host would accept ":8443" silently. + if u.Hostname() == "" { + return fmt.Errorf("gateway URL has no host — bare hostnames like %q are not absolute URLs", RedactGatewayURL(raw)) } if u.User != nil { return fmt.Errorf("gateway URL must not contain userinfo (credentials in URL leak on every enrollment response)") @@ -60,6 +63,31 @@ func ValidateGatewayURL(raw string) error { return nil } +// RedactGatewayURL strips userinfo from a URL-shaped string for safe +// logging / panic messages. Exported so cmd/control/main.go can use +// the same redaction on the startup-log error path. +// +// If the input is unparseable, returns a placeholder rather than the +// raw value — a malformed url.Parse input that still carries +// credentials in a substring shouldn't leak just because the parser +// rejected it (e.g. "https://u:p@host:notaport" fails to parse as +// a URL but still contains "u:p" in-band). +func RedactGatewayURL(raw string) string { + if raw == "" { + return "" + } + u, err := url.Parse(raw) + if err != nil { + return "" + } + if u.User == nil { + return raw + } + // Rebuild without userinfo. + u.User = nil + return u.String() +} + // RegistrationHandler handles agent registration requests. type RegistrationHandler struct { store *store.Store @@ -76,7 +104,10 @@ type RegistrationHandler struct { // check with a clean operator-facing error message. func NewRegistrationHandler(st *store.Store, certAuth *ca.CA, gatewayURL string, logger *slog.Logger) *RegistrationHandler { if err := ValidateGatewayURL(gatewayURL); err != nil { - panic(fmt.Sprintf("NewRegistrationHandler: invalid gateway URL %q: %v", gatewayURL, err)) + // Redact userinfo before panicking — a gateway URL that + // contains credentials (which the validator is rejecting) + // would otherwise leak them into the crash log. + panic(fmt.Sprintf("NewRegistrationHandler: invalid gateway URL %q: %v", RedactGatewayURL(gatewayURL), err)) } return &RegistrationHandler{ store: st, @@ -101,7 +132,7 @@ func (h *RegistrationHandler) Register(ctx context.Context, req *connect.Request // bare hostnames, http://, userinfo, etc. if err := ValidateGatewayURL(h.gatewayURL); err != nil { logger.Error("registration refused: gatewayURL failed validation", - "gateway_url", h.gatewayURL, "error", err) + "gateway_url", RedactGatewayURL(h.gatewayURL), "error", err) return nil, apiErrorCtx(ctx, ErrInternal, connect.CodeFailedPrecondition, "server misconfiguration: gateway URL is invalid") } diff --git a/internal/api/validate_gateway_url_test.go b/internal/api/validate_gateway_url_test.go index c3870f20..49e61147 100644 --- a/internal/api/validate_gateway_url_test.go +++ b/internal/api/validate_gateway_url_test.go @@ -38,6 +38,10 @@ func TestValidateGatewayURL(t *testing.T) { {"userinfo username only", "https://user@gateway.example.com", true, "userinfo"}, {"fragment", "https://gateway.example.com#frag", true, "fragment"}, {"host only", "https://", true, "host"}, + // CR called out that u.Host would accept ":8443" silently + // (it treats the empty part as a host with a port). Switching + // to u.Hostname() plus this regression test closes the gap. + {"port without hostname", "https://:8443", true, "host"}, {"happy path host", "https://gateway.example.com", false, ""}, {"happy path with port", "https://gateway.example.com:8443", false, ""}, diff --git a/internal/config/config.go b/internal/config/config.go index d59b8385..6a6eb9c7 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -2,6 +2,7 @@ package config import ( + "fmt" "os" "strconv" "time" @@ -212,6 +213,38 @@ func FromEnv() *Config { } } +// Validate returns a non-nil error when the loaded config has a +// combination that the gateway cannot serve coherently. Called once +// at startup from cmd/gateway/main.go; keeping it on the Config +// struct (rather than inline in main) so tests can exercise the +// shape checks without booting a full process. +func (c *Config) Validate() error { + // TTY / MTLS host collision: when the terminal WebSocket + // listener is enabled (GATEWAY_WEB_LISTEN_ADDR set) and Traefik + // self-registration is on, both the shared mTLS TCP router and + // the per-replica TTY HTTP router bind on the same + // --entrypoints.websecure address. Traefik's TCP router (with + // HostSNI + tls.passthrough) takes precedence for connections + // whose SNI matches, so a TTY client sending SNI=TTYHost gets + // routed to the mTLS passthrough and the WebSocket handshake + // fails against a backend that's expecting a raw TLS client + // cert. The only safe split is distinct hostnames. + // + // Refuse startup with a clear message instead of letting the + // operator discover this when a terminal session silently fails. + if c.TraefikSelfRegister && + c.WebListenAddr != "" && + c.TraefikTTYHost != "" && + c.TraefikMTLSHost != "" && + c.TraefikTTYHost == c.TraefikMTLSHost { + return fmt.Errorf( + "GATEWAY_TTY_DOMAIN / TraefikTTYHost cannot equal GATEWAY_DOMAIN / TraefikMTLSHost when terminal is enabled (both %q): Traefik TCP passthrough for mTLS would match the same SNI as the TTY HTTP router and break WebSocket sessions — set GATEWAY_TTY_DOMAIN to a distinct hostname", + c.TraefikTTYHost, + ) + } + return nil +} + // firstNonEmpty returns the first argument that isn't the empty // string. Used to resolve an env-var name (the legacy one) with a // more-preferred name as the primary source. diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 8bde0dfa..f6497c17 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -121,6 +121,72 @@ func TestFromEnv_TraefikTTYCertResolver(t *testing.T) { } } +// TestValidate_TTYMTLSHostCollision captures the config-shape invariant +// CodeRabbit flagged on the rc10 review: when the terminal WebSocket +// listener is enabled AND Traefik self-registration is on, the TTY +// host must not equal the mTLS host. If they match, Traefik's TCP +// passthrough router for the shared SNI wins over the TTY HTTP router +// and the WebSocket handshake fails against the mTLS backend. +func TestValidate_TTYMTLSHostCollision(t *testing.T) { + cases := []struct { + name string + cfg Config + wantErr bool + }{ + { + name: "collision with terminal enabled → error", + cfg: Config{ + TraefikSelfRegister: true, + WebListenAddr: ":8443", + TraefikMTLSHost: "gw.example.com", + TraefikTTYHost: "gw.example.com", + }, + wantErr: true, + }, + { + name: "collision but terminal disabled → OK", + cfg: Config{ + TraefikSelfRegister: true, + WebListenAddr: "", // terminal off + TraefikMTLSHost: "gw.example.com", + TraefikTTYHost: "gw.example.com", + }, + wantErr: false, + }, + { + name: "distinct hosts with terminal enabled → OK", + cfg: Config{ + TraefikSelfRegister: true, + WebListenAddr: ":8443", + TraefikMTLSHost: "gw.example.com", + TraefikTTYHost: "tty.example.com", + }, + wantErr: false, + }, + { + name: "collision but self-register off → OK (operator owns routing)", + cfg: Config{ + TraefikSelfRegister: false, + WebListenAddr: ":8443", + TraefikMTLSHost: "gw.example.com", + TraefikTTYHost: "gw.example.com", + }, + wantErr: false, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := tc.cfg.Validate() + if tc.wantErr && err == nil { + t.Fatalf("Validate() = nil, want error for %+v", tc.cfg) + } + if !tc.wantErr && err != nil { + t.Fatalf("Validate() = %v, want nil for %+v", err, tc.cfg) + } + }) + } +} + func TestGetEnvInt_ValidValues(t *testing.T) { tests := []struct { name string diff --git a/internal/control/inbox_worker.go b/internal/control/inbox_worker.go index e17d73c8..ecedd43f 100644 --- a/internal/control/inbox_worker.go +++ b/internal/control/inbox_worker.go @@ -563,13 +563,26 @@ func (w *InboxWorker) handleRevokeLuksDeviceKeyResult(ctx context.Context, t *as DeviceID: payload.DeviceID, ActionID: payload.ActionID, }) - if err != nil { - w.logger.Warn("could not look up LUKS revocation stream ID — appending to a fresh stream; projection will show only the terminal event", + switch { + case err == nil: + // Happy path — stream ID recovered. + case errors.Is(err, pgx.ErrNoRows): + // Genuinely absent: the Requested event never landed + // (original RPC crashed before append). Fall back to a + // fresh ULID so we still record the terminal outcome — + // an orphan Failed event is better than silently dropping + // the agent-reported failure on the floor. + w.logger.Warn("LUKS revocation stream ID not found — appending to a fresh stream; projection will show only the terminal event", "device_id", payload.DeviceID, "action_id", payload.ActionID, - "error", err, ) luksStreamID = ulid.Make().String() + default: + // Transient DB / context error. Return so Asynq retries; + // previously we masked these as "not found" and forked + // the stream, which would compound audit fragmentation + // under DB flakes. + return fmt.Errorf("look up LUKS revocation stream ID for device %s action %s: %w", payload.DeviceID, payload.ActionID, err) } if payload.Success { diff --git a/internal/store/generated/security_alerts.sql.go b/internal/store/generated/security_alerts.sql.go index 29f3377c..76cf048a 100644 --- a/internal/store/generated/security_alerts.sql.go +++ b/internal/store/generated/security_alerts.sql.go @@ -12,13 +12,40 @@ import ( "github.com/google/uuid" ) +const countSecurityAlertsForDevice = `-- name: CountSecurityAlertsForDevice :one +SELECT COUNT(*)::bigint +FROM security_alerts_projection +WHERE device_id = $1 + AND ($2::bool OR NOT acknowledged) +` + +type CountSecurityAlertsForDeviceParams struct { + DeviceID string `json:"device_id"` + IncludeAcknowledged bool `json:"include_acknowledged"` +} + +// Companion count for ListSecurityAlertsForDevice. Needed by +// buildNextPageToken to compute totalCount and emit a correct +// next-page token; mirrors the same include_acknowledged filter +// semantics as the list query so the two stay in lockstep. +func (q *Queries) CountSecurityAlertsForDevice(ctx context.Context, arg CountSecurityAlertsForDeviceParams) (int64, error) { + row := q.db.QueryRow(ctx, countSecurityAlertsForDevice, arg.DeviceID, arg.IncludeAcknowledged) + var column_1 int64 + err := row.Scan(&column_1) + return column_1, err +} + const countUnacknowledgedSecurityAlerts = `-- name: CountUnacknowledgedSecurityAlerts :one -SELECT COUNT(*)::int FROM security_alerts_projection WHERE NOT acknowledged +SELECT COUNT(*)::bigint FROM security_alerts_projection WHERE NOT acknowledged ` -func (q *Queries) CountUnacknowledgedSecurityAlerts(ctx context.Context) (int32, error) { +// COUNT(*) in PostgreSQL is bigint; keep the full precision so +// buildNextPageToken (which works in int64) doesn't see a silently +// truncated int32 once device counts climb past 2.1B aggregate +// alerts across all time. Matches the pagination helper contract. +func (q *Queries) CountUnacknowledgedSecurityAlerts(ctx context.Context) (int64, error) { row := q.db.QueryRow(ctx, countUnacknowledgedSecurityAlerts) - var column_1 int32 + var column_1 int64 err := row.Scan(&column_1) return column_1, err } diff --git a/internal/store/migrations/010_security_alerts_projection.sql b/internal/store/migrations/010_security_alerts_projection.sql index 9d89505d..fb75a833 100644 --- a/internal/store/migrations/010_security_alerts_projection.sql +++ b/internal/store/migrations/010_security_alerts_projection.sql @@ -65,11 +65,24 @@ BEGIN ) ON CONFLICT (event_id) DO NOTHING; WHEN 'SecurityAlertAcknowledged' THEN + -- Cast the right-hand side to UUID (not event_id to + -- TEXT) so the primary-key index on event_id is used + -- and a malformed alert_id surfaces as a projection + -- error instead of silently full-scanning and matching + -- zero rows. UPDATE security_alerts_projection SET acknowledged = TRUE, acknowledged_at = event.occurred_at, acknowledged_by = event.data->>'acknowledged_by' - WHERE event_id::TEXT = event.data->>'alert_id'; + WHERE event_id = (event.data->>'alert_id')::uuid; + IF NOT FOUND THEN + -- Out-of-order replay (ack before the alert row + -- exists) or an alert that was purged. Raise so + -- the sidecar trigger's EXCEPTION handler logs it + -- into projection_errors for operator visibility; + -- previously this was a silent no-op. + RAISE EXCEPTION 'SecurityAlertAcknowledged references unknown alert_id=%', event.data->>'alert_id'; + END IF; ELSE NULL; END CASE; diff --git a/internal/store/queries/security_alerts.sql b/internal/store/queries/security_alerts.sql index 3ad7fc19..589fb902 100644 --- a/internal/store/queries/security_alerts.sql +++ b/internal/store/queries/security_alerts.sql @@ -33,5 +33,19 @@ SELECT event_id, device_id, alert_type, message, details, raised_at, FROM security_alerts_projection WHERE event_id = $1; +-- COUNT(*) in PostgreSQL is bigint; keep the full precision so +-- buildNextPageToken (which works in int64) doesn't see a silently +-- truncated int32 once device counts climb past 2.1B aggregate +-- alerts across all time. Matches the pagination helper contract. -- name: CountUnacknowledgedSecurityAlerts :one -SELECT COUNT(*)::int FROM security_alerts_projection WHERE NOT acknowledged; +SELECT COUNT(*)::bigint FROM security_alerts_projection WHERE NOT acknowledged; + +-- Companion count for ListSecurityAlertsForDevice. Needed by +-- buildNextPageToken to compute totalCount and emit a correct +-- next-page token; mirrors the same include_acknowledged filter +-- semantics as the list query so the two stay in lockstep. +-- name: CountSecurityAlertsForDevice :one +SELECT COUNT(*)::bigint +FROM security_alerts_projection +WHERE device_id = $1 + AND (sqlc.arg(include_acknowledged)::bool OR NOT acknowledged); From 8c7aeda676d7f1436ad53463dbdfc5e26d7b5dee Mon Sep 17 00:00:00 2001 From: PaulDotterer Date: Thu, 23 Apr 2026 11:35:17 +0200 Subject: [PATCH 06/11] =?UTF-8?q?docs:=20rc10=20=E2=80=94=20correct=20TTY?= =?UTF-8?q?=20host=20fallback=20comments=20to=20match=20Validate()?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewer called out a coherence lie introduced in the rc10 review round 2: config.go's comment above the TraefikTTYHost fallback still promised single-domain terminal deployments, but the new Config.Validate() correctly rejects that shape when terminal + Traefik self-registration are both on. The code was right; the comment and .env.example weren't. config.go: rewrite the comment block so it states that the GATEWAY_DOMAIN rung only works when the terminal listener is disabled or when Traefik self-registration is off, and that a distinct GATEWAY_TTY_DOMAIN is required in the combined case. .env.example: same clarification in operator-facing terms, with the rationale (Traefik TCP passthrough shadows the TTY HTTP router on a shared SNI) and the fail-fast behavior at startup so an operator hitting this gets an immediate error instead of a silent WebSocket break. No code change; coherence fix only. --- deploy/.env.example | 20 ++++++++++++++++---- internal/config/config.go | 18 +++++++++++++----- 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/deploy/.env.example b/deploy/.env.example index c5de8f37..22c3ff87 100644 --- a/deploy/.env.example +++ b/deploy/.env.example @@ -56,10 +56,22 @@ CONTROL_DOMAIN=power-manage.example.com # gateway TCP passthrough for the mTLS handshake on this subdomain. GATEWAY_DOMAIN=gateway.example.com -# Public domain for TTY WebSocket traffic. Defaults to GATEWAY_DOMAIN when -# unset, so single-domain deployments just leave this empty. Each gateway -# replica takes a /gw/ path prefix on this host for session-specific -# routing. +# Public domain for TTY WebSocket traffic. +# +# When the terminal WebSocket listener is enabled (the default when +# GATEWAY_WEB_LISTEN_ADDR is set and Traefik self-registration is on), +# this MUST be a different hostname than GATEWAY_DOMAIN — otherwise +# Traefik's TCP passthrough router for mTLS shadows the TTY HTTP +# router on the shared SNI and WebSocket sessions break silently. +# The gateway refuses to start in that collision state. +# +# When the terminal listener is disabled (GATEWAY_WEB_LISTEN_ADDR +# empty), this falls back to GATEWAY_DOMAIN harmlessly — the +# fallback only exists so the Traefik-registry entry still has a +# non-empty host field. +# +# Each gateway replica takes a /gw/ path prefix on this host +# for session-specific routing. # GATEWAY_TTY_DOMAIN=tty.example.com # ============================================================================= diff --git a/internal/config/config.go b/internal/config/config.go index 6a6eb9c7..2eccdda2 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -195,11 +195,19 @@ func FromEnv() *Config { TraefikMTLSBackend: getEnv("GATEWAY_TRAEFIK_MTLS_BACKEND", ""), TraefikMTLSEntryPoint: getEnv("GATEWAY_TRAEFIK_MTLS_ENTRYPOINT", "websecure"), // TTYHost falls back through GATEWAY_TTY_DOMAIN (dedicated - // TTY subdomain) to GATEWAY_DOMAIN (single-domain deploys - // where terminal WebSocket shares the gateway hostname). - // .env.example documents exactly this chain; rc9 only - // implemented the first two rungs, which was the hidden - // empty-host trap that crashed gateway startup in staging. + // TTY subdomain) to GATEWAY_DOMAIN. Without this chain rc9 + // hit the hidden empty-host trap that crashed gateway + // startup in staging. + // + // Note on the GATEWAY_DOMAIN rung: it only works when the + // terminal WebSocket listener is disabled + // (GATEWAY_WEB_LISTEN_ADDR empty) or when Traefik self- + // registration is off. With terminal + self-register both + // enabled, a shared TTY/MTLS hostname is refused by + // Validate() below — Traefik's TCP passthrough would + // match the shared SNI and shadow the TTY HTTP router, + // silently breaking WebSocket sessions. Single-domain + // terminal deployments need a separate GATEWAY_TTY_DOMAIN. TraefikTTYHost: firstNonEmpty( os.Getenv("GATEWAY_TRAEFIK_TTY_HOST"), os.Getenv("GATEWAY_TTY_DOMAIN"), From fb4ab1f367eefeaf0ac5038bb6df61240388ca88 Mon Sep 17 00:00:00 2001 From: PaulDotterer Date: Thu, 23 Apr 2026 12:45:57 +0200 Subject: [PATCH 07/11] =?UTF-8?q?fix:=20rc10=20review=20round=203=20?= =?UTF-8?q?=E2=80=94=20CodeRabbit=20coherence=20+=20hostFromURL=20hardenin?= =?UTF-8?q?g?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the four findings CodeRabbit posted on the 8c7aeda diff (run 374e4ec0). All four are the same class of issue — "the code now does X but some sibling code/comment/check didn't get the memo" — which is exactly what happens when a previous review round fixes one site but misses its twins. Sweeping the twins now. 1. internal/config/config.go (outside diff, line ~169) The Traefik defaults summary comment still said "TTYHost falls back to empty → TTY router disabled" while the actual code (and the matching comment I fixed in the previous push) now falls back through GATEWAY_TTY_DOMAIN to GATEWAY_DOMAIN, with Validate() rejecting the unsafe shared-host case. Rewrite the bullet so the summary matches the body. 2. cmd/gateway/main.go hostFromURL (outside diff, lines ~695-707) ValidateGatewayURL got tightened to u.Hostname() == "" in the previous round, catching shapes like "https://:8443". The sibling hostFromURL helper used for PublicAgentURLTemplate / PublicTerminalURLTemplate still only checked u.Host == "", which is non-empty for ":8443" — so a template like "https://${UNSET}:8443/..." collapsed into a port-only URL still passed hostFromURL, enabling invalid bootstrap redirects and terminal registry entries. Add the same u.Hostname() invariant here so both validators agree. 3. cmd/control/main.go startup log (inline, line ~94) The rc10 review round 2 redacted GatewayURL on the error path but left the startup Info log emitting cfg.GatewayURL verbatim. A bad shape that slipped past the validator (or an operator paste mistake that the validator rejects after the log line) would still dump any userinfo into every boot log. Wrap the startup log's gateway_url field in api.RedactGatewayURL too so the redaction is symmetric across both log sites. 4. internal/config/config.go Validate() (inline, lines ~243-252) The collision check triggered only on WebListenAddr != "", missing the case where the operator sets an explicit GATEWAY_TRAEFIK_TTY_BACKEND without WebListenAddr. It also flagged topologies where the mTLS and TTY routers bind DIFFERENT entrypoints — which don't actually collide because Traefik routes per-entrypoint before per-host, so a false positive would fail-fast a valid bring-your-own-Traefik split- port setup. Split the precondition into: terminalEnabled = WebListenAddr || TraefikTTYBackend entrypointsCollide = TTYEntryPoint == MTLSEntryPoint (and non-empty) and only flag the host collision when all three preconditions line up. TestValidate_TTYMTLSHostCollision now covers two new cases (explicit TTYBackend triggers error; different entry- points skips the check) alongside the original four. All targeted go tests green: validator suite, config collision matrix, login bypass regression, terminal token single-use, registration handler, error-code parity. Part of manchtools/power-manage-server#76 review round 3. --- cmd/control/main.go | 5 ++- cmd/gateway/main.go | 15 ++++++-- internal/config/config.go | 38 ++++++++++++------- internal/config/config_test.go | 67 +++++++++++++++++++++++++--------- 4 files changed, 89 insertions(+), 36 deletions(-) diff --git a/cmd/control/main.go b/cmd/control/main.go index d18a349d..2f4a1266 100644 --- a/cmd/control/main.go +++ b/cmd/control/main.go @@ -91,7 +91,10 @@ func main() { logger := logging.SetupLogger(cfg.LogLevel, cfg.LogFormat, os.Stderr) slog.SetDefault(logger) - logger.Info("starting control server", "version", version, "listen_addr", cfg.ListenAddr, "gateway_url", cfg.GatewayURL, "dynamic_group_eval_interval", cfg.DynamicGroupEvalInterval) + // Redact the gateway URL on the startup line too. If a bad shape + // slipped in (e.g. https://u:p@host/ despite the validator, or an + // operator paste-mistake), it shouldn't land in every boot log. + logger.Info("starting control server", "version", version, "listen_addr", cfg.ListenAddr, "gateway_url", api.RedactGatewayURL(cfg.GatewayURL), "dynamic_group_eval_interval", cfg.DynamicGroupEvalInterval) // CONTROL_GATEWAY_URL is fatal when invalid: registration hands // it back to the agent verbatim, so any invalid shape — empty // string, bare hostname (parses as a relative path), http:// diff --git a/cmd/gateway/main.go b/cmd/gateway/main.go index 350258dd..22d325a9 100644 --- a/cmd/gateway/main.go +++ b/cmd/gateway/main.go @@ -698,10 +698,17 @@ func hostFromURL(raw string) string { default: return "" } - // u.Host includes the port (e.g. "gw-01.example.com:8443"). - // u.Hostname() strips it, which would break redirects on - // non-default ports. - if u.Host == "" { + // u.Host includes the port (e.g. "gw-01.example.com:8443"), which + // is what the caller needs for bootstrap redirects and registry + // registration. u.Hostname() strips the port, so we return u.Host. + // + // But both checks matter: a template like "https://${UNSET}:8443" + // collapses to "https://:8443" when the env var is missing. + // u.Host = ":8443" is non-empty (would pass the first check) + // while u.Hostname() = "" (no hostname). Require both — same + // invariant api.ValidateGatewayURL enforces for the control + // server's outbound URL. + if u.Host == "" || u.Hostname() == "" { return "" } return u.Host diff --git a/internal/config/config.go b/internal/config/config.go index 2eccdda2..49c23b1f 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -166,7 +166,8 @@ func FromEnv() *Config { // * RootKey = traefik — matches Traefik default --providers.redis.rootkey // * MTLSHost = GATEWAY_DOMAIN (not Traefik-prefixed — one name per thing) // * MTLSEntryPoint = websecure (same :443 as control, SNI-separated) - // * TTYHost = GATEWAY_TTY_DOMAIN (falls back to empty -> TTY router disabled) + // * TTYHost = GATEWAY_TTY_DOMAIN, then GATEWAY_DOMAIN + // (Validate rejects unsafe shared-host terminal+self-register configs) // * TTYEntryPoint = websecure // * TTYCertResolver = letsencrypt // @@ -227,27 +228,36 @@ func FromEnv() *Config { // struct (rather than inline in main) so tests can exercise the // shape checks without booting a full process. func (c *Config) Validate() error { - // TTY / MTLS host collision: when the terminal WebSocket - // listener is enabled (GATEWAY_WEB_LISTEN_ADDR set) and Traefik - // self-registration is on, both the shared mTLS TCP router and - // the per-replica TTY HTTP router bind on the same - // --entrypoints.websecure address. Traefik's TCP router (with - // HostSNI + tls.passthrough) takes precedence for connections - // whose SNI matches, so a TTY client sending SNI=TTYHost gets - // routed to the mTLS passthrough and the WebSocket handshake - // fails against a backend that's expecting a raw TLS client - // cert. The only safe split is distinct hostnames. + // TTY / MTLS host collision: when Traefik self-registration is + // on AND the terminal feature is in play AND the mTLS + TTY + // routers bind the same entrypoint, the shared hostname means + // Traefik's TCP-passthrough router for mTLS wins the SNI match + // and the TTY HTTP router never gets the request. The TLS + // handshake lands on an agent-mTLS backend that isn't speaking + // HTTP, so the WebSocket upgrade fails silently. + // + // Split the preconditions explicitly so the check handles both + // the auto-derived backend case (operator only set + // GATEWAY_WEB_LISTEN_ADDR) and the explicit backend case + // (operator set GATEWAY_TRAEFIK_TTY_BACKEND directly). Also + // narrow to "same entrypoint" — different entrypoints mean the + // routers don't actually collide even on a shared host, so + // flagging that shape would be a false positive for + // bring-your-own-Traefik topologies. // // Refuse startup with a clear message instead of letting the // operator discover this when a terminal session silently fails. + terminalEnabled := c.WebListenAddr != "" || c.TraefikTTYBackend != "" + entrypointsCollide := c.TraefikTTYEntryPoint != "" && c.TraefikTTYEntryPoint == c.TraefikMTLSEntryPoint if c.TraefikSelfRegister && - c.WebListenAddr != "" && + terminalEnabled && + entrypointsCollide && c.TraefikTTYHost != "" && c.TraefikMTLSHost != "" && c.TraefikTTYHost == c.TraefikMTLSHost { return fmt.Errorf( - "GATEWAY_TTY_DOMAIN / TraefikTTYHost cannot equal GATEWAY_DOMAIN / TraefikMTLSHost when terminal is enabled (both %q): Traefik TCP passthrough for mTLS would match the same SNI as the TTY HTTP router and break WebSocket sessions — set GATEWAY_TTY_DOMAIN to a distinct hostname", - c.TraefikTTYHost, + "GATEWAY_TTY_DOMAIN / TraefikTTYHost cannot equal GATEWAY_DOMAIN / TraefikMTLSHost when the terminal router and mTLS router share an entrypoint (both %q on entrypoint %q): Traefik TCP passthrough for mTLS matches the same SNI as the TTY HTTP router and breaks WebSocket sessions — set a distinct GATEWAY_TTY_DOMAIN or split the entrypoints", + c.TraefikTTYHost, c.TraefikTTYEntryPoint, ) } return nil diff --git a/internal/config/config_test.go b/internal/config/config_test.go index f6497c17..686858fb 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -134,42 +134,75 @@ func TestValidate_TTYMTLSHostCollision(t *testing.T) { wantErr bool }{ { - name: "collision with terminal enabled → error", + name: "collision with terminal enabled (WebListenAddr) → error", cfg: Config{ - TraefikSelfRegister: true, - WebListenAddr: ":8443", - TraefikMTLSHost: "gw.example.com", - TraefikTTYHost: "gw.example.com", + TraefikSelfRegister: true, + WebListenAddr: ":8443", + TraefikMTLSHost: "gw.example.com", + TraefikTTYHost: "gw.example.com", + TraefikMTLSEntryPoint: "websecure", + TraefikTTYEntryPoint: "websecure", + }, + wantErr: true, + }, + { + name: "collision with terminal enabled (explicit TTYBackend) → error", + cfg: Config{ + TraefikSelfRegister: true, + TraefikTTYBackend: "http://10.0.0.5:8443", + TraefikMTLSHost: "gw.example.com", + TraefikTTYHost: "gw.example.com", + TraefikMTLSEntryPoint: "websecure", + TraefikTTYEntryPoint: "websecure", }, wantErr: true, }, { name: "collision but terminal disabled → OK", cfg: Config{ - TraefikSelfRegister: true, - WebListenAddr: "", // terminal off - TraefikMTLSHost: "gw.example.com", - TraefikTTYHost: "gw.example.com", + TraefikSelfRegister: true, + WebListenAddr: "", // terminal off + TraefikTTYBackend: "", + TraefikMTLSHost: "gw.example.com", + TraefikTTYHost: "gw.example.com", + TraefikMTLSEntryPoint: "websecure", + TraefikTTYEntryPoint: "websecure", }, wantErr: false, }, { name: "distinct hosts with terminal enabled → OK", cfg: Config{ - TraefikSelfRegister: true, - WebListenAddr: ":8443", - TraefikMTLSHost: "gw.example.com", - TraefikTTYHost: "tty.example.com", + TraefikSelfRegister: true, + WebListenAddr: ":8443", + TraefikMTLSHost: "gw.example.com", + TraefikTTYHost: "tty.example.com", + TraefikMTLSEntryPoint: "websecure", + TraefikTTYEntryPoint: "websecure", + }, + wantErr: false, + }, + { + name: "shared host but different entrypoints → OK (routers don't collide)", + cfg: Config{ + TraefikSelfRegister: true, + WebListenAddr: ":8443", + TraefikMTLSHost: "gw.example.com", + TraefikTTYHost: "gw.example.com", + TraefikMTLSEntryPoint: "mtls", + TraefikTTYEntryPoint: "websecure", }, wantErr: false, }, { name: "collision but self-register off → OK (operator owns routing)", cfg: Config{ - TraefikSelfRegister: false, - WebListenAddr: ":8443", - TraefikMTLSHost: "gw.example.com", - TraefikTTYHost: "gw.example.com", + TraefikSelfRegister: false, + WebListenAddr: ":8443", + TraefikMTLSHost: "gw.example.com", + TraefikTTYHost: "gw.example.com", + TraefikMTLSEntryPoint: "websecure", + TraefikTTYEntryPoint: "websecure", }, wantErr: false, }, From 7f4179ad93be76335576bc91844a99edcf7520df Mon Sep 17 00:00:00 2001 From: PaulDotterer Date: Thu, 23 Apr 2026 14:20:32 +0200 Subject: [PATCH 08/11] =?UTF-8?q?fix:=20rc10=20review=20round=204=20?= =?UTF-8?q?=E2=80=94=20agent-template=20fallback=20gating=20+=20bounded=20?= =?UTF-8?q?Redis=20contexts?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit round 4 on fb4ab1f (run 03d850c8) surfaced one major correctness issue and one maintainability nitpick. Both fixed. 1. cmd/gateway/main.go lines 198-226 — major, outside diff The fallback at line 224 treated assignedHost == "" as "no agent template provided", which conflates two distinct states: (a) GATEWAY_PUBLIC_AGENT_URL_TEMPLATE unset entirely (b) GATEWAY_PUBLIC_AGENT_URL_TEMPLATE set but resolved to a URL with no host (e.g. "https://${UNSET_VAR}" collapses to "https://") Case (b) already logs a warning and disables bootstrap redirects. But the subsequent fallback `if assignedHost == "" { assignedHost = terminalHost }` couldn't distinguish (a) from (b), so a malformed agent template silently re-enabled bootstrap redirects to the TTY host — directly contradicting the intent of disabling them when the operator's explicit template couldn't resolve. Introduce agentURLTemplateConfigured, set it when cfg.PublicAgentURLTemplate != "", and gate the terminal-host fallback on `assignedHost == "" && !agentURLTemplateConfigured`. An operator who wrote a broken template now gets exactly what the warning promises: feature disabled, not silently redirected to the wrong host. 2. cmd/gateway/main.go lines 316-350, 403-425 — nitpick PublishTraefikRoute + RegisterGatewayInternal (both the initial call and the periodic refresh) used context.Background(), so a slow or hung Redis would ignore SIGTERM and delay graceful shutdown. The RevokeTraefikRoute defer already uses a 5s bound for exactly this reason. Bound both calls with shutdownCtx + 5s timeout (refresh calls derive from the refresh's own cancellable context rather than shutdownCtx directly, so the timeout cancels cleanly when the refresh goroutine stops). Matches the heartbeat pattern used inside registry.RegisterGateway. Targeted tests all pass: config Validate matrix, parity guard, registration validator, terminal token single-use, login bypass regression. Part of manchtools/power-manage-server#76 review round 4. --- cmd/gateway/main.go | 67 +++++++++++++++++++++++++++++++++------------ 1 file changed, 50 insertions(+), 17 deletions(-) diff --git a/cmd/gateway/main.go b/cmd/gateway/main.go index 22d325a9..cfdb2c49 100644 --- a/cmd/gateway/main.go +++ b/cmd/gateway/main.go @@ -195,7 +195,17 @@ func main() { // each exit, the pm-mtls TCP router fell through to the HTTP router // on the shared :443 and served the Let's Encrypt cert — giving // agents the misleading x509 "unknown authority" error. - if cfg.PublicAgentURLTemplate != "" { + // Track whether the agent URL template was *configured* separately + // from whether it *resolved*. Without this split, a malformed + // template (e.g. "https://${UNSET_VAR}" → "https://") leaves + // assignedHost empty and the terminal-template block below would + // silently paper over the misconfiguration by substituting the TTY + // host — re-enabling bootstrap redirects to the wrong hostname. + // Operators who explicitly set GATEWAY_PUBLIC_AGENT_URL_TEMPLATE + // want the feature off when the template is broken, not fallen- + // back to a different URL. + agentURLTemplateConfigured := cfg.PublicAgentURLTemplate != "" + if agentURLTemplateConfigured { agentURL := strings.ReplaceAll(cfg.PublicAgentURLTemplate, "{id}", gatewayID) assignedHost = hostFromURL(agentURL) if assignedHost == "" { @@ -218,10 +228,13 @@ func main() { logger.Warn("GATEWAY_PUBLIC_TERMINAL_URL_TEMPLATE resolved to a URL with no host — terminal session registration disabled on this gateway; check for unset env vars in the template", "template", cfg.PublicTerminalURLTemplate, "resolved", terminalURL) } else { - // If no agent URL template was set, fall back to deriving the - // agent redirect hostname from the terminal URL (legacy - // single-hostname mode). - if assignedHost == "" { + // Legacy single-hostname fallback: only substitute the + // terminal host when NO agent template was configured. + // If the operator set the agent template and it + // resolved to a broken URL, respect their intent to + // disable bootstrap redirects rather than silently + // masking the misconfiguration with the TTY host. + if assignedHost == "" && !agentURLTemplateConfigured { assignedHost = terminalHost } @@ -313,9 +326,16 @@ func main() { TTYCertResolver: cfg.TraefikTTYCertResolver, } - if err := gatewayReg.PublishTraefikRoute( - context.Background(), gatewayID, traefikCfg, registry.DefaultGatewayTTL, - ); err != nil { + // Bound the publish with shutdownCtx + a 5s timeout so a + // slow Redis can't stall startup, and the same bound is + // used on refresh so shutdown is prompt even during a + // hung publish. + publishCtx, cancelPublish := context.WithTimeout(shutdownCtx, 5*time.Second) + err := gatewayReg.PublishTraefikRoute( + publishCtx, gatewayID, traefikCfg, registry.DefaultGatewayTTL, + ) + cancelPublish() + if err != nil { // Fail-open: the refresh goroutine below retries on // the normal cadence, so a transient publish failure // at startup recovers within one refresh interval. @@ -338,9 +358,12 @@ func main() { for { select { case <-ticker.C: - if err := gatewayReg.PublishTraefikRoute( - context.Background(), gatewayID, traefikCfg, registry.DefaultGatewayTTL, - ); err != nil { + refreshCtx, cancelRefresh := context.WithTimeout(traefikRefreshCtx, 5*time.Second) + err := gatewayReg.PublishTraefikRoute( + refreshCtx, gatewayID, traefikCfg, registry.DefaultGatewayTTL, + ) + cancelRefresh() + if err != nil { logger.Warn("failed to refresh Traefik routing config", "error", err) } case <-traefikRefreshCtx.Done(): @@ -400,9 +423,16 @@ func main() { } } if internalURL != "" { - if err := gatewayReg.RegisterGatewayInternal( - context.Background(), gatewayID, internalURL, registry.DefaultGatewayTTL, - ); err != nil { + // Bound both the initial register and the periodic + // refresh with shutdownCtx + 5s so a slow Redis can't + // delay shutdown or pile up goroutines waiting on the + // registry during degraded Valkey health. + registerCtx, cancelRegister := context.WithTimeout(shutdownCtx, 5*time.Second) + err := gatewayReg.RegisterGatewayInternal( + registerCtx, gatewayID, internalURL, registry.DefaultGatewayTTL, + ) + cancelRegister() + if err != nil { logger.Warn("failed to register gateway internal URL", "error", err) } internalRefreshCtx, stopInternalRefresh := context.WithCancel(shutdownCtx) @@ -413,9 +443,12 @@ func main() { for { select { case <-ticker.C: - if err := gatewayReg.RegisterGatewayInternal( - context.Background(), gatewayID, internalURL, registry.DefaultGatewayTTL, - ); err != nil { + refreshCtx, cancelRefresh := context.WithTimeout(internalRefreshCtx, 5*time.Second) + err := gatewayReg.RegisterGatewayInternal( + refreshCtx, gatewayID, internalURL, registry.DefaultGatewayTTL, + ) + cancelRefresh() + if err != nil { logger.Warn("failed to refresh gateway internal URL", "error", err) } case <-internalRefreshCtx.Done(): From e8294f6eb2a8519b9d85004c4e24cc1c74b9e270 Mon Sep 17 00:00:00 2001 From: PaulDotterer Date: Thu, 23 Apr 2026 18:42:16 +0200 Subject: [PATCH 09/11] =?UTF-8?q?fix:=20rc10=20review=20round=205=20?= =?UTF-8?q?=E2=80=94=20bound=20RegisterGateway=20+=20drop=20dead=20gateway?= =?UTF-8?q?Reg=20nil=20guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit round 5 on 7f4179a (run 1d41b4b7) posted one actionable and one nitpick, both completing patterns the previous rounds established. 1. cmd/gateway/main.go line 242 — actionable RegisterGateway's initial call still used context.Background() while its round-4 siblings PublishTraefikRoute and RegisterGatewayInternal now use shutdownCtx + 5s. Match the pattern here too — a slow/hung Valkey at startup otherwise ignores SIGTERM on the registry register path. The stop() return value keeps its own refresh-goroutine cancellation wiring, so the bound only affects the one-shot initial call. 2. cmd/gateway/main.go line 499 — nitpick After the round-3 refactor moved ensureGatewayRegistry() to fire unconditionally above the internal-URL block, gatewayReg cannot be nil by the time we reach SetGatewayRouting. The `if gatewayReg != nil` guard is dead and misleading — a reader has to re-derive the invariant. Drop the guard, leave a one-line comment recording why it's safe to call unconditionally. Targeted go tests unchanged and still green (no test surface touched this round). Part of manchtools/power-manage-server#76 review round 5. --- cmd/gateway/main.go | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/cmd/gateway/main.go b/cmd/gateway/main.go index cfdb2c49..443e7670 100644 --- a/cmd/gateway/main.go +++ b/cmd/gateway/main.go @@ -239,13 +239,23 @@ func main() { } gatewayReg = ensureGatewayRegistry() + // Match the 5s bound used on the other Redis-touching + // calls in this file (PublishTraefikRoute, + // RegisterGatewayInternal). Without it, a slow or hung + // Valkey at startup stalls gateway boot past the point + // where SIGTERM should be respected. The refresh + // goroutine stop() returned from RegisterGateway carries + // its own shutdown wiring, so the bound only affects the + // one-shot register at line 242. + registerCtx, cancelRegister := context.WithTimeout(shutdownCtx, 5*time.Second) stop, err := gatewayReg.RegisterGateway( - context.Background(), + registerCtx, gatewayID, terminalURL, registry.DefaultGatewayTTL, registry.DefaultGatewayRefreshInterval, ) + cancelRegister() if err != nil { // Fail-open: the terminal feature is optional, so a // transient registry failure at startup must not kill @@ -484,11 +494,11 @@ func main() { // Setup HTTP mux for agent connections (mTLS-protected) mux := http.NewServeMux() - // Create agent handler (always mTLS) + // Create agent handler (always mTLS). gatewayReg is always non-nil + // by this point — ensureGatewayRegistry() is called unconditionally + // in the internal-URL block above — so no guard on SetGatewayRouting. agentHandler := handler.NewAgentHandlerWithTLS(manager, aqClient, controlProxy, workerMgr, version, cfg.HeartbeatInterval, logger) - if gatewayReg != nil { - agentHandler.SetGatewayRouting(gatewayReg, gatewayID) - } + agentHandler.SetGatewayRouting(gatewayReg, gatewayID) agentHandler.SetTerminalSessions(terminalSessions) path, h := pmv1connect.NewAgentServiceHandler(agentHandler) From 69abe32a60bc39380303003d87fc804e81d6389c Mon Sep 17 00:00:00 2001 From: PaulDotterer Date: Thu, 23 Apr 2026 19:52:56 +0200 Subject: [PATCH 10/11] =?UTF-8?q?fix:=20rc10=20review=20round=206=20?= =?UTF-8?q?=E2=80=94=20IMAGE=5FTAG=20docs=20coherence=20+=20Traefik=20publ?= =?UTF-8?q?ish=20status=20in=20log?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CR review on e8294f6 posted one low-severity doc mismatch and one nitpick. Practicing the sibling-sweep discipline from the pre-push memory: grep for every IMAGE_TAG reference in the repo before fixing, and grep for every "feature enabled" log site before touching the one CR flagged. 1. deploy/.env.example IMAGE_TAG contract — CR flagged Comment said "default: latest" with a commented-out `IMAGE_TAG=latest`. compose.yml's `${IMAGE_TAG:?...}` enforces the opposite: operators who don't set IMAGE_TAG get a hard startup refusal, which is the intent. Rewrite the doc to match reality: IMAGE_TAG is required, uncommented by default with a sensible placeholder (v2026.05), with a comment explaining why the compose file enforces it. 2. deploy/deploy.sh IMAGE_TAG fallback — sibling sweep caught this Not in CR's findings but the same contract. deploy.sh reads IMAGE_TAG from the remote .env and falls back to "latest" when missing: IMAGE_TAG=$(grep -oP '(?<=^IMAGE_TAG=).*' .env || echo latest) But compose.yml refuses to start without IMAGE_TAG explicitly set. So the fallback retags images to :latest, then compose rejects them with its own less-specific error. Fail here instead with a pointer at .env.example so the operator's actual fix is visible in the first error. 3. cmd/gateway/main.go "traefik self-registration enabled" log — CR nit After a failed initial PublishTraefikRoute the Warn line above still emits. The later Info line says "enabled" unconditionally, which is true (refresh goroutine will retry) but slightly muddy — an operator skimming a Valkey-wobble startup can't tell "registered now" from "will retry in 45s" without cross-referencing the Warn. Add `initial_publish_ok` to the structured fields so the state is explicit. Sibling check: the only other "feature enabled" log is the "multi-gateway routing enabled" line at ~269, which is already correctly scoped inside the else branch of its register-error check — it only fires when RegisterGateway succeeded. No action needed there. Part of manchtools/power-manage-server#76 review round 6. --- cmd/gateway/main.go | 7 +++++++ deploy/.env.example | 8 ++++++-- deploy/deploy.sh | 10 ++++++++-- 3 files changed, 21 insertions(+), 4 deletions(-) diff --git a/cmd/gateway/main.go b/cmd/gateway/main.go index 443e7670..9a92cb65 100644 --- a/cmd/gateway/main.go +++ b/cmd/gateway/main.go @@ -393,8 +393,15 @@ func main() { } }() + // Surface whether the initial publish succeeded. After a + // Valkey wobble the refresh loop will self-heal within + // one DefaultGatewayRefreshInterval, but an operator + // reading the log right after startup should be able to + // tell "registered now" from "will retry in N seconds" + // without having to cross-reference the Warn line above. logger.Info("traefik self-registration enabled", "gateway_id", gatewayID, + "initial_publish_ok", err == nil, "mtls_host", cfg.TraefikMTLSHost, "mtls_backend", mtlsBackend, "tty_host", cfg.TraefikTTYHost, diff --git a/deploy/.env.example b/deploy/.env.example index 22c3ff87..723a1da6 100644 --- a/deploy/.env.example +++ b/deploy/.env.example @@ -151,8 +151,12 @@ LOG_LEVEL=info # CONTROL_SSH_ACCESS_FOR_ALL=false # --- Docker Images ---------------------------------------------------------- -# Tag for control, gateway, and indexer images (default: latest) -# IMAGE_TAG=latest +# Required: pin the image tag for control, gateway, and indexer to a +# specific release (e.g. v2026.05). compose.yml enforces this via +# ${IMAGE_TAG:?...}; without this line every `docker compose up` refuses +# to start with a clear error message rather than silently rolling +# forward on whatever `:latest` points to today. +IMAGE_TAG=v2026.05 # --- Gateway scaling (Traefik Redis KV self-registration) ------------------- # The reference compose stack auto-configures Traefik self-registration with diff --git a/deploy/deploy.sh b/deploy/deploy.sh index 649847b3..42328ac0 100755 --- a/deploy/deploy.sh +++ b/deploy/deploy.sh @@ -202,9 +202,15 @@ log_info "Transfer complete" log_step "Loading images and restarting services on $SSH_HOST..." # Build the remote commands -# Read IMAGE_TAG from server's .env so we retag images to match what compose expects +# Read IMAGE_TAG from the server's .env and retag images to match what +# compose.yml expects. Fail hard when IMAGE_TAG is missing rather than +# falling back to :latest — compose.yml requires IMAGE_TAG explicitly +# (${IMAGE_TAG:?...}), so a silent "latest" fallback here would retag +# images compose will refuse to use anyway, just with a less clear +# error downstream. REMOTE_CMDS="cd ~/deploy && " -REMOTE_CMDS+="IMAGE_TAG=\$(grep -oP '(?<=^IMAGE_TAG=).*' .env 2>/dev/null || echo latest) && " +REMOTE_CMDS+="IMAGE_TAG=\$(grep -oP '(?<=^IMAGE_TAG=).*' .env 2>/dev/null) && " +REMOTE_CMDS+="if [ -z \"\$IMAGE_TAG\" ]; then echo '[ERROR] IMAGE_TAG missing from ~/deploy/.env on '\"\$(hostname)\"' — pin to a specific release tag (see .env.example)' >&2; exit 1; fi && " REMOTE_CMDS+="echo '[INFO] Server IMAGE_TAG='\$IMAGE_TAG && " for svc in "${SERVICES[@]}"; do REMOTE_CMDS+="echo '[INFO] Loading pm-$svc...' && " From e9d8f44662c503510fe38d0218cb0aa9542403c1 Mon Sep 17 00:00:00 2001 From: PaulDotterer Date: Thu, 23 Apr 2026 19:58:05 +0200 Subject: [PATCH 11/11] =?UTF-8?q?revert:=20restore=20${IMAGE=5FTAG:-latest?= =?UTF-8?q?}=20=E2=80=94=20:latest=20is=20a=20curated=20stable=20tag?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reverts the "IMAGE_TAG required" part of the rc10 review round 6 commit (69abe32). That change was based on a wrong assumption about the release-engineering workflow: I treated ${IMAGE_TAG:-latest} as a silent-rolling-forward footgun, but the CI tag contract here is deliberate: :latest — latest stable release (CI promotes only after a stable version is cut; does not advance on every rc) :latest-rc — latest pre-release / release candidate vYYYY.MM — pinned-release specific tag So :latest is a curated pointer, not an accidental one. Requiring explicit pinning broke quickstart (docker compose up on a fresh checkout) and CI-reference-stack workflows for zero real gain — the supply-chain concern I was mitigating doesn't apply to a promoted-by- CI floating tag. Restores: * compose.yml: ${IMAGE_TAG:-latest} on all three image lines * .env.example: commented-out IMAGE_TAG=latest default, with a comment block that documents all three tag shapes (latest, latest-rc, vYYYY.MM) so operators pick the right one for their stance on rollbacks * deploy.sh: |- || echo latest fallback restored; matches the compose default rather than failing hard on unset IMAGE_TAG Memory saved under project_image_tag_contract.md so this doesn't get re-litigated in a future review round. The Traefik "initial_publish_ok" log field from 69abe32 stays — that's an unrelated nit fix on its own merits. --- deploy/.env.example | 14 ++++++++------ deploy/compose.yml | 6 +++--- deploy/deploy.sh | 13 +++++-------- 3 files changed, 16 insertions(+), 17 deletions(-) diff --git a/deploy/.env.example b/deploy/.env.example index 723a1da6..0853b38d 100644 --- a/deploy/.env.example +++ b/deploy/.env.example @@ -151,12 +151,14 @@ LOG_LEVEL=info # CONTROL_SSH_ACCESS_FOR_ALL=false # --- Docker Images ---------------------------------------------------------- -# Required: pin the image tag for control, gateway, and indexer to a -# specific release (e.g. v2026.05). compose.yml enforces this via -# ${IMAGE_TAG:?...}; without this line every `docker compose up` refuses -# to start with a clear error message rather than silently rolling -# forward on whatever `:latest` points to today. -IMAGE_TAG=v2026.05 +# Tag for control, gateway, and indexer images. +# latest — latest stable release (default; CI promotes to this +# tag only after a stable release is cut) +# latest-rc — latest pre-release / rc, use this to track the +# current release-candidate line +# v2026.05 — pin to a specific release for stricter rollback +# semantics +# IMAGE_TAG=latest # --- Gateway scaling (Traefik Redis KV self-registration) ------------------- # The reference compose stack auto-configures Traefik self-registration with diff --git a/deploy/compose.yml b/deploy/compose.yml index dd9e03e8..7b53951e 100644 --- a/deploy/compose.yml +++ b/deploy/compose.yml @@ -99,7 +99,7 @@ services: - internal control: - image: ghcr.io/manchtools/power-manage-control:${IMAGE_TAG:?IMAGE_TAG must be set in .env (pin to a specific release tag like v2026.05 — defaulting to \"latest\" caused silent upgrades on every docker compose pull)} + image: ghcr.io/manchtools/power-manage-control:${IMAGE_TAG:-latest} container_name: pm-control restart: unless-stopped depends_on: @@ -155,7 +155,7 @@ services: - internal indexer: - image: ghcr.io/manchtools/power-manage-indexer:${IMAGE_TAG:?IMAGE_TAG must be set in .env (pin to a specific release tag like v2026.05 — defaulting to \"latest\" caused silent upgrades on every docker compose pull)} + image: ghcr.io/manchtools/power-manage-indexer:${IMAGE_TAG:-latest} container_name: pm-indexer restart: unless-stopped depends_on: @@ -182,7 +182,7 @@ services: # No container_name: scaling to N replicas needs Compose to auto-name # containers (pm-server-gateway-1, -2, ...). Self-registration into # Traefik's Redis KV means no per-replica Traefik labels are needed. - image: ghcr.io/manchtools/power-manage-gateway:${IMAGE_TAG:?IMAGE_TAG must be set in .env (pin to a specific release tag like v2026.05 — defaulting to \"latest\" caused silent upgrades on every docker compose pull)} + image: ghcr.io/manchtools/power-manage-gateway:${IMAGE_TAG:-latest} restart: unless-stopped depends_on: valkey: diff --git a/deploy/deploy.sh b/deploy/deploy.sh index 42328ac0..e94efc08 100755 --- a/deploy/deploy.sh +++ b/deploy/deploy.sh @@ -202,15 +202,12 @@ log_info "Transfer complete" log_step "Loading images and restarting services on $SSH_HOST..." # Build the remote commands -# Read IMAGE_TAG from the server's .env and retag images to match what -# compose.yml expects. Fail hard when IMAGE_TAG is missing rather than -# falling back to :latest — compose.yml requires IMAGE_TAG explicitly -# (${IMAGE_TAG:?...}), so a silent "latest" fallback here would retag -# images compose will refuse to use anyway, just with a less clear -# error downstream. +# Read IMAGE_TAG from the server's .env so we retag images to match +# what compose expects. Falls back to "latest" when unset — matches +# compose.yml's ${IMAGE_TAG:-latest} default, which points at the +# curated latest-stable tag CI promotes after a release is cut. REMOTE_CMDS="cd ~/deploy && " -REMOTE_CMDS+="IMAGE_TAG=\$(grep -oP '(?<=^IMAGE_TAG=).*' .env 2>/dev/null) && " -REMOTE_CMDS+="if [ -z \"\$IMAGE_TAG\" ]; then echo '[ERROR] IMAGE_TAG missing from ~/deploy/.env on '\"\$(hostname)\"' — pin to a specific release tag (see .env.example)' >&2; exit 1; fi && " +REMOTE_CMDS+="IMAGE_TAG=\$(grep -oP '(?<=^IMAGE_TAG=).*' .env 2>/dev/null || echo latest) && " REMOTE_CMDS+="echo '[INFO] Server IMAGE_TAG='\$IMAGE_TAG && " for svc in "${SERVICES[@]}"; do REMOTE_CMDS+="echo '[INFO] Loading pm-$svc...' && "