Fix and improve file permission linters; update tests to mirror actual usage

Signed-off-by: egibs <[email protected]>

Author: egibs

docs/md/melange_build.md

@@ -53,8 +53,8 @@ melange build [flags]
   -i, --interactive                                             when enabled, attaches stdin with a tty to the pod on failure
   -k, --keyring-append strings                                  path to extra keys to include in the build environment keyring
       --license string                                          license to use for the build config file itself (default "NOASSERTION")
-      --lint-require strings                                    linters that must pass (default [dev,infodir,tempdir,usrmerge,varempty])
-      --lint-warn strings                                       linters that will generate warnings (default [cudaruntimelib,lddcheck,maninfo,object,opt,pkgconf,python/docs,python/multiple,python/test,setuidgid,srv,strip,usrlocal,worldwrite])
+      --lint-require strings                                    linters that must pass (default [dev,infodir,setuidgid,tempdir,usrmerge,varempty,worldwrite])
+      --lint-warn strings                                       linters that will generate warnings (default [cudaruntimelib,lddcheck,maninfo,object,opt,pkgconf,python/docs,python/multiple,python/test,sbom,srv,strip,usrlocal])
       --memory string                                           default memory resources to use for builds
       --namespace string                                        namespace to use in package URLs in SBOM (eg wolfi, alpine) (default "unknown")
       --out-dir string                                          directory where packages will be output (default "./packages/")

docs/md/melange_lint.md

@@ -29,8 +29,8 @@ melange lint [flags]
 
 ```
   -h, --help                   help for lint
-      --lint-require strings   linters that must pass (default [dev,infodir,tempdir,usrmerge,varempty])
-      --lint-warn strings      linters that will generate warnings (default [cudaruntimelib,lddcheck,maninfo,object,opt,pkgconf,python/docs,python/multiple,python/test,setuidgid,srv,strip,usrlocal,worldwrite])
+      --lint-require strings   linters that must pass (default [dev,infodir,setuidgid,tempdir,usrmerge,varempty,worldwrite])
+      --lint-warn strings      linters that will generate warnings (default [cudaruntimelib,lddcheck,maninfo,object,opt,pkgconf,python/docs,python/multiple,python/test,sbom,srv,strip,usrlocal])
 ```
 
 ### Options inherited from parent commands

pkg/build/build.go

