From c888a46ccc46346deee8d483a706918341bc0a7f Mon Sep 17 00:00:00 2001 From: Renat Akhmerov Date: Fri, 27 Dec 2019 16:20:19 +0700 Subject: [PATCH] Fix keycloak authentication * Implement offline access token validation using Keycloak public key. Closes-bug: #1857871 Change-Id: I0eecec4b4e64381cac005622b16c6d9e4bed4df6 --- lower-constraints.txt | 2 +- mistral/auth/keycloak.py | 173 ++++++++++++------ mistral/config.py | 10 + mistral/context.py | 7 + .../tests/unit/api/v2/test_keycloak_auth.py | 143 +++++++++------ requirements.txt | 2 +- 6 files changed, 233 insertions(+), 104 deletions(-) diff --git a/lower-constraints.txt b/lower-constraints.txt index af05c12f3..bf90e5460 100644 --- a/lower-constraints.txt +++ b/lower-constraints.txt @@ -102,7 +102,7 @@ pycparser==2.18 pyflakes==0.8.1 Pygments==2.2.0 pyinotify==0.9.6 -PyJWT==1.0.1 +PyJWT==1.5 pyOpenSSL==17.1.0 pyparsing==2.1.0 pyperclip==1.5.27 diff --git a/mistral/auth/keycloak.py b/mistral/auth/keycloak.py index 1a87eacff..2d7cbb243 100644 --- a/mistral/auth/keycloak.py +++ b/mistral/auth/keycloak.py @@ -14,7 +14,11 @@ import os +from cachetools import cached +from cachetools import LRUCache +import json import jwt +from jwt import algorithms as jwt_algos from oslo_config import cfg from oslo_log import log as logging import pprint @@ -32,31 +36,34 @@ CONF = cfg.CONF class KeycloakAuthHandler(auth.AuthHandler): - def authenticate(self, req): - certfile = CONF.keycloak_oidc.certfile - keyfile = CONF.keycloak_oidc.keyfile - cafile = CONF.keycloak_oidc.cafile or self.get_system_ca_file() - insecure = CONF.keycloak_oidc.insecure - if 'X-Auth-Token' not in req.headers: msg = _("Auth token must be provided in 'X-Auth-Token' header.") + LOG.error(msg) + raise exc.UnauthorizedException(message=msg) + access_token = req.headers.get('X-Auth-Token') try: - decoded = jwt.decode(access_token, algorithms=['RS256'], - verify=False) + decoded = jwt.decode( + access_token, + algorithms=['RS256'], + verify=False + ) except Exception as e: msg = _("Token can't be decoded because of wrong format %s")\ % str(e) + LOG.error(msg) + raise exc.UnauthorizedException(message=msg) # Get user realm from parsed token # Format is "iss": "http://:/auth/realms/", __, __, realm_name = decoded['iss'].strip().rpartition('/realms/') + audience = decoded.get('aud') # Get roles from parsed token roles = ','.join(decoded['realm_access']['roles']) \ @@ -70,46 +77,31 @@ class KeycloakAuthHandler(auth.AuthHandler): user_info_endpoint_url = CONF.keycloak_oidc.user_info_endpoint_url if user_info_endpoint_url.startswith(('http://', 'https://')): - user_info_endpoint = user_info_endpoint_url - else: - user_info_endpoint = ( - ("%s" + user_info_endpoint_url) % - (CONF.keycloak_oidc.auth_url, realm_name)) - - verify = None - if urllib.parse.urlparse(user_info_endpoint).scheme == "https": - verify = False if insecure else cafile - - cert = (certfile, keyfile) if certfile and keyfile else None - - try: - resp = requests.get( - user_info_endpoint, - headers={"Authorization": "Bearer %s" % access_token}, - verify=verify, - cert=cert + self.send_request_to_auth_server( + url=user_info_endpoint_url, + access_token=access_token ) - except requests.ConnectionError: - msg = _("Can't connect to keycloak server with address '%s'." - ) % CONF.keycloak_oidc.auth_url - LOG.error(msg) - raise exc.MistralException(message=msg) - - if resp.status_code == 401: - LOG.warning("HTTP response from OIDC provider:" - " [%s] with WWW-Authenticate: [%s]", - pprint.pformat(resp.text), - resp.headers.get("WWW-Authenticate")) else: - LOG.debug("HTTP response from OIDC provider: %s", - pprint.pformat(resp.text)) + public_key = self.get_public_key(realm_name) - resp.raise_for_status() + keycloak_iss = None - LOG.debug( - "HTTP response from OIDC provider: %s", - pprint.pformat(resp.json()) - ) + try: + if CONF.keycloak_oidc.keycloak_iss: + keycloak_iss = CONF.keycloak_oidc.keycloak_iss % realm_name + + jwt.decode( + access_token, + public_key, + audience=audience, + issuer=keycloak_iss, + algorithms=['RS256'], + verify=True + ) + except Exception: + LOG.exception('The request access token is invalid.') + + raise exc.UnauthorizedException() req.headers["X-Identity-Status"] = "Confirmed" req.headers["X-Project-Id"] = realm_name @@ -119,16 +111,95 @@ class KeycloakAuthHandler(auth.AuthHandler): def get_system_ca_file(): """Return path to system default CA file.""" # Standard CA file locations for Debian/Ubuntu, RedHat/Fedora, - # Suse, FreeBSD/OpenBSD, MacOSX, and the bundled ca - ca_path = ['/etc/ssl/certs/ca-certificates.crt', - '/etc/pki/tls/certs/ca-bundle.crt', - '/etc/ssl/ca-bundle.pem', - '/etc/ssl/cert.pem', - '/System/Library/OpenSSL/certs/cacert.pem', - requests.certs.where()] + # Suse, FreeBSD/OpenBSD, MacOSX, and the bundled ca. + ca_path = [ + '/etc/ssl/certs/ca-certificates.crt', + '/etc/pki/tls/certs/ca-bundle.crt', + '/etc/ssl/ca-bundle.pem', + '/etc/ssl/cert.pem', + '/System/Library/OpenSSL/certs/cacert.pem', + requests.certs.where() + ] + for ca in ca_path: LOG.debug("Looking for ca file %s", ca) + if os.path.exists(ca): LOG.debug("Using ca file %s", ca) + return ca + LOG.warning("System ca file could not be found.") + + @cached(LRUCache(maxsize=32)) + def get_public_key(self, realm_name): + keycloak_key_url = ( + CONF.keycloak_oidc.auth_url + + CONF.keycloak_oidc.public_cert_url % realm_name + ) + + response_json = self.send_request_to_auth_server(keycloak_key_url) + + keys = response_json.get('keys') + + if not keys: + raise exc.MistralException( + 'Unexpected response structure from the keycloak server.' + ) + + public_key = jwt_algos.RSAAlgorithm.from_jwk( + json.dumps(keys[0]) + ) + + return public_key + + def send_request_to_auth_server(self, url, access_token=None): + certfile = CONF.keycloak_oidc.certfile + keyfile = CONF.keycloak_oidc.keyfile + cafile = CONF.keycloak_oidc.cafile or self.get_system_ca_file() + insecure = CONF.keycloak_oidc.insecure + + verify = None + + if urllib.parse.urlparse(url).scheme == "https": + verify = False if insecure else cafile + + cert = (certfile, keyfile) if certfile and keyfile else None + + headers = {} + + if access_token: + headers["Authorization"] = "Bearer %s" % access_token + + try: + resp = requests.get( + url, + headers=headers, + verify=verify, + cert=cert + ) + except requests.ConnectionError: + msg = _( + "Can't connect to the keycloak server with address '%s'." + ) % url + + LOG.exception(msg) + + raise exc.MistralException(message=msg) + + if resp.status_code == 401: + LOG.warning( + "HTTP response from OIDC provider:" + " [%s] with WWW-Authenticate: [%s]", + pprint.pformat(resp.text), + resp.headers.get("WWW-Authenticate") + ) + else: + LOG.debug( + "HTTP response from the OIDC provider: %s", + pprint.pformat(resp.json()) + ) + + resp.raise_for_status() + + return resp.json() diff --git a/mistral/config.py b/mistral/config.py index 4e8c7f182..e8e3d8716 100644 --- a/mistral/config.py +++ b/mistral/config.py @@ -546,6 +546,16 @@ keycloak_oidc_opts = [ default='/realms/%s/protocol/openid-connect/userinfo', help='Endpoint against which authorization will be performed' ), + cfg.StrOpt( + 'public_cert_url', + default="/realms/%s/protocol/openid-connect/certs", + help="URL to get the public key for a particular realm" + ), + cfg.StrOpt( + 'keycloak_iss', + help="Keycloak issuer(iss) url. " + "Example: https://ip_add:port/auth/realms/%s" + ) ] openstack_actions_opts = [ diff --git a/mistral/context.py b/mistral/context.py index 33f860f44..1746a509a 100644 --- a/mistral/context.py +++ b/mistral/context.py @@ -19,6 +19,7 @@ from mistral_lib.actions import context as lib_ctx from mistral_lib import serialization from oslo_config import cfg from oslo_context import context as oslo_context +from oslo_log import log as logging import oslo_messaging as messaging from oslo_serialization import jsonutils from osprofiler import profiler @@ -29,7 +30,11 @@ from mistral import auth from mistral import exceptions as exc from mistral_lib import utils + +LOG = logging.getLogger(__name__) + CONF = cfg.CONF + _CTX_THREAD_LOCAL_NAME = "MISTRAL_APP_CTX_THREAD_LOCAL" ALLOWED_WITHOUT_AUTH = ['/', '/v2/', '/workflowv2/', '/workflowv2/v2/'] @@ -249,6 +254,8 @@ class AuthHook(hooks.PecanHook): except Exception as e: msg = "Failed to validate access token: %s" % str(e) + LOG.exception(msg) + pecan.abort( status_code=401, detail=msg, diff --git a/mistral/tests/unit/api/v2/test_keycloak_auth.py b/mistral/tests/unit/api/v2/test_keycloak_auth.py index a221d39e7..db3e22232 100644 --- a/mistral/tests/unit/api/v2/test_keycloak_auth.py +++ b/mistral/tests/unit/api/v2/test_keycloak_auth.py @@ -30,6 +30,18 @@ from mistral.services import periodic from mistral.tests.unit import base from mistral.tests.unit.mstrlfixtures import policy_fixtures +KEYCLOAK_JSON = { + "keys": [ + { + "kid": "FJ86GcF3jTbNLOco4NvZkUCIUmfYCqoqtOQeMfbhNlE", + "kty": "RSA", + "alg": "RS256", + "use": "sig", + "n": "q1awrk7QK24Gmcy9Yb4dMbS-ZnO6", + "e": "AQAB" + } + ] +} WF_DEFINITION = """ --- @@ -86,7 +98,6 @@ WWW_AUTHENTICATE_HEADER = {'WWW-Authenticate': 'unauthorized reason is ...'} class TestKeyCloakOIDCAuth(base.BaseTest): - def setUp(self): super(TestKeyCloakOIDCAuth, self).setUp() @@ -102,8 +113,14 @@ class TestKeyCloakOIDCAuth(base.BaseTest): return req - @requests_mock.Mocker() - def test_header_parsing(self, req_mock): + @mock.patch("requests.get") + def test_header_parsing(self, mocked_get): + self.override_config( + 'user_info_endpoint_url', + 'https://127.0.0.1:9080', + 'keycloak_oidc' + ) + token = { "iss": "http://localhost:8080/auth/realms/my_realm", "realm_access": { @@ -111,8 +128,11 @@ class TestKeyCloakOIDCAuth(base.BaseTest): } } - # Imitate successful response from KeyCloak with user claims. - req_mock.get(USER_INFO_ENDPOINT, json=USER_CLAIMS) + mocked_resp = mock.Mock() + mocked_resp.status_code = 200 + mocked_resp.json.return_value = KEYCLOAK_JSON + + mocked_get.return_value = mocked_resp req = self._build_request(token) @@ -122,7 +142,7 @@ class TestKeyCloakOIDCAuth(base.BaseTest): self.assertEqual("Confirmed", req.headers["X-Identity-Status"]) self.assertEqual("my_realm", req.headers["X-Project-Id"]) self.assertEqual("role1,role2", req.headers["X-Roles"]) - self.assertEqual(1, req_mock.call_count) + self.assertEqual(1, mocked_get.call_count) def test_no_auth_token(self): req = webob.Request.blank("/") @@ -133,12 +153,19 @@ class TestKeyCloakOIDCAuth(base.BaseTest): req ) - @requests_mock.Mocker() - def test_no_realm_roles(self, req_mock): - token = {"iss": "http://localhost:8080/auth/realms/my_realm"} + @mock.patch("requests.get") + def test_no_realm_roles(self, mocked_get): + token = { + "aud": "openstack", + "iss": "http://localhost:8080/auth/realms/my_realm", + } - # Imitate successful response from KeyCloak with user claims. - req_mock.get(USER_INFO_ENDPOINT, json=USER_CLAIMS) + mocked_resp = mock.Mock() + + mocked_resp.status_code = 200 + mocked_resp.json.return_value = KEYCLOAK_JSON + + mocked_get.return_value = mocked_resp req = self._build_request(token) @@ -160,13 +187,20 @@ class TestKeyCloakOIDCAuth(base.BaseTest): @requests_mock.Mocker() def test_server_unauthorized(self, req_mock): + self.override_config( + 'user_info_endpoint_url', + 'https://127.0.0.1:9080', + 'keycloak_oidc' + ) + token = { + "aud": "openstack", "iss": "http://localhost:8080/auth/realms/my_realm", } # Imitate failure response from KeyCloak. req_mock.get( - USER_INFO_ENDPOINT, + 'https://127.0.0.1:9080', status_code=401, reason='Access token is invalid', headers=WWW_AUTHENTICATE_HEADER @@ -186,15 +220,20 @@ class TestKeyCloakOIDCAuth(base.BaseTest): 'unauthorized reason is ...', e.response.headers.get('WWW-Authenticate') ) - else: raise Exception("Test is broken") - @requests_mock.Mocker() - def test_connection_error(self, req_mock): - token = {"iss": "http://localhost:8080/auth/realms/my_realm"} + @mock.patch("requests.get") + def test_connection_error(self, mocked_get): + token = { + "aud": "openstack", + "iss": "http://localhost:8080/auth/realms/my_realm", + "realm_access": { + "roles": ["role1", "role2"] + } + } - req_mock.get(USER_INFO_ENDPOINT, exc=requests.ConnectionError) + mocked_get.side_effect = requests.ConnectionError req = self._build_request(token) @@ -235,11 +274,15 @@ class TestKeyCloakOIDCAuthScenarios(base.DbTestCase): self.policy = self.useFixture(policy_fixtures.PolicyFixture()) - @requests_mock.Mocker() + @mock.patch("requests.get") @mock.patch.object(db_api, 'get_workflow_definition', MOCK_WF) - def test_get_workflow_success_auth(self, req_mock): - # Imitate successful response from KeyCloak with user claims. - req_mock.get(USER_INFO_ENDPOINT, json=USER_CLAIMS) + def test_get_workflow_success_auth(self, mocked_get): + mocked_resp = mock.Mock() + + mocked_resp.status_code = 200 + mocked_resp.json.return_value = KEYCLOAK_JSON + + mocked_get.return_value = mocked_resp token = { "iss": "http://localhost:8080/auth/realms/%s" % REALM_NAME, @@ -258,19 +301,13 @@ class TestKeyCloakOIDCAuthScenarios(base.DbTestCase): @mock.patch("requests.get") @mock.patch.object(db_api, 'get_workflow_definition', MOCK_WF) - def test_get_workflow_invalid_token_format(self, req_mock): - # Imitate successful response from KeyCloak with user claims. - req_mock.get(USER_INFO_ENDPOINT, json=USER_CLAIMS) - - token = { - "iss": "http://localhost:8080/auth/realms/%s" % REALM_NAME, - "realm_access": { - "roles": ["role1", "role2"] - } - } + def test_get_workflow_invalid_token_format(self, mocked_get): + token = 'WRONG_FORMAT_TOKEN' headers = {'X-Auth-Token': str(token)} + # We don't mock jwt.decode so the test must fail. + resp = self.app.get( '/v2/workflows/123', headers=headers, @@ -285,18 +322,18 @@ class TestKeyCloakOIDCAuthScenarios(base.DbTestCase): resp.text ) - @requests_mock.Mocker() + @mock.patch("requests.get") @mock.patch.object(db_api, 'get_workflow_definition', MOCK_WF) - def test_get_workflow_failed_auth(self, req_mock): - # Imitate failure response from KeyCloak. - req_mock.get( - USER_INFO_ENDPOINT, - status_code=401, - reason='Access token is invalid' - ) + def test_get_workflow_failed_auth(self, mocked_get): + mocked_resp = mock.Mock() + mocked_resp.status_code = 200 + mocked_resp.json.return_value = KEYCLOAK_JSON + + mocked_get.return_value = mocked_resp + + # A token without an issuer (iss). token = { - "iss": "http://localhost:8080/auth/realms/%s" % REALM_NAME, "realm_access": { "roles": ["role1", "role2"] } @@ -313,12 +350,11 @@ class TestKeyCloakOIDCAuthScenarios(base.DbTestCase): self.assertEqual(401, resp.status_code) self.assertEqual('401 Unauthorized', resp.status) - self.assertIn('Failed to validate access token', resp.text) - self.assertIn('Access token is invalid', resp.text) + self.assertIn("Failed to validate access token: 'iss'", resp.text) class TestKeyCloakOIDCAuthApp(base.DbTestCase): - """Test that Keycloak auth params were successfully passed to Context""" + """Test that Keycloak auth params get passed to the security context.""" def setUp(self): super(TestKeyCloakOIDCAuthApp, self).setUp() @@ -339,10 +375,15 @@ class TestKeyCloakOIDCAuthApp(base.DbTestCase): self.policy = self.useFixture(policy_fixtures.PolicyFixture()) - @requests_mock.Mocker() + @mock.patch("requests.get") @mock.patch.object(db_api, 'get_workflow_definition', MOCK_WF) - def test_params_transition(self, req_mock): - req_mock.get(USER_INFO_ENDPOINT, json=USER_CLAIMS) + def test_params_transition(self, mocked_get): + mocked_resp = mock.Mock() + + mocked_resp.status_code = 200 + mocked_resp.json.return_value = KEYCLOAK_JSON + + mocked_get.return_value = mocked_resp token = { "iss": "http://localhost:8080/auth/realms/%s" % REALM_NAME, @@ -351,22 +392,22 @@ class TestKeyCloakOIDCAuthApp(base.DbTestCase): } } - headers = { - 'X-Auth-Token': str(token) - } + headers = {'X-Auth-Token': str(token)} with mock.patch("jwt.decode", return_value=token): with mock.patch("mistral.context.set_ctx") as mocked_set_cxt: self.app.get('/v2/workflows/123', headers=headers) + calls = mocked_set_cxt.call_args_list + self.assertEqual(2, len(calls)) - # First positional argument of the first call ('before') + # First positional argument of the first call ('before'). ctx = calls[0][0][0] self.assertIsInstance(ctx, context.MistralContext) self.assertEqual('my_realm', ctx.project_id) self.assertEqual(["role1", "role2"], ctx.roles) - # Second call of set_ctx ('after'), where we reset the context + # Second call of set_ctx ('after'), where we reset the context. self.assertIsNone(calls[1][0][0]) diff --git a/requirements.txt b/requirements.txt index 22ff08163..7cf24d0be 100644 --- a/requirements.txt +++ b/requirements.txt @@ -56,7 +56,7 @@ python-ironic-inspector-client>=1.5.0 # Apache-2.0 python-vitrageclient>=2.0.0 # Apache-2.0 python-zunclient>=3.4.0 # Apache-2.0 python-qinlingclient>=1.0.0 # Apache-2.0 -PyJWT>=1.0.1 # MIT +PyJWT>=1.5 # MIT PyYAML>=3.12 # MIT requests>=2.14.2 # Apache-2.0 tenacity>=5.0.1 # Apache-2.0