Do not inherit HOME and USER env vars from pebble daemon environment

[shayancanonical]

Resolves #260 and #263

Issue

As part of https://github.com/canonical/pebble/pull/234/files, pebble inherit's the daemon's environment when compiling the request to send to the pebble daemon. As a result, the HOME and USER environment variables are also inherited (especially troublesome as the HOME variable causes permission errors when executing pebble exec --user=ubuntu --group=ubuntu whoami given that the daemon user is running as root

Solution

The code already correctly sets the HOME and USER variables in request.go based on the specified user and user's home directory. Therefore, nullify these env vars when inherited from the daemon's environment.

Testing

root@host:/home/ubuntu/mnt/pebble# PEBBLE_TEST_USER=ubuntu PEBBLE_TEST_GROUP=ubuntu go test ./internals/daemon -v -check.v -check.f TestUserGroup                                               
=== RUN   Test                                                                                                                                                                                             
2023-07-27T22:02:51.055Z [test] Started daemon.                                                                                                                                                            
2023-07-27T22:02:51.061Z [test] POST /v1/exec 4.716403ms 202                                                                                                                                               
2023-07-27T22:02:51.066Z [test] GET /v1/tasks/1/websocket/control 4.3449ms 200                                                                                                                             
2023-07-27T22:02:51.067Z [test] GET /v1/tasks/1/websocket/stdio 116.761µs 200                                                                                                                              
2023-07-27T22:02:51.067Z [test] GET /v1/tasks/1/websocket/stderr 74.16µs 200                                                                                                                               
2023-07-27T22:02:51.083Z [test] GET /v1/changes/1/wait 15.936021ms 200                                                                                                                                     
PASS: api_exec_test.go:232: execSuite.TestUserGroup     0.028s                                                                                                                                             
2023-07-27T22:02:51.095Z [test] Started daemon.                                                                                                                                                            
PASS: api_exec_test.go:391: execSuite.TestUserGroupError        0.000s                                                                                                                                     
OK: 2 passed                                                                                                                                                                                               
--- PASS: Test (0.05s)                                                                                                                                                                                     
PASS                                                                                                                                                                                                       
ok      github.com/canonical/pebble/internals/daemon    0.060s                                                                                                                                             

[rebornplusplus]

Looks good to me, thanks!

There's a typo that Cris already pointed out. Should it also remove the "USERNAME" env variable?

[shayancanonical]

@rebornplusplus i am not sure about unsetting the USERNAME env var - it is currently not being set in my environment (either when running as ubuntu or as root following a sudo su)

[hpidcock]

I'm not entirely certain this is the behaviour we want. Rather we should only throw away the USER and HOME env vars if we are actually changing users.

If the current user != the exec user, throw away HOME, USER. If the HOME or USER is unset, try to get them from passwd, otherwise leave them unset. Then, always apply the requested env on top.

[benhoyt]

I agree with @hpidcock's point, that we only want to throw away HOME and USER if current user != exec user ... however, with the way the code is set up, I think it's okay to just delete them here. The reason being because they're set up below based on the specified user:

	// Set HOME and USER based on the UserID.
	if environment["HOME"] == "" || environment["USER"] == "" {
		var userID int
		if args.UserID != nil {
			userID = *args.UserID
		} else {
			userID = os.Getuid()
		}
		u, err := user.LookupId(strconv.Itoa(userID))
		if err != nil {
			logger.Noticef("Cannot look up user %d: %v", userID, err)
		} else {
			if environment["HOME"] == "" {
				environment["HOME"] = u.HomeDir
			}
			if environment["USER"] == "" {
				environment["USER"] = u.Username
			}
		}
	}

In summary, if we always delete HOME and USER here where @shayancanonical has:

• If HOME and USER are specified in the exec environment, that will always win.
• If they're not specified in the exec environment:
  • If an exec user is specified, look up HOME and USER from the specified user's configuration.
  • If an exec user is not specified, look them up from the current user's configuration.

One other thing: can we please add a test that exercises this? It will have to be one of the root tests (see here). That is, a test that fails before this code change, and succeeds after.

[benhoyt]

I think it's worth expanding this comment, explaining why it was necessary. Also, if HOME and USER aren't present in args.Environment, they are set up below based on the username and home directory of the provided user ID (or the current user if not specified).

[shayancanonical]

addressed in 88a02c5

[benhoyt]

@hpidcock said on Mattermost:

