Add `authctl user set-shell` command by adombeck · Pull Request #1165 · canonical/authd

adombeck · 2025-12-15T16:58:02Z

Important

This is based on #1271, please review and merge that first.

Allow users to change their shell.

Closes #939
UDENG-7089

codecov-commenter · 2025-12-17T11:46:31Z

Codecov Report

❌ Patch coverage is 63.65462% with 181 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.08%. Comparing base (6ff09d2) to head (dfb2353).

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	70.96%	27 Missing ⚠️
cmd/authctl/user/set-uid.go	0.00%	19 Missing ⚠️
cmd/authctl/group/set-gid.go	0.00%	16 Missing ⚠️
cmd/authctl/user/set-shell.go	0.00%	14 Missing ⚠️
internal/services/user/user.go	70.58%	10 Missing ⚠️
internal/services/permissions/permissions.go	66.66%	7 Missing ⚠️
internal/testlog/testlog.go	61.11%	7 Missing ⚠️
internal/users/manager.go	95.00%	6 Missing ⚠️
... and 4 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1165      +/-   ##
==========================================
- Coverage   87.64%   86.08%   -1.56%     
==========================================
  Files          90       97       +7     
  Lines        6222     6682     +460     
  Branches      111      111              
==========================================
+ Hits         5453     5752     +299     
- Misses        713      874     +161     
  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/users/manager.go

codecov · 2026-02-18T15:10:48Z

Codecov Report

❌ Patch coverage is 91.40625% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.17%. Comparing base (6925080) to head (ee00943).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
cmd/authctl/user/set-shell.go	64.28%	5 Missing ⚠️
internal/users/db/update.go	80.00%	2 Missing ⚠️
cmd/authctl/internal/client/client.go	0.00%	1 Missing ⚠️
internal/services/permissions/permissions.go	95.83%	1 Missing ⚠️
internal/users/manager.go	94.73%	1 Missing ⚠️
internal/users/userutils.go	96.96%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1165      +/-   ##
==========================================
+ Coverage   86.26%   87.17%   +0.90%     
==========================================
  Files          99      102       +3     
  Lines        6685     6798     +113     
  Branches      111      111              
==========================================
+ Hits         5767     5926     +159     
+ Misses        862      816      -46     
  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 · 2026-02-25T11:12:10Z

internal/services/user/user.go

