diff --git a/oci/spec_opts.go b/oci/spec_opts.go index 9b0cfc3f1..cd6b2ec68 100644 --- a/oci/spec_opts.go +++ b/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{ diff --git a/oci/spec_opts_linux.go b/oci/spec_opts_linux.go index 4d8841ee1..90c4887a4 100644 --- a/oci/spec_opts_linux.go +++ b/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") +} diff --git a/oci/spec_opts_unix.go b/oci/spec_opts_unix.go index 9d03091aa..a6165777f 100644 --- a/oci/spec_opts_unix.go +++ b/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") +} diff --git a/oci/spec_opts_windows.go b/oci/spec_opts_windows.go index 5502257a4..4ddb13d3f 100644 --- a/oci/spec_opts_windows.go +++ b/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, " ") +} diff --git a/oci/spec_opts_windows_test.go b/oci/spec_opts_windows_test.go index aa3fb1474..eea6aacb8 100644 --- a/oci/spec_opts_windows_test.go +++ b/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) + } + }) + } +}