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) }