(PDK-1091) Fix Sensitive value handling

.travis.yml

@@ -52,7 +52,7 @@ matrix:
       env: PUPPET_GEM_VERSION='~> 4.8.0'
     - rvm: 2.1.9
       env: PUPPET_GEM_VERSION='~> 4.7.0'
-    - rvm: 2.4.3
+    - rvm: 2.5.1
       env: PUPPET_GEM_VERSION='https://github.com/puppetlabs/puppet.git#master'
 notifications:
   hipchat:

Gemfile

@@ -45,8 +45,4 @@ def location_for(place_or_version, fake_version = nil)
   end
 end
 
-if ENV['PUPPET_GEM_VERSION']
-  gem 'puppet', *location_for(ENV['PUPPET_GEM_VERSION'])
-else
-  gem 'puppet', github: 'DavidS/puppet', ref: 'device-apply'
-end
+gem 'puppet', *location_for(ENV['PUPPET_GEM_VERSION'])

appveyor.yml

@@ -7,7 +7,8 @@ branches:
 environment:
   matrix:
     - RUBY_VERSION: 24-x64
-    - RUBY_VERSION: 21-x64
+    - PUPPET_GEM_VERSION: '~> 4.0'
+      RUBY_VERSION: 21-x64
 
 install:
   - set PATH=C:\Ruby%RUBY_VERSION%\bin;C:\mingw-w64\x86_64-6.3.0-posix-seh-rt_v5-rev1\mingw64\bin;%PATH%

lib/puppet/resource_api.rb

@@ -79,10 +79,21 @@ def type_definition
         if attributes.is_a? Puppet::Resource
           @title = attributes.title
           @catalog = attributes.catalog
+          sensitives = attributes.sensitive_parameters
           attributes = attributes.to_hash
         else
           @ral_find_absent = true
+          sensitives = []
         end
+
+        # undo puppet's unwrapping of Sensitive values to provide a uniform experience for providers
+        # See https://tickets.puppetlabs.com/browse/PDK-1091 for investigation and background
+        sensitives.each do |name|
+          if attributes.key?(name) && !attributes[name].is_a?(Puppet::Pops::Types::PSensitiveType::Sensitive)
+            attributes[name] = Puppet::Pops::Types::PSensitiveType::Sensitive.new(attributes[name])
+          end
+        end
+
         # $stderr.puts "B: #{attributes.inspect}"
         if type_definition.feature?('canonicalize')
           attributes = my_provider.canonicalize(context, [attributes])[0]
@@ -321,33 +332,33 @@ def call_provider(value); end
       end
 
       define_method(:refresh_current_state) do
-        @rapi_current_state = if type_definition.feature?('simple_get_filter')
-                                my_provider.get(context, [title]).find { |h| namevar_match?(h) }
-                              else
-                                my_provider.get(context).find { |h| namevar_match?(h) }
-                              end
-
-        if @rapi_current_state
-          type_definition.check_schema(@rapi_current_state)
-          strict_check(@rapi_current_state) if type_definition.feature?('canonicalize')
+        @rsapi_current_state = if type_definition.feature?('simple_get_filter')
+                                 my_provider.get(context, [title]).find { |h| namevar_match?(h) }
+                               else
+                                 my_provider.get(context).find { |h| namevar_match?(h) }
+                               end
+
+        if @rsapi_current_state
+          type_definition.check_schema(@rsapi_current_state)
+          strict_check(@rsapi_current_state) if type_definition.feature?('canonicalize')
         else
-          @rapi_current_state = { title: title }
-          @rapi_current_state[:ensure] = :absent if type_definition.ensurable?
+          @rsapi_current_state = { title: title }
+          @rsapi_current_state[:ensure] = :absent if type_definition.ensurable?
         end
       end
 
       # Use this to set the current state from the `instances` method
       def cache_current_state(resource_hash)
-        @rapi_current_state = resource_hash
-        strict_check(@rapi_current_state) if type_definition.feature?('canonicalize')
+        @rsapi_current_state = resource_hash
+        strict_check(@rsapi_current_state) if type_definition.feature?('canonicalize')
       end
 
       define_method(:retrieve) do
-        refresh_current_state unless @rapi_current_state
+        refresh_current_state unless @rsapi_current_state
 
-        Puppet.debug("Current State: #{@rapi_current_state.inspect}")
+        Puppet.debug("Current State: #{@rsapi_current_state.inspect}")
 
