Add lookup plugin option (file_mode) to store secret in a file #51

jcosteatcyberark · 2020-11-13T16:32:43Z

What does this PR do?

This PR offers a new option: ~~file_mode~~ as_file (boolean) which adds the possibility to store a secret as a file in ~~/dev/shm~~ a temporary file instead of returning it directly. The lookup function instead returns the path of the file.

This feature addresses an issue with ansible_ssh_private_key_file which doesn't accept inline ssh keys.
By adding this option, it is now possible to use the below lookup with ansible_ssh_private_key_file:

ansible_ssh_private_key_file: "{{ lookup('cyberark.conjur.conjur_variable', 'path/to/secret-id', as_file=True) }}"

Without the need to handle temporary file creation in playbooks, it extends the current support of the below:

ansible_password: "{{ lookup('cyberark.conjur.conjur_variable', 'path/to/secret-id') }}"

What ticket does this PR close?

This PR addresses Issue #52 .
This PR also addresses the post in https://discuss.cyberarkcommons.org/t/conjur-ansible-lookup-plugin-and-ssh-key-file/1070

Checklists

Change log

The CHANGELOG has been updated, or
This PR does not include user-facing changes and doesn't require a CHANGELOG update

Test coverage

This PR includes new unit and integration tests to go with the code changes, or
The changes in this PR do not require tests

Documentation

Docs (e.g. READMEs) were updated in this PR, and/or there is a follow-on issue to update docs, or
This PR does not require updating any documentation

This commit adds the file_mode option (boolean) which makes the lookup plugin store the secret in /dev/shm: a file with a random name (0600 permissions) is created. The lookup command returns the file path.

sgnn7

@jcosteatcyberark Left some comments on this PR. Needs a bit of work but you're on the right track I think.

sgnn7 · 2020-11-13T19:08:16Z

plugins/lookup/conjur_variable.py

@@ -31,6 +31,10 @@
        description: Flag to control SSL certificate validation
        type: boolean
        default: True
+      file_mode:
+        description: Store lookup result in a temp file


This should include another sentence regarding what this is useful for.

I agree, done in next commit.

sgnn7 · 2020-11-13T19:08:40Z

plugins/lookup/conjur_variable.py

@@ -86,6 +90,9 @@
 from base64 import b64encode
 from netrc import netrc
 from os import environ
+import stat
+from random import choice
+import string


Imports need to be sorted alphabetically

I sorted the one I added, others don't seem to be though.
I'll keep it out of this PR.

sgnn7 · 2020-11-13T19:10:19Z

plugins/lookup/conjur_variable.py

@@ -86,6 +90,9 @@
 from base64 import b64encode
 from netrc import netrc
 from os import environ
+import stat
+from random import choice
+import string


For stat and string, I think you can import just the constants/functions you are using instead of the whole package

Thanks, done in next commit

sgnn7 · 2020-11-13T19:12:32Z

CHANGELOG.md

