From a94d5d04ced42e94a5fe799c44533bf6214cdd20 Mon Sep 17 00:00:00 2001 From: Josh Cooper Date: Wed, 3 Jul 2024 17:13:15 -0700 Subject: [PATCH] (PUP-12050) Check for nested Sensitive arguments Previously, a manifest containing nested Deferred values did not mark the corresponding parameter as sensitive, resulting in the following: $ cat manifest.pp $vars = {'token' => Deferred('new', [Sensitive, "password"])} file { '/tmp/a.sh': ensure => file, content => Deferred('inline_epp', ['<%= $token %>', $vars]) } $ truncate --size 0 /tmp/a.sh $ puppet apply --show_diff manifest.pp Notice: Compiled catalog for localhost in environment production in 0.01 seconds Notice: /Stage[main]/Main/File[/tmp/a.sh]/content: --- /tmp/a.sh 2024-07-03 17:30:37.024543314 -0700 +++ /tmp/puppet-file20240703-1784698-2cu5s9 2024-07-03 17:30:41.880572413 -0700 @@ -0,0 +1 @@ +password \ No newline at end of file The issue occurred because we were only checking if the outermost DeferredValue contained any Sensitive arguments, in this case the arguments passed to `inline_epp` function, but not the `password` passed to the `new` function. This is not an issue when deferred values are preprocessed, because Deferred values are completely resolved and we can check if resolved value is Sensitive. --- .../pops/evaluator/deferred_resolver.rb | 47 +++++++++++++++--- spec/integration/application/apply_spec.rb | 15 ++++++ .../pops/evaluator/deferred_resolver_spec.rb | 49 +++++++++++++++++++ 3 files changed, 105 insertions(+), 6 deletions(-) diff --git a/lib/puppet/pops/evaluator/deferred_resolver.rb b/lib/puppet/pops/evaluator/deferred_resolver.rb index 6c4bf989693..ace0400dffa 100644 --- a/lib/puppet/pops/evaluator/deferred_resolver.rb +++ b/lib/puppet/pops/evaluator/deferred_resolver.rb @@ -89,17 +89,25 @@ def resolve_futures(catalog) overrides = {} r.parameters.each_pair do |k, v| resolved = resolve(v) - # If the value is instance of Sensitive - assign the unwrapped value - # and mark it as sensitive if not already marked - # case resolved when Puppet::Pops::Types::PSensitiveType::Sensitive + # If the resolved value is instance of Sensitive - assign the unwrapped value + # and mark it as sensitive if not already marked + # resolved = resolved.unwrap mark_sensitive_parameters(r, k) - # If the value is a DeferredValue and it has an argument of type PSensitiveType, mark it as sensitive - # The DeferredValue.resolve method will unwrap it during catalog application + when Puppet::Pops::Evaluator::DeferredValue - if v.arguments.any? { |arg| arg.is_a?(Puppet::Pops::Types::PSensitiveType) } + # If the resolved value is a DeferredValue and it has an argument of type + # PSensitiveType, mark it as sensitive. Since DeferredValues can nest, + # we must walk all arguments, e.g. the DeferredValue may call the `epp` + # function, where one of its arguments is a DeferredValue to call the + # `vault:lookup` function. + # + # The DeferredValue.resolve method will unwrap the sensitive during + # catalog application + # + if contains_sensitive_args?(v) mark_sensitive_parameters(r, k) end end @@ -109,6 +117,33 @@ def resolve_futures(catalog) end end + # Return true if x contains an argument that is an instance of PSensitiveType: + # + # Deferred('new', [Sensitive, 'password']) + # + # Or an instance of PSensitiveType::Sensitive: + # + # Deferred('join', [['a', Sensitive('b')], ':']) + # + # Since deferred values can nest, descend into Arrays and Hash keys and values, + # short-circuiting when the first occurrence is found. + # + def contains_sensitive_args?(x) + case x + when @deferred_class + contains_sensitive_args?(x.arguments) + when Array + x.any? { |v| contains_sensitive_args?(v) } + when Hash + x.any? { |k, v| contains_sensitive_args?(k) || contains_sensitive_args?(v) } + when Puppet::Pops::Types::PSensitiveType, Puppet::Pops::Types::PSensitiveType::Sensitive + true + else + false + end + end + private :contains_sensitive_args? + def mark_sensitive_parameters(r, k) unless r.sensitive_parameters.include?(k.to_sym) r.sensitive_parameters = (r.sensitive_parameters + [k.to_sym]).freeze diff --git a/spec/integration/application/apply_spec.rb b/spec/integration/application/apply_spec.rb index 9f159dbec70..674da6d3be8 100644 --- a/spec/integration/application/apply_spec.rb +++ b/spec/integration/application/apply_spec.rb @@ -777,5 +777,20 @@ def bogus() }.to exit_with(0) .and output(/ensure: changed \[redacted\] to \[redacted\]/).to_stdout end + + it "applies nested deferred sensitive file content" do + manifest = <<~END + $vars = {'token' => Deferred('new', [Sensitive, "hello"])} + file { '#{deferred_file}': + ensure => file, + content => Deferred('inline_epp', ['<%= $token %>', $vars]) + } + END + apply.command_line.args = ['-e', manifest] + expect { + apply.run + }.to exit_with(0) + .and output(/ensure: changed \[redacted\] to \[redacted\]/).to_stdout + end end end diff --git a/spec/unit/pops/evaluator/deferred_resolver_spec.rb b/spec/unit/pops/evaluator/deferred_resolver_spec.rb index 7f7d7751a1c..4cb98cc7984 100644 --- a/spec/unit/pops/evaluator/deferred_resolver_spec.rb +++ b/spec/unit/pops/evaluator/deferred_resolver_spec.rb @@ -1,6 +1,11 @@ require 'spec_helper' require 'puppet_spec/compiler' +Puppet::Type.newtype(:test_deferred) do + newparam(:name) + newproperty(:value) +end + describe Puppet::Pops::Evaluator::DeferredResolver do include PuppetSpec::Compiler @@ -46,4 +51,48 @@ def compile_and_resolve_catalog(code, preprocess = false) expect(deferred.resolve).to eq(["a", "b", "c"]) end + it 'marks the parameter as sensitive when passed an array containing a Sensitive instance' do + catalog = compile_and_resolve_catalog(<<~END) + test_deferred { "deferred": + value => Deferred('join', [['a', Sensitive('b')], ':']) + } + END + + resource = catalog.resource(:test_deferred, 'deferred') + expect(resource.sensitive_parameters).to eq([:value]) + end + + it 'marks the parameter as sensitive when passed a hash containing a Sensitive key' do + catalog = compile_and_resolve_catalog(<<~END) + test_deferred { "deferred": + value => Deferred('keys', [{Sensitive('key') => 'value'}]) + } + END + + resource = catalog.resource(:test_deferred, 'deferred') + expect(resource.sensitive_parameters).to eq([:value]) + end + + it 'marks the parameter as sensitive when passed a hash containing a Sensitive value' do + catalog = compile_and_resolve_catalog(<<~END) + test_deferred { "deferred": + value => Deferred('values', [{key => Sensitive('value')}]) + } + END + + resource = catalog.resource(:test_deferred, 'deferred') + expect(resource.sensitive_parameters).to eq([:value]) + end + + it 'marks the parameter as sensitive when passed a nested Deferred containing a Sensitive type' do + catalog = compile_and_resolve_catalog(<<~END) + $vars = {'token' => Deferred('new', [Sensitive, "hello"])} + test_deferred { "deferred": + value => Deferred('inline_epp', ['<%= $token %>', $vars]) + } + END + + resource = catalog.resource(:test_deferred, 'deferred') + expect(resource.sensitive_parameters).to eq([:value]) + end end