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

Jump to bottom

Encrypt passwords before putting them on the queue as args #19006

Merged
merged 1 commit into from
Jul 18, 2019
Merged

Encrypt passwords before putting them on the queue as args #19006

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
12 changes: 12 additions & 0 deletions app/models/manageiq/providers/embedded_ansible/automation_manager/credential.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,18 @@ def self.raw_create_in_provider(manager, params)
create!(create_params)
end

def self.password_attribute_keys
self::API_ATTRIBUTES.map do |k, v|
Copy link
Member

@NickLaMuro NickLaMuro Jul 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Since API_ATTRIBUTES are mapped to different DB columns as part of the .params_to_attributes, maybe include .encrypted_columns from PasswordMixin to this list before mapping?

Copy link
Member Author

@carbonin carbonin Jul 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I did this specifically for the API attributes. For other attributes you could use .encrypted_columns like you said. I can change the name to be more clear. Maybe api_attribute_password_keys?

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 guess what I'm saying is if the API attributes and the encrypted columns were the same then I wouldn't need this method.

Since that's the case then I wouldn't want to confuse the two. I'm not sure what the benefit of having both here would be.

Copy link
Member

@NickLaMuro NickLaMuro Jul 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I was only just suggesting this in the off chance something else besides the UI ends up queuing a credential operation, and isn't using the API_ATTRIBUTE keys to do it. I think the method name is fine as is.

Not a big deal though, so I don't think this is worth stopping a merge to consider this change.

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 don't think that would even work. Pretty sure params_to_attributes would break the cred data (we assume that the input has the API_ATTRIBUTES-style keys)

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I concede...

manageiq/app/models/manageiq/providers/embedded_ansible/automation_manager/vault_credential.rb

Line 29 in 712d9cd

attrs[:password] = attrs.delete(:vault_password)

Though I had done a ||= in some places, but I guess not.

You win this time Carboni! (shakes fist in air while onlookers look on in wonder...)

k if v[:type] == :password
end.compact
end

def self.encrypt_queue_params(params)
encrypted_params = params.slice(*password_attribute_keys)
encrypted_params.transform_values! { |v| ManageIQ::Password.try_encrypt(v) }
Copy link
Member

@NickLaMuro NickLaMuro Jul 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Just putting this as a note to anyone else that sees this and is confused:

There is no decrypt step in the changes because it isn't needed.

ManageIQ::Password.try_encrypt will only encrypt if the field if it hasn't been encrypted already (the check is done via ManageIQ::Password.split), and will just use the existing value if it has already been encrypted. This is more important not for here, but in PasswordMixin also uses .try_encrypt and there isn't any decrypt step in these changes prior to a save/update because it isn't required.

NOTE: My original concern for bringing this up is because I know the specs for the *_queue methods don't actually test the .deliver operations, so I was afraid we might have been "double encrypting". That isn't the case.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for this explanation...I really couldn't understand why there was no decrypt step.

params.merge(encrypted_params)
end

def raw_update_in_provider(params)
update!(self.class.params_to_attributes(params.except(:task_id, :miq_task_id)))
end
Expand Down
12 changes: 10 additions & 2 deletions app/models/manageiq/providers/embedded_ansible/crud_common.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,15 @@ def create_in_provider(manager_id, params)
end
end

def encrypt_queue_params(params)
params
end

def create_in_provider_queue(manager_id, params, auth_user = nil)
manager = parent.find(manager_id)
action = "Creating #{self::FRIENDLY_NAME}"
action << " (name=#{params[:name]})" if params[:name]
queue(manager.my_zone, nil, "create_in_provider", [manager_id, params], action, auth_user)
queue(manager.my_zone, nil, "create_in_provider", [manager_id, encrypt_queue_params(params)], action, auth_user)
end

private
Expand Down Expand Up @@ -84,8 +88,12 @@ def update_in_provider(params)
self
end

def encrypt_queue_params(params)
self.class.encrypt_queue_params(params)
end

def update_in_provider_queue(params, auth_user = nil)
queue("update_in_provider", [params], "Updating", auth_user)
queue("update_in_provider", [encrypt_queue_params(params)], "Updating", auth_user)
end

def raw_delete_in_provider
Expand Down
Loading