Merge pull request puppetlabs#1400 from cocker-cc/Use_Puppet-Datatype_Sensitive

pmcmaw · web-flow · commit 1fb724589487 · 2021-06-22T14:44:46.000+01:00
Use Puppet-Datatype Sensitive
diff --git a/.puppet-lint.rc b/.puppet-lint.rc
@@ -1 +1,2 @@
 --relative
+--no-140chars-check
diff --git a/lib/puppet/functions/mysql/password.rb b/lib/puppet/functions/mysql/password.rb
@@ -7,18 +7,35 @@
 Puppet::Functions.create_function(:'mysql::password') do
   # @param password
   #   Plain text password.
+  # @param sensitive
+  #   If the Postgresql-Passwordhash should be of Datatype Sensitive[String]
   #
   # @return hash
   #   The mysql password hash from the clear text password.
   #
   dispatch :password do
-    required_param 'String', :password
-    return_type 'String'
+    required_param 'Variant[String, Sensitive[String]]', :password
+    optional_param 'Boolean', :sensitive
+    return_type 'Variant[String, Sensitive[String]]'
   end
 
-  def password(password)
-    return '' if password.empty?
-    return password if %r{\*[A-F0-9]{40}$}.match?(password)
-    '*' + Digest::SHA1.hexdigest(Digest::SHA1.digest(password)).upcase
+  def password(password, sensitive = false)
+    if password.is_a?(Puppet::Pops::Types::PSensitiveType::Sensitive)
+      password = password.unwrap
+    end
+
+    result_string = if %r{\*[A-F0-9]{40}$}.match?(password)
+                      password
+                    elsif password.empty?
+                      ''
+                    else
+                      '*' + Digest::SHA1.hexdigest(Digest::SHA1.digest(password)).upcase
+                    end
+
+    if sensitive
+      Puppet::Pops::Types::PSensitiveType::Sensitive.new(result_string)
+    else
+      result_string
+    end
   end
 end
diff --git a/lib/puppet/functions/mysql_password.rb b/lib/puppet/functions/mysql_password.rb
@@ -8,12 +8,13 @@
   # @return
   #   The mysql password hash from the 4.x function mysql::password.
   dispatch :mysql_password do
-    required_param 'String', :password
-    return_type 'String'
+    required_param 'Variant[String, Sensitive[String]]', :password
+    optional_param 'Boolean', :sensitive
+    return_type 'Variant[String, Sensitive[String]]'
   end
 
-  def mysql_password(password)
+  def mysql_password(password, sensitive = false)
     call_function('deprecation', 'mysql_password', "This method has been deprecated, please use the namespaced version 'mysql::password' instead.")
-    call_function('mysql::password', password)
+    call_function('mysql::password', password, sensitive)
   end
 end
diff --git a/lib/puppet/provider/mysql_user/mysql.rb b/lib/puppet/provider/mysql_user/mysql.rb
@@ -74,6 +74,8 @@ def create
     max_updates_per_hour     = @resource.value(:max_updates_per_hour) || 0
     tls_options              = @resource.value(:tls_options) || ['NONE']
 
+    password_hash = password_hash.unwrap if password_hash.is_a?(Puppet::Pops::Types::PSensitiveType::Sensitive)
+
     # Use CREATE USER to be compatible with NO_AUTO_CREATE_USER sql_mode
     # This is also required if you want to specify a authentication plugin
     if !plugin.nil?
diff --git a/manifests/backup/mysqlbackup.pp b/manifests/backup/mysqlbackup.pp
@@ -5,7 +5,7 @@
 #
 class mysql::backup::mysqlbackup (
   $backupuser               = '',
-  $backuppassword           = '',
+  Variant[String, Sensitive[String]] $backuppassword = '',
   $maxallowedpacket         = '1M',
   $backupdir                = '',
   $backupdirmode            = '0700',
@@ -32,6 +32,11 @@
   $compression_command      = undef,
   $compression_extension    = undef,
 ) inherits mysql::params {
+  $backuppassword_unsensitive = if $backuppassword =~ Sensitive {
+    $backuppassword.unwrap
+  } else {
+    $backuppassword
+  }
   mysql_user { "${backupuser}@localhost":
     ensure        => $ensure,
     password_hash => mysql::password($backuppassword),
@@ -104,7 +109,7 @@
       'incremental_base'       => 'history:last_backup',
       'incremental_backup_dir' => $backupdir,
       'user'                   => $backupuser,
-      'password'               => $backuppassword,
+      'password'               => $backuppassword_unsensitive
     },
   }
   $options = mysql::normalise_and_deepmerge($default_options, $mysql::server::override_options)
