Add `authctl user set-uid` command #1087

adombeck · 2025-09-24T09:19:15Z

Support setting the UID of a user via authctl user set-uid <user> <uid>.

Also changes the owner and group of the user's home directory and all files in the home directory from the old UID and GID to the new UID and GID (if it is owned by the current user), same as usermod does when changing the UID of a user.

Closes #630
UDENG-7717
UDENG-8720

codecov-commenter · 2025-09-24T12:58:37Z

Codecov Report

❌ Patch coverage is 64.08451% with 153 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.36%. Comparing base (ba5104c) to head (0702184).

Files with missing lines	Patch %	Lines
internal/users/proc/proc.go	60.41%	38 Missing ⚠️
cmd/authctl/internal/completion/completion.go	0.00%	29 Missing ⚠️
internal/users/db/update.go	69.87%	25 Missing ⚠️
cmd/authctl/user/set-uid.go	0.00%	19 Missing ⚠️
cmd/authctl/group/set-gid.go	0.00%	16 Missing ⚠️
internal/services/user/user.go	72.72%	6 Missing ⚠️
internal/testlog/testlog.go	66.66%	6 Missing ⚠️
internal/users/manager.go	94.33%	6 Missing ⚠️
cmd/authctl/internal/client/client.go	72.72%	3 Missing ⚠️
internal/fileutils/fileutils.go	83.33%	3 Missing ⚠️
... and 2 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1087      +/-   ##
==========================================
- Coverage   87.65%   86.36%   -1.30%     
==========================================
  Files          90       96       +6     
  Lines        6222     6615     +393     
  Branches      111      111              
==========================================
+ Hits         5454     5713     +259     
- Misses        712      846     +134     
  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.

adombeck · 2025-11-12T10:18:02Z

@3v1n0 @denisonbarbosa The PR is still missing tests, but I would appreciate a first review nonetheless, especially regarding the decision to automatically change the owner of the home directory (see e68fd53) before I spend time adding tests for that.

cmd/authctl/user/set-uid.go

internal/services/permissions/permissions.go

denisonbarbosa · 2025-11-12T12:10:41Z

internal/users/db/update.go

+	// Update the users table. We update both the UID and the GID of the user private group,
+	// because UID == GID for user private groups (see https://wiki.debian.org/UserPrivateGroups#UPGs)
+	if _, err := tx.Exec(`UPDATE users SET uid = ?, gid = ? WHERE name = ?`, newUID, newUID, username); err != nil {
+		return err
+	}


We should probably update the group ID of the private group itself as well

Oh you're right. Now I'm not sure anymore if we should actually change the GID of the user private group automatically as well. usermod doesn't do that and there is no Debian-specific alternative to usermod which handles user private groups (like adduser is for useradd). Thinking about it, I see some potential problems if we change the GID as well:

Less flexibility for admins

More complexity in the implementation of SetUserID

Need to handle the case that the UID is an existing GID

What if a user private group's ID is changed via authctl group set-gid? Should we disallow that?

I'm leaning towards letting the admin use authctl set-gid to also change the GID of the user private group if they want to make use of user private groups, same as they have to do with usermod / groupmod. What do you think? @3v1n0, also interested in your opinion.

I'm leaning towards letting the admin use authctl set-gid to also change the GID of the user private group if they want to make use of user private groups

I just realized that would mean we don't only have to add a authctl group set-gid command (which we should do anyway) but also a authctl user set-gid, to change the GID of a user (analogous to usermod --gid). That also doesn't feel like a great solution 😕

I experimented some more with usermod and groupmod. Some of my findings:

Changing a user's UID (usermod --uid <new-uid> <user>) does not update the user's primary group (as already noted above).

Changing a group's GID (groupmod --gid <new-gid> <group>) does also update the ID of the user's primary group in /etc/passwd

For example if I have this entry in /etc/passwd:

test:x:1000:1000:,,,:/home/test:/bin/bash

and this entry in /etc/group:

test:x:1000

then after executing sudo groupmod --gid=1234 test the entries are changed to

test:x:1000:1234:,,,:/home/test:/bin/bash

and

test:x:1234

