ManageIQ / manageiq-pods

ManageIQ on Kubernetes and OpenShift
Apache License 2.0
50 stars 100 forks source link

Only return passwords that include letters. #1052

Closed bdunne closed 6 months ago

bdunne commented 7 months ago

Password decryption can fail if the database password is all numbers because ruby will read it as an integer instead of a string.

== Checking deployment status ==
rake aborted!
NoMethodError: undefined method `empty?' for 8783875645714351:Integer 
.../gems/manageiq-password-1.2.0/lib/manageiq/password.rb:165:in `remove_erb' 
.../gems/manageiq-password-1.2.0/lib/manageiq/password.rb:90:in `try_decrypt' 
/var/www/miq/vmdb/lib/vmdb/settings_walker.rb:76:in `block in decrypt_passwords!'
miq-bot commented 7 months ago

Checked commit https://github.com/bdunne/manageiq-pods/commit/57227fdce0499661786913ae252073cf7698c33a with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint 0 files checked, 0 offenses detected Everything looks fine. :trophy:

Fryguy commented 7 months ago

This feels like it's bandaiding the underlying problem. Why is Ruby seeing it as an integer in the first place? I would have expected it to see a String. Makes me think we are not encoding into yaml somewhere properly (and that gets me worried about passwords starting with ! for example)

bdunne commented 7 months ago

This feels like it's bandaiding the underlying problem. Why is Ruby seeing it as an integer in the first place? I would have expected it to see a String. Makes me think we are not encoding into yaml somewhere properly (and that gets me worried about passwords starting with ! for example)

The password is passed into the container via ENV or file mount here then written into database.yml. So, in this case we saw:

---
production:
  adapter: postgresql
  username: root
  password: 8783875645714351
...

And when it was YAML loaded, password became an integer.

Fryguy commented 7 months ago

Right, so that tells me we aren't writing the YAML correctly. The correct YAML should have been:

---
production:
  adapter: postgresql
  username: root
  password: "8783875645714351"

My concern is that there are other password characters that can cause similar issues, such as a leading !, ", or | character. I realize we use hex now, so that can't happen in this case, but I also don't think we should be using hex moving forward, but instead something like base64 with a wider character set.

Fryguy commented 6 months ago

Backported to quinteros in commit 743ce2ce751d476b3cdf3374749df21db3069312.

commit 743ce2ce751d476b3cdf3374749df21db3069312
Author: Jason Frey <fryguy9@gmail.com>
Date:   Thu Feb 8 10:35:25 2024 -0500

    Merge pull request #1052 from bdunne/database_password_integer

    Only return passwords that include letters.

    (cherry picked from commit 668e69924bd86df71c135251af091fe87c8d84ca)