From 1c7f0c3530846d9935bbc13cda33430cf5632975 Mon Sep 17 00:00:00 2001 From: Stefan Schulte Date: Tue, 12 Apr 2011 00:48:26 +0200 Subject: (#7114) Improve value validation for authorized_key Whitespaces in any of the properties can lead to incorrect entries in the authorized_keys file. Reviewed-By: Nick Lewis Reviewed-By: Josh Cooper --- lib/puppet/type/ssh_authorized_key.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/lib/puppet/type/ssh_authorized_key.rb b/lib/puppet/type/ssh_authorized_key.rb index 8338e2d64..170dc8383 100644 --- a/lib/puppet/type/ssh_authorized_key.rb +++ b/lib/puppet/type/ssh_authorized_key.rb @@ -14,6 +14,10 @@ module Puppet system-wide primary key and therefore has to be unique." isnamevar + + validate do |value| + raise Puppet::Error, "Resourcename must not contain whitespace: #{value}" if value =~ /\s/ + end end newproperty(:type) do @@ -28,6 +32,10 @@ module Puppet newproperty(:key) do desc "The key itself; generally a long string of hex digits." + + validate do |value| + raise Puppet::Error, "Key must not contain whitespace: #{value}" if value =~ /\s/ + end end newproperty(:user) do @@ -82,6 +90,10 @@ module Puppet value.join(",") end end + + validate do |value| + raise Puppet::Error, "Options must be provided as an array, not a comma separated list" if value != :absent and value.include?(',') + end end autorequire(:user) do -- cgit From a5ac82ab7ea6a72114872c9ad8a23e1c3b70e9d4 Mon Sep 17 00:00:00 2001 From: Stefan Schulte Date: Tue, 12 Apr 2011 00:49:50 +0200 Subject: (#7114) Improve unit tests for ssh_authorized_key Add tests to check if the type supports valid values for all the properties. Also check if the type rejects values that are invalid. Reviewed-By: Nick Lewis Reviewed-By: Josh Cooper --- spec/unit/type/ssh_authorized_key_spec.rb | 250 +++++++++++++++++++++--------- 1 file changed, 180 insertions(+), 70 deletions(-) diff --git a/spec/unit/type/ssh_authorized_key_spec.rb b/spec/unit/type/ssh_authorized_key_spec.rb index a5f167165..71b8a9ab0 100755 --- a/spec/unit/type/ssh_authorized_key_spec.rb +++ b/spec/unit/type/ssh_authorized_key_spec.rb @@ -16,115 +16,223 @@ describe ssh_authorized_key do @catalog = Puppet::Resource::Catalog.new end - it "should have a name parameter" do - @class.attrtype(:name).should == :param - end - it "should have :name be its namevar" do @class.key_attributes.should == [:name] end - it "should have a :provider parameter" do - @class.attrtype(:provider).should == :param - end + describe "when validating attributes" do - it "should have an ensure property" do - @class.attrtype(:ensure).should == :property - end + [:name, :provider].each do |param| + it "should have a #{param} parameter" do + @class.attrtype(param).should == :param + end + end - it "should support :present as a value for :ensure" do - proc { @class.new(:name => "whev", :ensure => :present, :user => "nobody") }.should_not raise_error - end + [:type, :key, :user, :target, :options, :ensure].each do |property| + it "should have a #{property} property" do + @class.attrtype(property).should == :property + end + end - it "should support :absent as a value for :ensure" do - proc { @class.new(:name => "whev", :ensure => :absent, :user => "nobody") }.should_not raise_error end - it "should have an type property" do - @class.attrtype(:type).should == :property - end - it "should support ssh-dss as an type value" do - proc { @class.new(:name => "whev", :type => "ssh-dss", :user => "nobody") }.should_not raise_error - end - it "should support ssh-rsa as an type value" do - proc { @class.new(:name => "whev", :type => "ssh-rsa", :user => "nobody") }.should_not raise_error - end - it "should support :dsa as an type value" do - proc { @class.new(:name => "whev", :type => :dsa, :user => "nobody") }.should_not raise_error - end - it "should support :rsa as an type value" do - proc { @class.new(:name => "whev", :type => :rsa, :user => "nobody") }.should_not raise_error - end + describe "when validating values" do - it "should not support values other than ssh-dss, ssh-rsa, dsa, rsa in the ssh_authorized_key_type" do - proc { @class.new(:name => "whev", :type => :something) }.should raise_error(Puppet::Error) - end + describe "for name" do - it "should have an key property" do - @class.attrtype(:key).should == :property - end + it "should support valid names" do + proc { @class.new(:name => "username", :ensure => :present, :user => "nobody") }.should_not raise_error + proc { @class.new(:name => "username@hostname", :ensure => :present, :user => "nobody") }.should_not raise_error + end - it "should have an user property" do - @class.attrtype(:user).should == :property - end + it "should not support whitespaces" do + proc { @class.new(:name => "my test", :ensure => :present, :user => "nobody") }.should raise_error(Puppet::Error,/Resourcename must not contain whitespace/) + proc { @class.new(:name => "my\ttest", :ensure => :present, :user => "nobody") }.should raise_error(Puppet::Error,/Resourcename must not contain whitespace/) + end - it "should have an options property" do - @class.attrtype(:options).should == :property - end + end - it "'s options property should return well formed string of arrays from is_to_s" do - resource = @class.new(:name => "whev", :type => :rsa, :user => "nobody", :options => ["a","b","c"]) + describe "for ensure" do - resource.property(:options).is_to_s(["a","b","c"]).should == "a,b,c" - end + it "should support :present" do + proc { @class.new(:name => "whev", :ensure => :present, :user => "nobody") }.should_not raise_error + end - it "'s options property should return well formed string of arrays from is_to_s" do - resource = @class.new(:name => "whev", :type => :rsa, :user => "nobody", :options => ["a","b","c"]) + it "should support :absent" do + proc { @class.new(:name => "whev", :ensure => :absent, :user => "nobody") }.should_not raise_error + end - resource.property(:options).should_to_s(["a","b","c"]).should == "a,b,c" - end + it "should not support other values" do + proc { @class.new(:name => "whev", :ensure => :foo, :user => "nobody") }.should raise_error(Puppet::Error, /Invalid value/) + end + + end + + describe "for type" do + + + it "should support ssh-dss" do + proc { @class.new(:name => "whev", :type => "ssh-dss", :user => "nobody") }.should_not raise_error + end + + it "should support ssh-rsa" do + proc { @class.new(:name => "whev", :type => "ssh-rsa", :user => "nobody") }.should_not raise_error + end + + it "should support :dsa" do + proc { @class.new(:name => "whev", :type => :dsa, :user => "nobody") }.should_not raise_error + end + + it "should support :rsa" do + proc { @class.new(:name => "whev", :type => :rsa, :user => "nobody") }.should_not raise_error + end + + it "should alias :rsa to :ssh-rsa" do + key = @class.new(:name => "whev", :type => :rsa, :user => "nobody") + key.should(:type).should == :'ssh-rsa' + end + + it "should alias :dsa to :ssh-dss" do + key = @class.new(:name => "whev", :type => :dsa, :user => "nobody") + key.should(:type).should == :'ssh-dss' + end + + it "should not support values other than ssh-dss, ssh-rsa, dsa, rsa" do + proc { @class.new(:name => "whev", :type => :something) }.should raise_error(Puppet::Error,/Invalid value/) + end + + end + + describe "for key" do + + it "should support a valid key like a 1024 bit rsa key" do + proc { @class.new(:name => "whev", :type => :rsa, :user => "nobody", :key => 'AAAAB3NzaC1yc2EAAAADAQABAAAAgQDCPfzW2ry7XvMc6E5Kj2e5fF/YofhKEvsNMUogR3PGL/HCIcBlsEjKisrY0aYgD8Ikp7ZidpXLbz5dBsmPy8hJiBWs5px9ZQrB/EOQAwXljvj69EyhEoGawmxQMtYw+OAIKHLJYRuk1QiHAMHLp5piqem8ZCV2mLb9AsJ6f7zUVw==')}.should_not raise_error + end + + it "should support a valid key like a 4096 bit rsa key" do + proc { @class.new(:name => "whev", :type => :rsa, :user => "nobody", :key => 'AAAAB3NzaC1yc2EAAAADAQABAAACAQDEY4pZFyzSfRc9wVWI3DfkgT/EL033UZm/7x1M+d+lBD00qcpkZ6CPT7lD3Z+vylQlJ5S8Wcw6C5Smt6okZWY2WXA9RCjNJMIHQbJAzwuQwgnwU/1VMy9YPp0tNVslg0sUUgpXb13WW4mYhwxyGmIVLJnUrjrQmIFhtfHsJAH8ZVqCWaxKgzUoC/YIu1u1ScH93lEdoBPLlwm6J0aiM7KWXRb7Oq1nEDZtug1zpX5lhgkQWrs0BwceqpUbY+n9sqeHU5e7DCyX/yEIzoPRW2fe2Gx1Iq6JKM/5NNlFfaW8rGxh3Z3S1NpzPHTRjw8js3IeGiV+OPFoaTtM1LsWgPDSBlzIdyTbSQR7gKh0qWYCNV/7qILEfa0yIFB5wIo4667iSPZw2pNgESVtenm8uXyoJdk8iWQ4mecdoposV/znknNb2GPgH+n/2vme4btZ0Sl1A6rev22GQjVgbWOn8zaDglJ2vgCN1UAwmq41RXprPxENGeLnWQppTnibhsngu0VFllZR5kvSIMlekLRSOFLFt92vfd+tk9hZIiKm9exxcbVCGGQPsf6dZ27rTOmg0xM2Sm4J6RRKuz79HQgA4Eg18+bqRP7j/itb89DmtXEtoZFAsEJw8IgIfeGGDtHTkfAlAC92mtK8byeaxGq57XCTKbO/r5gcOMElZHy1AcB8kw==')}.should_not raise_error + end + + it "should support a valid key like a 1024 bit dsa key" do + proc { @class.new(:name => "whev", :type => :dsa, :user => "nobody", :key => 'AAAAB3NzaC1kc3MAAACBAI80iR78QCgpO4WabVqHHdEDigOjUEHwIjYHIubR/7u7DYrXY+e+TUmZ0CVGkiwB/0yLHK5dix3Y/bpj8ZiWCIhFeunnXccOdE4rq5sT2V3l1p6WP33RpyVYbLmeuHHl5VQ1CecMlca24nHhKpfh6TO/FIwkMjghHBfJIhXK+0w/AAAAFQDYzLupuMY5uz+GVrcP+Kgd8YqMmwAAAIB3SVN71whLWjFPNTqGyyIlMy50624UfNOaH4REwO+Of3wm/cE6eP8n75vzTwQGBpJX3BPaBGW1S1Zp/DpTOxhCSAwZzAwyf4WgW7YyAOdxN3EwTDJZeyiyjWMAOjW9/AOWt9gtKg0kqaylbMHD4kfiIhBzo31ZY81twUzAfN7angAAAIBfva8sTSDUGKsWWIXkdbVdvM4X14K4gFdy0ZJVzaVOtZ6alysW6UQypnsl6jfnbKvsZ0tFgvcX/CPyqNY/gMR9lyh/TCZ4XQcbqeqYPuceGehz+jL5vArfqsW2fJYFzgCcklmr/VxtP5h6J/T0c9YcDgc/xIfWdZAlznOnphI/FA==')}.should_not raise_error + end + + it "should not support whitespaces" do + proc { @class.new(:name => "whev", :type => :rsa, :user => "nobody", :key => 'AAA FA==')}.should raise_error(Puppet::Error,/Key must not contain whitespace/) + end + + end + + describe "for options" do + + it "should support flags as options" do + proc { @class.new(:name => "whev", :type => :rsa, :user => "nobody", :options => 'cert-authority')}.should_not raise_error + proc { @class.new(:name => "whev", :type => :rsa, :user => "nobody", :options => 'no-port-forwarding')}.should_not raise_error + end + + it "should support key-value pairs as options" do + proc { @class.new(:name => "whev", :type => :rsa, :user => "nobody", :options => 'command="command"')}.should_not raise_error + end + + it "should support environments as options" do + proc { @class.new(:name => "whev", :type => :rsa, :user => "nobody", :options => 'environment="NAME=value"')}.should_not raise_error + end + + it "should support multiple options as an array" do + proc { @class.new(:name => "whev", :type => :rsa, :user => "nobody", :options => ['cert-authority','environment="NAME=value"'])}.should_not raise_error + end + + it "should not support a comma separated lists" do + proc { @class.new(:name => "whev", :type => :rsa, :user => "nobody", :options => 'cert-authority,no-port-forwarding')}.should raise_error(Puppet::Error, /must be provided as an array/) + end + + it "should use :absent as a default value" do + @class.new(:name => "whev", :type => :rsa, :user => "nobody").should(:options).should == [:absent] + end + + it "property should return well formed string of arrays from is_to_s" do + resource = @class.new(:name => "whev", :type => :rsa, :user => "nobody", :options => ["a","b","c"]) + resource.property(:options).is_to_s(["a","b","c"]).should == "a,b,c" + end + + it "property should return well formed string of arrays from is_to_s" do + resource = @class.new(:name => "whev", :type => :rsa, :user => "nobody", :options => ["a","b","c"]) + resource.property(:options).should_to_s(["a","b","c"]).should == "a,b,c" + end + + end + + describe "for user" do + + it "should support present users" do + proc { @class.new(:name => "whev", :type => :rsa, :user => "root") }.should_not raise_error + end + + it "should support absent users" do + proc { @class.new(:name => "whev", :type => :rsa, :user => "ihopeimabsent") }.should_not raise_error + end + + end + + describe "for target" do + + it "should support absolute paths" do + proc { @class.new(:name => "whev", :type => :rsa, :target => "/tmp/here") }.should_not raise_error + end + + it "should use the user's path if not explicitly specified" do + @class.new(:name => "whev", :user => 'root').should(:target).should == File.expand_path("~root/.ssh/authorized_keys") + end + + it "should not consider the user's path if explicitly specified" do + @class.new(:name => "whev", :user => 'root', :target => '/tmp/here').should(:target).should == '/tmp/here' + end + + it "should inform about an absent user" do + Puppet::Log.level = :debug + @class.new(:name => "whev", :user => 'idontexist').should(:target) + @logs.map(&:message).should include("The required user is not yet present on the system") + end + + end - it "should have a target property" do - @class.attrtype(:target).should == :property end describe "when neither user nor target is specified" do + it "should raise an error" do proc do - - @class.create( - + @class.new( :name => "Test", :key => "AAA", :type => "ssh-rsa", - :ensure => :present) - end.should raise_error(Puppet::Error) + end.should raise_error(Puppet::Error,/user.*or.*target.*mandatory/) end + end describe "when both target and user are specified" do - it "should use target" do - - resource = @class.create( + it "should use target" do + resource = @class.new( :name => "Test", :user => "root", - - :target => "/tmp/blah") + :target => "/tmp/blah" + ) resource.should(:target).should == "/tmp/blah" end + end describe "when user is specified" do - it "should determine target" do + it "should determine target" do resource = @class.create( - :name => "Test", - - :user => "root") + :user => "root" + ) target = File.expand_path("~root/.ssh/authorized_keys") resource.should(:target).should == target end @@ -135,17 +243,19 @@ describe ssh_authorized_key do target = File.expand_path("~root/.ssh/authorized_keys") resource.property(:target).safe_insync?(target).should == true end + end describe "when calling validate" do - it "should not crash on a non-existant user" do + it "should not crash on a non-existant user" do resource = @class.create( - :name => "Test", - - :user => "ihopesuchuserdoesnotexist") + :user => "ihopesuchuserdoesnotexist" + ) proc { resource.validate }.should_not raise_error end + end + end -- cgit From 15c6fc789494f93823bf566562b8da654a28a65e Mon Sep 17 00:00:00 2001 From: Stefan Schulte Date: Thu, 14 Apr 2011 07:51:26 +0200 Subject: (#7114) Add integration tests for authorized_key Add tests to show the issue that one key is not moved from one keyfile to another keyfile if target is not in sync. Reviewed-By: Nick Lewis Reviewed-By: Josh Cooper --- .../provider/ssh_authorized_key_spec.rb | 207 +++++++++++++++++++++ 1 file changed, 207 insertions(+) create mode 100644 spec/integration/provider/ssh_authorized_key_spec.rb diff --git a/spec/integration/provider/ssh_authorized_key_spec.rb b/spec/integration/provider/ssh_authorized_key_spec.rb new file mode 100644 index 000000000..902f9ad22 --- /dev/null +++ b/spec/integration/provider/ssh_authorized_key_spec.rb @@ -0,0 +1,207 @@ +#!/usr/bin/env ruby + +require 'spec_helper' +require 'puppet/file_bucket/dipper' + +describe "ssh_authorized_key provider (integration)" do + include PuppetSpec::Files + + before :each do + @fake_userfile = tmpfile('authorized_keys.user') + @fake_rootfile = tmpfile('authorized_keys.root') + + # few testkeys generated with ssh-keygen + @sample_rsa_keys = [ + 'AAAAB3NzaC1yc2EAAAADAQABAAAAgQCi18JBZOq10X3w4f67nVhO0O3s5Y1vHH4UgMSM3ZnQwbC5hjGyYSi9UULOoQQoQynI/a0I9NL423/Xk/XJVIKCHcS8q6V2Wmjd+fLNelOjxxoW6mbIytEt9rDvwgq3Mof3/m21L3t2byvegR00a+ikKbmInPmKwjeWZpexCIsHzQ==', # 1024 bit + 'AAAAB3NzaC1yc2EAAAADAQABAAAAgQDLClyvi3CsJw5Id6khZs2/+s11qOH4Gdp6iDioDsrIp0m8kSiPr71VGyQYAfPzzvHemHS7Xg0NkG1Kc8u9tRqBQfTvz7ubq0AT/g01+4P2hQ/soFkuwlUG/HVnnaYb6N0Qp5SHWvD5vBE2nFFQVpP5GrSctPtHSjzJq/i+6LYhmQ==', # 1024 bit + 'AAAAB3NzaC1yc2EAAAADAQABAAABAQDLygAO6txXkh9FNV8xSsBkATeqLbHzS7sFjGI3gt0Dx6q3LjyKwbhQ1RLf28kd5G6VWiXmClU/RtiPdUz8nrGuun++2mrxzrXrvpR9dq1lygLQ2wn2cI35dN5bjRMtXy3decs6HUhFo9MoNwX250rUWfdCyNPhGIp6OOfmjdy+UeLGNxq9wDx6i4bT5tVVSqVRtsEfw9+ICXchzl85QudjneVVpP+thriPZXfXA5eaGwAo/dmoKOIhUwF96gpdLqzNtrGQuxPbV80PTbGv9ZtAtTictxaDz8muXO7he9pXmchUpxUKtMFjHkL0FAZ9tRPmv3RA30sEr2fZ8+LKvnE50w0' #2048 Bit + ] + @sample_dsa_keys = [ + 'AAAAB3NzaC1kc3MAAACBAOPck2O8MIDSqxPSnvENt6tzRrKJ5oOhB6Nc6oEcWm+VEH1gvuxdiRqwoMgRwyEf1yUd+UAcLw3a6Jn+EtFyEBN/5WF+4Tt4xTxZ0Pfik2Wc5uqHbQ2dkmOoXiAOYPiD3JUQ1Xwm/J0CgetjitoLfzAGdCNhMqguqAuHcVJ78ZZbAAAAFQCIBKFYZ+I18I+dtgteirXh+VVEEwAAAIEAs1yvQ/wnLLrRCM660pF4kBiw3D6dJfMdCXWQpn0hZmkBQSIzZv4Wuk3giei5luxscDxNc+y3CTXtnyG4Kt1Yi2sOdvhRI3rX8tD+ejn8GHazM05l5VIo9uu4AQPIE32iV63IqgApSBbJ6vDJW91oDH0J492WdLCar4BS/KE3cRwAAACBAN0uSDyJqYLRsfYcFn4HyVf6TJxQm1IcwEt6GcJVzgjri9VtW7FqY5iBqa9B9Zdh5XXAYJ0XLsWQCcrmMHM2XGHGpA4gL9VlCJ/0QvOcXxD2uK7IXwAVUA7g4V4bw8EVnFv2Flufozhsp+4soo1xiYc5jiFVHwVlk21sMhAtKAeF' # 1024 Bit + ] + + @sample_lines = [ + "ssh-rsa #{@sample_rsa_keys[1]} root@someotherhost", + "ssh-dss #{@sample_dsa_keys[0]} root@anywhere", + "ssh-rsa #{@sample_rsa_keys[2]} paul" + ] + + end + + after :each do + Puppet::Type::Ssh_authorized_key::ProviderParsed.clear # Work around bug #6628 + end + + def create_fake_key(username, content) + filename = (username == :root ? @fake_rootfile : @fake_userfile ) + File.open(filename, 'w') do |f| + content.each do |line| + f.puts line + end + end + end + + def check_fake_key(username, expected_content) + filename = (username == :root ? @fake_rootfile : @fake_userfile ) + content = File.readlines(filename).map(&:chomp).sort.reject{ |x| x =~ /^#|^$/ } + content.join("\n").should == expected_content.sort.join("\n") + end + + def run_in_catalog(*resources) + Puppet::FileBucket::Dipper.any_instance.stubs(:backup) # Don't backup to the filebucket + catalog = Puppet::Resource::Catalog.new + catalog.host_config = false + resources.each do |resource| + resource.expects(:err).never + catalog.add_resource(resource) + end + catalog.apply + end + + describe "when managing one resource" do + + before :each do + # We are not running as root so chown/chmod is not possible + File.stubs(:chown) + File.stubs(:chmod) + Puppet::Util::SUIDManager.stubs(:asuser).yields + end + + describe "with ensure set to absent" do + + before :each do + @example = Puppet::Type.type(:ssh_authorized_key).new( + :name => 'root@hostname', + :type => :rsa, + :key => @sample_rsa_keys[0], + :target => @fake_rootfile, + :user => 'root', + :ensure => :absent + ) + end + + it "should not modify root's keyfile if resource is currently not present" do + create_fake_key(:root, @sample_lines) + run_in_catalog(@example) + check_fake_key(:root, @sample_lines) + end + + it "remove the key from root's keyfile if resource is currently present" do + create_fake_key(:root, @sample_lines + ["ssh-rsa #{@sample_rsa_keys[0]} root@hostname"]) + run_in_catalog(@example) + check_fake_key(:root, @sample_lines) + end + + end + + describe "when ensure is present" do + + before :each do + @example = Puppet::Type.type(:ssh_authorized_key).new( + :name => 'root@hostname', + :type => :rsa, + :key => @sample_rsa_keys[0], + :target => @fake_rootfile, + :user => 'root', + :ensure => :present + ) + + # just a dummy so the parsedfile provider is aware + # of the user's authorized_keys file + @dummy = Puppet::Type.type(:ssh_authorized_key).new( + :name => 'dummy', + :target => @fake_userfile, + :user => 'nobody', + :ensure => :absent + ) + end + + it "should add the key if it is not present" do + create_fake_key(:root, @sample_lines) + run_in_catalog(@example) + check_fake_key(:root, @sample_lines + ["ssh-rsa #{@sample_rsa_keys[0]} root@hostname" ]) + end + + it "should modify the type if type is out of sync" do + create_fake_key(:root,@sample_lines + [ "ssh-dss #{@sample_rsa_keys[0]} root@hostname" ]) + run_in_catalog(@example) + check_fake_key(:root, @sample_lines + [ "ssh-rsa #{@sample_rsa_keys[0]} root@hostname" ]) + end + + it "should modify the key if key is out of sync" do + create_fake_key(:root,@sample_lines + [ "ssh-rsa #{@sample_rsa_keys[1]} root@hostname" ]) + run_in_catalog(@example) + check_fake_key(:root, @sample_lines + [ "ssh-rsa #{@sample_rsa_keys[0]} root@hostname" ]) + end + + it "should remove the key from old file if target is out of sync" do + create_fake_key(:user, [ @sample_lines[0], "ssh-rsa #{@sample_rsa_keys[0]} root@hostname" ]) + create_fake_key(:root, [ @sample_lines[1], @sample_lines[2] ]) + run_in_catalog(@example, @dummy) + check_fake_key(:user, [ @sample_lines[0] ]) + #check_fake_key(:root, [ @sample_lines[1], @sample_lines[2], "ssh-rsa #{@sample_rsa_keys[0]} root@hostname" ]) + end + + it "should add the key to new file if target is out of sync" do + create_fake_key(:user, [ @sample_lines[0], "ssh-rsa #{@sample_rsa_keys[0]} root@hostname" ]) + create_fake_key(:root, [ @sample_lines[1], @sample_lines[2] ]) + run_in_catalog(@example, @dummy) + #check_fake_key(:user, [ @sample_lines[0] ]) + check_fake_key(:root, [ @sample_lines[1], @sample_lines[2], "ssh-rsa #{@sample_rsa_keys[0]} root@hostname" ]) + end + + it "should modify options if options are out of sync" do + @example[:options]=[ 'from="correct.domain.com"', 'no-port-forwarding', 'no-pty' ] + create_fake_key(:root, @sample_lines + [ "from=\"incorrect.domain.com\",no-port-forwarding,no-pty ssh-rsa #{@sample_rsa_keys[0]} root@hostname"]) + run_in_catalog(@example) + check_fake_key(:root, @sample_lines + [ "from=\"correct.domain.com\",no-port-forwarding,no-pty ssh-rsa #{@sample_rsa_keys[0]} root@hostname"] ) + end + + end + + end + + describe "when managing two resource" do + + before :each do + # We are not running as root so chown/chmod is not possible + File.stubs(:chown) + File.stubs(:chmod) + Puppet::Util::SUIDManager.stubs(:asuser).yields + @example_one = Puppet::Type.type(:ssh_authorized_key).new( + :name => 'root@hostname', + :type => :rsa, + :key => @sample_rsa_keys[0], + :target => @fake_rootfile, + :user => 'root', + :ensure => :present + ) + + @example_two = Puppet::Type.type(:ssh_authorized_key).new( + :name => 'user@hostname', + :key => @sample_rsa_keys[1], + :type => :rsa, + :target => @fake_userfile, + :user => 'nobody', + :ensure => :present + ) + end + + describe "and both keys are absent" do + + before :each do + create_fake_key(:root, @sample_lines) + create_fake_key(:user, @sample_lines) + end + + it "should add both keys" do + run_in_catalog(@example_one, @example_two) + check_fake_key(:root, @sample_lines + [ "ssh-rsa #{@sample_rsa_keys[0]} root@hostname" ]) + check_fake_key(:user, @sample_lines + [ "ssh-rsa #{@sample_rsa_keys[1]} user@hostname" ]) + end + + end + + end + +end -- cgit From 551cb3e5ee6c1ef4218adcebf04004c50fe4119f Mon Sep 17 00:00:00 2001 From: Stefan Schulte Date: Sat, 23 Apr 2011 10:30:09 +0200 Subject: (#7114) Target returns correct value Fix the ssh_authorized_key parsedfile provider to return the current target value instead of the should value. Without this change puppet always thinks that the target property is in sync and thus will never move one key to the correct file. Reviewed-By: Nick Lewis Reviewed-By: Josh Cooper --- lib/puppet/provider/ssh_authorized_key/parsed.rb | 6 ------ 1 file changed, 6 deletions(-) diff --git a/lib/puppet/provider/ssh_authorized_key/parsed.rb b/lib/puppet/provider/ssh_authorized_key/parsed.rb index 6a3855c0e..81b1fbcfa 100644 --- a/lib/puppet/provider/ssh_authorized_key/parsed.rb +++ b/lib/puppet/provider/ssh_authorized_key/parsed.rb @@ -42,12 +42,6 @@ require 'puppet/provider/parsedfile' 0600 end - def target - @resource.should(:target) || File.expand_path("~#{@resource.should(:user)}/.ssh/authorized_keys") - rescue - raise Puppet::Error, "Target not defined and/or specified user does not exist yet" - end - def user uid = File.stat(target).uid Etc.getpwuid(uid).name -- cgit From d22b1303de9a0dbc2c5b887eab0c9e146fcf13c6 Mon Sep 17 00:00:00 2001 From: Nick Lewis Date: Mon, 16 May 2011 16:32:18 -0700 Subject: (#7114) Fix specs for ssh authorized key parsed provider These tests were unnecessarily creating a stub resource, as well as failing to re-initvars on the provider before each test. This led to test ordering failures, and incompatibilities with the other commits in this series. Now we use a real resource, and properly reinitialize before each test. Paired-With: Josh Cooper --- .../provider/ssh_authorized_key/parsed_spec.rb | 53 +++++++++------------- 1 file changed, 21 insertions(+), 32 deletions(-) diff --git a/spec/unit/provider/ssh_authorized_key/parsed_spec.rb b/spec/unit/provider/ssh_authorized_key/parsed_spec.rb index 69d29c674..bd5e55a9e 100755 --- a/spec/unit/provider/ssh_authorized_key/parsed_spec.rb +++ b/spec/unit/provider/ssh_authorized_key/parsed_spec.rb @@ -9,23 +9,19 @@ describe provider_class do include PuppetSpec::Files before :each do - @sshauthkey_class = Puppet::Type.type(:ssh_authorized_key) - @provider = @sshauthkey_class.provider(:parsed) @keyfile = tmpfile('authorized_keys') - @provider.any_instance.stubs(:target).returns @keyfile + @provider_class = provider_class + @provider_class.initvars + @provider_class.any_instance.stubs(:target).returns @keyfile @user = 'random_bob' Puppet::Util.stubs(:uid).with(@user).returns 12345 end - after :each do - @provider.initvars - end - def mkkey(args) args[:target] = @keyfile args[:user] = @user resource = Puppet::Type.type(:ssh_authorized_key).new(args) - key = @provider.new(resource) + key = @provider_class.new(resource) args.each do |p,v| key.send(p.to_s + "=", v) end @@ -33,26 +29,26 @@ describe provider_class do end def genkey(key) - @provider.stubs(:filetype).returns(Puppet::Util::FileType::FileTypeRam) + @provider_class.stubs(:filetype).returns(Puppet::Util::FileType::FileTypeRam) File.stubs(:chown) File.stubs(:chmod) Puppet::Util::SUIDManager.stubs(:asuser).yields key.flush - @provider.target_object(@keyfile).read + @provider_class.target_object(@keyfile).read end it_should_behave_like "all parsedfile providers", provider_class it "should be able to generate a basic authorized_keys file" do - key = mkkey(:name => "Just Testing", + key = mkkey(:name => "Just_Testing", :key => "AAAAfsfddsjldjgksdflgkjsfdlgkj", :type => "ssh-dss", :ensure => :present, :options => [:absent] ) - genkey(key).should == "ssh-dss AAAAfsfddsjldjgksdflgkjsfdlgkj Just Testing\n" + genkey(key).should == "ssh-dss AAAAfsfddsjldjgksdflgkjsfdlgkj Just_Testing\n" end it "should be able to generate a authorized_keys file with options" do @@ -71,25 +67,25 @@ describe provider_class do options = %w{from="host1.reductlivelabs.com,host.reductivelabs.com" command="/usr/local/bin/run" ssh-pty} optionstr = options.join(", ") - @provider.parse_options(optionstr).should == options + @provider_class.parse_options(optionstr).should == options end it "should use '' as name for entries that lack a comment" do line = "ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEAut8aOSxenjOqF527dlsdHWV4MNoAsX14l9M297+SQXaQ5Z3BedIxZaoQthkDALlV/25A1COELrg9J2MqJNQc8Xe9XQOIkBQWWinUlD/BXwoOTWEy8C8zSZPHZ3getMMNhGTBO+q/O+qiJx3y5cA4MTbw2zSxukfWC87qWwcZ64UUlegIM056vPsdZWFclS9hsROVEa57YUMrehQ1EGxT4Z5j6zIopufGFiAPjZigq/vqgcAqhAKP6yu4/gwO6S9tatBeEjZ8fafvj1pmvvIplZeMr96gHE7xS3pEEQqnB3nd4RY7AF6j9kFixnsytAUO7STPh/M3pLiVQBN89TvWPQ==" - @provider.parse(line)[0][:name].should == "" + @provider_class.parse(line)[0][:name].should == "" end end describe provider_class do before :each do - @resource = stub("resource", :name => "foo") - @resource.stubs(:[]).returns "foo" - @resource.class.stubs(:key_attributes).returns( [:name] ) + @resource = Puppet::Type.type(:ssh_authorized_key).new(:name => "foo", :user => "random_bob") @provider = provider_class.new(@resource) provider_class.stubs(:filetype).returns(Puppet::Util::FileType::FileTypeRam) Puppet::Util::SUIDManager.stubs(:asuser).yields + + provider_class.initvars end describe "when flushing" do @@ -103,9 +99,9 @@ describe provider_class do describe "and both a user and a target have been specified" do before :each do Puppet::Util.stubs(:uid).with("random_bob").returns 12345 - @resource.stubs(:should).with(:user).returns "random_bob" + @resource[:user] = "random_bob" target = "/tmp/.ssh_dir/place_to_put_authorized_keys" - @resource.stubs(:should).with(:target).returns target + @resource[:target] = target end it "should create the directory" do @@ -134,8 +130,7 @@ describe provider_class do describe "and a user has been specified with no target" do before :each do - @resource.stubs(:should).with(:user).returns "nobody" - @resource.stubs(:should).with(:target).returns nil + @resource[:user] = "nobody" # # I'd like to use random_bob here and something like # @@ -186,26 +181,20 @@ describe provider_class do end describe "and a target has been specified with no user" do - before :each do - @resource.stubs(:should).with(:user).returns nil - @resource.stubs(:should).with(:target).returns("/tmp/.ssh_dir/place_to_put_authorized_keys") - end - it "should raise an error" do + @resource = Puppet::Type.type(:ssh_authorized_key).new(:name => "foo", :target => "/tmp/.ssh_dir/place_to_put_authorized_keys") + @provider = provider_class.new(@resource) + proc { @provider.flush }.should raise_error end end describe "and a invalid user has been specified with no target" do - before :each do - @resource.stubs(:should).with(:user).returns "thisusershouldnotexist" - @resource.stubs(:should).with(:target).returns nil - end - it "should catch an exception and raise a Puppet error" do + @resource[:user] = "thisusershouldnotexist" + lambda { @provider.flush }.should raise_error(Puppet::Error) end end - end end -- cgit