A user's primary group can only be changed to the GID of a group that actually exists.

So sudo usermod --gid 1234 test fails with usermod: group '1234' does not exist if there is no group with GID 1234

If we replicate the behavior of 1. with authctl user set-uid and of 2. with authctl group set-gid, I think we could get away without adding a authctl user set-gid command. For example, given the user entry

[email protected]:x:10000:10000:,,,:/home/[email protected]:/bin/bash

and the group entry

[email protected]::10000:[email protected]

To change the UID, the admin would use:

authctl user set-uid 12345 [email protected]

and to then also change the user private group's ID:

authctl group set-gid 12345 [email protected]

authctl user set-uid now only changes the user's UID. I will add a authctl group set-gid command in a follow-up PR.

If we replicate the behavior of 1. with authctl user set-uid and of 2. with authctl group set-gid, I think we could get away without adding a authctl user set-gid command.

While groupmod --gid <gid> <group> does change the primary group ID of users who have <group> as their primary group, it does not change the group ownership of these user's home directories. usermod --gid <gid> <user> does change the group ownership of the user's home directory (for files which are owned by the previous primary group).

I'm considering to deviate from that and make authctl group set-gid also change the group of the home directory of user's whose primary group is updated that way. Otherwise users will have to run sudo chown -R --from :<old-gid> :<new-gid> $HOME manually.

I'm considering to deviate from that and make authctl group set-gid also change the group of the home directory of user's whose primary group is updated that way

Although the approach you suggested makes more sense and is more usable, I'm a bit worried about mimicking the behavior of a well known tool but doing it quietly differently. Although, I think we shouldn't have any issues in this case, since we are adding a behavior rather than removing one.

I do prefer the way you suggested, though. I think it's a good one.

I'm a bit worried about mimicking the behavior of a well known tool but doing it quietly differently

We should pay attention that we:

Don't state that it behaves the same as usermod / groupmod

Make the behavior clear in the documentation

Make the behavior clear in the documentation

I added long descriptions for set-uid:

$ authctl user set-uid --help Set the UID of a user managed by authd to the specified value. The new UID value must be unique and non-negative. The user's home directory and any files within it owned by the user will automatically have their ownership updated to the new UID. If the home directory ownership cannot be changed, a warning will be displayed but the command will still exit successfully. Files outside the home directory are not updated and must be changed manually if needed. To update ownership of all files on the system, use: sudo chown -R --from OLD_UID NEW_UID / This command requires root privileges. Examples: authctl user set-uid john 15000 authctl user set-uid alice 20000 Usage: authctl user set-uid <name> <uid> [flags] Flags: -h, --help help for set-uid

and set-gid:

$ authctl group set-gid --help Set the GID of a group managed by authd to the specified value. The new GID value must be unique and non-negative. When a group's GID is changed, any users whose primary group is set to this group will have their primary group GID updated. The home directories of these users and files within them owned by the group will be updated to the new GID. If changing ownership fails, a warning will be displayed but the command will still succeed. Files outside users' home directories are not updated and must be changed manually. To update group ownership of all files on the system, use: sudo chown -R --from :OLD_GID :NEW_GID / This command requires root privileges. Examples: authctl group set-gid staff 30000 authctl group set-gid developers 40000 Usage: authctl group set-gid <name> <gid> [flags] Flags: -h, --help help for set-gid

internal/users/manager_bwrap_test.go

denisonbarbosa

Amazing work. Some small comments, but we should be able to address them quite quickly.

internal/testutils/bubblewrap.go

internal/users/manager_bwrap_test.go

adombeck · 2025-12-02T16:43:19Z

Will add the missing tests and also update TestChownRecursiveFrom to use bubblewrap :)

Done and I also added authctl group set-gid. Should be ready for another round of review.

internal/services/user/user.go

internal/fileutils/fileutils_test.go

3v1n0 · 2025-12-03T16:55:39Z

internal/proto/authd/authd.proto

+message SetUserIDRequest {
+  string name = 1;
+  uint32 id = 2;
+}
+
+message SetUserIDResponse {
+  repeated string warnings = 1;
+}
+
+message SetGroupIDRequest {
+  string name = 1;
+  uint32 id = 2;
+}
+
+message SetGroupIDResponse {
+  repeated string warnings = 1;
+}


