Skip to content

Commit f78b81b

Browse files
mihaibuzgaumihaibuzgau
authored
Merge pull request #20 from GabrielNagy/MODULES-9578/create-file-as-root
(MODULES-9578) Create ssh_authorized_key in root path
2 parents e3edf94 + b2c153b commit f78b81b

File tree

7 files changed

+184
-19
lines changed

7 files changed

+184
-19
lines changed

‎REFERENCE.md‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,8 @@ will autorequire this user if it is being managed as a `user` resource.
8585
The absolute filename in which to store the SSH key. This
8686
property is optional and should be used only in cases where keys
8787
are stored in a non-standard location, for instance when not in
88-
`~user/.ssh/authorized_keys`.
88+
`~user/.ssh/authorized_keys`. The parent directory must be present
89+
if the target is in a privileged path.
8990

9091
Default value: absent
9192

@@ -119,6 +120,14 @@ Due to internal limitations, this must be unique across all user accounts;
119120
if you want to specify one key for multiple users, you must use a different
120121
comment for each instance.
121122

123+
##### `drop_privileges`
124+
125+
Whether to drop privileges when writing the key file. This is
126+
useful for creating files in paths not writable by the target user. Note
127+
the possible security implications of managing file ownership and
128+
permissions as a privileged user.
129+
130+
Default value: `true`
122131

123132
### sshkey
124133

‎lib/puppet/provider/ssh_authorized_key/parsed.rb‎

Lines changed: 43 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -42,9 +42,28 @@ def file_perm
4242
0o600
4343
end
4444

45-
def user
46-
uid = Puppet::FileSystem.stat(target).uid
47-
Etc.getpwuid(uid).name
45+
def group_writable_perm
46+
0o020
47+
end
48+
49+
def group_writable?(path)
50+
path.stat.mode & group_writable_perm != 0
51+
end
52+
53+
def trusted_path
54+
# return if the parent directory does not exist
55+
return false unless Puppet::FileSystem.dir_exist?(target)
56+
path = Puppet::FileSystem.pathname(target).dirname
57+
until path.dirname.root?
58+
path = path.realpath if path.symlink?
59+
# do not trust if path is world or group writable
60+
if path.stat.uid != Process.euid || path.world_writable? || group_writable?(path)
61+
Puppet.debug('Path untrusted, will attempt to write as the target user')
62+
return false
63+
end
64+
path = path.dirname
65+
end
66+
Puppet.debug('Path trusted, writing the file as the current user')
4867
end
4968

5069
def flush
@@ -56,15 +75,30 @@ def flush
5675
# so calling it here suppresses the later attempt by our superclass's flush method.
5776
self.class.backup_target(target)
5877

59-
Puppet::Util::SUIDManager.asuser(@resource.should(:user)) do
60-
unless Puppet::FileSystem.exist?(dir = File.dirname(target))
61-
Puppet.debug "Creating #{dir} as #{@resource.should(:user)}"
62-
Dir.mkdir(dir, dir_perm)
63-
end
78+
# attempt to create the file as the specified user if we're not dropping privileges
79+
if @resource[:drop_privileges]
80+
Puppet::Util::SUIDManager.asuser(@resource.should(:user)) do
81+
unless Puppet::FileSystem.exist?(dir = File.dirname(target))
82+
Puppet.debug "Creating #{dir} as #{@resource.should(:user)}"
83+
Dir.mkdir(dir, dir_perm)
84+
end
85+
super
6486

87+
File.chmod(file_perm, target)
88+
end
89+
# to avoid race conditions when handling permissions as a privileged user
90+
# (CVE-2011-3870) we use the trusted_path method to ensure the entire
91+
# directory structure is "safe" to write in
92+
else
93+
raise Puppet::Error, 'drop_privileges is false but the target path is not trusted' unless trusted_path
6594
super
6695

67-
File.chmod(file_perm, target)
96+
uid = Puppet::Util.uid(@resource.should(:user))
97+
gid = Puppet::Util.gid(@resource.should(:user))
98+
File.open(target) do |target|
99+
target.chown(uid, gid)
100+
target.chmod(file_perm)
101+
end
68102
end
69103
end
70104

‎lib/puppet/type/ssh_authorized_key.rb‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
require 'puppet/parameter/boolean'
2+
13
module Puppet
24
Type.newtype(:ssh_authorized_key) do
35
@doc = "Manages SSH authorized keys. Currently only type 2 keys are supported.
@@ -48,6 +50,15 @@ module Puppet
4850
isnamevar
4951
end
5052

53+
newparam(:drop_privileges, boolean: true, parent: Puppet::Parameter::Boolean) do
54+
desc "Whether to drop privileges when writing the key file. This is
55+
useful for creating files in paths not writable by the target user. Note
56+
the possible security implications of managing file ownership and
57+
permissions as a privileged user."
58+
59+
defaultto true
60+
end
61+
5162
newproperty(:type) do
5263
desc 'The encryption type used.'
5364

