This patch is a pre-requisite config file consolidation. Currently we've
got a number of files which serve as a configuration either to the
lcitool itself or to the ansible playbooks (majority).  Once we replace
these with a single global lcitool config, we'd end up passing tokens
(potentially some passwords) as ansible extra variables bare naked on
the cmdline. In order to prevent this security flaw use temporary JSON
file holding all these extra variables and pass it as follows:

    $ ansible-playbook --extra-vars @extra_vars.json playbook.yml

Signed-off-by: Erik Skultety <eskultet@redhat.com>
---
 guests/lcitool | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/guests/lcitool b/guests/lcitool
index 51ee211..138b5e2 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -504,21 +504,26 @@ class Application:
             git_remote = "default"
             git_branch = "master"
 
+        tempdir = tempfile.TemporaryDirectory(prefix='lcitool')
+
         ansible_cfg_path = os.path.join(base, "ansible.cfg")
         playbook_base = os.path.join(base, "playbooks", playbook)
         playbook_path = os.path.join(playbook_base, "main.yml")
+        extra_vars_path = os.path.join(tempdir.name, 'extra_vars.json')
 
-        extra_vars = json.dumps({
-            "base": base,
-            "playbook_base": playbook_base,
-            "root_password_file": root_pass_file,
-            "flavor": flavor,
-            "selected_projects": selected_projects,
-            "git_remote": git_remote,
-            "git_branch": git_branch,
-            "gitlab_url_file": gitlab_url_file,
-            "gitlab_runner_token_file": gitlab_runner_token_file,
-        })
+        with open(extra_vars_path, 'w') as fp:
+            extra_vars = {
+                "base": base,
+                "playbook_base": playbook_base,
+                "root_password_file": root_pass_file,
+                "flavor": flavor,
+                "selected_projects": selected_projects,
+                "git_remote": git_remote,
+                "git_branch": git_branch,
+                "gitlab_url_file": gitlab_url_file,
+                "gitlab_runner_token_file": gitlab_runner_token_file,
+            }
+            json.dump(extra_vars, fp)
 
         ansible_playbook = distutils.spawn.find_executable("ansible-playbook")
         if ansible_playbook is None:
@@ -527,7 +532,7 @@ class Application:
         cmd = [
             ansible_playbook,
             "--limit", ansible_hosts,
-            "--extra-vars", extra_vars,
+            "--extra-vars", "@" + extra_vars_path,
         ]
 
         # Provide the vault password if available
@@ -546,6 +551,8 @@ class Application:
         except Exception as ex:
             raise Exception(
                 "Failed to run {} on '{}': {}".format(playbook, hosts, ex))
+        finally:
+            tempdir.cleanup()
 
     def _action_hosts(self, args):
         for host in self._inventory.expand_pattern("all"):
-- 
2.25.3

On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
> This patch is a pre-requisite config file consolidation. Currently we've
> got a number of files which serve as a configuration either to the
> lcitool itself or to the ansible playbooks (majority).  Once we replace
> these with a single global lcitool config, we'd end up passing tokens
> (potentially some passwords) as ansible extra variables bare naked on
> the cmdline. In order to prevent this security flaw use temporary JSON
> file holding all these extra variables and pass it as follows:
> 
>     $ ansible-playbook --extra-vars @extra_vars.json playbook.yml

I find it impossible not to point out that, if the configuration
file was in YAML format, then we could pass its contents to Ansible
without having to create a temporary file *or* risk exposing
sensitive data O:-)

> @@ -504,21 +504,26 @@ class Application:
>              git_remote = "default"
>              git_branch = "master"
>  
> +        tempdir = tempfile.TemporaryDirectory(prefix='lcitool')

If you want to pass prefix, do the same thing for the call introduced
in the previous commit.

Also, double quotes around strings please.

>          ansible_cfg_path = os.path.join(base, "ansible.cfg")
>          playbook_base = os.path.join(base, "playbooks", playbook)
>          playbook_path = os.path.join(playbook_base, "main.yml")
> +        extra_vars_path = os.path.join(tempdir.name, 'extra_vars.json')

Double quotes.

>  
> -        extra_vars = json.dumps({
> -            "base": base,
> -            "playbook_base": playbook_base,
> -            "root_password_file": root_pass_file,
> -            "flavor": flavor,
> -            "selected_projects": selected_projects,
> -            "git_remote": git_remote,
> -            "git_branch": git_branch,
> -            "gitlab_url_file": gitlab_url_file,
> -            "gitlab_runner_token_file": gitlab_runner_token_file,
> -        })
> +        with open(extra_vars_path, 'w') as fp:

Double quotes.

> +            extra_vars = {
> +                "base": base,
> +                "playbook_base": playbook_base,
> +                "root_password_file": root_pass_file,
> +                "flavor": flavor,
> +                "selected_projects": selected_projects,
> +                "git_remote": git_remote,
> +                "git_branch": git_branch,
> +                "gitlab_url_file": gitlab_url_file,
> +                "gitlab_runner_token_file": gitlab_runner_token_file,
> +            }
> +            json.dump(extra_vars, fp)

Moving the definition of the dictionary is not needed: just do

  extra_vars = {
      ...
  }

  with open(...) as fp:
      json.dumps(extra_vars, fp)

which keeps the with scope nice and small.


With these few nits fixed,

  Reviewed-by: Andrea Bolognani <abologna@redhat.com>

-- 
Andrea Bolognani / Red Hat / Virtualization