From d1f1594a069d311fb998d10ceb97e51e58e9b5be Mon Sep 17 00:00:00 2001 From: Josh Cooper Date: Wed, 10 Jul 2024 23:35:15 -0700 Subject: [PATCH 1/2] (PUP-12050) Refactor compile_and_resolve_catalog (cherry picked from commit bdf1a464af968b1b683588bb0632713186802503) --- .../unit/pops/evaluator/deferred_resolver_spec.rb | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/spec/unit/pops/evaluator/deferred_resolver_spec.rb b/spec/unit/pops/evaluator/deferred_resolver_spec.rb index d6c945facf8..7f7d7751a1c 100644 --- a/spec/unit/pops/evaluator/deferred_resolver_spec.rb +++ b/spec/unit/pops/evaluator/deferred_resolver_spec.rb @@ -7,37 +7,40 @@ let(:environment) { Puppet::Node::Environment.create(:testing, []) } let(:facts) { Puppet::Node::Facts.new('node.example.com') } + def compile_and_resolve_catalog(code, preprocess = false) + catalog = compile_to_catalog(code) + described_class.resolve_and_replace(facts, catalog, environment, preprocess) + catalog + end + it 'resolves deferred values in a catalog' do - catalog = compile_to_catalog(<<~END) + catalog = compile_and_resolve_catalog(<<~END, true) notify { "deferred": message => Deferred("join", [[1,2,3], ":"]) } END - described_class.resolve_and_replace(facts, catalog) expect(catalog.resource(:notify, 'deferred')[:message]).to eq('1:2:3') end it 'lazily resolves deferred values in a catalog' do - catalog = compile_to_catalog(<<~END) + catalog = compile_and_resolve_catalog(<<~END) notify { "deferred": message => Deferred("join", [[1,2,3], ":"]) } END - described_class.resolve_and_replace(facts, catalog, environment, false) deferred = catalog.resource(:notify, 'deferred')[:message] expect(deferred.resolve).to eq('1:2:3') end it 'lazily resolves nested deferred values in a catalog' do - catalog = compile_to_catalog(<<~END) + catalog = compile_and_resolve_catalog(<<~END) $args = Deferred("inline_epp", ["<%= 'a,b,c' %>"]) notify { "deferred": message => Deferred("split", [$args, ","]) } END - described_class.resolve_and_replace(facts, catalog, environment, false) deferred = catalog.resource(:notify, 'deferred')[:message] expect(deferred.resolve).to eq(["a", "b", "c"]) From 20d1a3721af710de2f4e6c9eb3ea4e28ea0b3836 Mon Sep 17 00:00:00 2001 From: Josh Cooper Date: Wed, 3 Jul 2024 17:13:15 -0700 Subject: [PATCH 2/2] (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. (cherry picked from commit a94d5d04ced42e94a5fe799c44533bf6214cdd20) --- .../pops/evaluator/deferred_resolver.rb | 52 ++++++++++++++++--- spec/integration/application/apply_spec.rb | 15 ++++++ .../pops/evaluator/deferred_resolver_spec.rb | 49 +++++++++++++++++ 3 files changed, 108 insertions(+), 8 deletions(-) diff --git a/lib/puppet/pops/evaluator/deferred_resolver.rb b/lib/puppet/pops/evaluator/deferred_resolver.rb index c36b3697cc0..93243918929 100644 --- a/lib/puppet/pops/evaluator/deferred_resolver.rb +++ b/lib/puppet/pops/evaluator/deferred_resolver.rb @@ -88,16 +88,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 - # - if resolved.is_a?(Puppet::Pops::Types::PSensitiveType::Sensitive) + 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 - elsif resolved.is_a?(Puppet::Pops::Evaluator::DeferredValue) - if v.arguments.any? {|arg| arg.is_a?(Puppet::Pops::Types::PSensitiveType)} + + when Puppet::Pops::Evaluator::DeferredValue + # 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 @@ -107,6 +116,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 cedeb0ed717..7c8f7b40f97 100644 --- a/spec/integration/application/apply_spec.rb +++ b/spec/integration/application/apply_spec.rb @@ -769,5 +769,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