Skip to content
This repository has been archived by the owner on Sep 9, 2024. It is now read-only.

Commit

Permalink
Use new AuthenticationStrategyInterface
Browse files Browse the repository at this point in the history
  • Loading branch information
szh committed Jun 1, 2022
1 parent 2507e87 commit 82d35f4
Show file tree
Hide file tree
Showing 13 changed files with 84 additions and 44 deletions.
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ coverage.xml
.conjurrc
*.key
*.crt
cleanup.log

# Build artifacts
build/
Expand Down
5 changes: 3 additions & 2 deletions conjur/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,12 @@

# SDK
from conjur_api.client import Client
from conjur_api.providers import AuthnAuthenticationStrategy
from conjur_api.errors.errors import HttpError, HttpStatusError
from conjur_api.models.policy.policy_data import PolicyData

# Internals
from conjur.argument_parser.argparse_builder import ArgParseBuilder
from conjur.data_object.policy_data import PolicyData
from conjur.logic.credential_provider.credential_store_factory import CredentialStoreFactory
from conjur.errors import CertificateVerificationException
from conjur.errors_messages import INCONSISTENT_VERIFY_MODE_MESSAGE
Expand Down Expand Up @@ -137,8 +138,8 @@ def _run_command_flow(self, args, resource):
ssl_verification_meta_data = get_ssl_verification_meta_data_from_conjurrc(args.ssl_verify)
client = Client(ssl_verification_mode=ssl_verification_meta_data.mode,
connection_info=ConjurrcData.load_from_file().get_client_connection_info(),
authn_strategy=AuthnAuthenticationStrategy(self.credential_provider),
debug=args.debug,
credentials_provider=self.credential_provider,
async_mode=False)

if resource == 'list':
Expand Down
3 changes: 2 additions & 1 deletion conjur/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ class Config():
('conjur_account', 'account', True),
('conjur_url', 'url', True),
('cert_file', 'ca_bundle', True),
('authn_type', 'authn_type', False),
]

_config = {}
Expand All @@ -54,7 +55,7 @@ def __init__(self, config_file=DEFAULT_CONFIG_FILE):
for config_field_name, attribute_name, mandatory in self.FIELDS:
if mandatory and config_field_name not in config:
raise InvalidConfigurationException
setattr(self, attribute_name, config[config_field_name])
setattr(self, attribute_name, config.get(config_field_name))
self._config[attribute_name] = getattr(self, attribute_name)

def __repr__(self) -> Union[str,bytes]:
Expand Down
28 changes: 24 additions & 4 deletions conjur/data_object/conjurrc_data.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
except ImportError: # pragma: no cover
from yaml import Loader as YamlLoader

from conjur_api.models import ConjurConnectionInfo
from conjur_api.models import ConjurConnectionInfo, AuthnTypes

# Internals
from conjur.constants import DEFAULT_CONFIG_FILE
Expand All @@ -26,10 +26,11 @@ class ConjurrcData:
Used for setting user input data
"""

def __init__(self, conjur_url: str = None, account: str = None, cert_file: str = None):
def __init__(self, conjur_url: str = None, account: str = None, cert_file: str = None, authn_type: str = None):
self.conjur_url = conjur_url
self.conjur_account = account
self.cert_file = cert_file
self.authn_type = ConjurrcData._parse_authn_type(authn_type)

# pylint: disable=unspecified-encoding
@classmethod
Expand All @@ -42,7 +43,8 @@ def load_from_file(cls, conjurrc_path: str = DEFAULT_CONFIG_FILE):
loaded_conjurrc = yaml_load(conjurrc, Loader=YamlLoader)
return ConjurrcData(loaded_conjurrc['conjur_url'],
loaded_conjurrc['conjur_account'],
loaded_conjurrc['cert_file'])
loaded_conjurrc['cert_file'],
loaded_conjurrc.get('authn_type'))
except KeyError as key_error:
raise InvalidConfigurationException from key_error
except FileNotFoundError as not_found_err:
Expand All @@ -54,7 +56,7 @@ def write_to_file(self, dest: str):
details needed to create a connection to Conjur
"""
with open(dest, 'w') as config_fp:
data = {key: val for key, val in self.__dict__.items() if val is not None}
data = {key: str(val) for key, val in self.__dict__.items() if val is not None}
out = f"---\n{yaml_dump(data)}"
config_fp.write(out)

Expand All @@ -68,3 +70,21 @@ def get_client_connection_info(self) -> ConjurConnectionInfo:
return ConjurConnectionInfo(conjur_url=self.conjur_url,
account=self.conjur_account,
cert_file=self.cert_file)

@staticmethod
def _parse_authn_type(authn_type: str | AuthnTypes) -> AuthnTypes:
"""
Method parses the authn_type string to the AuthnTypes enum
"""

if isinstance(authn_type, AuthnTypes):
return authn_type

if authn_type == 'authn' or authn_type is None:
return AuthnTypes.AUTHN

# Future:
# elif authn_type_str == 'ldap':
# return AuthnTypes.LDAP

raise InvalidConfigurationException(f"Invalid authn_type: {authn_type}")
4 changes: 2 additions & 2 deletions conjur/logic/init_logic.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@

from conjur_api.models import SslVerificationMetadata
from conjur_api import Client
from conjur_api.providers import SimpleCredentialsProvider
from conjur_api.providers import SimpleCredentialsProvider, AuthnAuthenticationStrategy
from conjur.util.ssl_utils.errors import TLSSocketConnectionException
from conjur.util.ssl_utils import SSLClient
from conjur.data_object import ConjurrcData
Expand Down Expand Up @@ -67,8 +67,8 @@ def fetch_account_from_server(cls, conjurrc_data: ConjurrcData, ssl_verification
"""
logging.debug("Attempting to fetch the account from the Conjur server...")
client = Client(connection_info=conjurrc_data.get_client_connection_info(),
authn_strategy=AuthnAuthenticationStrategy(SimpleCredentialsProvider()),
ssl_verification_mode=ssl_verification_metadata.mode,
credentials_provider=SimpleCredentialsProvider(),
async_mode=False)
response = client.get_server_info()
conjurrc_data.conjur_account = response['configuration']['conjur']['account']
Expand Down
4 changes: 2 additions & 2 deletions conjur/logic/login_logic.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
from conjur_api import Client
from conjur_api.models import SslVerificationMetadata, CredentialsData
from conjur_api.interface import CredentialsProviderInterface
from conjur_api.providers import SimpleCredentialsProvider
from conjur_api.providers import SimpleCredentialsProvider, AuthnAuthenticationStrategy
from conjur_api.errors.errors import HttpSslError

# Internals
Expand Down Expand Up @@ -50,8 +50,8 @@ def get_api_key(ssl_verification_metadata: SslVerificationMetadata,
username=credential_data.username,
password=password))
client = Client(connection_info=conjurrc.get_client_connection_info(),
authn_strategy=AuthnAuthenticationStrategy(credentials_provider),
ssl_verification_mode=ssl_verification_metadata.mode,
credentials_provider=credentials_provider,
async_mode=False)
api_key = client.login()
except HttpSslError:
Expand Down
10 changes: 5 additions & 5 deletions design/authn-ldap.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ this design will occur in the API, which will move us closer to a pluggable auth

### User Experience

The `conjur login` command should support LDAP authentication. It should be possible to specify the authentication type using the
The `conjur init` command should support LDAP authentication. It should be possible to specify the authentication type using the
`-t` or `--authn-type` option. The default value should be `authn` (same as current), and it should additionally support `ldap`.
When using `ldap`, a `--ldap-service-id` option should be mandatory.
If the `--ldap-service-id` option is specified, then `--authn-type` should default to `ldap`.
Expand All @@ -76,24 +76,24 @@ If the `--ldap-service-id` option is specified, then `--authn-type` should defau

##### In the CLI repository

The CLI implementation should be straightforward. It needs to support the new CLI arguments in the `login` command,
The CLI implementation should be straightforward. It needs to support the new CLI arguments in the `init` command,
and it will need to pass them to the conjur-api-python library.
Code changes will need to be made in the following files (may not be inclusive):

- [conjur/argument_parser/_login_parser.py](../conjur/argument_parser/_login_parser.py)
- [conjur/argument_parser/_init_parser.py](../conjur/argument_parser/_init_parser.py)
- [conjur/cli.py](../conjur/cli.py)
- [conjur/cli_actions.py](../conjur/cli_actions.py)

##### In the API repository

Create a new `AuthenticationStrategy` interface that will allow us to abstract the authentication flow. The interface should
take a `CredentialProvider` and Conjur account as inputs, as well as any other arguments that are needed to authenticate,
such as `service_id` for LDAP. The interface should have a `login` method which returns a Conjur auth token. All details of authentication
such as `service_id` for LDAP. The interface should have an `authenticate` method which returns a Conjur auth token. All details of authentication (including `login` for authn and authn-ldap)
should be handled by the `AuthenticationStrategy` interface.

With the new interface, we no longer need to pass a `CredentialsProvider` instance to the `Client` class. Instead, we can pass an
`AuthenticationStrategy` instance which will be used to authenticate and store the credentials in it's own `CredentialsProvider` instance.
When calling the `login` method on the `Client`, we will invoke the `AuthenticationStrategy` and return the auth token.
When calling the `authenticate` method on the `Client`, we will invoke the `AuthenticationStrategy` and return the auth token.

#### Out of Scope

Expand Down
3 changes: 2 additions & 1 deletion requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ aiohttp>=3.8.1
asynctest>=0.13.0
setuptools>=57.0.0
twine>=3.2.0
conjur-api>=0.0.3
# conjur-api>=8.0.0
git+https://github.com/cyberark/conjur-api-python.git@authn-ldap

# https://nvd.nist.gov/vuln/detail/CVE-2020-26137
urllib3>=1.25.9
4 changes: 2 additions & 2 deletions test/test_integration_oss.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,5 +55,5 @@ def test_https_conjurrc_is_created_with_no_parameters_given(self):
with open(f"{DEFAULT_CONFIG_FILE}", 'r') as conjurrc:
lines = conjurrc.readlines()
assert "---" in lines[0]
assert "conjur_account: someotheraccount" in lines[2]
assert f"conjur_url: {self.client_params.hostname}" in lines[3]
assert "conjur_account: someotheraccount" in lines[3]
assert f"conjur_url: {self.client_params.hostname}" in lines[4]
9 changes: 5 additions & 4 deletions test/test_unit_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ def test_run_action_runs_init_logic(self, mock_handle_init):
a command will be run to completion. In this case, variable.
'''

@patch.object(FileCredentialsProvider, 'is_exists', return_value=True)
@patch.object(FileCredentialsProvider, 'is_exists', side_effect=[True, False])
@patch('conjur.cli_actions.handle_init_logic')
@patch('conjur.cli_actions.handle_variable_logic')
@patch('os.path.exists', side_effect=[False, True])
Expand Down Expand Up @@ -356,6 +356,7 @@ def test_run_action_runs_init_if_conjurrc_not_found(
a command will be run to completion. In this case, variable.
'''

@patch.object(FileCredentialsProvider, 'is_exists', return_value=False)
@patch('conjur.cli_actions.handle_login_logic')
@patch('conjur.cli_actions.handle_variable_logic')
@patch('os.path.exists', side_effect=[True, False])
Expand All @@ -367,7 +368,7 @@ def test_run_action_runs_init_if_conjurrc_not_found(
def test_run_action_runs_login_if_netrc_not_found(
self, mock_keyring, mock_conjurrc, mock_size,
mock_getenv, mock_path_exists,
mock_variable_init, mock_handle_login):
mock_variable_init, mock_handle_login, mock_is_exists):
with patch('conjur.cli.Client') as mock_client:
mock_keyring.name.return_value = 'somekeyring'
mock_client.return_value = MagicMock()
Expand All @@ -378,7 +379,7 @@ def test_run_action_runs_login_if_netrc_not_found(
Cli().run_action('variable', mock_obj)
mock_handle_login.assert_called_once()

@patch.object(FileCredentialsProvider, 'is_exists', return_value=True)
@patch.object(FileCredentialsProvider, 'is_exists', side_effect=[True, False])
@patch('conjur.cli_actions.handle_user_logic')
@patch('os.path.exists', side_effect=[True, True])
@patch('os.getenv', return_value=None)
Expand All @@ -398,7 +399,7 @@ def test_run_action_runs_user_logic(
Cli().run_action('user', mock_obj)
mock_handle_user.assert_called_once()

@patch.object(FileCredentialsProvider, 'is_exists', return_value=True)
@patch.object(FileCredentialsProvider, 'is_exists', side_effect=[True, False])
@patch('conjur.cli_actions.handle_host_logic')
@patch('os.path.exists', side_effect=[True, True])
@patch('os.getenv', return_value=None)
Expand Down
1 change: 1 addition & 0 deletions test/test_unit_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ def test_config_printed_shows_formatted_fields(self):
re.compile(
"^config:\n" +
"\s+account: accountname\n" +
"\s+authn_type: null\n" +
"\s+ca_bundle: /cert/file/location\n" +
"\s+url: https://someurl/somepath\n",
re.MULTILINE | re.DOTALL,
Expand Down
47 changes: 30 additions & 17 deletions test/test_unit_conjurrc_data.py
Original file line number Diff line number Diff line change
@@ -1,35 +1,48 @@
import unittest
from unittest.mock import mock_open, patch

from conjur.data_object.conjurrc_data import ConjurrcData
from conjur.data_object import ConjurrcData, AuthnTypes
from conjur.errors import InvalidConfigurationException

EXPECTED_REP_OBJECT={'conjur_url': 'https://someurl', 'conjur_account': 'someaccount', 'cert_file': "/some/cert/path"}
EXPECTED_CONJURRC = \
"""
---
conjur_account: someacc
conjur_url: https://someurl
cert_file: /some/path/to/pem
"""

CONJURRC_DICT = {'conjur_url': 'https://someurl', 'conjur_account': 'someacc', 'cert_file': '/some/path/to/pem'}

class ConjurrcDataTest(unittest.TestCase):

def test_conjurrc_object_representation(self):
conjurrc_data = ConjurrcData("https://someurl", "someaccount", "/some/cert/path")
rep_obj = conjurrc_data.__repr__()
self.assertEquals(str(EXPECTED_REP_OBJECT), rep_obj)
expected_rep_obj = {'conjur_url': 'https://someurl', 'conjur_account': 'someaccount', 'cert_file': "/some/cert/path", 'authn_type': AuthnTypes.AUTHN}
self.assertEquals(str(expected_rep_obj), rep_obj)

@patch('yaml.load', return_value=CONJURRC_DICT)
input_dict = {'conjur_url': 'https://someurl', 'conjur_account': 'someacc', 'cert_file': '/some/path/to/pem'}
@patch('yaml.load', return_value=input_dict)
def test_conjurrc_object_is_filled_correctly(self, mock_yaml_load):
with patch("builtins.open", mock_open(read_data=EXPECTED_CONJURRC)):
read_data = \
"""
---
conjur_account: someacc
conjur_url: https://someurl
cert_file: /some/path/to/pem
"""
expected_dict = {'conjur_url': 'https://someurl', 'conjur_account': 'someacc', 'cert_file': '/some/path/to/pem', 'authn_type': AuthnTypes.AUTHN}
with patch("builtins.open", mock_open(read_data=read_data)):
mock_conjurrc_data = ConjurrcData.load_from_file()
self.assertEquals(mock_conjurrc_data.__dict__, CONJURRC_DICT)
self.assertEquals(mock_conjurrc_data.__dict__, expected_dict)

@patch('builtins.open', side_effect=KeyError)
def test_conjurrc_throws_error_when_key_is_missing(self, mock_open):
mock_conjurrc = ConjurrcData("https://someurl", "someaccount", "/some/cert/path")
with self.assertRaises(InvalidConfigurationException):
mock_conjurrc.load_from_file()
mock_conjurrc.load_from_file()

def test_conjurrc_throws_error_when_invalid_authn_type(self):
read_data = \
"""
---
conjur_account: someacc
conjur_url: https://someurl
cert_file: /some/path/to/pem
authn_type: abcd
"""
with patch("builtins.open", mock_open(read_data=read_data)):
with self.assertRaises(InvalidConfigurationException) as context:
ConjurrcData.load_from_file()
self.assertRegex(str(context.exception), "Invalid authn_type")
9 changes: 5 additions & 4 deletions test/util/test_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,13 +58,14 @@ def setup_cli(self):

# *************** INIT ***************

def verify_conjurrc_contents(account, hostname, cert):
def verify_conjurrc_contents(account, hostname, cert, authn_type='authn'):
with open(f"{DEFAULT_CONFIG_FILE}", 'r') as conjurrc:
lines = conjurrc.readlines()
assert "---" in lines[0]
assert f"cert_file: {cert}" in lines[1]
assert f"conjur_account: {account}" in lines[2]
assert f"conjur_url: {hostname}" in lines[3]
assert f"authn_type: {authn_type}" in lines[1], lines[1]
assert f"cert_file: {cert}" in lines[2], lines[2]
assert f"conjur_account: {account}" in lines[3], lines[3]
assert f"conjur_url: {hostname}" in lines[4], lines[4]


# *************** VARIABLE ***************
Expand Down

0 comments on commit 82d35f4

Please sign in to comment.