Skip to content

Commit 1647088

Browse files
authored
Merge pull request #156 from da-ar/recursive_sensitive
(FM-7698) Ensure that sensitive values are handled correctly
2 parents 8039157 + 95d65c0 commit 1647088

File tree

13 files changed

+415
-20
lines changed

13 files changed

+415
-20
lines changed

.travis.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ matrix:
2525
- env: RVM="jruby-9.1.9.0" PUPPET_GEM_VERSION='~> 5' JRUBY_OPTS="--debug"
2626
before_cache: pushd ~/.rvm && rm -rf archives rubies/ruby-2.2.7 rubies/ruby-2.3.4 && popd
2727
cache:
28-
bundler: true
28+
bundler: false
2929
directories: ~/.rvm
3030
before_install: rvm use jruby-9.1.9.0 --install --binary --fuzzy
3131
- rvm: 2.4.3

lib/puppet/resource_api/transport.rb

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,7 @@ def connect(name, connection_info)
3131
validate(name, connection_info)
3232
require "puppet/transport/#{name}"
3333
class_name = name.split('_').map { |e| e.capitalize }.join
34-
# passing the copy as it may have been stripped on invalid key/values by validate
35-
Puppet::Transport.const_get(class_name).new(get_context(name), connection_info)
34+
Puppet::Transport.const_get(class_name).new(get_context(name), wrap_sensitive(name, connection_info))
3635
end
3736
module_function :connect # rubocop:disable Style/AccessModifierDeclarations
3837

@@ -57,8 +56,8 @@ def self.validate(name, connection_info)
5756
environment: @environment,
5857
}
5958
end
60-
61-
transport_schema.check_schema(connection_info)
59+
message_prefix = 'The connection info provided does not match the Transport Schema'
60+
transport_schema.check_schema(connection_info, message_prefix)
6261
transport_schema.validate(connection_info)
6362
end
6463
private_class_method :validate
@@ -80,4 +79,17 @@ def self.init_transports
8079
@transports[@environment] ||= {}
8180
end
8281
private_class_method :init_transports
82+
83+
def self.wrap_sensitive(name, connection_info)
84+
transport_schema = @transports[@environment][name]
85+
if transport_schema
86+
transport_schema.definition[:connection_info].each do |attr_name, options|
87+
if options.key?(:sensitive) && (options[:sensitive] == true)
88+
connection_info[attr_name] = Puppet::Pops::Types::PSensitiveType::Sensitive.new(connection_info[attr_name])
89+
end
90+
end
91+
end
92+
connection_info
93+
end
94+
private_class_method :wrap_sensitive
8395
end

lib/puppet/resource_api/type_definition.rb

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -120,16 +120,17 @@ def validate_schema(definition, attr_key)
120120
end
121121

122122
# validates a resource hash against its type schema
123-
def check_schema(resource)
123+
def check_schema(resource, message_prefix = nil)
124124
namevars.each do |namevar|
125125
if resource[namevar].nil?
126126
raise Puppet::ResourceError, "`#{name}.get` did not return a value for the `#{namevar}` namevar attribute"
127127
end
128128
end
129129

130-
message = "Provider returned data that does not match the Type Schema for `#{name}[#{resource[namevars.first]}]`"
130+
message_prefix = 'Provider returned data that does not match the Type Schema' if message_prefix.nil?
131+
message = "#{message_prefix} for `#{name}[#{resource[namevars.first]}]`"
131132

132-
rejected_keys = check_schema_keys(resource) # removes bad keys
133+
rejected_keys = check_schema_keys(resource)
133134
bad_values = check_schema_values(resource)
134135

135136
unless rejected_keys.empty?
@@ -168,12 +169,17 @@ def check_schema_values(resource)
168169
resource.each do |key, value|
169170
next unless attributes[key]
170171
type = @data_type_cache[attributes[key][:type]]
172+
is_sensitive = (attributes[key].key?(:sensitive) && (attributes[key][:sensitive] == true))
171173
error_message = Puppet::ResourceApi::DataTypeHandling.try_validate(
172174
type,
173175
value,
174176
'',
175177
)
176-
bad_vals[key] = value unless error_message.nil?
178+
if is_sensitive
179+
bad_vals[key] = '<< redacted value >> ' + error_message unless error_message.nil?
180+
else
181+
bad_vals[key] = value unless error_message.nil?
182+
end
177183
end
178184
bad_vals
179185
end

