Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass secrets as sensitive data types to katello/candlepin #493

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+9 −9
Open

Pass secrets as sensitive data types to katello/candlepin #493

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions manifests/candlepin.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you prefer Variant[Undef… over Optional[?
The latter is more common.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. It's a bit shorter but perhaps also less readable

Boolean $db_ssl = false,
Boolean $db_ssl_verify = true,
Optional[Stdlib::Absolutepath] $db_ssl_ca = undef,
Expand All @@ -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),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]].

truststore_file => $certs::candlepin::truststore,
truststore_password => $certs::candlepin::truststore_password,
truststore_password => Sensitive($certs::candlepin::truststore_password),
artemis_client_dn => $artemis_client_dn,
java_home => '/usr/lib/jvm/jre-17',
java_package => 'java-17-openjdk',
Expand Down
8 changes: 4 additions & 4 deletions manifests/init.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
Optional[Stdlib::Port] $candlepin_db_port = undef,
String $candlepin_db_name = 'candlepin',
String $candlepin_db_user = 'candlepin',
Optional[String] $candlepin_db_password = undef,
Variant[Undef, Sensitive[String[1]], String] $candlepin_db_password = undef,
Boolean $candlepin_db_ssl = false,
Boolean $candlepin_db_ssl_verify = true,
Optional[Stdlib::Absolutepath] $candlepin_db_ssl_ca = undef,
Expand All @@ -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),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO wrong Order. Define in the Class-Header

Optional[Variant[Sensitive[String], String]] $candlepin_oauth_key = undef,

and just pass over the Variable here.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

candlepin_oauth_secret => Sensitive($candlepin_oauth_secret),
}

if $katello::params::meta_package != '' {
Expand All @@ -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 },
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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),

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to avoid Sensitive[Undef]. Perhaps that's already solved. I suppose I can check for String explicitly

db_ssl => $candlepin_db_ssl,
db_ssl_verify => $candlepin_db_ssl_verify,
db_ssl_ca => $candlepin_db_ssl_ca,
Expand Down
4 changes: 2 additions & 2 deletions manifests/params.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@
# @param postgresql_evr_package
# The contextual package name for the PostgreSQL EVR extension
class katello::params (
String[1] $candlepin_oauth_key = $katello::globals::candlepin_oauth_key,
String[1] $candlepin_oauth_secret = $katello::globals::candlepin_oauth_secret,
Variant[Sensitive[String[1], String[1]]] $candlepin_oauth_key = $katello::globals::candlepin_oauth_key,
Variant[Sensitive[String[1], String[1]]] $candlepin_oauth_secret = $katello::globals::candlepin_oauth_secret,
Stdlib::Host $candlepin_host = 'localhost',
Stdlib::Port $candlepin_port = 23443,
Stdlib::HTTPSUrl $candlepin_url = "https://${candlepin_host}:${candlepin_port}/candlepin",
Expand Down
Loading