ONYX-26897 To reuse the token #185

Pooja-Gangwar · 2023-01-06T11:11:30Z

Desired Outcome

The ansible-lookup-plugin does not support retries

Implemented Changes

Code fix is around supporting re-tries with authentication.
Create a mock test which can return 401 on first call and returns a token on subsequent call to prove re-try logic.

Meeting notes from 11/03/2022:

   Attendeess: [Jason Vanderhoof](https://ca-il-jira.il.cyber-ark.com:8443/secure/ViewProfile.jspa?name=jvanderhoof) , 
   [Andy Tinkham](https://ca-il-jira.il.cyber-ark.com:8443/secure/ViewProfile.jspa?name=atinkham) and 
    brugu.maharishi@cyberark.com

   Code fix is around supporting re-tries with authentication.
   Currently a single token is used to fetch one variable and is never re-used. In order to improve performance - ensure the 
   token can be used for it's life. Need to come up with better design to ensure token is stored securely.
   Need a security review on this project once code changes are done as we are modifying the way we authenticate and 
    storing tokens.
   It would be tough to re-create client environment, hence [Jason Vanderhoof](https://ca-il-jira.il.cyber- 
   ark.com:8443/secure/ViewProfile.jspa?name=jvanderhoof)  suggested to create a mock test which can return 401 on 
   first call and returns a token on subsequent call to prove re-try logic.

Connected Issue/Story

Jira ticket - ONYX-26897
Bug - https://cyberark.lightning.force.com/lightning/r/Case/5002J00001Y4ffFQAR/view

Definition of Done

At least 1 todo must be completed in the sections below for the PR to be
merged.

Changelog

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
A follow-up issue to update official docs has been filed here: [insert issue ID]
This PR does not require updating any documentation

Behavior

This PR changes product behavior and has been reviewed by a PO, or
These changes are part of a larger initiative that will be reviewed later, or
No behavior was changed with this PR

Security

Security architect has reviewed the changes in this PR,
These changes are part of a larger initiative with a separate security review, or
There are no security aspects to these changes

john-odonnell

A few questions:

In the PR description, I notice a link to a Salesforce case, but I can't tell the connection between the case and this PR. Any more information here?
We definitely want a security review with @andytinkham before merging this to make sure we're correctly handling token files.
Caching the token will only improve performance on requests executed within the token's 8 minute lifetime. Could we implement a batch secret retrieval instead, without caching the token?
You've added some more test cases, but none of them actually confirm the changed behavior - we need at least one more test that confirms a retrieved Conjur token is cached in a particular file.

john-odonnell · 2023-01-18T17:12:17Z

plugins/lookup/conjur_variable.py

+    if response.getcode() == 200:
+        display.vvvv('Conjur token was successfully retrieved and authorized with {0} code and {1} username '.format(code, username))
+        return response.read()
+    if response.getcode() == 401:
+        raise AnsibleError('Conjur request has invalid authorization credentials as {0} and {1} response'.format(code, username))
+    if response.getcode() == 403:
+        raise AnsibleError('The controlling host\'s Conjur identity does not have authorization as \'{0}\' (got {1} response)'
                           .format(username, code))
+    if response.getcode() == 404:
+        raise AnsibleError('The token does not exist with {0} response '.format(code))
+    if response.getcode() == 500:
+        raise AnsibleError('Internal Server Error with {0} response'.format(code))


For the purposes of determining whether an authentication request has failed or not, I don't know if we really need to capture all these conditions, or if we need to expose them to the Ansible client.

Suggested change

if response.getcode() == 200:

display.vvvv('Conjur token was successfully retrieved and authorized with {0} code and {1} username '.format(code, username))

return response.read()

if response.getcode() == 401:

raise AnsibleError('Conjur request has invalid authorization credentials as {0} and {1} response'.format(code, username))

if response.getcode() == 403:

raise AnsibleError('The controlling host\'s Conjur identity does not have authorization as \'{0}\' (got {1} response)'

.format(username, code))

if response.getcode() == 404:

raise AnsibleError('The token does not exist with {0} response '.format(code))

if response.getcode() == 500:

raise AnsibleError('Internal Server Error with {0} response'.format(code))

if code == 200:

display.vvvv('Conjur user {0} successfully authenticated'.format(username))

return response.read()

elif code == 500:

raise AnsibleError('Internal Server Error: {0}'.format(response.read()))

else:

raise AnsibleError('Authentication failed with status code {0}: {1}.'.format(code, response.read())

Shall I remove all except 200, 500 and 401 code . Please suggest .

john-odonnell · 2023-01-18T17:13:10Z

plugin_token.txt

@@ -0,0 +1 @@
+token


If this file location is used for token storage, it should be added to .gitignore

yes , it is there.

now in new changes , I am not using any new file .

john-odonnell · 2023-01-18T17:19:30Z

plugins/lookup/conjur_variable.py

+        path = '../../tests/conjur_variable/plugin_token.txt'
+        isExist = os.path.exists(path)


We should definitely include a default token file path, but this path is relative to some unknown sub-directory of the project's root, and points to the test directory - probably don't want to hard-code this.

Sure , I will update it .

john-odonnell · 2023-01-18T17:36:34Z

plugins/lookup/conjur_variable.py

+        path = '../../tests/conjur_variable/plugin_token.txt'
+        isExist = os.path.exists(path)
+
+        isEmpty = 0
+        if ((isExist is True)):
+            isEmpty = os.path.getsize(path)
+
        token = None
        if 'authn_token_file' not in conf:
-            token = _fetch_conjur_token(
-                conf['appliance_url'],
-                conf['account'],
-                identity['id'],
-                identity['api_key'],
-                validate_certs,
-                cert_file
-            )
+            if ((isExist is False) or (isEmpty == 0)):
+                token = _fetch_conjur_token(
+                    conf['appliance_url'],
+                    conf['account'],
+                    identity['id'],
+                    identity['api_key'],
+                    validate_certs,
+                    cert_file
+                )
+                with open("plugin_token.txt", "wb") as binary_file:
+                    binary_file.write(token)
+            else:
+                with open("plugin_token.txt", "rb") as f:
+                    token = f.read()


I think that we can use the existing authn_token_file configuration to clean this up a bit. Traditionally, it's used to point the service to an existing file that already contains a Conjur token, but maybe we could repurpose it to indicate that either:

The file already exists and has a Conjur token, or

The file does not exist, so we want to authenticate with Conjur and store our token there

Given that, we could probably re-arrange this bit:

if 'authn_token_file' not in conf: token = _fetch_conjur_token(...) else: if os.path.exists(conf['authn_token_file']): with open(conf['authn_token_file'], 'rb') as f: token = f.read() else: token = _fetch_conjur_token(...) with open(conf['authn_token_file'], "wb") as f: f.write(token)

Made changes to use the existing file "access_token" .

john-odonnell · 2023-01-18T20:08:21Z

plugins/lookup/conjur_variable.py

@@ -102,6 +102,7 @@
 from ansible.module_utils.urls import open_url
 from ansible.utils.display import Display
 import ssl
+from pathlib import Path


This import is not used in these changes

Pooja-Gangwar · 2023-01-23T13:45:04Z

A few questions:

In the PR description, I notice a link to a Salesforce case, but I can't tell the connection between the case and this PR. Any more information here? Ans - Sorry I missed to mention here , I worked on 11/03/2022 Meeting's comments , now these comments are available in PR and in Jira ticket comments section these comments were already available .

We definitely want a security review with @andytinkham before merging this to make sure we're correctly handling token files. Ans - Sure, John

Caching the token will only improve performance on requests executed within the token's 8 minute lifetime. Could we implement a batch secret retrieval instead, without caching the token? Ans- Please let's talk on it someday.

You've added some more test cases, but none of them actually confirm the changed behavior - we need at least one more test that confirms a retrieved Conjur token is cached in a particular file. Ans -Sure

testing

c4a5d1b

Pooja-Gangwar requested a review from a team as a code owner January 6, 2023 11:11

Pooja Gangwar added 2 commits January 7, 2023 13:36

testing

e3474b2

testing

f6401b4

john-odonnell reviewed Jan 18, 2023

View reviewed changes

Pooja Gangwar added 3 commits January 24, 2023 15:28

testing

4822119

testing

5e0fcf2

testing

5235939

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ONYX-26897 To reuse the token #185

ONYX-26897 To reuse the token #185

Pooja-Gangwar commented Jan 6, 2023 •

edited

Loading

john-odonnell left a comment

john-odonnell Jan 18, 2023

Pooja-Gangwar Jan 24, 2023

john-odonnell Jan 18, 2023

Pooja-Gangwar Jan 23, 2023

Pooja-Gangwar Jan 24, 2023

john-odonnell Jan 18, 2023 •

edited by Pooja-Gangwar

Loading

Pooja-Gangwar Jan 23, 2023

Pooja-Gangwar Jan 24, 2023

john-odonnell Jan 18, 2023

Pooja-Gangwar Jan 24, 2023

john-odonnell Jan 18, 2023

Pooja-Gangwar Jan 24, 2023

Pooja-Gangwar commented Jan 23, 2023 •

edited

Loading

		path = '../../tests/conjur_variable/plugin_token.txt'
		isExist = os.path.exists(path)

ONYX-26897 To reuse the token #185

Are you sure you want to change the base?

ONYX-26897 To reuse the token #185

Conversation

Pooja-Gangwar commented Jan 6, 2023 • edited Loading

Desired Outcome

Implemented Changes

Connected Issue/Story

Definition of Done

Changelog

Test coverage

Documentation

Behavior

Security

john-odonnell 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

john-odonnell Jan 18, 2023 • edited by Pooja-Gangwar Loading

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

Pooja-Gangwar commented Jan 23, 2023 • edited Loading

Pooja-Gangwar commented Jan 6, 2023 •

edited

Loading

john-odonnell Jan 18, 2023 •

edited by Pooja-Gangwar

Loading

Pooja-Gangwar commented Jan 23, 2023 •

edited

Loading