From b200955ab867eab735648de530fd4a315e4d0946 Mon Sep 17 00:00:00 2001 From: "courtmanr@gmail.com" Date: Mon, 27 Jul 2026 13:16:52 +0100 Subject: [PATCH] Backfill monitoring contract for the TrueNAS handshake-redirect upgrade MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit bf24a9a9c changed truenas runtime transport behavior without the canonical paperwork, so its governance run failed the completion guard. Document the redirect rule in the monitoring subsystem contract — a plaintext handshake redirect never wakes the REST bridge; a same-host https redirect upgrades once to wss and the upgrade is scheme-only and monotonic — and move the Issue1631 regression tests into transport_test.go, which is a registered verification artifact for the truenas runtime path policy. Co-Authored-By: Claude Fable 5 --- .../v6/internal/subsystems/monitoring.md | 12 ++ .../truenas/issue1631_http_redirect_test.go | 136 ------------------ internal/truenas/transport_test.go | 125 ++++++++++++++++ 3 files changed, 137 insertions(+), 136 deletions(-) delete mode 100644 internal/truenas/issue1631_http_redirect_test.go diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index 44d4685e54..e2f31669be 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -605,6 +605,18 @@ must never prevent the remaining configured hosts from being suppressed. CORE/FreeNAS. Unknown or current versions fail closed. The decision and persistent socket belong to one configured client, so reconnects or legacy negotiation for one appliance cannot alter another appliance. + A plaintext `ws://` handshake answered with a redirect is not an + unsupported endpoint and must never wake the REST bridge. When the + redirect target is an https URL on the same host — TrueNAS's HTTP to + HTTPS redirect, or an equivalent proxy — the dial retries once over TLS + with the entry's verification settings, and the upgraded `wss://` + endpoint becomes the client's endpoint for its remaining lifetime + (#1631). The upgrade is scheme-only and monotonic: cross-host redirects, + downgrades to plaintext, and relative targets fail closed with the + redirect target named in the error, so a redirect can move a connection + to TLS but never to another appliance or back to plaintext. Regression + coverage: the `Issue1631` handshake-redirect tests in + `internal/truenas/transport_test.go`. Current API-key authentication uses `auth.login_ex` with the key owner's username and `API_KEY_PLAIN`; password authentication uses `PASSWORD_PLAIN`. Username-less stored API keys may use the deprecated diff --git a/internal/truenas/issue1631_http_redirect_test.go b/internal/truenas/issue1631_http_redirect_test.go deleted file mode 100644 index 4c3762d705..0000000000 --- a/internal/truenas/issue1631_http_redirect_test.go +++ /dev/null @@ -1,136 +0,0 @@ -package truenas - -// Pins the issue #1631 regression: the v6.1.2 JSON-RPC migration broke -// http-configured appliances behind TrueNAS's HTTP -> HTTPS redirect. The -// REST transport used before followed the redirect transparently; the -// websocket handshake fails with status=302 instead, so the client must -// upgrade a same-host https redirect to wss itself, and refuse anything -// that would move the connection to another host. - -import ( - "context" - "net/http" - "net/http/httptest" - "net/url" - "strconv" - "strings" - "testing" - "time" -) - -func newHTTPRedirectServer(t *testing.T, location func() string, status int) *httptest.Server { - t.Helper() - server := httptest.NewServer(http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) { - http.Redirect(writer, request, location(), status) - })) - t.Cleanup(server.Close) - return server -} - -func redirectFixtureClient(t *testing.T, redirectURL string) *Client { - t.Helper() - parsed, err := url.Parse(redirectURL) - if err != nil { - t.Fatalf("parse redirect server URL %q: %v", redirectURL, err) - } - port, err := strconv.Atoi(parsed.Port()) - if err != nil { - t.Fatalf("parse redirect server port from %q: %v", redirectURL, err) - } - client, err := NewClient(ClientConfig{ - Host: "http://" + parsed.Hostname(), - Port: port, - Username: "pulse-readonly", - APIKey: "readonly-key", - InsecureSkipVerify: true, - Timeout: 5 * time.Second, - }) - if err != nil { - t.Fatalf("NewClient() error = %v", err) - } - t.Cleanup(client.Close) - return client -} - -func TestIssue1631HTTPRedirectUpgradesHandshakeToTLS(t *testing.T) { - fixture := newProtocolFixture(t, func(_ int, request trueNASRPCRequest) protocolFixtureReply { - if request.Method == "auth.login_ex" { - return protocolFixtureReply{result: map[string]any{"response_type": "SUCCESS", "user_info": nil}} - } - t.Errorf("unexpected rpc method %q", request.Method) - return protocolFixtureReply{close: true} - }, nil) - - redirect := newHTTPRedirectServer(t, func() string { - return fixture.server.URL + "/api/current" - }, http.StatusFound) - client := redirectFixtureClient(t, redirect.URL) - - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - defer cancel() - mode, err := client.ensureTransport(ctx) - if err != nil { - t.Fatalf("ensureTransport() error = %v", err) - } - if mode != TransportJSONRPC { - t.Fatalf("ensureTransport() mode = %s, want %s", mode, TransportJSONRPC) - } - - status := client.TransportStatus() - if !status.TLS { - t.Errorf("TransportStatus().TLS = false, want true after https upgrade") - } - if !strings.HasPrefix(status.Endpoint, "wss://") { - t.Errorf("TransportStatus().Endpoint = %q, want wss:// endpoint", status.Endpoint) - } - if !client.config.UseHTTPS { - t.Errorf("config.UseHTTPS = false, want true after https upgrade") - } - if fixture.sessions.Load() == 0 { - t.Errorf("TLS fixture saw no websocket sessions; upgrade retry never landed") - } -} - -func TestIssue1631CrossHostRedirectIsRefused(t *testing.T) { - redirect := newHTTPRedirectServer(t, func() string { - return "https://other-appliance.example/api/current" - }, http.StatusFound) - client := redirectFixtureClient(t, redirect.URL) - - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - defer cancel() - _, err := client.ensureTransport(ctx) - if err == nil { - t.Fatal("ensureTransport() error = nil, want cross-host redirect refusal") - } - if !strings.Contains(err.Error(), "redirected to") || !strings.Contains(err.Error(), "other-appliance.example") { - t.Errorf("ensureTransport() error = %q, want actionable message naming the redirect target", err) - } - if status := client.TransportStatus(); status.TLS { - t.Errorf("TransportStatus().TLS = true, want false when redirect is refused") - } -} - -func TestIssue1631HTTPSUpgradeTargetRules(t *testing.T) { - cases := []struct { - name string - current string - location string - want string - ok bool - }{ - {"same host default port", "ws://nas.local:80/api/current", "https://nas.local/ui/", "wss://nas.local:443/api/current", true}, - {"same host explicit port", "ws://nas.local:80/api/current", "https://nas.local:8443/", "wss://nas.local:8443/api/current", true}, - {"cross host", "ws://nas.local:80/api/current", "https://evil.example/", "", false}, - {"relative location", "ws://nas.local:80/api/current", "/ui/", "", false}, - {"downgrade from wss", "wss://nas.local:443/api/current", "https://nas.local/", "", false}, - {"http location", "ws://nas.local:80/api/current", "http://nas.local:8080/", "", false}, - {"empty location", "ws://nas.local:80/api/current", "", "", false}, - } - for _, tc := range cases { - got, ok := httpsUpgradeTarget(tc.current, tc.location) - if ok != tc.ok || got != tc.want { - t.Errorf("%s: httpsUpgradeTarget(%q, %q) = (%q, %v), want (%q, %v)", tc.name, tc.current, tc.location, got, ok, tc.want, tc.ok) - } - } -} diff --git a/internal/truenas/transport_test.go b/internal/truenas/transport_test.go index 4b2f57cdb3..abdcea1a8a 100644 --- a/internal/truenas/transport_test.go +++ b/internal/truenas/transport_test.go @@ -6,10 +6,13 @@ import ( "errors" "net/http" "net/http/httptest" + "net/url" + "strconv" "strings" "sync" "sync/atomic" "testing" + "time" "github.com/gorilla/websocket" ) @@ -796,3 +799,125 @@ func TestTransportDiagnosticsAreConnectionLocalAndSecretFree(t *testing.T) { t.Fatalf("failed authentication selected a transport: %+v", status) } } + +// Issue #1631 handshake-redirect coverage: the v6.1.2 JSON-RPC migration +// broke http-configured appliances behind TrueNAS's HTTP -> HTTPS redirect. +// The websocket dial must upgrade a same-host https redirect to wss itself +// and refuse anything that would move the connection to another host. + +func newHTTPRedirectServer(t *testing.T, location func() string, status int) *httptest.Server { + t.Helper() + server := httptest.NewServer(http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) { + http.Redirect(writer, request, location(), status) + })) + t.Cleanup(server.Close) + return server +} + +func redirectFixtureClient(t *testing.T, redirectURL string) *Client { + t.Helper() + parsed, err := url.Parse(redirectURL) + if err != nil { + t.Fatalf("parse redirect server URL %q: %v", redirectURL, err) + } + port, err := strconv.Atoi(parsed.Port()) + if err != nil { + t.Fatalf("parse redirect server port from %q: %v", redirectURL, err) + } + client, err := NewClient(ClientConfig{ + Host: "http://" + parsed.Hostname(), + Port: port, + Username: "pulse-readonly", + APIKey: "readonly-key", + InsecureSkipVerify: true, + Timeout: 5 * time.Second, + }) + if err != nil { + t.Fatalf("NewClient() error = %v", err) + } + t.Cleanup(client.Close) + return client +} + +func TestIssue1631HTTPRedirectUpgradesHandshakeToTLS(t *testing.T) { + fixture := newProtocolFixture(t, func(_ int, request trueNASRPCRequest) protocolFixtureReply { + if request.Method == "auth.login_ex" { + return protocolFixtureReply{result: map[string]any{"response_type": "SUCCESS", "user_info": nil}} + } + t.Errorf("unexpected rpc method %q", request.Method) + return protocolFixtureReply{close: true} + }, nil) + + redirect := newHTTPRedirectServer(t, func() string { + return fixture.server.URL + "/api/current" + }, http.StatusFound) + client := redirectFixtureClient(t, redirect.URL) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + mode, err := client.ensureTransport(ctx) + if err != nil { + t.Fatalf("ensureTransport() error = %v", err) + } + if mode != TransportJSONRPC { + t.Fatalf("ensureTransport() mode = %s, want %s", mode, TransportJSONRPC) + } + + status := client.TransportStatus() + if !status.TLS { + t.Errorf("TransportStatus().TLS = false, want true after https upgrade") + } + if !strings.HasPrefix(status.Endpoint, "wss://") { + t.Errorf("TransportStatus().Endpoint = %q, want wss:// endpoint", status.Endpoint) + } + if !client.config.UseHTTPS { + t.Errorf("config.UseHTTPS = false, want true after https upgrade") + } + if fixture.sessions.Load() == 0 { + t.Errorf("TLS fixture saw no websocket sessions; upgrade retry never landed") + } +} + +func TestIssue1631CrossHostRedirectIsRefused(t *testing.T) { + redirect := newHTTPRedirectServer(t, func() string { + return "https://other-appliance.example/api/current" + }, http.StatusFound) + client := redirectFixtureClient(t, redirect.URL) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + _, err := client.ensureTransport(ctx) + if err == nil { + t.Fatal("ensureTransport() error = nil, want cross-host redirect refusal") + } + if !strings.Contains(err.Error(), "redirected to") || !strings.Contains(err.Error(), "other-appliance.example") { + t.Errorf("ensureTransport() error = %q, want actionable message naming the redirect target", err) + } + if status := client.TransportStatus(); status.TLS { + t.Errorf("TransportStatus().TLS = true, want false when redirect is refused") + } +} + +func TestIssue1631HTTPSUpgradeTargetRules(t *testing.T) { + cases := []struct { + name string + current string + location string + want string + ok bool + }{ + {"same host default port", "ws://nas.local:80/api/current", "https://nas.local/ui/", "wss://nas.local:443/api/current", true}, + {"same host explicit port", "ws://nas.local:80/api/current", "https://nas.local:8443/", "wss://nas.local:8443/api/current", true}, + {"cross host", "ws://nas.local:80/api/current", "https://evil.example/", "", false}, + {"relative location", "ws://nas.local:80/api/current", "/ui/", "", false}, + {"downgrade from wss", "wss://nas.local:443/api/current", "https://nas.local/", "", false}, + {"http location", "ws://nas.local:80/api/current", "http://nas.local:8080/", "", false}, + {"empty location", "ws://nas.local:80/api/current", "", "", false}, + } + for _, tc := range cases { + got, ok := httpsUpgradeTarget(tc.current, tc.location) + if ok != tc.ok || got != tc.want { + t.Errorf("%s: httpsUpgradeTarget(%q, %q) = (%q, %v), want (%q, %v)", tc.name, tc.current, tc.location, got, ok, tc.want, tc.ok) + } + } +}