@@ -83,7 +94,8 @@ module Puppet
8394
desc "The absolute filename in which to store the SSH key. This
8495
property is optional and should be used only in cases where keys
8596
are stored in a non-standard location, for instance when not in
86-
`~user/.ssh/authorized_keys`."
97+
`~user/.ssh/authorized_keys`. The parent directory must be present
98+
if the target is in a privileged path."
8799

88100
defaultto :absent
89101

‎spec/acceptance/tests/resource/ssh_authorized_key/create_spec.rb‎

Lines changed: 54 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,13 @@
55

66
let(:auth_keys) { '~/.ssh/authorized_keys' }
77
let(:name) { "pl#{rand(999_999).to_i}" }
8+
let(:custom_key_directory) { "/etc/ssh_authorized_keys_#{name}" }
9+
let(:custom_key) { "#{custom_key_directory}/authorized_keys_#{name}" }
810

911
before(:each) do
1012
posix_agents.each do |agent|
11-
on(agent, "cp #{auth_keys} /tmp/auth_keys", acceptable_exit_codes: [0, 1])
12-
on(agent, "chown $LOGNAME #{auth_keys}")
13+
on(agent, "cp -a #{auth_keys} /tmp/auth_keys", acceptable_exit_codes: [0, 1])
14+
on(agent, "rm -f #{auth_keys}")
1315
end
1416
end
1517

@@ -32,5 +34,55 @@
3234
fail_test "didn't find the ssh_authorized_key for #{name}" unless stdout.include? name.to_s
3335
end
3436
end
37+
38+
it "#{agent} should create an entry for an SSH authorized key in a custom location" do
39+
on(agent, "mkdir #{custom_key_directory}")
40+
args = ['ensure=present',
41+
'user=$LOGNAME',
42+
"type='rsa'",
43+
"key='mykey'",
44+
"target='#{custom_key}'"]
45+
on(agent, puppet_resource('ssh_authorized_key', name.to_s, args))
46+
47+
on(agent, "cat #{custom_key}") do |_res|
48+
fail_test "didn't find the ssh_authorized_key for #{name}" unless stdout.include? name.to_s
49+
end
50+
on(agent, "rm -rf #{custom_key_directory}")
51+
end
52+
53+
it "#{agent} should fail if target user doesn't have permissions for symlinked path" do
54+
# create a dummy user
55+
on(agent, puppet_resource('user', 'testuser', 'ensure=present', 'managehome=true'))
56+
57+
on(agent, "mkdir #{custom_key_directory}")
58+
59+
# as the user, symlink an owned directory to something inside /root
60+
on(agent, puppet_resource('file', '/home/testuser/tmp', ['ensure=/etc', 'owner=testuser']))
61+
args = ['ensure=present',
62+
'user=testuser',
63+
"type='rsa'",
64+
"key='mykey'",
65+
'drop_privileges=false',
66+
"target=/home/testuser/tmp/ssh_authorized_keys_#{name}/authorized_keys_#{name}"]
67+
on(agent, puppet_resource('ssh_authorized_key', name.to_s, args)) do |_res|
68+
fail_test unless stderr =~ %r{the target path is not trusted}
69+
end
70+
on(agent, "rm -rf #{custom_key_directory}")
71+
72+
# purge the user
73+
on(agent, puppet_resource('user', 'testuser', 'ensure=absent'))
74+
end
75+
76+
it "#{agent} should not create directories for SSH authorized key in a custom location" do
77+
args = ['ensure=present',
78+
'user=$LOGNAME',
79+
"type='rsa'",
80+
"key='mykey'",
81+
'drop_privileges=false',
82+
"target='#{custom_key}'"]
83+
on(agent, puppet_resource('ssh_authorized_key', name.to_s, args), acceptable_exit_codes: [0, 1]) do |_res|
84+
fail_test unless stderr =~ %r{the target path is not trusted}
85+
end
86+
end
3587
end
3688
end

‎spec/acceptance/tests/resource/ssh_authorized_key/destroy_spec.rb‎

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,14 @@
55

66
let(:auth_keys) { '~/.ssh/authorized_keys' }
77
let(:name) { "pl#{rand(999_999).to_i}" }
8+
let(:custom_key_directory) { "/etc/ssh_authorized_keys_#{name}" }
9+
let(:custom_key) { "#{custom_key_directory}/authorized_keys_#{name}" }
810

911
before(:each) do
1012
posix_agents.each do |agent|
11-
on(agent, "cp #{auth_keys} /tmp/auth_keys", acceptable_exit_codes: [0, 1])
12-
13+
on(agent, "cp -a #{auth_keys} /tmp/auth_keys", acceptable_exit_codes: [0, 1])
14+
on(agent, "rm -f #{auth_keys}")
1315
on(agent, "echo '' >> #{auth_keys} && echo 'ssh-rsa mykey #{name}' >> #{auth_keys}")
14-
on(agent, "chown $LOGNAME #{auth_keys}")
1516
end
1617
end
1718

