docker_run_flags: Shellescape any provided values #811

neomilium · 2022-03-28T17:39:15Z

When passing an environment variable with special characters, it can broke the script or produce unexpected behavior.
I encountered two of them:

If a $ is in environment variable, its interpolated as a variable
If a " is in environment variable, it broke the end of string, so the script

This PR contains a commit to escape any value that will be pushed in the docker start script.

puppet-community-rangefinder · 2022-03-28T17:39:22Z

docker_run_flags is a function

Breaking changes to this file WILL impact these 1 modules (exact match):

glorpen-g_docker

Breaking changes to this file MAY impact these 6 modules (near match):

This module is declared in 6 of 579 indexed public `Puppetfiles`.

These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

CLAassistant · 2022-03-28T17:39:26Z

All committers have signed the CLA.

chelnak · 2022-03-28T21:03:41Z

Hey 👋

Can you set this PR to ready for review?

thanks!

smortex · 2022-03-29T03:34:56Z

I am wondering if the CI failures are relevant… Like volume-1:C:\volume_1 being escaped as volume-1:C:\\volume_1… Not sure about how to check this…

neomilium · 2022-03-31T09:33:28Z

@chelnak Test added by @smortex, PR is now ready.

chelnak · 2022-04-03T19:58:32Z

@smortex - i'll take a closer look at this tomorrow. Seems odd that all of the failures here are for Windows and they don't seem to be on the nightly tests either..

smortex · 2022-04-13T02:55:48Z

@neomilium @chelnak I added a commit that seems to help… but escaping on windows is total nonsense and I am sure of nothing 😱

Some windows node pass, the one failing seems to fail earlier than the code added / changed here and the failures might be the result of some external service not being reliable… maybe?

lib/puppet/parser/functions/docker_run_flags.rb

bastelfreak · 2022-04-18T16:57:30Z

lib/puppet/parser/functions/docker_run_flags.rb

+    subject = args[0]
+
+    if self['facts'] && self['facts']['os']['family'] == 'windows'
+      %("#{subject.gsub('"', '\\"')}")


is that just because the shellescape function is designed for linux? what about patching https://github.com/puppetlabs/puppetlabs-stdlib/blob/main/lib/puppet/parser/functions/shell_escape.rb instead?

The more I am thinking about it, the more I am concerned about cmd.exe escaping vs. powershell escaping: they seem to be different in incompatible ways:

https://ss64.com/nt/syntax-esc.html

https://ss64.com/ps/syntax-esc.html

Maybe the stdlib should have 3 shell escape functions: shell_escape, powershell_escape and cmd_escape, then modules could have a local escape function which call the right escape function on windows depending on what the module is using?

that sounds like a good idea

puppetlabs/puppetlabs-stdlib#1235 was merged in stdlib. When we have a new release with this code, we will be able to move on with this PR.

@smortex Looks like these changes are available as of v8.2.0 of stdlib

Happy to work with someone to get this through.

Awesome! I added a commit to rely on this new code. Let's see what the CI think about it 😉

lib/puppet/parser/functions/docker_run_flags.rb

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.

lib/puppet/parser/functions/docker_run_flags.rb

At the cost of one more function call, we gain in readability and what the docker::escape functions is more straightforward.

Escaped strings should not be quotted. While here, also rework the way networks are escaped: the previous code was generating invalid parameters.

smortex · 2022-06-01T07:10:51Z

@neomilium have you had a chance to see if the issue you originally tried to fix with this PR is indeed fixed with all these changes?

canihavethisone · 2022-06-11T03:20:59Z

@smortex @chelnak @bastelfreak - why has stdlib been raised to min v8.2.0 in the pull request? That is very high and will have breaking impact when combined with a number of other available modules

smortex · 2022-06-11T04:07:19Z

@smortex @chelnak @bastelfreak - why has stdlib been raised to min v8.2.0 in the pull request? That is very high and will have breaking impact when combined with a number of other available modules

Because we had to add escape functions for windows in the stdlib: see this discussion above and puppetlabs/puppetlabs-stdlib#1235.

chelnak · 2022-07-04T10:28:06Z

@smortex @bastelfreak How do you both feel about moving forward with this PR?

Pending test failures which I think may be transient, I think it is good.

Though I do wonder if it is a breaking change due to the higher requirements of stdlib.

What are your thoughts?

chelnak · 2022-07-04T13:18:05Z

@smortex

I've chatted with @canihavethisone around some of the concerns raised in the comment above.

I personally think stdlib is the right place for the new escape commands however I want to get more of a feel for the potential impact on other users of the module.

