Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

use /etc/passwd in place of the User struct #191

Closed
wants to merge 1 commit into from
Closed

use /etc/passwd in place of the User struct #191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ type Spec struct {
type Process struct {
// Terminal creates an interactive terminal for the container.
Terminal bool `json:"terminal"`
// User specifies user information for the process.
User User `json:"user"`
// User specifies user name for the process.
User string `json:"user"`
// Args specifies the binary and arguments for the application to execute.
Args []string `json:"args"`
// Env populates the process environment for the process.
Expand Down
14 changes: 2 additions & 12 deletions config.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,24 +71,14 @@ Each record in this array must have configuration in [runtime config](runtime-co
* **cwd** (string, optional) is the working directory that will be set for the executable.
* **env** (array of strings, optional) contains a list of variables that will be set in the process's environment prior to execution. Elements in the array are specified as Strings in the form "KEY=value". The left hand side must consist solely of letters, digits, and underscores `_` as outlined in [IEEE Std 1003.1-2001](http://pubs.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap08.html).
* **args** (string, required) executable to launch and any flags as an array. The executable is the first element and must be available at the given path inside of the rootfs. If the executable path is not an absolute path then the search $PATH is interpreted to find the executable.

The user for the process is a platform-specific structure that allows specific control over which user the process runs as.
For Linux-based systems the user structure has the following fields:

* **uid** (int, required) specifies the user id.
* **gid** (int, required) specifies the group id.
* **additionalGids** (array of ints, optional) specifies additional group ids to be added to the process.
* **user** (string, required) is the user name for the process. The runtime should resolve the required information for the process by its platform-specific ways. For Linux-based containers, the runtime could parse the /etc/passwd and /etc/group inside the rootfs.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

“could parse /etc/passwd and /etc/group” is too wishy. A bundle author will need to know exactly how this lookup will be performed, so they can setup their bundle to support it. How about using the appc wording referenced from #38?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getent is really the only correct way to handle this. Perhaps we just say if you want a UID map you can put that into your config.json similar to the mount points? https://github.com/opencontainers/specs/blob/master/config.md#mount-points

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Tue, Dec 22, 2015 at 11:07:42PM -0800, Brandon Philips wrote:

+* user (string, required) is the user name for the
process. The runtime should resolve the required information for
the process by its platform-specific ways. For Linux-based
containers, the runtime could parse the /etc/passwd and /etc/group
inside the rootfs.

getent is really the only correct way to handle this.

I don't buy that ;). For example, I don't see much difference
between:

Perhaps we just say if you want a UID map you can put that into your
config.json similar to the mount points?
https://github.com/opencontainers/specs/blob/master/config.md#mount-points

and defining the mappings in /etc/passwd and /etc/group. I think you
cover a useful subset of NSS implementations by supporting /etc/passwd
and /etc/group, and you allow runtimes to avoid running container-side
code to perform the lookup (for example, they can use a host-side NSS
implementation to parse the bundle's /etc/passwd and /etc/group).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should let random files inside of the container affect behavior of the runtime. And this is what /etc/passwd parsing does. It lets the contents inside of the container control the UID the runtime executes processes as.

We could trivially create a tool that extracts the /etc/passwd into a map and puts it into the JSON schema. Thoughts @jonboulle @vbatts ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Wed, Dec 23, 2015 at 11:19:16AM -0800, Brandon Philips wrote:

+* user (string, required) is the user name for the
process. The runtime should resolve the required information for
the process by its platform-specific ways. For Linux-based
containers, the runtime could parse the /etc/passwd and
/etc/group inside the rootfs.

I don't think we should let random files inside of the container
affect behavior of the runtime. And this is what /etc/passwd parsing
does. It lets the contents inside of the container control the UID
the runtime executes processes as.

I don't see a point in distinguishing between “inside the container”
(just /etc/passwd) and “inside the bundle” (both /etc/passwd and the
OCI configs). What do you gain by making such a distinction?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The /etc/passwd may be modified at runtime by processes executing inside of that filesystem while the manifest is inaccessible. So, if we have operations like "launch another process" the behavior may change based on the new state of that file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Wed, Dec 23, 2015 at 11:42:49AM -0800, Brandon Philips wrote:

+* user (string, required) is the user name for the
process. The runtime should resolve the required information for
the process by its platform-specific ways. For Linux-based
containers, the runtime could parse the /etc/passwd and
/etc/group inside the rootfs.

The /etc/passwd may be modified at runtime by processes executing
inside of that filesystem while the manifest is inaccessible. So, if
we have operations like "launch another process" the behavior may
change based on the new state of that file.

Yeah, this lookup should happen in the container process after joining
the namespace but before exec'ing the user-configured code. So if
/etc/passwd changes and you launch a new process, you get the new UID.
I don't have a problem with that (it seems like what the user intended
when they modified /etc/passwd).


*Example (Linux)*

```json
"process": {
"terminal": true,
"user": {
"uid": 1,
"gid": 1,
"additionalGids": [5, 6]
},
"user": "root",
"env": [
"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin",
"TERM=xterm"
Expand Down
11 changes: 0 additions & 11 deletions config_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,3 @@ type Linux struct {
// Capabilities are linux capabilities that are kept for the container.
Capabilities []string `json:"capabilities"`
}

// User specifies linux specific user and group information for the container's
// main process.
type User struct {
// UID is the user id.
UID int32 `json:"uid"`
// GID is the group id.
GID int32 `json:"gid"`
// AdditionalGids are additional group ids set for the container's process.
AdditionalGids []int32 `json:"additionalGids"`
}