atomicfile: use ReplaceFile on Windows so that attributes and ACLs are preserved

I moved the actual rename into separate, GOOS-specific files. On
non-Windows, we do a simple os.Rename. On Windows, we first try
ReplaceFile with a fallback to os.Rename if the target file does
not exist.

ReplaceFile is the recommended way to rename the file in this use case,
as it preserves attributes and ACLs set on the target file.

Updates #14428

Signed-off-by: Aaron Klotz <aaron@tailscale.com>

atomicfile/atomicfile.go

@@ -15,8 +15,9 @@ import (
 )
 
 // WriteFile writes data to filename+some suffix, then renames it into filename.
-// The perm argument is ignored on Windows. If the target filename already
-// exists but is not a regular file, WriteFile returns an error.
+// The perm argument is ignored on Windows, but if the target filename already
+// exists then the target file's attributes and ACLs are preserved. If the target
+// filename already exists but is not a regular file, WriteFile returns an error.
 func WriteFile(filename string, data []byte, perm os.FileMode) (err error) {
 	fi, err := os.Stat(filename)
 	if err == nil && !fi.Mode().IsRegular() {
@@ -47,5 +48,5 @@ func WriteFile(filename string, data []byte, perm os.FileMode) (err error) {
 	if err := f.Close(); err != nil {
 		return err
 	}
-	return os.Rename(tmpName, filename)
+	return rename(tmpName, filename)
 }

atomicfile/atomicfile_notwindows.go

@@ -0,0 +1,14 @@
+// Copyright (c) Tailscale Inc & AUTHORS
+// SPDX-License-Identifier: BSD-3-Clause
+
+//go:build !windows
+
+package atomicfile
+
+import (
+	"os"
+)
+
+func rename(srcFile, destFile string) error {
+	return os.Rename(srcFile, destFile)
+}

atomicfile/atomicfile_windows.go

@@ -0,0 +1,33 @@
+// Copyright (c) Tailscale Inc & AUTHORS
+// SPDX-License-Identifier: BSD-3-Clause
+
+package atomicfile
+
+import (
+	"os"
+
+	"golang.org/x/sys/windows"
+)
+
+func rename(srcFile, destFile string) error {
+	// Use replaceFile when possible to preserve the original file's attributes and ACLs.
+	if err := replaceFile(destFile, srcFile); err == nil || err != windows.ERROR_FILE_NOT_FOUND {
+		return err
+	}
+	// destFile doesn't exist. Just do a normal rename.
+	return os.Rename(srcFile, destFile)
+}
+
+func replaceFile(destFile, srcFile string) error {
+	destFile16, err := windows.UTF16PtrFromString(destFile)
+	if err != nil {
+		return err
+	}
+
+	srcFile16, err := windows.UTF16PtrFromString(srcFile)
+	if err != nil {
+		return err
+	}
+
+	return replaceFileW(destFile16, srcFile16, nil, 0, nil, nil)
+}

atomicfile/atomicfile_windows_test.go

@@ -0,0 +1,146 @@
+// Copyright (c) Tailscale Inc & AUTHORS
+// SPDX-License-Identifier: BSD-3-Clause
+
+package atomicfile
+
+import (
+	"os"
+	"testing"
+	"unsafe"
+
+	"golang.org/x/sys/windows"
+)
+
+var _SECURITY_RESOURCE_MANAGER_AUTHORITY = windows.SidIdentifierAuthority{[6]byte{0, 0, 0, 0, 0, 9}}
+
+// makeRandomSID generates a SID derived from a v4 GUID.
+// This is basically the same algorithm used by browser sandboxes for generating
+// random SIDs.
+func makeRandomSID() (*windows.SID, error) {
+	guid, err := windows.GenerateGUID()
+	if err != nil {
+		return nil, err
+	}
+
+	rids := *((*[4]uint32)(unsafe.Pointer(&guid)))
+
+	var pSID *windows.SID
+	if err := windows.AllocateAndInitializeSid(&_SECURITY_RESOURCE_MANAGER_AUTHORITY, 4, rids[0], rids[1], rids[2], rids[3], 0, 0, 0, 0, &pSID); err != nil {
+		return nil, err
+	}
+	defer windows.FreeSid(pSID)
+
+	// Make a copy that lives on the Go heap
+	return pSID.Copy()
+}
+
+func getExistingFileSD(name string) (*windows.SECURITY_DESCRIPTOR, error) {
+	const infoFlags = windows.DACL_SECURITY_INFORMATION
+	return windows.GetNamedSecurityInfo(name, windows.SE_FILE_OBJECT, infoFlags)
+}
+
+func getExistingFileDACL(name string) (*windows.ACL, error) {
+	sd, err := getExistingFileSD(name)
+	if err != nil {
+		return nil, err
+	}
+
+	dacl, _, err := sd.DACL()
+	return dacl, err
+}
+
+func addDenyACEForRandomSID(dacl *windows.ACL) (*windows.ACL, error) {
+	randomSID, err := makeRandomSID()
+	if err != nil {
+		return nil, err
+	}
+
+	randomSIDTrustee := windows.TRUSTEE{nil, windows.NO_MULTIPLE_TRUSTEE,
+		windows.TRUSTEE_IS_SID, windows.TRUSTEE_IS_UNKNOWN,
+		windows.TrusteeValueFromSID(randomSID)}
+
+	entries := []windows.EXPLICIT_ACCESS{
+		{
+			windows.GENERIC_ALL,
+			windows.DENY_ACCESS,
+			windows.NO_INHERITANCE,
+			randomSIDTrustee,
+		},
+	}
+
+	return windows.ACLFromEntries(entries, dacl)
+}
+
+func setExistingFileDACL(name string, dacl *windows.ACL) error {
+	return windows.SetNamedSecurityInfo(name, windows.SE_FILE_OBJECT,
+		windows.DACL_SECURITY_INFORMATION, nil, nil, dacl, nil)
+}
+
+// makeOrigFileWithCustomDACL creates a new, temporary file with a custom
+// DACL that we can check for later. It returns the name of the temporary
+// file and the security descriptor for the file in SDDL format.
+func makeOrigFileWithCustomDACL() (name, sddl string, err error) {
+	f, err := os.CreateTemp("", "foo*.tmp")
+	if err != nil {
+		return "", "", err
+	}
+	name = f.Name()
+	if err := f.Close(); err != nil {
+		return "", "", err
+	}
+	f = nil
+	defer func() {
+		if err != nil {
+			os.Remove(name)
+		}
+	}()
+
+	dacl, err := getExistingFileDACL(name)
+	if err != nil {
+		return "", "", err
+	}
+
+	// Add a harmless, deny-only ACE for a random SID that isn't used for anything
+	// (but that we can check for later).
+	dacl, err = addDenyACEForRandomSID(dacl)
+	if err != nil {
+		return "", "", err
+	}
+
+	if err := setExistingFileDACL(name, dacl); err != nil {
+		return "", "", err
+	}
+
+	sd, err := getExistingFileSD(name)
+	if err != nil {
+		return "", "", err
+	}
+
+	return name, sd.String(), nil
+}
+
+func TestPreserveSecurityInfo(t *testing.T) {
+	// Make a test file with a custom ACL.
+	origFileName, want, err := makeOrigFileWithCustomDACL()
+	if err != nil {
+		t.Fatalf("makeOrigFileWithCustomDACL returned %v", err)
+	}
+	t.Cleanup(func() {
+		os.Remove(origFileName)
+	})
+
+	if err := WriteFile(origFileName, []byte{}, 0); err != nil {
+		t.Fatalf("WriteFile returned %v", err)
+	}
+
+	// We expect origFileName's security descriptor to be unchanged despite
+	// the WriteFile call.
+	sd, err := getExistingFileSD(origFileName)
+	if err != nil {
+		t.Fatalf("getExistingFileSD(%q) returned %v", origFileName, err)
+	}
+
+	if got := sd.String(); got != want {
+		t.Errorf("security descriptor comparison failed: got %q, want %q", got, want)
+	}
+}

atomicfile/mksyscall.go

@@ -0,0 +1,8 @@
+// Copyright (c) Tailscale Inc & AUTHORS
+// SPDX-License-Identifier: BSD-3-Clause
+
+package atomicfile
+
+//go:generate go run golang.org/x/sys/windows/mkwinsyscall -output zsyscall_windows.go mksyscall.go
+
+//sys replaceFileW(replaced *uint16, replacement *uint16, backup *uint16, flags uint32, exclude unsafe.Pointer, reserved unsafe.Pointer) (err error) [int32(failretval)==0] = kernel32.ReplaceFileW

atomicfile/zsyscall_windows.go

@@ -0,0 +1,52 @@
+// Code generated by 'go generate'; DO NOT EDIT.
+
+package atomicfile
+
+import (
+	"syscall"
+	"unsafe"
+
+	"golang.org/x/sys/windows"
+)
+
+var _ unsafe.Pointer
+
+// Do the interface allocations only once for common
+// Errno values.
+const (
+	errnoERROR_IO_PENDING = 997
+)
+
+var (
+	errERROR_IO_PENDING error = syscall.Errno(errnoERROR_IO_PENDING)
+	errERROR_EINVAL     error = syscall.EINVAL
+)
+
+// errnoErr returns common boxed Errno values, to prevent
+// allocations at runtime.
+func errnoErr(e syscall.Errno) error {
+	switch e {
+	case 0:
+		return errERROR_EINVAL
+	case errnoERROR_IO_PENDING:
+		return errERROR_IO_PENDING
+	}
+	// TODO: add more here, after collecting data on the common
+	// error values see on Windows. (perhaps when running
+	// all.bat?)
+	return e
+}
+
+var (
+	modkernel32 = windows.NewLazySystemDLL("kernel32.dll")
+
+	procReplaceFileW = modkernel32.NewProc("ReplaceFileW")
+)
+
+func replaceFileW(replaced *uint16, replacement *uint16, backup *uint16, flags uint32, exclude unsafe.Pointer, reserved unsafe.Pointer) (err error) {
+	r1, _, e1 := syscall.Syscall6(procReplaceFileW.Addr(), 6, uintptr(unsafe.Pointer(replaced)), uintptr(unsafe.Pointer(replacement)), uintptr(unsafe.Pointer(backup)), uintptr(flags), uintptr(exclude), uintptr(reserved))
+	if int32(r1) == 0 {
+		err = errnoErr(e1)
+	}
+	return
+}

cmd/derper/depaware.txt

@@ -85,7 +85,7 @@ tailscale.com/cmd/derper dependencies: (generated by github.com/tailscale/depawa
         google.golang.org/protobuf/runtime/protoimpl                 from github.com/prometheus/client_model/go+
         google.golang.org/protobuf/types/known/timestamppb           from github.com/prometheus/client_golang/prometheus+
         tailscale.com                                                from tailscale.com/version
-        tailscale.com/atomicfile                                     from tailscale.com/cmd/derper+
+     💣 tailscale.com/atomicfile                                     from tailscale.com/cmd/derper+
         tailscale.com/client/tailscale                               from tailscale.com/derp
         tailscale.com/client/tailscale/apitype                       from tailscale.com/client/tailscale
         tailscale.com/derp                                           from tailscale.com/cmd/derper+

cmd/k8s-operator/depaware.txt

@@ -643,7 +643,7 @@ tailscale.com/cmd/k8s-operator dependencies: (generated by github.com/tailscale/
         sigs.k8s.io/yaml/goyaml.v2                                   from sigs.k8s.io/yaml
         tailscale.com                                                from tailscale.com/version
         tailscale.com/appc                                           from tailscale.com/ipn/ipnlocal
-        tailscale.com/atomicfile                                     from tailscale.com/ipn+
+     💣 tailscale.com/atomicfile                                     from tailscale.com/ipn+
         tailscale.com/client/tailscale                               from tailscale.com/client/web+
         tailscale.com/client/tailscale/apitype                       from tailscale.com/client/tailscale+
         tailscale.com/client/web                                     from tailscale.com/ipn/ipnlocal

cmd/tailscale/depaware.txt

@@ -69,7 +69,7 @@ tailscale.com/cmd/tailscale dependencies: (generated by github.com/tailscale/dep
         software.sslmate.com/src/go-pkcs12                           from tailscale.com/cmd/tailscale/cli
         software.sslmate.com/src/go-pkcs12/internal/rc2              from software.sslmate.com/src/go-pkcs12
         tailscale.com                                                from tailscale.com/version
-        tailscale.com/atomicfile                                     from tailscale.com/cmd/tailscale/cli+
+     💣 tailscale.com/atomicfile                                     from tailscale.com/cmd/tailscale/cli+
         tailscale.com/client/tailscale                               from tailscale.com/client/web+
         tailscale.com/client/tailscale/apitype                       from tailscale.com/client/tailscale+
         tailscale.com/client/web                                     from tailscale.com/cmd/tailscale/cli

cmd/tailscaled/depaware.txt

@@ -232,7 +232,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de
         gvisor.dev/gvisor/pkg/waiter                                 from gvisor.dev/gvisor/pkg/context+
         tailscale.com                                                from tailscale.com/version
         tailscale.com/appc                                           from tailscale.com/ipn/ipnlocal
-        tailscale.com/atomicfile                                     from tailscale.com/ipn+
+     💣 tailscale.com/atomicfile                                     from tailscale.com/ipn+
   LD    tailscale.com/chirp                                          from tailscale.com/cmd/tailscaled
         tailscale.com/client/tailscale                               from tailscale.com/client/web+
         tailscale.com/client/tailscale/apitype                       from tailscale.com/client/tailscale+