Add functionality to Lock/Unlock 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.

UDENG-5270

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

❌ Patch coverage is 76.78571% with 26 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.90%. Comparing base (229aa56) to head (3585468).
⚠️ Report is 23 commits behind head on main.

Files with missing lines	Patch %	Lines
cmd/authctl/main.go	30.76%	9 Missing ⚠️
internal/users/db/migration.go	83.33%	4 Missing ⚠️
cmd/authctl/user/user.go	72.72%	3 Missing ⚠️
cmd/authctl/user/lock.go	71.42%	2 Missing ⚠️
cmd/authctl/user/unlock.go	71.42%	2 Missing ⚠️
internal/services/pam/pam.go	75.00%	2 Missing ⚠️
internal/services/user/user.go	85.71%	2 Missing ⚠️
internal/users/db/update.go	80.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #782      +/-   ##
==========================================
- Coverage   88.10%   87.90%   -0.21%     
==========================================
  Files          85       89       +4     
  Lines        6013     6125     +112     
  Branches      111      111              
==========================================
+ Hits         5298     5384      +86     
- Misses        659      685      +26     
  Partials       56       56

☔ 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.

internal/services/pam/pam.go

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)

Assuming this goes after #993 (mostly because otherwise I won't be likely around to rebase that one), this will likely not needed anymore later on

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?

I would expect a function called RunDaemon to start the daemon and block until it has finished running. StartDaemon makes it clear that the function returns once the daemon was started.

It can take a while to build and start the daemon. Lets add some logs to make it a bit clearer what is taking so long.

authctl shows these errors messages, so let's improve them. For example, instead of: no result matching "foo" in "users" use: user "foo" not found

The BuildDaemon() function is only called to build the daemon for integration tests with the example broker. Lets avoid passing the same arguments everywhere. Also renames the function to BuildDaemonWithExampleBroker.

Everywhere we called StartDaemon(), we registered the cleanup of the daemon process via t.Cleanup(). Let's avoid the duplicate code by inlining the cleanup registration into StartDaemon().

Lets be more consistent with other command-line tools which control system services, like systemctl and machinectl, and not print any output if the command succeeds (for example like `systemctl start`/`systemctl stop` or `machinectl kill`).

When trying to log in with a locked user, the following error message is printed: $ su [email protected] can't select broker: error PermissionDenied from server: can't start authentication transaction: rpc error: code = PermissionDenied desc = user [email protected] is locked The "can't start authentication transaction" part doesn't add any valuable information to the error message, which is already too long. By omitting it, we also avoid that the gRPC status is formatted to a string containing the error code, which would duplicate information which the caller adds to the error message. With this commit, the error message is simplified to: can't select broker: error PermissionDenied from server: user [email protected] is locked

Following up on the error message printed when trying to log in as a locked user, we can further improve the message: $ su [email protected] can't select broker: error PermissionDenied from server: user [email protected] is locked by omitting the "can't select broker:" part, which doesn't add any useful information. With this commit, the error message is simplified to: error PermissionDenied from server: user [email protected] is locked

Further improve the error message printed when trying to log in with a locked user from: error PermissionDenied from server: user [email protected] is locked to permission denied: user [email protected] is locked

Co-Authored-By: Adrian Dombeck <[email protected]>

adombeck · 2025-07-12T15:50:20Z

Rebased on main and resolved the conflicts caused by #993

shiv-tyagi · 2025-07-22T16:40:06Z

Thanks @adombeck for all the work you did.

Any update on this @3v1n0?

adombeck · 2025-07-22T16:46:33Z

Thanks @adombeck for all the work you did.

Any update on this @3v1n0?

@3v1n0 is out of office until August, and I will be out of office soon until the second week of August, so I don't expect things to move forward until then. I'm sorry this is taking so long! It seems the issue wasn't such a good candidate after all, because there was a lot more work and iterations of reviews involved than I first anticipated :/

shiv-tyagi · 2025-07-23T03:40:29Z

No worries @adombeck. I just keep bugging to make sure this doesn't get lost. Once this one is merged, I plan to add more functionality to it or find another issue to work on.

3v1n0 · 2025-07-31T23:15:59Z

shell-completion/generate.go

+//go:generate sh -c "go run ../cmd/authctl/main.go completion bash > bash/authctl"
+//go:generate sh -c "go run ../cmd/authctl/main.go completion zsh > zsh/_authctl"
+//go:generate sh -c "go run ../cmd/authctl/main.go completion fish > fish/authctl.fish"


Is adding a file comment such as:

# Code generated by authctl completion. DO NOT EDIT.

Helping with github not showing the files in reviews?

No, I don't think a file comment will change how GitHub shows the files on a PR. I did a quick search and found this: https://github.com/github-linguist/linguist/blob/main/docs/overrides.md#generated-code

So we might be able to suppress diffs of generated files via entries in a .gitattributes file.

I can file a issue to do that as a follow-up if you think it's worth it.

3v1n0 · 2025-07-31T23:28:59Z

...ts/testdata/golden/TestSSHAuthenticate/Authenticate_user_locks_and_unlocks_it_on_shared_SSHd