Given we're likely also adding support for setting the user shell and probably GECOs and maybe in future (with user DB metadata) more fields, isn't better to just have a single SetUserProperties message when we handle it all?

As in the daemon side we'll have a single lock place instead of having to manage it for each case we will handle.

As in the daemon side we'll have a single lock place

what do you mean with lock place?

I currently don't see the advantage of a single SetUserProperties message. The good thing is that we don't have to be very careful about this API between authd and authctl, because we ship them both in the same package, so we can always change it however we want.

The good thing is that we don't have to be very careful about this API between authd and authctl, because we ship them both in the same package, so we can always change it however we want.

Yeah, but I'd like also to stick to an API at certain point since I would likely be able to add support to control these from control center too one day :)

what do you mean with lock place?

In order to make these user changes we need to lock the pwd db, and potentially other things, so having it in just one place simplifies things IMHO

Yeah, but I'd like also to stick to an API at certain point since I would likely be able to add support to control these from control center too one day :)

ack

In order to make these user changes we need to lock the pwd db, and potentially other things, so having it in just one place simplifies things IMHO

I think we only need to call lckpwdf when we choose a new UID/GID (and maybe if we support changing the name at some point). Changing other properties of authd users or groups should not cause conflicts with non-authd users/groups.

Depends, what if you change the home or the name?

Home doesn't need to be unique and there are no checks for it, so it doesn't matter if we call lckpwdf or not when changing it. When changing the name we do want to call lckpwdf, as I mentioned:

and maybe if we support changing the name at some point

Anyway, we can easily refactor this later if we see the need for it:

The good thing is that we don't have to be very careful about this API between authd and authctl, because we ship them both in the same package, so we can always change it however we want.

3v1n0 · 2025-12-03T16:58:25Z

internal/users/manager.go

+	err = userslocking.WriteLock()
+	if err != nil {
+		return nil, err
+	}
+	defer func() { err = errors.Join(err, userslocking.WriteUnlock()) }()
+
+	// Check if the user exists
+	oldUser, err := m.db.UserByName(name)
+	if err != nil {
+		return nil, err
+	}
+	// Check if the user already has the given UID
+	if oldUser.UID == uid {
+		warning := fmt.Sprintf("User %q already has UID %d", name, uid)
+		log.Info(context.Background(), warning)
+		return []string{warning}, nil
+	}


This is a per-process lock though, so... If another user is being added with the same ID while we're modifying one we could have racy behavior here.

Unless we have some kind of protection at DB commit phase (as we have in UpdateUser).

That said... I'd likely lock the whole thing now when manipulating users through these APIs (including UpdateUser).

userslocking.WriteLock is a global lock

Or am I missing something?

It's a global per-process lock but it would ErrLock in case of concurrent requests rather than blocking, so, not sure if we want that.

I would also use lockedentries instead, as I assume we want to check if the new UID/GIDs are not used locally, no?

I would also use lockedentries instead, as I assume we want to check if the new UID/GIDs are not used locally, no?

We already check that via user.LookupId. We can't use UserDBLocked.IsUniqueUID because that also returns false if the ID is used as a GID.

Oh, I thought it would behave exactly like lckpwdf, i.e. block for 15 seconds before returning an error. Is there a reason we made it behave differently?

No, that's the default behavior, since it is meant to give unique access to a process to the locked resources, so if we're owning the lock, we're good to go basically, but we can't lock again (thus the error).

Other processes instead will behave that way.

We already check that via user.LookupId. We can't use UserDBLocked.IsUniqueUID because that also returns false if the ID is used as a GID.

Well, can't it split it out?

It's a global per-process lock but it would ErrLock in case of concurrent requests rather than blocking

If that's the behavior of lckpwdf then the man page should be updated to mention that IMO. Currently it reads like it would always block for 15 seconds:

