Skip to content

fix: lie about ownership of all files in build context when running as non-root user #28

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

Closed
wants to merge 5 commits into from

Conversation

johnstcn
Copy link
Member

This PR modifies and builds on our previous lies:

  • All files in the build context are eligible to have their UID/GID reported as 0:0.
  • This will only occur if the process calling Kaniko is running as a non-root user.

@johnstcn johnstcn self-assigned this Sep 18, 2024
@johnstcn johnstcn requested review from mafredri and mtojek September 19, 2024 08:19
Copy link
Member

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

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

Approving to unblock, but I think we should at least open an issue to remove this bit of code in favor of a fs wrapper.

pkg/util/util.go Outdated
// operation is not root. This means that the cache probe operation will
// fail unless we lie about the UID/GID of the files used to build the
// image.
lyingAboutOwnership := !fi.IsDir() && !isRoot()
Copy link
Member

Choose a reason for hiding this comment

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

I would like to see this logic moved to a filesystem wrapper, one we set in envbuilder, and only for cache probing. The isRoot check is probably a good approximation to running a cache probe, but it still makes me a bit uneasy. 😅

Should we at least limit this to the workspace directory? (Not sure how we would pass that info over to Kaniko in a clean way, though.)

@johnstcn
Copy link
Member Author

Closing in favour of #29

@johnstcn johnstcn closed this Sep 24, 2024
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.

3 participants