Use OS-default permissions when creating archive root

[ryanmoran]

Summary

The archive.WriteDirToTar function takes a boolean, includeRoot, as an argument that controls whether the tar archive created should include a root directory. It also allows users to pass a sentinel value of -1 when specifying the mode argument which results in the archive adopting the file permissions for the existing files and directories from the given source.

When these two features combine, we have some strange behavior. The root directory is created adhoc in the tar archive with its mode set to the given value of the mode argument. When that value is -1, this translates into an fs.FileMode, which is a type of uint32, with a value that looks like dalTLDpSugct?rwxrwxrwx. If that looks odd, it looks less so in octal, 37777777777. Simply said, the permission bits overflow and results in a value of 7777 or rwsrwsrwt being set.

The key issue here is that we've triggered the special bits on the permission set. Specifically, we're setting SUID, SGID, and Sticky. This is strange to do in a function that simply wants to archive a directory.

This change causes the archive.WriteDirToTar function to follow the more standardized default permissions for directory creation, that is to use 0777 and apply the umask. This results in much more reasonable permission settings for this root archive directory.

Output

Given the following program:

package main

import (
	"archive/tar"
	"bytes"
	"fmt"
	"io"
	"io/fs"
	"os"

	"github.com/buildpacks/pack/pkg/archive"
)

func main() {
	dir, err := os.MkdirTemp("", "")
	if err != nil {
		panic(err)
	}

	reader := archive.ReadDirAsTar(dir, "/workspace", 123, 456, -1, false, true, nil)
	buffer := bytes.NewBuffer(nil)
	_, err = io.Copy(buffer, reader)
	if err != nil {
		panic(err)
	}

	tr := tar.NewReader(buffer)
	for {
		hdr, err := tr.Next()
		if err == io.EOF {
			break
		}
		if err != nil {
			panic(err)
		}

		fmt.Printf("%s %s\n", fs.FileMode(hdr.Mode), hdr.Name)
	}
}

Before

dalTLDpSugct?rwxrwxrwx /workspace

After

-rwxr-xr-x /workspace

Documentation

• Should this change be documented?
  • Yes, see #___
  • No

[jjbustamante]

I think the original issue is 1121, and these changes make a lot of sense to me, preserving the original permissions from the host looks like the correct expected behavior.

[natalieparellano]

Options:

• Always use 777 for /workspace <- this is the least breaking change
• Always use 775 for /workspace
• Revert (use dalTLDpSugct?rwxrwxrwx)
• Do nothing (inherit from default for newly created directories on host)
• Inherit from permissions on host/workspace
• Inherit from permissions on builder/workspace
• See what we did 2 years ago before we made changes to archive package

Preference from 6/8 Working Group is to do the first option as the least breaking change, but come back to this when we do the builder spec.