Fix review comments

changelog/unreleased/pull-4708

@@ -1,10 +1,9 @@
 Enhancement: Back up and restore SecurityDescriptors on Windows
 
-Restic did not back up SecurityDescriptors of files on Windows.
+Restic now backs up and restores SecurityDescriptors when backing up files and folders 
-Restic now backs up and restores SecurityDescriptors (which includes owner, group, 
+on Windows which includes owner, group, discretionary access control list (DACL), 
-discretionary access control list (DACL), system access control list (SACL)) 
+system access control list (SACL). This requires the user to be a member of backup 
-when backing up files and folders on Windows. This requires the user to be
+operators or the application must be run as admin.
-a member of backup operators or the application must be run as admin.
 If that is not the case, only the current user's owner, group and DACL will be backed up 
 and during restore only the DACL of the backed file will be restored while the current 
 user's owner and group will be set during the restore.

doc/040_backup.rst

@@ -514,9 +514,9 @@ written, and the next backup needs to write new metadata again. If you really
 want to save the access time for files and directories, you can pass the
 ``--with-atime`` option to the ``backup`` command.
 
-Backing up full security descriptors on windows is only possible when the user 
+Backing up full security descriptors on Windows is only possible when the user 
 has ``SeBackupPrivilege``privilege or is running as admin. This is a restriction 
-of windows not restic.
+of Windows not restic.
 If either of these conditions are not met, only the owner, group and DACL will 
 be backed up.
 

doc/050_restore.rst

@@ -72,9 +72,9 @@ Restoring symbolic links on windows is only possible when the user has
 ``SeCreateSymbolicLinkPrivilege`` privilege or is running as admin. This is a
 restriction of windows not restic.
 
-Restoring full security descriptors on windows is only possible when the user has
+Restoring full security descriptors on Windows is only possible when the user has
 ``SeRestorePrivilege``, ``SeSecurityPrivilege`` and ``SeTakeOwnershipPrivilege`` 
-privilege or is running as admin. This is a restriction of windows not restic.
+privilege or is running as admin. This is a restriction of Windows not restic.
 If either of these conditions are not met, only the DACL will be restored.
 
 By default, restic does not restore files as sparse. Use ``restore --sparse`` to

internal/debug/debug.go