spec/acceptance/device_spec.rb

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,14 @@
7878
DEVICE_CONF
7979
end
8080
let(:device_credentials) { Tempfile.new('credentials.txt') }
81-
let(:device_credentials_content) { {} }
81+
let(:device_credentials_content) do
82+
<<DEVICE_CREDS
83+
{
84+
username: foo
85+
secret: wibble
86+
}
87+
DEVICE_CREDS
88+
end
8289

8390
def is_device_apply_supported?
8491
Gem::Version.new(Puppet::PUPPETVERSION) >= Gem::Version.new('5.3.6') && Gem::Version.new(Puppet::PUPPETVERSION) != Gem::Version.new('5.4.0')

spec/acceptance/sensitive_spec.rb

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,22 @@
2121
expect(stdout_str).not_to match %r{foo}
2222
expect(stdout_str).not_to match %r{warn|error}i
2323
end
24+
25+
context 'when a sensitive value is not the top level type' do
26+
it 'is not exposed by a provider' do
27+
stdout_str, _status = Open3.capture2e("puppet apply #{common_args} -e \"test_sensitive { bar: secret => Sensitive('foo'), "\
28+
"optional_secret => Sensitive('optional foo'), variant_secret => [Sensitive('variant foo')] }\"")
29+
expect(stdout_str).to match %r{redacted}
30+
expect(stdout_str).not_to match %r{variant foo}
31+
expect(stdout_str).not_to match %r{warn|error}i
32+
end
33+
it 'properly validates the sensitive type value' do
34+
stdout_str, _status = Open3.capture2e("puppet apply #{common_args} -e \"test_sensitive { bar: secret => Sensitive('foo'), "\
35+
"optional_secret => Sensitive('optional foo'), variant_secret => [Sensitive(134679)] }\"")
36+
expect(stdout_str).to match %r{Sensitive\[String\]( value)?, got Sensitive\[Integer\]}
37+
expect(stdout_str).not_to match %r{134679}
38+
end
39+
end
2440
end
2541

