MWI: Fix missing O_CLOEXEC in botfs.openSecure() and other issues (#55411)

* MWI: Fix missing O_CLOEXEC in `botfs.openSecure()` and other issues

This fixes various issues in botfs's `openSecure` implementation for
Linux, particularly:
- Added missing `O_CLOEXEC` flag to `openat2()` flags
- Added EINTR retry loop to follow the stdlib's `OpenFile()`
  implementation.
- Remove `O_CREATE` from `ReadFlags`, and convert it to a pseudo-enum
  so we can accurately predict intent (see next item)
- Pass `0` mode to `openat2()` when reading (see above)
- Renamed confusingly named `OpenMode` to `OpenFlags`, because it
  corresponds to flags, not file modes.
- Fixed coincidentally correct misuse of `unix.O_RDONLY`

Note that while removing `O_CREATE` from read flags is technically a
breaking change, we still automatically files downstream as needed.
This results in a "new" debug-level log, but not any actual behavior
change.

* Fix lint

* Simplify mode conditional, fix comments

* Handle potentially nil file returns from `os.OpenFile`

lib/tbot/botfs/botfs.go

@@ -66,12 +66,7 @@ const (
 
 	// ACLRequired enables ACL support and fails if ACLs are unavailable.
 	ACLRequired ACLMode = "required"
-)
 
-// OpenMode is a mode for opening files.
-type OpenMode int
-
-const (
 	// DefaultMode is the preferred permissions mode for bot files.
 	DefaultMode fs.FileMode = 0600
 
@@ -79,16 +74,32 @@ const (
 	// Directories need the execute bit set for most operations on their
 	// contents to succeed.
 	DefaultDirMode fs.FileMode = 0700
-
-	// ReadMode is the mode with which files should be opened for reading and
-	// writing.
-	ReadMode OpenMode = OpenMode(os.O_CREATE | os.O_RDONLY)
-
-	// WriteMode is the mode with which files should be opened specifically
-	// for writing.
-	WriteMode OpenMode = OpenMode(os.O_CREATE | os.O_WRONLY | os.O_TRUNC)
 )
 
+// OpenFlags is a bitmask containing flags passed to `open()`
+type OpenFlags int
+
+const (
+	// ReadFlags contains `open()` flags to be used when opening files for
+	// reading. The file will be created if it does not exist, and reads should
+	// return an empty byte array.
+	ReadFlags = iota
+
+	// WriteFlags is the mode with which files should be opened specifically
+	// for writing.
+	WriteFlags
+)
+
+// Flags returns opening flags for this OpenFlags variant.
+func (f OpenFlags) Flags() int {
+	switch f {
+	case WriteFlags:
+		return os.O_CREATE | os.O_WRONLY | os.O_TRUNC
+	default:
+		return os.O_RDONLY
+	}
+}
+
 // ACLOptions contains parameters needed to configure ACLs
 type ACLOptions struct {
 	// BotUser is the bot user that should have write access to this entry
@@ -124,10 +135,10 @@ func (s *ACLSelector) CheckAndSetDefaults() error {
 	return nil
 }
 
-// openStandard attempts to open the given path for reading and writing with
-// O_CREATE set.
-func openStandard(path string, mode OpenMode) (*os.File, error) {
-	file, err := os.OpenFile(path, int(mode), DefaultMode)
+// openStandard attempts to open the given path. The file may be writable
+// depending on the provided `OpenFlags` value.
+func openStandard(path string, flags OpenFlags) (*os.File, error) {
+	file, err := os.OpenFile(path, flags.Flags(), DefaultMode)
 	if err != nil {
 		return nil, trace.ConvertSystemError(err)
 	}
@@ -146,7 +157,7 @@ func createStandard(path string, isDir bool) error {
 		return nil
 	}
 
-	f, err := openStandard(path, WriteMode)
+	f, err := openStandard(path, WriteFlags)
 	if err != nil {
 		return trace.Wrap(err)
 	}

lib/tbot/botfs/fs_linux.go

@@ -28,7 +28,6 @@ import (
 	"io/fs"
 	"os"
 	"os/user"
-	"path/filepath"
 	"strconv"
 	"sync"
 	"syscall"
@@ -62,45 +61,62 @@ const modeACLNone fs.FileMode = 0
 // missingSyscallWarning is used to reduce log spam when a syscall is missing.
 var missingSyscallWarning sync.Once
 
-// openSecure opens the given path for writing (with O_CREAT, mode 0600)
-// with the RESOLVE_NO_SYMLINKS flag set.
-func openSecure(path string, mode OpenMode) (*os.File, error) {
+// openSecure opens the given path for either reading or writing with the
+// RESOLVE_NO_SYMLINKS flag set.
+func openSecure(path string, flags OpenFlags) (*os.File, error) {
+	var mode uint64
+	if flags != ReadFlags {
+		// openat2() with a nonzero mode will raise EINVAL unless O_CREATE or
+		// O_TMPFILE is set, so only set this in non-read mode.
+		mode = uint64(DefaultMode.Perm())
+	}
+
 	how := unix.OpenHow{
-		// Equivalent to 0600. Unfortunately it's not worth reusing our
-		// default file mode constant here.
-		Mode:    unix.O_RDONLY | unix.S_IRUSR | unix.S_IWUSR,
-		Flags:   uint64(mode),
+		Mode:    mode,
+		Flags:   uint64(flags.Flags()) | unix.O_CLOEXEC,
 		Resolve: unix.RESOLVE_NO_SYMLINKS,
 	}
 
-	fd, err := unix.Openat2(unix.AT_FDCWD, path, &how)
-	if err != nil {
-		// note: returning the original error here for comparison purposes
-		return nil, err
-	}
+	for {
+		fd, err := unix.Openat2(unix.AT_FDCWD, path, &how)
+		if errors.Is(err, syscall.EINTR) {
+			// Per the stdlib's implementation, EINTR errors should be ignored
+			// and retried.
+			continue
+		} else if err != nil {
+			// note: returning the original error here for comparison purposes
+			return nil, err
+		}
 
-	// os.File.Close() appears to close wrapped files sanely, so rely on that
-	// rather than relying on callers to use unix.Close(fd)
-	return os.NewFile(uintptr(fd), filepath.Base(path)), nil
+		file := os.NewFile(uintptr(fd), path)
+		if file == nil {
+			// Probably useless since this implies the fd itself is invalid, but
+			// attempt to close the fd anyway.
+			_ = unix.Close(fd)
+			return nil, os.ErrInvalid
+		}
+
+		return file, nil
+	}
 }
 
 // openSymlinks mode opens the file for read or write using the given symlink
 // mode, potentially failing or logging a warning if symlinks can't be
 // secured.
-func openSymlinksMode(path string, mode OpenMode, symlinksMode SymlinksMode) (*os.File, error) {
+func openSymlinksMode(path string, flags OpenFlags, symlinksMode SymlinksMode) (*os.File, error) {
 	var file *os.File
 	var err error
 
 	switch symlinksMode {
 	case SymlinksSecure:
-		file, err = openSecure(path, mode)
+		file, err = openSecure(path, flags)
 		if errors.Is(err, unix.ENOSYS) {
 			return nil, trace.Errorf("openSecure failed due to missing syscall; configure `symlinks: insecure` for %q", path)
 		} else if err != nil {
 			return nil, trace.Wrap(err)
 		}
 	case SymlinksTrySecure:
-		file, err = openSecure(path, mode)
+		file, err = openSecure(path, flags)
 		if errors.Is(err, unix.ENOSYS) {
 			missingSyscallWarning.Do(func() {
 				log.DebugContext(
@@ -110,7 +126,7 @@ func openSymlinksMode(path string, mode OpenMode, symlinksMode SymlinksMode) (*o
 				)
 			})
 
-			file, err = openStandard(path, mode)
+			file, err = openStandard(path, flags)
 			if err != nil {
 				return nil, trace.Wrap(err)
 			}
@@ -118,7 +134,7 @@ func openSymlinksMode(path string, mode OpenMode, symlinksMode SymlinksMode) (*o
 			return nil, trace.Wrap(err)
 		}
 	case SymlinksInsecure:
-		file, err = openStandard(path, mode)
+		file, err = openStandard(path, flags)
 		if err != nil {
 			return nil, trace.Wrap(err)
 		}
@@ -144,7 +160,7 @@ func createSecure(path string, isDir bool) error {
 		return nil
 	}
 
-	f, err := openSecure(path, WriteMode)
+	f, err := openSecure(path, WriteFlags)
 	if errors.Is(err, unix.ENOSYS) {
 		// bubble up the original error for comparison
 		return err
@@ -217,7 +233,7 @@ func Create(path string, isDir bool, symlinksMode SymlinksMode) error {
 
 // Read reads the contents of the given file into memory.
 func Read(path string, symlinksMode SymlinksMode) ([]byte, error) {
-	file, err := openSymlinksMode(path, ReadMode, symlinksMode)
+	file, err := openSymlinksMode(path, ReadFlags, symlinksMode)
 	if err != nil {
 		return nil, trace.Wrap(err)
 	}
@@ -234,7 +250,7 @@ func Read(path string, symlinksMode SymlinksMode) ([]byte, error) {
 
 // Write stores the given data to the file at the given path.
 func Write(path string, data []byte, symlinksMode SymlinksMode) error {
-	file, err := openSymlinksMode(path, WriteMode, symlinksMode)
+	file, err := openSymlinksMode(path, WriteFlags, symlinksMode)
 	if err != nil {
 		return trace.Wrap(err)
 	}

lib/tbot/botfs/fs_unix.go

@@ -60,7 +60,7 @@ func Read(path string, symlinksMode SymlinksMode) ([]byte, error) {
 		})
 	}
 
-	file, err := openStandard(path, ReadMode)
+	file, err := openStandard(path, ReadFlags)
 	if err != nil {
 		return nil, trace.Wrap(err)
 	}
@@ -90,7 +90,7 @@ func Write(path string, data []byte, symlinksMode SymlinksMode) error {
 		})
 	}
 
-	file, err := openStandard(path, WriteMode)
+	file, err := openStandard(path, WriteFlags)
 	if err != nil {
 		return trace.Wrap(err)
 	}

lib/tbot/botfs/fs_windows.go

@@ -61,7 +61,7 @@ func Read(path string, symlinksMode SymlinksMode) ([]byte, error) {
 		})
 	}
 
-	file, err := openStandard(path, ReadMode)
+	file, err := openStandard(path, ReadFlags)
 	if err != nil {
 		return nil, trace.Wrap(err)
 	}
@@ -91,7 +91,7 @@ func Write(path string, data []byte, symlinksMode SymlinksMode) error {
 		})
 	}
 
-	file, err := openStandard(path, WriteMode)
+	file, err := openStandard(path, WriteFlags)
 	if err != nil {
 		return trace.Wrap(err)
 	}