diff --git a/keystone/oauth1/controllers.py b/keystone/oauth1/controllers.py index 277b677abb..987a7130d9 100644 --- a/keystone/oauth1/controllers.py +++ b/keystone/oauth1/controllers.py @@ -14,6 +14,7 @@ """Extensions supporting OAuth1.""" +from oslo_log import log from oslo_serialization import jsonutils from oslo_utils import timeutils from six.moves import http_client @@ -33,6 +34,7 @@ from keystone.oauth1 import validator CONF = keystone.conf.CONF +LOG = log.getLogger(__name__) def _emit_user_oauth_consumer_token_invalidate(payload): @@ -301,30 +303,54 @@ class OAuthControllerV3(controller.V3Controller): access_verifier = oauth1.AccessTokenEndpoint( request_validator=validator.OAuthValidator(), token_generator=oauth1.token_generator) - h, b, s = access_verifier.create_access_token_response( - url, - http_method='POST', - body=request.params, - headers=request.headers) + try: + h, b, s = access_verifier.create_access_token_response( + url, + http_method='POST', + body=request.params, + headers=request.headers) + except NotImplementedError: + # Client key or request token validation failed, since keystone + # does not yet support dummy client or dummy request token, + # so we will raise Unauthorized exception instead. + try: + self.oauth_api.get_consumer(consumer_id) + except exception.NotFound: + msg = _('Provided consumer does not exist.') + LOG.warning(msg) + raise exception.Unauthorized(message=msg) + if req_token['consumer_id'] != consumer_id: + msg = _('Provided consumer key does not match stored ' + 'consumer key.') + LOG.warning(msg) + raise exception.Unauthorized(message=msg) + # The response body is empty since either one of the following reasons + if not b: + if req_token['verifier'] != oauth_verifier: + msg = _('Provided verifier does not match stored verifier') + else: + msg = _('Invalid signature.') + LOG.warning(msg) + raise exception.Unauthorized(message=msg) params = oauth1.extract_non_oauth_params(b) + # Invalid request would end up with the body like below: + # 'error=invalid_request&description=missing+resource+owner+key' + # Log this detail message so that we will know where is the + # validation failed. if params: - msg = _('There should not be any non-oauth parameters') + if 'error' in params: + msg = _( + 'Validation failed with errors: %(error)s, detail ' + 'message is: %(desc)s.') % { + 'error': params['error'], + 'desc': params['error_description']} + else: + msg = _('There should not be any non-oauth parameters.') + LOG.warning(msg) raise exception.Unauthorized(message=msg) - - if req_token['consumer_id'] != consumer_id: - msg = _('provided consumer key does not match stored consumer key') - raise exception.Unauthorized(message=msg) - - if req_token['verifier'] != oauth_verifier: - msg = _('provided verifier does not match stored verifier') - raise exception.Unauthorized(message=msg) - - if req_token['id'] != request_token_id: - msg = _('provided request key does not match stored request key') - raise exception.Unauthorized(message=msg) - if not req_token.get('authorizing_user_id'): - msg = _('Request Token does not have an authorizing user id') + msg = _('Request Token does not have an authorizing user id.') + LOG.warning(msg) raise exception.Unauthorized(message=msg) access_token_duration = CONF.oauth1.access_token_duration diff --git a/keystone/oauth1/validator.py b/keystone/oauth1/validator.py index 0b7798e4ef..84761185d5 100644 --- a/keystone/oauth1/validator.py +++ b/keystone/oauth1/validator.py @@ -101,7 +101,11 @@ class OAuthValidator(oauth1.RequestValidator): def validate_request_token(self, client_key, token, request): try: - return self.oauth_api.get_request_token(token) is not None + req_token = self.oauth_api.get_request_token(token) + if req_token: + return req_token['consumer_id'] == client_key + else: + return False except exception.NotFound: return False diff --git a/keystone/tests/unit/test_v3_oauth1.py b/keystone/tests/unit/test_v3_oauth1.py index 432eac1297..8531aca3f8 100644 --- a/keystone/tests/unit/test_v3_oauth1.py +++ b/keystone/tests/unit/test_v3_oauth1.py @@ -13,6 +13,7 @@ # under the License. import copy +import random import uuid import mock @@ -680,6 +681,7 @@ class MaliciousOAuth1Tests(OAuth1Tests): self.post(url, headers=headers, expected_status=http_client.NOT_FOUND) def test_bad_verifier(self): + self.config_fixture.config(debug=True, insecure_debug=True) consumer = self._create_single_consumer() consumer_id = consumer['id'] consumer_secret = consumer['secret'] @@ -702,8 +704,77 @@ class MaliciousOAuth1Tests(OAuth1Tests): request_token.set_verifier(uuid.uuid4().hex) url, headers = self._create_access_token(consumer, request_token) - self.post(url, headers=headers, - expected_status=http_client.UNAUTHORIZED) + resp = self.post(url, headers=headers, + expected_status=http_client.UNAUTHORIZED) + resp_data = jsonutils.loads(resp.body) + self.assertIn('Validation failed with errors', + resp_data.get('error', {}).get('message')) + + def test_validate_access_token_request_failed(self): + self.config_fixture.config(debug=True, insecure_debug=True) + consumer = self._create_single_consumer() + consumer_id = consumer['id'] + consumer_secret = consumer['secret'] + consumer = {'key': consumer_id, 'secret': consumer_secret} + + url, headers = self._create_request_token(consumer, self.project_id) + content = self.post( + url, headers=headers, + response_content_type='application/x-www-form-urlencoded') + credentials = _urllib_parse_qs_text_keys(content.result) + request_key = credentials['oauth_token'][0] + request_secret = credentials['oauth_token_secret'][0] + request_token = oauth1.Token(request_key, request_secret) + + url = self._authorize_request_token(request_key) + body = {'roles': [{'id': self.role_id}]} + resp = self.put(url, body=body, expected_status=http_client.OK) + verifier = resp.result['token']['oauth_verifier'] + request_token.set_verifier(verifier) + + # 1. Invalid signature. + # Update the secret, so it will fail to validate the signature. + consumer.update({'secret': uuid.uuid4().hex}) + url, headers = self._create_access_token(consumer, request_token) + resp = self.post(url, headers=headers, + expected_status=http_client.UNAUTHORIZED) + resp_data = jsonutils.loads(resp.body) + self.assertIn('Invalid signature', + resp_data.get('error', {}).get('message')) + + # 2. Invalid verifier. + # Even though the verifier is well formatted, it is not verifier + # that is stored in the backend, this is different with the testcase + # above `test_bad_verifier` where it test that `verifier` is not + # well formatted. + verifier = ''.join(random.SystemRandom().sample(base.VERIFIER_CHARS, + 8)) + request_token.set_verifier(verifier) + url, headers = self._create_access_token(consumer, request_token) + resp = self.post(url, headers=headers, + expected_status=http_client.UNAUTHORIZED) + resp_data = jsonutils.loads(resp.body) + self.assertIn('Provided verifier', + resp_data.get('error', {}).get('message')) + + # 3. The provided consumer does not exist. + consumer.update({'key': uuid.uuid4().hex}) + url, headers = self._create_access_token(consumer, request_token) + resp = self.post(url, headers=headers, + expected_status=http_client.UNAUTHORIZED) + resp_data = jsonutils.loads(resp.body) + self.assertIn('Provided consumer does not exist', + resp_data.get('error', {}).get('message')) + + # 4. The consumer key provided does not match stored consumer key. + consumer2 = self._create_single_consumer() + consumer.update({'key': consumer2['id']}) + url, headers = self._create_access_token(consumer, request_token) + resp = self.post(url, headers=headers, + expected_status=http_client.UNAUTHORIZED) + resp_data = jsonutils.loads(resp.body) + self.assertIn('Provided consumer key', + resp_data.get('error', {}).get('message')) def test_bad_authorizing_roles(self): consumer = self._create_single_consumer() @@ -725,6 +796,40 @@ class MaliciousOAuth1Tests(OAuth1Tests): self.admin_request(path=url, method='PUT', body=body, expected_status=http_client.NOT_FOUND) + def test_no_authorizing_user_id(self): + consumer = self._create_single_consumer() + consumer_id = consumer['id'] + consumer_secret = consumer['secret'] + consumer = {'key': consumer_id, 'secret': consumer_secret} + + url, headers = self._create_request_token(consumer, self.project_id) + content = self.post( + url, headers=headers, + response_content_type='application/x-www-form-urlencoded') + credentials = _urllib_parse_qs_text_keys(content.result) + request_key = credentials['oauth_token'][0] + request_secret = credentials['oauth_token_secret'][0] + request_token = oauth1.Token(request_key, request_secret) + + url = self._authorize_request_token(request_key) + body = {'roles': [{'id': self.role_id}]} + resp = self.put(url, body=body, expected_status=http_client.OK) + verifier = resp.result['token']['oauth_verifier'] + request_token.set_verifier(verifier) + request_token_created = self.oauth_api.get_request_token( + request_key.decode('utf-8')) + request_token_created.update({'authorizing_user_id': ''}) + # Update the request token that is created instead of mocking + # the whole token object to focus on what's we want to test + # here and avoid any other factors that will result in the same + # exception. + with mock.patch.object(self.oauth_api, + 'get_request_token') as mock_token: + mock_token.return_value = request_token_created + url, headers = self._create_access_token(consumer, request_token) + self.post(url, headers=headers, + expected_status=http_client.UNAUTHORIZED) + def test_expired_authorizing_request_token(self): self.config_fixture.config(group='oauth1', request_token_duration=-1) diff --git a/releasenotes/notes/bug-1616424-c46ba773f7ac40ae.yaml b/releasenotes/notes/bug-1616424-c46ba773f7ac40ae.yaml new file mode 100644 index 0000000000..a0a45a417e --- /dev/null +++ b/releasenotes/notes/bug-1616424-c46ba773f7ac40ae.yaml @@ -0,0 +1,8 @@ +--- +fixes: + - > + [`bug 1616424 `_] + Python build-in exception was raised if create request token or access token + request from client with invalid request parameters, invalid signature for example. + The implementation is hardened by showing proper exception and displaying the + failure reasons if existent.