2642
describe 'using `puppet resource`' do
Lines changed: 185 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,185 @@
1+
require 'spec_helper'
2+
require 'tempfile'
3+
require 'open3'
4+
5+
RSpec.describe 'a transport' do
6+
let(:common_args) { '--verbose --trace --debug --strict=error --modulepath spec/fixtures' }
7+
8+
before(:all) do
9+
FileUtils.mkdir_p(File.expand_path('~/.puppetlabs/opt/puppet/cache/devices/the_node/state'))
10+
end
11+
12+
describe 'using `puppet device`' do
13+
let(:common_args) { super() + ' --target the_node' }
14+
let(:device_conf) { Tempfile.new('device.conf') }
15+
let(:device_conf_content) do
16+
<<DEVICE_CONF
17+
[the_node]
18+
type test_device_sensitive
19+
url file://#{device_credentials.path}
20+
DEVICE_CONF
21+
end
22+
let(:device_credentials) { Tempfile.new('credentials.txt') }
23+
24+
def is_device_apply_supported?
25+
Gem::Version.new(Puppet::PUPPETVERSION) >= Gem::Version.new('5.3.6') && Gem::Version.new(Puppet::PUPPETVERSION) != Gem::Version.new('5.4.0')
26+
end
27+
28+
before(:each) do
29+
skip "No device --apply in puppet before v5.3.6 nor in v5.4.0 (v#{Puppet::PUPPETVERSION} is installed)" unless is_device_apply_supported?
30+
device_conf.write(device_conf_content)
31+
device_conf.close
32+
33+
device_credentials.write(device_credentials_content)
34+
device_credentials.close
35+
end
36+
37+
after(:each) do
38+
device_conf.unlink
39+
device_credentials.unlink
40+
end
41+
42+
context 'when all sensitive values are valid' do
43+
let(:device_credentials_content) do
44+
<<DEVICE_CREDS
45+
{
46+
username: foo
47+
secret_string: wibble
48+
optional_secret: bar
49+
array_secret: [meep]
50+
variant_secret: 1234567890
51+
}
52+
DEVICE_CREDS
53+
end
54+
55+
it 'does not throw' do
56+
Tempfile.create('apply_success') do |f|
57+
f.write 'notify { "foo": }'
58+
f.close
59+
60+
stdout_str, _status = Open3.capture2e("puppet device #{common_args} --deviceconfig #{device_conf.path} --apply #{f.path}")
61+
expect(stdout_str).not_to match %r{Value type mismatch}
62+
63+
expect(stdout_str).to match %r{transport connection_info:}
64+
expect(stdout_str).to match %r{:username=>"foo"}
65+
expect(stdout_str).not_to match %r{wibble}
66+
expect(stdout_str).not_to match %r{bar}
67+
expect(stdout_str).not_to match %r{meep}
68+
expect(stdout_str).not_to match %r{1234567890}
69+
end
70+
end
71+
end
72+
73+
context 'with a sensitive string value that is invalid' do
74+
let(:device_credentials_content) do
75+
<<DEVICE_CREDS
76+
{
77+
username: foo
78+
secret_string: 12345
79+
optional_secret: wibble
80+
array_secret: [meep]
81+
variant_secret: 21
82+
}
83+
DEVICE_CREDS
84+
end
85+
86+
it 'Value type mismatch' do
87+
Tempfile.create('apply_success') do |f|
88+
f.write 'notify { "foo": }'
89+
f.close
90+
91+
stdout_str, _status = Open3.capture2e("puppet device #{common_args} --deviceconfig #{device_conf.path} --apply #{f.path}")
92+
expect(stdout_str).to match %r{Value type mismatch}
93+
expect(stdout_str).to match %r{secret_string: << redacted value >> }
94+
expect(stdout_str).not_to match %r{optional_secret}
95+
expect(stdout_str).not_to match %r{array_secret}
96+
expect(stdout_str).not_to match %r{variant_secret}
97+
end
98+
end
99+
end
100+
101+
context 'with an optional sensitive string value that is invalid' do
102+
let(:device_credentials_content) do
103+
<<DEVICE_CREDS
104+
{
105+
username: foo
106+
secret_string: wibble
107+
optional_secret: 12345
108+
array_secret: [meep]
109+
variant_secret: 21
110+
}
111+
DEVICE_CREDS
112+
end
113+
114+
it 'Value type mismatch' do
115+
Tempfile.create('apply_success') do |f|
116+
f.write 'notify { "foo": }'
117+
f.close
118+
119+
stdout_str, _status = Open3.capture2e("puppet device #{common_args} --deviceconfig #{device_conf.path} --apply #{f.path}")
120+
expect(stdout_str).to match %r{Value type mismatch}
121+
expect(stdout_str).not_to match %r{secret_string }
122+
expect(stdout_str).to match %r{optional_secret: << redacted value >>}
123+
expect(stdout_str).not_to match %r{array_secret}
124+
expect(stdout_str).not_to match %r{variant_secret}
125+
end
126+
end
127+
end
128+
129+
context 'with an array of sensitive strings that is invalid' do
130+
let(:device_credentials_content) do
131+
<<DEVICE_CREDS
132+
{
133+
username: foo
134+
secret_string: wibble
135+
optional_secret: bar
136+
array_secret: [17]
137+
variant_secret: 21
138+
}
139+
DEVICE_CREDS
140+
end
141+
142+
it 'Value type mismatch' do
143+
Tempfile.create('apply_success') do |f|
144+
f.write 'notify { "foo": }'
145+
f.close
146+
147+
stdout_str, _status = Open3.capture2e("puppet device #{common_args} --deviceconfig #{device_conf.path} --apply #{f.path}")
148+
expect(stdout_str).to match %r{Value type mismatch}
149+
expect(stdout_str).not_to match %r{secret_string }
150+
expect(stdout_str).not_to match %r{optional_secret}
151+
expect(stdout_str).to match %r{array_secret: << redacted value >>}
152+
expect(stdout_str).not_to match %r{variant_secret}
153+
end
154+
end
155+
end
156+
157+
context 'with an variant containing a sensitive value that is invalid' do
158+
let(:device_credentials_content) do
159+
<<DEVICE_CREDS
160+
{
161+
username: foo
162+
secret_string: wibble
163+
optional_secret: bar
164+
array_secret: [meep]
165+
variant_secret: wobble
166+
}
167+
DEVICE_CREDS
168+
end
169+
170+
it 'Value type mismatch' do
171+
Tempfile.create('apply_success') do |f|
172+
f.write 'notify { "foo": }'
173+
f.close
174+
175+
stdout_str, _status = Open3.capture2e("puppet device #{common_args} --deviceconfig #{device_conf.path} --apply #{f.path}")
176+
expect(stdout_str).to match %r{Value type mismatch}
177+
expect(stdout_str).not_to match %r{secret_string }
178+
expect(stdout_str).not_to match %r{optional_secret}
179+
expect(stdout_str).not_to match %r{array_secret}
180+
expect(stdout_str).to match %r{variant_secret: << redacted value >>}
181+
end
182+
end
183+
end
184+
end
185+
end

