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

ci, make: allow to build release as root user #1242

Merged
merged 1 commit into from
Nov 29, 2022

Conversation

tklauser
Copy link
Member

In local environments we usually want to keep the current behavior (i.e. release artifacts owned by the uid/gid of the user invoking make release). However in GitHub actions, the command recently started failing with:

addgroup: gid '123' in use

(see https://github.com/cilium/cilium-cli/actions/runs/3572725726/jobs/6006032172)

In GitHub actions, the ownership of the release artifacts shouldn't matter since they're ephemeral anyway. We can now use the following command in the release action:

RELEASE_UID=0 RELEASE_GID=0 RELEASER_USER= RELEASER_GROUP= make

In local environments we usually want to keep the current behavior (i.e.
release artifacts owned by the uid/gid of the user invoking `make
release`). However in GitHub actions, the command recently started
failing with:

    addgroup: gid '123' in use

(see https://github.com/cilium/cilium-cli/actions/runs/3572725726/jobs/6006032172)

In GitHub actions, the ownership of the release artifacts shouldn't
matter since they're ephemeral anyway. We can now use the following
command in the release action:

    RELEASE_UID=0 RELEASE_GID=0 RELEASER_USER= RELEASER_GROUP= make

Signed-off-by: Tobias Klauser <[email protected]>
@tklauser tklauser requested a review from gandro November 29, 2022 13:03
@tklauser tklauser requested review from a team as code owners November 29, 2022 13:03
@tklauser tklauser temporarily deployed to ci November 29, 2022 13:03 Inactive
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

In the past, we've had problems with the release artifacts owned by root, so they couldn't be deleted anymore without sudo. But maybe that only applies to local-release which does a rm on them?

@tklauser
Copy link
Member Author

In the past, we've had problems with the release artifacts owned by root, so they couldn't be deleted anymore without sudo. But maybe that only applies to local-release which does a rm on them?

I think this only applies to make local-release. It worked on a test tag I pushed on this PR's branch: https://github.com/cilium/cilium-cli/actions/runs/3574633584

@gandro
Copy link
Member

gandro commented Nov 29, 2022

In the past, we've had problems with the release artifacts owned by root, so they couldn't be deleted anymore without sudo. But maybe that only applies to local-release which does a rm on them?

I think this only applies to make local-release. It worked on a test tag I pushed on this PR's branch: https://github.com/cilium/cilium-cli/actions/runs/3574633584

Yes, I just double checked as well, the problem only exists if the user changes between the first and second call to make release (which isn't an issue on GHActions, and generally is rather rare).

However, I don't fully understand how building as root is not triggering the protection against CVE-2022-24765, which was the whole reason we added the release user in the first place 🤔

@gandro
Copy link
Member

gandro commented Nov 29, 2022

However, I don't fully understand how building as root is not triggering the protection against CVE-2022-24765, which was the whole reason we added the release user in the first place thinking

Ah! I guess because you added -buildvcs=false to go build. That explains it.

@tklauser
Copy link
Member Author

However, I don't fully understand how building as root is not triggering the protection against CVE-2022-24765, which was the whole reason we added the release user in the first place thinking

Ah! I guess because you added -buildvcs=false to go build. That explains it.

Yeah, that's the downside to it 😞 I couldn't find another way unfortunately.

Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Alternative solution: cilium/hubble#821

@michi-covalent michi-covalent merged commit 2991ec2 into master Nov 29, 2022
@michi-covalent michi-covalent deleted the pr/tklauser/release-target branch November 29, 2022 15:41
@tklauser
Copy link
Member Author

Alternative solution: cilium/hubble#821

Nice, thanks! TIL about setpriv. I'll adjust cilium-cli to that solution as well.

@tklauser
Copy link
Member Author

Alternative solution: cilium/hubble#821

Nice, thanks! TIL about setpriv. I'll adjust cilium-cli to that solution as well.

Addressed in #1244

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