Resolve run USER names in a chroot, change default run group to 0 #313
Conversation
nalind
force-pushed
the
user2
branch
2 times, most recently
from
ac70618
to
bed9a3a
Compare
|
We are going to need this for kpod run also... |
user_linux_cgo.go
Outdated
| defer unix.Close(rootfd) | ||
|
|
||
| // Start resolving the pathname. | ||
| components := strings.Split(filepath.Clean(filename), string(os.PathSeparator)) |
There was a problem hiding this comment.
Why are you cleaning filename a second time?
user_linux_cgo.go
Outdated
| pwd := C.fgetpwent(f) | ||
| for pwd != nil { | ||
| if uint64(pwd.pw_uid) != userid { | ||
| pwd = C.fgetpwent(f) |
There was a problem hiding this comment.
Won't this go into in infinite loop?
There was a problem hiding this comment.
It shouldn't? We either (read another entry, and either iterate or fall out of the loop) or (return).
There was a problem hiding this comment.
Ok never mind, I forgot what fgetpwent was doing.
user_linux_cgo.go
Outdated
| grp := C.fgetgrent(f) | ||
| for grp != nil { | ||
| if C.strcmp(grp.gr_name, name) != 0 { | ||
| grp = C.fgetgrent(f) |
There was a problem hiding this comment.
It shouldn't? We either (read another entry, and either iterate or fall out of the loop) or (return).
tests/run.bats
Outdated
| buildah config $cid -u ${testotheruid} | ||
| buildah run -- $cid id | ||
| run buildah --debug=false run -- $cid id -u | ||
| [ "$output" = $testotheruid ] |
There was a problem hiding this comment.
Should you check status as well?
|
LGTM. |
|
@umohnani8 @mitrmac PTAL |
|
LGTM |
nalind
force-pushed
the
user2
branch
2 times, most recently
from
c4da284
to
acd7690
Compare
|
Rewrote the parsing logic to use less hand-rolled code and take advantage of bufio. |
nalind
force-pushed
the
user2
branch
2 times, most recently
from
c9f2af0
to
245feb4
Compare
|
Simplified further. |
|
@mheon PTAL |
user_linux.go
Outdated
| } | ||
| } | ||
| // That last read was the end of a line, so now we try to read the (beginning of?) the next line. | ||
| line, isPrefix, err = rc.ReadLine() |
There was a problem hiding this comment.
OK, it's past my "it's 8:00, don't ask questions" time, but I gotta ask. If the first line to be read is too long for the buffer and we spin through to the end of the line, we then do another ReadLine. Won't that first line be ignored? I'm guessing it's rarely if ever going to be an issue for group or passwd, but I'm curious.
There was a problem hiding this comment.
Yup, this is an attempt to avoid trying to parse any part of a line that we can't read in its entirety. The older version of this function used an 8192 byte buffer, but bufio defaults to something smaller, and particularly for group entries, I'm wary of lines that are too long to fit (i.e., groups with an astonishing number of members).
There was a problem hiding this comment.
OK, I'm a little leary about skipping a line. Can/should you a debug log here to note which lines are skipped?
|
LGTM, one question on ReadLine functionality which is most likely going to be educational for me. |
user_linux.go
Outdated
| } | ||
|
|
||
| func parseNextPasswd(rc *bufio.Reader) *lookupPasswdEntry { | ||
| line, isPrefix, err := rc.ReadLine() |
There was a problem hiding this comment.
This line retrieval code looks to be shared between parseNextPasswd and parseNextGroup - can probably be moved into a function that returns a line and error that both of them call
|
Code mostly looks fine, though I think I share the concerns of @TomSweeneyRedHat about potentially discarding incompletely-read lines. I feel like this might happen on occasion, considering we're reading from another process's STDOUT |
|
LGTM |
Switch fopenContainerFile from using Stat/Lstat after opening the file to using openat() to walk the given path, resolving links to keep them from escaping the container's root fs. This should resolve #66. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
Drop fallbacks for resolving USER values that attempt to look up names on the host, since that's never predictable. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
Add a test that makes sure we catch cases where we attempt to open a file in the container's tree that's actually a symlink that points out of the tree. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
Change our behavior when we're given USER with a numeric UID and no GID, so that we no longer error out if the UID doesn't correspond to a known user so that we can use that user's primary GID. Instead, use GID 0. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
* Use chroot() instead of trying to read the right file ourselves. This should resolve #66. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
Add a test that makes sure that "buildah run" fails if it can't resolve the name of the user for the container. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
Always be sure to check $status after using the run helper. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
|
Rebased, added a debug log message, and factored the ReadLine() calling into its own function. |
There was a problem hiding this comment.
LGTM. Thanks for adding the debug. Also like the ReadWholeLine function, good thought @mheon
|
LGTM |
|
📌 Commit 57f4135 has been approved by |
Drop fallbacks for resolving USER values that attempt to look up names on the host, since that's never predictable. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com> Closes: #313 Approved by: rhatdan
Add a test that makes sure we catch cases where we attempt to open a file in the container's tree that's actually a symlink that points out of the tree. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com> Closes: #313 Approved by: rhatdan
Change our behavior when we're given USER with a numeric UID and no GID, so that we no longer error out if the UID doesn't correspond to a known user so that we can use that user's primary GID. Instead, use GID 0. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com> Closes: #313 Approved by: rhatdan
Add a test that makes sure that "buildah run" fails if it can't resolve the name of the user for the container. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com> Closes: #313 Approved by: rhatdan
Always be sure to check $status after using the run helper. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com> Closes: #313 Approved by: rhatdan
|
☀️ Test successful - status-papr |
shebang presence causes rpmlint error: "non-executable-script /usr/share/bash-completion/completions/podman 644 /bin/bash" completions aren't executable in themselves so there's no need for a shebang there. Signed-off-by: Lokesh Mandvekar <lsm5@fedoraproject.org> Closes: #313 Approved by: rhatdan
This update changes things so that we chroot() before attempting to read
passwdorgroupfiles in a container's root, to try to avoid symlink traversal issues.It also corrects the default behavior when a numeric UID is specified for a container's run, but no group information is provided, to use group 0 rather than attempting to convert the UID to a user name and primary group, or looking up the UID as if it were a name.