From 828579d974e26f33c030abf24068a291bf47b05a Mon Sep 17 00:00:00 2001 From: "R.I.Pienaar" Date: Sun, 18 Mar 2018 20:50:40 +0100 Subject: [PATCH] (#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 | 2 +- Gemfile.lock | 25 ++++------- LICENSE.txt | 2 +- NOTICE.txt | 5 +++ lib/mcollective/application/playbook.rb | 10 ++--- lib/mcollective/application/tasks.rb | 41 ++++++------------- lib/mcollective/util/tasks_support.rb | 39 +++++++++++++----- lib/mcollective/util/tasks_support/cli.rb | 31 ++++++++++---- .../mcollective/util/tasks_support_spec.rb | 18 ++++++++ 9 files changed, 103 insertions(+), 70 deletions(-) create mode 100644 NOTICE.txt diff --git a/Gemfile b/Gemfile index 3575824..e0946ba 100644 --- a/Gemfile +++ b/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" diff --git a/Gemfile.lock b/Gemfile.lock index 7be9c2a..3cddadb 100644 --- a/Gemfile.lock +++ b/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) diff --git a/LICENSE.txt b/LICENSE.txt index 41193c6..6f75635 100644 --- a/LICENSE.txt +++ b/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 + 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. diff --git a/NOTICE.txt b/NOTICE.txt new file mode 100644 index 0000000..79413f4 --- /dev/null +++ b/NOTICE.txt @@ -0,0 +1,5 @@ +Choria Orchestrator +Copyright 2018 R.I. Pienaar + +This product includes software developed by members of +the Choria Project (https://choria.io) diff --git a/lib/mcollective/application/playbook.rb b/lib/mcollective/application/playbook.rb index 8d91e0e..17ce41f 100644 --- a/lib/mcollective/application/playbook.rb +++ b/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 diff --git a/lib/mcollective/application/tasks.rb b/lib/mcollective/application/tasks.rb index 2415ffb..4309312 100644 --- a/lib/mcollective/application/tasks.rb +++ b/lib/mcollective/application/tasks.rb @@ -10,8 +10,8 @@ class Tasks < Application mco tasks run [OPTIONS] mco tasks status [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 diff --git a/lib/mcollective/util/tasks_support.rb b/lib/mcollective/util/tasks_support.rb index 80c794c..2ccf259 100644 --- a/lib/mcollective/util/tasks_support.rb +++ b/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 diff --git a/lib/mcollective/util/tasks_support/cli.rb b/lib/mcollective/util/tasks_support/cli.rb index 6549c9b..621ef51 100644 --- a/lib/mcollective/util/tasks_support/cli.rb +++ b/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 diff --git a/spec/unit/mcollective/util/tasks_support_spec.rb b/spec/unit/mcollective/util/tasks_support_spec.rb index afb00b6..f3c31db 100644 --- a/spec/unit/mcollective/util/tasks_support_spec.rb +++ b/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])