(maint) Hardening manifests and tasks #863
Conversation
docker::compose is a classBreaking changes to this file WILL impact these 3 modules (exact match):docker::install is a classthat may have no external impact to Forge modules. docker::machine is a classthat may have no external impact to Forge modules. docker::plugin is a typethat may have no external impact to Forge modules. docker::registry is a typeBreaking changes to this file WILL impact these 2 modules (exact match):docker::run is a typeBreaking changes to this file WILL impact these 9 modules (exact match):Breaking changes to this file MAY impact these 22 modules (near match):
docker::secrets is a typethat may have no external impact to Forge modules. docker::services is a typethat may have no external impact to Forge modules. docker::stack is a typethat may have no external impact to Forge modules. docker::swarm is a typeBreaking changes to this file WILL impact these 2 modules (exact match):This module is declared in 6 of 579 indexed public
|
LukasAud
force-pushed
the
maint-codebase-hardening
branch
2 times, most recently
from
ed28eed to
dce80eb
Compare
| @@ -89,10 +89,11 @@ | |||
| ensure_packages(['curl']) | |||
| } | |||
|
|
|||
| $compose_install = "curl -s -S -L ${proxy_opt} ${docker_compose_url} -o ${docker_compose_location_versioned}" | |||
There was a problem hiding this comment.
Should this not be broken in to args?
| $compose_install = "curl -s -S -L ${proxy_opt} ${docker_compose_url} -o ${docker_compose_location_versioned}" | |
| $compose_install = ['curl', '-s', '-S', '-L', $proxy_opt, docker_compose_url, '-o', $docker_compose_location_versioned]" |
There was a problem hiding this comment.
Anything that is not broken into args will cry, scream and kick at me when I run test cases. Tried to figure out the reason behind those failures but eventually had to give up and just let some commands stay the same.
There was a problem hiding this comment.
ok cool - understand the frustration but we should investigate why it's failing. There are multiple options here that don't involve running the tests - one being running the module via puppet apply and using pry.
| plugin_name => $plugin_name, | ||
| force_remove => $force_remove, | ||
| } | ||
| ) | ||
|
|
||
| $exec_rm = "${docker_command} rm ${docker_plugin_remove_flags}" | ||
| $exec_rm = [$docker_command, 'rm', $docker_plugin_remove_flags] | ||
| $onlyif_rm = "${docker_command} ls --format='{{.PluginReference}}' | grep -w ${plugin_name}" |
There was a problem hiding this comment.
This is an interesting one. $onlyif_rm should be broken out because both $docker_command and $plugin_name come from parameters.
With exec, both onlyif and unless accept a String or an Array[Array[String]].
For the latter it would look like this
onlyif => [ ['my', 'command', 'and', 'args'] ]
When it is evaluated exec looks to see if all commands are truthy (e.g you may have multiple inner arrays for different commands).
The tricky part here is that when using an array of args, exec will disregard any metacharacters (right word??). In this case it would ignore the pipe and everything to the right of it.
There may be an example of how to handle this in apt or mysql. Alternatively we could just clean the strings with shell_escape or something similar.
There was a problem hiding this comment.
Talked with @david22swan about this cases. I believe we just agreed to not touch anything with pipes because of the reason mentioned. I did try some variations arraying and segmenting like:
$onlyif_rm = ["$docker_command, 'ls', "--format='{{.PluginReference}}' | grep -w ${plugin_name}"]
Just for the sake of testing whether the problem was separating the pipe from the rest of the arguments but it did not work. Currently any command with a grep is untouched in this and every other PR afaik.
There was a problem hiding this comment.
It shouldn't be left. We need to address these.
There was a problem hiding this comment.
It should be fine so long as the command is sanitized in some other way.
In this case the only variable part, docker_command should be given a shell escape as while it has a default value of docker it is able to be manually set.
It's a difficult one as it is explicitly meant to be a variable for overriding what the command does....
There was a problem hiding this comment.
..... Could add a regex check to it to make sure that the command always includes docker at the end with no white spaces in front of it, i.e. that it's always a path to the docker command
This PR aims to implement some changes that ensure no malformed commands are passed through to the system. Certain commands were left undivided as the commands did not get correctly interpreted. Primarily, the commands targeted were the ones related to Open3 and exec.
Prior to this commit lint would not work properly in Github Actions. This commit aims to fix the problems that cause failure in CI.
Prior to this commit, there was a block of code checking for puppet 5, which we no longer support. This commit aims to clean up this unnecesary code in order to keep the module tidy.
LukasAud
force-pushed
the
maint-codebase-hardening
branch
from
ce64aa5 to
40f1a09
Compare
This PR aims to implement some changes that ensure no malformed commands are passed through to the system. Certain commands were left undivided as the commands did not get correctly interpreted.
Primarily, the commands targeted were the ones related to Open3 and exec.