(MODULES-9578) Create authorized_key in root path

GabrielNagy · GabrielNagy · commit b2c153b6ff07 · 2019-10-23T12:23:47.000+03:00
Previously, when the `target` property was set, the ssh_authorized_key
resource could not create directories/files within root-owned paths.
This behavior is due to the module switching context to the user, then
attempting to create the directory/file as the specified user,
ultimately failing because of insufficient permissions.

This commit adds a new parameter, `drop_privileges` which when set to
false allows the module to write a ssh_authorized_key file in a
privileged path. Due to the possible security implications of this,
the parameter must be manually specified in order to activate this
functionality.

A path is considered to be privileged/trusted if all of its ancestors:
- do not contain any symlinks
- have the same owner as the user who runs Puppet
- are not world/group writable
diff --git a/REFERENCE.md b/REFERENCE.md
@@ -85,7 +85,8 @@ will autorequire this user if it is being managed as a `user` resource.
 The absolute filename in which to store the SSH key. This
 property is optional and should be used only in cases where keys
 are stored in a non-standard location, for instance when not in
-`~user/.ssh/authorized_keys`.
+`~user/.ssh/authorized_keys`. The parent directory must be present
+if the target is in a privileged path.
 
 Default value: absent
 
@@ -119,6 +120,14 @@ Due to internal limitations, this must be unique across all user accounts;
 if you want to specify one key for multiple users, you must use a different
 comment for each instance.
 
+##### `drop_privileges`
+
+Whether to drop privileges when writing the key file. This is
+useful for creating files in paths not writable by the target user. Note
+the possible security implications of managing file ownership and
+permissions as a privileged user.
+
+Default value: `true`
 
 ### sshkey
 
diff --git a/lib/puppet/provider/ssh_authorized_key/parsed.rb b/lib/puppet/provider/ssh_authorized_key/parsed.rb
@@ -42,9 +42,28 @@ def file_perm
     0o600
   end
 
-  def user
-    uid = Puppet::FileSystem.stat(target).uid
-    Etc.getpwuid(uid).name
+  def group_writable_perm
+    0o020
+  end
+
+  def group_writable?(path)
+    path.stat.mode & group_writable_perm != 0
+  end
+
+  def trusted_path
+    # return if the parent directory does not exist
+    return false unless Puppet::FileSystem.dir_exist?(target)
+    path = Puppet::FileSystem.pathname(target).dirname
+    until path.dirname.root?
+      path = path.realpath if path.symlink?
+      # do not trust if path is world or group writable
+      if path.stat.uid != Process.euid || path.world_writable? || group_writable?(path)
+        Puppet.debug('Path untrusted, will attempt to write as the target user')
+        return false
+      end
+      path = path.dirname
+    end
+    Puppet.debug('Path trusted, writing the file as the current user')
   end
 
   def flush
@@ -56,15 +75,30 @@ def flush
     # so calling it here suppresses the later attempt by our superclass's flush method.
     self.class.backup_target(target)
 
-    Puppet::Util::SUIDManager.asuser(@resource.should(:user)) do
-      unless Puppet::FileSystem.exist?(dir = File.dirname(target))
-        Puppet.debug "Creating #{dir} as #{@resource.should(:user)}"
-        Dir.mkdir(dir, dir_perm)
-      end
+    # attempt to create the file as the specified user if we're not dropping privileges
+    if @resource[:drop_privileges]
+      Puppet::Util::SUIDManager.asuser(@resource.should(:user)) do
+        unless Puppet::FileSystem.exist?(dir = File.dirname(target))
+          Puppet.debug "Creating #{dir} as #{@resource.should(:user)}"
+          Dir.mkdir(dir, dir_perm)
+        end
+        super
 
+        File.chmod(file_perm, target)
+      end
+    # to avoid race conditions when handling permissions as a privileged user
+    # (CVE-2011-3870) we use the trusted_path method to ensure the entire
+    # directory structure is "safe" to write in
+    else
+      raise Puppet::Error, 'drop_privileges is false but the target path is not trusted' unless trusted_path
       super
 
-      File.chmod(file_perm, target)
+      uid = Puppet::Util.uid(@resource.should(:user))
+      gid = Puppet::Util.gid(@resource.should(:user))
+      File.open(target) do |target|
+        target.chown(uid, gid)
+        target.chmod(file_perm)
+      end
     end
   end
 
diff --git a/lib/puppet/type/ssh_authorized_key.rb b/lib/puppet/type/ssh_authorized_key.rb
@@ -1,3 +1,5 @@
+require 'puppet/parameter/boolean'
+
 module Puppet
   Type.newtype(:ssh_authorized_key) do
     @doc = "Manages SSH authorized keys. Currently only type 2 keys are supported.
@@ -48,6 +50,15 @@ module Puppet
       isnamevar
     end
 
+    newparam(:drop_privileges, boolean: true, parent: Puppet::Parameter::Boolean) do
+      desc "Whether to drop privileges when writing the key file. This is
+        useful for creating files in paths not writable by the target user. Note
+        the possible security implications of managing file ownership and
+        permissions as a privileged user."
+
+      defaultto true
+    end
+
     newproperty(:type) do
       desc 'The encryption type used.'
 
