Secure auth credential and profile files with ansible vault encryption #59

chambridge · 2017-07-13T16:26:47Z

This is a first step where all i/o to data/credentials and data/profiles are encrypted/decrypted using the ansible vault api.

Some notable changes:

Added ability to pass in vault password via a file (this is for scripting purposes & easing testing)
Data in credentials and profiles file are no longer using CSV Writer. Now data is stored as an encrypted JSON string that allows us to load the file back into an Python list of dictionaries instead of harder to read file parsing.

…ncryption.

coveralls · 2017-07-13T16:30:04Z

Coverage increased (+10.8%) to 62.143% when pulling 14c6493 on issues/47-part1 into f95cbb8 on master.

noahl

I have some comments, but overall I am looking forward to this change. It will be a big improvement to Rho, and will remove some hesitations that customers could have about using Rho.

noahl · 2017-07-13T16:45:48Z

rho/authaddcommand.py

@@ -15,28 +15,50 @@
 """

 from __future__ import print_function
-import csv
+import json


noahl · 2017-07-13T16:47:16Z

rho/authaddcommand.py

+    """
+    auth_found = False
+    for cred in cred_list:
+        if cred.get('name') == auth_name:


Do we ever have credentials with no "name" field? I really don't know, just asking.

No. A name is a required field for an auth credential.
https://github.com/quipucords/rho/blob/master/rho/authaddcommand.py#L78

noahl · 2017-07-13T17:08:07Z

rho/authaddcommand.py

+    cred_list.append(new_cred)
+
+    data = json.dumps(cred_list, separators=(',', ': '))
+    with open(credentials_path, 'wb') as cred_file:


This makes me nervous, because if something goes wrong, I think we lose the old credentials file as well as the new one. How about writing to a new temporary file and then renaming the temp file to credentials_path?

moved writing data into vault file. It now takes an object then coverts to json, the writes to temporary file, the moves to final file location.

noahl · 2017-07-13T17:31:56Z

rho/authclearcommand.py


-            os.remove('data/cred-temp')
+        if self.options.name:


I think that in general there is a lot of duplicated code in these files, specifically in the self.parse.add_option calls and in the "get Vault password, read credentials file, and then rewrite credentials file" patterns. I would like to move that to helper functions/classes. We could either put them in a new file, or combine all of the auth-related commands into a single file and put the helpers there.

I've collapsed the vault init flow to a helper method in the vault.py file. Open to changes in the init for commands around the self.parse.add_option, perhaps we can pursue this as part of #38.

Sounds good to me.

noahl · 2017-07-13T20:40:58Z

rho/vault.py

+
+    def load_as_json(self, secure_file):
+        """read vault secured file and return json decoded object"""
+        return json.loads(self.load(open(secure_file).read()))


Maybe json.loads(self.load_secure_file(secure_file))?

yep. That was my intent, but must have gotten side tracked. Will fix.

noahl · 2017-07-13T20:43:58Z

test/test_clicommand.py

@@ -58,7 +71,7 @@ def test_scan_facts_default(self):
                                           "--reset", "--reportfile",
                                           "data/test_report.csv", "--facts",
                                           "default", "ansible_forks",
-                                           "100"])
+                                           "100", "--vault", "./vault_pass"])


Let's use /tmp for these files. I can imagine that writing to ./vault_pass in some directory we don't control could cause problems for someone later on.

Also, please pull the default file paths for testing out into module-level constants or group the shared code. I know the rest of the code isn't written that way, but I think that it all should be, and at least we can avoid making it worse.

I've moved all the files for testing to /tmp. Hopefully this is what you were going for, if not lets capture further updates here: #33

coveralls · 2017-07-14T14:23:35Z

Coverage increased (+11.2%) to 62.5% when pulling bef60ac on issues/47-part1 into f95cbb8 on master.

coveralls · 2017-07-14T14:23:35Z

Coverage increased (+11.2%) to 62.5% when pulling bef60ac on issues/47-part1 into f95cbb8 on master.

noahl

Looks good! This is a big improvement in Rho. 👍

noahl · 2017-07-14T14:29:53Z

rho/authaddcommand.py

+    """
+    auth_found = False
+    for cred in cred_list:
+        if cred.get('name') == auth_name:


noahl · 2017-07-14T15:03:01Z

rho/authclearcommand.py


-            os.remove('data/cred-temp')
+        if self.options.name:


Sounds good to me.

noahl · 2017-07-14T15:14:41Z

rho/scancommand.py

@@ -66,7 +67,14 @@ def _create_ping_inventory(profile_ranges, profile_port, profile_auth_list,

    string_header = copy(string_to_write)

-    for auth_item in profile_auth_list:
+    for cred_item in profile_auth_list:
+        auth_item = []


Minor suggestion:

auth_item = [ cred_item.get('id'), cred_item.get('name'), ... ]

coveralls · 2017-07-14T15:27:42Z

Coverage increased (+11.5%) to 62.839% when pulling e24704d on issues/47-part1 into f95cbb8 on master.

coveralls · 2017-07-14T15:27:42Z

Coverage increased (+11.5%) to 62.839% when pulling e24704d on issues/47-part1 into f95cbb8 on master.

coveralls · 2017-07-14T15:27:42Z

Coverage increased (+11.5%) to 62.839% when pulling e24704d on issues/47-part1 into f95cbb8 on master.

Secure auth credential and network profile files with ansible vault e…

14c6493

…ncryption.

chambridge assigned noahl Jul 13, 2017

noahl reviewed Jul 13, 2017

View reviewed changes

Updates per review comments.

bef60ac

noahl approved these changes Jul 14, 2017

View reviewed changes

auth_item construction update.

e24704d

chambridge merged commit ce25427 into master Jul 14, 2017

chambridge deleted the issues/47-part1 branch July 14, 2017 15:31

chambridge mentioned this pull request Jul 18, 2017

Implement authoriation credentials utilizing the Ansible secure vault #47

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Secure auth credential and profile files with ansible vault encryption #59

Secure auth credential and profile files with ansible vault encryption #59

chambridge commented Jul 13, 2017

coveralls commented Jul 13, 2017 •

edited

noahl left a comment

noahl Jul 13, 2017

noahl Jul 13, 2017

chambridge Jul 14, 2017

noahl Jul 14, 2017

noahl Jul 13, 2017

chambridge Jul 14, 2017

noahl Jul 13, 2017

chambridge Jul 14, 2017

noahl Jul 14, 2017

noahl Jul 13, 2017

chambridge Jul 14, 2017

noahl Jul 13, 2017

chambridge Jul 14, 2017

coveralls commented Jul 14, 2017 •

edited

coveralls commented Jul 14, 2017

noahl left a comment

noahl Jul 14, 2017

noahl Jul 14, 2017

noahl Jul 14, 2017

coveralls commented Jul 14, 2017

coveralls commented Jul 14, 2017

coveralls commented Jul 14, 2017

Secure auth credential and profile files with ansible vault encryption #59

Secure auth credential and profile files with ansible vault encryption #59

Conversation

chambridge commented Jul 13, 2017

coveralls commented Jul 13, 2017 • edited

noahl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 14, 2017 • edited

coveralls commented Jul 14, 2017

noahl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 14, 2017

coveralls commented Jul 14, 2017

coveralls commented Jul 14, 2017

coveralls commented Jul 13, 2017 •

edited

coveralls commented Jul 14, 2017 •

edited