Allow overriding passwords generated by extlib #409

coreone · 2023-06-08T18:03:10Z

No description provided.

ekohl

Apologies for missing this completely while I was on vacation, but I do like this change.

manifests/candlepin.pp

ekohl · 2023-08-08T17:47:13Z

manifests/init.pp

@@ -73,6 +75,7 @@
  Boolean $generate = true,
  Boolean $regenerate = false,
  Boolean $deploy = true,
+  String[1] $ca_key_password = $certs::params::ca_key_password,


What's the reason for moving this parameter to another line?

I also like the data type, but should we enforce at least X characters? for NSS we use 10 and I think that's a good number.

This wasn't a parameter before. The other line in the diff was where it was defined inside the class. I also increased the string size to 10 per your suggestion.

ekohl · 2023-08-08T17:49:19Z

manifests/params.pp

+  # Generate and cache the password on the master once
+  # In multi-puppetmaster setups, the user should specify their own
+  $ca_key_password = extlib::cache_data('foreman_cache_data', 'ca_key_password', extlib::random_password(24))
+  $nss_db_password = extlib::cache_data('foreman_cache_data', 'certs-nss-db-password', extlib::random_password(32))


What's the reason to move the NSS DB pass to params.pp? Looks like we can continue using it in certs::ssltools:nssdb. You do need to set it via Hiera, but you don't introduce it as a top level parameter anyway.

This was just following a pattern. Most of the other modules I looked at had this kind of thing done in params.pp, so I just did it there as well. I can move it out to the class itself if that makes more sense.

Ideally we would get rid of params.pp. In our installer we use Kafo, which can't deal with function calls in parameters (default parsing breaks). Now I do see that ca_key_password already was doing that in init.pp so now I wonder if that was broken all along.

Ah, I missed something in my review: it's in the body now, not the actual class parameter. That's why it's not a problem today.

coreone · 2023-09-05T14:57:54Z

@ekohl anything else needed on this?

ekohl · 2023-09-12T11:18:14Z

manifests/candlepin.pp

