From 4916edde4de96b59217a5e92af9c147773639e53 Mon Sep 17 00:00:00 2001 From: Romuald Conty Date: Mon, 28 Mar 2022 19:38:47 +0200 Subject: [PATCH 1/7] docker_run_flags: Shellescape any provided values --- lib/puppet/parser/functions/docker_run_flags.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/puppet/parser/functions/docker_run_flags.rb b/lib/puppet/parser/functions/docker_run_flags.rb index 64379ae7..aa0b6d5e 100644 --- a/lib/puppet/parser/functions/docker_run_flags.rb +++ b/lib/puppet/parser/functions/docker_run_flags.rb @@ -72,7 +72,7 @@ module Puppet::Parser::Functions multi_flags = ->(values, fmt) { filtered = [values].flatten.compact - filtered.map { |val| (fmt + params_join_char) % val } + filtered.map { |val| (fmt + params_join_char) % val.shellescape } } [ @@ -80,9 +80,9 @@ module Puppet::Parser::Functions ['--dns-search %s', 'dns_search'], ['--expose=%s', 'expose'], ['--link %s', 'links'], - ['--lxc-conf="%s"', 'lxc_conf'], + ['--lxc-conf=%s', 'lxc_conf'], ['--volumes-from %s', 'volumes_from'], - ['-e "%s"', 'env'], + ['-e %s', 'env'], ['--env-file %s', 'env_file'], ['-p %s', 'ports'], ['-l %s', 'labels'], From c173a3c97da6ca16afb884174e1530ce3a236058 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romain=20Tarti=C3=A8re?= Date: Tue, 8 Nov 2022 11:23:15 +0000 Subject: [PATCH 2/7] Add unit test for new behavior --- .../parser/functions/docker_run_flags_spec.rb | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 spec/unit/lib/puppet/parser/functions/docker_run_flags_spec.rb diff --git a/spec/unit/lib/puppet/parser/functions/docker_run_flags_spec.rb b/spec/unit/lib/puppet/parser/functions/docker_run_flags_spec.rb new file mode 100644 index 00000000..6eb342c9 --- /dev/null +++ b/spec/unit/lib/puppet/parser/functions/docker_run_flags_spec.rb @@ -0,0 +1,15 @@ +require 'spec_helper' + +describe 'the "docker_run_flags" parser function' do + let :scope do + node = Puppet::Node.new('localhost') + compiler = Puppet::Parser::Compiler.new(node) + scope = Puppet::Parser::Scope.new(compiler) + allow(scope).to receive(:environment).and_return(nil) + scope + end + + it 'env with special chars' do + expect(scope.function_docker_run_flags([{ 'env' => [%.MYSQL_PASSWORD='"$()[]{}<>.], 'extra_params' => [] }])).to match(/^-e MYSQL_PASSWORD\\=\\'\\"\\\$\\\(\\\)\\\[\\\]\\\{\\\}\\<\\> \\$/) + end +end From b5082c272cad3c47b15e77f139d2407f6d7178a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romain=20Tarti=C3=A8re?= Date: Tue, 8 Nov 2022 11:23:16 +0000 Subject: [PATCH 3/7] Attempt to escape things in on OS dependant way Ruby Shellwords#shellescape "Escapes a string so that it can be safely used in a Bourne shell command line". This is fine for a UNIX like operating system, but windows does not complies with this. Attempt to fix CI by manually "escaping" strings on windows (i.e. quote them and escape quotes with a backslash), and relying on shellescape for other operating systems. --- .../parser/functions/docker_run_flags.rb | 20 ++++++++++++++----- .../parser/functions/docker_run_flags_spec.rb | 17 ++++++++++++++-- 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/lib/puppet/parser/functions/docker_run_flags.rb b/lib/puppet/parser/functions/docker_run_flags.rb index aa0b6d5e..00086813 100644 --- a/lib/puppet/parser/functions/docker_run_flags.rb +++ b/lib/puppet/parser/functions/docker_run_flags.rb @@ -5,17 +5,27 @@ # docker_run_flags.rb # module Puppet::Parser::Functions + newfunction(:'docker::escape', type: :rvalue) do |args| + subject = args[0] + + if self['facts'] && self['facts']['os']['family'] == 'windows' + %("#{subject.gsub('"', '\\"')}") + else + subject.shellescape + end + end + # Transforms a hash into a string of docker flags newfunction(:docker_run_flags, type: :rvalue) do |args| opts = args[0] || {} flags = [] if opts['username'] - flags << "-u '#{opts['username'].shellescape}'" + flags << "-u '#{call_function('docker::escape', [opts['username']])}'" end if opts['hostname'] - flags << "-h '#{opts['hostname'].shellescape}'" + flags << "-h '#{call_function('docker::escape', [opts['hostname']])}'" end if opts['restart'] @@ -24,9 +34,9 @@ module Puppet::Parser::Functions if opts['net'] if opts['net'].is_a? String - flags << "--net #{opts['net'].shellescape}" + flags << "--net #{call_function('docker::escape', [opts['net']])}" elsif opts['net'].is_a? Array - flags << "--net #{opts['net'].join(' --net ').shellescape}" + flags << "--net #{call_function('docker::escape', [opts['net'].join(' --net ')])}" # FIXME: escaping is buggy end end @@ -72,7 +82,7 @@ module Puppet::Parser::Functions multi_flags = ->(values, fmt) { filtered = [values].flatten.compact - filtered.map { |val| (fmt + params_join_char) % val.shellescape } + filtered.map { |val| (fmt + params_join_char) % call_function('docker::escape', [val]) } } [ diff --git a/spec/unit/lib/puppet/parser/functions/docker_run_flags_spec.rb b/spec/unit/lib/puppet/parser/functions/docker_run_flags_spec.rb index 6eb342c9..53ca218d 100644 --- a/spec/unit/lib/puppet/parser/functions/docker_run_flags_spec.rb +++ b/spec/unit/lib/puppet/parser/functions/docker_run_flags_spec.rb @@ -6,10 +6,23 @@ compiler = Puppet::Parser::Compiler.new(node) scope = Puppet::Parser::Scope.new(compiler) allow(scope).to receive(:environment).and_return(nil) + allow(scope).to receive(:[]).with('facts').and_return({ 'os' => { 'family' => os_family } }) scope end - it 'env with special chars' do - expect(scope.function_docker_run_flags([{ 'env' => [%.MYSQL_PASSWORD='"$()[]{}<>.], 'extra_params' => [] }])).to match(/^-e MYSQL_PASSWORD\\=\\'\\"\\\$\\\(\\\)\\\[\\\]\\\{\\\}\\<\\> \\$/) + context 'on POSIX system' do + let(:os_family) { 'Linux' } + + it 'escapes special chars' do + expect(scope.function_docker_run_flags([{ 'env' => [%.MYSQL_PASSWORD='"$()[]{}<>.], 'extra_params' => [] }])).to eq(%(-e MYSQL_PASSWORD\\=\\'\\"\\$\\(\\)\\[\\]\\{\\}\\<\\> \\\n)) + end + end + + context 'on windows' do + let(:os_family) { 'windows' } + + it 'escapes special chars' do + expect(scope.function_docker_run_flags([{ 'env' => [%.MYSQL_PASSWORD='"$()[]{}<>.], 'extra_params' => [] }])).to eq(%^-e "MYSQL_PASSWORD='\\"$()[]{}<>" \\\n^) + end end end From 3dd5a52d9198dc242a0c90db150c6f375a0ab9d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romain=20Tarti=C3=A8re?= Date: Tue, 8 Nov 2022 11:23:16 +0000 Subject: [PATCH 4/7] Rely on latest stdlib for proper powershell escaping --- lib/puppet/parser/functions/docker_run_flags.rb | 2 +- metadata.json | 2 +- .../lib/puppet/parser/functions/docker_run_flags_spec.rb | 6 +++++- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/puppet/parser/functions/docker_run_flags.rb b/lib/puppet/parser/functions/docker_run_flags.rb index 00086813..962cf4db 100644 --- a/lib/puppet/parser/functions/docker_run_flags.rb +++ b/lib/puppet/parser/functions/docker_run_flags.rb @@ -9,7 +9,7 @@ module Puppet::Parser::Functions subject = args[0] if self['facts'] && self['facts']['os']['family'] == 'windows' - %("#{subject.gsub('"', '\\"')}") + call_function('powershell_escape', subject) else subject.shellescape end diff --git a/metadata.json b/metadata.json index d30b48fd..b4aac919 100644 --- a/metadata.json +++ b/metadata.json @@ -10,7 +10,7 @@ "dependencies": [ { "name": "puppetlabs/stdlib", - "version_requirement": ">= 4.24.0 < 9.0.0" + "version_requirement": ">= 8.2.0 < 9.0.0" }, { "name": "puppetlabs/apt", diff --git a/spec/unit/lib/puppet/parser/functions/docker_run_flags_spec.rb b/spec/unit/lib/puppet/parser/functions/docker_run_flags_spec.rb index 53ca218d..157a98ba 100644 --- a/spec/unit/lib/puppet/parser/functions/docker_run_flags_spec.rb +++ b/spec/unit/lib/puppet/parser/functions/docker_run_flags_spec.rb @@ -1,6 +1,10 @@ require 'spec_helper' describe 'the "docker_run_flags" parser function' do + before :each do + Puppet[:modulepath] = 'spec/fixtures/modules' + end + let :scope do node = Puppet::Node.new('localhost') compiler = Puppet::Parser::Compiler.new(node) @@ -22,7 +26,7 @@ let(:os_family) { 'windows' } it 'escapes special chars' do - expect(scope.function_docker_run_flags([{ 'env' => [%.MYSQL_PASSWORD='"$()[]{}<>.], 'extra_params' => [] }])).to eq(%^-e "MYSQL_PASSWORD='\\"$()[]{}<>" \\\n^) + expect(scope.function_docker_run_flags([{ 'env' => [%.MYSQL_PASSWORD='"$()[]{}<>.], 'extra_params' => [] }])).to eq(%^-e MYSQL_PASSWORD=`'\\`"`$()[]{}<> \\\n^) end end end From 3d5d64572cfd1ddd04b4b7c2eefef91a4b387914 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romain=20Tarti=C3=A8re?= Date: Tue, 8 Nov 2022 11:23:16 +0000 Subject: [PATCH 5/7] Always pass subject to stdlib escape function At the cost of one more function call, we gain in readability and what the docker::escape functions is more straightforward. --- lib/puppet/parser/functions/docker_run_flags.rb | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/lib/puppet/parser/functions/docker_run_flags.rb b/lib/puppet/parser/functions/docker_run_flags.rb index 962cf4db..f14f6a63 100644 --- a/lib/puppet/parser/functions/docker_run_flags.rb +++ b/lib/puppet/parser/functions/docker_run_flags.rb @@ -1,6 +1,5 @@ # frozen_string_literal: true -require 'shellwords' # # docker_run_flags.rb # @@ -8,11 +7,13 @@ module Puppet::Parser::Functions newfunction(:'docker::escape', type: :rvalue) do |args| subject = args[0] - if self['facts'] && self['facts']['os']['family'] == 'windows' - call_function('powershell_escape', subject) - else - subject.shellescape - end + escape_function = if self['facts'] && self['facts']['os']['family'] == 'windows' + 'powershell_escape' + else + 'shell_escape' + end + + call_function(escape_function, subject) end # Transforms a hash into a string of docker flags From e8482839f487152f9c90931b1498a7af49ab7159 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romain=20Tarti=C3=A8re?= Date: Tue, 8 Nov 2022 11:23:16 +0000 Subject: [PATCH 6/7] Fix escaped string usage Escaped strings should not be quotted. While here, also rework the way networks are escaped: the previous code was generating invalid parameters. --- lib/puppet/parser/functions/docker_run_flags.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/puppet/parser/functions/docker_run_flags.rb b/lib/puppet/parser/functions/docker_run_flags.rb index f14f6a63..84db5d92 100644 --- a/lib/puppet/parser/functions/docker_run_flags.rb +++ b/lib/puppet/parser/functions/docker_run_flags.rb @@ -22,11 +22,11 @@ module Puppet::Parser::Functions flags = [] if opts['username'] - flags << "-u '#{call_function('docker::escape', [opts['username']])}'" + flags << "-u #{call_function('docker::escape', [opts['username']])}" end if opts['hostname'] - flags << "-h '#{call_function('docker::escape', [opts['hostname']])}'" + flags << "-h #{call_function('docker::escape', [opts['hostname']])}" end if opts['restart'] @@ -37,7 +37,7 @@ module Puppet::Parser::Functions if opts['net'].is_a? String flags << "--net #{call_function('docker::escape', [opts['net']])}" elsif opts['net'].is_a? Array - flags << "--net #{call_function('docker::escape', [opts['net'].join(' --net ')])}" # FIXME: escaping is buggy + flags += opts['net'].map { |item| ["--net #{call_function('docker::escape', [item])}"] } end end From 3f09d92a3eba0d1f3e5eea0818440dfcd14c708c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romain=20Tarti=C3=A8re?= Date: Tue, 8 Nov 2022 11:23:16 +0000 Subject: [PATCH 7/7] Avoid mixing Linux and Windows path separator --- spec/acceptance/docker_params_changed_spec.rb | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/spec/acceptance/docker_params_changed_spec.rb b/spec/acceptance/docker_params_changed_spec.rb index cb0833e1..71b4bbe4 100644 --- a/spec/acceptance/docker_params_changed_spec.rb +++ b/spec/acceptance/docker_params_changed_spec.rb @@ -23,7 +23,7 @@ else docker_args = '' docker_network = 'bridge' - volume_location = '/opt' + volume_location = '/opt/' docker_image = 'hello-world:linux' end @@ -33,8 +33,8 @@ install_pp = "class { 'docker': #{docker_args}}" apply_manifest(install_pp) end - run_shell("mkdir #{volume_location}/volume_1") - run_shell("mkdir #{volume_location}/volume_2") + run_shell("mkdir #{volume_location}volume_1") + run_shell("mkdir #{volume_location}volume_2") end context 'when image is changed' do @@ -78,8 +78,8 @@ class {'docker': #{docker_args}} volumes1 = "volumes => ['volume-1:C:\\volume_1']" volumes2 = "volumes => ['volume-1:C:\\volume_1', 'volume-2:C:\\volume_2']" else - volumes1 = "volumes => ['volume-1:#{volume_location}/volume_1']" - volumes2 = "volumes => ['volume-1:#{volume_location}/volume_1', 'volume-2:#{volume_location}/volume_2']" + volumes1 = "volumes => ['volume-1:#{volume_location}volume_1']" + volumes2 = "volumes => ['volume-1:#{volume_location}volume_1', 'volume-2:#{volume_location}volume_2']" end let(:pp1) do @@ -143,7 +143,7 @@ class {'docker': #{docker_args}} end after(:all) do - run_shell("rm -r #{volume_location}/volume_1") - run_shell("rm -r #{volume_location}/volume_2") + run_shell("rm -r #{volume_location}volume_1") + run_shell("rm -r #{volume_location}volume_2") end end