diff --git a/pkg/agent/plugin/keymanager/disk/disk.go b/pkg/agent/plugin/keymanager/disk/disk.go index 2dbd91f49e..8a91014ad8 100644 --- a/pkg/agent/plugin/keymanager/disk/disk.go +++ b/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) } diff --git a/pkg/agent/storage/legacy.go b/pkg/agent/storage/legacy.go index 18a7487ac4..acf1d863bd 100644 --- a/pkg/agent/storage/legacy.go +++ b/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 diff --git a/pkg/agent/storage/storage.go b/pkg/agent/storage/storage.go index e1f26886cb..4498d2e1d4 100644 --- a/pkg/agent/storage/storage.go +++ b/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) } diff --git a/pkg/common/diskutil/file_posix.go b/pkg/common/diskutil/file_posix.go index ddee077ba8..180386d3c4 100644 --- a/pkg/common/diskutil/file_posix.go +++ b/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 { diff --git a/pkg/common/diskutil/file_posix_test.go b/pkg/common/diskutil/file_posix_test.go new file mode 100644 index 0000000000..c305d54ddf --- /dev/null +++ b/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)) + }) + } +} diff --git a/pkg/common/diskutil/file_test.go b/pkg/common/diskutil/file_test.go deleted file mode 100644 index 6389af3f36..0000000000 --- a/pkg/common/diskutil/file_test.go +++ /dev/null @@ -1,68 +0,0 @@ -package diskutil - -import ( - "os" - "path/filepath" - "runtime" - "testing" - - "github.com/spiffe/spire/test/spiretest" - "github.com/stretchr/testify/require" -) - -func TestAtomicWriteFile(t *testing.T) { - dir := spiretest.TempDir(t) - - tests := []struct { - name string - data []byte - mode os.FileMode - expectPosixMode os.FileMode - expectWindowsMode os.FileMode - }{ - { - name: "basic test", - data: []byte("Hello, World"), - mode: 0600, - expectPosixMode: 0600, - expectWindowsMode: 0666, - }, - { - name: "empty", - data: []byte{}, - mode: 0440, - expectPosixMode: 0440, - expectWindowsMode: 0444, - }, - { - name: "binary", - data: []byte{0xFF, 0, 0xFF, 0x3D, 0xD8, 0xA9, 0xDC, 0xF0, 0x9F, 0x92, 0xA9}, - mode: 0644, - expectPosixMode: 0644, - expectWindowsMode: 0666, - }, - } - for _, tt := range tests { - tt := tt - t.Run(tt.name, func(t *testing.T) { - file := filepath.Join(dir, "file") - err := AtomicWriteFile(file, tt.data, tt.mode) - require.NoError(t, err) - - info, err := os.Stat(file) - require.NoError(t, err) - switch runtime.GOOS { - case "windows": - require.EqualValues(t, tt.expectWindowsMode, info.Mode()) - default: - require.EqualValues(t, tt.expectPosixMode, info.Mode()) - } - - content, err := os.ReadFile(file) - require.NoError(t, err) - require.Equal(t, tt.data, content) - - require.NoError(t, os.Remove(file)) - }) - } -} diff --git a/pkg/common/diskutil/file_windows.go b/pkg/common/diskutil/file_windows.go index d440d32719..f192daede8 100644 --- a/pkg/common/diskutil/file_windows.go +++ b/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 } diff --git a/pkg/common/diskutil/file_windows_test.go b/pkg/common/diskutil/file_windows_test.go new file mode 100644 index 0000000000..1cd62785bf --- /dev/null +++ b/pkg/common/diskutil/file_windows_test.go @@ -0,0 +1,95 @@ +//go:build windows +// +build windows + +package diskutil + +import ( + "os" + "path/filepath" + "testing" + + "github.com/spiffe/spire/pkg/common/sddl" + "github.com/spiffe/spire/test/spiretest" + "github.com/stretchr/testify/require" + "golang.org/x/sys/windows" +) + +func TestAtomicWritePrivateFile(t *testing.T) { + dir := spiretest.TempDir(t) + + tests := []struct { + name string + data []byte + expectSecurityDescriptor string + atomicWriteFunc func(string, []byte) error + }{ + { + name: "basic - AtomicWritePrivateFile", + data: []byte("Hello, World"), + expectSecurityDescriptor: sddl.PrivateFile, + atomicWriteFunc: AtomicWritePrivateFile, + }, + { + name: "empty - AtomicWritePrivateFile", + data: []byte{}, + expectSecurityDescriptor: sddl.PrivateFile, + atomicWriteFunc: AtomicWritePrivateFile, + }, + { + name: "binary - AtomicWritePrivateFile", + data: []byte{0xFF, 0, 0xFF, 0x3D, 0xD8, 0xA9, 0xDC, 0xF0, 0x9F, 0x92, 0xA9}, + expectSecurityDescriptor: sddl.PrivateFile, + atomicWriteFunc: AtomicWritePrivateFile, + }, + { + name: "basic - AtomicWritePubliclyReadableFile", + data: []byte("Hello, World"), + expectSecurityDescriptor: sddl.PubliclyReadableFile, + atomicWriteFunc: AtomicWritePubliclyReadableFile, + }, + { + name: "empty - AtomicWritePubliclyReadableFile", + data: []byte{}, + expectSecurityDescriptor: sddl.PubliclyReadableFile, + atomicWriteFunc: AtomicWritePubliclyReadableFile, + }, + { + name: "binary - AtomicWritePubliclyReadableFile", + data: []byte{0xFF, 0, 0xFF, 0x3D, 0xD8, 0xA9, 0xDC, 0xF0, 0x9F, 0x92, 0xA9}, + expectSecurityDescriptor: sddl.PubliclyReadableFile, + atomicWriteFunc: AtomicWritePubliclyReadableFile, + }, + } + 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) + + pathUTF16Ptr, err := windows.UTF16PtrFromString(file) + require.NoError(t, err) + + handle, err := windows.CreateFile(pathUTF16Ptr, + windows.GENERIC_WRITE, + 0, + nil, + windows.OPEN_EXISTING, + windows.FILE_ATTRIBUTE_NORMAL, + 0) + + require.NoError(t, err) + sd, err := windows.GetSecurityInfo(handle, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) + require.NoError(t, windows.CloseHandle(handle)) + require.NoError(t, err) + + require.Equal(t, sd.String(), tt.expectSecurityDescriptor) + + content, err := os.ReadFile(file) + require.NoError(t, err) + require.Equal(t, tt.data, content) + + require.NoError(t, os.Remove(file)) + }) + } +} diff --git a/pkg/common/sddl/sddl_windows.go b/pkg/common/sddl/sddl_windows.go index cd28de897f..5bd91390b8 100644 --- a/pkg/common/sddl/sddl_windows.go +++ b/pkg/common/sddl/sddl_windows.go @@ -11,6 +11,14 @@ const ( // to the creator owner only. PrivateFile = "D:P(A;;FA;;;OW)" + // PubliclyReadableFile describes a security descriptor using + // the security descriptor definition language (SDDL) that is meant + // to be used to define the access control to files that need to + // be publicly readable but writable only by the owner of the file. + // The security descriptor grants full access to the creator owner + // and read access to everyone. + PubliclyReadableFile = "D:P(A;;FA;;;OW)(A;;FR;;;WD)" + // PrivateListener describes a security descriptor using the // security descriptor definition language (SDDL) that is meant // to be used to define the access control to named pipes diff --git a/pkg/server/ca/journal.go b/pkg/server/ca/journal.go index de939cb111..0df23f84a9 100644 --- a/pkg/server/ca/journal.go +++ b/pkg/server/ca/journal.go @@ -149,7 +149,7 @@ func saveJournalEntries(path string, entries *JournalEntries) error { Bytes: entriesBytes, }) - if err := diskutil.AtomicWriteFile(path, pemBytes, 0644); err != nil { + if err := diskutil.AtomicWritePubliclyReadableFile(path, pemBytes); err != nil { return errs.Wrap(err) } diff --git a/pkg/server/plugin/keymanager/disk/disk.go b/pkg/server/plugin/keymanager/disk/disk.go index 78f781e55e..0dc6b01b8d 100644 --- a/pkg/server/plugin/keymanager/disk/disk.go +++ b/pkg/server/plugin/keymanager/disk/disk.go @@ -150,7 +150,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) }