wip

* docker: escape key-value pair as -e KEY and VALUE being environment var

* removed code duplication, removed unused method and test

* add release notes

Co-authored-by: Nikola Jokic <nikola-jokic@github.com>

Co-authored-by: Thomas Boop <52323235+thboop@users.noreply.github.com>
Co-authored-by: Nikola Jokic <nikola-jokic@github.com>

releaseNote.md

@@ -1,9 +1,6 @@
 ## Bugs
-- Avoid key based command injection via Docker command arguments (#2062)
+- Fixed an issue where job and service container envs were corrupted (#2091)
 ## Misc
-- Added step context name and start/finish time in step telemetry (#2069)
-- Improved error logs when there is a missing 'using' token configuration in the metadata file  (#2052)
-- Added full job name and nested workflow details in log (#2049)
 
 ## Windows x64
 We recommend configuring the runner in a root folder of the Windows drive (e.g. "C:\actions-runner"). This will help avoid issues related to service identity folder permissions and long file path restrictions on Windows.

src/Runner.Worker/Container/DockerCommandManager.cs

@@ -107,6 +107,7 @@ namespace GitHub.Runner.Worker.Container
         public async Task<string> DockerCreate(IExecutionContext context, ContainerInfo container)
         {
             IList<string> dockerOptions = new List<string>();
+            IDictionary<string, string> environment = new Dictionary<string, string>();
             // OPTIONS
             dockerOptions.Add($"--name {container.ContainerDisplayName}");
             dockerOptions.Add($"--label {DockerInstanceLabel}");
@@ -135,7 +136,8 @@ namespace GitHub.Runner.Worker.Container
                 }
                 else
                 {
-                    dockerOptions.Add(DockerUtil.CreateEscapedOption("-e", env.Key, env.Value));
+                    environment.Add(env.Key, env.Value);
+                    dockerOptions.Add(DockerUtil.CreateEscapedOption("-e", env.Key));
                 }
             }
 
@@ -183,7 +185,7 @@ namespace GitHub.Runner.Worker.Container
             dockerOptions.Add($"{container.ContainerEntryPointArgs}");
 
             var optionsString = string.Join(" ", dockerOptions);
-            List<string> outputStrings = await ExecuteDockerCommandAsync(context, "create", optionsString);
+            List<string> outputStrings = await ExecuteDockerCommandAsync(context, "create", optionsString, environment);
 
             return outputStrings.FirstOrDefault();
         }
@@ -443,6 +445,11 @@ namespace GitHub.Runner.Worker.Container
         }
 
         private async Task<List<string>> ExecuteDockerCommandAsync(IExecutionContext context, string command, string options)
+        {
+            return await ExecuteDockerCommandAsync(context, command, options, null);
+        }
+
+        private async Task<List<string>> ExecuteDockerCommandAsync(IExecutionContext context, string command, string options, IDictionary<string, string> environment)
         {
             string arg = $"{command} {options}".Trim();
             context.Command($"{DockerPath} {arg}");
@@ -470,7 +477,7 @@ namespace GitHub.Runner.Worker.Container
                             workingDirectory: context.GetGitHubContext("workspace"),
                             fileName: DockerPath,
                             arguments: arg,
-                            environment: null,
+                            environment: environment,
                             requireExitCodeZero: true,
                             outputEncoding: null,
                             cancellationToken: CancellationToken.None);

src/Runner.Worker/Container/DockerUtil.cs

@@ -71,15 +71,6 @@ namespace GitHub.Runner.Worker.Container
             return $"{flag} \"{EscapeString(key)}\"";
         }
 
-        public static string CreateEscapedOption(string flag, string key, string value)
-        {
-            if (String.IsNullOrEmpty(key))
-            {
-                return "";
-            }
-            return $"{flag} \"{EscapeString(key)}={value.Replace("\"", "\\\"")}\"";
-        }
-
         private static string EscapeString(string value)
         {
             return value.Replace("\\", "\\\\").Replace("\"", "\\\"");

src/Runner.Worker/ContainerOperationProvider.cs

@@ -134,7 +134,6 @@ namespace GitHub.Runner.Worker
             ArgUtil.NotNull(executionContext, nameof(executionContext));
             ArgUtil.NotNull(container, nameof(container));
             ArgUtil.NotNullOrEmpty(container.ContainerImage, nameof(container.ContainerImage));
-
             Trace.Info($"Container name: {container.ContainerName}");
             Trace.Info($"Container image: {container.ContainerImage}");
             Trace.Info($"Container options: {container.ContainerCreateOptions}");
@@ -197,6 +196,8 @@ namespace GitHub.Runner.Worker
 
             container.ContainerId = await _dockerManager.DockerCreate(executionContext, container);
             ArgUtil.NotNullOrEmpty(container.ContainerId, nameof(container.ContainerId));
+            
+
 
             // Start container
             int startExitCode = await _dockerManager.DockerStart(executionContext, container.ContainerId);
@@ -207,20 +208,37 @@ namespace GitHub.Runner.Worker
 
             try
             {
+                
                 // Make sure container is up and running
                 var psOutputs = await _dockerManager.DockerPS(executionContext, $"--all --filter id={container.ContainerId} --filter status=running --no-trunc --format \"{{{{.ID}}}} {{{{.Status}}}}\"");
+                
                 if (psOutputs.FirstOrDefault(x => !string.IsNullOrEmpty(x))?.StartsWith(container.ContainerId) != true)
                 {
                     // container is not up and running, pull docker log for this container.
                     await _dockerManager.DockerPS(executionContext, $"--all --filter id={container.ContainerId} --no-trunc --format \"{{{{.ID}}}} {{{{.Status}}}}\"");
+                    
+                    
+                    //executionContext.Output("##[group]Getting docker logs..");
+
                     int logsExitCode = await _dockerManager.DockerLogs(executionContext, container.ContainerId);
+                    
                     if (logsExitCode != 0)
                     {
                         executionContext.Warning($"Docker logs fail with exit code {logsExitCode}");
                     }
-
+                    //executionContext.Output("##[endgroup]");
                     executionContext.Warning($"Docker container {container.ContainerId} is not in running state.");
                 }
+                else {
+                        executionContext.Output($"##[group]Container {container.ContainerId} is not running!");
+                        string opt = "--format=\'{{.State.ExitCode}}\'";
+                        await _dockerManager.DockerInspect(executionContext, container.ContainerId, opt);
+                        await _dockerManager.DockerLogs(executionContext, container.ContainerId);
+                        //if exit code is not 0 we can maybe print a message?
+                        //executionContext.Output("Container exit code: ");
+                
+                    executionContext.Output("##[endgroup]");
+                }
             }
             catch (Exception ex)
             {

src/Runner.Worker/JobExtension.cs

@@ -279,7 +279,7 @@ namespace GitHub.Runner.Worker
 
                         preJobSteps.Add(new JobExtensionRunner(runAsync: containerProvider.StartContainersAsync,
                                                                           condition: $"{PipelineTemplateConstants.Success}()",
-                                                                          displayName: "Initialize containers",
+                                                                          displayName: "**Initialize containers**",
                                                                           data: (object)containers));
                     }
 

src/Test/L0/Container/DockerUtilL0.cs

@@ -171,32 +171,5 @@ namespace GitHub.Runner.Common.Tests.Worker.Container
             }
             Assert.Equal(expected, actual);
         }
