Merge pull request #6727 from melissa/ticket/master/PUP-8514-sensitiv…

…e-params

(PUP-8514) Define sensitive parameters globally

lib/puppet/parameter.rb

@@ -93,6 +93,14 @@ def defaultto(value = nil, &block)
       end
     end
 
+    def sensitive(value = nil, &block)
+      if block
+        define_method(:is_sensitive, &block)
+      else
+        define_method(:is_sensitive) do value end
+      end
+    end
+
     # Produces a documentation string.
     # If an enumeration of _valid values_ has been defined, it is appended to the documentation
     # for this parameter specified with the {desc} method.

lib/puppet/type.rb

@@ -826,6 +826,7 @@ def property(name)
   #
   def set_default(attr)
     return unless klass = self.class.attrclass(attr)
+    # TODO this is not a necessary check, as we define a class level attr_reader
     return unless klass.method_defined?(:default)
     return if @parameters.include?(klass.name)
 
@@ -2443,6 +2444,13 @@ def set_sensitive_parameters(sensitive_parameters)
         err(_("Unable to mark '%{name}' as sensitive: the property itself is not defined on %{type}.") % { name: name, type: type })
       end
     end
+
+    parameters.each do |name, param|
+      next if param.sensitive
+      if param.is_a?(Puppet::Parameter)
+        param.sensitive = param.is_sensitive if param.respond_to?(:is_sensitive)
+      end
+    end
   end
 
   private

lib/puppet/type/user.rb

@@ -224,21 +224,7 @@ def change_to_s(currentvalue, newvalue)
         raise ArgumentError, _("Passwords cannot include ':'") if value.is_a?(String) and value.include?(":")
       end
 
-      def change_to_s(currentvalue, newvalue)
-        if currentvalue == :absent
-          return _("created password")
-        else
-          return _("changed password")
-        end
-      end
-
-      def is_to_s( currentvalue )
-        return _('[old password hash redacted]')
-      end
-      def should_to_s( newvalue )
-        return _('[new password hash redacted]')
-      end
-
+      sensitive true
     end
 
     newproperty(:password_min_age, :required_features => :manages_password_age) do

lib/puppet/type/yumrepo.rb

@@ -326,6 +326,8 @@
     desc "Password for this proxy. #{ABSENT_DOC}"
 
     newvalues(/.*/, :absent)
+
+    sensitive true
   end
 
   newproperty(:s3_enabled) do
@@ -428,12 +430,6 @@
     desc "Password to use with the username for basic authentication.
       #{ABSENT_DOC}"
     newvalues(/.*/, :absent)
-  end
-
-  private
-
-  def set_sensitive_parameters(sensitive_parameters)
-    parameter(:password).sensitive = true if parameter(:password)
-    super(sensitive_parameters)
+    sensitive true
   end
 end

spec/unit/type/user_spec.rb

@@ -325,24 +325,24 @@ def self.instances; []; end
   end
 
   describe "when managing passwords" do
-    before do
-      @password = described_class.new(:name => 'foo', :password => 'mypass').parameter(:password)
-    end
+    let(:transaction) { Puppet::Transaction.new(Puppet::Resource::Catalog.new, nil, nil) }
+    let(:harness) { Puppet::Transaction::ResourceHarness.new(transaction) }
+    let(:provider) { @provider_class.new(:name => 'foo', :ensure => :present) }
+    let(:resource) { described_class.new(:name => 'foo', :ensure => :present, :password => 'top secret', :provider => provider) }
 
     it "should not include the password in the change log when adding the password" do
-      expect(@password.change_to_s(:absent, "mypass")).not_to be_include("mypass")
+      status = harness.evaluate(resource)
+      sync_event = status.events[0]
+      expect(sync_event.message).not_to include('top secret')
+      expect(sync_event.message).to eql('changed [redacted] to [redacted]')
     end
 
     it "should not include the password in the change log when changing the password" do
