Merge pull request #6479 from jterry75/jterry75/args_escaped

Adds support for Windows ArgsEscaped images

oci/spec_opts.go

@@ -218,6 +218,7 @@ func WithProcessArgs(args ...string) SpecOpts {
 	return func(_ context.Context, _ Client, _ *containers.Container, s *Spec) error {
 		setProcess(s)
 		s.Process.Args = args
+		s.Process.CommandLine = ""
 		return nil
 	}
 }
@@ -347,17 +348,19 @@ func WithImageConfigArgs(image Image, args []string) SpecOpts {
 			return err
 		}
 		var (
-			ociimage v1.Image
-			config   v1.ImageConfig
+			imageConfigBytes []byte
+			ociimage         v1.Image
+			config           v1.ImageConfig
 		)
 		switch ic.MediaType {
 		case v1.MediaTypeImageConfig, images.MediaTypeDockerSchema2Config:
-			p, err := content.ReadBlob(ctx, image.ContentStore(), ic)
+			var err error
+			imageConfigBytes, err = content.ReadBlob(ctx, image.ContentStore(), ic)
 			if err != nil {
 				return err
 			}
 
-			if err := json.Unmarshal(p, &ociimage); err != nil {
+			if err := json.Unmarshal(imageConfigBytes, &ociimage); err != nil {
 				return err
 			}
 			config = ociimage.Config
@@ -393,12 +396,72 @@ func WithImageConfigArgs(image Image, args []string) SpecOpts {
 			// even if there is no specified user in the image config
 			return WithAdditionalGIDs("root")(ctx, client, c, s)
 		} else if s.Windows != nil {
+			// imageExtended is a superset of the oci Image struct that changes
+			// the Config type to be imageConfigExtended in order to add the
+			// ability to deserialize `ArgsEscaped` which is not an OCI field,
+			// but is supported by Docker built images.
+			type imageExtended struct {
+				Config struct {
+					ArgsEscaped bool `json:"ArgsEscaped,omitempty"`
+				}
+			}
+			// Deserialize the extended image format for Windows.
+			var ociImageExtended imageExtended
+			if err := json.Unmarshal(imageConfigBytes, &ociImageExtended); err != nil {
+				return err
+			}
+			argsEscaped := ociImageExtended.Config.ArgsEscaped
+
 			s.Process.Env = replaceOrAppendEnvValues(config.Env, s.Process.Env)
+
+			// To support Docker ArgsEscaped on Windows we need to combine the
+			// image Entrypoint & (Cmd Or User Args) while taking into account
+			// if Docker has already escaped them in the image config. When
+			// Docker sets `ArgsEscaped==true` in the config it has pre-escaped
+			// either Entrypoint or Cmd or both. Cmd should always be treated as
+			// arguments appended to Entrypoint unless:
+			//
+			// 1. Entrypoint does not exist, in which case Cmd[0] is the
+			// executable.
+			//
+			// 2. The user overrides the Cmd with User Args when activating the
+			// container in which case those args should be appended to the
+			// Entrypoint if it exists.
+			//
+			// To effectively do this we need to know if the arguments came from
+			// the user or if the arguments came from the image config when
+			// ArgsEscaped==true. In this case we only want to escape the
+			// additional user args when forming the complete CommandLine. This
+			// is safe in both cases of Entrypoint or Cmd being set because
+			// Docker will always escape them to an array of length one. Thus in
+			// both cases it is the "executable" portion of the command.
+			//
+			// In the case ArgsEscaped==false, Entrypoint or Cmd will contain
+			// any number of entries that are all unescaped and can simply be
+			// combined (potentially overwriting Cmd with User Args if present)
+			// and forwarded the container start as an Args array.
 			cmd := config.Cmd
+			cmdFromImage := true
 			if len(args) > 0 {
 				cmd = args
+				cmdFromImage = false
+			}
+
+			cmd = append(config.Entrypoint, cmd...)
+			if len(cmd) == 0 {
+				return errors.New("no arguments specified")
+			}
+
+			if argsEscaped && (len(config.Entrypoint) > 0 || cmdFromImage) {
+				s.Process.Args = nil
+				s.Process.CommandLine = cmd[0]
+				if len(cmd) > 1 {
+					s.Process.CommandLine += " " + escapeAndCombineArgs(cmd[1:])
+				}
+			} else {
+				s.Process.Args = cmd
+				s.Process.CommandLine = ""
 			}
-			s.Process.Args = append(config.Entrypoint, cmd...)
 
 			s.Process.Cwd = config.WorkingDir
 			s.Process.User = specs.User{

oci/spec_opts_linux.go

@@ -153,3 +153,7 @@ func WithRdt(closID, l3CacheSchema, memBwSchema string) SpecOpts {
 		return nil
 	}
 }
+
+func escapeAndCombineArgs(args []string) string {
+	panic("not supported")
+}

oci/spec_opts_unix.go

@@ -57,3 +57,7 @@ func WithCPUCFS(quota int64, period uint64) SpecOpts {
 		return nil
 	}
 }
+
+func escapeAndCombineArgs(args []string) string {
+	panic("not supported")
+}

oci/spec_opts_windows.go

@@ -19,9 +19,12 @@ package oci
 import (
 	"context"
 	"errors"
+	"strings"
 
 	"github.com/containerd/containerd/containers"
+
 	specs "github.com/opencontainers/runtime-spec/specs-go"
+	"golang.org/x/sys/windows"
 )
 
 // WithWindowsCPUCount sets the `Windows.Resources.CPU.Count` section to the
@@ -89,3 +92,11 @@ func WithWindowsNetworkNamespace(ns string) SpecOpts {
 		return nil
 	}
 }
+
+func escapeAndCombineArgs(args []string) string {
+	escaped := make([]string, len(args))
+	for i, a := range args {
+		escaped[i] = windows.EscapeArg(a)
+	}
+	return strings.Join(escaped, " ")
+}

oci/spec_opts_windows_test.go

@@ -18,10 +18,15 @@ package oci
 
 import (
 	"context"
+	"encoding/json"
 	"testing"
 
 	"github.com/containerd/containerd/containers"
 	"github.com/containerd/containerd/namespaces"
+
+	"github.com/opencontainers/go-digest"
+	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
+	"github.com/opencontainers/runtime-spec/specs-go"
 )
 
 func TestWithCPUCount(t *testing.T) {
@@ -108,3 +113,436 @@ func TestWithWindowNetworksAllowUnqualifiedDNSQuery(t *testing.T) {
 		}
 	}
 }
