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.

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

integrate github.com/opencontainers/runc/libcontainer/user #132

Closed
wants to merge 54 commits into from

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Sep 16, 2023

Integrate the github.com/opencontainers/runc/libcontainer/user package at
commit a3a0ec48c464fe6458d2ba067f6707c027e5384e (opencontainers/runc@a3a0ec4). This is the result of
the following steps:

# create a temporary clone of runc
cd ~/Projects
git clone https://github.com/opencontainers/runc.git libcontainer_user
cd libcontainer_user

# commit taken from
git rev-parse --verify HEAD
a3a0ec48c464fe6458d2ba067f6707c027e5384e

# remove all code, except for 'libcontainer/user', and rename to /user
git filter-repo \
  --path-glob 'libcontainer/user/*' \
  --path LICENSE \
  --path NOTICE \
  --path-rename libcontainer/user:user \
  --path-rename LICENSE:user/LICENSE \
  --path-rename NOTICE:user/NOTICE

# go to the target github.com/moby/sys repository
cd ~/go/src/github.com/moby/sys

# create a branch to work with
git checkout -b integrate_libcontainer_user

# add the temporary repository as an upstream and make sure it's up-to-date
git remote add libcontainer_user ~/Projects/libcontainer_user
git fetch libcontainer_user

# merge the upstream code
git merge --allow-unrelated-histories --signoff -S libcontainer_user/main

Solomon Hykes and others added 30 commits June 9, 2014 16:16
Signed-off-by: Solomon Hykes <solomon@docker.com>
Spotted this one when creating moby/moby#10938

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Michael Crosby <crosbymichael@gmail.com>
Signed-off-by: Michael Crosby <crosbymichael@gmail.com>
This moves much of the documentation on contributing and maintainer the
codebase from the libcontainer sub directory to the root of the repo.

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>
Move libcontainer documenation to root of repo
The old GetAdditionalGroups* API didn't match the rest of
libcontainer/user, we make functions that take io.Readers and then make
wrappers around them. Otherwise we have to do dodgy stuff when testing
our code.

Fixes: d4ece29c0bd38 ("refactor GetAdditionalGroupsPath")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Update the tests to use the test-friendly GetAdditionalGroups API,
rather than making random files for no good reason.

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
/etc/groups is not needed when specifying numeric group ids. This
change allows containers without /etc/groups to specify numeric
supplemental groups.

Signed-off-by: Sami Wagiaalla <swagiaal@redhat.com>
Export errors as variables when no matching entries are found in passwd or group file.

Signed-off-by: Thomas LE ROUX <thomas@november-eleven.fr>
Most shadow-related tools don't treat numeric ids as potential
usernames, so change our behaviour to match that. Previously, using an
explicit specification like 111:222 could result in the UID and GID not
being 111 and 222 respectively (which is confusing).

Signed-off-by: Aleksa Sarai <asarai@suse.de>
Signed-off-by: Aleksa Sarai <asarai@suse.de>
Some of the code was quite confusing inside libcontainer/user, so
refactor and comment it so future maintainers can understand what's
going and what edge cases we have to deal with.

Signed-off-by: Aleksa Sarai <asarai@suse.de>
libcontainer: user: always treat numeric ids numerically
Fixes: #941

Signed-off-by: Qiang Huang <h.huangqiang@huawei.com>
Signed-off-by: Lei Jitang <leijitang@huawei.com>
The parameters passed to `GetExecUser` is not correct.
Consider the following code:

```
package main

import (
	"fmt"
	"io"
	"os"
)

func main() {
	passwd, err := os.Open("/etc/passwd1")
	if err != nil {
		passwd = nil
	} else {
		defer passwd.Close()
	}

	err = GetUserPasswd(passwd)
	if err != nil {
		fmt.Printf("%#v\n", err)
	}
}

func GetUserPasswd(r io.Reader) error {
	if r == nil {
		return fmt.Errorf("nil source for passwd-formatted
data")
	} else {
		fmt.Printf("r = %#v\n", r)
	}
	return nil
}
```

If the file `/etc/passwd1` is not exist, we expect to return
`nil source for passwd-formatted data` error, and in fact, the func
`GetUserPasswd` return nil.