spec/fixtures/test_module/lib/puppet/transport/schema/test_device.rb

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,5 +4,29 @@
44
name: 'test_device', # points at class Puppet::Transport::TestDevice
55
desc: 'Connects to a device',
66
connection_info: {
7+
username: {
8+
type: 'String',
9+
desc: 'The name of the resource you want to manage.',
10+
},
11+
secret: {
12+
type: 'String',
13+
desc: 'A secret to protect.',
14+
sensitive: true,
15+
},
16+
optional_secret: {
17+
type: 'Optional[String]',
18+
desc: 'An optional secret to protect.',
19+
sensitive: true,
20+
},
21+
array_secret: {
22+
type: 'Optional[Array[String]]',
23+
desc: 'An array secret to protect.',
24+
sensitive: true,
25+
},
26+
variant_secret: {
27+
type: 'Optional[Variant[Array[String], Integer]]',
28+
desc: 'An array secret to protect.',
29+
sensitive: true,
30+
},
731
},
832
)
Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
require 'puppet/resource_api'
2+
3+
Puppet::ResourceApi.register_transport(
4+
name: 'test_device_sensitive', # points at class Puppet::Transport::TestDevice
5+
desc: 'Connects to a device',
6+
connection_info: {
7+
username: {
8+
type: 'String',
9+
desc: 'The name of the resource you want to manage.',
10+
},
11+
secret_string: {
12+
type: 'String',
13+
desc: 'A secret to protect.',
14+
sensitive: true,
15+
},
16+
optional_secret: {
17+
type: 'Optional[String]',
18+
desc: 'An optional secret to protect.',
19+
sensitive: true,
20+
},
21+
array_secret: {
22+
type: 'Optional[Array[String]]',
23+
desc: 'An array secret to protect.',
24+
sensitive: true,
25+
},
26+
variant_secret: {
27+
type: 'Optional[Variant[Array[String], Integer]]',
28+
desc: 'An array secret to protect.',
29+
sensitive: true,
30+
},
31+
},
32+
)
Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
module Puppet::Transport
2+
# a transport for a test_device
3+
class TestDeviceSensitive
4+
def initialize(_context, connection_info);
5+
puts "transport connection_info: #{connection_info}"
6+
end
7+
8+
def facts(_context)
9+
{ 'foo' => 'bar' }
10+
end
11+
12+
def verify(_context)
13+
return true
14+
end
15+
16+
def close(_context)
17+
return
18+
end
19+
end
20+
21+
end

0 commit comments

Comments
 (0)