Pass secrets as sensitive data types to katello/candlepin #493
Conversation
katello/candlepin 12.2.0 introduced support for Sensitive and this utilizes it. This allows Puppet to better avoid logging passwords.
|
@cocker-cc I think this replaces #436 and goes a step further. Would you mind taking a look? |
| @@ -29,7 +29,7 @@ | |||
| Optional[Stdlib::Port] $db_port = undef, | |||
| String $db_name = 'candlepin', | |||
| String $db_user = 'candlepin', | |||
| Optional[String] $db_password = undef, | |||
| Variant[Undef, Sensitive[String], String] $db_password = undef, | |||
There was a problem hiding this comment.
Why do you prefer Variant[Undef… over Optional[?
The latter is more common.
There was a problem hiding this comment.
I'm not sure. It's a bit shorter but perhaps also less readable
| @@ -75,7 +75,7 @@ | |||
| db_port => $candlepin_db_port, | |||
| db_name => $candlepin_db_name, | |||
| db_user => $candlepin_db_user, | |||
| db_password => $candlepin_db_password, | |||
| db_password => if $candlepin_db_password { Sensitive($candlepin_db_password) } else { $candlepin_db_password }, | |||
There was a problem hiding this comment.
A Sensitive[Sensitive[String]] could be produced here. This is not good.
Perhaps you mean
db_password => if $candlepin_db_password =~ Sensitive { $candlepin_db_password } else { Sensitive($candlepin_db_password) },but this could be written simpler with
db_password => Sensitive($candlepin_db_password.unwrap),There was a problem hiding this comment.
I want to avoid Sensitive[Undef]. Perhaps that's already solved. I suppose I can check for String explicitly
| @@ -54,9 +54,9 @@ | |||
| ca_key => $certs::candlepin::ca_key, | |||
| ca_cert => $certs::candlepin::ca_cert, | |||
| keystore_file => $certs::candlepin::keystore, | |||
| keystore_password => $certs::candlepin::keystore_password, | |||
| keystore_password => Sensitive($certs::candlepin::keystore_password), | |||
There was a problem hiding this comment.
Coding for the Future:
keystore_password => Sensitive($certs::candlepin::keystore_password.unwrap),because the Day will come, when certs also uses Sensitive, and then you would have Sensitive[Sensitive[String]].
| @@ -55,8 +55,8 @@ | |||
| Integer[0] $hosts_queue_workers = 1, | |||
| ) { | |||
| class { 'katello::params': | |||
| candlepin_oauth_key => $candlepin_oauth_key, | |||
| candlepin_oauth_secret => $candlepin_oauth_secret, | |||
| candlepin_oauth_key => Sensitive($candlepin_oauth_key), | |||
There was a problem hiding this comment.
IMHO wrong Order. Define in the Class-Header
Optional[Variant[Sensitive[String], String]] $candlepin_oauth_key = undef,and just pass over the Variable here.
There was a problem hiding this comment.
My Guideline is:
If You get a Sensitive, then you must pass it over as Sensitive, and if the receiving Module cannot deal with it, then Work should be done to improve this.
If you already get an un-Sensitive, then there is no need to cast it to Sensitive, because the Damage already happend. Work should be done on the sending Side to not receive this un-Sensitive.
There was a problem hiding this comment.
I was conservative here: I don't yet know if our installer handles it properly and only init.pp is exposed there. But I'll evaluate it more in depth
katello/candlepin 12.2.0 introduced support for Sensitive and this utilizes it. This allows Puppet to better avoid logging passwords.