Add functionality to Enable/Disable users #782

shiv-tyagi · 2025-02-07T16:14:45Z

As discussed in #640, this adds a property to the UserDB to mark a user enable/disabled. Before creating an authentication session in pam, we check if the user exists in the cache and is disabled.

This also adds the initial implementation of the authctl tool which can be used to enable/disable a user.

shiv-tyagi · 2025-02-07T16:15:41Z

@adombeck Does this look good?

I see that the tests are failing on main branch as well. So I am hoping that it is not me.

codecov-commenter · 2025-02-07T16:22:07Z

Codecov Report

Attention: Patch coverage is 86.15385% with 9 lines in your changes missing coverage. Please review.

Project coverage is 79.05%. Comparing base (5d2f34d) to head (85c7275).
Report is 29 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/users/db/update.go	57.14%	4 Missing and 2 partials ⚠️
internal/services/pam/pam.go	57.14%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main     #782    +/-   ##
========================================
  Coverage   79.05%   79.05%            
========================================
  Files         102      103     +1     
  Lines       10383    10489   +106     
  Branches       74       75     +1     
========================================
+ Hits         8208     8292    +84     
- Misses       1709     1730    +21     
- Partials      466      467     +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

adombeck · 2025-02-10T13:15:09Z

internal/services/pam/pam.go

@@ -138,6 +138,12 @@ func (s Service) SelectBroker(ctx context.Context, req *authd.SBRequest) (resp *
 		lang = "C"
 	}

+	// Throw an error if the user trying to authenticate already exists in cache and is disabled


We use the term "database" instead of "cache" now (related: #775)

Suggested change

// Throw an error if the user trying to authenticate already exists in cache and is disabled

// Throw an error if the user trying to authenticate already exists in the database and is disabled

adombeck · 2025-02-10T13:17:56Z

Hi @shiv-tyagi, yes this looks good, thanks! One small thing: I would prefer to have a Disabled field instead of Enabled, so that the default value (false) means that the user is enabled, even when we're not using NewUserDB to instantiate the UserDB struct (like we do here).

adombeck · 2025-02-10T13:20:16Z

There should also be a new test case "Error_when_user_is_disabled" in https://github.com/shiv-tyagi/authd/blob/fed39218cfcce6c23e80e0d02fe2a2d748d3d912/internal/brokers/manager_test.go#L187-L190.

adombeck · 2025-02-10T13:22:24Z

Oh and I don't think it makes sense to merge this before we have code which actually causes a user to be disabled (i.e. the command-line tool). If you still plan to work on that soon, feel free to repurpose this PR. Otherwise, I'll start working on that soon and would then cherry-pick your commits to a new branch.

shiv-tyagi · 2025-02-10T17:30:14Z

I would prefer to have a Disabled field instead of Enabled, so that the default value (false) means that the user is enabled, even when we're not using NewUserDB to instantiate the UserDB struct (like we do here).

Sure. I will do that.

There should also be a new test case "Error_when_user_is_disabled"

Noted.

If you still plan to work on that soon, feel free to repurpose this PR.

Yes, I really intend to work on that. I will push my work soon :)

internal/users/manager.go

internal/services/pam/pam.go

adombeck · 2025-02-18T14:31:00Z

I rebased on main and resolved the conflicts caused by #779.

shiv-tyagi · 2025-02-18T17:41:33Z

I rebased on main and resolved the conflicts caused by #779.

Thanks for this @adombeck. I had some more work which I had not pushed yet. I would need to fix that as well. Planning to push that work soon.

shiv-tyagi · 2025-02-19T06:53:46Z

I have finished the part till extending the GRPC API with new methods and adding unit tests for those. Now only the part of creating the CLI tool to call those methods remains.

shiv-tyagi · 2025-02-26T05:28:29Z

@adombeck I have added the initial implementation of the authctl tool. Please check if it looks good and provide any suggestions for me to proceed. This is my first time writing Go code, so I apologise if I made any rookie mistakes. I’ll be happy to fix them.

Thanks in advance.

adombeck

I've got a few comments but the overall code style is good! Thanks a lot, @shiv-tyagi!

internal/users/manager_test.go

adombeck · 2025-02-26T16:39:23Z

internal/users/manager_test.go

+			// We don't care about the output of gpasswd in this test, but we still need to mock it.
+			_ = localgroupstestutils.SetupGPasswdMock(t, "empty.group")


I don't understand why we would need to mock gpasswd in this test, but I also don't see why that's done in many of the other tests in this file which are unrelated to /etc/group. I see that was introduced in fd10538, do you remember the reason, @denisonbarbosa?