@@ -24,6 +24,7 @@ import (
 	"io"
 	"io/fs"
 	"log/slog"
+	"math"
 	"os"
 	"path/filepath"
 	"runtime"
@@ -37,10 +38,10 @@ import (
 	"chainguard.dev/apko/pkg/apk/apk"
 	apkofs "chainguard.dev/apko/pkg/apk/fs"
 	apko_build "chainguard.dev/apko/pkg/build"
-	"chainguard.dev/apko/pkg/tarfs"
 	apko_types "chainguard.dev/apko/pkg/build/types"
 	"chainguard.dev/apko/pkg/options"
 	"chainguard.dev/apko/pkg/sbom/generator/spdx"
+	"chainguard.dev/apko/pkg/tarfs"
 	"github.com/chainguard-dev/clog"
 	purl "github.com/package-url/packageurl-go"
 	"github.com/yookoala/realpath"
@@ -103,7 +104,7 @@ type Build struct {
 	WorkspaceDir    string
 	WorkspaceDirFS  apkofs.FullFS
 	WorkspaceIgnore string
-	GuestFS apkofs.FullFS
+	GuestFS         apkofs.FullFS
 	// Ordered directories where to find 'uses' pipelines.
 	PipelineDirs          []string
 	SourceDir             string
@@ -770,7 +771,7 @@ func (b *Build) BuildPackage(ctx context.Context) error {
 			}
 		}
 
-		// add the main package to the linter queue
+		// add the subpackage to the linter queue
 		lintTarget := linterTarget{
 			pkgName:  sp.Name,
 			disabled: sp.Checks.Disabled,
@@ -819,7 +820,7 @@ func (b *Build) BuildPackage(ctx context.Context) error {
 		uid := owner["uid"]
 		gid := owner["gid"]
 		if err := b.WorkspaceDirFS.Chown(path, uid, gid); err != nil {
-			log.Warnf("failed to change ownership of %s to %d:%d", path, uid, gid)
+			log.Warnf("failed to change ownership of %s to %d:%d: %v", path, uid, gid, err)
 		}
 	}
 
@@ -859,7 +860,12 @@ func (b *Build) BuildPackage(ctx context.Context) error {
 	// perform package linting
 	for _, lt := range linterQueue {
 		log.Infof("running package linters for %s", lt.pkgName)
-		path := filepath.Join(b.WorkspaceDir, melangeOutputDirName, lt.pkgName)
+
+		// lint the in-memory filesystem which contains the correct mode bits, xattrs, etc.
+		fsys, err := apkofs.Sub(b.WorkspaceDirFS, filepath.Join(melangeOutputDirName, lt.pkgName))
+		if err != nil {
+			return fmt.Errorf("failed to return filesystem for workspace subtree: %w", err)
+		}
 
 		// Downgrade disabled checks from required to warn
 		require := slices.DeleteFunc(b.LintRequire, func(s string) bool {
@@ -869,13 +875,13 @@ func (b *Build) BuildPackage(ctx context.Context) error {
 			return a == b
 		})
 
-		if err := linter.LintBuild(ctx, b.Configuration, lt.pkgName, path, require, warn); err != nil {
+		if err := linter.LintBuild(ctx, b.Configuration, lt.pkgName, require, warn, fsys); err != nil {
 			return fmt.Errorf("unable to lint package %s: %w", lt.pkgName, err)
 		}
 	}
 
 	// Perform all license related linting and analysis
-	if _, _, err := license.LicenseCheck(ctx, b.Configuration, apkofs.DirFS(ctx, b.WorkspaceDir)); err != nil {
+	if _, _, err := license.LicenseCheck(ctx, b.Configuration, b.WorkspaceDirFS); err != nil {
 		return fmt.Errorf("license check: %w", err)
 	}
 
@@ -1315,6 +1321,10 @@ func storeMetadata(dir string) (map[string]map[string][]byte, map[string]fs.File
 			return err
 		}
 
+		if d.IsDir() || d.Type()&fs.ModeSymlink == fs.ModeSymlink {
+			return nil
+		}
+
 		relPath, err := filepath.Rel(dir, path)
 		if err != nil {
 			return err
@@ -1324,25 +1334,24 @@ func storeMetadata(dir string) (map[string]map[string][]byte, map[string]fs.File
 		if err != nil {
 			return err
 		}
-		mode := fi.Mode()
 
-		// Store ownership info, defaulting to root when unavailable
+		// store the file mode bits for every relative path in the provided directory
+		// some paths may not be related to or used by the package itself (i.e., they may be candidates for cleanup)
+		// but we may want to have them for future linting at the very least
+		modes[relPath] = fi.Mode()
+
+		// Store ownership info, defaulting to root when unavailable or invalid
 		owners[relPath] = map[string]int{
 			"uid": 0,
 			"gid": 0,
 		}
 		if stat, ok := fi.Sys().(*syscall.Stat_t); ok {
-			owners[relPath]["uid"] = int(stat.Uid)
-			owners[relPath]["gid"] = int(stat.Gid)
-		}
-
-		if d.IsDir() || d.Type()&fs.ModeSymlink == fs.ModeSymlink {
-			return nil
-		}
-
-		// If the path is within the melange-out directory, store the relative path and mode bits
-		if strings.Contains(path, melangeOutputDirName) {
-			modes[relPath] = mode
+			if stat.Uid <= math.MaxInt32 {
+				owners[relPath]["uid"] = int(stat.Uid)
+			}
+			if stat.Gid <= math.MaxInt32 {
+				owners[relPath]["gid"] = int(stat.Gid)
+			}
 		}
 
 		size, err := unix.Listxattr(path, nil)

pkg/build/test_test.go

@@ -103,7 +103,7 @@ func TestBuildWorkspaceConfig(t *testing.T) {
 			name: "test - with cache dir, exists",
 			t: func() *Test {
 				cacheT := baseTest
-				cacheT.CacheDir = tmpDir
+				cacheT.CacheDir = tmpDirReal
 				return &cacheT
 			}(),
 			want: func() *container.Config {
@@ -116,15 +116,15 @@ func TestBuildWorkspaceConfig(t *testing.T) {
 			name: "test - with cache dir, exists, environment",
 			t: func() *Test {
 				cacheT := baseTest
-				cacheT.CacheDir = tmpDir
+				cacheT.CacheDir = tmpDirReal
 				return &cacheT
 			}(),
 			env: map[string]string{"FOO": "bar", "BAZ": "zzz"},
 			want: func() *container.Config {
 				want := wantBase
 				want.Mounts = append(want.Mounts, container.BindMount{Source: tmpDirReal, Destination: "/var/cache/melange"})
 				want.Environment = map[string]string{"FOO": "bar", "BAZ": "zzz", "HOME": "/root"}
-				want.CacheDir = tmpDir
+				want.CacheDir = tmpDirReal
 				return &want
 			}(),
 		},

pkg/config/config.go

@@ -1240,11 +1240,11 @@ func replaceEntrypoint(r *strings.Replacer, in apko_types.ImageEntrypoint) apko_
 
 func replaceImageContents(r *strings.Replacer, in apko_types.ImageContents) apko_types.ImageContents {
 	return apko_types.ImageContents{
-		BuildRepositories:   replaceAll(r, in.BuildRepositories),
-		Repositories: replaceAll(r, in.Repositories),
-		Keyring:             replaceAll(r, in.Keyring),
-		Packages:            replaceAll(r, in.Packages),
-		BaseImage:           in.BaseImage, // TODO
+		BuildRepositories: replaceAll(r, in.BuildRepositories),
+		Repositories:      replaceAll(r, in.Repositories),
+		Keyring:           replaceAll(r, in.Keyring),
+		Packages:          replaceAll(r, in.Packages),
+		BaseImage:         in.BaseImage, // TODO
 	}
 }
 

pkg/linter/linter.go

@@ -27,6 +27,7 @@ import (
 	"path/filepath"
 	"regexp"
 	"slices"
+	"strconv"
 	"strings"
 
 	apkofs "chainguard.dev/apko/pkg/apk/fs"
@@ -122,12 +123,12 @@ var linterMap = map[string]linter{
 	"sbom": {
 		LinterFunc:      allPaths(sbomLinter),
 		Explain:         "Remove any files in /var/lib/db/sbom from the package",
-		defaultBehavior: Ignore, // TODO: needs work to be useful
+		defaultBehavior: Warn, // TODO: needs work to be useful
 	},
 	"setuidgid": {
 		LinterFunc:      isSetUIDOrGIDLinter,
 		Explain:         "Unset the setuid/setgid bit on the relevant files, or remove this linter",
-		defaultBehavior: Warn,
+		defaultBehavior: Require,
 	},
 	"srv": {
 		LinterFunc:      allPaths(srvLinter),
@@ -151,8 +152,8 @@ var linterMap = map[string]linter{
 	},
 	"worldwrite": {
 		LinterFunc:      worldWriteableLinter,
-		Explain:         "Change the permissions of any world-writeable files in the package, disable the linter, or make this a -compat package",
-		defaultBehavior: Warn,
+		Explain:         "Change the permissions of any permissive files in the package, disable the linter, or make this a -compat package",
+		defaultBehavior: Require,
 	},
 	"strip": {
 		LinterFunc:      strippedLinter,
@@ -267,15 +268,16 @@ func isSetUIDOrGIDLinter(ctx context.Context, _ *config.Configuration, _ string,
 		if err != nil {
 			return err
 		}
-		if isIgnoredPath(path) {
-			return nil
-		}
 
 		info, err := d.Info()
 		if err != nil {
 			return err
 		}
 
+		if !d.Type().IsRegular() { // Don't worry about non-files
+			return nil
+		}
+
 		mode := info.Mode()
 		if mode&fs.ModeSetuid != 0 {
 			return fmt.Errorf("file is setuid")
@@ -287,8 +289,8 @@ func isSetUIDOrGIDLinter(ctx context.Context, _ *config.Configuration, _ string,
 }
 
 func sbomLinter(_ context.Context, _ *config.Configuration, _, path string) error {
-	if strings.HasPrefix(path, "var/lib/db/sbom/") {
-		return fmt.Errorf("package writes to %s", path)
+	if filepath.Dir(path) == "var/lib/db/sbom" && !strings.HasSuffix(path, ".spdx.json") {
+		return fmt.Errorf("package writes to %s", filepath.Dir(path))
 	}
 	return nil
 }
@@ -338,9 +340,6 @@ func worldWriteableLinter(ctx context.Context, _ *config.Configuration, pkgname
 		if err != nil {
 			return err
 		}
-		if isIgnoredPath(path) {
-			return nil
-		}
 
 		if !d.Type().IsRegular() { // Don't worry about non-files
 			return nil
@@ -352,11 +351,13 @@ func worldWriteableLinter(ctx context.Context, _ *config.Configuration, pkgname
 		}
 
 		mode := info.Mode()
+		perm := fmt.Sprintf("0%s", strconv.FormatUint(uint64(mode.Perm()), 8))
+
 		if mode&0o002 != 0 {
 			if mode&0o111 != 0 {
-				return fmt.Errorf("world-writeable executable file found in package (security risk): %s", path)
+				return fmt.Errorf("world-writeable executable file found in package (security risk): %s - %s", path, perm)
 			}
-			return fmt.Errorf("world-writeable file found in package: %s", path)
+			return fmt.Errorf("world-writeable file found in package: %s - %s", path, perm)
 		}
 		return nil
 	})
@@ -700,18 +701,18 @@ func checkLinters(linters []string) error {
 }
 
 // Lint the given build directory at the given path
-func LintBuild(ctx context.Context, cfg *config.Configuration, packageName string, path string, require, warn []string) error {
+func LintBuild(ctx context.Context, cfg *config.Configuration, packageName string, require, warn []string, fsys apkofs.FullFS) error {
 	if err := checkLinters(append(require, warn...)); err != nil {
 		return err
 	}
 
 	log := clog.FromContext(ctx)
-	fsys := apkofs.DirFS(ctx, path)
+	log.Infof("linting apk: %s", packageName)
 
 	if err := lintPackageFS(ctx, cfg, packageName, fsys, warn); err != nil {
 		log.Warn(err.Error())
 	}
-	log.Infof("linting apk: %s", packageName)
+
 	return lintPackageFS(ctx, cfg, packageName, fsys, require)
 }