From ce233aa2a511bf6818f28c226144ec5b05a468ee Mon Sep 17 00:00:00 2001 From: Markus Roberts Date: Wed, 28 Apr 2010 15:39:39 -0700 Subject: Write ssh_authorized_keys as user This is a targeted fix to the issue of permissions when writing ssh authorized key files by 1) requiring that an existing users be specified on the resource and 2) doing the write as that user. It's based on Michael DeHaan's initial implementation of Luke's idea, but with a number of simplifications (mostly by testing necessary conditions as early as possible so the code isn't cluttered up with a lot of checks). --- lib/puppet/provider/ssh_authorized_key/parsed.rb | 38 +++++-------------- spec/unit/provider/ssh_authorized_key/parsed.rb | 47 +++++++++++++++--------- 2 files changed, 38 insertions(+), 47 deletions(-) diff --git a/lib/puppet/provider/ssh_authorized_key/parsed.rb b/lib/puppet/provider/ssh_authorized_key/parsed.rb index b435c513c..fb4d0956e 100644 --- a/lib/puppet/provider/ssh_authorized_key/parsed.rb +++ b/lib/puppet/provider/ssh_authorized_key/parsed.rb @@ -62,36 +62,16 @@ Puppet::Type.type(:ssh_authorized_key).provide(:parsed, end def flush - # As path expansion had to be moved in the provider, we cannot generate new file - # resources and thus have to chown and chmod here. It smells hackish. - - # Create target's parent directory if nonexistant - if target - dir = File.dirname(target) - if not File.exist? dir - Puppet.debug("Creating directory %s which did not exist" % dir) - Dir.mkdir(dir, dir_perm) - end - end - - # Generate the file - super - - # Ensure correct permissions - if target and user - uid = Puppet::Util.uid(user) - - if uid - File.chown(uid, nil, dir) - File.chown(uid, nil, target) - else - raise Puppet::Error, "Specified user does not exist" - end - end - - if target - File.chmod(file_perm, target) + raise Puppet::Error, "Cannot write SSH authorized keys without user" unless user + raise Puppet::Error, "User '#{user}' does not exist" unless uid = Puppet::Util.uid(user) + unless File.exist?(dir = File.dirname(target)) + Puppet.debug "Creating #{dir}" + Dir.mkdir(dir, dir_perm) + File.chown(uid, nil, dir) end + Puppet::Util::SUIDManager.asuser(user) { super } + File.chown(uid, nil, target) + File.chmod(file_perm, target) end # parse sshv2 option strings, wich is a comma separated list of diff --git a/spec/unit/provider/ssh_authorized_key/parsed.rb b/spec/unit/provider/ssh_authorized_key/parsed.rb index d5c66d7b0..fc3f55044 100755 --- a/spec/unit/provider/ssh_authorized_key/parsed.rb +++ b/spec/unit/provider/ssh_authorized_key/parsed.rb @@ -15,6 +15,9 @@ describe provider_class do before :each do @sshauthkey_class = Puppet::Type.type(:ssh_authorized_key) @provider = @sshauthkey_class.provider(:parsed) + @keyfile = File.join(tmpdir, 'authorized_keys') + @user = 'random_bob' + Puppet::Util.stubs(:uid).with(@user).returns 12345 end after :each do @@ -23,22 +26,23 @@ describe provider_class do def mkkey(args) fakeresource = fakeresource(:ssh_authorized_key, args[:name]) + fakeresource.stubs(:should).with(:user).returns @user + fakeresource.stubs(:should).with(:target).returns @keyfile key = @provider.new(fakeresource) args.each do |p,v| key.send(p.to_s + "=", v) end - return key + key end def genkey(key) @provider.stubs(:filetype).returns(Puppet::Util::FileType::FileTypeRam) - file = @provider.default_target - + File.stubs(:chown) + File.stubs(:chmod) key.flush - text = @provider.target_object(file).read - return text + @provider.target_object(@keyfile).read end PuppetTest.fakedata("data/providers/ssh_authorized_key/parsed").each { |file| @@ -147,20 +151,35 @@ describe provider_class do # but mocha objects strenuously to stubbing File.expand_path # so I'm left with using nobody. @dir = File.expand_path("~nobody/.ssh") - end + end - it "should create the directory" do + it "should create the directory if it doesn't exist" do File.stubs(:exist?).with(@dir).returns false Dir.expects(:mkdir).with(@dir,0700) @provider.flush end - it "should chown the directory to the user" do + it "should not create or chown the directory if it already exist" do + File.stubs(:exist?).with(@dir).returns false + Dir.expects(:mkdir).never + @provider.flush + end + + it "should chown the directory to the user if it creates it" do + File.stubs(:exist?).with(@dir).returns false + Dir.stubs(:mkdir).with(@dir,0700) uid = Puppet::Util.uid("nobody") File.expects(:chown).with(uid, nil, @dir) @provider.flush end + it "should not create or chown the directory if it already exist" do + File.stubs(:exist?).with(@dir).returns false + Dir.expects(:mkdir).never + File.expects(:chown).never + @provider.flush + end + it "should chown the key file to the user" do uid = Puppet::Util.uid("nobody") File.expects(:chown).with(uid, nil, File.expand_path("~nobody/.ssh/authorized_keys")) @@ -179,17 +198,9 @@ describe provider_class do @resource.stubs(:should).with(:target).returns("/tmp/.ssh_dir/place_to_put_authorized_keys") end - it "should make the directory" do - File.stubs(:exist?).with("/tmp/.ssh_dir").returns false - Dir.expects(:mkdir).with("/tmp/.ssh_dir", 0755) - @provider.flush - end - - it "should chmod the key file to 0644" do - File.expects(:chmod).with(0644, "/tmp/.ssh_dir/place_to_put_authorized_keys") - @provider.flush + it "should raise an error" do + proc { @provider.flush }.should raise_error end end - end end -- cgit