The same logic exists in runc code. this patch fix it.

Signed-off-by: Wang Long <long.wanglong@huawei.com>
Signed-off-by: Valentin Rothberg <vrothberg@suse.com>
Since syscall is outdated and broken for some architectures,
use x/sys/unix instead.

There are still some dependencies on the syscall package that will
remain in syscall for the forseeable future:

Errno
Signal
SysProcAttr

Additionally:
- os still uses syscall, so it needs to be kept for anything
returning *os.ProcessState, such as process.Wait.

Signed-off-by: Christy Perez <christy@linux.vnet.ibm.com>
libcontainer/user: add supplementary groups only for non-numeric users
Signed-off-by: Kenfe-Mickael Laventure <mickael.laventure@gmail.com>
Move user pkg unix specific calls to unix file
This reverts commit f013111, reversing
changes made to 51b501dab1889ca609db9c536ac976f0f53e7021.

Signed-off-by: Kenfe-Mickael Laventure <mickael.laventure@gmail.com>
runc currently only support Linux platform, and since we dont intend to expose
the support to other platform, removing all other platforms placeholder code.

`libcontainer/configs` still being used in
https://github.com/moby/moby/blob/master/daemon/daemon_windows.go so
keeping it for now.

After this, we probably should also rename files to drop linux suffices
if possible.

Signed-off-by: Daniel Dao <dqminh89@gmail.com>
This rearranges a bit of the user and group lookup, such that only a
basic subset is exposed.

Signed-off-by: Vincent Batts <vbatts@hashbangbash.com>
libcontainer/user: platform dependent calls
Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
This fixes the following compilation error on 32bit ARM:
```
$ GOARCH=arm GOARCH=6 go build ./libcontainer/system/
libcontainer/system/linux.go:119:89: constant 4294967295 overflows int
```

Signed-off-by: Tibor Vass <tibor@docker.com>
subgid is defined per user, not group (see subgid(5))

This commit also adds support for specifying subuid owner with a numeric UID.

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
The Err() method should be called after the Scan() loop, not inside it.

Found by

 git grep -A3 -F '.Scan()' | grep Err

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin and others added 8 commits July 2, 2021 12:25
Same as in other places (other parsers here, as well as golang os/user
parser and glibc parser all tolerate extra space at BOL and EOL).

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Lines in /etc/group longer than 64 characters breaks the current
implementation of group parser. This is caused by bufio.Scanner
buffer limit.

Fix by re-using the fix for a similar problem in golang os/user,
namely https://go-review.googlesource.com/c/go/+/283601.

Add some tests.

Co-authored-by: Andrey Bokhanko <andreybokhanko@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Go 1.17 introduce this new (and better) way to specify build tags.
For more info, see https://golang.org/design/draft-gobuild.

As a way to seamlessly switch from old to new build tags, gofmt (and
gopls) from go 1.17 adds the new tags along with the old ones.

Later, when go < 1.17 is no longer supported, the old build tags
can be removed.

Now, as I started to use latest gopls (v0.7.1), it adds these tags
while I edit. Rather than to randomly add new build tags, I guess
it is better to do it once for all files.

Mind that previous commits removed some tags that were useless,
so this one only touches packages that can at least be built
on non-linux.

Brought to you by

        go1.17 fmt ./...

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since Go 1.19, godoc recognizes lists, code blocks, headings etc. It
also reformats the sources making it more apparent that these features
are used.

Fix a few places where it misinterpreted the formatting (such as
indented vs unindented), and format the result using the gofumpt
from HEAD, which already incorporates gofmt 1.19 changes.

