Atomic writing of files on Windows with a specific security descriptor (

spiffe#3577)

* Atomic writing of files on Windows with a specific security descriptor

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

pkg/agent/plugin/keymanager/disk/disk.go

@@ -167,7 +167,7 @@ func writeEntries(path string, entries []*keymanagerbase.KeyEntry) error {
 		return status.Errorf(codes.Internal, "unable to marshal entries: %v", err)
 	}
 
-	if err := diskutil.AtomicWriteFile(path, jsonBytes, 0600); err != nil {
+	if err := diskutil.AtomicWritePrivateFile(path, jsonBytes); err != nil {
 		return status.Errorf(codes.Internal, "unable to write entries: %v", err)
 	}
 

pkg/agent/storage/legacy.go

@@ -31,7 +31,7 @@ func storeLegacyBundle(dir string, bundle []*x509.Certificate) error {
 	for _, cert := range bundle {
 		data.Write(cert.Raw)
 	}
-	if err := diskutil.AtomicWriteFile(legacyBundlePath(dir), data.Bytes(), 0600); err != nil {
+	if err := diskutil.AtomicWritePrivateFile(legacyBundlePath(dir), data.Bytes()); err != nil {
 		return fmt.Errorf("failed to store legacy bundle: %w", err)
 	}
 	return nil
@@ -55,7 +55,7 @@ func storeLegacySVID(dir string, svidChain []*x509.Certificate) error {
 	for _, cert := range svidChain {
 		data.Write(cert.Raw)
 	}
-	if err := diskutil.AtomicWriteFile(legacySVIDPath(dir), data.Bytes(), 0600); err != nil {
+	if err := diskutil.AtomicWritePrivateFile(legacySVIDPath(dir), data.Bytes()); err != nil {
 		return fmt.Errorf("failed to store legacy SVID: %w", err)
 	}
 	return nil

pkg/agent/storage/storage.go

@@ -244,7 +244,7 @@ func storeData(dir string, data storageData) error {
 		return fmt.Errorf("failed to marshal data: %w", err)
 	}
 
-	if err := diskutil.AtomicWriteFile(path, marshaled, 0600); err != nil {
+	if err := diskutil.AtomicWritePrivateFile(path, marshaled); err != nil {
 		return fmt.Errorf("failed to write data file: %w", err)
 	}
 

pkg/common/diskutil/file_posix.go

@@ -8,16 +8,36 @@ import (
 	"path/filepath"
 )
 
-// AtomicWriteFile writes data out.  It writes to a temp file first, fsyncs that file,
+// AtomicWritePrivateFile writes data out.  It writes to a temp file first, fsyncs that file,
 // then swaps the file in.  os.Rename is an atomic operation, so this sequence avoids having
 // a partially written file at the final location.  Finally, fsync is called on the directory
 // to ensure the rename is persisted.
-func AtomicWriteFile(path string, data []byte, mode os.FileMode) error {
+func AtomicWritePrivateFile(path string, data []byte) error {
+	return atomicWrite(path, data, 0600)
+}
+
+// AtomicWritePubliclyReadableFile writes data out.  It writes to a temp file first, fsyncs that file,
+// then swaps the file in.  os.Rename is an atomic operation, so this sequence avoids having
+// a partially written file at the final location.  Finally, fsync is called on the directory
+// to ensure the rename is persisted.
+func AtomicWritePubliclyReadableFile(path string, data []byte) error {
+	return atomicWrite(path, data, 0644)
+}
+
+func CreateDataDirectory(path string) error {
+	return os.MkdirAll(path, 0755)
+}
+
+func atomicWrite(path string, data []byte, mode os.FileMode) error {
 	tmpPath := path + ".tmp"
 	if err := write(tmpPath, data, mode); err != nil {
 		return err
 	}
 
+	return rename(tmpPath, path)
+}
+
+func rename(tmpPath, path string) error {
 	if err := os.Rename(tmpPath, path); err != nil {
 		return err
 	}
@@ -35,10 +55,6 @@ func AtomicWriteFile(path string, data []byte, mode os.FileMode) error {
 	return dir.Close()
 }
 
-func CreateDataDirectory(path string) error {
-	return os.MkdirAll(path, 0755)
-}
-
 func write(tmpPath string, data []byte, mode os.FileMode) error {
 	file, err := os.OpenFile(tmpPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, mode)
 	if err != nil {

pkg/common/diskutil/file_posix_test.go

@@ -0,0 +1,79 @@
+//go:build !windows
+// +build !windows
+
+package diskutil
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/spiffe/spire/test/spiretest"
+	"github.com/stretchr/testify/require"
+)
+
+func TestAtomicWritePrivateFile(t *testing.T) {
+	dir := spiretest.TempDir(t)
+
+	tests := []struct {
+		name            string
+		data            []byte
+		atomicWriteFunc func(string, []byte) error
+		expectMode      os.FileMode
+	}{
+		{
+			name:            "basic - AtomicWritePrivateFile",
+			data:            []byte("Hello, World"),
+			atomicWriteFunc: AtomicWritePrivateFile,
+			expectMode:      0600,
+		},
+		{
+			name:            "empty - AtomicWritePrivateFile",
+			data:            []byte{},
+			atomicWriteFunc: AtomicWritePrivateFile,
+			expectMode:      0600,
+		},
+		{
+			name:            "binary - AtomicWritePrivateFile",
+			data:            []byte{0xFF, 0, 0xFF, 0x3D, 0xD8, 0xA9, 0xDC, 0xF0, 0x9F, 0x92, 0xA9},
+			atomicWriteFunc: AtomicWritePrivateFile,
+			expectMode:      0600,
+		},
+		{
+			name:            "basic - AtomicWritePubliclyReadableFile",
+			data:            []byte("Hello, World"),
+			atomicWriteFunc: AtomicWritePubliclyReadableFile,
+			expectMode:      0644,
+		},
+		{
+			name:            "empty - AtomicWritePubliclyReadableFile",
+			data:            []byte{},
+			atomicWriteFunc: AtomicWritePubliclyReadableFile,
+			expectMode:      0644,
+		},
+		{
+			name:            "binary - AtomicWritePubliclyReadableFile",
+			data:            []byte{0xFF, 0, 0xFF, 0x3D, 0xD8, 0xA9, 0xDC, 0xF0, 0x9F, 0x92, 0xA9},
+			atomicWriteFunc: AtomicWritePubliclyReadableFile,
+			expectMode:      0644,
+		},
+	}
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			file := filepath.Join(dir, "file")
+			err := tt.atomicWriteFunc(file, tt.data)
+			require.NoError(t, err)
+
+			info, err := os.Stat(file)
+			require.NoError(t, err)
+			require.EqualValues(t, tt.expectMode, info.Mode())
+
+			content, err := os.ReadFile(file)
+			require.NoError(t, err)
+			require.Equal(t, tt.data, content)
+
+			require.NoError(t, os.Remove(file))
+		})
+	}
+}

pkg/common/diskutil/file_windows.go

@@ -4,6 +4,7 @@
 package diskutil
 
 import (
+	"fmt"
 	"os"
 	"syscall"
 	"unsafe"
@@ -17,16 +18,25 @@ const (
 	movefileWriteThrough    = 0x8
 )
 
-// AtomicWriteFile writes data out.  It writes to a temp file first, fsyncs that file,
-// then swaps the file in. Rename file using a custom MoveFileEx that uses 'MOVEFILE_WRITE_THROUGH' witch waits until
-// file is synced to disk.
-func AtomicWriteFile(path string, data []byte, mode os.FileMode) error {
-	tmpPath := path + ".tmp"
-	if err := write(tmpPath, data, mode); err != nil {
-		return err
-	}
+type fileAttribs struct {
+	pathUTF16Ptr *uint16
+	sa           *windows.SecurityAttributes
+}
 
-	return atomicRename(tmpPath, path)
+// AtomicWritePrivateFile writes data out to a private file.
+// It writes to a temp file first, fsyncs that file, then swaps the file in.
+// Rename file using a custom MoveFileEx that uses 'MOVEFILE_WRITE_THROUGH'
+// witch waits until file is synced to disk.
+func AtomicWritePrivateFile(path string, data []byte) error {
+	return atomicWrite(path, data, sddl.PrivateFile)
+}
+
+// AtomicWritePubliclyReadableFile writes data out to a publicly readable file.
+// It writes to a temp file first, fsyncs that file, then swaps the file in.
+// Rename file using a custom MoveFileEx that uses 'MOVEFILE_WRITE_THROUGH'
+// witch waits until file is synced to disk.
+func AtomicWritePubliclyReadableFile(path string, data []byte) error {
+	return atomicWrite(path, data, sddl.PubliclyReadableFile)
 }
 
 func CreateDataDirectory(path string) error {
@@ -78,12 +88,25 @@ func MkdirAll(path string, sddl string) error {
 	return nil
 }
 
-func write(tmpPath string, data []byte, mode os.FileMode) error {
-	file, err := os.OpenFile(tmpPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, mode)
+func atomicWrite(path string, data []byte, sddl string) error {
+	tmpPath := path + ".tmp"
+	if err := write(tmpPath, data, sddl); err != nil {
+		return err
+	}
+
+	return atomicRename(tmpPath, path)
+}
+
+func write(tmpPath string, data []byte, sddl string) error {
+	handle, err := createFileForWriting(tmpPath, sddl)
 	if err != nil {
 		return err
 	}
 
+	file := os.NewFile(uintptr(handle), tmpPath)
+	if file == nil {
+		return fmt.Errorf("invalid file descriptor for file %q", tmpPath)
+	}
 	if _, err := file.Write(data); err != nil {
 		file.Close()
 		return err
@@ -97,6 +120,25 @@ func write(tmpPath string, data []byte, mode os.FileMode) error {
 	return file.Close()
 }
 
+func createFileForWriting(path string, sddl string) (windows.Handle, error) {
+	file, err := getFileWithSecurityAttr(path, sddl)
+	if err != nil {
+		return windows.InvalidHandle, err
+	}
+	handle, err := windows.CreateFile(file.pathUTF16Ptr,
+		windows.GENERIC_WRITE,
+		0,
+		file.sa,
+		windows.CREATE_ALWAYS,
+		windows.FILE_ATTRIBUTE_NORMAL,
+		0)
+
+	if err != nil {
+		return windows.InvalidHandle, fmt.Errorf("could not create file %q: %w", path, err)
+	}
+	return handle, nil
+}
+
 func atomicRename(oldPath, newPath string) error {
 	if err := rename(oldPath, newPath); err != nil {
 		return &os.LinkError{
@@ -129,23 +171,35 @@ func rename(oldPath, newPath string) error {
 //
 // In the same way as os.MkDir, errors returned are of type *os.PathError.
 func mkdir(path string, sddl string) error {
-	sa := windows.SecurityAttributes{Length: 0}
-	sd, err := windows.SecurityDescriptorFromString(sddl)
+	file, err := getFileWithSecurityAttr(path, sddl)
+	if err != nil {
+		return err
+	}
+
+	err = windows.CreateDirectory(file.pathUTF16Ptr, file.sa)
 	if err != nil {
-		return &os.PathError{Op: "mkdir", Path: path, Err: err}
+		return fmt.Errorf("could not create directory: %w", err)
 	}
-	sa.Length = uint32(unsafe.Sizeof(sa))
-	sa.InheritHandle = 1
-	sa.SecurityDescriptor = sd
+	return nil
+}
 
-	pathUTF16, err := windows.UTF16PtrFromString(path)
+func getFileWithSecurityAttr(path, sddl string) (*fileAttribs, error) {
+	sd, err := windows.SecurityDescriptorFromString(sddl)
 	if err != nil {
-		return &os.PathError{Op: "mkdir", Path: path, Err: err}
+		return nil, fmt.Errorf("could not convert SDDL %q into a self-relative security descriptor object: %w", sddl, err)
 	}
 
-	e := windows.CreateDirectory(pathUTF16, &sa)
-	if e != nil {
-		return &os.PathError{Op: "mkdir", Path: path, Err: e}
+	pathUTF16Ptr, err := windows.UTF16PtrFromString(path)
+	if err != nil {
+		return nil, fmt.Errorf("could not get pointer to the UTF-16 encoding of path %q: %w", path, err)
 	}
-	return nil
+
+	return &fileAttribs{
+		pathUTF16Ptr: pathUTF16Ptr,
+		sa: &windows.SecurityAttributes{
+			InheritHandle:      1,
+			Length:             uint32(unsafe.Sizeof(windows.SecurityAttributes{})),
+			SecurityDescriptor: sd,
+		},
+	}, nil
 }