+
+func newFakeArgsEscapedImage(config ocispec.ImageConfig) (Image, error) {
+	type imageExtended struct {
+		Config struct {
+			ocispec.ImageConfig
+			ArgsEscaped bool `json:"ArgsEscaped,omitempty"`
+		}
+	}
+
+	// Copy to extended format.
+	configExtended := imageExtended{}
+	configExtended.Config.ImageConfig = config
+	configExtended.Config.ArgsEscaped = true
+
+	configBlob, err := json.Marshal(configExtended)
+	if err != nil {
+		return nil, err
+	}
+	configDescriptor := ocispec.Descriptor{
+		MediaType: ocispec.MediaTypeImageConfig,
+		Digest:    digest.NewDigestFromBytes(digest.SHA256, configBlob),
+	}
+
+	return fakeImage{
+		config: configDescriptor,
+		blobs: map[string]blob{
+			configDescriptor.Digest.String(): configBlob,
+		},
+	}, nil
+}
+
+// TestWithProcessArgsOverwritesWithImage verifies that when calling
+// WithImageConfig followed by WithProcessArgs when `ArgsEscaped==false` that
+// the process args overwrite the image args.
+func TestWithProcessArgsOverwritesWithImage(t *testing.T) {
+	t.Parallel()
+
+	img, err := newFakeImage(ocispec.Image{
+		Config: ocispec.ImageConfig{
+			Entrypoint: []string{"powershell.exe", "-Command", "Write-Host Hello"},
+			Cmd:        []string{"cmd.exe", "/S", "/C", "echo Hello"},
+		},
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	s := Spec{
+		Version: specs.Version,
+		Root:    &specs.Root{},
+		Windows: &specs.Windows{},
+	}
+
+	args := []string{"cmd.exe", "echo", "should be set"}
+	opts := []SpecOpts{
+		WithImageConfig(img),
+		WithProcessArgs(args...),
+	}
+
+	for _, opt := range opts {
+		if err := opt(nil, nil, nil, &s); err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	if err := assertEqualsStringArrays(args, s.Process.Args); err != nil {
+		t.Fatal(err)
+	}
+	if s.Process.CommandLine != "" {
+		t.Fatalf("Expected empty CommandLine, got: '%s'", s.Process.CommandLine)
+	}
+}
+
+// TestWithProcessArgsOverwritesWithImageArgsEscaped verifies that when calling
+// WithImageConfig followed by WithProcessArgs when `ArgsEscaped==true` that the
+// process args overwrite the image args.
+func TestWithProcessArgsOverwritesWithImageArgsEscaped(t *testing.T) {
+	t.Parallel()
+
+	img, err := newFakeArgsEscapedImage(ocispec.ImageConfig{
+		Entrypoint: []string{`powershell.exe -Command "C:\My Data\MyExe.exe" -arg1 "-arg2 value2"`},
+		Cmd:        []string{`cmd.exe /S /C "C:\test path\test.exe"`},
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	s := Spec{
+		Version: specs.Version,
+		Root:    &specs.Root{},
+		Windows: &specs.Windows{},
+	}
+
+	args := []string{"cmd.exe", "echo", "should be set"}
+	opts := []SpecOpts{
+		WithImageConfig(img),
+		WithProcessArgs(args...),
+	}
+
+	for _, opt := range opts {
+		if err := opt(nil, nil, nil, &s); err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	if err := assertEqualsStringArrays(args, s.Process.Args); err != nil {
+		t.Fatal(err)
+	}
+	if s.Process.CommandLine != "" {
+		t.Fatalf("Expected empty CommandLine, got: '%s'", s.Process.CommandLine)
+	}
+}
+
+// TestWithImageOverwritesWithProcessArgs verifies that when calling
+// WithProcessArgs followed by WithImageConfig `ArgsEscaped==false` that the
+// image args overwrites process args.
+func TestWithImageOverwritesWithProcessArgs(t *testing.T) {
+	t.Parallel()
+
+	img, err := newFakeImage(ocispec.Image{
+		Config: ocispec.ImageConfig{
+			Entrypoint: []string{"powershell.exe", "-Command"},
+			Cmd:        []string{"Write-Host", "echo Hello"},
+		},
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	s := Spec{
+		Version: specs.Version,
+		Root:    &specs.Root{},
+		Windows: &specs.Windows{},
+	}
+
+	opts := []SpecOpts{
+		WithProcessArgs("cmd.exe", "echo", "should not be set"),
+		WithImageConfig(img),
+	}
+
+	for _, opt := range opts {
+		if err := opt(nil, nil, nil, &s); err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	expectedArgs := []string{"powershell.exe", "-Command", "Write-Host", "echo Hello"}
+	if err := assertEqualsStringArrays(expectedArgs, s.Process.Args); err != nil {
+		t.Fatal(err)
+	}
+	if s.Process.CommandLine != "" {
+		t.Fatalf("Expected empty CommandLine, got: '%s'", s.Process.CommandLine)
+	}
+}
+
+// TestWithImageOverwritesWithProcessArgs verifies that when calling
+// WithProcessArgs followed by WithImageConfig `ArgsEscaped==true` that the
+// image args overwrites process args.
+func TestWithImageArgsEscapedOverwritesWithProcessArgs(t *testing.T) {
+	t.Parallel()
+
+	img, err := newFakeArgsEscapedImage(ocispec.ImageConfig{
+		Entrypoint: []string{`powershell.exe -Command "C:\My Data\MyExe.exe" -arg1 "-arg2 value2"`},
+		Cmd:        []string{`cmd.exe /S /C "C:\test path\test.exe"`},
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	s := Spec{
+		Version: specs.Version,
+		Root:    &specs.Root{},
+		Windows: &specs.Windows{},
+	}
+
+	opts := []SpecOpts{
+		WithProcessArgs("cmd.exe", "echo", "should not be set"),
+		WithImageConfig(img),
+	}
+
+	expectedCommandLine := `powershell.exe -Command "C:\My Data\MyExe.exe" -arg1 "-arg2 value2" "cmd.exe /S /C \"C:\test path\test.exe\""`
+
+	for _, opt := range opts {
+		if err := opt(nil, nil, nil, &s); err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	if s.Process.Args != nil {
+		t.Fatalf("Expected empty Process.Args, got: '%v'", s.Process.Args)
+	}
+	if expectedCommandLine != s.Process.CommandLine {
+		t.Fatalf("Expected CommandLine '%s', got: '%s'", expectedCommandLine, s.Process.CommandLine)
+	}
+}
+
+func TestWithImageConfigArgsWindows(t *testing.T) {
+	testcases := []struct {
+		name       string
+		entrypoint []string
+		cmd        []string
+		args       []string
+
+		expectError bool
+		// When ArgsEscaped==false we always expect args and CommandLine==""
+		expectedArgs []string
+	}{
+		{
+			// This is not really a valid test case since Docker would have made
+			// the default cmd to be the shell. So just verify it hits the error
+			// case we expect.
+			name:        "EmptyEntrypoint_EmptyCmd_EmptyArgs",
+			entrypoint:  nil,
+			cmd:         nil,
+			args:        nil,
+			expectError: true,
+		},
+		{
+			name:         "EmptyEntrypoint_EmptyCmd_Args",
+			entrypoint:   nil,
+			cmd:          nil,
+			args:         []string{"additional", "args"},
+			expectedArgs: []string{"additional", "args"},
+		},
+		{
+			name:         "EmptyEntrypoint_Cmd_EmptyArgs",
+			entrypoint:   nil,
+			cmd:          []string{"cmd", "args"},
+			args:         nil,
+			expectedArgs: []string{"cmd", "args"},
+		},
+		{
+			name:         "EmptyEntrypoint_Cmd_Args",
+			entrypoint:   nil,
+			cmd:          []string{"cmd", "args"},
+			args:         []string{"additional", "args"},
+			expectedArgs: []string{"additional", "args"}, // Args overwrite Cmd
+		},
+		{
+			name:         "Entrypoint_EmptyCmd_EmptyArgs",
+			entrypoint:   []string{"entrypoint", "args"},
+			cmd:          nil,
+			args:         nil,
+			expectedArgs: []string{"entrypoint", "args"},
+		},
+		{
+			name:         "Entrypoint_EmptyCmd_Args",
+			entrypoint:   []string{"entrypoint", "args"},
+			cmd:          nil,
+			args:         []string{"additional", "args"},
+			expectedArgs: []string{"entrypoint", "args", "additional", "args"},
+		},
+		{
+			name:         "Entrypoint_Cmd_EmptyArgs",
+			entrypoint:   []string{"entrypoint", "args"},
+			cmd:          []string{"cmd", "args"},
+			args:         nil,
+			expectedArgs: []string{"entrypoint", "args", "cmd", "args"},
+		},
+		{
+			name:         "Entrypoint_Cmd_Args",
+			entrypoint:   []string{"entrypoint", "args"},
+			cmd:          []string{"cmd", "args"},
+			args:         []string{"additional", "args"}, // Args overwrites Cmd
+			expectedArgs: []string{"entrypoint", "args", "additional", "args"},
+		},
+	}
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			img, err := newFakeImage(ocispec.Image{
+				Config: ocispec.ImageConfig{
+					Entrypoint: tc.entrypoint,
+					Cmd:        tc.cmd,
+				},
+			})
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			s := Spec{
+				Version: specs.Version,
+				Root:    &specs.Root{},
+				Windows: &specs.Windows{},
+			}
+
+			opts := []SpecOpts{
+				WithImageConfigArgs(img, tc.args),
+			}
+
+			for _, opt := range opts {
+				if err := opt(nil, nil, nil, &s); err != nil {
+					if tc.expectError {
+						continue
+					}
+					t.Fatal(err)
+				}
+			}
+
+			if err := assertEqualsStringArrays(tc.expectedArgs, s.Process.Args); err != nil {
+				t.Fatal(err)
+			}
+			if s.Process.CommandLine != "" {
+				t.Fatalf("Expected empty CommandLine, got: '%s'", s.Process.CommandLine)
+			}
+		})
+	}
+}
+
+func TestWithImageConfigArgsEscapedWindows(t *testing.T) {
+	testcases := []struct {
+		name       string
+		entrypoint []string
+		cmd        []string
+		args       []string
+
+		expectError         bool
+		expectedArgs        []string
+		expectedCommandLine string
+	}{
+		{
+			// This is not really a valid test case since Docker would have made
+			// the default cmd to be the shell. So just verify it hits the error
+			// case we expect.
+			name:                "EmptyEntrypoint_EmptyCmd_EmptyArgs",
+			entrypoint:          nil,
+			cmd:                 nil,
+			args:                nil,
+			expectError:         true,
+			expectedArgs:        nil,
+			expectedCommandLine: "",
+		},
+		{
+			// This case is special for ArgsEscaped, since there is no Image
+			// Default Args should be passed as ProcessArgs not as Cmdline
+			name:                "EmptyEntrypoint_EmptyCmd_Args",
+			entrypoint:          nil,
+			cmd:                 nil,
+			args:                []string{"additional", "-args", "hello world"},
+			expectedArgs:        []string{"additional", "-args", "hello world"},
+			expectedCommandLine: "",
+		},
+		{
+			name:                "EmptyEntrypoint_Cmd_EmptyArgs",
+			entrypoint:          nil,
+			cmd:                 []string{`cmd -args "hello world"`},
+			args:                nil,
+			expectedCommandLine: `cmd -args "hello world"`,
+		},
+		{
+			// This case is a second special case for ArgsEscaped, since Args
+			// overwrite Cmd the args are not from the image, so ArgsEscaped
+			// should be ignored, and passed as Args not CommandLine.
+			name:                "EmptyEntrypoint_Cmd_Args",
+			entrypoint:          nil,
+			cmd:                 []string{`cmd -args "hello world"`},
+			args:                []string{"additional", "args"},
+			expectedArgs:        []string{"additional", "args"}, // Args overwrite Cmd
+			expectedCommandLine: "",
+		},
+		{
+			name:                "Entrypoint_EmptyCmd_EmptyArgs",
+			entrypoint:          []string{`"C:\My Folder\MyProcess.exe" -arg1 "test value"`},
+			cmd:                 nil,
+			args:                nil,
+			expectedCommandLine: `"C:\My Folder\MyProcess.exe" -arg1 "test value"`,
+		},
+		{
+			name:                "Entrypoint_EmptyCmd_Args",
+			entrypoint:          []string{`"C:\My Folder\MyProcess.exe" -arg1 "test value"`},
+			cmd:                 nil,
+			args:                []string{"additional", "args with spaces"},
+			expectedCommandLine: `"C:\My Folder\MyProcess.exe" -arg1 "test value" additional "args with spaces"`,
+		},
+		{
+			// This case will not work in Docker today so adding the test to
+			// confirm we fail in the same way. Although the appending of
+			// Entrypoint + " " + Cmd here works, Cmd is double escaped and the
+			// container would not launch. This is because when Docker built
+			// such an image it escaped both Entrypoint and Cmd. However the
+			// docs say that CMD should always be appened to entrypoint if not
+			// overwritten so this results in an incorrect cmdline.
+			name:                "Entrypoint_Cmd_EmptyArgs",
+			entrypoint:          []string{`"C:\My Folder\MyProcess.exe" -arg1 "test value"`},
+			cmd:                 []string{`cmd -args "hello world"`},
+			args:                nil,
+			expectedCommandLine: `"C:\My Folder\MyProcess.exe" -arg1 "test value" "cmd -args \"hello world\""`,
+		},
+		{
+			name:                "Entrypoint_Cmd_Args",
+			entrypoint:          []string{`"C:\My Folder\MyProcess.exe" -arg1 "test value"`},
+			cmd:                 []string{`cmd -args "hello world"`},
+			args:                []string{"additional", "args with spaces"}, // Args overwrites Cmd
+			expectedCommandLine: `"C:\My Folder\MyProcess.exe" -arg1 "test value" additional "args with spaces"`,
+		},
+	}
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			img, err := newFakeArgsEscapedImage(ocispec.ImageConfig{
+				Entrypoint: tc.entrypoint,
+				Cmd:        tc.cmd,
+			})
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			s := Spec{
+				Version: specs.Version,
+				Root:    &specs.Root{},
+				Windows: &specs.Windows{},
+			}
+
+			opts := []SpecOpts{
+				WithImageConfigArgs(img, tc.args),
+			}
+
+			for _, opt := range opts {
+				if err := opt(nil, nil, nil, &s); err != nil {
+					if tc.expectError {
+						continue
+					}
+					t.Fatal(err)
+				}
+			}
+
+			if err := assertEqualsStringArrays(tc.expectedArgs, s.Process.Args); err != nil {
+				t.Fatal(err)
+			}
+			if tc.expectedCommandLine != s.Process.CommandLine {
+				t.Fatalf("Expected CommandLine: '%s', got: '%s'", tc.expectedCommandLine, s.Process.CommandLine)
+			}
+		})
+	}
+}