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.

Sign up for GitHub

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
Closed

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

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1acb95e
hack: lie more about ownership
johnstcn Sep 16, 2024
a6f6393
use sync.OnceValue
johnstcn Sep 18, 2024
e0a7370
improve comment + logging
johnstcn Sep 18, 2024
b7f13d0
skip failing test if not run as root
johnstcn Sep 23, 2024
f813228
move definition inside CacheHasher
johnstcn Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 17 additions & 13 deletions pkg/util/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,7 @@ import (
"io"
"math"
"os"
"path/filepath"
"strconv"
"strings"
"sync"
"syscall"
"time"
Expand All @@ -38,6 +36,15 @@ import (
"golang.org/x/sys/unix"
)

// In order to avoid failing cache probes when a cache probe operation
// is run as a non-root user, we need to pretend that files are owned
// by root.
var isRoot = sync.OnceValue(func() bool {
b := os.Getuid() == 0
logrus.Debugf("kaniko running as root: %t", b)
return b
})

// Hasher returns a hash function, used in snapshotting to determine if a file has changed
func Hasher() func(string) (string, error) {
pool := sync.Pool{
Expand Down Expand Up @@ -114,17 +121,14 @@ func CacheHasher() func(string) (string, error) {

h.Write([]byte(fi.Mode().String()))

// Cian: this is a disgusting hack, but it removes the need for the
// envbuilder binary to be owned by root when doing a cache probe.
// We want to ignore UID and GID changes for the envbuilder binary
// specifically. When building and pushing an image using the envbuilder
// image, the embedded envbuilder binary will most likely be owned by
// root:root. However, when performing a cache probe operation, it is more
// likely that the file will be owned by the UID/GID that is running
// envbuilder, which in this case is not guaranteed to be root.
// Let's just pretend that it is, cross our fingers, and hope for the best.
lyingAboutOwnership := !fi.IsDir() &&
strings.HasSuffix(filepath.Clean(filepath.Dir(p)), ".envbuilder.tmp")
// Cian: this is a disgusting hack. When building and pushing an image
// using the envbuilder image, the files in .envbuilder will most likely
// be owned by root:root. However, when performing a cache probe operation,
// it is almost certain that the UID/GID that is running the cache probe
// 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.)

if lyingAboutOwnership {
h.Write([]byte(strconv.FormatUint(uint64(0), 36)))
h.Write([]byte(","))
Expand Down
Loading