From ae19fcf5d674ea356655849dbe7d9d1ea858ee44 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Fri, 23 Oct 2026 13:46:51 +0100 Subject: [PATCH] Preserve PVE authentication endings while reading responses A password login can end while its response is being read: the caller cancels, its deadline passes, or the client's own login timeout expires. Go's HTTP transport can still hand back the server's answer to that abandoned request. The construction fixture answers a departed client with an empty 200, which surfaced as "invalid authentication response" and failed TestClientConstructionHonoursContext and its client-timeout twin intermittently in the required backend check. The shared password exchange now reports the ending instead of diagnosing that answer, keeps transport errors that already carry the ending, and starts no form attempt after an ended JSON refusal. This mirrors the PBS client fix from 18 October. Renewal shares the exchange, so the same rule applies when its own bounded timeout ends a login. Complete sessions are still accepted, and construction still discards a client whose caller has ended. A new control ends the login during response reading, by caller cancellation and by a simulated login deadline, for every unusable answer; without this change all of its cases fail. It also checks that both encoding attempts carry the one login budget the client timeout sets. The TLS fixture now aborts a request whose client has left rather than answering it with an empty 200. --- .../v6/internal/subsystems/monitoring.md | 20 +++ pkg/proxmox/client.go | 9 +- .../client_construction_context_test.go | 114 +++++++++++++++++- .../host_diagnostics_regression_test.go | 4 + 4 files changed, 145 insertions(+), 2 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index 9da4bf99c3..7d5e9d3f81 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -4260,6 +4260,26 @@ during response reading for JSON/form, complete and incomplete success, encoding and credential refusal, and body-read failure. The real TLS constructor cancellation/deadline and shared-renewal controls remain separate. +### PVE authentication keeps an ended login's outcome + +When a PVE password exchange ends while its response is being read (the caller +cancels, the caller's deadline passes, or the client's own login timeout +expires), the login now reports that context error. The server's answer to the +abandoned exchange, such as an empty 200 or a refusal, is no longer reported as +a malformed session or a credential refusal, and an ended JSON refusal does not +start a form attempt. Transport errors that already report the ending keep +their wording. A complete session is still accepted, and construction still +discards a client whose caller has ended. Shared renewal gets the same rule +through the common password exchange, where only its own bounded timeout can +end the detached login. `TestClientEndedAuthResponseOwnsErrorAndSession` ends +the login during response reading, by caller cancellation and by a simulated +login deadline, for JSON/form, empty and invalid success, encoding and +credential refusal, and body-read failure. It also checks that both encoding +attempts carry the one login budget. The real TLS construction fixture now +aborts a request whose client has left instead of answering it with an empty +200; reading that answer as a malformed session is what made +`TestClientConstructionHonoursContext` fail intermittently in CI. + ### AI Discovery removed (2026-10-11) The [AI runtime retirement boundary](ai-runtime.md#ai-discovery-removed-2026-10-11) diff --git a/pkg/proxmox/client.go b/pkg/proxmox/client.go index eabbed51fa..20bca9088b 100644 --- a/pkg/proxmox/client.go +++ b/pkg/proxmox/client.go @@ -446,11 +446,18 @@ func (c *Client) authenticate(ctx context.Context) error { return c.authenticatePassword(loginCtx, auth.user+"@"+auth.realm) } +// A response can finish just as ctx ends. The server's answer to that +// abandoned exchange, such as an empty or refused response, is not a +// credential diagnosis: an ended exchange tries no other encoding and reports +// the ending, keeping a transport error that already carries it. func (c *Client) authenticatePassword(ctx context.Context, username string) error { err := c.authenticateJSON(ctx, username, c.config.Password) - if shouldFallbackToForm(err) { + if ctx.Err() == nil && shouldFallbackToForm(err) { err = c.authenticateForm(ctx, username, c.config.Password) } + if ended := ctx.Err(); err != nil && ended != nil && !errors.Is(err, ended) { + return ended + } return err } diff --git a/pkg/proxmox/client_construction_context_test.go b/pkg/proxmox/client_construction_context_test.go index 5ea2b7f1cf..58ce7d4182 100644 --- a/pkg/proxmox/client_construction_context_test.go +++ b/pkg/proxmox/client_construction_context_test.go @@ -77,7 +77,8 @@ func testClientConstructionHonoursContext(t *testing.T, endings ...string) { select { case <-r.Context().Done(): close(requestEnded) - return + // Abort: returning would answer the departed client with an empty 200. + panic(http.ErrAbortHandler) case <-release: } } @@ -169,3 +170,114 @@ func testClientConstructionHonoursContext(t *testing.T, endings ...string) { } } } + +// End the login while its response is consumed, after HTTP has already +// returned an unusable 200 or a refusal. This reproduces the constructor race +// without scheduler timing. +type endingAuthBody struct { + io.Reader + end func() +} + +func (r endingAuthBody) Read(p []byte) (int, error) { + r.end() + return r.Reader.Read(p) +} + +// The caller's deadline and the client's own login timeout both reach the +// password exchange as a login context ending with DeadlineExceeded. The +// response read ends this one, so no wall-clock timer races the answer. +type endedLoginDeadline struct { + context.Context + done <-chan struct{} +} + +func (c endedLoginDeadline) Done() <-chan struct{} { return c.done } + +func (c endedLoginDeadline) Err() error { + select { + case <-c.done: + return context.DeadlineExceeded + default: + return nil + } +} + +func testClientEndedAuthResponseOwnsErrorAndSession(t *testing.T) { + const good = `{"data":{"ticket":"recovered-ticket","CSRFPreventionToken":"recovered-csrf"}}` + for _, ending := range []string{"cancel", "deadline"} { + for _, encoding := range []string{"json", "form"} { + for _, outcome := range []struct { + name, body string + status int + readError bool + }{ + {"empty-success", ``, http.StatusOK, false}, + {"invalid-success", `{}`, http.StatusOK, false}, + {"encoding-refusal", `{}`, http.StatusUnsupportedMediaType, false}, + {"credential-refusal", `{}`, http.StatusUnauthorized, false}, + {"body-read-failure", good, http.StatusOK, true}, + } { + t.Run(ending+"/"+encoding+"/"+outcome.name, func(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + deadline := make(chan struct{}) + end, want := func() { cancel() }, context.Canceled + if ending == "deadline" { + end, want = sync.OnceFunc(func() { close(deadline) }), context.DeadlineExceeded + } + calls, recovering := 0, false + var budget time.Time + c := &Client{ + baseURL: "https://pve.fixture.invalid/api2/json", + config: ClientConfig{Password: "fixture-password"}, + auth: auth{user: "fixture", realm: "pam"}, + } + c.httpClient = &http.Client{Transport: roundTripperFunc(func(r *http.Request) (*http.Response, error) { + calls++ + if ending == "cancel" && !recovering { + // authenticate gives every encoding attempt one login budget. + deadline, ok := r.Context().Deadline() + if !ok || (!budget.IsZero() && !deadline.Equal(budget)) { + t.Error("login attempts lost their shared login budget") + } + budget = deadline + } + if encoding == "form" && r.Header.Get("Content-Type") == "application/json" { + return &http.Response{StatusCode: http.StatusUnsupportedMediaType, Body: io.NopCloser(strings.NewReader(`{}`)), ContentLength: -1}, nil + } + if recovering { + return &http.Response{StatusCode: http.StatusOK, Body: io.NopCloser(strings.NewReader(good)), ContentLength: -1}, nil + } + var body io.Reader = strings.NewReader(outcome.body) + if outcome.readError { + body = sessionAdmissionReadError{body} + } + return &http.Response{StatusCode: outcome.status, Body: io.NopCloser(endingAuthBody{body, end}), ContentLength: -1}, nil + })} + before, generation := c.authSnapshot() + var err error + if ending == "deadline" { + err = c.authenticatePassword(endedLoginDeadline{context.Background(), deadline}, "fixture@pam") + } else { + err = c.authenticate(ctx) + } + if after, afterGeneration := c.authSnapshot(); !errors.Is(err, want) || after != before || afterGeneration != generation { + t.Errorf("ended login response lost its ending or changed the session: error=%v", err) + } + wantCalls := 1 + if encoding == "form" { + wantCalls = 2 + } + if calls != wantCalls { + t.Errorf("ended login tried another encoding: calls=%d", calls) + } + recovering = true + if err := c.authenticate(context.Background()); err != nil || c.auth.ticket != "recovered-ticket" || c.auth.csrfToken != "recovered-csrf" || calls != 2*wantCalls { + t.Errorf("distinct later login did not recover: error=%v calls=%d", err, calls) + } + }) + } + } + } +} diff --git a/pkg/proxmox/host_diagnostics_regression_test.go b/pkg/proxmox/host_diagnostics_regression_test.go index 2834a3f914..27e1a81c2d 100644 --- a/pkg/proxmox/host_diagnostics_regression_test.go +++ b/pkg/proxmox/host_diagnostics_regression_test.go @@ -167,6 +167,10 @@ func TestClientConstructionHonoursClientTimeout(t *testing.T) { testClientConstructionHonoursContext(t, "client-timeout") } +func TestClientEndedAuthResponseOwnsErrorAndSession(t *testing.T) { + testClientEndedAuthResponseOwnsErrorAndSession(t) +} + func TestClusterCancelledCallerStartsNoSession(t *testing.T) { testClusterCancelledCallerStartsNoSession(t) }