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.

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

Add a migration to encrypt database password using scram-sha-256 #757

Merged
merged 1 commit into from
Oct 23, 2024

Conversation

@Fryguy
Copy link
Member

Fryguy commented Oct 17, 2024

Note that if anyone has their database locally setup as md5 (or just on a old version), this will break their local database. I'm ok with that, but we might need some instructions either here or in guides/dev-setup on the changes needed to unbreak them.

class ReencryptPasswordScramsha < ActiveRecord::Migration[6.1]
def up
say_with_time('Reencrypting database user password with scram-sha-256') do
database_yml = YAML.safe_load(Rails.root.join("config", "database.yml"))
Copy link
Member

@Fryguy Fryguy Oct 17, 2024

Choose a reason for hiding this comment

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

There are cases where we could use DATABASE_URL, so I think it's better to use the database configuration instead. Additionally, database confiuguration should have already decrypted the password for us, so there's no need to further try_decrypt:

ActiveRecord::Base.connection_db_config.configuration_hash
# => {:adapter=>"postgresql", :encoding=>"utf8", :username=>"root", :pool=>"5", :wait_timeout=>5, :min_messages=>"notice", :database=>"vmdb_development", :password=>"smartvm", :port=>5432, :host=>"localhost"}

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@bdunne bdunne force-pushed the scramsha branch 4 times, most recently from f496a83 to 288c276 Compare October 22, 2024 17:09
@Fryguy Fryguy merged commit 3762bf5 into ManageIQ:master Oct 23, 2024
4 of 5 checks passed
@kbrock
Copy link
Member

kbrock commented Oct 23, 2024

I think the only concern here is the plaintext password will be printed into the pg logs in some cases. This is from what I've read and I have not researched/looked too deep into this

@Fryguy
Copy link
Member

Fryguy commented Oct 24, 2024

Backported to radjabov in commit 37e87fa.

commit 37e87fa519bb9df9f8fe41d9c005b9b1790b4a32
Author: Jason Frey <fryguy9@gmail.com>
Date:   Wed Oct 23 16:29:10 2024 -0400

    Merge pull request #757 from bdunne/scramsha
    
    Add a migration to encrypt database password using scram-sha-256
    
    (cherry picked from commit 3762bf55c1f5037c7e5421ec426ecbb9dad12fdb)

Fryguy added a commit that referenced this pull request Oct 24, 2024
Add a migration to encrypt database password using scram-sha-256

(cherry picked from commit 3762bf5)
@bdunne bdunne deleted the scramsha branch October 24, 2024 19:33
@Fryguy
Copy link
Member

Fryguy commented Oct 25, 2024

@kbrock the way it's done here encrypts the password first (in memory) then sends that, so it should not be in the logs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants