From 79168ad0111ad48655734f1e651d93aa4196636c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Mon, 8 Apr 2024 01:40:33 +0200 Subject: [PATCH 1/4] debian/authd.gdm-authd.pam: Use pam_authd.so in account step too In order to be able to read and save the module data in the same way we should use the very same pam module. In this case the exec module could be skipped explicitly, but this is happening anyways implicitly since no data for it will be set, so no need to add more complexity --- debian/authd.gdm-authd.pam | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/debian/authd.gdm-authd.pam b/debian/authd.gdm-authd.pam index 4f3f4ce52..1a1b7d316 100644 --- a/debian/authd.gdm-authd.pam +++ b/debian/authd.gdm-authd.pam @@ -8,7 +8,13 @@ auth substack common-auth auth requisite pam_nologin.so auth optional pam_gnome_keyring.so +account [default=ignore success=ok user_unknown=ignore] pam_authd.so +# This is potentially loading pam_authd.again but since the two modules use a +# different data model, the common-account one will always be ignored. +# Such thing is currently implicitly happening because of the data check, but +# if the module AcctMgmt() implementation changes, this has to be checked. @include common-account + # SELinux needs to be the first session rule. This ensures that any # lingering context has been cleared. Without this it is possible # that a module could execute code in the wrong domain. From 59d8db4f67d665362310b74ab76503c187907d97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 10 Apr 2024 14:16:15 +0200 Subject: [PATCH 2/4] integration-tests/gdm: Check that previous broker for user is saved We were calling AcctMgmt but not checking if the previous broker for user was actually saved, so do it now --- pam/integration-tests/gdm_test.go | 5 +++++ pam/integration-tests/helpers_test.go | 28 +++++++++++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/pam/integration-tests/gdm_test.go b/pam/integration-tests/gdm_test.go index feb9cc1a7..8d19dc413 100644 --- a/pam/integration-tests/gdm_test.go +++ b/pam/integration-tests/gdm_test.go @@ -333,16 +333,21 @@ func testGdmModule(t *testing.T, libPath string, args []string) { require.Equal(t, tc.wantPamInfoMessages, gh.pamInfoMessages, "PAM Info messages do not match") + requirePreviousBrokerForUser(t, socketPath, "", tc.pamUser) + require.ErrorIs(t, gh.tx.AcctMgmt(pamFlags), tc.wantAcctMgmtErr, "Account Management PAM Error messages do not match") if tc.wantError != nil { + requirePreviousBrokerForUser(t, socketPath, "", tc.pamUser) return } user, err := gh.tx.GetItem(pam.User) require.NoError(t, err, "Can't get the pam user") require.Equal(t, tc.pamUser, user, "PAM user name does not match expected") + + requirePreviousBrokerForUser(t, socketPath, gh.selectedBrokerName, user) }) } } diff --git a/pam/integration-tests/helpers_test.go b/pam/integration-tests/helpers_test.go index fd6c82645..05e9c3e14 100644 --- a/pam/integration-tests/helpers_test.go +++ b/pam/integration-tests/helpers_test.go @@ -1,6 +1,7 @@ package main_test import ( + "context" "errors" "io/fs" "os" @@ -8,6 +9,9 @@ import ( "testing" "github.com/stretchr/testify/require" + "github.com/ubuntu/authd" + "google.golang.org/grpc" + "google.golang.org/grpc/credentials/insecure" ) func prepareFileLogging(t *testing.T, fileName string) string { @@ -25,3 +29,27 @@ func prepareFileLogging(t *testing.T, fileName string) string { return cliLog } + +func requirePreviousBrokerForUser(t *testing.T, socketPath string, brokerName string, user string) { + t.Helper() + + conn, err := grpc.Dial( + "unix://"+socketPath, + grpc.WithTransportCredentials(insecure.NewCredentials()), + grpc.WithBlock(), + ) + require.NoError(t, err, "Can't connect to authd socket") + t.Cleanup(func() { conn.Close() }) + pamClient := authd.NewPAMClient(conn) + brokers, err := pamClient.AvailableBrokers(context.TODO(), nil) + require.NoError(t, err, "Can't get available brokers") + prevBroker, err := pamClient.GetPreviousBroker(context.TODO(), &authd.GPBRequest{Username: user}) + require.NoError(t, err, "Can't get previous broker") + var prevBrokerID string + for _, b := range brokers.BrokersInfos { + if b.Name == brokerName { + prevBrokerID = b.Id + } + } + require.Equal(t, prevBroker.PreviousBroker, prevBrokerID) +} From 1ba4d00b0d5e226e15b28291e93e9ae52f68aa86 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 10 Apr 2024 15:06:16 +0200 Subject: [PATCH 3/4] pam: Ignore account management if we're in gdm-authd service without extension support --- debian/authd.gdm-authd.pam | 7 +++---- pam/pam.go | 14 ++++++++++++++ 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/debian/authd.gdm-authd.pam b/debian/authd.gdm-authd.pam index 1a1b7d316..020f430c4 100644 --- a/debian/authd.gdm-authd.pam +++ b/debian/authd.gdm-authd.pam @@ -9,10 +9,9 @@ auth requisite pam_nologin.so auth optional pam_gnome_keyring.so account [default=ignore success=ok user_unknown=ignore] pam_authd.so -# This is potentially loading pam_authd.again but since the two modules use a -# different data model, the common-account one will always be ignored. -# Such thing is currently implicitly happening because of the data check, but -# if the module AcctMgmt() implementation changes, this has to be checked. +# This is potentially loading pam_authd.again but we've checks in AcctMgmt() to +# prevent this to happen when the gdm-authd service is used without GDM extensions. +# Plus the model used by the services is different, so there's no risk for this to happen. @include common-account # SELinux needs to be the first session rule. This ensures that any diff --git a/pam/pam.go b/pam/pam.go index 5f8f2ce45..6284648a3 100644 --- a/pam/pam.go +++ b/pam/pam.go @@ -42,6 +42,10 @@ const ( // we've already authenticated with this module and so that we should not // do this again. alreadyAuthenticatedKey = "authd.already-authenticated-flag" + + // gdmServiceName is the name of the service that is loaded by GDM. + // Keep this in sync with the service file installed by the package. + gdmServiceName = "gdm-authd" ) var supportedArgs = []string{ @@ -332,6 +336,16 @@ func (h *pamModule) AcctMgmt(mTx pam.ModuleTransaction, flags pam.Flags, args [] } logArgsIssues() + // We ignore AcctMgmt in case we're loading the module through the exec client + serviceName, err := mTx.GetItem(pam.Service) + if err != nil { + log.Warningf(context.TODO(), "Impossible to get PAM service name: %v", err) + return pam.ErrIgnore + } + if serviceName == gdmServiceName && !gdm.IsPamExtensionSupported(gdm.PamExtensionCustomJSON) { + return pam.ErrIgnore + } + brokerData, err := mTx.GetData(authenticationBrokerIDKey) if err != nil && errors.Is(err, pam.ErrNoModuleData) { return pam.ErrIgnore From 3189c15af1e3dba0df8f142db5eaf33f16990af4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 10 Apr 2024 15:07:42 +0200 Subject: [PATCH 4/4] pam/integration-tests/gdm: Simulate gdm module behavior when not loaded from gdm --- pam/integration-tests/gdm_test.go | 107 +++++++++++++++++- .../TestGdmModuleAcctMgmtWithoutGdmExtension | 1 + ...stGdmModuleAuthenticateWithoutGdmExtension | 1 + 3 files changed, 107 insertions(+), 2 deletions(-) create mode 120000 pam/integration-tests/testdata/TestGdmModuleAcctMgmtWithoutGdmExtension create mode 120000 pam/integration-tests/testdata/TestGdmModuleAuthenticateWithoutGdmExtension diff --git a/pam/integration-tests/gdm_test.go b/pam/integration-tests/gdm_test.go index 8d19dc413..de6325a4c 100644 --- a/pam/integration-tests/gdm_test.go +++ b/pam/integration-tests/gdm_test.go @@ -20,10 +20,14 @@ import ( "github.com/ubuntu/authd/pam/internal/proto" ) -func init() { +func enableGdmExtension() { gdm.AdvertisePamExtensions([]string{gdm.PamExtensionCustomJSON}) } +func init() { + enableGdmExtension() +} + const ( exampleBrokerName = "ExampleBroker" localBrokerName = "local" @@ -284,7 +288,7 @@ func testGdmModule(t *testing.T, libPath string, args []string) { t.Cleanup(func() { saveArtifactsForDebug(t, []string{gdmLog}) }) moduleArgs = append(moduleArgs, "debug=true", "logfile="+gdmLog) - serviceFile := createServiceFile(t, "module-loader", libPath, + serviceFile := createServiceFile(t, "gdm-authd", libPath, moduleArgs) gh := newGdmTestModuleHandler(t, serviceFile, tc.pamUser) @@ -369,6 +373,105 @@ func TestGdmModuleWithCWrapper(t *testing.T) { testGdmModule(t, wrapperLibPath, []string{libPath}) } +func TestGdmModuleAuthenticateWithoutGdmExtension(t *testing.T) { + // This cannot be parallel! + t.Cleanup(pam_test.MaybeDoLeakCheck) + + libPath := buildPAMModule(t) + moduleArgs := []string{libPath} + + gpasswdOutput := filepath.Join(t.TempDir(), "gpasswd.output") + groupsFile := filepath.Join(testutils.TestFamilyPath(t), "gpasswd.group") + ctx, cancel := context.WithCancel(context.Background()) + socketPath, stopped := testutils.RunDaemon(ctx, t, daemonPath, + testutils.WithEnvironment(grouptests.GPasswdMockEnv(t, gpasswdOutput, groupsFile)...)) + t.Cleanup(func() { + cancel() + <-stopped + }) + moduleArgs = append(moduleArgs, "socket="+socketPath) + + gdmLog := prepareFileLogging(t, "authd-pam-gdm.log") + t.Cleanup(func() { saveArtifactsForDebug(t, []string{gdmLog}) }) + moduleArgs = append(moduleArgs, "debug=true", "logfile="+gdmLog) + + serviceFile := createServiceFile(t, "gdm-authd", libPath, moduleArgs) + pamUser := "user1" + gh := newGdmTestModuleHandler(t, serviceFile, pamUser) + t.Cleanup(func() { require.NoError(t, gh.tx.End(), "PAM: can't end transaction") }) + + // We disable gdm extension support, as if it was the case when the module is loaded + // outside GDM. + gdm.AdvertisePamExtensions(nil) + t.Cleanup(enableGdmExtension) + + var pamFlags pam.Flags + if !testutils.IsVerbose() { + pamFlags = pam.Silent + } + + require.ErrorIs(t, gh.tx.Authenticate(pamFlags), pam.ErrSystem, + "Authentication should fail") + requirePreviousBrokerForUser(t, socketPath, "", pamUser) +} + +func TestGdmModuleAcctMgmtWithoutGdmExtension(t *testing.T) { + // This cannot be parallel! + t.Cleanup(pam_test.MaybeDoLeakCheck) + + libPath := buildPAMModule(t) + moduleArgs := []string{libPath} + + gpasswdOutput := filepath.Join(t.TempDir(), "gpasswd.output") + groupsFile := filepath.Join(testutils.TestFamilyPath(t), "gpasswd.group") + ctx, cancel := context.WithCancel(context.Background()) + socketPath, stopped := testutils.RunDaemon(ctx, t, daemonPath, + testutils.WithEnvironment(grouptests.GPasswdMockEnv(t, gpasswdOutput, groupsFile)...)) + t.Cleanup(func() { + cancel() + <-stopped + }) + moduleArgs = append(moduleArgs, "socket="+socketPath) + + gdmLog := prepareFileLogging(t, "authd-pam-gdm.log") + t.Cleanup(func() { saveArtifactsForDebug(t, []string{gdmLog}) }) + moduleArgs = append(moduleArgs, "debug=true", "logfile="+gdmLog) + + serviceFile := createServiceFile(t, "gdm-authd", libPath, moduleArgs) + pamUser := "user1" + gh := newGdmTestModuleHandler(t, serviceFile, pamUser) + t.Cleanup(func() { require.NoError(t, gh.tx.End(), "PAM: can't end transaction") }) + + gh.supportedLayouts = []*authd.UILayout{pam_test.FormUILayout()} + gh.protoVersion = gdm.ProtoVersion + gh.selectedBrokerName = exampleBrokerName + gh.selectedAuthModeIDs = []string{passwordAuthID} + gh.eventPollResponses = map[gdm.EventType][]*gdm.EventData{ + gdm.EventType_startAuthentication: { + gdm_test.IsAuthenticatedEvent(&authd.IARequest_AuthenticationData_Challenge{ + Challenge: "goodpass", + }), + }, + } + + var pamFlags pam.Flags + if !testutils.IsVerbose() { + pamFlags = pam.Silent + } + + require.NoError(t, gh.tx.Authenticate(pamFlags), "Setup: Authentication failed") + requirePreviousBrokerForUser(t, socketPath, "", pamUser) + + // We disable gdm extension support, as if it was the case when the module is loaded + // again from the exec module. + gdm.AdvertisePamExtensions(nil) + t.Cleanup(enableGdmExtension) + + require.ErrorIs(t, gh.tx.AcctMgmt(pamFlags), pam_test.ErrIgnore, + "Account Management PAM Error message do not match") + requirePreviousBrokerForUser(t, socketPath, "", pamUser) +} + func buildPAMModule(t *testing.T) string { t.Helper() diff --git a/pam/integration-tests/testdata/TestGdmModuleAcctMgmtWithoutGdmExtension b/pam/integration-tests/testdata/TestGdmModuleAcctMgmtWithoutGdmExtension new file mode 120000 index 000000000..311ec0c1f --- /dev/null +++ b/pam/integration-tests/testdata/TestGdmModuleAcctMgmtWithoutGdmExtension @@ -0,0 +1 @@ +TestGdmModule \ No newline at end of file diff --git a/pam/integration-tests/testdata/TestGdmModuleAuthenticateWithoutGdmExtension b/pam/integration-tests/testdata/TestGdmModuleAuthenticateWithoutGdmExtension new file mode 120000 index 000000000..311ec0c1f --- /dev/null +++ b/pam/integration-tests/testdata/TestGdmModuleAuthenticateWithoutGdmExtension @@ -0,0 +1 @@ +TestGdmModule \ No newline at end of file