-
Notifications
You must be signed in to change notification settings - Fork 900
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
Encrypt/Decrypt passwords in Automate Workspace #16202
Conversation
:tenant => user.current_tenant, | ||
:input => input) | ||
end | ||
let(:password) { "secret" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those some bonus numbers and special characters in there... popular password is ca$hcOw
;)
let(:encrypted) { MiqAePassword.encrypt(password) } | ||
let(:input) do | ||
{ 'objects' => {'root' => { 'var1' => '1', 'var2' => "password::#{encrypted}"}}, | ||
'method_parameters' => {'arg1' => "password::#{encrypted}"} } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't mix and match "
and '
, just use "
for everything
app/models/automate_workspace.rb
Outdated
def encrypt(object_name, attribute, value) | ||
self.output ||= {} | ||
self.output.store_path('objects', object_name, attribute, "password::#{MiqAePassword.encrypt(value)}") | ||
save |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this shouldn't do the saving, but I'm not sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, what is the expected return value here? Should this be doing save! instead, and if so, would it be better to just use merge_output!
Checked commits mkanoor/manageiq@6b65848~...e9c556e with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0 |
value = fetch_value(object_name, attribute) | ||
raise ArgumentError, "#{object_name} : Attribute #{attribute} not found" unless value | ||
raise ArgumentError, "#{object_name} : Attribute #{attribute} invalid type" unless value.kind_of?(String) | ||
match_data = /password::(.*)/.match(value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should have a \A
at the start of the regex.
@@ -18,4 +18,33 @@ def merge_output!(hash) | |||
save! | |||
self | |||
end | |||
|
|||
def decrypt(object_name, attribute) | |||
MiqPassword.decrypt(encrypted_value(object_name, attribute)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to be careful this doesn't expose us to an Oracle attack. For example, a user can potentially manually create an entry with the string "password:v2{23423h42h}". If we aren't careful with the error message, they can use that information to determine "good" v2 keys from "bad" v2 keys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Fryguy
The caller never provides the encrypted value. Instead inside of an Automate Workspace identified with a GUID, they pass in the object name and attribute name.
When the call comes into the server we locate the Automate Workspace and use the object nam e and attribute name to get the encrypted value.
For this to be an oracle attack they would have to guess the GUID of the Automate Workspace, then look at objects and the attribute name and be able to tweak the encrypted value and then call this API to get the decrypted value.
A customer can create encrypted attributes in Automate Model to store passwords. These attributes get stored in the AutomateWorkspace to be accessible via REST API.
This PR adds 2 methods
This is needed for 4.6 to allow Ansible Playbooks to access customer defined password attributes from their Automate Models. E.g. An ansible playbook connecting to IPAM system might need a password stored in Automate Model.