-        result = Puppet::Resource.new(self.class, title, parameters: @rapi_current_state)
+        result = Puppet::Resource.new(self.class, title, parameters: @rsapi_current_state)
         # puppet needs ensure to be a symbol
         result[:ensure] = result[:ensure].to_sym if type_definition.ensurable? && result[:ensure].is_a?(String)
 
@@ -371,17 +382,17 @@ def cache_current_state(resource_hash)
         target_state = Hash[actual_params.map { |k, v| [k, v.rs_value] }]
         target_state = my_provider.canonicalize(context, [target_state]).first if type_definition.feature?('canonicalize')
 
-        retrieve unless @rapi_current_state
+        retrieve unless @rsapi_current_state
 
-        return if @rapi_current_state == target_state
+        return if @rsapi_current_state == target_state
 
         Puppet.debug("Target State: #{target_state.inspect}")
 
         # enforce init_only attributes
-        if Puppet.settings[:strict] != :off && @rapi_current_state && (@rapi_current_state[:ensure] == 'present' && target_state[:ensure] == 'present')
+        if Puppet.settings[:strict] != :off && @rsapi_current_state && (@rsapi_current_state[:ensure] == 'present' && target_state[:ensure] == 'present')
           target_state.each do |name, value|
-            next unless definition[:attributes][name][:behaviour] == :init_only && value != @rapi_current_state[name]
-            message = "Attempting to change `#{name}` init_only attribute value from `#{@rapi_current_state[name]}` to `#{value}`"
+            next unless definition[:attributes][name][:behaviour] == :init_only && value != @rsapi_current_state[name]
+            message = "Attempting to change `#{name}` init_only attribute value from `#{@rsapi_current_state[name]}` to `#{value}`"
             case Puppet.settings[:strict]
             when :warning
               Puppet.warning(message)
@@ -392,14 +403,14 @@ def cache_current_state(resource_hash)
         end
 
         if type_definition.feature?('supports_noop')
-          my_provider.set(context, { title => { is: @rapi_current_state, should: target_state } }, noop: noop?)
+          my_provider.set(context, { title => { is: @rsapi_current_state, should: target_state } }, noop: noop?)
         else
-          my_provider.set(context, title => { is: @rapi_current_state, should: target_state }) unless noop?
+          my_provider.set(context, title => { is: @rsapi_current_state, should: target_state }) unless noop?
         end
         raise 'Execution encountered an error' if context.failed?
 
         # remember that we have successfully reached our desired state
-        @rapi_current_state = target_state
+        @rsapi_current_state = target_state
       end
 
       define_method(:raise_missing_attrs) do

spec/acceptance/device_spec.rb

@@ -1,15 +1,19 @@
+require 'open3'
+require 'puppet/version'
 require 'spec_helper'
 require 'tempfile'
 
 RSpec.describe 'exercising a device provider' do
   let(:common_args) { '--verbose --trace --strict=error --modulepath spec/fixtures' }
   let(:default_type_values) do
     'string="meep" boolean=true integer=15 float=1.23 ensure=present variant_pattern=AE321EEF '\
-                                'url="http://www.puppet.com" boolean_param=false integer_param=99 float_param=3.21 '\
-                                'ensure_param=present variant_pattern_param=0xAE321EEF url_param="https://www.google.com"'
+    'url="http://www.puppet.com" boolean_param=false integer_param=99 float_param=3.21 '\
+    'ensure_param=present variant_pattern_param=0xAE321EEF url_param="https://www.google.com"'
   end
 
-  before(:each) { skip 'No device --apply in the puppet gems yet' if ENV['PUPPET_GEM_VERSION'] }
+  before(:all) do
+    FileUtils.mkdir_p(File.expand_path('~/.puppetlabs/opt/puppet/cache/devices/the_node/state'))
+  end
 
   describe 'using `puppet resource`' do
     it 'manages resources on the target system' do
@@ -74,7 +78,12 @@
 DEVICE_CONF
     end
 
+    def is_device_apply_supported?
+      Gem::Version.new(Puppet::PUPPETVERSION) >= Gem::Version.new('5.3.6') && Gem::Version.new(Puppet::PUPPETVERSION) != Gem::Version.new('5.4.0')
+    end
+
     before(:each) do
+      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?
       device_conf.write(device_conf_content)
       device_conf.close
     end
@@ -91,25 +100,40 @@
     end
 
     it 'applies a catalog successfully' do