+> ssh ${AUTHD_PAM_SSH_USER}@localhost ${AUTHD_PAM_SSH_ARGS}
+permission denied: user user-integration-pre-check-ssh-authenticate-user-locks-and-unlocks-it-on-shared-sshd is locked
+Received disconnect from ${SSH_HOST} port ${SSH_PORT} Too many authentication failures
+Disconnected from ${SSH_HOST} port ${SSH_PORT}


This does not seem to have been re-generated, see: https://github.com/ubuntu/authd/actions/runs/16239608510/job/45854663647?pr=782

Same as pam_unix, we now avoid leaking to unauthenticated users whether a user account is locked. That's achieved by only checking if the user is locked after they successfully authenticated to the broker, aborting login in that case. That has the side effect that locked users can still refresh the token and user info which the broker stores on disk. This commit also implements the same improvements to the "permission denied: user is locked" error message which were already implemented in the SelectBroker method. I'm not reverting the changes to the SelectBroker method because I think they are still improvements, even if they are not relevant for this specific error message anymore.

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

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

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 added 14 commits July 11, 2025 22:34

authctl: Add tests for root command

961e958

authctl: Add integration test for authctl user

921597d

refactor: Rename RunDaemon -> StartDaemon

7733ac8

I would expect a function called RunDaemon to start the daemon and block until it has finished running. StartDaemon makes it clear that the function returns once the daemon was started.

Improve error message

aeb6b84

Add debug logs to testutils

688d969

It can take a while to build and start the daemon. Lets add some logs to make it a bit clearer what is taking so long.

authctl: Add integration test for authctl user lock

572d765

Improve error messages

bd91a30

authctl shows these errors messages, so let's improve them. For example, instead of: no result matching "foo" in "users" use: user "foo" not found

refactor: Inline extra args in BuildDaemon()

5e65a55

The BuildDaemon() function is only called to build the daemon for integration tests with the example broker. Lets avoid passing the same arguments everywhere. Also renames the function to BuildDaemonWithExampleBroker.

refactor: Add WithCurrentUserAsRoot option for testutils.StartDaemon()

798c61e

refactor: Register cleanup of daemon process as part of StartDaemon()

7ef348f

Everywhere we called StartDaemon(), we registered the cleanup of the daemon process via t.Cleanup(). Let's avoid the duplicate code by inlining the cleanup registration into StartDaemon().

authctl: Print no output on success

f04da68

Lets be more consistent with other command-line tools which control system services, like systemctl and machinectl, and not print any output if the command succeeds (for example like `systemctl start`/`systemctl stop` or `machinectl kill`).

Further improve error message

7826ea6

Further improve the error message printed when trying to log in with a locked user from: error PermissionDenied from server: user [email protected] is locked to permission denied: user [email protected] is locked

adombeck force-pushed the user-disable branch from e73cd33 to f8c9eb6 Compare July 11, 2025 21:10

pam/integration-tests/ssh: Add a simple lock/unlock test via SSH

5dd1078

Co-Authored-By: Adrian Dombeck <[email protected]>

adombeck force-pushed the user-disable branch from f8c9eb6 to 2a505e1 Compare July 12, 2025 15:49

adombeck requested a review from 3v1n0 July 12, 2025 15:52

3v1n0 reviewed Jul 31, 2025

View reviewed changes

adombeck added 4 commits August 14, 2025 14:25

refactor: Extract GoBuildFlags()

da75c6a

Pass GoBuildFlags to authctl built in tests

3d6d660

refactor: Use WithCurrentUserAsRoot option for testutils.runAuthd()

3585468

adombeck force-pushed the user-disable branch from 2a505e1 to 3585468 Compare August 14, 2025 12:25

adombeck requested a review from 3v1n0 August 14, 2025 12:26

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

Add functionality to Lock/Unlock users #782

Are you sure you want to change the base?

Add functionality to Lock/Unlock users #782

Uh oh!

Conversation

shiv-tyagi commented Feb 7, 2025 • edited by adombeck Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shiv-tyagi commented Feb 7, 2025

Uh oh!

codecov-commenter commented Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

adombeck commented Feb 10, 2025

Uh oh!

adombeck commented Feb 10, 2025

Uh oh!

adombeck commented Feb 10, 2025

Uh oh!

shiv-tyagi commented Feb 10, 2025

Uh oh!

Uh oh!

Uh oh!

adombeck commented Feb 18, 2025

Uh oh!

shiv-tyagi commented Feb 18, 2025

Uh oh!

shiv-tyagi commented Feb 19, 2025

Uh oh!

shiv-tyagi commented Feb 26, 2025

Uh oh!

adombeck left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shiv-tyagi commented Mar 4, 2025

Uh oh!

adombeck commented Jul 12, 2025

Uh oh!

shiv-tyagi commented Jul 22, 2025

Uh oh!

adombeck commented Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shiv-tyagi commented Jul 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shiv-tyagi commented Feb 7, 2025 •

edited by adombeck

Loading

codecov-commenter commented Feb 7, 2025 •

edited

Loading

adombeck commented Jul 22, 2025 •

edited

Loading