Puppet

My Environment¶

Facter: 1.6.0 
Puppet: 2.7.3rc1
OS: CentOS Linux release 6.0 

Test Run¶

Using the following block:

ssh_authorized_key { 'puppet_authorized_keys':
  ensure => present,
  key    => 'AAAAB3NzaC1yc2EAAAABIwAAAQEA2lB0+BZW2XPO6QMMgVfdUmJHfrrcT0Wo0gHA8xhZcArVY44hijn==',
  name   => 'puppet@example.com',
  target => '/var/lib/public-keys/puppet.pub',
  type   => 'ssh-rsa',
  user   => 'puppet',
}

It appears the the directory /var/lib/public-keys and the authorized_keys file /var/lib/public-keys/puppet.pub are being created as root.

[root@agent ~]# rm -rf /var/lib/public-keys
[root@agent ~]# puppet agent --test
info: Caching catalog for agent.lab.com
info: Applying configuration version '1313303287'
notice: /Stage[main]/Sshkeys/Ssh_authorized_key[puppet_authorized_keys]/ensure: created
notice: Finished catalog run in 0.25 seconds

[root@agent ~]# ls -l /var/lib
drwx------   2 puppet root 4096 Aug 13 05:47 public-keys

[root@agent ~]# ls -l /var/lib/public-keys
total 4
-rw------- 1 puppet root 578 Aug 13 05:47 puppet.pub

Try this instead:

  ssh_authorized_key { 'puppet_authorized_keys':
    ensure => present,
    key    => 'AAAAB3NzaC1yc2EAAAABIwAAAQEA2lB0+BZW2XPO6QMMgVfdUmJHfrrcT0Wo0gHA8xhZcArVY44hijn==',
    name   => 'puppet@example.com',
    target => '/tmp/public-keys/puppet.pub',
    type   => 'ssh-rsa',
    user   => 'puppet',
  }

  file { '/tmp/public-keys':
    ensure => 'directory',
    owner => 'root',
    group => 'root',
    mode => '644',
    notify => Ssh_authorized_key['puppet_authorized_keys']
  }

After updating my init.pp I can confirm this issue. It was only working for me because the module was creating /var/lib/public-keys before writing puppet.pub.

[root@agent ~]# puppet agent --test
info: Caching catalog for agent.lab.com
info: Applying configuration version '1313370151'
notice: /Stage[main]/Sshkeys/File[/tmp/public-keys]/ensure: created
notice: /Stage[main]/Sshkeys/Ssh_authorized_key[puppet_authorized_keys]/ensure: created
err: /Stage[main]/Sshkeys/Ssh_authorized_key[puppet_authorized_keys]: Could not evaluate: Puppet::Util::FileType::FileTypeFlat could not write /tmp/public-keys/puppet.pub: Permission denied - /tmp/public-keys/puppet.pub

Trying to narrow down the source of this issue, I modified /lib/puppet/provider/ssh_authorized_key/parsed.rb:

diff --git a/lib/puppet/provider/ssh_authorized_key/parsed.rb b/lib/puppet/provider/ssh_authorized_key/parsed.rb
index 81b1fbc..17428fc 100644
--- a/lib/puppet/provider/ssh_authorized_key/parsed.rb
+++ b/lib/puppet/provider/ssh_authorized_key/parsed.rb
@@ -62,7 +62,8 @@ require 'puppet/provider/parsedfile'
     # so calling it here supresses the later attempt by our superclass's flush method.
     self.class.backup_target(target)

-    Puppet::Util::SUIDManager.asuser(@resource.should(:user)) { super }
+    # Puppet::Util::SUIDManager.asuser(@resource.should(:user)) { super }
+    Puppet::Util::SUIDManager.asuser('root') { super }
     File.chown(uid, nil, target)
     File.chmod(file_perm, target)
 end

Above I have hard-coded the ‘root’ user in place of the user defined in the resource block. There needs to be a discussion on whether or not to limit the ssh_authorized_key type to updating authorized_keys in user’s home directories, or make it more generic to support managing authorized_keys anywhere.

If a more generic approach is desired then puppet will have to remain as the “root” user or equivalent when creating the ssh_authorized_key resource.

After reviewing the code further, it appears ssh_authorized_key requires that a user be available on the system before trying to manage a ssh_authorized_key resource. The very act of setting the target to a location not writable by the user specified within the ssh_authorized_key block it what is causing the issue.

Based on the way this resource type is defined, there are some unclear assumptions being made, one of them is that the target must be a location that is writable by the specified user, preferably the default: $HOME/.ssh/authorized_keys location. While exposed, the target parameter is more of a convenience, only there to allow adjusting small variations to this assumption.

Possible solutions include:

• update the documentation to add clarity around the above assumptions and limitations
• Refactor: remove the requirement that a user exist and treat the target like any other file resource: set the owner, group, and mode as required.

I would like to vote for the second method.

There’s absolutely no reason that the user has to exist before writing the file or that the file has to be owned by the user at all. The only requirement is that it can be read by the user which can be done with a group setting or a world readable mode.

Perhaps a failure case would be appropriate where, if the user doesn’t exist, the file is set to world readable by default.

I am not sure what the process is for getting some one to review this, but it should not be much work to get this refactored. I am going to work on a patch with tests in the meanwhile

Pull request has been sent via github.

After looking at the options around this, the security issues involved, and the documentation, my proposal is that we will merge the change proposed by Ricky Zhou, which will result in all the file operations being performed with the privileges of the target user.

For the most common situations this is the correct behaviour, and for most uncommon situations the behaviour of setting user => 'root' should take care of things; OpenSSH accepts root ownership of the authorized_keys file.

For situations where other behaviour is required – user owned, but not in a user writable directory, fundamentally – I am going to recommend using a simple file command to install the file with the required content.

Unless we here objections this will probably be implemented next Friday as proposed.

I wonder if we could add a note somewhere in the “docs” about this recommendation.

+1 on using a file type with the required content.

Nick – can you please add a note.

I’m going to need some more context about what exactly has changed and what needs to be documented. I’ll be in charge of following up with Daniel, and will make a note to do so the week after puppetconf.

I agree with Daniel here. And given that the patch is already in 2.7.5 as of commit:b29b1785 anything that speaks against closing this one?