-
-        [Theory]
-        [Trait("Level", "L0")]
-        [Trait("Category", "Worker")]
-        [InlineData("HOME", "", "HOME", "")]
-        [InlineData("HOME alpine:3.8 sh -c id #", "HOME alpine:3.8 sh -c id #", "HOME alpine:3.8 sh -c id #", "HOME alpine:3.8 sh -c id #")]
-        [InlineData("HOME \"alpine:3.8 sh -c id #", "HOME \"alpine:3.8 sh -c id #", "HOME \\\"alpine:3.8 sh -c id #", "HOME \\\"alpine:3.8 sh -c id #")]
-        [InlineData("HOME \\\"alpine:3.8 sh -c id #", "HOME \\\"alpine:3.8 sh -c id #", "HOME \\\\\\\"alpine:3.8 sh -c id #", "HOME \\\\\"alpine:3.8 sh -c id #")]
-        [InlineData("HOME \\\\\"alpine:3.8 sh -c id #", "HOME \\\\\"alpine:3.8 sh -c id #", "HOME \\\\\\\\\\\"alpine:3.8 sh -c id #", "HOME \\\\\\\"alpine:3.8 sh -c id #")]
-        [InlineData("HOME \"\"alpine:3.8 sh -c id #", "HOME \"\"alpine:3.8 sh -c id #", "HOME \\\"\\\"alpine:3.8 sh -c id #", "HOME \\\"\\\"alpine:3.8 sh -c id #")]
-        [InlineData("HOME \\\"\"alpine:3.8 sh -c id #", "HOME \\\"\"alpine:3.8 sh -c id #", "HOME \\\\\\\"\\\"alpine:3.8 sh -c id #", "HOME \\\\\"\\\"alpine:3.8 sh -c id #")]
-        [InlineData("HOME \"\\\"alpine:3.8 sh -c id #", "HOME \"\\\"alpine:3.8 sh -c id #", "HOME \\\"\\\\\\\"alpine:3.8 sh -c id #", "HOME \\\"\\\\\"alpine:3.8 sh -c id #")]
-        public void CreateEscapedOption_keyValue(string keyInput, string valueInput, string escapedKey, string escapedValue)
-        {
-            var flag = "--example";
-            var actual = DockerUtil.CreateEscapedOption(flag, keyInput, valueInput);
-            string expected;
-            if (String.IsNullOrEmpty(keyInput))
-            {
-                expected = "";
-            }
-            else
-            {
-                expected = $"{flag} \"{escapedKey}={escapedValue}\"";
-            }
-            Assert.Equal(expected, actual);
-        }
     }
 }

src/runnerversion

@@ -1 +1 @@
-2.296.0
+2.296.1