-      expect(@password.change_to_s("other", "mypass")).not_to be_include("mypass")
-    end
-
-    it "should redact the password when displaying the old value" do
-      expect(@password.is_to_s("currentpassword")).to match(/^\[old password hash redacted\]$/)
-    end
-
-    it "should redact the password when displaying the new value" do
-      expect(@password.should_to_s("newpassword")).to match(/^\[new password hash redacted\]$/)
+      resource[:password] = 'super extra classified'
+      status = harness.evaluate(resource)
+      sync_event = status.events[0]
+      expect(sync_event.message).not_to include('super extra classified')
+      expect(sync_event.message).to eql('changed [redacted] to [redacted]')
     end
 
     it "should fail if a ':' is included in the password" do

spec/unit/type/yumrepo_spec.rb

@@ -311,6 +311,36 @@
 
     describe "proxy_password" do
       it_behaves_like "a yumrepo parameter that can be absent", :proxy_password
+
+      context "for password information in the logs" do
+        let(:transaction) { Puppet::Transaction.new(Puppet::Resource::Catalog.new, nil, nil) }
+        let(:harness) { Puppet::Transaction::ResourceHarness.new(transaction) }
+        let(:provider_class) { described_class.provide(:simple) do
+          mk_resource_methods
+          def create; end
+          def delete; end
+          def exists?; get(:ensure) != :absent; end
+          def flush; end
+          def self.instances; []; end
+        end
+        }
+        let(:provider) { provider_class.new(:name => 'foo', :ensure => :present) }
+        let(:resource) { described_class.new(:name => 'puppetlabs', :proxy_password => 'top secret', :provider => provider) }
+
+        it "redacts on creation" do
+          status = harness.evaluate(resource)
+          sync_event = status.events[0]
+          expect(sync_event.message).to eq 'changed [redacted] to [redacted]'
+        end
+
+        it "redacts on update" do
+          harness.evaluate(resource)
+          resource[:proxy_password] = 'super classified'
+          status = harness.evaluate(resource)
+          sync_event = status.events[0]
+          expect(sync_event.message).to eq 'changed [redacted] to [redacted]'
+        end
+      end
     end
 
     describe "s3_enabled" do

spec/unit/type_spec.rb

@@ -736,6 +736,46 @@ class something {
     end
   end
 
+  describe "#set_sensitive_parameters" do
+    let(:sensitive_type) do
+      Puppet::Type.newtype(:sensitive_test) do
+        newparam(:name) { isnamevar }
+        newproperty(:secret) do
+          newvalues(/.*/)
+          sensitive true
+        end
+        newproperty(:transparency) do
+          newvalues(/.*/)
+          sensitive false
+        end
+        newproperty(:things) { newvalues(/.*/) }
+      end
+    end
+
+    it "should mark properties as sensitive" do
+      resource = sensitive_type.new(:name => 'foo', :secret => 'uber classified')
+      expect(resource.parameters[:secret].sensitive).to be true
+    end
+
+    it "should not have a sensitive flag when not set" do
+      resource = sensitive_type.new(:name => 'foo', :things => '1337')
+      expect(resource.parameters[:things].sensitive).to be_nil
+    end
+
+    it "should define things as not sensitive" do
+      resource = sensitive_type.new(:name => 'foo', :transparency => 'public knowledge')
+      expect(resource.parameters[:transparency].sensitive).to be false
+    end
+
+    it "should honor when sensitivity is set in a manifest" do
+      resource = sensitive_type.new(:name => 'foo',
+                                    :transparency => Puppet::Pops::Types::PSensitiveType::Sensitive.new('top secret'),
+                                    :sensitive_parameters => [:transparency]
+                                    )
+      expect(resource.parameters[:transparency].sensitive).to be true
+    end
+  end
+
   describe "when #finish is called on a type" do
     let(:post_hook_type) do
       Puppet::Type.newtype(:finish_test) do