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.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..6cd5d1638f 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,14 @@ 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, }, + // 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{ { @@ -1238,6 +1242,11 @@ func TestGdmModel(t *testing.T) { gdm.EventType_startAuthentication, gdm.EventType_authEvent, }, + // 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}, @@ -2672,6 +2681,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) 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!"},