Preserve file and directory mount permissions

Before that change, all directory permissions were `0755` and all file
permissions had `0700`, without taking umask into consideration.

We now save the file permissions and map them correctly into the target
container.

All necessary integration tests have been added as well.

Signed-off-by: Sascha Grunert <[email protected]>

Closes: containers#1833
Approved by: rhatdan

cmd/buildah/common.go

@@ -7,6 +7,7 @@ import (
  "time"
 
  "github.com/containers/buildah"
+ "github.com/containers/buildah/pkg/umask"
  "github.com/containers/buildah/pkg/unshare"
  is "github.com/containers/image/storage"
  "github.com/containers/image/types"
@@ -103,7 +104,7 @@ func getStore(c *cobra.Command) (storage.Store, error) {
  options.GIDMap = gidmap
  }
 
- checkUmask()
+ umask.CheckUmask()
 
  store, err := storage.GetStore(options)
  if store != nil {

pkg/secrets/secrets.go

@@ -7,6 +7,7 @@ import (
  "path/filepath"
  "strings"
 
+ "github.com/containers/buildah/pkg/umask"
  "github.com/containers/storage/pkg/idtools"
  rspec "github.com/opencontainers/runtime-spec/specs-go"
  "github.com/opencontainers/selinux/go-selinux/label"
@@ -28,20 +29,22 @@ var (
 
 // secretData stores the name of the file and the content read from it
 type secretData struct {
- name string
- data []byte
+ name string
+ data []byte
+ mode os.FileMode
+ dirMode os.FileMode
 }
 
 // saveTo saves secret data to given directory
 func (s secretData) saveTo(dir string) error {
  path := filepath.Join(dir, s.name)
- if err := os.MkdirAll(filepath.Dir(path), 0700); err != nil && !os.IsExist(err) {
+ if err := os.MkdirAll(filepath.Dir(path), s.dirMode); err != nil && !os.IsExist(err) {
  return err
  }
- return ioutil.WriteFile(path, s.data, 0700)
+ return ioutil.WriteFile(path, s.data, s.mode)
 }
 
-func readAll(root, prefix string) ([]secretData, error) {
+func readAll(root, prefix string, parentMode os.FileMode) ([]secretData, error) {
  path := filepath.Join(root, prefix)
 
  data := []secretData{}
@@ -56,7 +59,7 @@ func readAll(root, prefix string) ([]secretData, error) {
  }
 
  for _, f := range files {
- fileData, err := readFile(root, filepath.Join(prefix, f.Name()))
+ fileData, err := readFileOrDir(root, filepath.Join(prefix, f.Name()), parentMode)
  if err != nil {
  // If the file did not exist, might be a dangling symlink
  // Ignore the error
@@ -71,7 +74,7 @@ func readAll(root, prefix string) ([]secretData, error) {
  return data, nil
 }
 
-func readFile(root, name string) ([]secretData, error) {
+func readFileOrDir(root, name string, parentMode os.FileMode) ([]secretData, error) {
  path := filepath.Join(root, name)
 
  s, err := os.Stat(path)
@@ -80,7 +83,7 @@ func readFile(root, name string) ([]secretData, error) {
  }
 
  if s.IsDir() {
- dirData, err := readAll(root, name)
+ dirData, err := readAll(root, name, s.Mode())
  if err != nil {
  return nil, err
  }
@@ -90,12 +93,17 @@ func readFile(root, name string) ([]secretData, error) {
  if err != nil {
  return nil, err
  }
- return []secretData{{name: name, data: bytes}}, nil
+ return []secretData{{
+ name: name,
+ data: bytes,
+ mode: s.Mode(),
+ dirMode: parentMode,
+ }}, nil
 }
 
-func getHostSecretData(hostDir string) ([]secretData, error) {
+func getHostSecretData(hostDir string, mode os.FileMode) ([]secretData, error) {
  var allSecrets []secretData
- hostSecrets, err := readAll(hostDir, "")
+ hostSecrets, err := readAll(hostDir, "", mode)
  if err != nil {
  return nil, errors.Wrapf(err, "failed to read secrets from %q", hostDir)
  }
@@ -223,12 +231,16 @@ func addSecretsFromMountsFile(filePath, mountLabel, containerWorkingDir, mountPr
  return nil, err
  }
 
+ // Don't let the umask have any influence on the file and directory creation
+ oldUmask := umask.SetUmask(0)
+ defer umask.SetUmask(oldUmask)
+
  switch mode := fileInfo.Mode(); {
  case mode.IsDir():
- if err = os.MkdirAll(ctrDirOrFileOnHost, 0755); err != nil {
+ if err = os.MkdirAll(ctrDirOrFileOnHost, mode.Perm()); err != nil {
  return nil, errors.Wrapf(err, "making container directory %q failed", ctrDirOrFileOnHost)
  }
- data, err := getHostSecretData(hostDirOrFile)
+ data, err := getHostSecretData(hostDirOrFile, mode.Perm())
  if err != nil {
  return nil, errors.Wrapf(err, "getting host secret data failed")
  }
@@ -238,16 +250,16 @@ func addSecretsFromMountsFile(filePath, mountLabel, containerWorkingDir, mountPr
  }
  }
  case mode.IsRegular():
- data, err := readFile("", hostDirOrFile)
+ data, err := readFileOrDir("", hostDirOrFile, mode.Perm())
  if err != nil {
  return nil, errors.Wrapf(err, "error reading file %q", hostDirOrFile)
 
  }
  for _, s := range data {
- if err := os.MkdirAll(filepath.Dir(ctrDirOrFileOnHost), 0700); err != nil {
+ if err := os.MkdirAll(filepath.Dir(ctrDirOrFileOnHost), s.dirMode); err != nil {
  return nil, err
  }
- if err := ioutil.WriteFile(ctrDirOrFileOnHost, s.data, 0700); err != nil {
+ if err := ioutil.WriteFile(ctrDirOrFileOnHost, s.data, s.mode); err != nil {
  return nil, errors.Wrapf(err, "error saving data to container filesystem on host %q", ctrDirOrFileOnHost)
  }
  }

cmd/buildah/common_unix.go → pkg/umask/umask_unix.go

@@ -1,16 +1,20 @@
 // +build linux darwin
 
-package main
+package umask
 
 import (
  "syscall"
 
  "github.com/sirupsen/logrus"
 )
 
-func checkUmask() {
+func CheckUmask() {
  oldUmask := syscall.Umask(0022)
  if (oldUmask & ^0022) != 0 {
  logrus.Debugf("umask value too restrictive. Forcing it to 022")
  }
 }
+
+func SetUmask(value int) int {
+ return syscall.Umask(value)
+}

pkg/umask/umask_unsupported.go

@@ -0,0 +1,7 @@
+// +build !linux,!darwin
+
+package umask
+
+func CheckUmask() {}
+
+func SetUmask(int) int { return 0 }

tests/secrets.bats

@@ -3,27 +3,39 @@
 load helpers
 
 function setup() {
- # prepare the test mounts file
- mkdir $TESTSDIR/containers
- touch $TESTSDIR/containers/mounts.conf
- MOUNTS_PATH=$TESTSDIR/containers/mounts.conf
+ # create the files and directories to be tested
+ SECRETS_DIR=$TESTSDIR/rhel/secrets
+ mkdir -p $SECRETS_DIR
 
- # add the mounts entries
- echo "$TESTSDIR/rhel/secrets:/run/secrets" > $MOUNTS_PATH
- echo "$TESTSDIR/rhel/secrets" >> $MOUNTS_PATH
- echo "$TESTSDIR/rhel/secrets/test.txt:/test.txt" >> $MOUNTS_PATH
+ TESTFILE1=$SECRETS_DIR/test.txt
+ touch $TESTFILE1
+ TESTFILE_CONTENT="Testing secrets mounts. I am mounted!"
+ echo $TESTFILE_CONTENT > $TESTFILE1
 
- # create the files to be tested
- mkdir -p $TESTSDIR/rhel/secrets
- TESTFILE=$TESTSDIR/rhel/secrets/test.txt
- touch $TESTFILE
+ TESTFILE2=$SECRETS_DIR/file.txt
+ touch $TESTFILE2
+ chmod 604 $TESTFILE2
 
- TESTFILE_CONTENT="Testing secrets mounts. I am mounted!"
- echo $TESTFILE_CONTENT > $TESTSDIR/rhel/secrets/test.txt
+ TESTDIR1=$SECRETS_DIR/test-dir
+ mkdir -m704 $TESTDIR1
+
+ TESTFILE3=$TESTDIR1/file.txt
+ touch $TESTFILE3
+ chmod 777 $TESTFILE3
 
  mkdir -p $TESTSDIR/symlink/target
  touch $TESTSDIR/symlink/target/key.pem
- ln -s $TESTSDIR/symlink/target $TESTSDIR/rhel/secrets/mysymlink
+ ln -s $TESTSDIR/symlink/target $SECRETS_DIR/mysymlink
+
+ # prepare the test mounts file
+ mkdir $TESTSDIR/containers
+ touch $TESTSDIR/containers/mounts.conf
+ MOUNTS_PATH=$TESTSDIR/containers/mounts.conf
+
+ # add the mounts entries
+ echo "$SECRETS_DIR:/run/secrets" > $MOUNTS_PATH
+ echo "$SECRETS_DIR" >> $MOUNTS_PATH
+ echo "$TESTFILE1:/test.txt" >> $MOUNTS_PATH
 }
 
 function teardown() {
@@ -59,6 +71,18 @@ function teardown() {
  run_buildah --log-level=error run $cid cat /test.txt
  expect_output --substring $TESTFILE_CONTENT
 
+ # test permissions for a file-based mount
+ run_buildah --log-level=error run $cid stat -c %a /run/secrets/file.txt
+ expect_output --substring 604
+
+ # test permissions for a directory-based mount
+ run_buildah --log-level=error run $cid stat -c %a /run/secrets/test-dir
+ expect_output --substring 704
+
+ # test permissions for a file-based mount within a sub-directory
+ run_buildah --log-level=error run $cid stat -c %a /run/secrets/test-dir/file.txt
+ expect_output --substring 777
+
  # test a symlink
  run_buildah run $cid ls /run/secrets/mysymlink
  expect_output --substring "key.pem"