This is also an issue that we will probably hit again with other modules. In order to take advantage of newer features in stdlib, the minimum version number will need to be raised.

canihavethisone · 2022-07-04T13:31:19Z

My concern is that the major fast forward of the stdlib dependency may have significant impact for users of this module in deployments alongside other modules that haven't caught up yet. I recon a good metric would be how many popular modules still have stdlib pinned lower than v.8x, and weigh that against the urgency of this change or if it can be merged when 8x is more commonly scoped. I understand there will be diversity of opinions but think its worth considering.

neomilium · 2022-07-05T14:00:10Z

I recon a good metric would be how many popular modules still have stdlib pinned lower than v.8x, and weigh that against the urgency of this change or if it can be merged when 8x is more commonly scoped.

How to define "popular modules"?

smortex

How do you both feel about moving forward with this PR?

My position is that we have tooling to check module versions and that roles and profile in a control-repo are working as expected (e.g. using onceover), so do not worry much about bumping versions. I would not expect users to blindly update things, skipping major versions and assume everything will be fine witout testing .

canihavethisone · 2022-08-18T22:05:28Z

@chelnak @smortex
Hi, im not concerned about people blindly updating, I'm concerned that we will lock out users who use this module alongside numerous others. Locking them out will deny them of new features and bugfixes until their other dependencies support stdlib v8. Bottom line IMO is that there is a huge difference between setting the upper limit to v8 or the lower requirement to v8. Its also a cost v benefit consideration at this moment in v8's youth.

binford2k · 2022-08-19T15:08:54Z

Here's a list of all the Forge modules that pin their stdlib dependencies to <8.x, sorted by download count (a weak correlation to popularity.) Notice that many of them are deprecated.

https://docs.google.com/spreadsheets/d/16QT1wd5CmwJL9TbKe4GZgpPi3W4nXHo7Q4fD5aK8bRM/edit#gid=223629225

LukasAud · 2022-10-12T09:54:55Z

Hi @neomilium. After careful evaluation of your PR, I'm happy to inform you that we are ready to go ahead and merge it. Thanks for your contribution!

We also noted that there are some concerns about this PR, such as the one raised by @canihavethisone. We have considered the potential impact of these changes and we have decided to still go forward with it. While it is true that there may be some compatibility issues with a few non-supported modules, we also have noted that this change may be important for one of our current security enhancement projects and, as such, either this or something very similar was bound to be implemented sooner or later. We are sorry if this causes any inconvenience.

LukasAud · 2022-10-12T10:55:29Z

This PR will be temporarily reverted until a release can be cut. Then it will be reinstated. This process is done to follow with standard best practices.

LukasAud · 2022-10-28T15:02:45Z

Hey @neomilium. We are sorry for this inconvenience but we had to revert your PR in order to follow best practices when releasing a new version of our module, by making a minor release prior to a backwards incompatible one. We could re-implement it but I'm afraid it would appear under our name. Could you reopen your branch under a new PR so that we can merge and give proper credit to all contributors?

kenyon · 2022-10-28T21:18:24Z

Rebasing or cherry-picking the commits would maintain the original commit authors.

docker_run_flags: Shellescape any provided values

neomilium requested a review from a team as a code owner March 28, 2022 17:39

neomilium marked this pull request as draft March 28, 2022 17:39

neomilium changed the title ~~WIP: docker_run_flags: Shellescape any provided values~~ docker_run_flags: Shellescape any provided values Mar 28, 2022

neomilium force-pushed the shellescape branch from 90feeb0 to c8ea99e Compare March 28, 2022 18:39

smortex marked this pull request as ready for review March 29, 2022 00:34

chelnak added the community label Mar 29, 2022

chelnak self-assigned this Apr 3, 2022

smortex reviewed Apr 13, 2022

View reviewed changes

lib/puppet/parser/functions/docker_run_flags.rb Outdated Show resolved Hide resolved

smortex reviewed Apr 13, 2022

View reviewed changes

lib/puppet/parser/functions/docker_run_flags.rb Outdated Show resolved Hide resolved

smortex force-pushed the shellescape branch from ddc1295 to 52eb180 Compare April 18, 2022 16:53

smortex self-requested a review April 18, 2022 16:54

bastelfreak reviewed Apr 18, 2022

View reviewed changes

smortex reviewed Apr 18, 2022

View reviewed changes

lib/puppet/parser/functions/docker_run_flags.rb Outdated Show resolved Hide resolved

neomilium and others added 3 commits May 31, 2022 09:36

docker_run_flags: Shellescape any provided values

5cd916a

Add unit test for new behavior

e6956bf