@@ -83,7 +94,8 @@ module Puppet
       desc "The absolute filename in which to store the SSH key. This
         property is optional and should be used only in cases where keys
         are stored in a non-standard location, for instance when not in
-        `~user/.ssh/authorized_keys`."
+        `~user/.ssh/authorized_keys`. The parent directory must be present
+        if the target is in a privileged path."
 
       defaultto :absent
 
diff --git a/spec/acceptance/tests/resource/ssh_authorized_key/create_spec.rb b/spec/acceptance/tests/resource/ssh_authorized_key/create_spec.rb
@@ -7,21 +7,18 @@
   let(:name) { "pl#{rand(999_999).to_i}" }
   let(:custom_key_directory) { "/etc/ssh_authorized_keys_#{name}" }
   let(:custom_key) { "#{custom_key_directory}/authorized_keys_#{name}" }
-  let(:custom_name) { "custom_#{name}" }
 
   before(:each) do
     posix_agents.each do |agent|
-      on(agent, "cp #{auth_keys} /tmp/auth_keys", acceptable_exit_codes: [0, 1])
+      on(agent, "cp -a #{auth_keys} /tmp/auth_keys", acceptable_exit_codes: [0, 1])
       on(agent, "rm -f #{auth_keys}")
-      on(agent, "mkdir #{custom_key_directory}")
     end
   end
 
   after(:each) do
     posix_agents.each do |agent|
       # (teardown) restore the #{auth_keys} file
       on(agent, "mv /tmp/auth_keys #{auth_keys}", acceptable_exit_codes: [0, 1])
-      on(agent, "rm -rf #{custom_key_directory}")
     end
   end
 
@@ -37,17 +34,54 @@
         fail_test "didn't find the ssh_authorized_key for #{name}" unless stdout.include? name.to_s
       end
     end
-    it "#{agent} should create an entry for an SSH authorized key in a custom location" do
-      custom_args = ['ensure=present',
-                     'user=$LOGNAME',
-                     "type='rsa'",
-                     "key='mykey'",
-                     "target='#{custom_key}'"]
 
-      on(agent, puppet_resource('ssh_authorized_key', custom_name.to_s, custom_args))
+    it "#{agent} should create an entry for an SSH authorized key in a custom location" do
+      on(agent, "mkdir #{custom_key_directory}")
+      args = ['ensure=present',
+              'user=$LOGNAME',
+              "type='rsa'",
+              "key='mykey'",
+              "target='#{custom_key}'"]
+      on(agent, puppet_resource('ssh_authorized_key', name.to_s, args))
 
       on(agent, "cat #{custom_key}") do |_res|
-        fail_test "didn't find the ssh_authorized_key for #{custom_name}" unless stdout.include? name.to_s
+        fail_test "didn't find the ssh_authorized_key for #{name}" unless stdout.include? name.to_s
+      end
+      on(agent, "rm -rf #{custom_key_directory}")
+    end
+
+    it "#{agent} should fail if target user doesn't have permissions for symlinked path" do
+      # create a dummy user
+      on(agent, puppet_resource('user', 'testuser', 'ensure=present', 'managehome=true'))
+
+      on(agent, "mkdir #{custom_key_directory}")
+
+      # as the user, symlink an owned directory to something inside /root
+      on(agent, puppet_resource('file', '/home/testuser/tmp', ['ensure=/etc', 'owner=testuser']))
+      args = ['ensure=present',
+              'user=testuser',
+              "type='rsa'",
+              "key='mykey'",
+              'drop_privileges=false',
+              "target=/home/testuser/tmp/ssh_authorized_keys_#{name}/authorized_keys_#{name}"]
+      on(agent, puppet_resource('ssh_authorized_key', name.to_s, args)) do |_res|
+        fail_test unless stderr =~ %r{the target path is not trusted}
+      end
+      on(agent, "rm -rf #{custom_key_directory}")
+
+      # purge the user
+      on(agent, puppet_resource('user', 'testuser', 'ensure=absent'))
+    end
+
+    it "#{agent} should not create directories for SSH authorized key in a custom location" do
+      args = ['ensure=present',
+              'user=$LOGNAME',
+              "type='rsa'",
+              "key='mykey'",
+              'drop_privileges=false',
+              "target='#{custom_key}'"]
+      on(agent, puppet_resource('ssh_authorized_key', name.to_s, args), acceptable_exit_codes: [0, 1]) do |_res|
+        fail_test unless stderr =~ %r{the target path is not trusted}
       end
     end
   end
diff --git a/spec/acceptance/tests/resource/ssh_authorized_key/destroy_spec.rb b/spec/acceptance/tests/resource/ssh_authorized_key/destroy_spec.rb
@@ -5,13 +5,14 @@
 
   let(:auth_keys) { '~/.ssh/authorized_keys' }
   let(:name) { "pl#{rand(999_999).to_i}" }
+  let(:custom_key_directory) { "/etc/ssh_authorized_keys_#{name}" }
+  let(:custom_key) { "#{custom_key_directory}/authorized_keys_#{name}" }
 
   before(:each) do
     posix_agents.each do |agent|