Some more fixes (and enhancements) might be required.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The exception was fixed by polyfloyd/go-errorlint#12
which eventually made its way into golangci-lint.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
ci: bump golangci-lint to v1.53, remove fixed exception
Integrate the github.com/opencontainers/runc/libcontainer/user package at
commit a3a0ec48c464fe6458d2ba067f6707c027e5384e. This is the result of
the following steps:

    # create a temporary clone of runc
    cd ~/Projects
    git clone https://github.com/opencontainers/runc.git libcontainer_user
    cd libcontainer_user

    # commit taken from
    git rev-parse --verify HEAD
    a3a0ec48c464fe6458d2ba067f6707c027e5384e

    # remove all code, except for 'libcontainer/user', and rename to /user
    git filter-repo \
      --path-glob 'libcontainer/user/*' \
      --path LICENSE \
      --path NOTICE \
      --path-rename libcontainer/user:user \
      --path-rename LICENSE:user/LICENSE \
      --path-rename NOTICE:user/NOTICE

    # go to the target github.com/moby/sys repository
    cd ~/go/src/github.com/moby/sys

    # create a branch to work with
    git checkout -b integrate_libcontainer_user

    # add the temporary repository as an upstream and make sure it's up-to-date
    git remote add libcontainer_user ~/Projects/libcontainer_user
    git fetch libcontainer_user

    # merge the upstream code
    git merge --allow-unrelated-histories --signoff -S libcontainer_user/main

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Member Author

One failure in a DCO sign-off; possibly could be fixed through a .mailmap, but I think this one is fine to ignore, so I manually set it to pass.

Commit sha: 0741b3a, Author: Thomas LE ROUX, Committer: Thomas LE ROUX; Expected "Thomas LE ROUX github@november-eleven.fr", but got "Thomas LE ROUX thomas@november-eleven.fr".

@thaJeztah thaJeztah marked this pull request as draft September 16, 2023 10:37
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Member Author

@cyphar @kolyshkin @tianon @AkihiroSuda PTAL

@@ -0,0 +1,191 @@

Apache License
Copy link
Member

Choose a reason for hiding this comment

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

The file may reside only in the root dir?

Copy link
Member Author

Choose a reason for hiding this comment

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

We have a license at the root, and I was considering skipping this one; wasn't sure if the NOTICE could be in the subdirectory without the license next to it, but maybe that's fine

@tianon
Copy link
Member

tianon commented Sep 16, 2023

If you include path "user/" you might get more (older) history too

@tianon
Copy link
Member

tianon commented Sep 16, 2023

(it originally lived in "github.com/docker/docker/pkg/user" but I think Crosby wouldn't let me copy the history at the time)

@tianon
Copy link
Member

tianon commented Sep 16, 2023

All four commits touching pkg/user in Docker are now in https://github.com/tianon/moby-sys/commits/docker-pkg-user-commits, if you wanted them. 😅

(I originally included LICENSE and NOTICE as you did here, but Docker's have had a lot of touches over the years so it made the history very polluted and doesn't seem all that important since we're all Apache 2.0 across all the repos this has moved between.)

@thaJeztah
Copy link
Member Author

Oh! is there a way to integrate that history from what you have? (also feel more than free to either push to my branch or to push an alternative ☺️)

@tianon
Copy link
Member

tianon commented Sep 16, 2023

https://github.com/tianon/moby-sys/commits/user-commits is my best attempt so far (although I've also ditched NOTICE and LICENSE completely here for the reasons stated above) -- this includes a squash of my two commit pair that removed the code from docker and the one that added it here, which gives a much nicer diff for that commit

@tianon
Copy link
Member

tianon commented Sep 16, 2023

(the squashed commit in question being tianon@e70ad14)

@tianon
Copy link
Member

tianon commented Sep 16, 2023

Prior to the commits in that history, it was a very short function in utils/utils.go, which makes it hard to make the history go any further than this. 😅

@tianon
Copy link
Member

tianon commented Sep 16, 2023

moby/moby@b07314e
moby/moby@e41507b
moby/moby@eb38750

being the only other three possibly relevant commits, but difficult to extract from 🙈

@tianon
Copy link
Member

tianon commented Sep 16, 2023

(feel free to take and use any of this [or not] -- I'm out of time for this morning 🙈)

@thaJeztah
Copy link
Member Author

@AkihiroSuda @tianon I opened #134 with the alternative (based on your branch)

@tianon tianon closed this in #134 Sep 18, 2023
@thaJeztah thaJeztah deleted the integrate_libcontainer_user branch September 18, 2023 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet