Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ONYX-26897 To reuse the token #185

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

ONYX-26897 To reuse the token #185

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c4a5d1b
testing
Jan 6, 2023
e3474b2
testing
Jan 7, 2023
f6401b4
testing
Jan 7, 2023
4822119
testing
Jan 24, 2023
5e0fcf2
testing
Jan 24, 2023
5235939
testing
Jan 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ansible-test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ jobs:
- stable-2.10
- stable-2.11
- stable-2.12
- devel
# - devel
python:
- 3.9
runs-on: ubuntu-latest
Expand Down
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ tests/*/reports/
tests/*/conjur.pem
tests/*/conjur-enterprise.pem
tests/*/access_token
tests/*/plugin_token.txt
**/test-files/output
**/conjur-intro/
bot
Expand Down
1 change: 1 addition & 0 deletions plugin_token.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
token
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes , it is there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

43 changes: 33 additions & 10 deletions plugins/lookup/conjur_variable.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@
from ansible.module_utils.urls import open_url
from ansible.utils.display import Display
import ssl
from pathlib import Path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This import is not used in these changes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


display = Display()

Expand Down Expand Up @@ -176,9 +177,18 @@ def _fetch_conjur_token(conjur_url, account, username, api_key, validate_certs,
validate_certs=validate_certs,
ca_path=cert_file)
code = response.getcode()
if code != 200:
raise AnsibleError('Failed to authenticate as \'{0}\' (got {1} response)'
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))
Comment on lines +179 to +190
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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())

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


return response.read()

Expand Down Expand Up @@ -335,16 +345,29 @@ def run(self, terms, variables=None, **kwargs):
display.vvv("Using cert file path {0}".format(conf['cert_file']))
cert_file = conf['cert_file']

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure , I will update it .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. The file already exists and has a Conjur token, or
  2. 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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made changes to use the existing file "access_token" .

else:
if not os.path.exists(conf['authn_token_file']):
raise AnsibleError('Conjur authn token file `{0}` was not found on the host'
Expand Down
13 changes: 9 additions & 4 deletions tests/conjur_variable/test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,9 @@ function cleanup {
docker-compose down -v
rm -f conjur.pem \
access_token

}
trap cleanup EXIT
# trap cleanup EXIT

while getopts 'e' flag; do
case "${flag}" in
Expand All @@ -47,8 +48,6 @@ while getopts 'e' flag; do
esac
done

cleanup

function wait_for_conjur {
echo "Waiting for Conjur server to come up"
docker-compose exec -T conjur conjurctl wait -r 30 -p 3000
Expand Down Expand Up @@ -209,7 +208,13 @@ function main() {
export CONJUR_ACCOUNT="cucumber"
COMPOSE_PROJECT_NAME="${ANSIBLE_PROJECT}"

setup_conjur_open_source
file=plugin_token.txt
if [ ! -f "$file" ];
then
cleanup
setup_conjur_open_source
fi

fi

COMPOSE_PROJECT_NAME="${ANSIBLE_PROJECT}"
Expand Down
32 changes: 30 additions & 2 deletions tests/unit/plugins/lookup/test_conjur_variable.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,34 @@ def test_fetch_conjur_token(self, mock_open_url):
ca_path="cert_file")
self.assertEquals("response body", result)

@patch('ansible_collections.cyberark.conjur.plugins.lookup.conjur_variable.open_url')
def test_fetch_conjur_token_401(self, mock_open_url):
mock_response = MagicMock()
mock_response.getcode.return_value == 401
mock_response.read.return_value = "Conjur request has invalid authorization credentials"
mock_open_url.return_value = mock_response
result = _fetch_conjur_token("url", "account", "username1", "api_key", True, "cert_file")
mock_open_url.assert_called_with("url/authn/account/username1/authenticate",
data="api_key",
method="POST",
validate_certs=True,
ca_path="cert_file")
self.assertEquals("Conjur request has invalid authorization credentials", result)

@patch('ansible_collections.cyberark.conjur.plugins.lookup.conjur_variable.open_url')
def test_fetch_conjur_token_500(self, mock_open_url):
mock_response = MagicMock()
mock_response.getcode.return_value == 500
mock_response.read.return_value = "Internal Server Error"
mock_open_url.return_value = mock_response
result = _fetch_conjur_token("url", "account", "username1", "api_key", True, "cert_file")
mock_open_url.assert_called_with("url/authn/account/username1/authenticate",
data="api_key",
method="POST",
validate_certs=True,
ca_path="cert_file")
self.assertEquals("Internal Server Error", result)

@patch('ansible_collections.cyberark.conjur.plugins.lookup.conjur_variable._repeat_open_url')
def test_fetch_conjur_variable(self, mock_repeat_open_url):
mock_response = MagicMock()
Expand All @@ -69,7 +97,7 @@ def test_fetch_conjur_variable(self, mock_repeat_open_url):
@patch('ansible_collections.cyberark.conjur.plugins.lookup.conjur_variable._fetch_conjur_token')
@patch('ansible_collections.cyberark.conjur.plugins.lookup.conjur_variable._merge_dictionaries')
def test_run(self, mock_merge_dictionaries, mock_fetch_conjur_token, mock_fetch_conjur_variable):
mock_fetch_conjur_token.return_value = "token"
mock_fetch_conjur_token.return_value = b'token'
mock_fetch_conjur_variable.return_value = ["conjur_variable"]
mock_merge_dictionaries.side_effect = [
{'account': 'fakeaccount', 'appliance_url': 'https://conjur-fake', 'cert_file': './conjurfake.pem'},
Expand All @@ -86,7 +114,7 @@ def test_run(self, mock_merge_dictionaries, mock_fetch_conjur_token, mock_fetch_
@patch('ansible_collections.cyberark.conjur.plugins.lookup.conjur_variable._fetch_conjur_token')
@patch('ansible_collections.cyberark.conjur.plugins.lookup.conjur_variable._merge_dictionaries')
def test_retrieve_to_file(self, mock_merge_dictionaries, mock_fetch_conjur_token, mock_fetch_conjur_variable):
mock_fetch_conjur_token.return_value = "token"
mock_fetch_conjur_token.return_value = b'token'
mock_fetch_conjur_variable.return_value = ["conjur_variable"]
mock_merge_dictionaries.side_effect = [
{'account': 'fakeaccount', 'appliance_url': 'https://conjur-fake', 'cert_file': './conjurfake.pem'},
Expand Down