Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to use upstream sddl/SecurityAttribute but retain old exported functions #172

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions backuptar/tar.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"strings"
"syscall"
"time"
"unsafe"

"github.com/Microsoft/go-winio"
"golang.org/x/sys/windows"
Expand Down Expand Up @@ -330,32 +331,34 @@ func FileInfoFromHeader(hdr *tar.Header) (name string, size int64, fileInfo *win
// tar file that was not processed, or io.EOF is there are no more.
func WriteBackupStreamFromTarFile(w io.Writer, t *tar.Reader, hdr *tar.Header) (*tar.Header, error) {
bw := winio.NewBackupStreamWriter(w)
var sd []byte
sd := &windows.SECURITY_DESCRIPTOR{}
var err error
// Maintaining old SDDL-based behavior for backward compatibility. All new tar headers written
// by this library will have raw binary for the security descriptor.
if sddl, ok := hdr.PAXRecords[hdrSecurityDescriptor]; ok {
sd, err = winio.SddlToSecurityDescriptor(sddl)
sd, err = windows.SecurityDescriptorFromString(sddl)
if err != nil {
return nil, err
}
}
if sdraw, ok := hdr.PAXRecords[hdrRawSecurityDescriptor]; ok {
sd, err = base64.StdEncoding.DecodeString(sdraw)
sdAsByteSlice, err := base64.StdEncoding.DecodeString(sdraw)
if err != nil {
return nil, err
}
sd = (*windows.SECURITY_DESCRIPTOR)(unsafe.Pointer(&sdAsByteSlice[0]))
}
if len(sd) != 0 {
sdLen := sd.Length()
if sdLen != 0 {
bhdr := winio.BackupHeader{
Id: winio.BackupSecurity,
Size: int64(len(sd)),
Size: int64(sdLen),
}
err := bw.WriteHeader(&bhdr)
if err != nil {
return nil, err
}
_, err = bw.Write(sd)
_, err = bw.Write((*[(1 << 31) - 1]byte)(unsafe.Pointer(sd))[:sdLen])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This idiom fails -gcflags=all=-d=checkptr as of Go 1.14. In this case, it's just being moved from elsewhere, so it's not an objection to this change, but a reminder that we need to do a pass over the code-base with checkptr (or race enabled, which includes it) with Go 1.15 or later, and fix occurrences of this.

The fix itself is pretty simple, I did the same pass for hcsshim in microsoft/hcsshim#926, see Uint16BufferToSlice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This idiom fails -gcflags=all=-d=checkptr as of Go 1.14. In this case

Oh! It just occurred to me now that there's no CI running in this repository (other than the license/cla check); should we add a basic github-action?

if err != nil {
return nil, err
}
Expand Down
79 changes: 78 additions & 1 deletion backuptar/tar_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package backuptar
import (
"archive/tar"
"bytes"
"io"
"io/ioutil"
"os"
"path/filepath"
Expand All @@ -22,7 +23,7 @@ func ensurePresent(t *testing.T, m map[string]string, keys ...string) {
}
}

func TestRoundTrip(t *testing.T) {
func TestTarFileFromBackupStream(t *testing.T) {
f, err := ioutil.TempFile("", "tst")
if err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -84,3 +85,79 @@ func TestRoundTrip(t *testing.T) {

ensurePresent(t, hdr.PAXRecords, "MSWINDOWS.fileattr", "MSWINDOWS.rawsd")
}

func TestBackupStreamFromTar(t *testing.T) {
f, err := ioutil.TempFile("", "tst")
if err != nil {
t.Fatal(err)
}
defer f.Close()
defer os.Remove(f.Name())

expectedContent := "testing 1 2 3\n"
if _, err = f.Write([]byte(expectedContent)); err != nil {
t.Fatal(err)
}

if _, err = f.Seek(0, 0); err != nil {
t.Fatal(err)
}

fi, err := f.Stat()
if err != nil {
t.Fatal(err)
}

bi, err := winio.GetFileBasicInfo(f)
if err != nil {
t.Fatal(err)
}

br := winio.NewBackupFileReader(f, true)
defer br.Close()

var buf bytes.Buffer
tw := tar.NewWriter(&buf)
err = WriteTarFileFromBackupStream(tw, br, f.Name(), fi.Size(), bi)
if err != nil {
t.Fatal(err)
}

tarContentReader := tar.NewReader(&buf)
hdr2, err := tarContentReader.Next()
if err != nil {
t.Fatal(err)
}
var backupStreamBuf bytes.Buffer
_, err = WriteBackupStreamFromTarFile(&backupStreamBuf, tarContentReader, hdr2)
if err != nil && err != io.EOF {
t.Fatal(err)
}

bsr := winio.NewBackupStreamReader(&backupStreamBuf)

// read the first header that has security descriptor
_, err = bsr.Next()
if err != nil && err != io.EOF {
t.Fatal(err)
}

// read header for contents
bhdr, err := bsr.Next()
if err != nil && err != io.EOF {
t.Fatal(err)
}

resultBuf := make([]byte, int(bhdr.Size))
written, err := bsr.Read(resultBuf)
if err != nil && err != io.EOF {
t.Fatal(err)
}
if int64(written) != bhdr.Size {
t.Fatal("unexpected size of read bytes for backup stream")
}

if expectedContent != string(resultBuf) {
t.Fatalf("expected to read \"%v\" instead got \"%v\"", expectedContent, string(resultBuf))
}
}
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,5 @@ go 1.12
require (
github.com/pkg/errors v0.9.1
github.com/sirupsen/logrus v1.7.0
golang.org/x/sys v0.0.0-20210124154548-22da62e12c0c
golang.org/x/sys v0.0.0-20210309074719-68d13333faf2
)
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,5 @@ github.com/stretchr/testify v1.2.2 h1:bSDNvY7ZPG5RlJ8otE/7V6gMiyenm9RtJ7IUVIAoJ1
github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
golang.org/x/sys v0.0.0-20191026070338-33540a1f6037 h1:YyJpGZS1sBuBCzLAR1VEpK193GlqGZbnPFnPV/5Rsb4=
golang.org/x/sys v0.0.0-20191026070338-33540a1f6037/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20210124154548-22da62e12c0c h1:VwygUrnw9jn88c4u8GD3rZQbqrP/tgas88tPUbBxQrk=
golang.org/x/sys v0.0.0-20210124154548-22da62e12c0c/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20210309074719-68d13333faf2 h1:46ULzRKLh1CwgRq2dC5SlBzEqqNCi8rreOZnNrbqcIY=
golang.org/x/sys v0.0.0-20210309074719-68d13333faf2/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
46 changes: 16 additions & 30 deletions pipe.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import (
"syscall"
"time"
"unsafe"

"golang.org/x/sys/windows"
)

//sys connectNamedPipe(pipe syscall.Handle, o *syscall.Overlapped) (err error) = ConnectNamedPipe
Expand All @@ -35,7 +37,7 @@ type objectAttributes struct {
RootDirectory uintptr
ObjectName *unicodeString
Attributes uintptr
SecurityDescriptor *securityDescriptor
SecurityDescriptor *windows.SECURITY_DESCRIPTOR
SecurityQoS uintptr
}

Expand All @@ -45,16 +47,6 @@ type unicodeString struct {
Buffer uintptr
}

type securityDescriptor struct {
Revision byte
Sbz1 byte
Control uint16
Owner uintptr
Group uintptr
Sacl uintptr
Dacl uintptr
}

type ntstatus int32

func (status ntstatus) Err() error {
Expand Down Expand Up @@ -82,8 +74,6 @@ const (

cFILE_PIPE_MESSAGE_TYPE = 1
cFILE_PIPE_REJECT_REMOTE_CLIENTS = 2

cSE_DACL_PRESENT = 4
)

var (
Expand Down Expand Up @@ -273,7 +263,7 @@ type win32PipeListener struct {
doneCh chan int
}

func makeServerPipeHandle(path string, sd []byte, c *PipeConfig, first bool) (syscall.Handle, error) {
func makeServerPipeHandle(path string, sd *windows.SECURITY_DESCRIPTOR, c *PipeConfig, first bool) (syscall.Handle, error) {
path16, err := syscall.UTF16FromString(path)
if err != nil {
return 0, &os.PathError{Op: "open", Path: path, Err: err}
Expand All @@ -286,29 +276,25 @@ func makeServerPipeHandle(path string, sd []byte, c *PipeConfig, first bool) (sy
if err := rtlDosPathNameToNtPathName(&path16[0], &ntPath, 0, 0).Err(); err != nil {
return 0, &os.PathError{Op: "open", Path: path, Err: err}
}
defer localFree(ntPath.Buffer)
defer windows.LocalFree(windows.Handle(ntPath.Buffer))
oa.ObjectName = &ntPath

// The security descriptor is only needed for the first pipe.
if first {
if sd != nil {
len := uint32(len(sd))
sdb := localAlloc(0, len)
defer localFree(sdb)
copy((*[0xffff]byte)(unsafe.Pointer(sdb))[:], sd)
oa.SecurityDescriptor = (*securityDescriptor)(unsafe.Pointer(sdb))
oa.SecurityDescriptor = sd
} else {
// Construct the default named pipe security descriptor.
var dacl uintptr
if err := rtlDefaultNpAcl(&dacl).Err(); err != nil {
dacl := &windows.ACL{}
if err := windows.RtlDefaultNpAcl(&dacl); err != nil {
return 0, fmt.Errorf("getting default named pipe ACL: %s", err)
}
defer localFree(dacl)

sdb := &securityDescriptor{
Revision: 1,
Control: cSE_DACL_PRESENT,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to set windows.SE_DACL_PRESENT anymore?

Dacl: dacl,
sdb, err := windows.NewSecurityDescriptor()
if err != nil {
return 0, err
}
if err := sdb.SetDACL(dacl, true, true); err != nil {
return 0, err
}
oa.SecurityDescriptor = sdb
}
Expand Down Expand Up @@ -440,14 +426,14 @@ type PipeConfig struct {
// The pipe must not already exist.
func ListenPipe(path string, c *PipeConfig) (net.Listener, error) {
var (
sd []byte
sd *windows.SECURITY_DESCRIPTOR
err error
)
if c == nil {
c = &PipeConfig{}
}
if c.SecurityDescriptor != "" {
sd, err = SddlToSecurityDescriptor(c.SecurityDescriptor)
sd, err = windows.SecurityDescriptorFromString(c.SecurityDescriptor)
if err != nil {
return nil, err
}
Expand Down
4 changes: 2 additions & 2 deletions vhd/vhd.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@ import (

//go:generate go run mksyscall_windows.go -output zvhd_windows.go vhd.go

//sys createVirtualDisk(virtualStorageType *VirtualStorageType, path string, virtualDiskAccessMask uint32, securityDescriptor *uintptr, createVirtualDiskFlags uint32, providerSpecificFlags uint32, parameters *CreateVirtualDiskParameters, overlapped *syscall.Overlapped, handle *syscall.Handle) (err error) [failretval != 0] = virtdisk.CreateVirtualDisk
//sys createVirtualDisk(virtualStorageType *VirtualStorageType, path string, virtualDiskAccessMask uint32, securityDescriptor *windows.SECURITY_DESCRIPTOR, createVirtualDiskFlags uint32, providerSpecificFlags uint32, parameters *CreateVirtualDiskParameters, overlapped *syscall.Overlapped, handle *syscall.Handle) (err error) [failretval != 0] = virtdisk.CreateVirtualDisk
//sys openVirtualDisk(virtualStorageType *VirtualStorageType, path string, virtualDiskAccessMask uint32, openVirtualDiskFlags uint32, parameters *OpenVirtualDiskParameters, handle *syscall.Handle) (err error) [failretval != 0] = virtdisk.OpenVirtualDisk
//sys attachVirtualDisk(handle syscall.Handle, securityDescriptor *uintptr, attachVirtualDiskFlag uint32, providerSpecificFlags uint32, parameters *AttachVirtualDiskParameters, overlapped *syscall.Overlapped) (err error) [failretval != 0] = virtdisk.AttachVirtualDisk
//sys attachVirtualDisk(handle syscall.Handle, securityDescriptor *windows.SECURITY_DESCRIPTOR, attachVirtualDiskFlag uint32, providerSpecificFlags uint32, parameters *AttachVirtualDiskParameters, overlapped *syscall.Overlapped) (err error) [failretval != 0] = virtdisk.AttachVirtualDisk
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TBBle The signature for these is changing in this PR anyways so let's make sure we get in the other /x/sys/windows'ify PR shortly after and we can cut a release.

Copy link
Contributor

@TBBle TBBle Mar 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I did a quick review previously, and the conflicts are textual-only. i.e. on this line I changed the type of handle and overlapped only.

So assuming this lands first, rebasing #197 should only take a few minutes, mostly compile-checking everything.

//sys detachVirtualDisk(handle syscall.Handle, detachVirtualDiskFlags uint32, providerSpecificFlags uint32) (err error) [failretval != 0] = virtdisk.DetachVirtualDisk
//sys getVirtualDiskPhysicalPath(handle syscall.Handle, diskPathSizeInBytes *uint32, buffer *uint16) (err error) [failretval != 0] = virtdisk.GetVirtualDiskPhysicalPath

Expand Down
6 changes: 3 additions & 3 deletions vhd/zvhd_windows.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.