@@ -34,5 +35,21 @@
3435
expect(stdout).not_to include(name.to_s)
3536
end
3637
end
38+
39+
it "#{agent} should delete an entry for an SSH authorized key in a custom location" do
40+
on(agent, "mkdir #{custom_key_directory}")
41+
on(agent, "echo '' >> #{custom_key} && echo 'ssh-rsa mykey #{name}' >> #{custom_key}")
42+
args = ['ensure=absent',
43+
'user=$LOGNAME',
44+
"type='rsa'",
45+
"key='mykey'",
46+
"target='#{custom_key}'"]
47+
on(agent, puppet_resource('ssh_authorized_key', name.to_s, args))
48+
49+
on(agent, "cat #{custom_key}") do |_res|
50+
expect(stdout).not_to include(name.to_s)
51+
end
52+
on(agent, "rm -rf #{custom_key_directory}")
53+
end
3754
end
3855
end

‎spec/acceptance/tests/resource/ssh_authorized_key/modify_spec.rb‎

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,14 @@
33
RSpec.context 'sshkeys: Modify' do
44
let(:auth_keys) { '~/.ssh/authorized_keys' }
55
let(:name) { "pl#{rand(999_999).to_i}" }
6+
let(:custom_key_directory) { "/etc/ssh_authorized_keys_#{name}" }
7+
let(:custom_key) { "#{custom_key_directory}/authorized_keys_#{name}" }
68

79
before(:each) do
810
posix_agents.each do |agent|
9-
on(agent, "cp #{auth_keys} /tmp/auth_keys", acceptable_exit_codes: [0, 1])
11+
on(agent, "cp -a #{auth_keys} /tmp/auth_keys", acceptable_exit_codes: [0, 1])
12+
on(agent, "rm -f #{auth_keys}")
1013
on(agent, "echo '' >> #{auth_keys} && echo 'ssh-rsa mykey #{name}' >> #{auth_keys}")
11-
on(agent, "chown $LOGNAME #{auth_keys}")
1214
end
1315
end
1416

@@ -32,5 +34,22 @@
3234
expect(stdout).not_to include("mykey #{name}")
3335
end
3436
end
37+
38+
it "#{agent} should update an entry for an SSH authorized key in a custom location" do
39+
on(agent, "mkdir #{custom_key_directory}")
40+
on(agent, "echo '' >> #{custom_key} && echo 'ssh-rsa mykey #{name}' >> #{custom_key}")
41+
args = ['ensure=present',
42+
'user=$LOGNAME',
43+
"type='rsa'",
44+
"key='mynewshinykey'",
45+
"target='#{custom_key}'"]
46+
on(agent, puppet_resource('ssh_authorized_key', name.to_s, args))
47+
48+
on(agent, "cat #{custom_key}") do |_res|
49+
expect(stdout).to include("mynewshinykey #{name}")
50+
expect(stdout).not_to include("mykey #{name}")
51+
end
52+
on(agent, "rm -rf #{custom_key_directory}")
53+
end
3554
end
3655
end

‎spec/unit/type/ssh_authorized_key_spec.rb‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
end
1818

1919
describe 'when validating attributes' do
20-
[:name, :provider].each do |param|
20+
[:name, :provider, :drop_privileges].each do |param|
2121
it "has a #{param} parameter" do
2222
expect(described_class.attrtype(param)).to eq :param
2323
end
@@ -56,6 +56,28 @@
5656
end
5757
end
5858

59+
describe 'for drop_privileges' do
60+
it 'uses true as a default value' do
61+
expect(described_class.new(name: 'whev', user: 'nobody')[:drop_privileges]).to eq true
62+
end
63+
64+
[true, :true, 'true', :yes, 'yes'].each do |value|
65+
it "supports #{value} and returns a boolean true" do
66+
expect(described_class.new(name: 'whev', user: 'nobody', drop_privileges: value)[:drop_privileges]).to eq true
67+
end
68+
end
69+
70+
[false, :false, 'false', :no, 'no'].each do |value|
71+
it "supports #{value} and returns a boolean false" do
72+
expect(described_class.new(name: 'whev', user: 'nobody', drop_privileges: value)[:drop_privileges]).to eq false
73+
end
74+
end
75+
76+
it 'raises an exception on something else' do
77+
expect { described_class.new(name: 'whev', user: 'nobody', drop_privileges: 'nope') }.to raise_error(Puppet::Error, %r{Invalid value})
78+
end
79+
end
80+
5981
describe 'for type' do
6082
[
6183
:'ssh-dss', :dsa,

0 commit comments

Comments
 (0)