The lckpwdf() function is intended to protect against multiple simultaneous accesses of the shadow password database. It tries to acquire a lock, and returns 0 on success, or -1 on failure (lock not obtained within 15 seconds). The ul‐ ckpwdf() function releases the lock again. Note that there is no protection against direct access of the shadow pass‐ word file. Only programs that use lckpwdf() will notice the lock.

Well, can't it split it out?

I don't see why we should change UserDBLocked. We don't need it here AFAICT, userslocking.WriteLock and user.LookupId give us everything we need.

userslocking.WriteLock and user.LookupId give us everything we need.

That's fine, but we then need to just not rely on userslocking.WriteLock as global lock, or add a version with a mutex on top.

IIRC these were things I somewhat had drafted in the past but then we went for a simpler approach.

If that's the behavior of lckpwdf then the man page should be updated to mention that IMO

Well, feel free to update libc, but we also are testing this case (in fact we do not hang):

go test -C internal/users/locking -v -run TestLockAndLockAgainGroupFile

And it was one of the reasons why I initially added the reflock too.

That's fine, but we then need to just not rely on userslocking.WriteLock as global lock, or add a version with a mutex on top.

yeah I did that now

Well, feel free to update libc

not today, maybe I'll create an issue / PR tomorrow :)

but we also are testing this case (in fact we do not hang)

👍

3v1n0 · 2025-12-03T18:10:30Z

internal/testutils/bubblewrap.go

+	// Since 25.10 Ubuntu ships the AppArmor profile /etc/apparmor.d/bwrap-userns-restrict
+	// which restricts bwrap and causes chown to fail with "Operation not permitted".


Was this reported upstream? Is it something that should not be done?

I assume it's on purpose to better confine processes run in bwrap. We don't care about that because we don't use bwrap for security.

Well not here, I'm speaking at wider level in the distro.

It doesn't test anything that's not already covered by other tests and it's annoying to have to manually update the golden files of the SSH integration tests whenever the authctl usage message changes.

userslocking.WriteLock() immediately returns ErrLock if the lock is already taken *by the current process*. lckpwdf behaves similarly (even though the man page doesn't mention it). To avoid that issue, we now take another lock which blocks concurrent goroutines.

We broke the bubblewrap tests in the CI without noticing it (at first) because the tests were skipped. The only case where we really want to skip the tests is on Launchpad builders. To detect that, we check if the DEB_BUILD_ARCH environment variable is set and we're *not* in GitHub CI.

When executing `unshare --map-user` via exec.Command and connecting the process's stdout or stderr, the command hangs forever if unprivileged user namespaces are disabled. We avoid that by checking via `unshare --user` if unprivileged user namespaces are enabled.

The "Run autopkgtests" CI job runs the tests in an LXD container which doesn't allow using bubblewrap. It fails with: bwrap: Failed to make / slave: Permission denied To avoid that these jobs fail, we allow them to skip the bubblewrap tests. We still run the tests in the "Go Tests" CI jobs.

Running our tests with -v produces so much output that it makes it harder to inspect test failures, for example when viewing the logs of the "Run autopkgtests" CI job in GitHub. Running the tests without -v still prints the logs of the failed tests which should include all the information we need to debug test failures.

As suggested by reviewer. It's not implemented for now, warnings are always returned in English.

We don't need to load the bwrap-userns-restrict AppArmor profile for the bubblewrap tests to work. In fact, we even have to circumvent the AppArmor profile (if it's loaded) for the tests to work. This reverts commit 7b926c0.

adombeck force-pushed the 630-set-uid branch 3 times, most recently from b8f41fb to 07b3b86 Compare September 24, 2025 12:46

adombeck force-pushed the 630-set-uid branch from 07b3b86 to 59488fa Compare November 12, 2025 09:57

adombeck requested review from 3v1n0 and denisonbarbosa November 12, 2025 10:15

denisonbarbosa reviewed Nov 12, 2025

View reviewed changes

adombeck force-pushed the 630-set-uid branch 3 times, most recently from 09f3d78 to 8ddbc86 Compare November 21, 2025 11:02

adombeck marked this pull request as ready for review November 21, 2025 15:56

adombeck requested a review from a team as a code owner November 21, 2025 15:56

adombeck requested a review from denisonbarbosa November 21, 2025 15:56

adombeck force-pushed the 630-set-uid branch from 8ddbc86 to 1a807d4 Compare November 25, 2025 21:25

adombeck commented Nov 27, 2025

View reviewed changes

internal/users/manager_bwrap_test.go Show resolved Hide resolved

denisonbarbosa reviewed Nov 27, 2025

View reviewed changes

internal/testutils/bubblewrap.go Outdated Show resolved Hide resolved

internal/users/manager_bwrap_test.go Show resolved Hide resolved

adombeck force-pushed the 630-set-uid branch from 1a807d4 to 32b9396 Compare December 2, 2025 15:33

adombeck requested a review from denisonbarbosa December 2, 2025 16:43

adombeck commented Dec 2, 2025

View reviewed changes

internal/services/user/user.go Show resolved Hide resolved

adombeck force-pushed the 630-set-uid branch from 02ad239 to d162471 Compare December 2, 2025 20:08

denisonbarbosa reviewed Dec 3, 2025

View reviewed changes

internal/fileutils/fileutils_test.go Outdated Show resolved Hide resolved

adombeck force-pushed the 630-set-uid branch 3 times, most recently from d863174 to 8b9dadb Compare December 3, 2025 13:41

3v1n0 reviewed Dec 3, 2025

View reviewed changes

adombeck added 20 commits January 5, 2026 15:29

Add SetGroupID

2d8d744

Add db_test.TestSetGroupID

aa32001

Add users_test.TestSetGroupID

7ede028

Add authctl group command

0a9ba92

Add authctl group set-gid command

ecd14a6

Add TestSetGIDCommand

c480cc3

pam/integration-tests: Don't print authctl usage message in VHS tape

f66f045

It doesn't test anything that's not already covered by other tests and it's annoying to have to manually update the golden files of the SSH integration tests whenever the authctl usage message changes.

authctl: Add long description for set-uid command

4838d19

authctl: Add long description for set-gid command

b63bd81

Try enabling unprivileged user namespaces in CI

7287225

tests: Make chown work with bubblewrap executed via sudo

48d1b0b

ci: Group log lines of llvm-symbolizer installation

db7fc60

Check if user is busy before changing its UID

f4edf9d

authctl: Add argument completion

1a41df1

testlog: Fix duplicate newlines

2e19650

adombeck requested a review from 3v1n0 January 5, 2026 16:15

adombeck force-pushed the 630-set-uid branch from 608e3c2 to 20c334d Compare January 5, 2026 16:42

adombeck added 4 commits January 5, 2026 17:53

tests: Improve logging of bubblewrap commands

a1fd3cb

tests: Improve logging when building authd and authctl

81d48f2

tests: Improve logging of authctl tests

c8e40f2

authctl: Tell authd which language to return warnings in

0702184

As suggested by reviewer. It's not implemented for now, warnings are always returned in English.

adombeck force-pushed the 630-set-uid branch from 20c334d to 0702184 Compare January 5, 2026 16:53

This was referenced Jan 5, 2026

feat(gh-actions/build-debian): Support running tests with bubblewrap canonical/desktop-engineering#93

Open

Add authctl docs #1179

Open

Revert "ci: Install and load apparmor-profiles"

c79e1de

We don't need to load the bwrap-userns-restrict AppArmor profile for the bubblewrap tests to work. In fact, we even have to circumvent the AppArmor profile (if it's loaded) for the tests to work. This reverts commit 7b926c0.

		// Since 25.10 Ubuntu ships the AppArmor profile /etc/apparmor.d/bwrap-userns-restrict
		// which restricts bwrap and causes chown to fail with "Operation not permitted".

Add authctl user set-uid command #1087

Are you sure you want to change the base?

Add authctl user set-uid command #1087

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

adombeck commented Nov 12, 2025

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

denisonbarbosa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

adombeck commented Dec 2, 2025

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!

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!

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!

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

Add `authctl user set-uid` command #1087

Add `authctl user set-uid` command #1087

adombeck commented Sep 24, 2025 •

edited

Loading

codecov-commenter commented Sep 24, 2025 •

edited

Loading