Encrypt passwords before putting them on the queue as args #19006
Conversation
Before this change we were logging these passwords as plaintext
|
Checked commit carbonin@5656323 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0 |
| @@ -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| | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Okay, I concede...
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...)
|
|
||
| def self.encrypt_queue_params(params) | ||
| encrypted_params = params.slice(*password_attribute_keys) | ||
| encrypted_params.transform_values! { |v| ManageIQ::Password.try_encrypt(v) } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks for this explanation...I really couldn't understand why there was no decrypt step.
|
@carbonin Probably worth noting that this is at least also being down in |
Before this change we were logging these passwords as plaintext