Add username to /etc/passwd inside of container if --userns keep-id #6829

rhatdan · 2020-06-30T19:49:36Z

If I enter a continer with --userns keep-id, my UID will be present
inside of the container, but most likely my user will not be defined.

This patch will take information about the user and stick it into the
container.

Signed-off-by: Daniel J Walsh [email protected]

openshift-ci-robot · 2020-06-30T19:49:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mheon · 2020-06-30T19:52:28Z

libpod/container.go

+	NetNsCtr     string `json:"netNsCtr,omitempty"`
+	PIDNsCtr     string `json:"pidNsCtr,omitempty"`
+	UserNsCtr    string `json:"userNsCtr,omitempty"`
+	UserNsKeepId bool


This should be elsewhere, have a JSON tag, and a comment describing what it does

mheon · 2020-06-30T19:54:25Z

libpod/options.go

@@ -844,6 +844,19 @@ func WithPIDNSFrom(nsCtr *Container) CtrCreateOption {
 	}
 }

+// WithUserNSKeepID indicates that container should add user entry to passwd
+// file, since the UID will be mapped into the container, via user namespace
+func WithUserNSKeepId() CtrCreateOption {


Can we rename this? Maybe "WithAddCurrentUserToPasswd" or something more clear about what it does?

vrothberg · 2020-07-01T09:00:08Z

libpod/container_internal_linux.go

-	}
-	if err == nil {
-		return "", nil
+	originPasswdFile := filepath.Join(c.state.Mountpoint, "/etc/passwd")


I think it's time to either break this code into a separate function or move it into a pkg/passwd.

It was already really hard to brain-parse the code before but now we have four nested branches that let my brain parser overflow.

Breaking the code into a separate function would simplify it (as we can use returns), we could add unit tests and ideally add more comments.

mheon · 2020-07-01T15:14:18Z

libpod/container.go

@@ -278,6 +278,9 @@ type ContainerConfig struct {
 	User string `json:"user,omitempty"`
 	// Additional groups to add
 	Groups []string `json:"groups,omitempty"`
+	// AddCurrentUserPasswdEntry indicates that the current user passwd entry
+	// should be added to the /etc/passwd within the container
+	AddCurrentUserPasswdEntry bool


Can this have an omitempty so we minimize the amount of stuff saved to the DB?

vrothberg

LGTM

vrothberg · 2020-07-07T11:27:29Z

Looks like a bug in the tests. I'll check out the PR and have a look.

vrothberg

Yep, tests need some tweaking:

diff --git a/libpod/container_internal_linux_test.go b/libpod/container_internal_linux_test.go 
index 5d92ff6c612c..078cc53a73c9 100644                                                        
--- a/libpod/container_internal_linux_test.go                                                  
+++ b/libpod/container_internal_linux_test.go                                                  
@@ -7,6 +7,7 @@ import (                                                                       
        "os"                                                                                   
        "testing"                                                                              
                                                                                               
+       spec "github.com/opencontainers/runtime-spec/specs-go"                                 
        "github.com/stretchr/testify/assert"                                                   
 )                                                                                             
                                                                                               
@@ -20,6 +21,7 @@ func TestGenerateUserPasswdEntry(t *testing.T) {                             
        c := Container{                                                                        
                config: &ContainerConfig{                                                      
                        User: "123:456",                                                       
+                       Spec: &spec.Spec{},                                                    
                },                                                                             
                state: &ContainerState{                                                        
                        Mountpoint: "/does/not/exist/tmp/",                                    
@@ -29,12 +31,12 @@ func TestGenerateUserPasswdEntry(t *testing.T) {                           
        if err != nil {                                                                        
                t.Fatal(err)                                                                   
        }                                                                                      
-       assert.Equal(t, user, "123:x:123:456:container user:/:bin/sh\n")                       
+       assert.Equal(t, user, "123:x:123:456:container user:/:/bin/sh\n")                      
                                                                                               
        c.config.User = "567"                                                                  
        user, err = c.generateUserPasswdEntry()                                                
        if err != nil {                                                                        
                t.Fatal(err)                                                                   
        }                                                                                      
-       assert.Equal(t, user, "567:x:567:567:container user:/:bin/sh\n")                       
+       assert.Equal(t, user, "567:x:567:0:container user:/:/bin/sh\n")                        
 }

If I enter a continer with --userns keep-id, my UID will be present inside of the container, but most likely my user will not be defined. This patch will take information about the user and stick it into the container. Signed-off-by: Daniel J Walsh <[email protected]>

vrothberg

LGTM

mheon · 2020-07-07T17:19:43Z

/lgtm

- Issue containers#6735 : problem with multiple namespaces; confirms combinations of --userns=keep-id, --privileged, --user=XX - Issue containers#6829 : --userns=keep-id will add a /etc/passwd entry - Issue containers#6593 : podman exec, with --userns=keep-id, errors (test is currently skipped because issue remains live) ...and, addendum: add new helper function, remove_same_dev_warning. Some CI systems issue a warning on podman run --privileged: WARNING: The same type, major and minor should not be used for multiple devices. We already had special-case code to ignore than in the SELinux test, but now we're seeing it in the new run tests I added, so I've refactored the "ignore this warning" code and written tests for the removal code. Signed-off-by: Ed Santiago <[email protected]>

debarshiray · 2020-08-28T16:55:49Z

Shouldn't it also have created the group? See #6829 :)

Since Podman 2.0.5, containers that were created with 'podman create --userns=keep-id ...' automatically get the user added to /etc/passwd [1]. However, this user isn't as fully configured as it needs to be. The home directory is specified as "/" and the shell is /bin/sh. Therefore, the entry point needs to call usermod(8) to update the user, instead of using useradd(8) to create it. [1] Podman commit 6c6670f12a3e6b91 containers/podman#6829 containers#523

Since Podman 2.0.5, containers that were created with 'podman create --userns=keep-id ...' automatically get the user added to /etc/passwd [1]. However, this user isn't as fully configured as it needs to be. The home directory is specified as '/' and the shell is /bin/sh. Note that Podman doesn't add the user's login group to /etc/group [2]. This leads to the following error when entering the container: /usr/bin/id: cannot find name for group ID 1000 It's expected that this will be fixed in Podman itself. Therefore, the entry point needs to call usermod(8) to update the user, instead of using useradd(8) to create it. [1] Podman commit 6c6670f12a3e6b91 containers/podman#6829 [2] containers/podman#7389 containers#523

Since Podman 2.0.5, containers that were created with 'podman create --userns=keep-id ...' automatically get the user added to /etc/passwd [1]. However, this user isn't as fully configured as it needs to be. The home directory is specified as '/' and the shell is /bin/sh. Note that Podman doesn't add the user's login group to /etc/group [2]. This leads to the following error message when entering the container: /usr/bin/id: cannot find name for group ID 1000 It's expected that this will be fixed in Podman itself. Therefore, the entry point needs to call usermod(8) to update the user, instead of using useradd(8) to create it. [1] Podman commit 6c6670f12a3e6b91 containers/podman#6829 [2] containers/podman#7389 containers#523

openshift-ci-robot requested review from umohnani8 and vrothberg June 30, 2020 19:49

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 30, 2020

mheon reviewed Jun 30, 2020

View reviewed changes

rhatdan force-pushed the keepid branch from ef3d733 to c532bda Compare June 30, 2020 19:55

vrothberg reviewed Jul 1, 2020

View reviewed changes

rhatdan force-pushed the keepid branch 4 times, most recently from 1be270f to 2f00a31 Compare July 1, 2020 10:57

mheon reviewed Jul 1, 2020

View reviewed changes

rhatdan force-pushed the keepid branch from 2f00a31 to 7a27be9 Compare July 1, 2020 17:21

vrothberg reviewed Jul 2, 2020

View reviewed changes

rhatdan force-pushed the keepid branch 10 times, most recently from 904bc1b to 1eccb19 Compare July 7, 2020 10:19

vrothberg reviewed Jul 7, 2020

View reviewed changes

rhatdan force-pushed the keepid branch from 1eccb19 to 6c6670f Compare July 7, 2020 12:34

vrothberg reviewed Jul 7, 2020

View reviewed changes

openshift-ci-robot assigned mheon Jul 7, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 7, 2020

openshift-merge-robot merged commit 54d16f3 into containers:master Jul 7, 2020

edsantiago mentioned this pull request Jul 13, 2020

system tests: new tests for run, exec #6958

Merged

debarshiray mentioned this pull request Aug 28, 2020

sudo(8) inside toolbox containers asks for a password with Podman 2.0.5 containers/toolbox#523

Closed

debarshiray mentioned this pull request Aug 30, 2020

Unbreak 'sudo' inside toolbox containers with Podman 2.0.5 containers/toolbox#547

Merged

debarshiray mentioned this pull request Aug 22, 2023

Doesn't work if user is managed by Active Directory (contains @ "at" sign) containers/toolbox#585

Open

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 24, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add username to /etc/passwd inside of container if --userns keep-id #6829

Add username to /etc/passwd inside of container if --userns keep-id #6829

rhatdan commented Jun 30, 2020

openshift-ci-robot commented Jun 30, 2020

mheon Jun 30, 2020

mheon Jun 30, 2020

vrothberg Jul 1, 2020

rhatdan Jul 1, 2020

mheon Jul 1, 2020

vrothberg left a comment

vrothberg commented Jul 7, 2020

vrothberg left a comment

vrothberg left a comment

mheon commented Jul 7, 2020

debarshiray commented Aug 28, 2020 •

edited

Loading

Add username to /etc/passwd inside of container if --userns keep-id #6829

Add username to /etc/passwd inside of container if --userns keep-id #6829

Conversation

rhatdan commented Jun 30, 2020

openshift-ci-robot commented Jun 30, 2020

mheon Jun 30, 2020

Choose a reason for hiding this comment

mheon Jun 30, 2020

Choose a reason for hiding this comment

vrothberg Jul 1, 2020

Choose a reason for hiding this comment

rhatdan Jul 1, 2020

Choose a reason for hiding this comment

mheon Jul 1, 2020

Choose a reason for hiding this comment

vrothberg left a comment

Choose a reason for hiding this comment

vrothberg commented Jul 7, 2020

vrothberg left a comment

Choose a reason for hiding this comment

vrothberg left a comment

Choose a reason for hiding this comment

mheon commented Jul 7, 2020

debarshiray commented Aug 28, 2020 • edited Loading

debarshiray commented Aug 28, 2020 •

edited

Loading