From 22dc60e05922e7ea0f0ae08a6e9bf3e41671bd75 Mon Sep 17 00:00:00 2001 From: Gabriel Adrian Samfira Date: Fri, 19 Nov 2021 17:11:24 +0200 Subject: [PATCH 1/2] Enable TestVolumeOwnership on Windows This change enables the TestVolumeOwnership on Windows. The test assumes that the volume-ownership image is built on Windows, thus ensuring that Windows file security info (ACLs and ownership info) are attached to the C:\volumes\test_dir path. Signed-off-by: Gabriel Adrian Samfira --- integration/volume_copy_up_test.go | 39 ++++++++++---- integration/volume_copy_up_unix_test.go | 35 +++++++++++++ integration/volume_copy_up_windows_test.go | 59 ++++++++++++++++++++++ 3 files changed, 123 insertions(+), 10 deletions(-) create mode 100644 integration/volume_copy_up_unix_test.go create mode 100644 integration/volume_copy_up_windows_test.go diff --git a/integration/volume_copy_up_test.go b/integration/volume_copy_up_test.go index b04a9be37..07023c3b0 100644 --- a/integration/volume_copy_up_test.go +++ b/integration/volume_copy_up_test.go @@ -27,6 +27,13 @@ import ( exec "golang.org/x/sys/execabs" ) +const ( + containerUserName = "ContainerUser" + // containerUserSID is a well known SID that is set on the + // ContainerUser username inside a Windows container. + containerUserSID = "S-1-5-93-2-2" +) + func TestVolumeCopyUp(t *testing.T) { var ( testImage = GetImage(VolumeCopyUp) @@ -84,9 +91,6 @@ func TestVolumeCopyUp(t *testing.T) { } func TestVolumeOwnership(t *testing.T) { - if goruntime.GOOS == "windows" { - t.Skip("Skipped on Windows.") - } var ( testImage = GetImage(VolumeOwnership) execTimeout = time.Minute @@ -101,7 +105,7 @@ func TestVolumeOwnership(t *testing.T) { cnConfig := ContainerConfig( "container", testImage, - WithCommand("tail", "-f", "/dev/null"), + WithCommand("sleep", "150"), ) cn, err := runtimeService.CreateContainer(sb, cnConfig, sbConfig) require.NoError(t, err) @@ -111,17 +115,32 @@ func TestVolumeOwnership(t *testing.T) { // ghcr.io/containerd/volume-ownership:2.1 contains a test_dir // volume, which is owned by nobody:nogroup. + // On Windows, the folder is situated in C:\volumes\test_dir and is owned + // by ContainerUser (SID: S-1-5-93-2-2). A helper tool get_owner.exe should + // exist inside the container that returns the owner in the form of USERNAME:SID. t.Logf("Check ownership of test directory inside container") - stdout, stderr, err := runtimeService.ExecSync(cn, []string{ + + cmd := []string{ "stat", "-c", "%U:%G", "/test_dir", - }, execTimeout) + } + expectedContainerOutput := "nobody:nogroup\n" + expectedHostOutput := "nobody:nogroup\n" + if goruntime.GOOS == "windows" { + cmd = []string{ + "C:\\bin\\get_owner.exe", + "C:\\volumes\\test_dir", + } + expectedContainerOutput = fmt.Sprintf("%s:%s", containerUserName, containerUserSID) + // The username is unknown on the host, but we can still get the SID. + expectedHostOutput = containerUserSID + } + stdout, stderr, err := runtimeService.ExecSync(cn, cmd, execTimeout) require.NoError(t, err) assert.Empty(t, stderr) - assert.Equal(t, "nobody:nogroup\n", string(stdout)) + assert.Equal(t, expectedContainerOutput, string(stdout)) t.Logf("Check ownership of test directory on the host") - hostCmd := fmt.Sprintf("find %s/containers/%s/volumes/* | xargs stat -c %%U:%%G", *criRoot, cn) - output, err := exec.Command("sh", "-c", hostCmd).CombinedOutput() + output, err := getVolumeHostPathOwnership(*criRoot, cn) require.NoError(t, err) - assert.Equal(t, "nobody:nogroup\n", string(output)) + assert.Equal(t, expectedHostOutput, output) } diff --git a/integration/volume_copy_up_unix_test.go b/integration/volume_copy_up_unix_test.go new file mode 100644 index 000000000..e191aaa2d --- /dev/null +++ b/integration/volume_copy_up_unix_test.go @@ -0,0 +1,35 @@ +//go:build !windows +// +build !windows + +/* + Copyright The containerd Authors. + + 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 integration + +import ( + "fmt" + + exec "golang.org/x/sys/execabs" +) + +func getVolumeHostPathOwnership(criRoot, containerID string) (string, error) { + hostCmd := fmt.Sprintf("find %s/containers/%s/volumes/* | xargs stat -c %%U:%%G", criRoot, containerID) + output, err := exec.Command("sh", "-c", hostCmd).CombinedOutput() + if err != nil { + return "", err + } + return string(output), nil +} diff --git a/integration/volume_copy_up_windows_test.go b/integration/volume_copy_up_windows_test.go new file mode 100644 index 000000000..f879b53aa --- /dev/null +++ b/integration/volume_copy_up_windows_test.go @@ -0,0 +1,59 @@ +//go:build windows +// +build windows + +/* + Copyright The containerd Authors. + + 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 integration + +import ( + "fmt" + "io/ioutil" + "os" + "path/filepath" + + "golang.org/x/sys/windows" +) + +func getVolumeHostPathOwnership(criRoot, containerID string) (string, error) { + hostPath := fmt.Sprintf("%s/containers/%s/volumes/", criRoot, containerID) + if _, err := os.Stat(hostPath); err != nil { + return "", err + } + + volumes, err := ioutil.ReadDir(hostPath) + if err != nil { + return "", err + } + + if len(volumes) != 1 { + return "", fmt.Errorf("expected to find exactly 1 volume (got %d)", len(volumes)) + } + + secInfo, err := windows.GetNamedSecurityInfo( + filepath.Join(hostPath, volumes[0].Name()), windows.SE_FILE_OBJECT, + windows.OWNER_SECURITY_INFORMATION|windows.DACL_SECURITY_INFORMATION) + + if err != nil { + return "", err + } + + sid, _, err := secInfo.Owner() + if err != nil { + return "", err + } + return sid.String(), nil +} From 77a321a073fc6f48f8a30ebfd1194306cc25f22b Mon Sep 17 00:00:00 2001 From: Gabriel Adrian Samfira Date: Tue, 7 Dec 2021 10:46:25 +0000 Subject: [PATCH 2/2] Replace find with native Go code Signed-off-by: Gabriel Adrian Samfira --- integration/volume_copy_up_test.go | 49 ++++++++++++++++++---- integration/volume_copy_up_unix_test.go | 4 +- integration/volume_copy_up_windows_test.go | 23 +--------- 3 files changed, 44 insertions(+), 32 deletions(-) diff --git a/integration/volume_copy_up_test.go b/integration/volume_copy_up_test.go index 07023c3b0..664f4de35 100644 --- a/integration/volume_copy_up_test.go +++ b/integration/volume_copy_up_test.go @@ -18,13 +18,15 @@ package integration import ( "fmt" + "io/ioutil" + "os" + "path/filepath" goruntime "runtime" "testing" "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - exec "golang.org/x/sys/execabs" ) const ( @@ -69,12 +71,14 @@ func TestVolumeCopyUp(t *testing.T) { assert.Equal(t, "test_content\n", string(stdout)) t.Logf("Check host path of the volume") - // Windows paths might have spaces in them (e.g.: Program Files), which would - // cause issues for this command. This will allow us to bypass them. - hostCmd := fmt.Sprintf("find '%s/containers/%s/volumes/' -type f -print0 | xargs -0 cat", *criRoot, cn) - output, err := exec.Command("sh", "-c", hostCmd).CombinedOutput() + volumePaths, err := getHostPathForVolumes(*criRoot, cn) require.NoError(t, err) - assert.Equal(t, "test_content\n", string(output)) + assert.Equal(t, len(volumePaths), 1, "expected exactly 1 volume") + + testFilePath := filepath.Join(volumePaths[0], "test_file") + contents, err := ioutil.ReadFile(testFilePath) + require.NoError(t, err) + assert.Equal(t, "test_content\n", string(contents)) t.Logf("Update volume from inside the container") _, _, err = runtimeService.ExecSync(cn, []string{ @@ -85,9 +89,9 @@ func TestVolumeCopyUp(t *testing.T) { require.NoError(t, err) t.Logf("Check whether host path of the volume is updated") - output, err = exec.Command("sh", "-c", hostCmd).CombinedOutput() + contents, err = ioutil.ReadFile(testFilePath) require.NoError(t, err) - assert.Equal(t, "new_content\n", string(output)) + assert.Equal(t, "new_content\n", string(contents)) } func TestVolumeOwnership(t *testing.T) { @@ -140,7 +144,34 @@ func TestVolumeOwnership(t *testing.T) { assert.Equal(t, expectedContainerOutput, string(stdout)) t.Logf("Check ownership of test directory on the host") - output, err := getVolumeHostPathOwnership(*criRoot, cn) + volumePaths, err := getHostPathForVolumes(*criRoot, cn) + require.NoError(t, err) + assert.Equal(t, len(volumePaths), 1, "expected exactly 1 volume") + + output, err := getOwnership(volumePaths[0]) require.NoError(t, err) assert.Equal(t, expectedHostOutput, output) } + +func getHostPathForVolumes(criRoot, containerID string) ([]string, error) { + hostPath := filepath.Join(criRoot, "containers", containerID, "volumes") + if _, err := os.Stat(hostPath); err != nil { + return nil, err + } + + volumes, err := ioutil.ReadDir(hostPath) + if err != nil { + return nil, err + } + + if len(volumes) == 0 { + return []string{}, nil + } + + volumePaths := make([]string, len(volumes)) + for idx, volume := range volumes { + volumePaths[idx] = filepath.Join(hostPath, volume.Name()) + } + + return volumePaths, nil +} diff --git a/integration/volume_copy_up_unix_test.go b/integration/volume_copy_up_unix_test.go index e191aaa2d..1dc76abc6 100644 --- a/integration/volume_copy_up_unix_test.go +++ b/integration/volume_copy_up_unix_test.go @@ -25,8 +25,8 @@ import ( exec "golang.org/x/sys/execabs" ) -func getVolumeHostPathOwnership(criRoot, containerID string) (string, error) { - hostCmd := fmt.Sprintf("find %s/containers/%s/volumes/* | xargs stat -c %%U:%%G", criRoot, containerID) +func getOwnership(path string) (string, error) { + hostCmd := fmt.Sprintf("stat -c %%U:%%G '%s'", path) output, err := exec.Command("sh", "-c", hostCmd).CombinedOutput() if err != nil { return "", err diff --git a/integration/volume_copy_up_windows_test.go b/integration/volume_copy_up_windows_test.go index f879b53aa..3ea5b2b18 100644 --- a/integration/volume_copy_up_windows_test.go +++ b/integration/volume_copy_up_windows_test.go @@ -20,31 +20,12 @@ package integration import ( - "fmt" - "io/ioutil" - "os" - "path/filepath" - "golang.org/x/sys/windows" ) -func getVolumeHostPathOwnership(criRoot, containerID string) (string, error) { - hostPath := fmt.Sprintf("%s/containers/%s/volumes/", criRoot, containerID) - if _, err := os.Stat(hostPath); err != nil { - return "", err - } - - volumes, err := ioutil.ReadDir(hostPath) - if err != nil { - return "", err - } - - if len(volumes) != 1 { - return "", fmt.Errorf("expected to find exactly 1 volume (got %d)", len(volumes)) - } - +func getOwnership(path string) (string, error) { secInfo, err := windows.GetNamedSecurityInfo( - filepath.Join(hostPath, volumes[0].Name()), windows.SE_FILE_OBJECT, + path, windows.SE_FILE_OBJECT, windows.OWNER_SECURITY_INFORMATION|windows.DACL_SECURITY_INFORMATION) if err != nil {