From 77f9aa4dc4249982cb13dea1d8b2fd003a0cff66 Mon Sep 17 00:00:00 2001 From: Steven Rhodes Date: Wed, 3 May 2023 06:07:36 +0000 Subject: [PATCH 1/4] Compile sansshell on windows. SansShell is primarily meant for linux server management, but most of the go libraries that it uses have some windows support. Making it compile under GOOS=windows requires modest tweaks. Various unix-isms like signals and uid/gid are present in the SansShell API and can only be best-effort supported on windows. I haven't comformed that the code runs on windows. I've only confirmed that I can cross-compile for windows from linux. --- .github/workflows/build-test.yaml | 37 ++++++++--- services/localfile/server/localfile.go | 18 +++--- .../localfile/server/localfile_default.go | 8 +-- services/localfile/server/localfile_unix.go | 12 ++++ .../localfile/server/localfile_windows.go | 64 +++++++++++++++++++ services/process/server/process.go | 6 +- services/process/server/process_default.go | 8 ++- .../process/server/process_default_test.go | 2 +- services/process/server/process_linux.go | 4 -- services/util/util.go | 9 ++- services/util/util_unix.go | 32 ++++++++++ services/util/util_windows.go | 10 +++ 12 files changed, 173 insertions(+), 37 deletions(-) create mode 100644 services/localfile/server/localfile_unix.go create mode 100644 services/localfile/server/localfile_windows.go create mode 100644 services/util/util_unix.go create mode 100644 services/util/util_windows.go diff --git a/.github/workflows/build-test.yaml b/.github/workflows/build-test.yaml index 8fe08ff1..bf849a75 100644 --- a/.github/workflows/build-test.yaml +++ b/.github/workflows/build-test.yaml @@ -8,18 +8,18 @@ on: - v* pull_request: schedule: - - cron: '7 3 * * *' + - cron: "7 3 * * *" jobs: pre-commit: name: Pre commit runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3 - - uses: actions/setup-go@v3 - with: - go-version-file: 'go.mod' - - uses: actions/setup-python@v3 - - uses: pre-commit/action@v3.0.0 + - uses: actions/checkout@v3 + - uses: actions/setup-go@v3 + with: + go-version-file: "go.mod" + - uses: actions/setup-python@v3 + - uses: pre-commit/action@v3.0.0 test: name: Unit tests runs-on: ubuntu-20.04 @@ -27,7 +27,7 @@ jobs: - uses: actions/checkout@v3 - uses: actions/setup-go@v3 with: - go-version-file: 'go.mod' + go-version-file: "go.mod" - name: Install tools run: | sudo apt-get update @@ -37,4 +37,23 @@ jobs: - name: integration tests run: ./testing/integrate.sh shell: bash - + test-macos: + name: Test on macos + runs-on: macos-latest + steps: + - uses: actions/checkout@v3 + - uses: actions/setup-go@v3 + with: + go-version-file: "go.mod" + - name: unit tests + run: go test ./... + test-windows: + name: Test on windows + runs-on: windows-latest + steps: + - uses: actions/checkout@v3 + - uses: actions/setup-go@v3 + with: + go-version-file: "go.mod" + - name: unit tests + run: go test ./... diff --git a/services/localfile/server/localfile.go b/services/localfile/server/localfile.go index 227ea900..6692dfed 100644 --- a/services/localfile/server/localfile.go +++ b/services/localfile/server/localfile.go @@ -43,8 +43,6 @@ import ( "github.com/Snowflake-Labs/sansshell/services/util" "github.com/Snowflake-Labs/sansshell/telemetry/metrics" - "golang.org/x/sys/unix" - "github.com/go-logr/logr" "google.golang.org/grpc" "google.golang.org/grpc/codes" @@ -57,7 +55,7 @@ var ( AbsolutePathError = status.Error(codes.InvalidArgument, "filename path must be absolute and clean") // For testing since otherwise tests have to run as root for these. - chown = unix.Chown + chown = os.Chown changeImmutableOS = changeImmutable // ReadTimeout is how long tail should wait on a given poll call @@ -95,7 +93,7 @@ var ( // This encompasses the permission plus the setuid/gid/sticky bits one // can set on a file/directory. -const modeMask = uint32(fs.ModePerm | fs.ModeSticky | fs.ModeSetuid | fs.ModeSetgid) +const modeMask = fs.ModePerm | fs.ModeSticky | fs.ModeSetuid | fs.ModeSetgid // server is used to implement the gRPC server type server struct{} @@ -577,7 +575,7 @@ type immutableState struct { func validateAndSetAttrs(filename string, attrs []*pb.FileAttribute, doImmutable bool) (*immutableState, error) { uid, gid := int(-1), int(-1) setMode, setImmutable, immutable := false, false, false - mode := uint32(0) + mode := fs.FileMode(0) for _, attr := range attrs { switch a := attr.Value.(type) { @@ -621,7 +619,7 @@ func validateAndSetAttrs(filename string, attrs []*pb.FileAttribute, doImmutable if setMode { return nil, status.Error(codes.InvalidArgument, "cannot set mode more than once") } - mode = a.Mode + mode = fs.FileMode(a.Mode) setMode = true case *pb.FileAttribute_Immutable: if setImmutable { @@ -639,7 +637,7 @@ func validateAndSetAttrs(filename string, attrs []*pb.FileAttribute, doImmutable } if setMode { - if err := unix.Chmod(filename, mode&modeMask); err != nil { + if err := os.Chmod(filename, mode&modeMask); err != nil { return nil, status.Errorf(codes.Internal, "error from chmod: %v", err) } } @@ -687,7 +685,7 @@ func (s *server) Rm(ctx context.Context, req *pb.RmRequest) (*emptypb.Empty, err recorder.CounterOrLog(ctx, localfileRmFailureCounter, 1, attribute.String("reason", "invalid_path")) return nil, err } - err := unix.Unlink(req.Filename) + err := osAgnosticRm(req.Filename) if err != nil { recorder.CounterOrLog(ctx, localfileRmFailureCounter, 1, attribute.String("reason", "unlink_err")) return nil, status.Errorf(codes.Internal, "unlink error: %v", err) @@ -703,7 +701,7 @@ func (s *server) Rmdir(ctx context.Context, req *pb.RmdirRequest) (*emptypb.Empt recorder.CounterOrLog(ctx, localfileRmDirFailureCounter, 1, attribute.String("reason", "invalid_path")) return nil, err } - err := unix.Rmdir(req.Directory) + err := osAgnosticRmdir(req.Directory) if err != nil { recorder.CounterOrLog(ctx, localfileRmDirFailureCounter, 1, attribute.String("reason", "rmdir_err")) return nil, status.Errorf(codes.Internal, "rmdir error: %v", err) @@ -723,7 +721,7 @@ func (s *server) Rename(ctx context.Context, req *pb.RenameRequest) (*emptypb.Em recorder.CounterOrLog(ctx, localfileRenameFailureCounter, 1, attribute.String("reason", "invalid_dst_path")) return nil, err } - err := unix.Rename(req.OriginalName, req.DestinationName) + err := os.Rename(req.OriginalName, req.DestinationName) if err != nil { recorder.CounterOrLog(ctx, localfileRenameFailureCounter, 1, attribute.String("reason", "rename_err")) return nil, status.Errorf(codes.Internal, "rename error: %v", err) diff --git a/services/localfile/server/localfile_default.go b/services/localfile/server/localfile_default.go index 604cccd1..f44bcb83 100644 --- a/services/localfile/server/localfile_default.go +++ b/services/localfile/server/localfile_default.go @@ -1,5 +1,5 @@ -//go:build !(linux || darwin) -// +build !linux,!darwin +//go:build !(linux || darwin || windows) +// +build !linux,!darwin,!windows /* Copyright (c) 2019 Snowflake Inc. All rights reserved. @@ -83,14 +83,14 @@ func dataPrep(f *os.File) (interface{}, func(), error) { // stream and then return no matter what (assuming the file was already // at EOF). func dataReady(_ interface{}, stream pb.LocalFile_ReadServer) error { - // We sleep for READ_TIMEOUT_SEC between calls as there's no good + // We sleep for ReadTimeout between calls as there's no good // way to poll on a file. Once it reaches EOF it's always readable // (you just get EOF). We have to poll like this so we can check // the context state and return if it's canclled. if stream.Context().Err() != nil { return stream.Context().Err() } - time.Sleep(READ_TIMEOUT_SEC * time.Second) + time.Sleep(ReadTimeout * time.Second) // Time to try again. return nil } diff --git a/services/localfile/server/localfile_unix.go b/services/localfile/server/localfile_unix.go new file mode 100644 index 00000000..ba842331 --- /dev/null +++ b/services/localfile/server/localfile_unix.go @@ -0,0 +1,12 @@ +//go:build unix + +package server + +import ( + "golang.org/x/sys/unix" +) + +var ( + osAgnosticRm = unix.Unlink + osAgnosticRmdir = unix.Rmdir +) diff --git a/services/localfile/server/localfile_windows.go b/services/localfile/server/localfile_windows.go new file mode 100644 index 00000000..9bccdbcd --- /dev/null +++ b/services/localfile/server/localfile_windows.go @@ -0,0 +1,64 @@ +package server + +import ( + "io/fs" + "os" + "time" + + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + "google.golang.org/protobuf/types/known/timestamppb" + + pb "github.com/Snowflake-Labs/sansshell/services/localfile" +) + +var ( + osAgnosticRm = os.Remove + osAgnosticRmdir = os.Remove +) + +// osStat is the Windows version of geting file status. We only support +// the Windows-relevant subset of information. +func osStat(path string, useLstat bool) (*pb.StatReply, error) { + var stat fs.FileInfo + var err error + if useLstat { + stat, err = os.Lstat(path) + if err != nil { + return nil, status.Errorf(codes.Internal, "stat: os.Lstat error %v", err) + } + } else { + stat, err = os.Stat(path) + if err != nil { + return nil, status.Errorf(codes.Internal, "stat: os.Stat error %v", err) + } + } + resp := &pb.StatReply{ + Filename: path, + Size: stat.Size(), + Mode: uint32(stat.Mode()), + Modtime: timestamppb.New(stat.ModTime()), + } + return resp, nil +} + +// Windows doesn't have immutable files +func changeImmutable(path string, immutable bool) error { + return status.Error(codes.Unimplemented, "immutable not supported") +} + +func dataPrep(f *os.File) (interface{}, func(), error) { + return nil, func() {}, nil +} + +// dataReady is the OS specific version to indicate the given +// file has more data. This could be optimized for Windows with +// ReadDirectoryChangesW and golang.org/x/sys/windows. +func dataReady(_ interface{}, stream pb.LocalFile_ReadServer) error { + if stream.Context().Err() != nil { + return stream.Context().Err() + } + time.Sleep(ReadTimeout * time.Second) + // Time to try again. + return nil +} diff --git a/services/process/server/process.go b/services/process/server/process.go index ed11b265..644675f6 100644 --- a/services/process/server/process.go +++ b/services/process/server/process.go @@ -177,8 +177,12 @@ func (s *server) Kill(ctx context.Context, req *pb.KillRequest) (*emptypb.Empty, recorder.CounterOrLog(ctx, processKillFailureCounter, 1, attribute.String("reason", "invalid_pid")) return nil, status.Error(codes.InvalidArgument, "pid must be positive and non-zero") } - err := syscall.Kill(int(req.Pid), syscall.Signal(req.Signal)) + process, err := os.FindProcess(int(req.Pid)) if err != nil { + recorder.CounterOrLog(ctx, processKillFailureCounter, 1, attribute.String("reason", "kill_err")) + return nil, err + } + if err := process.Signal(syscall.Signal(req.Signal)); err != nil { recorder.CounterOrLog(ctx, processKillFailureCounter, 1, attribute.String("reason", "kill_err")) return nil, status.Errorf(codes.Internal, "kill returned error: %v", err) } diff --git a/services/process/server/process_default.go b/services/process/server/process_default.go index be380808..ab64ef37 100644 --- a/services/process/server/process_default.go +++ b/services/process/server/process_default.go @@ -23,6 +23,8 @@ import ( "fmt" "io" "runtime" + + pb "github.com/Snowflake-Labs/sansshell/services/process" ) var ( @@ -35,11 +37,11 @@ var ( // GcoreBin is the location of the gcore binary. On non linux/OS/X this isn't supported. GcoreBin = "" - psOptions = func() ([]string, error) { - return nil, fmt.Errorf("No support for OS %s", runtime.GOOS) + psOptions = func() []string { + return nil } ) -func parser(r io.Reader) (map[int64]*ProcessEntry, error) { +func parser(r io.Reader) (map[int64]*pb.ProcessEntry, error) { return nil, fmt.Errorf("No support for OS %s", runtime.GOOS) } diff --git a/services/process/server/process_default_test.go b/services/process/server/process_default_test.go index 5e431816..5ddd4623 100644 --- a/services/process/server/process_default_test.go +++ b/services/process/server/process_default_test.go @@ -17,7 +17,7 @@ under the License. */ -package process +package server // OS specific locations for finding test data. // In this case all blank so tests skip. diff --git a/services/process/server/process_linux.go b/services/process/server/process_linux.go index 20448791..43fe55e4 100644 --- a/services/process/server/process_linux.go +++ b/services/process/server/process_linux.go @@ -19,10 +19,6 @@ package server -// To regenerate the proto headers if the .proto changes, just run go generate -// and this encodes the necessary magic: -//go:generate protoc --go_out=. --go_opt=paths=source_relative --go-grpc_out=require_unimplemented_servers=false:. --go-grpc_opt=paths=source_relative process.proto - import ( "bufio" "fmt" diff --git a/services/util/util.go b/services/util/util.go index 695831cf..42316000 100644 --- a/services/util/util.go +++ b/services/util/util.go @@ -27,7 +27,6 @@ import ( "path/filepath" "strconv" "strings" - "syscall" "github.com/go-logr/logr" "google.golang.org/grpc/codes" @@ -244,11 +243,11 @@ func RunCommand(ctx context.Context, bin string, args []string, opts ...Option) // Only do this if it's different than our current ones since // attempting to setuid/gid() to even your current values is EPERM. if options.uid != euid || options.gid != gid { - cmd.SysProcAttr = &syscall.SysProcAttr{ - Credential: &syscall.Credential{}, + sysProcAddr, err := getSysProcAttr(options) + if err != nil { + return nil, err } - cmd.SysProcAttr.Credential.Uid = options.uid - cmd.SysProcAttr.Credential.Gid = options.gid + cmd.SysProcAttr = sysProcAddr } logger.Info("executing local command", "cmd", cmd.String()) run.Error = cmd.Run() diff --git a/services/util/util_unix.go b/services/util/util_unix.go new file mode 100644 index 00000000..5637ed9b --- /dev/null +++ b/services/util/util_unix.go @@ -0,0 +1,32 @@ +//go:build unix + +/* Copyright (c) 2019 Snowflake Inc. All rights reserved. + + Licensed under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. +*/ + +package util + +import ( + "syscall" +) + +func getSysProcAttr(options *cmdOptions) (*syscall.SysProcAttr, error) { + return &syscall.SysProcAttr{ + Credential: &syscall.Credential{ + Uid: options.uid, + Gid: options.gid, + }, + }, nil +} diff --git a/services/util/util_windows.go b/services/util/util_windows.go new file mode 100644 index 00000000..34b6e1c8 --- /dev/null +++ b/services/util/util_windows.go @@ -0,0 +1,10 @@ +package util + +import ( + "errors" + "syscall" +) + +func getSysProcAttr(options *cmdOptions) (*syscall.SysProcAttr, error) { + return nil, errors.New("setting uid/gid is unsupported on Windows") +} From bfeb81599a82601a8ffef3ce0a8c8f0a1a25f8f0 Mon Sep 17 00:00:00 2001 From: Steven Rhodes Date: Sun, 4 Jun 2023 23:30:33 -0700 Subject: [PATCH 2/4] Update tests on windows to pass --- .gitattributes | 3 + server/server_test.go | 31 +--- server/server_unix_test.go | 21 +++ server/server_windows_test.go | 21 +++ services/fdb/server/conf_test.go | 2 + services/fdb/server/fdbcli_test.go | 2 + services/fdb/server/server_test.go | 2 + services/localfile/server/localfile.go | 6 +- .../localfile/server/localfile_default.go | 6 +- services/localfile/server/localfile_test.go | 146 +++++++++++------- .../localfile/server/localfile_unix_test.go | 8 + .../localfile/server/localfile_windows.go | 10 +- .../server/localfile_windows_test.go | 8 + .../process/server/process_default_test.go | 16 +- services/process/server/process_test.go | 9 ++ services/util/util_test.go | 9 ++ 16 files changed, 208 insertions(+), 92 deletions(-) create mode 100644 .gitattributes create mode 100644 server/server_unix_test.go create mode 100644 server/server_windows_test.go create mode 100644 services/localfile/server/localfile_unix_test.go create mode 100644 services/localfile/server/localfile_windows_test.go diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 00000000..3d9c9751 --- /dev/null +++ b/.gitattributes @@ -0,0 +1,3 @@ +# We checksum this data in tests so we need the content to be +# identical across operating systems. +services/localfile/server/testdata/sum.data text eol=lf \ No newline at end of file diff --git a/server/server_test.go b/server/server_test.go index 58e13e57..a45f3f28 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -47,12 +47,13 @@ package sansshell.authz default allow = false allow { - input.type = "LocalFile.ReadActionRequest" - input.message.file.filename = "/etc/hosts" -} -allow { - input.type = "LocalFile.ReadActionRequest" - input.message.file.filename = "/no-such-filename-for-sansshell-unittest" + input.type = "LocalFile.ReadActionRequest" + input.message.file.filename in [ + "/etc/hosts", + "/no-such-filename-for-sansshell-unittest", + "C:\\Windows\\win.ini", + "C:\\no-such-filename-for-sansshell-unittest", + ] } ` ) @@ -138,23 +139,7 @@ func TestRead(t *testing.T) { testutil.FatalOnErr("Failed to dial bufnet", err, t) t.Cleanup(func() { conn.Close() }) - for _, tc := range []struct { - filename string - err string - }{ - { - filename: "/etc/hosts", - err: "", - }, - { - filename: "/no-such-filename-for-sansshell-unittest", - err: "no such file or directory", - }, - { - filename: "/permission-denied-filename-for-sansshell-unittest", - err: "PermissionDenied", - }, - } { + for _, tc := range readFileTestCases { tc := tc t.Run(tc.filename, func(t *testing.T) { client := lfpb.NewLocalFileClient(conn) diff --git a/server/server_unix_test.go b/server/server_unix_test.go new file mode 100644 index 00000000..937f605a --- /dev/null +++ b/server/server_unix_test.go @@ -0,0 +1,21 @@ +//go:build unix + +package server + +var readFileTestCases = []struct { + filename string + err string +}{ + { + filename: "/etc/hosts", + err: "", + }, + { + filename: "/no-such-filename-for-sansshell-unittest", + err: "no such file or directory", + }, + { + filename: "/permission-denied-filename-for-sansshell-unittest", + err: "PermissionDenied", + }, +} diff --git a/server/server_windows_test.go b/server/server_windows_test.go new file mode 100644 index 00000000..e44577a8 --- /dev/null +++ b/server/server_windows_test.go @@ -0,0 +1,21 @@ +//go:build windows + +package server + +var readFileTestCases = []struct { + filename string + err string +}{ + { + filename: "C:\\Windows\\win.ini", + err: "", + }, + { + filename: "C:\\no-such-filename-for-sansshell-unittest", + err: "The system cannot find the file specified", + }, + { + filename: "/permission-denied-filename-for-sansshell-unittest", + err: "PermissionDenied", + }, +} diff --git a/services/fdb/server/conf_test.go b/services/fdb/server/conf_test.go index 873181ff..7d608619 100644 --- a/services/fdb/server/conf_test.go +++ b/services/fdb/server/conf_test.go @@ -1,3 +1,5 @@ +//go:build unix + /* Copyright (c) 2019 Snowflake Inc. All rights reserved. Licensed under the Apache License, Version 2.0 (the diff --git a/services/fdb/server/fdbcli_test.go b/services/fdb/server/fdbcli_test.go index 1a7b54bd..ab8f7c46 100644 --- a/services/fdb/server/fdbcli_test.go +++ b/services/fdb/server/fdbcli_test.go @@ -1,3 +1,5 @@ +//go:build unix + /* Copyright (c) 2019 Snowflake Inc. All rights reserved. Licensed under the Apache License, Version 2.0 (the diff --git a/services/fdb/server/server_test.go b/services/fdb/server/server_test.go index a1cfad4e..e8bd5cc8 100644 --- a/services/fdb/server/server_test.go +++ b/services/fdb/server/server_test.go @@ -1,3 +1,5 @@ +//go:build unix + /* Copyright (c) 2019 Snowflake Inc. All rights reserved. Licensed under the Apache License, Version 2.0 (the diff --git a/services/localfile/server/localfile.go b/services/localfile/server/localfile.go index 6692dfed..8ca89fa0 100644 --- a/services/localfile/server/localfile.go +++ b/services/localfile/server/localfile.go @@ -32,6 +32,7 @@ import ( "os" "os/user" "path/filepath" + "runtime" "strconv" "time" @@ -318,6 +319,9 @@ func setupOutput(a *pb.FileAttributes) (*os.File, *immutableState, error) { // to accidentally leave this in another otherwise default state. // Except we don't trigger immutable now or we won't be able to write to it. immutable, err := validateAndSetAttrs(f.Name(), a.Attributes, false) + if err != nil { + f.Close() + } return f, immutable, err } @@ -630,7 +634,7 @@ func validateAndSetAttrs(filename string, attrs []*pb.FileAttribute, doImmutable } } - if uid != -1 || gid != -1 { + if runtime.GOOS != "windows" { if err := chown(filename, uid, gid); err != nil { return nil, status.Errorf(codes.Internal, "error from chown: %v", err) } diff --git a/services/localfile/server/localfile_default.go b/services/localfile/server/localfile_default.go index f44bcb83..7ec6a7e4 100644 --- a/services/localfile/server/localfile_default.go +++ b/services/localfile/server/localfile_default.go @@ -32,9 +32,11 @@ import ( pb "github.com/Snowflake-Labs/sansshell/services/localfile" ) -// osStat is the platform agnostic version which uses basic os.Stat. +var osStat = defaultOsStat + +// defaultOsStat is the platform agnostic version which uses basic os.Stat. // As a result immutable bits cannot be returned. -func osStat(path string, useLstat bool) (*pb.StatReply, error) { +func defaultOsStat(path string, useLstat bool) (*pb.StatReply, error) { var stat fs.FileInfo var err error if useLstat { diff --git a/services/localfile/server/localfile_test.go b/services/localfile/server/localfile_test.go index c5aaa6f0..fb841604 100644 --- a/services/localfile/server/localfile_test.go +++ b/services/localfile/server/localfile_test.go @@ -27,15 +27,14 @@ import ( "net" "os" "os/user" - "path" "path/filepath" + "runtime" "strconv" "strings" "testing" "time" _ "gocloud.dev/blob/fileblob" - "golang.org/x/sys/unix" "google.golang.org/grpc" "google.golang.org/grpc/credentials/insecure" "google.golang.org/grpc/test/bufconn" @@ -101,25 +100,25 @@ func TestRead(t *testing.T) { length int64 }{ { - name: "/etc/hosts-normal", - filename: "/etc/hosts", + name: "validFile-normal", + filename: validFile, chunksize: 10, }, { - name: "/etc/hosts-1-byte-chunk", - filename: "/etc/hosts", + name: "validFile-1-byte-chunk", + filename: validFile, chunksize: 1, }, { - name: "/etc/hosts-with-offset-and-length", - filename: "/etc/hosts", + name: "validFile-with-offset-and-length", + filename: validFile, chunksize: 10, offset: 10, length: 15, }, { - name: "/etc/hosts-from-end", - filename: "/etc/hosts", + name: "validFile-from-end", + filename: validFile, chunksize: 10, offset: -20, length: 15, @@ -131,7 +130,7 @@ func TestRead(t *testing.T) { }, { name: "non-absolute file", - filename: "/tmp/foo/../../etc/passwd", + filename: nonAbsolutePath, wantErr: true, }, } { @@ -203,7 +202,7 @@ func TestRead(t *testing.T) { } func TestTail(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) conn, err := grpc.DialContext(ctx, "bufnet", grpc.WithContextDialer(bufDialer), grpc.WithTransportCredentials(insecure.NewCredentials())) testutil.FatalOnErr("grpc.DialContext(bufnet)", err, t) t.Cleanup(func() { conn.Close() }) @@ -416,8 +415,8 @@ func TestSum(t *testing.T) { req: &pb.SumRequest{Filename: temp, SumType: pb.SumType_SUM_TYPE_SHA256}, sendErrFunc: testutil.FatalOnErr, recvErrFunc: func(op string, err error, t *testing.T) { - if err == nil || !strings.Contains(err.Error(), "directory") { - t.Fatalf("%s : err was %v, want err containing 'directory'", op, err) + if err == nil || (!strings.Contains(err.Error(), "directory") && !strings.Contains(err.Error(), "Incorrect")) { + t.Fatalf("%s : err was %v, want err containing 'directory' or 'Incorrect'", op, err) } }, }, @@ -509,6 +508,10 @@ func TestSum(t *testing.T) { } func TestSetFileAttributes(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("file attributes are mostly unsupported on windows") + } + ctx := context.Background() conn, err := grpc.DialContext(ctx, "bufnet", grpc.WithContextDialer(bufDialer), grpc.WithTransportCredentials(insecure.NewCredentials())) testutil.FatalOnErr("grpc.DialContext(bufnet)", err, t) @@ -528,7 +531,7 @@ func TestSetFileAttributes(t *testing.T) { testutil.FatalOnErr("os.Mkdir", err, t) f2, err := os.CreateTemp(badDir, "testfile.*") testutil.FatalOnErr("os.CreateTemp", err, t) - err = unix.Chmod(badDir, 0) + err = os.Chmod(badDir, 0) testutil.FatalOnErr("chmod", err, t) setPath := "" @@ -565,7 +568,7 @@ func TestSetFileAttributes(t *testing.T) { chown = savedChown changeImmutableOS = savedChangeImmutableOS // Needed or we panic with generated cleanup trying to remove tmp directories. - err = unix.Chmod(badDir, uint32(fs.ModePerm)) + err = os.Chmod(badDir, fs.ModePerm) testutil.FatalOnErr("Chmod", err, t) }) // Tests below set user to nobody. @@ -911,6 +914,7 @@ func TestList(t *testing.T) { temp := t.TempDir() f1, err := os.CreateTemp(temp, "testfile.*") testutil.FatalOnErr("os.CreateTemp", err, t) + f1.Close() symlink := filepath.Join(temp, "sym") testutil.FatalOnErr("osSymlink", os.Symlink("broken", symlink), t) @@ -931,11 +935,12 @@ func TestList(t *testing.T) { origOsStat := osStat for _, tc := range []struct { - name string - req *pb.ListRequest - osStat func(string, bool) (*pb.StatReply, error) - wantErr bool - expected []*pb.StatReply + name string + req *pb.ListRequest + osStat func(string, bool) (*pb.StatReply, error) + wantErr bool + skipOnWindows bool + expected []*pb.StatReply }{ { name: "Blank filename", @@ -945,7 +950,7 @@ func TestList(t *testing.T) { { name: "Non absolute path", req: &pb.ListRequest{ - Entry: "/tmp/foo/../../etc/passwd", + Entry: nonAbsolutePath, }, wantErr: true, }, @@ -981,7 +986,8 @@ func TestList(t *testing.T) { req: &pb.ListRequest{ Entry: badDir, }, - wantErr: true, + skipOnWindows: true, + wantErr: true, }, { name: "stat fails inside directory", @@ -1000,6 +1006,9 @@ func TestList(t *testing.T) { } { tc := tc t.Run(tc.name, func(t *testing.T) { + if tc.skipOnWindows { + return + } client := pb.NewLocalFileClient(conn) t.Cleanup(func() { @@ -1072,6 +1081,7 @@ func TestWrite(t *testing.T) { touchFile bool immutableError bool validateImmutable bool + skipOnWindows bool }{ { name: "Blank request", @@ -1530,12 +1540,13 @@ func TestCopy(t *testing.T) { testutil.FatalOnErr("Close", err, t) for _, tc := range []struct { - name string - req *pb.CopyRequest - wantErr bool - filename string - validate string - touchFile bool + name string + req *pb.CopyRequest + wantErr bool + filename string + validate string + touchFile bool + skipOnWindows bool }{ { name: "Blank request", @@ -1668,12 +1679,17 @@ func TestCopy(t *testing.T) { Bucket: fmt.Sprintf("file://%s", filepath.Dir(f1.Name())), Key: filepath.Base(f1.Name()), }, - validate: contents, - filename: filepath.Join(temp, "file"), + validate: contents, + filename: filepath.Join(temp, "file"), + skipOnWindows: true, }, } { tc := tc t.Run(tc.name, func(t *testing.T) { + if tc.skipOnWindows && runtime.GOOS == "windows" { + return + } + defer func() { if tc.filename != "" { os.Remove(tc.filename) @@ -1714,34 +1730,38 @@ func TestRm(t *testing.T) { temp := t.TempDir() f1, err := os.CreateTemp(temp, "testfile.*") testutil.FatalOnErr("os.CreateTemp", err, t) - badDir := filepath.Join(temp, "/bad") + badDir := filepath.Join(temp, "bad") err = os.Mkdir(badDir, fs.ModePerm) testutil.FatalOnErr("os.Mkdir", err, t) f2, err := os.CreateTemp(badDir, "testfile.*") testutil.FatalOnErr("os.CreateTemp", err, t) - err = unix.Chmod(badDir, 0) + err = os.Chmod(badDir, 0) testutil.FatalOnErr("Chmod", err, t) + f1.Close() + f2.Close() t.Cleanup(func() { // Needed or we panic with generated cleanup trying to remove tmp directories. - err = unix.Chmod(badDir, uint32(fs.ModePerm)) + err = os.Chmod(badDir, fs.ModePerm) testutil.FatalOnErr("Chmod", err, t) }) for _, tc := range []struct { - name string - filename string - wantErr bool + name string + filename string + skipOnWindows bool + wantErr bool }{ { name: "bad path", - filename: "/tmp/foo/../../etc/passwd", + filename: nonAbsolutePath, wantErr: true, }, { - name: "bad permissions to file", - filename: f2.Name(), - wantErr: true, + name: "bad permissions to file", + filename: f2.Name(), + skipOnWindows: true, + wantErr: true, }, { name: "working remove", @@ -1750,6 +1770,9 @@ func TestRm(t *testing.T) { } { tc := tc t.Run(tc.name, func(t *testing.T) { + if tc.skipOnWindows { + return + } client := pb.NewLocalFileClient(conn) _, err := client.Rm(ctx, &pb.RmRequest{Filename: tc.filename}) testutil.WantErr(tc.name, err, tc.wantErr, t) @@ -1773,19 +1796,20 @@ func TestRmdir(t *testing.T) { failDir := filepath.Join(temp, "/bad", "/bad") err = os.Mkdir(failDir, 0) testutil.FatalOnErr("os.Mkdir", err, t) - err = unix.Chmod(badDir, 0) + err = os.Chmod(badDir, 0) testutil.FatalOnErr("Chmod", err, t) t.Cleanup(func() { // Needed or we panic with generated cleanup trying to remove tmp directories. - err = unix.Chmod(badDir, uint32(fs.ModePerm)) + err = os.Chmod(badDir, fs.ModePerm) testutil.FatalOnErr("Chmod", err, t) }) for _, tc := range []struct { - name string - directory string - wantErr bool + name string + directory string + skipOnWindows bool + wantErr bool }{ { name: "bad path", @@ -1793,9 +1817,10 @@ func TestRmdir(t *testing.T) { wantErr: true, }, { - name: "bad permissions to directory", - directory: failDir, - wantErr: true, + name: "bad permissions to directory", + directory: failDir, + skipOnWindows: true, + wantErr: true, }, { name: "working remove", @@ -1804,6 +1829,9 @@ func TestRmdir(t *testing.T) { } { tc := tc t.Run(tc.name, func(t *testing.T) { + if tc.skipOnWindows && runtime.GOOS == "windows" { + return + } client := pb.NewLocalFileClient(conn) _, err := client.Rmdir(ctx, &pb.RmdirRequest{Directory: tc.directory}) testutil.WantErr(tc.name, err, tc.wantErr, t) @@ -1827,7 +1855,7 @@ func TestRename(t *testing.T) { t.Cleanup(func() { // Needed or we panic with generated cleanup trying to remove tmp directories. - err = unix.Chmod(badDir, uint32(fs.ModePerm)) + err = os.Chmod(badDir, fs.ModePerm) testutil.FatalOnErr("Chmod", err, t) }) @@ -1854,7 +1882,7 @@ func TestRename(t *testing.T) { { name: "bad permissions to directory", req: &pb.RenameRequest{ - OriginalName: path.Join(temp, "file"), + OriginalName: filepath.Join(temp, "file"), DestinationName: badDir, }, wantErr: true, @@ -1862,23 +1890,23 @@ func TestRename(t *testing.T) { { name: "working rename", req: &pb.RenameRequest{ - OriginalName: path.Join(temp, "file"), - DestinationName: path.Join(temp, "newfile"), + OriginalName: filepath.Join(temp, "file"), + DestinationName: filepath.Join(temp, "newfile"), }, }, { name: "rename a directory", req: &pb.RenameRequest{ OriginalName: dir, - DestinationName: path.Join(temp, "newdir"), + DestinationName: filepath.Join(temp, "newdir"), }, }, } { tc := tc t.Run(tc.name, func(t *testing.T) { - err := os.WriteFile(path.Join(temp, "file"), []byte("contents"), 0644) + err := os.WriteFile(filepath.Join(temp, "file"), []byte("contents"), 0644) testutil.FatalOnErr("WriteFile", err, t) - defer os.Remove(path.Join(temp, "file")) + defer os.Remove(filepath.Join(temp, "file")) client := pb.NewLocalFileClient(conn) _, err = client.Rename(ctx, tc.req) testutil.WantErr(tc.name, err, tc.wantErr, t) @@ -1895,7 +1923,8 @@ func TestRename(t *testing.T) { } func TestReadlink(t *testing.T) { - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() conn, err := grpc.DialContext(ctx, "bufnet", grpc.WithContextDialer(bufDialer), grpc.WithTransportCredentials(insecure.NewCredentials())) testutil.FatalOnErr("grpc.DialContext(bufnet)", err, t) t.Cleanup(func() { conn.Close() }) @@ -1903,6 +1932,7 @@ func TestReadlink(t *testing.T) { temp := t.TempDir() f1, err := os.CreateTemp(temp, "testfile.*") testutil.FatalOnErr("os.CreateTemp", err, t) + t.Cleanup(func() { f1.Close() }) symlink := filepath.Join(temp, "sym") testutil.FatalOnErr("osSymlink", os.Symlink(f1.Name(), symlink), t) @@ -1923,7 +1953,7 @@ func TestReadlink(t *testing.T) { { name: "Non absolute path", req: &pb.ReadlinkRequest{ - Filename: "/tmp/foo/../../etc/passwd", + Filename: nonAbsolutePath, }, wantErr: true, }, diff --git a/services/localfile/server/localfile_unix_test.go b/services/localfile/server/localfile_unix_test.go new file mode 100644 index 00000000..474b77b2 --- /dev/null +++ b/services/localfile/server/localfile_unix_test.go @@ -0,0 +1,8 @@ +//go:build unix + +package server + +var ( + nonAbsolutePath = "/tmp/foo/../../etc/passwd" + validFile = "/etc/hosts" +) diff --git a/services/localfile/server/localfile_windows.go b/services/localfile/server/localfile_windows.go index 9bccdbcd..d4a83855 100644 --- a/services/localfile/server/localfile_windows.go +++ b/services/localfile/server/localfile_windows.go @@ -2,6 +2,7 @@ package server import ( "io/fs" + "math" "os" "time" @@ -15,11 +16,12 @@ import ( var ( osAgnosticRm = os.Remove osAgnosticRmdir = os.Remove + osStat = windowsOsStat ) -// osStat is the Windows version of geting file status. We only support +// windowsOsStat is the Windows version of geting file status. We only support // the Windows-relevant subset of information. -func osStat(path string, useLstat bool) (*pb.StatReply, error) { +func windowsOsStat(path string, useLstat bool) (*pb.StatReply, error) { var stat fs.FileInfo var err error if useLstat { @@ -38,6 +40,8 @@ func osStat(path string, useLstat bool) (*pb.StatReply, error) { Size: stat.Size(), Mode: uint32(stat.Mode()), Modtime: timestamppb.New(stat.ModTime()), + Uid: math.MaxUint32, + Gid: math.MaxUint32, } return resp, nil } @@ -58,7 +62,7 @@ func dataReady(_ interface{}, stream pb.LocalFile_ReadServer) error { if stream.Context().Err() != nil { return stream.Context().Err() } - time.Sleep(ReadTimeout * time.Second) + time.Sleep(ReadTimeout) // Time to try again. return nil } diff --git a/services/localfile/server/localfile_windows_test.go b/services/localfile/server/localfile_windows_test.go new file mode 100644 index 00000000..c3eadad0 --- /dev/null +++ b/services/localfile/server/localfile_windows_test.go @@ -0,0 +1,8 @@ +//go:build windows + +package server + +var ( + nonAbsolutePath = "C:\\Windows\\..\\Windows\\win.ini" + validFile = "C:\\Windows\\win.ini" +) diff --git a/services/process/server/process_default_test.go b/services/process/server/process_default_test.go index 5ddd4623..1e2c59c1 100644 --- a/services/process/server/process_default_test.go +++ b/services/process/server/process_default_test.go @@ -22,10 +22,16 @@ package server // OS specific locations for finding test data. // In this case all blank so tests skip. var ( - testdataPsFile = "" - testdataPs = "" - badFilesPS = nil + testdataPsTextProto = "" + testdataPs = "" + badFilesPs = []string{} - testdataPstackNoThreadsFile = "" - testdataPstackThreadsFile = "" + testdataPstackNoThreads = "" + testdataPstackNoThreadsTextProto = "" + testdataPstackThreads = "" + testdataPstackThreadsTextProto = "" + testdataPstackThreadsBadThread = "" + testdataPstackThreadsBadThreadNumber = "" + testdataPstackThreadsBadThreadID = "" + testdataPstackThreadsBadLwp = "" ) diff --git a/services/process/server/process_test.go b/services/process/server/process_test.go index a77b8ce5..5fcd640f 100644 --- a/services/process/server/process_test.go +++ b/services/process/server/process_test.go @@ -26,6 +26,7 @@ import ( "net" "os" "path/filepath" + "runtime" "strings" "syscall" "testing" @@ -240,6 +241,10 @@ func TestKill(t *testing.T) { testutil.FatalOnErr("failed to dial bufnet", err, t) t.Cleanup(func() { conn.Close() }) + if runtime.GOOS == "windows" { + t.Skip("OS not supported") + } + for _, tc := range []struct { name string pid uint64 @@ -590,6 +595,10 @@ func TestMemoryDump(t *testing.T) { testutil.FatalOnErr("failed to dial bufnet", err, t) t.Cleanup(func() { conn.Close() }) + if runtime.GOOS == "windows" { + t.Skip("OS not supported") + } + client := pb.NewProcessClient(conn) // Setup for tests where we use cat and pre-canned data diff --git a/services/util/util_test.go b/services/util/util_test.go index cbb7b5f9..d8af7057 100644 --- a/services/util/util_test.go +++ b/services/util/util_test.go @@ -20,12 +20,17 @@ import ( "bytes" "context" "reflect" + "runtime" "testing" "github.com/Snowflake-Labs/sansshell/testing/testutil" ) func TestRunCommand(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Test has unix-specific assumptions") + } + for _, tc := range []struct { name string bin string @@ -203,6 +208,10 @@ func TestLimitedBuffer(t *testing.T) { } } func TestValidPath(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Test has unix-specific assumptions") + } + for _, tc := range []struct { name string path string From a8404a0c009e69806055fff4769d96141d7eb02b Mon Sep 17 00:00:00 2001 From: Steven Rhodes Date: Mon, 5 Jun 2023 20:14:13 -0700 Subject: [PATCH 3/4] Make error checking more generic --- services/localfile/server/localfile_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/services/localfile/server/localfile_test.go b/services/localfile/server/localfile_test.go index fb841604..a0d0dafb 100644 --- a/services/localfile/server/localfile_test.go +++ b/services/localfile/server/localfile_test.go @@ -415,8 +415,8 @@ func TestSum(t *testing.T) { req: &pb.SumRequest{Filename: temp, SumType: pb.SumType_SUM_TYPE_SHA256}, sendErrFunc: testutil.FatalOnErr, recvErrFunc: func(op string, err error, t *testing.T) { - if err == nil || (!strings.Contains(err.Error(), "directory") && !strings.Contains(err.Error(), "Incorrect")) { - t.Fatalf("%s : err was %v, want err containing 'directory' or 'Incorrect'", op, err) + if err == nil || (!strings.Contains(err.Error(), "directory") && !strings.Contains(err.Error(), "copy/read error")) { + t.Fatalf("%s : err was %v, want err containing 'directory' or 'copy/read error'", op, err) } }, }, From 79014113b18b19b6dafc66f575617d8fdea8c274 Mon Sep 17 00:00:00 2001 From: Steven Rhodes Date: Mon, 5 Jun 2023 21:37:57 -0700 Subject: [PATCH 4/4] Add newline and change chown logic --- .gitattributes | 2 +- services/localfile/server/localfile.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.gitattributes b/.gitattributes index 3d9c9751..88d758d4 100644 --- a/.gitattributes +++ b/.gitattributes @@ -1,3 +1,3 @@ # We checksum this data in tests so we need the content to be # identical across operating systems. -services/localfile/server/testdata/sum.data text eol=lf \ No newline at end of file +services/localfile/server/testdata/sum.data text eol=lf diff --git a/services/localfile/server/localfile.go b/services/localfile/server/localfile.go index 8ca89fa0..b723ebea 100644 --- a/services/localfile/server/localfile.go +++ b/services/localfile/server/localfile.go @@ -634,7 +634,7 @@ func validateAndSetAttrs(filename string, attrs []*pb.FileAttribute, doImmutable } } - if runtime.GOOS != "windows" { + if (uid != -1 || gid != -1) && runtime.GOOS != "windows" { if err := chown(filename, uid, gid); err != nil { return nil, status.Errorf(codes.Internal, "error from chown: %v", err) }