voxpupuli / puppet-yum

Puppet module for Yum
https://forge.puppet.com/puppet/yum
MIT License
16 stars 99 forks source link

Add sensitive support for configs #275

Closed teluq-pbrideau closed 1 year ago

teluq-pbrideau commented 1 year ago

Pull Request (PR) description

I would like the proxy_password config not be leaked in the puppet log. This PR address this issue by disabling the show_diff when the given config is of type Sensitive

class example {
  $secret = Sensitive('mysecretpassword')
  class { 'yum' :
    config_options => {
      'proxy'          => 'https://host.example.com:3128',
      'proxy_username' => 'user',
      'proxy_password' => $secret,
    },
  }
}

I could add support for Sensitive[Boolean] and Sensitive[Integer] also if you think it is necessary, feel free to ask for it. I was not sure it would be required.

teluq-pbrideau commented 1 year ago

@bastelfreak Thanks for this quick merge, but I’ve discovered a problem today about this feature: The password is not censored when another config is changed:

Notice: Augeas[yum.conf_proxy_username](provider=augeas): 
--- /etc/yum.conf       2022-09-28 10:53:13.958280359 -0400
+++ /etc/yum.conf.augnew        2022-09-28 11:44:01.581689900 -0400
@@ -10,5 +10,5 @@
 metadata_expire=0
 mirrorlist_expire=0
 proxy=http://host.example.com:3128
-proxy_username=user
+proxy_username=anotheruser
 proxy_password=mysecretpassword

Notice: /Stage[main]/Yum/Yum::Config[proxy_username]/Augeas[yum.conf_proxy_username]/returns: executed successfully (corrective)

It might lure people in a false sense of security by using this config... The better way to deal with this would probably use a variable yum::show_diff and completely revert this commit. I’ll gladly provide the PR and tests. What do you think?

bastelfreak commented 1 year ago

I still think that the sensitive type is a good idea, it should not be reverted. However, adding the show_diff param would be a good addition (and maybe supporting sensitive for all parameters in that file).

teluq-pbrideau commented 1 year ago

All right, i’ll make another PR about the show_diff. :+1:

I do not understand what you means when you say «supporting sensitive for all parameters». Do you mean supporting the sensitive type for $yum::repos for example? I don’t see a case where I would want to censor the repository I connect to, but I guess someone might want that somehow…