Handle token exception and use proper url for verification

This commit is a product of two separate commits in order to unwedge
the stable/newton gate. The first commit is a oauth refactor to
properly handle token exceptions. The second is a patch to that
uses the proper url when verifying an oauth request token. The
problem is that the second patch can't be applied due to the
refactor from the first. This commit merges the two commits
together so that their isn't a merge conflict and it passes the
currently broken gate.

The first commit is:

Handle the exception from creating access token properly

If there is any request from client with any invalid request
parameters, invalid signature for example, keystone should
capture that and raise the exception.

It was `NotImplementedError`, `TypeError` thrown out and
presented directly to end user, and nothing helpful message
is given.

This patch fix that and show as many exception message that
is helpful for diagnosis as possible.

Change-Id: I112d0cd0c8a460c7b4d8d0e1c0b9c742aab9fde7
Closes-Bug: #1616424
(cherry picked from commit be5385c5389aa9c4879647c9b9e4327cc73189a2)

This is the second commit

Change url passed to oauth signature verifier to request url

OAUTH signature verification should happen with the same URL used for signing.
Typically at the user end it should be signed with the request URL and hence it
should be verified with the same.
Currently keystone uses public endpoint URL for signature verification.

Modified the URL passed to oauth signature verification to request URL.

Change-Id: I28059a43cb0088c2952c19f696042ebec54d26c9
Partial-Bug: #1687593
(cherry picked from commit 926685c5a4823d7e3ab3879bae1529052fff7d68)

4 files changed, 197 insertions, 32 deletions

diff --git a/keystone/oauth1/controllers.py b/keystone/oauth1/controllers.py
index 277b677ab..d774cd777 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):
@@ -228,15 +230,13 @@ class OAuthControllerV3(controller.V3Controller):
         self.resource_api.get_project(requested_project_id)
         self.oauth_api.get_consumer(consumer_id)
 
-        url = self.base_url(request.context_dict, request.context_dict['path'])
-
         req_headers = {'Requested-Project-Id': requested_project_id}
         req_headers.update(request.headers)
         request_verifier = oauth1.RequestTokenEndpoint(
             request_validator=validator.OAuthValidator(),
             token_generator=oauth1.token_generator)
         h, b, s = request_verifier.create_request_token_response(
-            url,
+            request.url,
             http_method='POST',
             body=request.params,
             headers=req_headers)
@@ -296,35 +296,57 @@ class OAuthControllerV3(controller.V3Controller):
             if now > expires:
                 raise exception.Unauthorized(_('Request token is expired'))
 
-        url = self.base_url(request.context_dict, request.context_dict['path'])
-
         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(
+                request.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')
-            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')
+            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['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 0b7798e4e..84761185d 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 432eac129..235d0fe87 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
@@ -71,19 +72,21 @@ class OAuth1Tests(test_v3.RestfulTestCase):
             body={'consumer': ref})
         return resp.result['consumer']
 
-    def _create_request_token(self, consumer, project_id):
+    def _create_request_token(self, consumer, project_id, base_url=None):
         endpoint = '/OS-OAUTH1/request_token'
         client = oauth1.Client(consumer['key'],
                                client_secret=consumer['secret'],
                                signature_method=oauth1.SIG_HMAC,
                                callback_uri="oob")
         headers = {'requested_project_id': project_id}
-        url, headers, body = client.sign(self.base_url + endpoint,
+        if not base_url:
+            base_url = self.base_url
+        url, headers, body = client.sign(base_url + endpoint,
                                          http_method='POST',
                                          headers=headers)
         return endpoint, headers
 
-    def _create_access_token(self, consumer, token):
+    def _create_access_token(self, consumer, token, base_url=None):
         endpoint = '/OS-OAUTH1/access_token'
         client = oauth1.Client(consumer['key'],
                                client_secret=consumer['secret'],
@@ -91,7 +94,9 @@ class OAuth1Tests(test_v3.RestfulTestCase):
                                resource_owner_secret=token.secret,
                                signature_method=oauth1.SIG_HMAC,
                                verifier=token.verifier)
-        url, headers, body = client.sign(self.base_url + endpoint,
+        if not base_url:
+            base_url = self.base_url
+        url, headers, body = client.sign(base_url + endpoint,
                                          http_method='POST')
         headers.update({'Content-Type': 'application/json'})
         return endpoint, headers
@@ -649,6 +654,17 @@ class MaliciousOAuth1Tests(OAuth1Tests):
         self.post(url, headers=headers,
                   expected_status=http_client.UNAUTHORIZED)
 
+    def test_bad_request_url(self):
+        consumer = self._create_single_consumer()
+        consumer_id = consumer['id']
+        consumer_secret = consumer['secret']
+        consumer = {'key': consumer_id, 'secret': consumer_secret}
+        bad_base_url = 'http://localhost/identity_admin/v3'
+        url, headers = self._create_request_token(consumer, self.project_id,
+                                                  base_url=bad_base_url)
+        self.post(url, headers=headers,
+                  expected_status=http_client.UNAUTHORIZED)
+
     def test_bad_request_token_key(self):
         consumer = self._create_single_consumer()
         consumer_id = consumer['id']
@@ -680,6 +696,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 +719,88 @@ 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 base url.
+        # Update the base url, so it will fail to validate the signature.
+        base_url = 'http://localhost/identity_admin/v3'
+        url, headers = self._create_access_token(consumer, request_token,
+                                                 base_url=base_url)
+        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 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'))
+
+        # 3. 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'))
+
+        # 4. 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'))
+
+        # 5. 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 +822,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 000000000..a0a45a417
--- /dev/null
+++ b/releasenotes/notes/bug-1616424-c46ba773f7ac40ae.yaml
@@ -0,0 +1,8 @@
+---
+fixes:
+  - >
+    [`bug 1616424 <https://bugs.launchpad.net/keystone/+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.