From 601f681787c8d6c02bb3566b8cefde289377be0e Mon Sep 17 00:00:00 2001 From: Eli Young Date: Thu, 28 May 2015 18:15:05 -0700 Subject: [PATCH 1/3] fqdn_rotate: Don't use the value itself as part of the random seed Previously, the random number generator was seeded with the array or string to be rotated in addition to any values specifically provided for seeding. This behavior is potentially insecure in that it allows an attacker who can modify the source data to choose the post-shuffle order. --- lib/puppet/parser/functions/fqdn_rotate.rb | 2 +- spec/acceptance/fqdn_rotate_spec.rb | 2 +- spec/functions/fqdn_rotate_spec.rb | 6 +----- 3 files changed, 3 insertions(+), 7 deletions(-) diff --git a/lib/puppet/parser/functions/fqdn_rotate.rb b/lib/puppet/parser/functions/fqdn_rotate.rb index d9741a02f..e1a50e6e1 100644 --- a/lib/puppet/parser/functions/fqdn_rotate.rb +++ b/lib/puppet/parser/functions/fqdn_rotate.rb @@ -11,7 +11,7 @@ module Puppet::Parser::Functions raise(Puppet::ParseError, "fqdn_rotate(): Wrong number of arguments " + "given (#{arguments.size} for 1)") if arguments.size < 1 - value = arguments[0] + value = arguments.shift require 'digest/md5' unless value.is_a?(Array) || value.is_a?(String) diff --git a/spec/acceptance/fqdn_rotate_spec.rb b/spec/acceptance/fqdn_rotate_spec.rb index 753068bfe..366d0273e 100755 --- a/spec/acceptance/fqdn_rotate_spec.rb +++ b/spec/acceptance/fqdn_rotate_spec.rb @@ -36,7 +36,7 @@ EOS apply_manifest(pp, :catch_failures => true) do |r| - expect(r.stdout).to match(/fqdn_rotate is \["c", "d", "a", "b"\]/) + expect(r.stdout).to match(/fqdn_rotate is \["d", "a", "b", "c"\]/) end end end diff --git a/spec/functions/fqdn_rotate_spec.rb b/spec/functions/fqdn_rotate_spec.rb index fe54490ef..6c76781e1 100755 --- a/spec/functions/fqdn_rotate_spec.rb +++ b/spec/functions/fqdn_rotate_spec.rb @@ -5,10 +5,6 @@ it { is_expected.to run.with_params().and_raise_error(Puppet::ParseError, /wrong number of arguments/i) } it { is_expected.to run.with_params(0).and_raise_error(Puppet::ParseError, /Requires either array or string to work with/) } it { is_expected.to run.with_params({}).and_raise_error(Puppet::ParseError, /Requires either array or string to work with/) } - it { - pending("Current implementation ignores parameters after the first.") - is_expected.to run.with_params("one", "two").and_raise_error(Puppet::ParseError) - } it { is_expected.to run.with_params('').and_return('') } it { is_expected.to run.with_params('a').and_return('a') } @@ -38,7 +34,7 @@ it "should use the Puppet::Util.deterministic_rand function" do if Puppet::Util.respond_to?(:deterministic_rand) - Puppet::Util.expects(:deterministic_rand).with(113646079810780526294648115052177588845,4) + Puppet::Util.expects(:deterministic_rand).with(44489829212339698569024999901561968770,4) fqdn_rotate("asdf") else skip 'Puppet::Util#deterministic_rand not available' From d7c846035321774e824e3424f59cb24703fcfb2a Mon Sep 17 00:00:00 2001 From: Eli Young Date: Mon, 1 Jun 2015 16:09:47 -0700 Subject: [PATCH 2/3] fqdn_rotate: Improve documentation --- README.markdown | 15 +++++++++++++- lib/puppet/parser/functions/fqdn_rotate.rb | 24 ++++++++++++++-------- 2 files changed, 29 insertions(+), 10 deletions(-) diff --git a/README.markdown b/README.markdown index c140af447..8ed3d9b24 100644 --- a/README.markdown +++ b/README.markdown @@ -259,7 +259,20 @@ fqdn_rand_string(10, '', 'custom seed') #### `fqdn_rotate` -Rotates an array a random number of times, based on a node's fqdn. *Type*: rvalue. +Rotates an array or string a random number of times, combining the `$fqdn` fact and an optional seed for repeatable randomness. + +*Usage:* +~~~ +fqdn_rotate(VALUE, [SEED]) +~~~ +*Examples:* +~~~ +fqdn_rotate(['a', 'b', 'c', 'd']) +fqdn_rotate('abcd') +fqdn_rotate([1, 2, 3], 'custom seed') +~~~ + +*Type*: rvalue. #### `get_module_path` diff --git a/lib/puppet/parser/functions/fqdn_rotate.rb b/lib/puppet/parser/functions/fqdn_rotate.rb index e1a50e6e1..b66431d1e 100644 --- a/lib/puppet/parser/functions/fqdn_rotate.rb +++ b/lib/puppet/parser/functions/fqdn_rotate.rb @@ -2,16 +2,23 @@ # fqdn_rotate.rb # -module Puppet::Parser::Functions - newfunction(:fqdn_rotate, :type => :rvalue, :doc => <<-EOS -Rotates an array a random number of times based on a nodes fqdn. - EOS - ) do |arguments| +Puppet::Parser::Functions.newfunction( + :fqdn_rotate, + :type => :rvalue, + :doc => "Usage: `fqdn_rotate(VALUE, [SEED])`. VALUE is required and + must be an array or a string. SEED is optional and may be any number + or string. + + Rotates VALUE a random number of times, combining the `$fqdn` fact and + the value of SEED for repeatable randomness. (That is, each node will + get a different random rotation from this function, but a given node's + result will be the same every time unless its hostname changes.) Adding + a SEED can be useful if you need more than one unrelated rotation.") do |args| raise(Puppet::ParseError, "fqdn_rotate(): Wrong number of arguments " + - "given (#{arguments.size} for 1)") if arguments.size < 1 + "given (#{args.size} for 1)") if args.size < 1 - value = arguments.shift + value = args.shift require 'digest/md5' unless value.is_a?(Array) || value.is_a?(String) @@ -31,7 +38,7 @@ module Puppet::Parser::Functions elements = result.size - seed = Digest::MD5.hexdigest([lookupvar('::fqdn'),arguments].join(':')).hex + seed = Digest::MD5.hexdigest([lookupvar('::fqdn'),args].join(':')).hex # deterministic_rand() was added in Puppet 3.2.0; reimplement if necessary if Puppet::Util.respond_to?(:deterministic_rand) offset = Puppet::Util.deterministic_rand(seed, elements).to_i @@ -51,7 +58,6 @@ module Puppet::Parser::Functions result = string ? result.join : result return result - end end # vim: set ts=2 sw=2 et : From b436216fe68c11d67cc15aec83ce0f1eeb7ededf Mon Sep 17 00:00:00 2001 From: Eli Young Date: Mon, 1 Jun 2015 16:29:39 -0700 Subject: [PATCH 3/3] fqdn_rotate: Add tests for custom seeds --- spec/functions/fqdn_rotate_spec.rb | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/spec/functions/fqdn_rotate_spec.rb b/spec/functions/fqdn_rotate_spec.rb index 6c76781e1..db7a71732 100755 --- a/spec/functions/fqdn_rotate_spec.rb +++ b/spec/functions/fqdn_rotate_spec.rb @@ -21,6 +21,18 @@ expect(val1).to eq(val2) end + it "allows extra arguments to control the random rotation on a single host" do + val1 = fqdn_rotate("abcdefg", :extra_identifier => [1, "different", "host"]) + val2 = fqdn_rotate("abcdefg", :extra_identifier => [2, "different", "host"]) + expect(val1).not_to eq(val2) + end + + it "considers the same host and same extra arguments to have the same random rotation" do + val1 = fqdn_rotate("abcdefg", :extra_identifier => [1, "same", "host"]) + val2 = fqdn_rotate("abcdefg", :extra_identifier => [1, "same", "host"]) + expect(val1).to eq(val2) + end + it "should rotate a string to give different values on different hosts" do val1 = fqdn_rotate("abcdefg", :host => 'one') val2 = fqdn_rotate("abcdefg", :host => 'two') @@ -51,11 +63,13 @@ def fqdn_rotate(value, args = {}) host = args[:host] || '127.0.0.1' + extra = args[:extra_identifier] || [] # workaround not being able to use let(:facts) because some tests need # multiple different hostnames in one context scope.stubs(:lookupvar).with("::fqdn").returns(host) - scope.function_fqdn_rotate([value]) + function_args = [value] + extra + scope.function_fqdn_rotate(function_args) end end