diff --git a/manifests/backup/mysqldump.pp b/manifests/backup/mysqldump.pp
@@ -4,7 +4,7 @@
 #
 class mysql::backup::mysqldump (
   $backupuser               = '',
-  $backuppassword           = '',
+  Variant[String, Sensitive[String]] $backuppassword = '',
   $backupdir                = '',
   $maxallowedpacket         = '1M',
   $backupdirmode            = '0700',
@@ -33,6 +33,12 @@
   $compression_command      = 'bzcat -zc',
   $compression_extension    = '.bz2'
 ) inherits mysql::params {
+  $backuppassword_unsensitive = if $backuppassword =~ Sensitive {
+    $backuppassword.unwrap
+  } else {
+    $backuppassword
+  }
+
   unless $::osfamily == 'FreeBSD' {
     if $backupcompress and $compression_command == 'bzcat -zc' {
       ensure_packages(['bzip2'])
@@ -82,6 +88,7 @@
     require  => File['mysqlbackup.sh'],
   }
 
+  # TODO: use EPP instead of ERB, as EPP can handle Data of Type Sensitive without further ado
   file { 'mysqlbackup.sh':
     ensure  => $ensure,
     path    => '/usr/local/sbin/mysqlbackup.sh',
diff --git a/manifests/backup/xtrabackup.pp b/manifests/backup/xtrabackup.pp
@@ -5,7 +5,7 @@
 class mysql::backup::xtrabackup (
   $xtrabackup_package_name  = $mysql::params::xtrabackup_package_name,
   $backupuser               = undef,
-  $backuppassword           = undef,
+  Optional[Variant[String, Sensitive[String]]] $backuppassword = undef,
   $backupdir                = '',
   $maxallowedpacket         = '1M',
   $backupmethod             = 'xtrabackup',
@@ -36,6 +36,12 @@
 ) inherits mysql::params {
   ensure_packages($xtrabackup_package_name)
 
+  $backuppassword_unsensitive = if $backuppassword =~ Sensitive {
+    $backuppassword.unwrap
+  } else {
+    $backuppassword
+  }
+
   if $backupuser and $backuppassword {
     mysql_user { "${backupuser}@localhost":
       ensure        => $ensure,
@@ -121,6 +127,7 @@
     group  => $backupdirgroup,
   }
 
+  # TODO: use EPP instead of ERB, as EPP can handle Data of Type Sensitive without further ado
   file { 'xtrabackup.sh':
     ensure  => $ensure,
     path    => '/usr/local/sbin/xtrabackup.sh',
diff --git a/manifests/bindings.pp b/manifests/bindings.pp
@@ -102,13 +102,13 @@
 ) inherits mysql::params {
   case $::osfamily {
     'Archlinux': {
-      if $java_enable { fail("::mysql::bindings::java cannot be managed by puppet on ${osfamily}
+      if $java_enable { fail("::mysql::bindings::java cannot be managed by puppet on ${::facts['os']['family']}
                           as it is not in official repositories. Please disable java mysql binding.") }
       if $perl_enable { include 'mysql::bindings::perl' }
-      if $php_enable { warning("::mysql::bindings::php does not need to be managed by puppet on ${osfamily}
+      if $php_enable { warning("::mysql::bindings::php does not need to be managed by puppet on ${::facts['os']['family']}
                           as it is included in mysql package by default.") }
       if $python_enable { include 'mysql::bindings::python' }
-      if $ruby_enable { fail("::mysql::bindings::ruby cannot be managed by puppet on %{osfamily}
+      if $ruby_enable { fail("::mysql::bindings::ruby cannot be managed by puppet on %{::facts['os']['family']}
                           as it is not in official repositories. Please disable ruby mysql binding.") }
     }
 
diff --git a/manifests/bindings/client_dev.pp b/manifests/bindings/client_dev.pp
@@ -12,6 +12,6 @@
       provider        => $mysql::bindings::client_dev_package_provider,
     }
   } else {
-    warning("No MySQL client development package configured for ${os}.")
+    warning("No MySQL client development package configured for ${::facts['os']['family']}.")
   }
 }
diff --git a/manifests/bindings/daemon_dev.pp b/manifests/bindings/daemon_dev.pp
@@ -12,6 +12,6 @@
       provider        => $mysql::bindings::daemon_dev_package_provider,
     }
   } else {
-    warning("No MySQL daemon development package configured for ${os}.")
+    warning("No MySQL daemon development package configured for ${::facts['os']['family']}.")
   }
 }
diff --git a/manifests/db.pp b/manifests/db.pp
@@ -40,7 +40,7 @@
 #
 define mysql::db (
   $user,
-  $password,
+  Variant[String, Sensitive[String]] $password,
   $tls_options                                = undef,
   $dbname                                     = $name,
   $charset                                    = 'utf8',
diff --git a/manifests/params.pp b/manifests/params.pp
@@ -259,7 +259,7 @@
         $python_package_name = 'python-mysqldb'
       }
 
-      $ruby_package_name   =  $facts['operatingsystemmajrelease']  ? {
+      $ruby_package_name   =  $facts['os']['release']['major']  ? {
         '8'     => 'ruby-mysql', # jessie
         '9'     => 'ruby-mysql2', # stretch
         '10'    => 'ruby-mysql2', # buster
diff --git a/manifests/server.pp b/manifests/server.pp
@@ -98,7 +98,7 @@
   $mysql_group             = $mysql::params::mysql_group,
   $mycnf_owner             = $mysql::params::mycnf_owner,
   $mycnf_group             = $mysql::params::mycnf_group,
-  $root_password           = $mysql::params::root_password,
+  Variant[String, Sensitive[String]] $root_password = $mysql::params::root_password,
   $service_enabled         = $mysql::params::server_service_enabled,
   $service_manage          = $mysql::params::server_service_manage,
   $service_name            = $mysql::params::server_service_name,
diff --git a/manifests/server/backup.pp b/manifests/server/backup.pp
@@ -74,7 +74,7 @@
 #   Configure the file extension for the compressed backup (when using the mysqldump provider)
 class mysql::server::backup (
   $backupuser               = undef,
-  $backuppassword           = undef,
+  Optional[Variant[String, Sensitive[String]]] $backuppassword = undef,
   $backupdir                = undef,
   $backupdirmode            = '0700',
   $backupdirowner           = 'root',
diff --git a/manifests/server/monitor.pp b/manifests/server/monitor.pp
@@ -10,7 +10,7 @@
 #
 class mysql::server::monitor (
   $mysql_monitor_username = '',
-  $mysql_monitor_password = '',
+  Optional[Variant[String, Sensitive[String]]] $mysql_monitor_password = '',
   $mysql_monitor_hostname = ''
 ) {
   Anchor['mysql::server::end'] -> Class['mysql::server::monitor']
diff --git a/manifests/server/root_password.pp b/manifests/server/root_password.pp
@@ -1,9 +1,20 @@
-# @summary 
+# @summary
 #   Private class for managing the root password
 #
 # @api private
 #
 class mysql::server::root_password {
+  if $mysql::server::root_password =~ Sensitive {
+    $root_password = $mysql::server::root_password.unwrap
+  } else {
+    $root_password = $mysql::server::root_password
+  }
+  if $root_password == 'UNSET' {
+    $root_password_set = false
+  } else {
+    $root_password_set = true
+  }
+
   $options = $mysql::server::_options
   $secret_file = $mysql::server::install_secret_file
   $login_file = $mysql::server::login_file
@@ -23,15 +34,16 @@
   }
 
   # manage root password if it is set
-  if $mysql::server::create_root_user == true and $mysql::server::root_password != 'UNSET' {
+  if $mysql::server::create_root_user and $root_password_set {
     mysql_user { 'root@localhost':
       ensure        => present,
       password_hash => mysql::password($mysql::server::root_password),
       require       => Exec['remove install pass'],
     }
   }
 
-  if $mysql::server::create_root_my_cnf == true and $mysql::server::root_password != 'UNSET' {
+  if $mysql::server::create_root_my_cnf and $root_password_set {
+    # TODO: use EPP instead of ERB, as EPP can handle Data of Type Sensitive without further ado
     file { "${::root_home}/.my.cnf":
       content => template('mysql/my.cnf.pass.erb'),
       owner   => 'root',
@@ -42,12 +54,12 @@
     if versioncmp($::puppetversion, '3.0') >= 0 {
       File["${::root_home}/.my.cnf"] { show_diff => false }
     }
-    if $mysql::server::create_root_user == true {
+    if $mysql::server::create_root_user {
       Mysql_user['root@localhost'] -> File["${::root_home}/.my.cnf"]
     }
   }
 
-  if $mysql::server::create_root_login_file == true and $mysql::server::root_password != 'UNSET' {
+  if $mysql::server::create_root_login_file and $root_password_set {
     file { "${::root_home}/.mylogin.cnf":
       source => $login_file,
       owner  => 'root',
diff --git a/spec/classes/mysql_server_spec.rb b/spec/classes/mysql_server_spec.rb
@@ -241,6 +241,29 @@
           }
         end
 
+        describe 'with users and Sensitive password_hash' do
+          let(:params) do
+            { users: {
+              'foo@localhost' => {
+                'max_connections_per_hour' => '1',
+                'max_queries_per_hour'     => '2',
+                'max_updates_per_hour'     => '3',
+                'max_user_connections'     => '4',
+                'password_hash'            => sensitive('*F3A2A51A9B0F2BE2468926B4132313728C250DBF'),
+              },
+              'foo2@localhost' => {},
+            } }
+          end
+
+          it {
+            is_expected.to contain_mysql_user('foo@localhost').with(
+              max_connections_per_hour: '1', max_queries_per_hour: '2',
+              max_updates_per_hour: '3', max_user_connections: '4',
+              password_hash: 'Sensitive [value redacted]'
+            )
+          }
+        end
+
         describe 'with grants' do
           let(:params) do
             { grants: {
diff --git a/spec/functions/mysql_password_spec.rb b/spec/functions/mysql_password_spec.rb
@@ -11,14 +11,24 @@
     is_expected.to run.with_params.and_raise_error(ArgumentError)
   end
 
-  it 'raises a ArgumentError if there is more than 1 arguments' do
-    is_expected.to run.with_params('foo', 'bar').and_raise_error(ArgumentError)
+  it 'raises a ArgumentError if there is more than 2 arguments' do
+    is_expected.to run.with_params('foo', false, 'bar').and_raise_error(ArgumentError)
   end
 
   it 'converts password into a hash' do
     is_expected.to run.with_params('password').and_return('*2470C0C06DEE42FD1618BB99005ADCA2EC9D1E19')
   end
 
+  it 'accept password as Sensitive' do
+    is_expected.to run.with_params(sensitive('password')).and_return('*2470C0C06DEE42FD1618BB99005ADCA2EC9D1E19')
+  end
+
+  # Test of a Returnvalue of Datatype Sensitive does not work
+  it 'returns Sensitive with sensitive=true' do
+    pending 'should have a Returnvalue of Datatype Sensitive'
+    is_expected.to run.with_params('password', true).and_return(sensitive('*2470C0C06DEE42FD1618BB99005ADCA2EC9D1E19'))
+  end
+
   it 'password should be String' do
     is_expected.to run.with_params(123).and_raise_error(ArgumentError)
   end
diff --git a/templates/my.cnf.pass.erb b/templates/my.cnf.pass.erb
@@ -4,8 +4,8 @@
 [<%= section -%>]
 user=root
 host=localhost
-<% unless scope.lookupvar('mysql::server::root_password') == 'UNSET' -%>
-password='<%= scope.lookupvar('mysql::server::root_password') %>'
+<% if @root_password_set -%>
+password='<%= @root_password %>'
 <% end -%>
 socket=<%= @options['client']['socket'] %>
 <% end %>
diff --git a/templates/mysqlbackup.sh.erb b/templates/mysqlbackup.sh.erb
@@ -15,7 +15,7 @@
 ##### START CONFIG ###################################################
 
 USER=<%= @backupuser %>
-PASS='<%= @backuppassword %>'
+PASS='<%= @backuppassword_unsensitive %>'
 MAX_ALLOWED_PACKET=<%= @maxallowedpacket %>
 DIR=<%= @backupdir %>
 ROTATE=<%= [ Integer(@backuprotate) - 1, 0 ].max %>
diff --git a/templates/xtrabackup.sh.erb b/templates/xtrabackup.sh.erb
@@ -39,8 +39,8 @@ cleanup
 
 <%- _innobackupex_args = '' -%>
 
-<%- if @backupuser and @backuppassword -%>
-  <%- _innobackupex_args = '--user="' + @backupuser + '" --password="' + @backuppassword + '"' -%>
+<%- if @backupuser and @backuppassword_unsensitive -%>
+  <%- _innobackupex_args = '--user="' + @backupuser + '" --password="' + @backuppassword_unsensitive + '"' -%>
 <%- end -%>
 
 <%- if @backupcompress -%>

Original file line number	Diff line number	Diff line change
`@@ -12,6 +12,6 @@`
`12`	`12`	`provider => $mysql::bindings::client_dev_package_provider,`
`13`	`13`	`}`
`14`	`14`	`} else {`
`15`		`- warning("No MySQL client development package configured for ${os}.")`
	`15`	`+ warning("No MySQL client development package configured for ${::facts['os']['family']}.")`
`16`	`16`	`}`
`17`	`17`	`}`