From 01b8942a473e62ba7195485783b05eb084af1398 Mon Sep 17 00:00:00 2001 From: Toilal Date: Thu, 14 Sep 2017 08:43:50 +0200 Subject: [PATCH 1/9] [FIX] Fix CertificateRequest default key configuration --- cfssl/models/certificate_request.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cfssl/models/certificate_request.py b/cfssl/models/certificate_request.py index caef770..edd92b4 100644 --- a/cfssl/models/certificate_request.py +++ b/cfssl/models/certificate_request.py @@ -26,7 +26,7 @@ def __init__(self, common_name, names=None, hosts=None, key=None): self.common_name = common_name self.names = names or [] self.hosts = hosts or [] - self.key = key or KeyConfig() + self.key = key or ConfigKey() def to_api(self): """ It returns an object compatible with the API. """ From 2a4ed8aab99731d92476cf7034220d6af3f54972 Mon Sep 17 00:00:00 2001 From: Toilal Date: Thu, 14 Sep 2017 09:59:36 +0200 Subject: [PATCH 2/9] [FIX] Replace iteritems() with items() for Python 3 support --- cfssl/cfssl.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cfssl/cfssl.py b/cfssl/cfssl.py index b32be2d..7b7de8e 100644 --- a/cfssl/cfssl.py +++ b/cfssl/cfssl.py @@ -392,4 +392,4 @@ def call(self, endpoint, method='GET', params=None, data=None): def _clean_mapping(self, mapping): """ It removes false entries from mapping """ - return {k:v for k, v in mapping.iteritems() if v} + return {k:v for k, v in mapping.items() if v} From c6150b9d950b07d65729acadd45109eb8ffc3a8d Mon Sep 17 00:00:00 2001 From: Toilal Date: Thu, 14 Sep 2017 10:12:29 +0200 Subject: [PATCH 3/9] [FIX] Format response messages properly to fix error handling --- cfssl/cfssl.py | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/cfssl/cfssl.py b/cfssl/cfssl.py index 7b7de8e..827af9a 100644 --- a/cfssl/cfssl.py +++ b/cfssl/cfssl.py @@ -383,13 +383,24 @@ def call(self, endpoint, method='GET', params=None, data=None): raise CFSSLRemoteException( '\n'.join([ 'Errors:', - '\n'.join(response.get('errors', [])), + '\n'.join(map(CFSSL._format_response_message, response.get('errors', []))), 'Messages:' - '\n'.join(response.get('messages', [])), + '\n'.join(map(CFSSL._format_response_message, response.get('messages', []))), ]) ) return response['result'] + @staticmethod + def _format_response_message(error): + message = '' + if 'message' in error: + message += error['message'] + if 'code' in error: + message += ' (%s)' % error['code'] + if not message: + message = str(error) + return message + def _clean_mapping(self, mapping): """ It removes false entries from mapping """ return {k:v for k, v in mapping.items() if v} From 3500c991b94ddf4c506c4ce23a4684513c3e5f1a Mon Sep 17 00:00:00 2001 From: Toilal Date: Thu, 14 Sep 2017 10:36:05 +0200 Subject: [PATCH 4/9] [FIX] Use application/json to send data --- cfssl/cfssl.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cfssl/cfssl.py b/cfssl/cfssl.py index 827af9a..80b7ac5 100644 --- a/cfssl/cfssl.py +++ b/cfssl/cfssl.py @@ -375,7 +375,7 @@ def call(self, endpoint, method='GET', params=None, data=None): method=method, url=endpoint, params=params, - data=data, + json=data, verify=self.verify, ) response = response.json() From b3b5a70957e824e85767500fa7611368331f4d30 Mon Sep 17 00:00:00 2001 From: Toilal Date: Thu, 14 Sep 2017 10:56:04 +0200 Subject: [PATCH 5/9] [IMP] Log messages from CFSSL api responses --- cfssl/cfssl.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/cfssl/cfssl.py b/cfssl/cfssl.py index 80b7ac5..db9d1d0 100644 --- a/cfssl/cfssl.py +++ b/cfssl/cfssl.py @@ -3,11 +3,14 @@ # License MIT (https://opensource.org/licenses/MIT). import requests +import logging from .exceptions import CFSSLException, CFSSLRemoteException from .models.config_key import ConfigKey +log = logging.getLogger(__name__) + class CFSSL(object): """ It provides Python bindings to a remote CFSSL server via HTTP(S). @@ -388,6 +391,9 @@ def call(self, endpoint, method='GET', params=None, data=None): '\n'.join(map(CFSSL._format_response_message, response.get('messages', []))), ]) ) + if response['messages']: + for message in response['messages']: + log.warning(CFSSL._format_response_message(message)) return response['result'] @staticmethod From 72d0c42436666647d5dd5422079428dee9641f8a Mon Sep 17 00:00:00 2001 From: Toilal Date: Thu, 14 Sep 2017 10:57:20 +0200 Subject: [PATCH 6/9] [IMP] Add missing state parameter in SubjectInfo doctring --- cfssl/models/subject_info.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cfssl/models/subject_info.py b/cfssl/models/subject_info.py index d5b7140..0fb72a0 100644 --- a/cfssl/models/subject_info.py +++ b/cfssl/models/subject_info.py @@ -15,6 +15,8 @@ def __init__(self, org_name, org_unit, city, state, country): org_unit (str): Section of the organization. city (str): The city where the organization is legally located. + state (str): The state or province where your organization + is legally located. Can not be abbreviated. country (str): The two letter ISO abbreviation for the country. """ From 6d43f5093236c172d130b53e845f090044010882 Mon Sep 17 00:00:00 2001 From: Toilal Date: Thu, 14 Sep 2017 17:19:06 +0200 Subject: [PATCH 7/9] [FIX] Make CertificateRequest mandatory values consistent with API --- cfssl/models/certificate_request.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/cfssl/models/certificate_request.py b/cfssl/models/certificate_request.py index edd92b4..637c5e5 100644 --- a/cfssl/models/certificate_request.py +++ b/cfssl/models/certificate_request.py @@ -10,11 +10,11 @@ class CertificateRequest(object): """ It provides a Certificate Request compatible with CFSSL. """ - def __init__(self, common_name, names=None, hosts=None, key=None): + def __init__(self, common_name=None, names=None, hosts=None, key=None): """ Initialize a new CertificateRequest. Args: - common_name (str): The fully qualified domain name for the + common_name (str, optional): The fully qualified domain name for the server. This must be an exact match. names (tuple of SubjectInfo, optional): Subject Information to be added to the request. @@ -26,17 +26,20 @@ def __init__(self, common_name, names=None, hosts=None, key=None): self.common_name = common_name self.names = names or [] self.hosts = hosts or [] - self.key = key or ConfigKey() + self.key = key def to_api(self): """ It returns an object compatible with the API. """ - return { - 'CN': self.common_name, + api = { 'names': [ name.to_api() for name in self.names ], 'hosts': [ host.to_api() for host in self.hosts - ], - 'key': self.key.to_api(), + ] } + if self.common_name: + api['CN'] = self.common_name + if self.key: + api['key'] = self.key.to_api() + return api From 46259755aaf653e13429ea72c0f96a0c9e73dc31 Mon Sep 17 00:00:00 2001 From: Toilal Date: Wed, 20 Sep 2017 14:46:33 +0200 Subject: [PATCH 8/9] [IMP] Support native structures In some use case, it's easier to pass native structures (dict, string, ...) instead of domain objects (CertificateRequest, SubjectInfo, ...) --- cfssl/cfssl.py | 25 +++++++++++++------------ cfssl/models/certificate_request.py | 7 ++++--- cfssl/models/config_client.py | 3 ++- cfssl/models/config_mixer.py | 8 +++++--- cfssl/models/policy_sign.py | 3 ++- cfssl/utils.py | 15 +++++++++++++++ 6 files changed, 41 insertions(+), 20 deletions(-) create mode 100644 cfssl/utils.py diff --git a/cfssl/cfssl.py b/cfssl/cfssl.py index db9d1d0..b721add 100644 --- a/cfssl/cfssl.py +++ b/cfssl/cfssl.py @@ -8,6 +8,7 @@ from .exceptions import CFSSLException, CFSSLRemoteException from .models.config_key import ConfigKey +from .utils import to_api log = logging.getLogger(__name__) @@ -48,7 +49,7 @@ def auth_sign(self, token, request, datetime=None, remote_address=None): """ data = self._clean_mapping({ 'token': token, - 'request': request.to_api(), + 'request': to_api(request), 'datetime': datetime, 'remote_address': remote_address, }) @@ -186,13 +187,13 @@ def init_ca(self, certificate_request, ca=None): * private key (str): a PEM-encoded CA private key. * certificate (str): a PEM-encoded self-signed CA certificate. """ - csr_api = certificate_request.to_api() + csr_api = to_api(certificate_request) data = self._clean_mapping({ 'hosts': csr_api['hosts'], 'names': csr_api['names'], 'CN': csr_api['CN'], 'key': csr_api['key'], - 'ca': ca and ca.to_api() or None, + 'ca': ca and to_api(ca) or None, }) return self.call('init_ca', 'POST', data=data) @@ -217,14 +218,14 @@ def new_key(self, hosts, names, common_name=None, key=None, ca=None): """ data = self._clean_mapping({ 'hosts': [ - host.to_api() for host in hosts + to_api(host) for host in hosts ], 'names': [ - name.to_api() for name in names + to_api(name) for name in names ], 'CN': common_name, - 'key': key and key.to_api() or ConfigKey().to_api(), - 'ca': ca and ca.to_api() or None, + 'key': key and to_api(key) or ConfigKey().to_api(), + 'ca': ca and to_api(ca) or None, }) return self.call('newkey', 'POST', data=data) @@ -251,7 +252,7 @@ def new_cert(self, request, label=None, profile=None, bundle=None): if the bundle parameter was set). """ data = self._clean_mapping({ - 'request': request.to_api(), + 'request': to_api(request), 'label': label, 'profile': profile, 'bundle': bundle, @@ -303,7 +304,7 @@ def scan(self, host, ip=None, timeout=None, family=None, scanner=None): * output: (dict) Arbitrary data retrieved during the scan. """ data = self._clean_mapping({ - 'host': host.to_api(), + 'host': to_api(host), 'ip': ip, 'timeout': timeout, 'family': family, @@ -345,14 +346,14 @@ def sign(self, certificate_request, hosts=None, subject=None, server. """ data = self._clean_mapping({ - 'certificate_request': certificate_request.to_api(), + 'certificate_request': to_api(certificate_request), 'hosts': [ - host.to_api() for host in hosts + to_api(host) for host in hosts ], 'subject': subject, 'serial_sequence': serial_sequence, 'label': label, - 'profile': profile.to_api(), + 'profile': to_api(profile), }) result = self.call('sign', 'POST', data=data) return result['certificate'] diff --git a/cfssl/models/certificate_request.py b/cfssl/models/certificate_request.py index 637c5e5..30730c3 100644 --- a/cfssl/models/certificate_request.py +++ b/cfssl/models/certificate_request.py @@ -5,6 +5,7 @@ from .host import Host from .config_key import ConfigKey from .subject_info import SubjectInfo +from ..utils import to_api class CertificateRequest(object): @@ -32,14 +33,14 @@ def to_api(self): """ It returns an object compatible with the API. """ api = { 'names': [ - name.to_api() for name in self.names + to_api(name) for name in self.names ], 'hosts': [ - host.to_api() for host in self.hosts + to_api(host) for host in self.hosts ] } if self.common_name: api['CN'] = self.common_name if self.key: - api['key'] = self.key.to_api() + api['key'] = to_api(self.key) return api diff --git a/cfssl/models/config_client.py b/cfssl/models/config_client.py index 8cd4cb4..62f4531 100644 --- a/cfssl/models/config_client.py +++ b/cfssl/models/config_client.py @@ -3,6 +3,7 @@ # License MIT (https://opensource.org/licenses/MIT). from .config_mixer import ConfigMixer +from ..utils import to_api class ConfigClient(ConfigMixer): @@ -31,6 +32,6 @@ def to_api(self): """ It returns an object compatible with the API. """ res = super(ConfigClient, self).to_api() res['remotes'] = { - r.name: r.to_api() for r in self.remotes + r.name: to_api(r) for r in self.remotes } return res diff --git a/cfssl/models/config_mixer.py b/cfssl/models/config_mixer.py index 175e9fd..38d4d2d 100644 --- a/cfssl/models/config_mixer.py +++ b/cfssl/models/config_mixer.py @@ -2,6 +2,8 @@ # Copyright 2016 LasLabs Inc. # License MIT (https://opensource.org/licenses/MIT). +from ..utils import to_api + class ConfigMixer(object): """ It provides a mixer for the Client and Server Configs """ @@ -25,12 +27,12 @@ def to_api(self): """ It returns an object compatible with the API. """ return { 'signing': { - 'default': self.sign_policy.to_api(), + 'default': to_api(self.sign_policy), 'profiles': { - p.name: p.to_api() for p in self.sign_policies + p.name: to_api(p) for p in self.sign_policies }, }, 'auth_keys': { - k.name: k.to_api() for k in self.auth_policies + k.name: to_api(k) for k in self.auth_policies }, } diff --git a/cfssl/models/policy_sign.py b/cfssl/models/policy_sign.py index 6f21808..3c313c7 100644 --- a/cfssl/models/policy_sign.py +++ b/cfssl/models/policy_sign.py @@ -3,6 +3,7 @@ # License MIT (https://opensource.org/licenses/MIT). from ..defaults import DEFAULT_EXPIRE_DELTA +from ..utils import to_api class PolicySign(object): @@ -31,5 +32,5 @@ def to_api(self): return { 'auth_key': self.auth_policy.name, 'expiry': '%ds' % self.expire_delta.total_seconds(), - 'usages': [u.to_api() for u in self.usage_policies], + 'usages': [to_api(u) for u in self.usage_policies], } diff --git a/cfssl/utils.py b/cfssl/utils.py new file mode 100644 index 0000000..24bb7c7 --- /dev/null +++ b/cfssl/utils.py @@ -0,0 +1,15 @@ +# -*- coding: utf-8 -*- +# Copyright 2016 LasLabs Inc. +# License MIT (https://opensource.org/licenses/MIT). + + +def to_api(object): + """ Ensure an object is converted using it's to_api method if it exists. + + Args: + object (any): + Returns: + str: A PEM-encoded certificate that has been signed by the + server. + """ + return object.to_api() if hasattr(object, 'to_api') else object \ No newline at end of file From 23e1a2afda56ac47efb075f12d8a95b3685e82a4 Mon Sep 17 00:00:00 2001 From: Toilal Date: Wed, 20 Sep 2017 15:21:33 +0200 Subject: [PATCH 9/9] [FIX] Fix existing tests and add more --- cfssl/cfssl.py | 10 ++++----- cfssl/tests/test_certificate_request.py | 15 +++++++++++++ cfssl/tests/test_cfssl.py | 23 +++++++++++++++++-- cfssl/tests/test_utils.py | 30 +++++++++++++++++++++++++ 4 files changed, 71 insertions(+), 7 deletions(-) create mode 100644 cfssl/tests/test_utils.py diff --git a/cfssl/cfssl.py b/cfssl/cfssl.py index b721add..1cf8d68 100644 --- a/cfssl/cfssl.py +++ b/cfssl/cfssl.py @@ -392,7 +392,7 @@ def call(self, endpoint, method='GET', params=None, data=None): '\n'.join(map(CFSSL._format_response_message, response.get('messages', []))), ]) ) - if response['messages']: + if 'messages' in response: for message in response['messages']: log.warning(CFSSL._format_response_message(message)) return response['result'] @@ -400,10 +400,10 @@ def call(self, endpoint, method='GET', params=None, data=None): @staticmethod def _format_response_message(error): message = '' - if 'message' in error: - message += error['message'] - if 'code' in error: - message += ' (%s)' % error['code'] + if isinstance(error, dict): + message += error['message'] if 'message' in error else '' + if 'code' in error: + message += ' (%s)' % error['code'] if not message: message = str(error) return message diff --git a/cfssl/tests/test_certificate_request.py b/cfssl/tests/test_certificate_request.py index ba63e10..35f33c5 100644 --- a/cfssl/tests/test_certificate_request.py +++ b/cfssl/tests/test_certificate_request.py @@ -20,6 +20,12 @@ def setUp(self): } self.model = CertificateRequest(**self.vals) + self.partial_vals = { + 'names': [mock.MagicMock()], + 'hosts': [mock.MagicMock()], + } + self.model_partial = CertificateRequest(**self.partial_vals) + def test_to_api(self): """ It should return the correctly compatible obj """ res = self.model.to_api() @@ -31,6 +37,15 @@ def test_to_api(self): } self.assertDictEqual(res, expect) + def test_to_api_partial(self): + """ It should return the correctly compatible obj when no CN and no key are defined """ + res = self.model_partial.to_api() + expect = { + 'names': [self.partial_vals['names'][0].to_api()], + 'hosts': [self.partial_vals['hosts'][0].to_api()], + } + self.assertDictEqual(res, expect) + if __name__ == '__main__': unittest.main() diff --git a/cfssl/tests/test_cfssl.py b/cfssl/tests/test_cfssl.py index 2dff9b8..d971193 100644 --- a/cfssl/tests/test_cfssl.py +++ b/cfssl/tests/test_cfssl.py @@ -10,6 +10,7 @@ CFSSLRemoteException, requests, ) +from cfssl import cfssl _logger = logging.getLogger(__name__) @@ -184,7 +185,7 @@ def test_call_request(self, requests): method='method', url='https://test:1/api/v1/cfssl/endpoint', params='params', - data='data', + json='data', verify=True, ) @@ -197,11 +198,29 @@ def test_call_error(self, requests): @mock.patch.object(requests, 'request') def test_call_success(self, requests): - """ It should reteurn result on success response """ + """ It should return result on success response """ requests().json.return_value = {'success': True, 'result': 'result'} res = self.cfssl.call(None) self.assertEqual(res, 'result') + @mock.patch.object(cfssl, 'log') + @mock.patch.object(requests, 'request') + def test_log_messages(self, requests, log): + """ It should return result on success response """ + requests().json.return_value = {'success': True, + 'messages': [ + {'message': 'some message', 'code': 5000}, + {'code': 5001}, + {'message': 'message only'}, + 'another message'], + 'result': 'result'} + res = self.cfssl.call(None) + self.assertEqual(res, 'result') + log.warning.assert_any_call('some message (5000)') + log.warning.assert_any_call(' (5001)') + log.warning.assert_any_call('message only') + log.warning.assert_any_call('another message') + if __name__ == '__main__': unittest.main() diff --git a/cfssl/tests/test_utils.py b/cfssl/tests/test_utils.py new file mode 100644 index 0000000..01107a1 --- /dev/null +++ b/cfssl/tests/test_utils.py @@ -0,0 +1,30 @@ +# -*- coding: utf-8 -*- +# Copyright 2016 LasLabs Inc. +# License MIT (https://opensource.org/licenses/MIT). + +import unittest +from ..utils import to_api + + +class TestUtils(unittest.TestCase): + def test_to_api_native_structure(self): + """ It should return the same object when it doesn't implement to_api()""" + + expect = "fallback" + res = to_api(expect) + self.assertIs(res, expect) + + def test_to_api_object(self): + """ It should delegate to to_api() method of a supported object""" + + class SupportedObject(object): + def to_api(self): + return "supported" + + expect = "supported" + res = to_api(SupportedObject()) + self.assertEqual(res, expect) + + +if __name__ == '__main__': + unittest.main()