I think the reason is to make sure that for no reason the groups are changed when unexpected (and if that's the case, it's a good negative test IMHO)

internal/users/manager_test.go

internal/proto/authd/authd.proto

cmd/authctl/main.go

cmd/authctl/user/disable.go

cmd/authctl/user/user.go

shiv-tyagi · 2025-03-04T18:36:52Z

Thank you so much @adombeck for reviewing this. I have addressed all your suggestions. What next? How should I test it locally?

adombeck · 2025-03-11T14:41:53Z

cmd/authctl/user/user.go

+// UserCmd is a command to perform user-related operations.
+var UserCmd = &cobra.Command{
+	Use:   "user",
+	Short: "Commands retaled to users",


Suggested change

Short: "Commands retaled to users",

Short: "Commands related to users",

Silly me. I've fixed that. Thanks.

adombeck · 2025-03-11T14:53:18Z

Looks great, thanks for addressing the comments, @shiv-tyagi! I just added one more comment. Once you resolved that, I plan to merge this and create follow-up tasks for adding documentation and actually shipping the tool as part of the authd package.

3v1n0 · 2025-03-11T15:32:55Z

cmd/authctl/user/disable.go

+	Short: "Disable a user managed by authd",
+	Args:  cobra.ExactArgs(1),
+	RunE: func(cmd *cobra.Command, args []string) error {
+		fmt.Printf("Disabling user %s\n", args[0])


Maybe use %q? Otherwise we can just rely on fmt.Println

Changed to %q. Thanks.

3v1n0 · 2025-03-11T15:33:45Z

cmd/authctl/user/disable.go

+	Args:  cobra.ExactArgs(1),
+	RunE: func(cmd *cobra.Command, args []string) error {
+		fmt.Printf("Disabling user %s\n", args[0])
+		conn, err := grpc.NewClient("unix://"+consts.DefaultSocketPath, grpc.WithTransportCredentials(insecure.NewCredentials()))


This should be overridable, for testing purposes mostly.

An env variable is probably enough.

Changed to check the value of the AUTHD_SOCKET environment variable first and use the default socket it the env var is unset.

3v1n0 · 2025-03-11T15:37:58Z

cmd/authctl/user/disable.go

+		if err != nil {
+			return err
+		}
+


Also check for the HealthCheck to be around? See pam adapter model but it can be simpler here.

What benefit would that provide here?

Ah, do not make the request when the socket is there but not ready (so in a socket-activated scenario) or in tests where the daemon may take some time to show up.

Mmh, I don't really understand how calling the Health.Check RPC method instead of the NSS.DisableUser RPC method would help us when the daemon is not ready. Both have a timeout and if the daemon is ready before the timeout then they succeed, otherwise they fail.

My understanding of the gRPC health checks is that they are useful for two things: monitoring the status of gRPC services and choosing a healthy service from a list of services. Both don't apply to our use case.

3v1n0 · 2025-03-11T16:05:06Z

cmd/authctl/user/enable.go

+		conn, err := grpc.NewClient("unix://"+consts.DefaultSocketPath, grpc.WithTransportCredentials(insecure.NewCredentials()))
+		if err != nil {
+			return err
+		}
+
+		client := authd.NewNSSClient(conn)


Maybe group all this to a re-usable function

Um okay. I will do that. Once we decide if we are going to keep this stuff in NSS or we want to create a separate service.

3v1n0 · 2025-03-11T16:06:17Z

cmd/authctl/user/enable.go

+)
+
+// EnableCmd is a command to enable a user.
+var EnableCmd = &cobra.Command{


Any need for being exported?

I am a golang newbie. I though I would need that to be able to use this command in users.go. I can move this whole subcommand to users.go if we want to avoid exporting this. Or I will be happy to do anything else you would suggest.

no need to move it, can just be named with a lower case initial.

Done. Thanks.

3v1n0 · 2025-03-11T16:15:17Z

internal/services/nss/nss.go

+// DisableUser marks a user as disabled.
+func (s Service) DisableUser(ctx context.Context, req *authd.DisableUserRequest) (*authd.Empty, error) {
+	if err := s.permissionManager.IsRequestFromRoot(ctx); err != nil {
+		return nil, err
+	}
+
+	if req.GetName() == "" {
+		return nil, status.Error(codes.InvalidArgument, "no user name provided")
+	}
+
+	if err := s.userManager.DisableUser(req.GetName()); err != nil {
+		return nil, err
+	}
+
+	return &authd.Empty{}, nil
+}
+
+// EnableUser marks a user as enabled.
+func (s Service) EnableUser(ctx context.Context, req *authd.EnableUserRequest) (*authd.Empty, error) {
+	if err := s.permissionManager.IsRequestFromRoot(ctx); err != nil {
+		return nil, err
+	}
+
+	if req.GetName() == "" {
+		return nil, status.Error(codes.InvalidArgument, "no user name provided")
+	}
+
+	if err := s.userManager.EnableUser(req.GetName()); err != nil {
+		return nil, err
+	}
+
+	return &authd.Empty{}, nil
+}
+


I'm a bit unsure this should be part of the NSS service...

The nss service is meant to be the one for NSS operations and these do not include enabling/disabling an user AFAIK, so this should be IMHO part of an authd-management specific service, rather than in the NSS one, that is the one to be used by the NSS module to provide the group and passwd information.

@didrocks any opinion here?

Ah I saw Didier had the same concerns: #640 (comment)

So we should move this out from NSS and have another management service, let's define a good name then.

Yes, we should decide how we want to restructure the service. I made some suggestions on #640 (comment). I'm not sure we have to block merging this PR on that. I don't expect @shiv-tyagi to do that work, IMO we could do it as a follow-up.

Isn't better to make it before and then rebase this PR on that?

We can also do that.

If we are doing a PR for restructuring this, I will wait till it is merged and then rebase my work on top of it.

3v1n0 · 2025-03-11T16:22:19Z

internal/services/nss/nss.go

I feel that all the various GetPasswdByName and friends (includind pre-check) should also be adjusted, otherwise you may end up returning an user that has been enabled as a valid one from the NSS side, no?

If GetPasswdByName doesn't return a disabled user, its UID becomes available for use on the system again, which is exactly what we want to avoid and why we support disabling a user instead of deleting it.

Mh, I see... That would imply having it available in other places though? For example will it be listed in the gdm list of users?

Good question. I didn't test that. If it does, maybe we should use the org.freedesktop.Accounts D-Bus service to set Locked to True when disabling a user. That should hide it from the list of users in the GDM login screen, right?

I would probably need more help to understand what exactly needs to be done. It would be a good learning opportunity for me, but might be a little annoying for you guys 😬😬.

3v1n0 · 2025-03-11T16:23:17Z

internal/services/pam/pam.go

+	userIsDisabled, err := s.userManager.IsUserDisabled(username)
+	if err != nil && !errors.Is(err, users.NoDataFoundError{}) {
+		return nil, fmt.Errorf("could not check if user %q is disabled: %w", username, err)
+	}
+	// Throw an error if the user trying to authenticate already exists in the database and is disabled
+	if err == nil && userIsDisabled {
+		return nil, status.Error(codes.PermissionDenied, fmt.Sprintf("user %s is disabled", username))
+	}


This check can't be moved at lower level in the stack so that it's not only PAM-related?

What do you have in mind? Where would "lower in the stack" be here?

I was thinking to have this in the brokers side, but as per your NSS comment it's not something we want then.

3v1n0 · 2025-03-11T16:30:57Z

cmd/authctl/main.go

It would be nice to have integration tests for this, but if you don't have the time it would be nice if you could open an issue so that we can track it.

What do you want to see tested in these integration tests?

I would like to have tests which start authd and then use authctl and check that it works as expected. That would be an end-to-end test which we don't have a framework for yet. I'm not sure if makes sense to have something in between the tests we already have and this end-to-end test.

I would like to have tests which start authd and then use authctl and check that it works as expected.

Exactly what I mean, and what it should be done IMHO in an integration test, similar to the PAM ones.

Basically, adding a new (VHS based, sorry) test that:

Authenticates with an user

Disables it

Tries authentication again that must fail

Enables it back

It should be in

Can you point me to the examples of existing integration tests in the code? I will use them as a reference to write new ones.

❯ sudo gdbus call --system --dest org.freedesktop.Accounts --object-path /org/freedesktop/Accounts/User1001 --method org.freedesktop.Accounts.User.SetLocked true ❯ gdbus call --system --dest org.freedesktop.Accounts --object-path /org/freedesktop/Accounts --method org.freedesktop.Accounts.ListCachedUsers ([objectpath '/org/freedesktop/Accounts/User1000', '/org/freedesktop/Accounts/User1001'],)

But indeed that's marked as locked and so we filtering it out in gdm...

However... What lock does is that eventually calls usermod -L -- $USER that indeed would fail for authd users, and so the user wouldn't never be marked as locked either.

In fact:

$ sudo gdbus call --system --dest org.freedesktop.Accounts --object-path /org/freedesktop/Accounts/User65692 --method org.freedesktop.Accounts.User.SetLocked true Errore: GDBus.Error:org.freedesktop.Accounts.Error.Failed: running '/usr/sbin/usermod' failed: Processo figlio uscito con codice 6

Thus it wouldn't be enough... Account service has support for vendor overrides, but I'm unsure if they can help either

If integration tests are not a blocker to merge this work, can we create another issue for that?

This also adds tests for the same.

shiv-tyagi requested a review from a team as a code owner February 7, 2025 16:14

shiv-tyagi changed the title ~~Check is a user is disabled before logging in~~ Check if a user is disabled before logging in Feb 10, 2025

adombeck reviewed Feb 10, 2025

View reviewed changes

shiv-tyagi force-pushed the user-disable branch from e6410f4 to a7d839c Compare February 10, 2025 17:27

3v1n0 reviewed Feb 11, 2025

View reviewed changes

internal/users/manager.go Outdated Show resolved Hide resolved

shiv-tyagi force-pushed the user-disable branch 4 times, most recently from 7183092 to 5e81b35 Compare February 15, 2025 18:22

adombeck reviewed Feb 17, 2025

View reviewed changes

internal/services/pam/pam.go Outdated Show resolved Hide resolved

adombeck force-pushed the user-disable branch from 5e81b35 to 54aae19 Compare February 18, 2025 14:30

shiv-tyagi force-pushed the user-disable branch 2 times, most recently from 7f1338f to fce5e88 Compare February 19, 2025 06:51

shiv-tyagi force-pushed the user-disable branch from fce5e88 to 7208138 Compare February 26, 2025 05:20

shiv-tyagi changed the title ~~Check if a user is disabled before logging in~~ Add functionality to Enable/Disable users Feb 26, 2025

adombeck reviewed Feb 26, 2025

View reviewed changes

shiv-tyagi force-pushed the user-disable branch from 7208138 to 33ddd8c Compare March 4, 2025 18:25

shiv-tyagi force-pushed the user-disable branch from 33ddd8c to 5152313 Compare March 4, 2025 18:56

adombeck mentioned this pull request Mar 7, 2025

Removing user via command-line tool should also delete user data from brokers #830

Open

adombeck reviewed Mar 11, 2025

View reviewed changes

3v1n0 reviewed Mar 11, 2025

View reviewed changes

shiv-tyagi added 8 commits March 16, 2025 00:08

db: add Disabled property to UserRow struct

7bcc727

users: add method to check if a user is disabled

5a0c8ea

pam: check if the user is disabled before creating session

7db3b90

pam: add test case Error_when_user_is_disabled while selecting broker

54b8833

internal/users: implement logic to enable/disable user

7c8e8f0

This also adds tests for the same.

nss: add API methods to enable/disable user

f6ad608

nss: add tests for DisableUser/EnableUser API methods

7061140

create authctl cli tool

85c7275

shiv-tyagi force-pushed the user-disable branch from 5152313 to 85c7275 Compare March 15, 2025 18:40

	// Throw an error if the user trying to authenticate already exists in cache and is disabled
	// Throw an error if the user trying to authenticate already exists in the database and is disabled

		// We don't care about the output of gpasswd in this test, but we still need to mock it.
		_ = localgroupstestutils.SetupGPasswdMock(t, "empty.group")

	Short: "Commands retaled to users",
	Short: "Commands related to users",

Add functionality to Enable/Disable users #782

Are you sure you want to change the base?

Add functionality to Enable/Disable users #782

Conversation

shiv-tyagi commented Feb 7, 2025 • edited Loading

shiv-tyagi commented Feb 7, 2025

codecov-commenter commented Feb 7, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adombeck commented Feb 10, 2025

adombeck commented Feb 10, 2025

adombeck commented Feb 10, 2025

shiv-tyagi commented Feb 10, 2025

adombeck commented Feb 18, 2025

shiv-tyagi commented Feb 18, 2025

shiv-tyagi commented Feb 19, 2025

shiv-tyagi commented Feb 26, 2025

adombeck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shiv-tyagi commented Mar 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adombeck commented Mar 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adombeck Mar 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

3v1n0 Mar 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

3v1n0 Mar 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shiv-tyagi commented Feb 7, 2025 •

edited

Loading

codecov-commenter commented Feb 7, 2025 •

edited

Loading

adombeck commented Mar 11, 2025 •

edited

Loading

adombeck Mar 17, 2025 •

edited

Loading

3v1n0 Mar 11, 2025 •

edited

Loading

3v1n0 Mar 14, 2025 •

edited

Loading