-      on(agent, "cp #{auth_keys} /tmp/auth_keys", acceptable_exit_codes: [0, 1])
-
+      on(agent, "cp -a #{auth_keys} /tmp/auth_keys", acceptable_exit_codes: [0, 1])
+      on(agent, "rm -f #{auth_keys}")
       on(agent, "echo '' >> #{auth_keys} && echo 'ssh-rsa mykey #{name}' >> #{auth_keys}")
-      on(agent, "chown $LOGNAME #{auth_keys}")
     end
   end
 
@@ -34,5 +35,21 @@
         expect(stdout).not_to include(name.to_s)
       end
     end
+
+    it "#{agent} should delete an entry for an SSH authorized key in a custom location" do
+      on(agent, "mkdir #{custom_key_directory}")
+      on(agent, "echo '' >> #{custom_key} && echo 'ssh-rsa mykey #{name}' >> #{custom_key}")
+      args = ['ensure=absent',
+              'user=$LOGNAME',
+              "type='rsa'",
+              "key='mykey'",
+              "target='#{custom_key}'"]
+      on(agent, puppet_resource('ssh_authorized_key', name.to_s, args))
+
+      on(agent, "cat #{custom_key}") do |_res|
+        expect(stdout).not_to include(name.to_s)
+      end
+      on(agent, "rm -rf #{custom_key_directory}")
+    end
   end
 end
diff --git a/spec/acceptance/tests/resource/ssh_authorized_key/modify_spec.rb b/spec/acceptance/tests/resource/ssh_authorized_key/modify_spec.rb
@@ -3,12 +3,14 @@
 RSpec.context 'sshkeys: Modify' do
   let(:auth_keys) { '~/.ssh/authorized_keys' }
   let(:name) { "pl#{rand(999_999).to_i}" }
+  let(:custom_key_directory) { "/etc/ssh_authorized_keys_#{name}" }
+  let(:custom_key) { "#{custom_key_directory}/authorized_keys_#{name}" }
 
   before(:each) do
     posix_agents.each do |agent|
-      on(agent, "cp #{auth_keys} /tmp/auth_keys", acceptable_exit_codes: [0, 1])
+      on(agent, "cp -a #{auth_keys} /tmp/auth_keys", acceptable_exit_codes: [0, 1])
+      on(agent, "rm -f #{auth_keys}")
       on(agent, "echo '' >> #{auth_keys} && echo 'ssh-rsa mykey #{name}' >> #{auth_keys}")
-      on(agent, "chown $LOGNAME #{auth_keys}")
     end
   end
 
@@ -32,5 +34,22 @@
         expect(stdout).not_to include("mykey #{name}")
       end
     end
+
+    it "#{agent} should update an entry for an SSH authorized key in a custom location" do
+      on(agent, "mkdir #{custom_key_directory}")
+      on(agent, "echo '' >> #{custom_key} && echo 'ssh-rsa mykey #{name}' >> #{custom_key}")
+      args = ['ensure=present',
+              'user=$LOGNAME',
+              "type='rsa'",
+              "key='mynewshinykey'",
+              "target='#{custom_key}'"]
+      on(agent, puppet_resource('ssh_authorized_key', name.to_s, args))
+
+      on(agent, "cat #{custom_key}") do |_res|
+        expect(stdout).to include("mynewshinykey #{name}")
+        expect(stdout).not_to include("mykey #{name}")
+      end
+      on(agent, "rm -rf #{custom_key_directory}")
+    end
   end
 end
diff --git a/spec/unit/type/ssh_authorized_key_spec.rb b/spec/unit/type/ssh_authorized_key_spec.rb
@@ -17,7 +17,7 @@
   end
 
   describe 'when validating attributes' do
-    [:name, :provider].each do |param|
+    [:name, :provider, :drop_privileges].each do |param|
       it "has a #{param} parameter" do
         expect(described_class.attrtype(param)).to eq :param
       end
@@ -56,6 +56,28 @@
       end
     end
 
+    describe 'for drop_privileges' do
+      it 'uses true as a default value' do
+        expect(described_class.new(name: 'whev', user: 'nobody')[:drop_privileges]).to eq true
+      end
+
+      [true, :true, 'true', :yes, 'yes'].each do |value|
+        it "supports #{value} and returns a boolean true" do
+          expect(described_class.new(name: 'whev', user: 'nobody', drop_privileges: value)[:drop_privileges]).to eq true
+        end
+      end
+
+      [false, :false, 'false', :no, 'no'].each do |value|
+        it "supports #{value} and returns a boolean false" do
+          expect(described_class.new(name: 'whev', user: 'nobody', drop_privileges: value)[:drop_privileges]).to eq false
+        end
+      end
+
+      it 'raises an exception on something else' do
+        expect { described_class.new(name: 'whev', user: 'nobody', drop_privileges: 'nope') }.to raise_error(Puppet::Error, %r{Invalid value})
+      end
+    end
+
     describe 'for type' do
       [
         :'ssh-dss', :dsa,
