From c97be0a9ee778f25e2931aeb1f678c145c2f1d2b Mon Sep 17 00:00:00 2001 From: Brant Knudson Date: Wed, 10 Dec 2014 09:46:44 -0600 Subject: [PATCH] Fix auth_token does version request for no token When a request came in with no token and the auth_token middleware wasn't configured with an auth version, a request would be made to fetch the versions supported by the server. This causes problems in projects that have unit tests using the auth_token middleware because they didn't expect to have an Identity server running. The fix for this is to refactor the identity version handling to a strategy pattern. There were 2 subclasses of _IdentityServer in auth_token, one for V2 and one for V3. This is excessive since there's only a couple of methods that are needed and not all the _IdentityServer class. Using the pricipal of preferring composition over inheritance, the code to handle V2/V3 is moved into a simpler strategy pattern. This also moves code out of the AuthProtocol class which is too complicated already, and it allows more refactoring since the _IdentityServer reference can be created and passed to extracted classes for them to use. Closes-Bug: #1404294 Change-Id: If69fbb73bea268b96e4b1e9ad81a736495a2b58a --- keystonemiddleware/auth_token.py | 213 ++++++++++-------- .../tests/test_auth_token_middleware.py | 18 +- 2 files changed, 129 insertions(+), 102 deletions(-) diff --git a/keystonemiddleware/auth_token.py b/keystonemiddleware/auth_token.py index 5d407c8c..caf922f4 100644 --- a/keystonemiddleware/auth_token.py +++ b/keystonemiddleware/auth_token.py @@ -709,7 +709,7 @@ class AuthProtocol(object): self._include_service_catalog = self._conf_get( 'include_service_catalog') - self._identity_server_obj = None + self._identity_server = self._create_identity_server() # signing self._signing_dirname = self._conf_get('signing_dir') @@ -814,11 +814,6 @@ class AuthProtocol(object): _LI('Invalid service token - rejecting request')) return self._reject_request(env, start_response) - except exceptions.NoMatchingPlugin as e: - msg = _LC('Required auth plugin does not exist. %s') % e - self._LOG.critical(msg) - return self._do_503_error(env, start_response) - except ServiceError as e: self._LOG.critical(_LC('Unable to obtain admin token: %s'), e) return self._do_503_error(env, start_response) @@ -970,6 +965,8 @@ class AuthProtocol(object): self._LOG.debug('Token validation failure.', exc_info=True) self._LOG.warn(_LW('Authorization failed for token')) raise InvalidToken(_('Token authorization failed')) + except ServiceError: + raise except Exception: self._LOG.debug('Token validation failure.', exc_info=True) if token_id: @@ -1306,86 +1303,57 @@ class AuthProtocol(object): self._signing_ca_file_name, self._identity_server.fetch_ca_cert()) - @property - def _identity_server(self): - if not self._identity_server_obj: - # NOTE(jamielennox): Loading Session here should be exactly the - # same as calling Session.load_from_conf_options(CONF, GROUP) - # however we can't do that because we have to use _conf_get to - # support the paste.ini options. - sess = session.Session.construct(dict( - cert=self._conf_get('certfile'), - key=self._conf_get('keyfile'), - cacert=self._conf_get('cafile'), - insecure=self._conf_get('insecure'), - timeout=self._conf_get('http_connect_timeout') - )) + def _create_identity_server(self): + # NOTE(jamielennox): Loading Session here should be exactly the + # same as calling Session.load_from_conf_options(CONF, GROUP) + # however we can't do that because we have to use _conf_get to + # support the paste.ini options. + sess = session.Session.construct(dict( + cert=self._conf_get('certfile'), + key=self._conf_get('keyfile'), + cacert=self._conf_get('cafile'), + insecure=self._conf_get('insecure'), + timeout=self._conf_get('http_connect_timeout') + )) - # NOTE(jamielennox): The original auth mechanism allowed deployers - # to configure authentication information via paste file. These - # are accessible via _conf_get, however this doesn't work with the - # plugin loading mechanisms. For using auth plugins we only support - # configuring via the CONF file. - auth_plugin = auth.load_from_conf_options(CONF, _AUTHTOKEN_GROUP) + # NOTE(jamielennox): The original auth mechanism allowed deployers + # to configure authentication information via paste file. These + # are accessible via _conf_get, however this doesn't work with the + # plugin loading mechanisms. For using auth plugins we only support + # configuring via the CONF file. + auth_plugin = auth.load_from_conf_options(CONF, _AUTHTOKEN_GROUP) - if not auth_plugin: - # NOTE(jamielennox): Loading AuthTokenPlugin here should be - # exactly the same as calling - # _AuthTokenPlugin.load_from_conf_options(CONF, GROUP) however - # we can't do that because we have to use _conf_get to support - # the paste.ini options. - auth_plugin = _AuthTokenPlugin.load_from_options( - auth_host=self._conf_get('auth_host'), - auth_port=int(self._conf_get('auth_port')), - auth_protocol=self._conf_get('auth_protocol'), - auth_admin_prefix=self._conf_get('auth_admin_prefix'), - admin_user=self._conf_get('admin_user'), - admin_password=self._conf_get('admin_password'), - admin_tenant_name=self._conf_get('admin_tenant_name'), - admin_token=self._conf_get('admin_token'), - identity_uri=self._conf_get('identity_uri'), - log=self._LOG) + if not auth_plugin: + # NOTE(jamielennox): Loading AuthTokenPlugin here should be + # exactly the same as calling + # _AuthTokenPlugin.load_from_conf_options(CONF, GROUP) however + # we can't do that because we have to use _conf_get to support + # the paste.ini options. + auth_plugin = _AuthTokenPlugin.load_from_options( + auth_host=self._conf_get('auth_host'), + auth_port=int(self._conf_get('auth_port')), + auth_protocol=self._conf_get('auth_protocol'), + auth_admin_prefix=self._conf_get('auth_admin_prefix'), + admin_user=self._conf_get('admin_user'), + admin_password=self._conf_get('admin_password'), + admin_tenant_name=self._conf_get('admin_tenant_name'), + admin_token=self._conf_get('admin_token'), + identity_uri=self._conf_get('identity_uri'), + log=self._LOG) - adap = adapter.Adapter( - sess, - auth=auth_plugin, - service_type='identity', - interface='admin', - connect_retries=self._conf_get('http_request_max_retries')) + adap = adapter.Adapter( + sess, + auth=auth_plugin, + service_type='identity', + interface='admin', + connect_retries=self._conf_get('http_request_max_retries')) - server_class = self._get_server_class(adap) - adap.version = server_class.AUTH_VERSION - - self._identity_server_obj = server_class( - self._LOG, - adap, - include_service_catalog=self._include_service_catalog, - auth_uri=self._conf_get('auth_uri')) - - return self._identity_server_obj - - def _get_server_class(self, adap): - # FIXME(jamielennox): Checking string equality is bad, but consistent - # with the existing code. Fix this to better handle selecting v3. - auth_version = self._conf_get('auth_version') - - if auth_version == 'v3.0': - return _V3IdentityServer - elif auth_version: - return _V2IdentityServer - - for klass in _VERSIONS_TO_ATTEMPT: - if adap.get_endpoint(version=klass.AUTH_VERSION): - msg = _LI('Auth Token confirmed use of %s apis') - self._LOG.info(msg, auth_version) - return klass - - versions = ['v%d.%d' % s.AUTH_VERSION for s in _VERSIONS_TO_ATTEMPT] - self._LOG.error(_LE('No attempted versions [%s] supported by server') % - ', '.join(versions)) - - msg = _('No compatible apis supported by server') - raise ServiceError(msg) + return _IdentityServer( + self._LOG, + adap, + include_service_catalog=self._include_service_catalog, + auth_uri=self._conf_get('auth_uri'), + requested_auth_version=self._conf_get('auth_version')) def _token_cache_factory(self): security_strategy = self._conf_get('memcache_security_strategy') @@ -1483,10 +1451,12 @@ class _IdentityServer(object): AUTH_VERSION = None - def __init__(self, log, adap, include_service_catalog=None, auth_uri=None): + def __init__(self, log, adap, include_service_catalog=None, auth_uri=None, + requested_auth_version=None): self._LOG = log self._adapter = adap self._include_service_catalog = include_service_catalog + self._requested_auth_version = requested_auth_version if auth_uri is None: self._LOG.warning( @@ -1506,6 +1476,43 @@ class _IdentityServer(object): self.auth_uri = auth_uri + # Built on-demand with self._request_strategy. + self._request_strategy_obj = None + + @property + def _request_strategy(self): + if not self._request_strategy_obj: + strategy_class = self._get_strategy_class() + self._adapter.version = strategy_class.AUTH_VERSION + + self._request_strategy_obj = strategy_class( + self._json_request, + self._adapter, + include_service_catalog=self._include_service_catalog) + + return self._request_strategy_obj + + def _get_strategy_class(self): + # FIXME(jamielennox): Checking string equality is bad, but consistent + # with the existing code. Fix this to better handle selecting v3. + if self._requested_auth_version == 'v3.0': + return _V3RequestStrategy + elif self._requested_auth_version: + return _V2RequestStrategy + + for klass in _REQUEST_STRATEGIES: + if self._adapter.get_endpoint(version=klass.AUTH_VERSION): + msg = _LI('Auth Token confirmed use of %s apis') + self._LOG.info(msg, self._requested_auth_version) + return klass + + versions = ['v%d.%d' % s.AUTH_VERSION for s in _REQUEST_STRATEGIES] + self._LOG.error(_LE('No attempted versions [%s] supported by server') % + ', '.join(versions)) + + msg = _('No compatible apis supported by server') + raise ServiceError(msg) + def verify_token(self, user_token, retry=True): """Authenticate user token with identity server. @@ -1521,7 +1528,7 @@ class _IdentityServer(object): user_token = _safe_quote(user_token) try: - response, data = self._do_verify_token(user_token) + response, data = self._request_strategy.verify_token(user_token) except exceptions.NotFound as e: self._LOG.warn(_LW('Authorization failed for token')) self._LOG.warn(_LW('Identity response: %s') % e.response.text) @@ -1588,7 +1595,7 @@ class _IdentityServer(object): def _fetch_cert_file(self, cert_type): try: - response = self._do_fetch_cert_file(cert_type) + response = self._request_strategy.fetch_cert_file(cert_type) except exceptions.HTTPError as e: raise exceptions.CertificateConfigError(e.details) if response.status_code != 200: @@ -1596,25 +1603,41 @@ class _IdentityServer(object): return response.text -class _V2IdentityServer(_IdentityServer): +class _RequestStrategy(object): + + AUTH_VERSION = None + + def __init__(self, json_request, adap, include_service_catalog=None): + self._json_request = json_request + self._adapter = adap + self._include_service_catalog = include_service_catalog + + def verify_token(self, user_token): + pass + + def fetch_cert_file(self, cert_type): + pass + + +class _V2RequestStrategy(_RequestStrategy): AUTH_VERSION = (2, 0) - def _do_verify_token(self, user_token): + def verify_token(self, user_token): return self._json_request('GET', '/tokens/%s' % user_token, authenticated=True) - def _do_fetch_cert_file(self, cert_type): + def fetch_cert_file(self, cert_type): return self._adapter.get('/certificates/%s' % cert_type, authenticated=False) -class _V3IdentityServer(_IdentityServer): +class _V3RequestStrategy(_RequestStrategy): AUTH_VERSION = (3, 0) - def _do_verify_token(self, user_token): + def verify_token(self, user_token): path = '/auth/tokens' if not self._include_service_catalog: path += '?nocatalog' @@ -1624,7 +1647,7 @@ class _V3IdentityServer(_IdentityServer): authenticated=True, headers={'X-Subject-Token': user_token}) - def _do_fetch_cert_file(self, cert_type): + def fetch_cert_file(self, cert_type): if cert_type == 'signing': cert_type = 'certificates' @@ -1632,6 +1655,10 @@ class _V3IdentityServer(_IdentityServer): authenticated=False) +# NOTE(jamielennox): must be defined after request strategy classes +_REQUEST_STRATEGIES = [_V3RequestStrategy, _V2RequestStrategy] + + class _TokenCache(object): """Encapsulates the auth_token token cache functionality. @@ -1937,10 +1964,6 @@ def app_factory(global_conf, **local_conf): return AuthProtocol(None, conf) -# NOTE(jamielennox): must be defined after identity server classes -_VERSIONS_TO_ATTEMPT = [_V3IdentityServer, _V2IdentityServer] - - if __name__ == '__main__': def echo_app(environ, start_response): """A WSGI application that echoes the CGI environment to the user.""" diff --git a/keystonemiddleware/tests/test_auth_token_middleware.py b/keystonemiddleware/tests/test_auth_token_middleware.py index f35af8a1..05062025 100644 --- a/keystonemiddleware/tests/test_auth_token_middleware.py +++ b/keystonemiddleware/tests/test_auth_token_middleware.py @@ -628,6 +628,13 @@ class CommonAuthTokenMiddlewareTest(object): self.set_middleware(conf=conf) self.assertLastPath(None) + def test_auth_with_no_token_does_not_call_http(self): + self.set_middleware() + req = webob.Request.blank('/') + self.middleware(req.environ, self.start_fake_response) + self.assertLastPath(None) + self.assertEqual(401, self.response_status) + def test_init_by_ipv6Addr_auth_host(self): del self.conf['identity_uri'] conf = { @@ -2544,16 +2551,13 @@ class AuthProtocolLoadingTests(BaseAuthTokenMiddlewareTest): self.assertEqual(200, self.response_status) self.assertEqual(six.b(body), resp[0]) - def test_invalid_plugin_503(self): + def test_invalid_plugin_fails_to_intialize(self): self.cfg.config(auth_plugin=uuid.uuid4().hex, group=auth_token._AUTHTOKEN_GROUP) - app = auth_token.AuthProtocol(new_app('200 OK', '')(), {}) - req = webob.Request.blank('/') - req.headers['X-Auth-Token'] = uuid.uuid4().hex - app(req.environ, self.start_fake_response) - - self.assertEqual(503, self.response_status) + self.assertRaises( + exceptions.NoMatchingPlugin, + lambda: auth_token.AuthProtocol(new_app('200 OK', '')(), {})) def load_tests(loader, tests, pattern):