+  # Generate and cache the password on the master once
+  # In multi-puppetmaster setups, the user should specify their own
+  $final_keystore_password = pick(
+    $keystore_password, extlib::cache_data('foreman_cache_data', $keystore_password_file, extlib::random_password(32))


This may be a slight nuance, but in this case I do think the data is generated and cached, even if unused. Would it make sense to use a more traditional approach with if/else here?

Changed these 2 lines into if/else blocks

ekohl · 2023-09-12T11:20:08Z

manifests/init.pp

@@ -73,6 +75,7 @@
  Boolean $generate = true,
  Boolean $regenerate = false,
  Boolean $deploy = true,
+  String[10] $ca_key_password = $certs::params::ca_key_password,


Can you keep it on the line where it used to be? So between ca_cert_stripped and ca_key_password_file?

This is new because ca_key_password was not in the parameters for the class previously.

I'm debating having a default or not. Having it means it ends up in our installer's answer files. Not sure if that's a benefit or a downside. @ehelms?

I also wonder what happens if you change the password parameter. I fear it would break since there's no support to change the password.

I'm a bit confused. I do not like having it as a default and showing up in the answers file. This currently already has a default coming from params.pp?

The diff in GH isn't clear (I also failed at that at first). Right now it's in the body of the class. This PR moves it to a class parameter (with indirection via params since Kafo can't deal with calling functions as defaults).

@ehelms can we revisit this? I still think this is a good direction.

I'm still leaning to undef and also support Sensitive. So Variant[Undef, Sensitive[String[10]], String[10]]] $ca_key_password = undef (or Optional[Variant[Sensitive[String[10]], String[10]]] which is equivalent).

If I understand correctly, for an existing setup, if a new ca_key_password is provided this will force a new key to be generated, which forces a new CA to be generated which cascades and forces all smart-proxy / Capsules and clients to need to be updated.

Will the code properly handle that? I think we need a test for it. How do we help users not break their installations accidentally?

One data point that just came up today: if puppet-agent is removed then the cache is wiped. If the ssl-build directory isn't wiped, then there's a mismatch. If we have the value as an explicit parameter, the value is stored in the answer file and there wouldn't be an issue. So an argument against undef as default and in favor of the current code.

ekohl · 2023-09-28T15:38:59Z

I did some testing: if you change the password for either the keystore or the truststore it fails to open it:

Error 1: Puppet Truststore_certificate resource '/etc/candlepin/certs/truststore:artemis-client' failed. Logs:
  /Stage[main]/Certs::Candlepin/Truststore_certificate[/etc/candlepin/certs/truststore:artemis-client]
    Adding autorequire relationship with File[/etc/pki/katello/truststore_password-file]
    Adding autorequire relationship with File[/etc/foreman/client_cert.pem]
    Adding autonotify relationship with File[/etc/candlepin/certs/truststore]
    Starting to evaluate the resource (581 of 1413)
    Evaluated in 0.46 seconds
  /Stage[main]/Certs::Candlepin/Truststore_certificate[/etc/candlepin/certs/truststore:artemis-client]/ensure
    change from 'absent' to 'present' failed: Execution of '/bin/keytool -import -v -noprompt -storetype pkcs12 -keystore /etc/candlepin/certs/truststore -alias artemis-client -file /etc/foreman/client_cert.pem -storepass:file /etc/pki/katello/truststore_password-file -J-Dcom.redhat.fips=false' returned 1: keytool error: java.io.IOException: keystore password was incorrect
java.io.IOException: keystore password was incorrect
	at sun.security.pkcs12.PKCS12KeyStore.engineLoad(PKCS12KeyStore.java:2089)
	at java.security.KeyStore.load(KeyStore.java:1445)
	at sun.security.tools.keytool.Main.doCommands(Main.java:839)
	at sun.security.tools.keytool.Main.run(Main.java:380)
	at sun.security.tools.keytool.Main.main(Main.java:373)
Caused by: java.security.UnrecoverableKeyException: failed to decrypt safe contents entry: javax.crypto.BadPaddingException: Given final block not properly padded. Such issues can arise if a bad key is used during decryption.
	... 5 more
Error 2: Puppet Truststore_certificate resource '/etc/candlepin/certs/truststore:candlepin-ca' failed. Logs:
  /Stage[main]/Certs::Candlepin/Truststore_certificate[/etc/candlepin/certs/truststore:candlepin-ca]
    Adding autorequire relationship with File[/etc/pki/katello/truststore_password-file]
    Adding autorequire relationship with File[/etc/candlepin/certs/candlepin-ca.crt]
    Adding autonotify relationship with File[/etc/candlepin/certs/truststore]
    Starting to evaluate the resource (602 of 1413)
    Evaluated in 0.36 seconds
  /Stage[main]/Certs::Candlepin/Truststore_certificate[/etc/candlepin/certs/truststore:candlepin-ca]/ensure
    change from 'absent' to 'present' failed: Execution of '/bin/keytool -import -v -noprompt -storetype pkcs12 -keystore /etc/candlepin/certs/truststore -alias candlepin-ca -file /etc/candlepin/certs/candlepin-ca.crt -storepass:file /etc/pki/katello/truststore_password-file -J-Dcom.redhat.fips=false' returned 1: keytool error: java.io.IOException: keystore password was incorrect
java.io.IOException: keystore password was incorrect
	at sun.security.pkcs12.PKCS12KeyStore.engineLoad(PKCS12KeyStore.java:2089)
	at java.security.KeyStore.load(KeyStore.java:1445)
	at sun.security.tools.keytool.Main.doCommands(Main.java:839)
	at sun.security.tools.keytool.Main.run(Main.java:380)
	at sun.security.tools.keytool.Main.main(Main.java:373)
Caused by: java.security.UnrecoverableKeyException: failed to decrypt safe contents entry: javax.crypto.BadPaddingException: Given final block not properly padded. Such issues can arise if a bad key is used during decryption.
	... 5 more

You can delete the file and it will be recreated, but rotating passwords is not supported. I'm not sure what's the best solution to prevent users from this footgun.

@ehelms any thoughts on changing the keystore & truststore types to catch the password error and recreate the file?

ehelms · 2023-09-28T15:41:17Z

@ehelms any thoughts on changing the keystore & truststore types to catch the password error and recreate the file?

Possible to add I should think -- if password change is detected we need to trigger a delete of the truststore and then let it re-build itself.

ekohl · 2023-09-28T16:24:10Z

I've opened #428 but it doesn't work yet. I'll continue on it tomorrow.

coreone · 2023-12-20T14:57:53Z

@ekohl circling back around to this after I see that the #428 has been merged.

Update suggestions

ehelms · 2024-05-15T21:38:38Z

I think this needs at least two acceptance tests added:

Providing a password on fresh run
Changing the password for an existing setup

ekohl · 2024-08-08T17:48:34Z

@coreone Any chance you could take a look at the acceptance tests @ehelms asked about?

theforeman-bot added the Not yet reviewed label Jun 8, 2023

coreone force-pushed the passwords branch from 4fb1059 to 3f499cc Compare June 10, 2023 17:23

ekohl reviewed Aug 8, 2023

View reviewed changes

coreone force-pushed the passwords branch 2 times, most recently from be4f369 to 7b4234a Compare August 11, 2023 14:25

ekohl reviewed Sep 12, 2023

View reviewed changes

coreone force-pushed the passwords branch 3 times, most recently from d1e4700 to 7a7de26 Compare September 23, 2023 12:04

ekohl added the Enhancement label Nov 7, 2023

coreone force-pushed the passwords branch from 7a7de26 to 6b21ed2 Compare December 20, 2023 14:45

Allow overriding passwords generated by extlib

41453c5

Update suggestions

coreone force-pushed the passwords branch from 6b21ed2 to 41453c5 Compare April 4, 2024 14:53

ekohl added Waiting on contributor and removed Not yet reviewed labels Aug 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow overriding passwords generated by extlib #409

Allow overriding passwords generated by extlib #409

coreone commented Jun 8, 2023

ekohl left a comment

ekohl Aug 8, 2023

coreone Aug 11, 2023

ekohl Aug 8, 2023

coreone Aug 11, 2023

ekohl Sep 12, 2023

ekohl Sep 15, 2023

coreone commented Sep 5, 2023

ekohl Sep 12, 2023

coreone Sep 23, 2023

ekohl Sep 12, 2023

coreone Sep 23, 2023

ekohl Sep 27, 2023

ehelms Sep 27, 2023

ekohl Sep 28, 2023

ekohl May 15, 2024

ehelms May 15, 2024

ekohl May 16, 2024

ekohl commented Sep 28, 2023

ehelms commented Sep 28, 2023

ekohl commented Sep 28, 2023

coreone commented Dec 20, 2023

ehelms commented May 15, 2024

ekohl commented Aug 8, 2024

Allow overriding passwords generated by extlib #409

Are you sure you want to change the base?

Allow overriding passwords generated by extlib #409

Conversation

coreone commented Jun 8, 2023

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coreone commented Sep 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl commented Sep 28, 2023

ehelms commented Sep 28, 2023

ekohl commented Sep 28, 2023

coreone commented Dec 20, 2023

ehelms commented May 15, 2024

ekohl commented Aug 8, 2024