diff options
| -rw-r--r-- | REFERENCE.md | 11 | ||||
| -rw-r--r-- | lib/puppet/provider/ssh_authorized_key/parsed.rb | 52 | ||||
| -rw-r--r-- | lib/puppet/type/ssh_authorized_key.rb | 14 | ||||
| -rw-r--r-- | spec/acceptance/tests/resource/ssh_authorized_key/create_spec.rb | 58 | ||||
| -rw-r--r-- | spec/acceptance/tests/resource/ssh_authorized_key/destroy_spec.rb | 23 | ||||
| -rw-r--r-- | spec/acceptance/tests/resource/ssh_authorized_key/modify_spec.rb | 23 | ||||
| -rw-r--r-- | spec/unit/type/ssh_authorized_key_spec.rb | 24 |
7 files changed, 176 insertions, 29 deletions
diff --git a/REFERENCE.md b/REFERENCE.md index 6f80106..1e6b933 100644 --- 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 index 02a19eb..b10066e 100644 --- a/lib/puppet/provider/ssh_authorized_key/parsed.rb +++ b/lib/puppet/provider/ssh_authorized_key/parsed.rb @@ -42,9 +42,28 @@ Puppet::Type.type(:ssh_authorized_key).provide( 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 @@ Puppet::Type.type(:ssh_authorized_key).provide( # 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 index a36c069..648055c 100644 --- 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 index 34154ee..bfc75a7 100644 --- a/spec/acceptance/tests/resource/ssh_authorized_key/create_spec.rb +++ b/spec/acceptance/tests/resource/ssh_authorized_key/create_spec.rb @@ -7,13 +7,11 @@ RSpec.context 'ssh_authorized_key: Create' do 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 @@ -21,7 +19,6 @@ RSpec.context 'ssh_authorized_key: Create' 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 @@ RSpec.context 'ssh_authorized_key: Create' do 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 index af160ce..a491eb6 100644 --- 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 @@ RSpec.context 'sshkeys: Destroy' 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 @@ -34,5 +35,21 @@ RSpec.context 'sshkeys: Destroy' do 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 index 3a46374..711d2fc 100644 --- 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 @@ require 'spec_helper_acceptance' 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 @@ RSpec.context 'sshkeys: Modify' do 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 index 866c688..457537c 100644 --- a/spec/unit/type/ssh_authorized_key_spec.rb +++ b/spec/unit/type/ssh_authorized_key_spec.rb @@ -17,7 +17,7 @@ describe Puppet::Type.type(:ssh_authorized_key), unless: Puppet.features.microso 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 @@ describe Puppet::Type.type(:ssh_authorized_key), unless: Puppet.features.microso 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, |