-      stdout_str, _status = Open3.capture2e("puppet device #{common_args} --deviceconfig #{device_conf.path} --apply 'notify{\"foo\":}'")
-      expect(stdout_str).to match %r{starting applying configuration to the_node at file:///etc/credentials.txt}
-      expect(stdout_str).to match %r{defined 'message' as 'foo'}
-      expect(stdout_str).not_to match %r{Error:}
+      Tempfile.create('apply_success') do |f|
+        f.write 'notify { "foo": }'
+        f.close
+
+        stdout_str, _status = Open3.capture2e("puppet device #{common_args} --deviceconfig #{device_conf.path} --apply #{f.path}")
+        expect(stdout_str).to match %r{Compiled catalog for the_node}
+        expect(stdout_str).to match %r{defined 'message' as 'foo'}
+        expect(stdout_str).not_to match %r{Error:}
+      end
     end
 
     it 'has the "foo" fact set to "bar"' do
-      stdout_str, status = Open3.capture2e("puppet device #{common_args} --deviceconfig #{device_conf.path} --apply 'if $facts[\"foo\"] != \"bar\" { fail(\"fact not found\") }'")
-      expect(stdout_str).not_to match %r{Error:}
-      expect(status).to eq 0
+      Tempfile.create('fact_set') do |f|
+        f.write 'if $facts["foo"] != "bar" { fail("fact not found") }'
+        f.close
+
+        stdout_str, status = Open3.capture2e("puppet device #{common_args} --deviceconfig #{device_conf.path} --apply #{f.path}")
+        expect(stdout_str).not_to match %r{Error:}
+        expect(status).to eq 0
+      end
     end
 
     context 'with a device resource in the catalog' do
       it 'applies the catalog successfully' do
-        stdout_str, _status = Open3.capture2e("puppet device #{common_args} --deviceconfig #{device_conf.path} --apply 'device_provider{\"foo\": "\
-                                              'ensure => "present", boolean => true, integer => 15, float => 1.23, variant_pattern => "0x1234ABCD", '\
-                                              'url => "http://www.google.com", boolean_param => false, integer_param => 99, float_param => 3.21, '\
-                                              "ensure_param => \"present\", variant_pattern_param => \"9A2222ED\", url_param => \"http://www.puppet.com\"}'")
-        expect(stdout_str).not_to match %r{Error:}
+        Tempfile.create('fact_set') do |f|
+          f.write 'device_provider{ "foo":' \
+            'ensure => "present", boolean => true, integer => 15, float => 1.23, variant_pattern => "0x1234ABCD", '\
+            'url => "http://www.google.com", boolean_param => false, integer_param => 99, float_param => 3.21, '\
+            'ensure_param => "present", variant_pattern_param => "9A2222ED", url_param => "http://www.puppet.com" }'
+          f.close
+
+          stdout_str, _status = Open3.capture2e("puppet device #{common_args} --deviceconfig #{device_conf.path} --apply #{f.path}")
+          expect(stdout_str).not_to match %r{Error:}
+        end
       end
     end
   end

spec/acceptance/sensitive_spec.rb

@@ -0,0 +1,34 @@
+require 'spec_helper'
+require 'tempfile'
+require 'open3'
+
+RSpec.describe 'sensitive data' do
+  # these common_args *have* to use debug to check *all* log messages for the sensitive value
+  let(:common_args) { '--verbose --trace --strict=error --modulepath spec/fixtures  --debug' }
+
+  describe 'using `puppet apply`' do
+    it 'is not exposed by notify' do
+      stdout_str, _status = Open3.capture2e("puppet apply #{common_args} -e \"notice(Sensitive('foo'))\"")
+      expect(stdout_str).to match %r{redacted}
+      expect(stdout_str).not_to match %r{foo}
+      expect(stdout_str).not_to match %r{warn|error}i
+    end
+
+    it 'is not exposed by a provider' do
+      stdout_str, _status = Open3.capture2e("puppet apply #{common_args} -e \"test_sensitive { bar: secret => Sensitive('foo'), "\
+        "optional_secret => Sensitive('optional foo'), array_secret => [Sensitive('array foo')] }\"")
+      expect(stdout_str).to match %r{redacted}
+      expect(stdout_str).not_to match %r{foo}
+      expect(stdout_str).not_to match %r{warn|error}i
+    end
+  end
+
+  describe 'using `puppet resource`' do
+    it 'is not exposed in the output' do
+      stdout_str, _status = Open3.capture2e("puppet resource #{common_args} test_sensitive")
+      expect(stdout_str).to match %r{redacted}
+      expect(stdout_str).not_to match %r{(foo|bar)secret}
+      expect(stdout_str).not_to match %r{warn|error}i
+    end
+  end
+end

spec/fixtures/test_module/lib/puppet/provider/test_sensitive/test_sensitive.rb

@@ -0,0 +1,31 @@
+require 'puppet/resource_api/simple_provider'
+
+# Implementation for the test_sensitive type using the Resource API.
+class Puppet::Provider::TestSensitive::TestSensitive < Puppet::ResourceApi::SimpleProvider
+  def get(_context)
+    [
+      {
+        name: 'foo',
+        ensure: 'present',
+        secret: Puppet::Pops::Types::PSensitiveType::Sensitive.new('foosecret')
+      },
+      {
+        name: 'bar',
+        ensure: 'present',
+        secret: Puppet::Pops::Types::PSensitiveType::Sensitive.new('barsecret')
+      },
+    ]
+  end
+
+  def create(context, name, should)
+    context.notice("Creating '#{name}' with #{should.inspect}")
+  end
+
+  def update(context, name, should)
+    context.notice("Updating '#{name}' with #{should.inspect}")
+  end
+
+  def delete(context, name)
+    context.notice("Deleting '#{name}'")
+  end
+end

spec/fixtures/test_module/lib/puppet/type/test_sensitive.rb

@@ -0,0 +1,33 @@
+require 'puppet/resource_api'
+
+Puppet::ResourceApi.register_type(
+  name: 'test_sensitive',
+  docs: <<-EOS,
+      This type provides Puppet with the capabilities to manage ...
+    EOS
+  features: [],
+  attributes:   {
+    ensure:      {
+      type:    'Enum[present, absent]',
+      desc:    'Whether this resource should be present or absent on the target system.',
+      default: 'present',
+    },
+    name:        {
+      type:      'String',
+      desc:      'The name of the resource you want to manage.',
+      behaviour: :namevar,
+    },
+    secret: {
+      type:      'Sensitive[String]',
+      desc:      'A secret to protect.',
+    },
+    optional_secret: {
+      type:      'Optional[Sensitive[String]]',
+      desc:      'An optional secret to protect.',
+    },
+    array_secret: {
+      type:      'Array[Sensitive[String]]',
+      desc:      'An array secret to protect.',
+    },
+  },
+)

spec/fixtures/test_module/spec/unit/puppet/provider/test_sensitive/test_sensitive_spec.rb

@@ -0,0 +1,49 @@
+require 'spec_helper'
+
+ensure_module_defined('Puppet::Provider::TestSensitive')
+require 'puppet/provider/test_sensitive/test_sensitive'
+
+RSpec.describe Puppet::Provider::TestSensitive::TestSensitive do
+  subject(:provider) { described_class.new }
+
+  let(:context) { instance_double('Puppet::ResourceApi::BaseContext', 'context') }
+
+  describe '#get' do
+    it 'processes resources' do
+      expect(provider.get(context)).to eq [
+        {
+          name: 'foo',
+          ensure: 'present',
+        },
+        {
+          name: 'bar',
+          ensure: 'present',
+        },
+      ]
+    end
+  end
+
+  describe 'create(context, name, should)' do
+    it 'creates the resource' do
+      expect(context).to receive(:notice).with(%r{\ACreating 'a'})
+
+      provider.create(context, 'a', name: 'a', ensure: 'present')
+    end
+  end
+
+  describe 'update(context, name, should)' do
+    it 'updates the resource' do
+      expect(context).to receive(:notice).with(%r{\AUpdating 'foo'})
+
+      provider.update(context, 'foo', name: 'foo', ensure: 'present')
+    end
+  end
+
+  describe 'delete(context, name, should)' do
+    it 'deletes the resource' do
+      expect(context).to receive(:notice).with(%r{\ADeleting 'foo'})
+
+      provider.delete(context, 'foo')
+    end
+  end
+end

spec/fixtures/test_module/spec/unit/puppet/type/test_sensitive_spec.rb

@@ -0,0 +1,8 @@
+require 'spec_helper'
+require 'puppet/type/test_sensitive'
+
+RSpec.describe 'the test_sensitive type' do
+  it 'loads' do
+    expect(Puppet::Type.type(:test_sensitive)).not_to be_nil
+  end
+end