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

Makefile: Fix potential uid/gid collision by using setpriv #1244

Merged
merged 1 commit into from
Nov 29, 2022

Conversation

tklauser
Copy link
Member

Follows cilium/hubble#821

The release build should run with the same numeric user and group id as the caller of the Makefile. This solves two problems:

  1. The generated artefacts should be owned by the user who invoked the Makefile
  2. go build -buildvcs=true (the default since Go 1.18) invokes git, and git requires that the .git folder is owned by the user invoking it.

Previously, we achieved this by creating a new user and group inside the container with the wanted uid/gid. The problem with that approach is it fails if the uid/gid is already taken (such as e.g. gid 123, which seems to be used by the GitHub runner as of recently). Therefore, instead of trying to create a new user, this commit now uses setpriv from util-linux to force the make process (and all its children) to run as the RELEASE_{UID/GID}. This ensures that both git can access the .git directory owned by the host user, and that the host user can access the generated artifacts. One limitation of setpriv is that it runs the child process without an accessible $HOME directory, which go build needs for the GOCACHE. To solve this for now, we point it to a temporary directory. In the future, we could consider using a GOCACHE owned by the host, to allow cache-reuse across builds.

@tklauser tklauser requested review from a team as code owners November 29, 2022 16:32
@tklauser tklauser requested a review from nbusseneau November 29, 2022 16:32
@tklauser tklauser temporarily deployed to ci November 29, 2022 16:32 Inactive
Copy link
Contributor

@michi-covalent michi-covalent left a comment

Choose a reason for hiding this comment

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

getting better day by day

Makefile Outdated Show resolved Hide resolved
The release build should run with the same numeric user and group id as
the caller of the Makefile. This solves two problems:

1. The generated artefacts should be owned by the user who invoked the
Makefile
2. `go build -buildvcs=true` (the default since Go 1.18) invokes git,
and git requires that the .git folder is owned by the user invoking
it.

Previously, we achieved this by creating a new user and group inside the
container with the wanted uid/gid. The problem with that approach is it
fails if the uid/gid is already taken (such as e.g. gid 123, which seems
to be used by the GitHub runner as of recently). Therefore, instead of
trying to create a new user, this commit now uses `setpriv` from
util-linux to force the `make` process (and all its children) to run as
the RELEASE_{UID/GID}. This ensures that both `git` can access the
`.git` directory owned by the host user, and that the host user can
access the generated artifacts. One limitation of `setpriv` is that it
runs the child process without an accessible `$HOME` directory, which
`go build` needs for the GOCACHE. To solve this for now, we point it to
a temporary directory. In the future, we could consider using a GOCACHE
owned by the host, to allow cache-reuse across builds.

Co-authored-by: Sebastian Wicki <[email protected]>
Signed-off-by: Tobias Klauser <[email protected]>
@tklauser tklauser force-pushed the pr/tklauser/release-setpriv branch from ecdd969 to 0ea7717 Compare November 29, 2022 16:40
@tklauser tklauser temporarily deployed to ci November 29, 2022 16:40 Inactive
@tklauser tklauser merged commit fada9d3 into master Nov 29, 2022
@tklauser tklauser deleted the pr/tklauser/release-setpriv branch November 29, 2022 17:21
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.

4 participants