+	if err := s.permissionManager.CheckRequestIsFromRootOrUID(ctx, user.UID); err != nil {
+		return nil, status.Error(codes.PermissionDenied, err.Error())


I noticed that in case the request is from the non-root user, chsh requires the user to authenticate in order to change the shell.

We should also be able to do that by letting the user authenticate to the broker. Implementing that seems like quite a lot of effort though, so I would postpone that and only allow root to set user shells for now.

only allow root to set user shells for now

I implemented that

We should also be able to do that by letting the user authenticate to the broker. Implementing that seems like quite a lot of effort though

Can't we do it in the client-side for now, by just re-exec with sudo if any or (better) using polkit?

My understanding is that requiring the user to authenticate is meant to improve security. If we would do that client-side, it would just make it more cumbersome for the user without improving security at all, because user processes could still change the user's shell without authentication by talking to the server directly instead of using authctl.

If we would do that client-side, it would just make it more cumbersome for the user without improving security at all, because user processes could still change the user's shell without authentication by talking to the server directly instead of using authctl.

How that? The server will still only allows request from root. So basically the client has just to do if $UID != 0; then exec pkexec "$0" ${@}; fi or something.

Then ideally, we could do it with proper polkit rules too, so that any user can change it's own password, even if not root or if they have not sudo permissions.

ah, I misunderstood your proposal. yes, we can add polkit support but I think that's mostly orthogonal to this PR, so I would still do it as a follow-up.

The function that's tested is CheckRequestIsFromRoot.

Also renames peerCredsInfo to peerAuthInfo because the field in peer.Peer is named AuthInfo. Also improves the comment of WithUnixPeerCreds.

The envutils package provides utilities for manipulating string slices representing environment variables.

We have to pass the GOCOVERDIR environment variable to the authctl binary.

The grpc.NewClient does not actually try to connect to authd, it just sets up the client but doesn't do any I/O.

The original plan was to allow users to change their own shell without root permissions, like chsh does. However, chsh requires the user to authenticate before it allows change the shell. We should also be able to do that by letting the user authenticate to the broker. That's a lot more effort than originally planned though, so for now we only allow root to change user shells.

But display a warning, similar to the behavior of chsh.

The warning message in the golden file still didn't have the "Warning:" prefix we now add to most of our warnings. That's because the tests replaced the entire warning message. Fix by only replacing the parts which need to be replaced.

3v1n0 · 2026-03-04T13:00:05Z

internal/services/permissions/permissions.go

+
+// CheckRequestIsFromRootOrUID checks if the current gRPC request is from a root
+// user or a specified user and returns an error if not.
+func (m Manager) CheckRequestIsFromRootOrUID(ctx context.Context, uid uint32) (err error) {


I think this change should stay in a separate commit

A per 93315ac#r2852397294 do we still need this change though or can we post-pone it?

3v1n0 · 2026-03-04T13:03:04Z

internal/services/user/user.go

+	if err := s.permissionManager.CheckRequestIsFromRootOrUID(ctx, user.UID); err != nil {
+		return nil, status.Error(codes.PermissionDenied, err.Error())


We should also be able to do that by letting the user authenticate to the broker. Implementing that seems like quite a lot of effort though

Can't we do it in the client-side for now, by just re-exec with sudo if any or (better) using polkit?

3v1n0 · 2026-03-04T13:06:25Z

internal/users/userutils.go

+
+	// Check if the shell is in the list of allowed shells in /etc/shells
+	shells, err := os.ReadFile("/etc/shells")
+	if err != nil {


Do we want to skip the check if the file does not exist?

Also, wondering if for testing purposes we should allow some kind of overriding via a test-time variable.

3v1n0 · 2026-03-04T13:10:07Z

internal/users/userutils.go

+		return err
+	}
+	for _, allowedShell := range strings.Split(string(shells), "\n") {
+		if allowedShell[0] == '#' {


line := strings.TrimSpace(allowedShell) if line == "" || line[0] == '#' {

But I'd actually swap the definition of allowedShell and line.

3v1n0 · 2026-03-04T13:10:31Z

internal/users/userutils.go

+	if err != nil {
+		return err
+	}
+	for _, allowedShell := range strings.Split(string(shells), "\n") {


What about using bufio.NewScanner()

3v1n0 · 2026-03-04T13:26:05Z

internal/envutils/envutils.go

+		if strings.HasPrefix(kv, key+"=") {
+			return strings.TrimPrefix(kv, key+"=")
+		}


strings.CutPrefix?

3v1n0 · 2026-03-04T13:27:04Z

internal/envutils/envutils.go

+	if strings.ContainsRune(value, '\x00') {
+		return nil, fmt.Errorf("invalid value: %q", value)
+	}
+


Maybe also check for valid utf value?

3v1n0 · 2026-03-04T13:30:08Z

internal/envutils/envutils.go

@@ -0,0 +1,46 @@
+// Package envutils provides utilities for manipulating string slices representing environment variables.
+package envutils


Wondering if we could make this a struct that holds the env variables in a map and we can create it from a []string and it can generate one on request.

3v1n0 · 2026-03-04T13:33:04Z

cmd/authctl/group/group_test.go


 			//nolint:gosec // G204 it's safe to use exec.Command with a variable here
 			cmd := exec.Command(authctlPath, append([]string{"group"}, tc.args...)...)
+			cmd.Env = []string{testutils.CoverDirEnv()}


Oh, I thought this was addressed already as part of #782 (comment) :)

3v1n0 · 2026-03-04T13:36:29Z

internal/proto/authd/authd.proto

 }

+message SetShellResponse {
+  repeated string warnings = 1;


This not a new thing, but I feel that we should try to return error / warning codes instead to clients, so that clients can in case localize them depending on the local configuration (while the daemon may know the locale set by the user, we also need to be able to follow LC_ALL or LANG).

adombeck force-pushed the 939-set-shell branch 2 times, most recently from eb3fed6 to dfb2353 Compare December 17, 2025 11:35

adombeck marked this pull request as ready for review December 17, 2025 12:29

adombeck requested a review from a team as a code owner December 17, 2025 12:29

adombeck force-pushed the 939-set-shell branch from dfb2353 to ee39e97 Compare February 10, 2026 14:03

3v1n0 reviewed Feb 10, 2026

View reviewed changes

internal/users/manager.go Show resolved Hide resolved

3v1n0 reviewed Feb 10, 2026

View reviewed changes

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

adombeck force-pushed the 939-set-shell branch from ee39e97 to 5911ade Compare February 18, 2026 14:14

adombeck marked this pull request as draft February 18, 2026 14:15

adombeck force-pushed the 939-set-shell branch 2 times, most recently from 4380115 to e564efe Compare February 18, 2026 14:49

adombeck force-pushed the 939-set-shell branch 4 times, most recently from 342adc9 to 5b82500 Compare February 20, 2026 11:37

adombeck marked this pull request as ready for review February 20, 2026 11:54

adombeck requested a review from 3v1n0 February 20, 2026 12:07

adombeck force-pushed the 939-set-shell branch 5 times, most recently from 0d9d00c to a1048eb Compare February 25, 2026 09:50

adombeck commented Feb 25, 2026

View reviewed changes

adombeck added 5 commits February 26, 2026 13:02

Add gRPC method SetShell

93315ac

Test SetShell methods in db and users packages

7af7110

Add authctl user set-shell command

b0cfa01

docs: Generate docs for authctl user set-shell

45ab5d4

Rename TestIsRequestFromRoot -> TestCheckRequestIsFromRoot

6e3c5d5

The function that's tested is CheckRequestIsFromRoot.

Add TestCheckRequestIsFromRootOrUID

e379217

Also renames peerCredsInfo to peerAuthInfo because the field in peer.Peer is named AuthInfo. Also improves the comment of WithUnixPeerCreds.

adombeck force-pushed the 939-set-shell branch from a1048eb to f19e238 Compare February 26, 2026 12:02

adombeck added 8 commits February 26, 2026 13:23

Test the SetShell method of the user service

eb9383d

Add envutils

b8f6038

The envutils package provides utilities for manipulating string slices representing environment variables.

Fix authctl tests not producing coverage data

4551e08

We have to pass the GOCOVERDIR environment variable to the authctl binary.

Improve error message

9591b09

The grpc.NewClient does not actually try to connect to authd, it just sets up the client but doesn't do any I/O.

SetShell: Allow root to set an invalid shell

7941b7e

But display a warning, similar to the behavior of chsh.

SetUserID: Improve warning messages

5c758e9

TestSetUserID: Fix entire warning message replaced

ee00943

The warning message in the golden file still didn't have the "Warning:" prefix we now add to most of our warnings. That's because the tests replaced the entire warning message. Fix by only replacing the parts which need to be replaced.

adombeck force-pushed the 939-set-shell branch from f19e238 to ee00943 Compare February 26, 2026 12:24

3v1n0 reviewed Mar 4, 2026

View reviewed changes

		if err := s.permissionManager.CheckRequestIsFromRootOrUID(ctx, user.UID); err != nil {
		return nil, status.Error(codes.PermissionDenied, err.Error())

		@@ -0,0 +1,46 @@
		// Package envutils provides utilities for manipulating string slices representing environment variables.
		package envutils

Conversation

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

Uh oh!

Uh oh!

codecov-commenter commented Dec 17, 2025

Codecov Report

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Feb 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

adombeck commented Dec 15, 2025 •

edited

Loading

codecov bot commented Feb 18, 2026 •

edited

Loading