My point is that when the user is the same user, but the user doesn't exist in /etc/passwd, even if the USER/HOME is set, we clear it. All I want to change is that the resetting of USER/HOME happens only if the user is changing, to preserve the USER/HOME set on pebble.

Ah, I see. In a container with no /etc/passwd the user lookup will fail. Yeah, makes sense. If you could update this, @shayancanonical, that would be great.

[shayancanonical]

@hpidcock With the new changes, we now only just reset the HOME and USER env vars when the specified user for exec is not the current user (https://github.com/canonical/pebble/pull/262/files#diff-73a0b33c7f5299e3f54a1420f50ee1c44e83249410eba36e5ae55685a94a1d13R77)

@benhoyt I looked into the the root test that was supposed to fail, but was passing (TestUserGroup) on master. I realized that why they were passing when they should have been failing: we run the tests as user runner with sudo. The $HOME directory is the directory of the runner when sudo is invoked, and thus the exec --user=runner --group=runner command has access to runner's home directory. However, passing -H to sudo ensures that $HOME is the root user's home directory in the test -> which then gets inherited when exec --user=runner --group=runner is invoked, but the runner user does not have access to the inherited root user's home directory. Please let me know if this made no sense and I can attempt to clarify better

ubuntu@wellington:~/mnt/pebble$ gs
On branch fix/exec_as_user
ubuntu@wellington:~/mnt/pebble$ go test -c ./internals/daemon/
ubuntu@wellington:~/mnt/pebble$ PEBBLE_TEST_USER=ubuntu PEBBLE_TEST_GROUP=ubuntu sudo -E -H ./daemon.test -check.v -check.f ^execSuite\.TestUserGroup$                                 
2023-08-07T13:10:53.063Z [test] Started daemon.
2023-08-07T13:10:53.069Z [test] POST /v1/exec 4.923782ms 202
2023-08-07T13:10:53.073Z [test] GET /v1/tasks/1/websocket/control 3.909042ms 200
2023-08-07T13:10:53.074Z [test] GET /v1/tasks/1/websocket/stdio 195.794µs 200
2023-08-07T13:10:53.074Z [test] GET /v1/tasks/1/websocket/stderr 156.063µs 200
2023-08-07T13:10:53.086Z [test] GET /v1/changes/1/wait 12.082336ms 200
PASS: internals/daemon/api_exec_test.go:232: execSuite.TestUserGroup    0.024s
OK: 1 passed
PASS

ubuntu@wellington:~/mnt/pebble$ gc master
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
ubuntu@wellington:~/mnt/pebble$ go test -c ./internals/daemon/
ubuntu@wellington:~/mnt/pebble$ PEBBLE_TEST_USER=ubuntu PEBBLE_TEST_GROUP=ubuntu sudo -E -H ./daemon.test -check.v -check.f ^execSuite\.TestUserGroup$ 
2023-08-07T13:11:58.610Z [test] Started daemon.
2023-08-07T13:11:58.620Z [test] POST /v1/exec 9.088091ms 202
2023-08-07T13:11:58.626Z [test] GET /v1/tasks/1/websocket/control 4.28942ms 200
2023-08-07T13:11:58.626Z [test] GET /v1/tasks/1/websocket/stdio 168.925µs 200
2023-08-07T13:11:58.627Z [test] GET /v1/tasks/1/websocket/stderr 69.744µs 200
2023-08-07T13:11:58.642Z [test] GET /v1/changes/1/wait 14.689631ms 200

----------------------------------------------------------------------
FAIL: internals/daemon/api_exec_test.go:232: execSuite.TestUserGroup

internals/daemon/api_exec_test.go:246:
    c.Assert(waitErr, IsNil)
... value *errors.errorString = &errors.errorString{s:"cannot perform the following tasks:\n- exec command \"/bin/sh\" (fork/exec /bin/sh: permission denied)"} ("cannot perform the following tasks:\n- exec command \"/bin/sh\" (fork/exec /bin/sh: permission denied)")

OOPS: 0 passed, 1 FAILED
--- FAIL: Test (0.05s)
FAIL

[benhoyt]

Great, looks good! Two minor suggestions, but good to go either way.

[benhoyt]

I've just added my suggestions directly. I'll merge this PR as soon as @hpidcock has given his thumbs-up.

[hpidcock]

Thanks, I'm happy with this.

[benhoyt]

Thanks for this @shayancanonical!