(#347) validate input given on the client before making the request

This uses Puppet to validate the input given on the CLI against the
task specification, input from json/yaml files are combined with the
options given on the CLI

This is only half the story, we must do validation on the nodes as well
but right now the nodes do not get the full task metadata only the files
spec, this is the mechanism that creates a specific predictable set of
executables being run and fetching the metadata from the server would be
open to a time based race

But sending the metadata to validate against to the server provides no
security, so perhaps this is fine, need to think some more, but for now
we have quick client side failure on input validation errors

Gemfile

@@ -13,7 +13,7 @@ group :development, :test do
   gem "listen", "~> 3"
   gem "mcollective-client"
   gem "mocha"
-  gem "puppet", "~> 5"
+  gem "puppet", "~> 5.4"
   gem "rake"
   gem "rspec"
   gem "rubocop", "0.51.0"

Gemfile.lock

@@ -25,15 +25,8 @@ GEM
     faraday (0.11.0)
       multipart-post (>= 1.2, < 3)
     fast_gettext (1.1.2)
-    ffi (1.9.21)
+    ffi (1.9.23)
     formatador (0.2.5)
-    gettext (3.2.6)
-      locale (>= 2.0.5)
-      text (>= 1.3.0)
-    gettext-setup (0.30)
-      fast_gettext (~> 1.1.0)
-      gettext (>= 3.0.2)
-      locale
     google-protobuf (3.5.1.2)
     googleauth (0.5.1)
       faraday (~> 0.9)
@@ -94,15 +87,15 @@ GEM
       metaclass (~> 0.0.1)
     multi_json (1.13.1)
     multipart-post (2.0.0)
-    nats-pure (0.2.4)
+    nats-pure (0.4.0)
     nenv (0.3.0)
     notiffany (0.1.1)
       nenv (~> 0.1)
       shellany (~> 0.0)
     os (0.9.6)
     parallel (1.12.1)
-    parser (2.4.0.2)
-      ast (~> 2.3)
+    parser (2.5.0.4)
+      ast (~> 2.4.0)
     powerpack (0.1.1)
     pry (0.11.3)
       coderay (~> 1.1.0)
@@ -116,7 +109,7 @@ GEM
     rainbow (2.2.2)
       rake
     rake (12.3.0)
-    rb-fsevent (0.10.2)
+    rb-fsevent (0.10.3)
     rb-inotify (0.9.10)
       ffi (>= 0.5.0, < 2)
     rspec (3.7.0)
@@ -142,8 +135,7 @@ GEM
     ruby-progressbar (1.9.0)
     ruby_dep (1.5.0)
     safe_yaml (1.0.4)
-    semantic_puppet (1.0.1)
-      gettext-setup (>= 0.3)
+    semantic_puppet (1.0.2)
     shellany (0.0.1)
     signet (0.8.1)
       addressable (~> 2.3)
@@ -159,7 +151,6 @@ GEM
     systemu (2.6.5)
     term-ansicolor (1.6.0)
       tins (~> 1.0)
-    text (1.3.1)
     thor (0.19.4)
     tins (1.16.3)
     unicode-display_width (1.3.0)
@@ -183,8 +174,8 @@ DEPENDENCIES
   listen (~> 3)
   mcollective-client
   mocha
-  nats-pure (~> 0.2)
-  puppet (~> 5)
+  nats-pure (~> 0.4)
+  puppet (~> 5.4)
   rake
   rspec
   rubocop (= 0.51.0)

LICENSE.txt

@@ -186,7 +186,7 @@
       same "printed page" as the copyright notice for easier
       identification within third-party archives.
 
-   Copyright 2017 R.I.Pienaar <rip@devco.net>
+   Copyright [yyyy] [name of copyright owner]
 
    Licensed under the Apache License, Version 2.0 (the "License");
    you may not use this file except in compliance with the License.

NOTICE.txt

@@ -0,0 +1,5 @@
+Choria Orchestrator
+Copyright 2018 R.I. Pienaar <rip@devco.net>
+
+This product includes software developed by members of
+the Choria Project (https://choria.io)

lib/mcollective/application/playbook.rb

@@ -44,11 +44,11 @@ def pre_parse_find_playbook
         pb if pb =~ /\A([a-z][a-z0-9_]*)?(::[a-z][a-z0-9_]*)*\Z/
       end
 
-      # Creates an instance of the playbook
+      # Creates an instance of the plan runner
       #
-      # @param plan [String] path to a playbook yaml
+      # @param plan [String] the name of a plan
       # @return [Util::BoltSupport::PlanRunner]
-      def playbook(plan, loglevel=nil)
+      def runner(plan, loglevel=nil)
         unless configuration[:__modulepath]
           configuration[:__modulepath] = File.expand_path("~/.puppetlabs/etc/code/modules")
         end
@@ -82,7 +82,7 @@ def run
 
           if playbook_name = pre_parse_find_playbook
             configuration[:__playbook] = playbook_name
-            playbook(playbook_name).add_cli_options(self, false)
+            runner(playbook_name).add_cli_options(self, false)
           end
 
           # Hackily done here to force it below the playbook options
@@ -151,7 +151,7 @@ def run_command
         pb_config = configuration.clone
         pb_config.delete_if {|k, _| k.to_s.start_with?("__")}
 
-        pb = playbook(configuration[:__playbook], configuration[:__loglevel])
+        pb = runner(configuration[:__playbook], configuration[:__loglevel])
 
         run_plan(pb, pb_config)
       end

lib/mcollective/application/tasks.rb

@@ -10,8 +10,8 @@ class Tasks < Application
     mco tasks run <TASK NAME> [OPTIONS]
     mco tasks status <REQUEST> [FLAGS]
 
- The Bolt Task Orchestrator is designed to provide a consistent
- management environment for Bolt Tasks.
+ The Task Orchestrator is designed to provide a consistent
+ management environment for Puppet Tasks.
 
  It will download tasks from your Puppet Server onto all nodes
  and after verifying they were able to correctly download the
@@ -34,12 +34,6 @@ def list_options
                           :description => "Show command descriptions",
                           :default => false,
                           :type => :boolean
-
-        self.class.option :__environment,
-                          :arguments => ["--environment"],
-                          :description => "Environment to retrieve tasks from",
-                          :default => "production",
-                          :type => String
       end
 
       def status_options
@@ -67,12 +61,6 @@ def status_options
                           :default => false,
                           :type => :boolean
 
-        self.class.option :__environment,
-                          :arguments => ["--environment"],
-                          :description => "Environment to retrieve tasks from",
-                          :default => "production",
-                          :type => String
-
         self.class.option :__json_format,
                           :arguments => ["--json"],
                           :description => "Display results in JSON format",
@@ -128,7 +116,7 @@ def run_options
 
         abort("Please specify a task to run") unless task
 
-        cli.create_task_options(task, environment, self)
+        cli.create_task_options(task, "production", self)
 
         self.class.option :__summary,
                           :arguments => ["--summary"],
@@ -147,12 +135,6 @@ def run_options
                           :description => "JSON input to pass to the task",
                           :required => false,
                           :type => String
-
-        self.class.option :__environment,
-                          :arguments => ["--environment"],
-                          :description => "Environment to retrieve tasks from",
-                          :default => "production",
-                          :type => String
       end
 
       def say(msg="")
@@ -164,16 +146,19 @@ def run_command
 
         input = cli.task_input(configuration)
 
-        say("Attempting to download and run task %s on %d nodes" % [Util.colorize(:bold, task), bolt_task.discover.size])
-        say
         say("Retrieving task metadata for task %s from the Puppet Server" % task)
 
         begin
-          meta = cli.task_metadata(task, configuration[:__environment])
+          meta = cli.task_metadata(task, "production")
         rescue
           abort($!.to_s)
         end
 
+        cli.validate_task_input(task, meta, input)
+
+        say("Attempting to download and run task %s on %d nodes" % [Util.colorize(:bold, task), bolt_task.discover.size])
+        say
+
         download_files(task, meta["files"])
 
         request = {
@@ -212,7 +197,7 @@ def download_files(task, files)
         cnt = bolt_task.discover.size
         idx = 0
 
-        bolt_task.download(:environment => configuration[:__environment], :task => task, :files => files.to_json) do |_, s|
+        bolt_task.download(:environment => "production", :task => task, :files => files.to_json) do |_, s|
           print(cli.twirl("Downloading and verifying %d file(s) from the Puppet Server to all nodes:" % [files.size], cnt, idx + 1)) unless configuration[:__json_format]
           idx += 1
           downloads << s
@@ -330,7 +315,7 @@ def request_and_report(action, arguments, taskid=nil) # rubocop:disable Metrics/
       end
 
       def list_command
-        cli.show_task_list(configuration[:__environment], configuration[:__detail])
+        cli.show_task_list("production", configuration[:__detail])
       end
 
       def run
@@ -343,7 +328,7 @@ def run
       end
 
       def show_task_help(task)
-        cli.show_task_help(task, configuration[:__environment])
+        cli.show_task_help(task, "production")
       end
 
       def bolt_task
@@ -375,7 +360,7 @@ def tasks_support
       def cli
         format = configuration[:__json_format] ? :json : :default
 
-        @__cli ||= tasks_support.cli(format, options[:verbose])
+        @__cli ||= tasks_support.cli(format, application_options[:verbose])
       end
 
       def main

lib/mcollective/util/tasks_support.rb

@@ -280,8 +280,8 @@ def wait_for_task_completion(requestid)
       # will continue and one can later check again using the request id
       #
       # @note before this should be run be sure to download the tasks first
-      # @param task [Hash] task specification
       # @param requestid [String] the task requestid
+      # @param task [Hash] task specification
       # @param wait [Boolean] should the we wait for the task to complete
       # @param callerid [String] the mcollective callerid who is running the task
       # @return [Hash] the task result as per {#task_result}
@@ -564,6 +564,34 @@ def task_metadata(task, environment)
         JSON.parse(resp.body)
       end
 
+      # Validates that the inputs would be acceptable to the task
+      #
+      # @note Copied from PAL TaskSignature#runnable_with?
+      # @param inputs [Hash] of keys and values
+      # @param task [Hash] task metadata
+      # @return [Array[Boolean, Error]]
+      def validate_task_inputs(inputs, task)
+        return [true, ""] if task["metadata"]["parameters"].empty? && inputs.empty?
+
+        require "puppet"
+
+        signature = {}
+
+        task["metadata"]["parameters"].each do |k, v|
+          signature[k] = Puppet::Pops::Types::TypeParser.singleton.parse(v["type"])
+        end
+
+        signature_type = Puppet::Pops::Types::TypeFactory.struct(signature)
+
+        return [true, ""] if signature_type.instance?(inputs)
+
+        tm = Puppet::Pops::Types::TypeMismatchDescriber.singleton
+        reason = tm.describe_struct_signature(signature_type, inputs).flatten.map(&:format).join("\n")
+        reason = "\nInvalid input: \n\t%s" % [reason]
+
+        [false, reason]
+      end
+
       # Calculates a hex digest SHA256 for a specific file
       #
       # @param file_path [String] a full path to the file to check
@@ -632,15 +660,6 @@ def cache_task_file(file)
         end
       end
 
-      # Downloads all the files in a task
-      #
-      # @param files [Array] the files description
-      # @return [Boolean] indicating download success
-      # @raise [StandardError] on download failures
-      def download_task(task)
-        download_files(task["files"])
-      end
-
       # Downloads and caches a file set
       #
       # @param files [Array] the files description

lib/mcollective/util/tasks_support/cli.rb

@@ -137,18 +137,20 @@ def print_result_metadata(*args)
         # Parses the given CLI input string and creates results based on it
         #
         # @param configuration [Hash] the mcollective Application configuration
-        # @return [Hash,nil]
+        # @return [Hash]
         def task_input(configuration)
           result = {}
 
           input = configuration[:__json_input]
 
-          if input && input.start_with?("@")
-            input.sub!("@", "")
-            result = JSON.parse(File.read(input)) if input.end_with?("json")
-            result = YAML.safe_load(File.read(input)) if input.end_with?("yaml")
-          else
-            result = JSON.parse(input)
+          if input
+            if input.start_with?("@")
+              input.sub!("@", "")
+              result = JSON.parse(File.read(input)) if input.end_with?("json")
+              result = YAML.safe_load(File.read(input)) if input.end_with?("yaml")
+            else
+              result = JSON.parse(input)
+            end
           end
 
           configuration.each do |item, value|
@@ -159,8 +161,21 @@ def task_input(configuration)
           return result unless result.empty?
 
           abort("Could not parse input from --input as YAML or JSON")
+        end
+
+        # Validates the inputs provided on the CLI would be acceptable to the task
+        #
+        # @param task [String] task name
+        # @param meta [Hash] task metadata
+        # @param input [Hash] proposed input to pass to the task
+        # @return [Boolean]
+        # @raize [Exit] on failure
+        def validate_task_input(task, meta, input)
+          ok, reason = @support.validate_task_inputs(input, task, meta)
+
+          return true if ok
 
-          nil
+          abort(reason)
         end
 
         # Adds CLI options for all defined input

spec/unit/mcollective/util/tasks_support_spec.rb

@@ -23,6 +23,24 @@ module Util
         FileUtils.rm_rf("/tmp/tasks-cache-#{$$}")
       end
 
+      describe "#validate_task_inputs" do
+        it "should handle tasks without inputs" do
+          task_fixture["metadata"]["parameters"].clear
+
+          expect(ts.validate_task_inputs({}, task_fixture)).to eq([true, ""])
+        end
+
+        it "should handle bad inputs" do
+          task_fixture["metadata"]["parameters"].clear
+
+          expect(ts.validate_task_inputs({"x" => 1}, task_fixture)).to eq([false, "\nInvalid input: \n\t has no parameter named 'x'"])
+        end
+
+        it "should handle good inputs" do
+          expect(ts.validate_task_inputs({"directory" => "/tmp"}, task_fixture)).to eq([true, ""])
+        end
+      end
+
       describe "#puppet_type_to_ruby" do
         it "should handle arrays" do
           expect(ts.puppet_type_to_ruby("Array[Integer]")).to eq([Numeric, true, true])