@@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - The [Conjur Ansible role](https://galaxy.ansible.com/cyberark/conjur-host-identity) has been
  migrated to this collection, where it will be maintained moving forward.
  [cyberark/ansible-conjur-host-identity#30](https://github.com/cyberark/ansible-conjur-host-identity/issues/30)
+- Add `file_mode` boolean option to store the secret as a temporary file in /dev/shm/ and return its path.


This should also include a link to the issue it is trying to resolve.

I think we also need to add usage of this in the README too

This is a UX issue but file_mode isn't quite as descriptive as it should be to explain this feature. Any suggestions for a better one maybe?

Added a link to the Cyberark Commons post. Regarding the usage, I'm not sure where to put it in README.
I renamed file_mode to as_file

sgnn7 · 2020-11-13T19:14:55Z

plugins/lookup/conjur_variable.py

+
+        if file_mode:
+            return _store_in_file(conjur_variable)
+        else:


else isn't needed at this point since there's only a single logic path that remain after file_mode return

Thanks, removed.

sgnn7 · 2020-11-13T19:21:22Z

plugins/lookup/conjur_variable.py

@@ -203,12 +210,24 @@ def _fetch_conjur_variable(conjur_variable, token, conjur_url, account, validate
    return {}


+def _store_in_file(value):
+    file_name = ''.join(choice(string.ascii_uppercase + string.digits) for i in range(12))
+    file_path = os.path.join("/dev/shm", file_name)


/dev/shm is not guaranteed to exist on the target system

I created a function to return the best possible path, inspired by Summon equivalent

sgnn7 · 2020-11-13T19:23:36Z

plugins/lookup/conjur_variable.py

@@ -203,12 +210,24 @@ def _fetch_conjur_variable(conjur_variable, token, conjur_url, account, validate
    return {}


+def _store_in_file(value):
+    file_name = ''.join(choice(string.ascii_uppercase + string.digits) for i in range(12))


You probably should use something like tempfile.NamedTemporaryFile and set prefix/suffix/dir instead of manually assembling the name.

Thanks, I didn't know this lib. I made the change in next commit

sgnn7 · 2020-11-13T19:24:44Z

plugins/lookup/conjur_variable.py

+def _store_in_file(value):
+    file_name = ''.join(choice(string.ascii_uppercase + string.digits) for i in range(12))
+    file_path = os.path.join("/dev/shm", file_name)
+    with open(file_path, 'w+') as file:


+ in the mode isn't needed since we won't be reading the file

file -> secrets_file

sgnn7 · 2020-11-13T19:28:42Z

plugins/lookup/conjur_variable.py

+    file_path = os.path.join("/dev/shm", file_name)
+    with open(file_path, 'w+') as file:
+        file.write(value[0])
+        os.chmod(file_path, stat.S_IRUSR | stat.S_IWUSR)


We should chmod the file before we write the secret into it

Fixed in next commit

sgnn7 · 2020-11-13T19:29:25Z

plugins/lookup/conjur_variable.py

+        file.write(value[0])
+        os.chmod(file_path, stat.S_IRUSR | stat.S_IWUSR)
+
+        return [file_path]


This return should happen as part of the function context, not the with context so it should be indented 1 lower level

This piece of code is removed in next commit thanks to tempfile.NamedTemporaryFile.

sgnn7 · 2020-11-18T16:57:05Z

@jcosteatcyberark Is this ready for another review cycle? I haven't seen any messages that indicate that it is.

jcosteatcyberark · 2020-11-30T14:34:46Z

Hi @sgnn7, sorry for the delay. It's updated and ready for a review!

doodlesbykumbi · 2020-12-04T22:09:23Z

Hi @jcosteatcyberark. Just wanted to let you know that I've tagged the team (@cyberark/community-and-integrations-team) and we should be able to get someone to look at this soon.

diverdane

@jcosteatcyberark , thank you for proposing this change! This looks very good to me. The only suggestion that I have is that it might be helpful to add an example of this lookup in the README (in addition to the plain variable lookup that's already there).
Also a question on the syntax for a call to check_output(), just for my own understanding.

diverdane · 2020-12-09T17:25:23Z

tests/conjur_variable/test_cases/retrieve-variable-into-file/tests/test_default.py

+    assert secret_file.exists
+    assert secret_file.mode == 0o600
+
+    secret = host.check_output("cat {0}".format(secret_path), shell=True)


@jcosteatcyberark, just for my own understanding... Can you explain what this line is doing? I know it's doing a level of indirection (taking secret_path and cating the file that it points to), but I'd like to understand better.

Hi @diverdane, the test playbook stores the lookup output in /conjur_secret_path.txt. This file contains the path to the file that contains the secret (secret_path).
To resume this test asserts that cat (cat /conjur_secret_path) = secret

since it wasn't clear to Dane from the code, is it worth writing this in a more-verbose-but-more-clear way or at least adding a code comment?

Hi @izgeri I cleaned up the code and commented. It should be easier to understand now.

jcosteatcyberark · 2020-12-15T17:53:51Z

Hi @diverdane I added an example to the README and rewrote + commented the test for better readability.
It's ready for review!

diverdane

LGTM! Thanks for adding this feature!

jcosteatcyberark requested a review from a team as a code owner November 13, 2020 16:32

jcosteatcyberark added 3 commits November 13, 2020 17:47

Add lookup option to store the secret in file and returns its path.

9cd442f

This commit adds the file_mode option (boolean) which makes the lookup plugin store the secret in /dev/shm: a file with a random name (0600 permissions) is created. The lookup command returns the file path.

Add new file_mode option in CHANGELOG.md.

ad5862b

Add unit test for newly added file_mode option.

82bc6ca

jcosteatcyberark force-pushed the feature/lookup-secret-in-file branch from 18a6b92 to 82bc6ca Compare November 13, 2020 16:47

sgnn7 suggested changes Nov 13, 2020

View reviewed changes

jcosteatcyberark force-pushed the feature/lookup-secret-in-file branch from 7bd9026 to b8c2c28 Compare November 17, 2020 15:35

Various fixes from PR feedback.

501df6a

jcosteatcyberark force-pushed the feature/lookup-secret-in-file branch from b8c2c28 to 501df6a Compare November 17, 2020 15:39

doodlesbykumbi requested a review from a team December 4, 2020 22:08

diverdane reviewed Dec 9, 2020

View reviewed changes

jcosteatcyberark added 2 commits December 15, 2020 17:30

Improve retrieve-into-file test code readability.

7402d1f

Add an example for the as_file lookup parameter in README.md.

7271d12

diverdane approved these changes Dec 16, 2020

View reviewed changes

diverdane merged commit 5421f66 into cyberark:master Dec 16, 2020

izgeri added the contributor label Jan 4, 2021

izgeri added internal-contributor and removed contributor labels Jan 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add lookup plugin option (file_mode) to store secret in a file #51

Add lookup plugin option (file_mode) to store secret in a file #51

jcosteatcyberark commented Nov 13, 2020 •

edited by diverdane

sgnn7 left a comment

sgnn7 Nov 13, 2020

jcosteatcyberark Nov 17, 2020

sgnn7 Nov 13, 2020

jcosteatcyberark Nov 17, 2020

sgnn7 Nov 13, 2020

jcosteatcyberark Nov 17, 2020

sgnn7 Nov 13, 2020

sgnn7 Nov 13, 2020

sgnn7 Nov 13, 2020

jcosteatcyberark Nov 17, 2020

sgnn7 Nov 13, 2020

jcosteatcyberark Nov 17, 2020

sgnn7 Nov 13, 2020

jcosteatcyberark Nov 17, 2020

sgnn7 Nov 13, 2020

jcosteatcyberark Nov 17, 2020

sgnn7 Nov 13, 2020

sgnn7 Nov 13, 2020

jcosteatcyberark Nov 17, 2020

sgnn7 Nov 13, 2020

jcosteatcyberark Nov 17, 2020

sgnn7 Nov 13, 2020

jcosteatcyberark Nov 17, 2020 •

edited

sgnn7 commented Nov 18, 2020

jcosteatcyberark commented Nov 30, 2020

doodlesbykumbi commented Dec 4, 2020

diverdane left a comment

diverdane Dec 9, 2020

jcosteatcyberark Dec 9, 2020

izgeri Dec 9, 2020

jcosteatcyberark Dec 15, 2020

jcosteatcyberark commented Dec 15, 2020 •

edited

diverdane left a comment

Add lookup plugin option (file_mode) to store secret in a file #51

Add lookup plugin option (file_mode) to store secret in a file #51

Conversation

jcosteatcyberark commented Nov 13, 2020 • edited by diverdane

What does this PR do?

What ticket does this PR close?

Checklists

Change log

Test coverage

Documentation

sgnn7 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

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

jcosteatcyberark Nov 17, 2020 • edited

Choose a reason for hiding this comment

sgnn7 commented Nov 18, 2020

jcosteatcyberark commented Nov 30, 2020

doodlesbykumbi commented Dec 4, 2020

diverdane 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

jcosteatcyberark commented Dec 15, 2020 • edited

diverdane left a comment

Choose a reason for hiding this comment

jcosteatcyberark commented Nov 13, 2020 •

edited by diverdane

jcosteatcyberark Nov 17, 2020 •

edited

jcosteatcyberark commented Dec 15, 2020 •

edited