From 79225fb7e99ccf730621112cdac1fdecbd43f9c9 Mon Sep 17 00:00:00 2001 From: Adrian Dombeck Date: Thu, 25 Jun 2026 17:06:11 +0200 Subject: [PATCH 1/2] pam/gdm: Ignore GDM echo of our own auth mode selection Device authentication through GDM could hang on the QR screen even after the user completed the browser flow. The QR screen showed one device code while the broker kept polling for a different one, so the authorization the user granted was never observed and the login stalled until the device code expired. When an auth mode is auto-selected, the adapter both calls SelectAuthenticationMode (minting a device code and starting the poll) and tells GDM to select that mode. GDM echoes the selection back in its next poll, and the adapter treated that echo as a fresh selection, issuing a second SelectAuthenticationMode that minted a new device code and orphaned the first poll. Track the selection we send to GDM and drop its echo. The suppression is a one-shot consumed by the first matching echo, and is also cleared on any stage change, so a genuine re-selection of the same mode (the user navigating back to auth mode selection and picking it again) is still honored. A pure stage-change reset raced with the echo in the change-password flow; a pure one-shot swallowed legitimate re-selection of an auto-selected single mode. Combining both covers each gap. Co-Authored-By: Claude Opus 4.8 --- pam/internal/adapter/gdmmodel.go | 31 +++- .../adapter/gdmmodel_authmode_echo_test.go | 135 ++++++++++++++++++ pam/internal/adapter/gdmmodel_test.go | 30 +++- 3 files changed, 192 insertions(+), 4 deletions(-) create mode 100644 pam/internal/adapter/gdmmodel_authmode_echo_test.go diff --git a/pam/internal/adapter/gdmmodel.go b/pam/internal/adapter/gdmmodel.go index 3cfd7ce83f..38010a2c08 100644 --- a/pam/internal/adapter/gdmmodel.go +++ b/pam/internal/adapter/gdmmodel.go @@ -34,6 +34,14 @@ type gdmModel struct { // further conversation with GDM should happen. conversationsStopped bool stoppingConversations bool + + // pendingEchoAuthModeID is the auth mode we last told GDM to select and + // whose echo we still expect back. GDM echoes our selection in its next + // poll; acting on that echo would issue a second SelectAuthenticationMode + // RPC (and, for device auth, mint a second device code that orphans the + // in-flight poll). It is consumed (cleared) by the first matching echo, so + // a later genuine re-selection of the same mode is still honored. + pendingEchoAuthModeID string } type gdmPollResponse struct { @@ -111,7 +119,7 @@ func (m gdmModel) pollGdm() tea.Cmd { } } -func (m gdmModel) handlePollResponse(gdmPollResults []*gdm.EventData) tea.Cmd { +func (m *gdmModel) handlePollResponse(gdmPollResults []*gdm.EventData) tea.Cmd { if log.IsLevelEnabled(log.DebugLevel) { for _, result := range gdmPollResults { log.Debugf(context.TODO(), "GDM poll response: %v", result.SafeString()) @@ -141,6 +149,19 @@ func (m gdmModel) handlePollResponse(gdmPollResults []*gdm.EventData) tea.Cmd { status: pam.ErrSystem, msg: "missing auth mode id", }) } + // GDM echoes back the auth mode we just told it to select. Ignore + // that one echo to avoid issuing a duplicate SelectAuthenticationMode + // RPC (which, for device auth, mints a second device code and + // orphans the in-flight poll). This is a one-shot per selection: + // a later genuine re-selection of the same mode (the user picking + // it again) is honored because the pending echo has been consumed. + if res.AuthModeSelected.AuthModeId == m.pendingEchoAuthModeID { + log.Debugf(context.TODO(), + "Ignoring GDM auth mode selection echo for %q", + res.AuthModeSelected.AuthModeId) + m.pendingEchoAuthModeID = "" + break + } commands = append(commands, selectAuthMode(res.AuthModeSelected.AuthModeId)) case *gdm.EventData_IsAuthenticatedRequested: @@ -211,7 +232,8 @@ func (m gdmModel) Update(msg tea.Msg) (gdmModel, tea.Cmd) { switch msg := msg.(type) { case gdmPollResponse: - return m, m.handlePollResponse(msg.pollResponse) + cmd := m.handlePollResponse(msg.pollResponse) + return m, cmd case gdmPollDone: return m, tea.Sequence( @@ -219,6 +241,10 @@ func (m gdmModel) Update(msg tea.Msg) (gdmModel, tea.Cmd) { m.pollGdm()) case StageChanged: + // A genuine (re-)selection always follows a stage change into the + // authModeSelection stage, so any echo we were still expecting from a + // previous selection is no longer relevant once the stage changes. + m.pendingEchoAuthModeID = "" return m, m.changeStage(msg.Stage) case userSelected: @@ -242,6 +268,7 @@ func (m gdmModel) Update(msg tea.Msg) (gdmModel, tea.Cmd) { }) case AuthModeSelected: + m.pendingEchoAuthModeID = msg.ID return m, m.emitEvent(&gdm.EventData_AuthModeSelected{ AuthModeSelected: &gdm.Events_AuthModeSelected{AuthModeId: msg.ID}, }) diff --git a/pam/internal/adapter/gdmmodel_authmode_echo_test.go b/pam/internal/adapter/gdmmodel_authmode_echo_test.go new file mode 100644 index 0000000000..85d37f6cd3 --- /dev/null +++ b/pam/internal/adapter/gdmmodel_authmode_echo_test.go @@ -0,0 +1,135 @@ +package adapter + +import ( + "reflect" + "testing" + + "github.com/canonical/authd/pam/internal/gdm" + "github.com/canonical/authd/pam/internal/gdm_test" + "github.com/canonical/authd/pam/internal/proto" + tea "github.com/charmbracelet/bubbletea" + "github.com/stretchr/testify/require" +) + +// collectMessages runs a command and recursively flattens the batch/sequence +// messages it produces into the concrete messages they ultimately deliver. +// tea.Batch and tea.Sequence return []tea.Cmd-shaped messages whose concrete +// types are unexported, so they are detected structurally via reflection. +func collectMessages(cmd tea.Cmd) []tea.Msg { + if cmd == nil { + return nil + } + msg := cmd() + if cmds, ok := asCmdSlice(msg); ok { + var msgs []tea.Msg + for _, c := range cmds { + msgs = append(msgs, collectMessages(c)...) + } + return msgs + } + return []tea.Msg{msg} +} + +// asCmdSlice reports whether msg is a []tea.Cmd-shaped batch/sequence message +// and, if so, returns its commands. +func asCmdSlice(msg tea.Msg) ([]tea.Cmd, bool) { + v := reflect.ValueOf(msg) + if v.Kind() != reflect.Slice || v.Type().Elem() != reflect.TypeOf(tea.Cmd(nil)) { + return nil, false + } + cmdType := reflect.TypeOf(tea.Cmd(nil)) + cmds := make([]tea.Cmd, v.Len()) + for i := range cmds { + cmd, ok := v.Index(i).Convert(cmdType).Interface().(tea.Cmd) + if !ok { + return nil, false + } + cmds[i] = cmd + } + return cmds, true +} + +func containsAuthModeSelected(msgs []tea.Msg, id string) bool { + for _, msg := range msgs { + if m, ok := msg.(authModeSelected); ok && m.id == id { + return true + } + } + return false +} + +func TestGdmModelIgnoresAuthModeSelectedEcho(t *testing.T) { + t.Parallel() + + // After we select an auth mode, GDM echoes the selection back as a poll + // event. Acting on that echo would re-run SelectAuthenticationMode and, for + // device auth, mint a second device code while the poll is still on the + // first one (UDENG-8799). + m := gdmModel{} + m, _ = m.Update(AuthModeSelected{ID: "device_auth_qr"}) + require.Equal(t, "device_auth_qr", m.pendingEchoAuthModeID, + "selecting an auth mode should record the expected echo") + + echo := []*gdm.EventData{gdm_test.AuthModeSelectedEvent("device_auth_qr")} + msgs := collectMessages(m.handlePollResponse(echo)) + require.False(t, containsAuthModeSelected(msgs, "device_auth_qr"), + "echo of the just-selected auth mode must not trigger a re-selection") + require.Empty(t, m.pendingEchoAuthModeID, + "consuming the echo should clear the expected echo") +} + +func TestGdmModelActsOnAuthModeChange(t *testing.T) { + t.Parallel() + + // A genuine change to a different auth mode must still be acted on. + m := gdmModel{} + m, _ = m.Update(AuthModeSelected{ID: "device_auth_qr"}) + + change := []*gdm.EventData{gdm_test.AuthModeSelectedEvent("password")} + msgs := collectMessages(m.handlePollResponse(change)) + require.True(t, containsAuthModeSelected(msgs, "password"), + "selecting a different auth mode must trigger a re-selection") +} + +func TestGdmModelActsOnSameAuthModeReselection(t *testing.T) { + t.Parallel() + + // Suppression is a one-shot: only the immediate echo of our own selection + // is dropped. A later genuine re-selection of the same auth mode (the user + // picking it again) must be honored, because the pending echo has already + // been consumed. + m := gdmModel{} + m, _ = m.Update(AuthModeSelected{ID: "device_auth_qr"}) + + echo := []*gdm.EventData{gdm_test.AuthModeSelectedEvent("device_auth_qr")} + _ = collectMessages(m.handlePollResponse(echo)) + require.Empty(t, m.pendingEchoAuthModeID, + "the echo should have been consumed") + + reselect := []*gdm.EventData{gdm_test.AuthModeSelectedEvent("device_auth_qr")} + msgs := collectMessages(m.handlePollResponse(reselect)) + require.True(t, containsAuthModeSelected(msgs, "device_auth_qr"), + "a genuine re-selection of the same auth mode must be honored") +} + +func TestGdmModelStageChangeClearsPendingEcho(t *testing.T) { + t.Parallel() + + // A genuine re-selection always follows a stage change back into + // authModeSelection. The stage change must drop any echo we were still + // expecting, so that the re-selection is acted on instead of being + // mistaken for the (never-delivered) echo of the previous selection. + m := gdmModel{} + m, _ = m.Update(AuthModeSelected{ID: "device_auth_qr"}) + require.Equal(t, "device_auth_qr", m.pendingEchoAuthModeID) + + m, _ = m.Update(StageChanged{Stage: proto.Stage_challenge}) + m, _ = m.Update(StageChanged{Stage: proto.Stage_authModeSelection}) + require.Empty(t, m.pendingEchoAuthModeID, + "a stage change must drop a still-pending echo") + + reselect := []*gdm.EventData{gdm_test.AuthModeSelectedEvent("device_auth_qr")} + msgs := collectMessages(m.handlePollResponse(reselect)) + require.True(t, containsAuthModeSelected(msgs, "device_auth_qr"), + "re-selecting the same auth mode after a stage change must be honored") +} diff --git a/pam/internal/adapter/gdmmodel_test.go b/pam/internal/adapter/gdmmodel_test.go index b8b3c8a091..0a1db238dd 100644 --- a/pam/internal/adapter/gdmmodel_test.go +++ b/pam/internal/adapter/gdmmodel_test.go @@ -104,6 +104,7 @@ func TestGdmModel(t *testing.T) { wantExitStatus PamReturnStatus wantGdmRequests []gdm.RequestType wantGdmEvents []gdm.EventType + wantGdmEventsCount map[gdm.EventType]int wantGdmAuthRes []*authd.IAResponse wantNoGdmRequests []gdm.RequestType wantNoGdmEvents []gdm.EventType @@ -654,11 +655,16 @@ func TestGdmModel(t *testing.T) { gdm.EventType_authModesReceived, gdm.EventType_authModeSelected, gdm.EventType_uiLayoutReceived, - gdm.EventType_authModeSelected, - gdm.EventType_uiLayoutReceived, gdm.EventType_authEvent, // retry gdm.EventType_startAuthentication, }, + // One authModeSelected/uiLayoutReceived per genuine selection (the + // three password-stage cycles in wantGdmRequests). The GDM echo of + // each selection must not add extra cycles. + wantGdmEventsCount: map[gdm.EventType]int{ + gdm.EventType_authModeSelected: 3, + gdm.EventType_uiLayoutReceived: 3, + }, wantStage: proto.Stage_challenge, wantGdmAuthRes: []*authd.IAResponse{ { @@ -1238,6 +1244,15 @@ func TestGdmModel(t *testing.T) { gdm.EventType_startAuthentication, gdm.EventType_authEvent, }, + // Each genuine selection of the auth mode (the initial one and the + // re-selection after navigating back to authModeSelection) must + // produce exactly one selection cycle: the GDM echo of the + // selection must not add a third one. + wantGdmEventsCount: map[gdm.EventType]int{ + gdm.EventType_authModeSelected: 2, + gdm.EventType_uiLayoutReceived: 2, + gdm.EventType_startAuthentication: 2, + }, wantStage: proto.Stage_challenge, wantGdmAuthRes: []*authd.IAResponse{ {Access: auth.Granted}, @@ -2672,6 +2687,17 @@ func TestGdmModel(t *testing.T) { "Required events have not been received: %v vs %v", stringifySlice(tc.wantGdmEvents), stringifySlice(receivedEventTypes)) + for evType, wantN := range tc.wantGdmEventsCount { + gotN := 0 + for _, e := range receivedEventTypes { + if e == evType { + gotN++ + } + } + require.Equal(t, wantN, gotN, + "GDM event %q received %d times, want %d", evType, gotN, wantN) + } + require.Empty(t, appState.wantMessages, "Wanted messages have not all been processed") username, err := appState.pamMTx.GetItem(pam.User) From c61f309556a22fb833cc09f41ef70c0814c25af6 Mon Sep 17 00:00:00 2001 From: Adrian Dombeck Date: Thu, 25 Jun 2026 19:08:07 +0200 Subject: [PATCH 2/2] pam/adapter: Ignore stale stopAuthentication from a superseded challenge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After successful device authentication through GDM, the broker returns "next" and authd switches to the newpassword challenge so the user can set a local password. The "Create a local password" entry would sometimes not appear, leaving the login stuck on the greeter until the device code expired (intermittent; see #1424, #1414). Switching auth modes tears down the current challenge via authenticationModel.Reset(), which schedules cancelIsAuthenticated() — an asynchronous command that emits stopAuthentication once the in-flight IsAuthenticated call has been cancelled. The new challenge is composed and started in the meantime. When the stale stopAuthentication from the previous challenge's cancellation arrived after the new challenge had started, it cleared inProgress and the entry was never rendered. Stamp each stopAuthentication with the challenge generation current when it was scheduled, bump the generation when a new challenge is composed, and ignore a stop whose generation no longer matches. A stop belonging to a superseded challenge can no longer tear down the current one. The exact-event-count assertions added alongside the GDM echo suppression are removed from TestGdmModel: they race with the test conversation handler, which delivers echo and stage-change events concurrently with the model's polls. The invariant they checked (the echo of a selection must not add an extra selection cycle) is covered deterministically in gdmmodel_authmode_echo_test.go. Co-Authored-By: Claude Opus 4.8 --- pam/internal/adapter/authentication.go | 33 ++++++++++++++++++++++---- pam/internal/adapter/gdmmodel_test.go | 26 ++++++++------------ pam/internal/adapter/utils_test.go | 2 +- 3 files changed, 40 insertions(+), 21 deletions(-) diff --git a/pam/internal/adapter/authentication.go b/pam/internal/adapter/authentication.go index 0e0846a6d9..33ef6d99a0 100644 --- a/pam/internal/adapter/authentication.go +++ b/pam/internal/adapter/authentication.go @@ -132,6 +132,13 @@ type authenticationModel struct { currentSecret string currentLayout string + // authGen identifies the current challenge. It is bumped every time a new + // challenge is composed, so that a stopAuthentication scheduled by a + // previous challenge's cancellation can be recognised as stale and ignored + // once a new challenge has been set up. See Compose and the + // stopAuthentication handling. + authGen uint64 + authTracker *authTracker encryptionKey *rsa.PublicKey @@ -158,7 +165,12 @@ type authTracker struct { type startAuthentication struct{} // startAuthentication signals that the authentication has been stopped. -type stopAuthentication struct{} +// +// gen is the challenge generation that was current when the stop was +// scheduled. A stop whose gen no longer matches the model belongs to a +// superseded challenge and is ignored, so that cancelling a previous +// challenge cannot tear down a challenge that has started in the meantime. +type stopAuthentication struct{ gen uint64 } // errMsgToDisplay signals from an authentication form to display an error message. type errMsgToDisplay struct { @@ -195,9 +207,10 @@ func (m authenticationModel) Init() tea.Cmd { func (m *authenticationModel) cancelIsAuthenticated() tea.Cmd { authTracker := m.authTracker + gen := m.authGen return func() tea.Msg { authTracker.cancelAndWait() - return stopAuthentication{} + return stopAuthentication{gen: gen} } } @@ -224,8 +237,14 @@ func (m authenticationModel) Update(msg tea.Msg) (authModel authenticationModel, m.inProgress = true case stopAuthentication: - safeMessageDebug(msg, "current model %v, focused %v", - m.currentModel, m.Focused()) + safeMessageDebug(msg, "current model %v, focused %v, gen %d (current %d)", + m.currentModel, m.Focused(), msg.gen, m.authGen) + // Ignore a stop scheduled by a challenge that has since been superseded + // by a newly started one, otherwise it would wrongly tear down the + // current challenge (e.g. the local password entry after device auth). + if msg.gen != m.authGen { + return m, nil + } m.inProgress = false case reselectAuthMode: @@ -471,6 +490,12 @@ func (m *authenticationModel) Compose(brokerID, sessionID string, encryptionKey m.encryptionKey = encryptionKey m.currentLayout = layout.Type + // A new challenge is being set up: any stopAuthentication scheduled by the + // cancellation of the previous challenge (e.g. when switching auth modes + // after device authentication returns "next") now belongs to a superseded + // challenge and must be ignored, otherwise it would tear down this one. + m.authGen++ + m.errorMsg = "" if m.clientType != InteractiveTerminal { diff --git a/pam/internal/adapter/gdmmodel_test.go b/pam/internal/adapter/gdmmodel_test.go index 0a1db238dd..6cd5d1638f 100644 --- a/pam/internal/adapter/gdmmodel_test.go +++ b/pam/internal/adapter/gdmmodel_test.go @@ -658,13 +658,11 @@ func TestGdmModel(t *testing.T) { gdm.EventType_authEvent, // retry gdm.EventType_startAuthentication, }, - // One authModeSelected/uiLayoutReceived per genuine selection (the - // three password-stage cycles in wantGdmRequests). The GDM echo of - // each selection must not add extra cycles. - wantGdmEventsCount: map[gdm.EventType]int{ - gdm.EventType_authModeSelected: 3, - gdm.EventType_uiLayoutReceived: 3, - }, + // The invariant that the GDM echo of a selection must not add an + // extra selection cycle is asserted deterministically in + // gdmmodel_authmode_echo_test.go. Asserting an exact event count + // here is racy because the test conversation handler delivers the + // echo and stage-change events concurrently with the model's polls. wantStage: proto.Stage_challenge, wantGdmAuthRes: []*authd.IAResponse{ { @@ -1244,15 +1242,11 @@ func TestGdmModel(t *testing.T) { gdm.EventType_startAuthentication, gdm.EventType_authEvent, }, - // Each genuine selection of the auth mode (the initial one and the - // re-selection after navigating back to authModeSelection) must - // produce exactly one selection cycle: the GDM echo of the - // selection must not add a third one. - wantGdmEventsCount: map[gdm.EventType]int{ - gdm.EventType_authModeSelected: 2, - gdm.EventType_uiLayoutReceived: 2, - gdm.EventType_startAuthentication: 2, - }, + // The invariant that the GDM echo of a selection must not add an + // extra selection cycle is asserted deterministically in + // gdmmodel_authmode_echo_test.go. Asserting an exact event count + // here is racy because the test conversation handler delivers the + // echo and stage-change events concurrently with the model's polls. wantStage: proto.Stage_challenge, wantGdmAuthRes: []*authd.IAResponse{ {Access: auth.Granted}, diff --git a/pam/internal/adapter/utils_test.go b/pam/internal/adapter/utils_test.go index 97fd62c109..a4584386ff 100644 --- a/pam/internal/adapter/utils_test.go +++ b/pam/internal/adapter/utils_test.go @@ -174,7 +174,7 @@ func TestSafeMessageDebug(t *testing.T) { msg: startAuthentication{}, prefix: "prefix", formatAndArgs: []any{"suffix is %#v and %q", stopAuthentication{}, "suffix"}, - wantSafeString: `prefix: adapter.startAuthentication{}, suffix is adapter.stopAuthentication{} and "suffix"`, + wantSafeString: `prefix: adapter.startAuthentication{}, suffix is adapter.stopAuthentication{gen:0x0} and "suffix"`, }, "New_password_check": { msg: newPasswordCheck{password: "Super secret password!"},