@@ -8,8 +8,6 @@ import (
 	"path/filepath"
 	"runtime"
 	"strings"
-
-	"github.com/restic/restic/internal/fs"
 )
 
 var opts struct {
@@ -46,7 +44,7 @@ func initDebugLogger() {
 
 	fmt.Fprintf(os.Stderr, "debug log file %v\n", debugfile)
 
-	f, err := fs.OpenFile(debugfile, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0600)
+	f, err := os.OpenFile(debugfile, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0600)
 	if err != nil {
 		fmt.Fprintf(os.Stderr, "unable to open debug log file: %v\n", err)
 		os.Exit(2)

internal/fs/sd_windows.go

@@ -9,7 +9,7 @@ import (
 	"unicode/utf16"
 	"unsafe"
 
-	"github.com/restic/restic/internal/errors"
+	"github.com/restic/restic/internal/debug"
 	"golang.org/x/sys/windows"
 )
 
@@ -26,9 +26,7 @@ var (
 	// SeTakeOwnershipPrivilege allows the application to take ownership of files and directories, regardless of the permissions set on them.
 	SeTakeOwnershipPrivilege = "SeTakeOwnershipPrivilege"
 
-	backupPrivilegeError  error
+	lowerPrivileges bool
-	restorePrivilegeError error
-	lowerPrivileges       bool
 )
 
 // Flags for backup and restore with admin permissions
@@ -49,14 +47,14 @@ func GetSecurityDescriptor(filePath string) (securityDescriptor *[]byte, err err
 	var sd *windows.SECURITY_DESCRIPTOR
 
 	if lowerPrivileges {
-		sd, err = getNamedSecurityInfoLow(sd, err, filePath)
+		sd, err = getNamedSecurityInfoLow(filePath)
 	} else {
-		sd, err = getNamedSecurityInfoHigh(sd, err, filePath)
+		sd, err = getNamedSecurityInfoHigh(filePath)
 	}
 	if err != nil {
 		if isHandlePrivilegeNotHeldError(err) {
 			lowerPrivileges = true
-			sd, err = getNamedSecurityInfoLow(sd, err, filePath)
+			sd, err = getNamedSecurityInfoLow(filePath)
 			if err != nil {
 				return nil, fmt.Errorf("get low-level named security info failed with: %w", err)
 			}
@@ -128,12 +126,12 @@ func SetSecurityDescriptor(filePath string, securityDescriptor *[]byte) error {
 }
 
 // getNamedSecurityInfoHigh gets the higher level SecurityDescriptor which requires admin permissions.
-func getNamedSecurityInfoHigh(sd *windows.SECURITY_DESCRIPTOR, err error, filePath string) (*windows.SECURITY_DESCRIPTOR, error) {
+func getNamedSecurityInfoHigh(filePath string) (*windows.SECURITY_DESCRIPTOR, error) {
 	return windows.GetNamedSecurityInfo(filePath, windows.SE_FILE_OBJECT, highSecurityFlags)
 }
 
 // getNamedSecurityInfoLow gets the lower level SecurityDescriptor which requires no admin permissions.
-func getNamedSecurityInfoLow(sd *windows.SECURITY_DESCRIPTOR, err error, filePath string) (*windows.SECURITY_DESCRIPTOR, error) {
+func getNamedSecurityInfoLow(filePath string) (*windows.SECURITY_DESCRIPTOR, error) {
 	return windows.GetNamedSecurityInfo(filePath, windows.SE_FILE_OBJECT, lowBackupSecurityFlags)
 }
 
@@ -151,7 +149,7 @@ func setNamedSecurityInfoLow(filePath string, dacl *windows.ACL) error {
 func enableBackupPrivilege() {
 	err := enableProcessPrivileges([]string{SeBackupPrivilege})
 	if err != nil {
-		backupPrivilegeError = fmt.Errorf("error enabling backup privilege: %w", err)
+		debug.Log("error enabling backup privilege: %v", err)
 	}
 }
 
@@ -159,26 +157,10 @@ func enableBackupPrivilege() {
 func enableRestorePrivilege() {
 	err := enableProcessPrivileges([]string{SeRestorePrivilege, SeSecurityPrivilege, SeTakeOwnershipPrivilege})
 	if err != nil {
-		restorePrivilegeError = fmt.Errorf("error enabling restore/security privilege: %w", err)
+		debug.Log("error enabling restore/security privilege: %v", err)
 	}
 }
 
-// DisableBackupPrivileges disables privileges that are needed for backup operations.
-// They may be reenabled if GetSecurityDescriptor is called again.
-func DisableBackupPrivileges() error {
-	//Reset the once so that backup privileges can be enabled again if needed.
-	onceBackup = sync.Once{}
-	return enableDisableProcessPrivilege([]string{SeBackupPrivilege}, 0)
-}
-
-// DisableRestorePrivileges disables privileges that are needed for restore operations.
-// They may be reenabled if SetSecurityDescriptor is called again.
-func DisableRestorePrivileges() error {
-	//Reset the once so that restore privileges can be enabled again if needed.
-	onceRestore = sync.Once{}
-	return enableDisableProcessPrivilege([]string{SeRestorePrivilege, SeSecurityPrivilege}, 0)
-}
-
 // isHandlePrivilegeNotHeldError checks if the error is ERROR_PRIVILEGE_NOT_HELD
 func isHandlePrivilegeNotHeldError(err error) bool {
 	// Use a type assertion to check if the error is of type syscall.Errno
@@ -189,23 +171,26 @@ func isHandlePrivilegeNotHeldError(err error) bool {
 	return false
 }
 
-// IsAdmin checks if current user is an administrator.
+// SecurityDescriptorBytesToStruct converts the security descriptor bytes representation
-func IsAdmin() (isAdmin bool, err error) {
+// into a pointer to windows SECURITY_DESCRIPTOR.
-	var sid *windows.SID
+func SecurityDescriptorBytesToStruct(sd []byte) (*windows.SECURITY_DESCRIPTOR, error) {
-	err = windows.AllocateAndInitializeSid(&windows.SECURITY_NT_AUTHORITY, 2, windows.SECURITY_BUILTIN_DOMAIN_RID, windows.DOMAIN_ALIAS_RID_ADMINS,
+	if l := int(unsafe.Sizeof(windows.SECURITY_DESCRIPTOR{})); len(sd) < l {
-		0, 0, 0, 0, 0, 0, &sid)
+		return nil, fmt.Errorf("securityDescriptor (%d) smaller than expected (%d): %w", len(sd), l, windows.ERROR_INCORRECT_SIZE)
-	if err != nil {
-		return false, errors.Errorf("sid error: %s", err)
 	}
-	token := windows.Token(0)
+	s := (*windows.SECURITY_DESCRIPTOR)(unsafe.Pointer(&sd[0]))
-	member, err := token.IsMember(sid)
+	return s, nil
-	if err != nil {
-		return false, errors.Errorf("token membership error: %s", err)
-	}
-	return member, nil
 }
 
-// The code below was adapted from github.com/Microsoft/go-winio under MIT license.
+// securityDescriptorStructToBytes converts the pointer to windows SECURITY_DESCRIPTOR
+// into a security descriptor bytes representation.
+func securityDescriptorStructToBytes(sd *windows.SECURITY_DESCRIPTOR) ([]byte, error) {
+	b := unsafe.Slice((*byte)(unsafe.Pointer(sd)), sd.Length())
+	return b, nil
+}
+
+// The code below was adapted from
+// https://github.com/microsoft/go-winio/blob/3c9576c9346a1892dee136329e7e15309e82fb4f/privilege.go
+// under MIT license.
 
 // The MIT License (MIT)
 
@@ -262,23 +247,6 @@ type PrivilegeError struct {
 	privileges []uint64
 }
 
-// SecurityDescriptorBytesToStruct converts the security descriptor bytes representation
-// into a pointer to windows SECURITY_DESCRIPTOR.
-func SecurityDescriptorBytesToStruct(sd []byte) (*windows.SECURITY_DESCRIPTOR, error) {
-	if l := int(unsafe.Sizeof(windows.SECURITY_DESCRIPTOR{})); len(sd) < l {
-		return nil, fmt.Errorf("securityDescriptor (%d) smaller than expected (%d): %w", len(sd), l, windows.ERROR_INCORRECT_SIZE)
-	}
-	s := (*windows.SECURITY_DESCRIPTOR)(unsafe.Pointer(&sd[0]))
-	return s, nil
-}
-
-// securityDescriptorStructToBytes converts the pointer to windows SECURITY_DESCRIPTOR
-// into a security descriptor bytes representation.
-func securityDescriptorStructToBytes(sd *windows.SECURITY_DESCRIPTOR) ([]byte, error) {
-	b := unsafe.Slice((*byte)(unsafe.Pointer(sd)), sd.Length())
-	return b, nil
-}
-
 // Error returns the string message for the error.
 func (e *PrivilegeError) Error() string {
 	s := "Could not enable privilege "
@@ -293,12 +261,6 @@ func (e *PrivilegeError) Error() string {
 		s += getPrivilegeName(p)
 		s += `"`
 	}
-	if backupPrivilegeError != nil {
-		s += " backupPrivilegeError:" + backupPrivilegeError.Error()
-	}
-	if restorePrivilegeError != nil {
-		s += " restorePrivilegeError:" + restorePrivilegeError.Error()
-	}
 	return s
 }
 

internal/fs/sd_windows_test.go

@@ -13,7 +13,7 @@ import (
 	"github.com/restic/restic/internal/test"
 )
 
-func Test_SetGetFileSecurityDescriptors(t *testing.T) {
+func TestSetGetFileSecurityDescriptors(t *testing.T) {
 	tempDir := t.TempDir()
 	testfilePath := filepath.Join(tempDir, "testfile.txt")
 	// create temp file
@@ -31,7 +31,7 @@ func Test_SetGetFileSecurityDescriptors(t *testing.T) {
 	testSecurityDescriptors(t, TestFileSDs, testfilePath)
 }
 
-func Test_SetGetFolderSecurityDescriptors(t *testing.T) {
+func TestSetGetFolderSecurityDescriptors(t *testing.T) {
 	tempDir := t.TempDir()
 	testfolderPath := filepath.Join(tempDir, "testfolder")
 	// create temp folder

internal/fs/sd_windows_test_helpers.go

@@ -23,6 +23,23 @@ var (
 	}
 )
 
+// IsAdmin checks if current user is an administrator.
+func IsAdmin() (isAdmin bool, err error) {
+	var sid *windows.SID
+	err = windows.AllocateAndInitializeSid(&windows.SECURITY_NT_AUTHORITY, 2, windows.SECURITY_BUILTIN_DOMAIN_RID, windows.DOMAIN_ALIAS_RID_ADMINS,
+		0, 0, 0, 0, 0, 0, &sid)
+	if err != nil {
+		return false, errors.Errorf("sid error: %s", err)
+	}
+	windows.GetCurrentProcessToken()
+	token := windows.Token(0)
+	member, err := token.IsMember(sid)
+	if err != nil {
+		return false, errors.Errorf("token membership error: %s", err)
+	}
+	return member, nil
+}
+
 // CompareSecurityDescriptors runs tests for comparing 2 security descriptors in []byte format.
 func CompareSecurityDescriptors(t *testing.T, testPath string, sdInputBytes, sdOutputBytes []byte) {
 	sdInput, err := SecurityDescriptorBytesToStruct(sdInputBytes)

internal/restic/node.go

@@ -721,7 +721,7 @@ func (node *Node) fillExtra(path string, fi os.FileInfo, ignoreXattrListError bo
 	allowExtended, err := node.fillGenericAttributes(path, fi, stat)
 	if allowExtended {
 		// Skip processing ExtendedAttributes if allowExtended is false.
-		err = errors.CombineErrors(err, node.fillExtendedAttributes(path))
+		err = errors.CombineErrors(err, node.fillExtendedAttributes(path, ignoreXattrListError))
 	}
 	return err
 }

internal/restic/node_windows.go

@@ -24,7 +24,7 @@ type WindowsAttributes struct {
 	// FileAttributes is used for storing file attributes for windows files.
 	FileAttributes *uint32 `generic:"file_attributes"`
 	// SecurityDescriptor is used for storing security descriptors which includes
-	// owner, group, discretionary access control list (DACL), system access control list (SACL))
+	// owner, group, discretionary access control list (DACL), system access control list (SACL)
 	SecurityDescriptor *[]byte `generic:"security_descriptor"`
 }