smortex force-pushed the shellescape branch from 52eb180 to 9b3b903 Compare May 31, 2022 20:24

Rely on latest stdlib for proper powershell escaping

df3d2b0

smortex force-pushed the shellescape branch from 9b3b903 to df3d2b0 Compare May 31, 2022 20:26

smortex reviewed May 31, 2022

View reviewed changes

lib/puppet/parser/functions/docker_run_flags.rb Outdated Show resolved Hide resolved

smortex added 2 commits May 31, 2022 11:09

Always pass subject to stdlib escape function

7ce43ad

At the cost of one more function call, we gain in readability and what the docker::escape functions is more straightforward.

Fix escaped string usage

0f9e2ea

Escaped strings should not be quotted. While here, also rework the way networks are escaped: the previous code was generating invalid parameters.

smortex marked this pull request as draft May 31, 2022 21:12

bastelfreak previously approved these changes Jun 1, 2022

View reviewed changes

Avoid mixing Linux and Windows path separator

8d282a8

smortex dismissed bastelfreak’s stale review via 8d282a8 June 11, 2022 04:13

chelnak added the enhancement label Jul 4, 2022

chelnak approved these changes Jul 4, 2022

View reviewed changes

chelnak self-requested a review July 11, 2022 08:41

smortex approved these changes Aug 17, 2022

View reviewed changes

LukasAud merged commit 92e58f2 into puppetlabs:main Oct 12, 2022

LukasAud added the backwards-incompatible label Oct 12, 2022

david22swan removed the enhancement label Oct 12, 2022

LukasAud mentioned this pull request Oct 12, 2022

Revert "docker_run_flags: Shellescape any provided values" #862

Merged

LukasAud added maintenance and removed backwards-incompatible labels Oct 12, 2022

LukasAud mentioned this pull request Oct 28, 2022

docker_run_flags: Shellescape any provided values #867

Closed

LukasAud mentioned this pull request Nov 7, 2022

docker_run_flags: Shellescape any provided values #869

Merged

GSPatton added a commit that referenced this pull request Nov 8, 2022

Merge pull request #869 from puppetlabs/GH-811-reimplement_reverted_PR

088cb33

docker_run_flags: Shellescape any provided values

neomilium deleted the shellescape branch November 9, 2022 06:26

docker_run_flags: Shellescape any provided values #811

docker_run_flags: Shellescape any provided values #811

Uh oh!

Conversation

neomilium commented Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

puppet-community-rangefinder bot commented Mar 28, 2022

docker_run_flags is a function

This module is declared in 6 of 579 indexed public Puppetfiles.

Uh oh!

CLAassistant commented Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chelnak commented Mar 28, 2022

Uh oh!

smortex commented Mar 29, 2022

Uh oh!

neomilium commented Mar 31, 2022

Uh oh!

chelnak commented Apr 3, 2022

Uh oh!

smortex commented Apr 13, 2022

Uh oh!

Uh oh!

Uh oh!

bastelfreak Apr 18, 2022

Choose a reason for hiding this comment

Uh oh!

smortex Apr 18, 2022

Choose a reason for hiding this comment

Uh oh!

bastelfreak Apr 18, 2022

Choose a reason for hiding this comment

Uh oh!

smortex May 11, 2022

Choose a reason for hiding this comment

Uh oh!

chelnak May 30, 2022

Choose a reason for hiding this comment

Uh oh!

smortex May 31, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

smortex commented Jun 1, 2022

Uh oh!

canihavethisone commented Jun 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smortex commented Jun 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chelnak commented Jul 4, 2022

Uh oh!

chelnak commented Jul 4, 2022

Uh oh!

canihavethisone commented Jul 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neomilium commented Jul 5, 2022

Uh oh!

smortex left a comment

Choose a reason for hiding this comment

Uh oh!

canihavethisone commented Aug 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

binford2k commented Aug 19, 2022

Uh oh!

LukasAud commented Oct 12, 2022

Uh oh!

LukasAud commented Oct 12, 2022

Uh oh!

LukasAud commented Oct 28, 2022

Uh oh!

kenyon commented Oct 28, 2022

Uh oh!

Uh oh!

neomilium commented Mar 28, 2022 •

edited

Loading

This module is declared in 6 of 579 indexed public `Puppetfiles`.

CLAassistant commented Mar 28, 2022 •

edited

Loading

canihavethisone commented Jun 11, 2022 •

edited

Loading

smortex commented Jun 11, 2022 •

edited

Loading

canihavethisone commented Jul 4, 2022 •

edited

Loading

canihavethisone commented Aug 18, 2022 •

edited

Loading