From f651d2cc84dc2c67782acf8081a425ec6a84b4af Mon Sep 17 00:00:00 2001 From: Slavek Kabrda Date: Jan 07 2019 15:02:11 +0000 Subject: Fix race condition in OpenIDConnect api Cherrypy seems to reuse objects if running in a multithreaded environment (e.g. mod_wsgi on httpd). This can result in multiple threads resetting attributes of OpenIDConnect api objects which then leads to various 500 errors. A fresh cherrypy.request object is generated for every request, therefore it can be used to store these attributes safely. Related: #302 Signed-off-by: Slavek Kabrda Reviewed-by: Michal Konečný Reviewed-by: Patrick Uiterwijk --- diff --git a/ipsilon/providers/openidc/api.py b/ipsilon/providers/openidc/api.py index c1cb07d..f665256 100644 --- a/ipsilon/providers/openidc/api.py +++ b/ipsilon/providers/openidc/api.py @@ -40,15 +40,25 @@ class APIRequest(ProviderPageBase): def __init__(self, site, provider, *args, **kwargs): super(APIRequest, self).__init__(site, provider) - self.api_client_id = None - self.api_valid_token = False - self.api_username = None - self.api_scopes = [] - self.api_client_authenticated = False - self.api_client_authenticated_no_secret = False - self.api_client_id = None - self.api_token = None - self.api_client = None + self._set_apistore_key('api_client_id', None) + self._set_apistore_key('api_valid_token', False) + self._set_apistore_key('api_username', None) + self._set_apistore_key('api_scopes', []) + self._set_apistore_key('api_client_authenticated', False) + self._set_apistore_key('api_client_authenticated_no_secret', False) + self._set_apistore_key('api_client_id', None) + self._set_apistore_key('api_token', None) + self._set_apistore_key('api_client', None) + + def _get_apistore_key(self, key): + return cherrypy.request.apistore[self][key] + + def _set_apistore_key(self, key, value): + if not hasattr(cherrypy.request, 'apistore'): + cherrypy.request.apistore = {} + if self not in cherrypy.request.apistore: + cherrypy.request.apistore[self] = {} + cherrypy.request.apistore[self][key] = value def pre_GET(self, *args, **kwargs): # Note that we explicitly do NOT support URI Query parameter posting @@ -63,18 +73,18 @@ class APIRequest(ProviderPageBase): 'Content-Type': 'application/json' }) - self.api_client_id = None - self.api_valid_token = False - self.api_username = None - self.api_scopes = [] - self.api_client_authenticated = False - self.api_client_id = None - self.api_token = None - self.api_client = None + self._set_apistore_key('api_client_id', None) + self._set_apistore_key('api_valid_token', False) + self._set_apistore_key('api_username', None) + self._set_apistore_key('api_scopes', []) + self._set_apistore_key('api_client_authenticated', False) + self._set_apistore_key('api_client_id', None) + self._set_apistore_key('api_token', None) + self._set_apistore_key('api_client', None) if self.authenticate_client: self._authenticate_client(kwargs) - if self.requires_client_auth and not self.api_client_id: + if self.requires_client_auth and not self._get_apistore_key('api_client_id'): raise APIError(400, 'invalid_client', 'client authentication required') @@ -116,7 +126,7 @@ class APIRequest(ProviderPageBase): 'client authentication error') if auth_method == 'none': - self.api_client_authenticated_no_secret = True + self._set_apistore_key('api_client_authenticated_no_secret', True) elif not constant_time_string_comparison(client['client_secret'], client_secret): @@ -125,9 +135,9 @@ class APIRequest(ProviderPageBase): raise APIError(400, 'invalid_client', 'client authentication error') - self.api_client_authenticated = True - self.api_client_id = client_id - self.api_client = client + self._set_apistore_key('api_client_authenticated', True) + self._set_apistore_key('api_client_id', client_id) + self._set_apistore_key('api_client', client) def _authenticate_client(self, post_args): request = cherrypy.serving.request @@ -180,14 +190,17 @@ class APIRequest(ProviderPageBase): self.error('Unknown token provided') raise APIError(400, 'invalid_token') - if self.api_client_id: + if self._get_apistore_key('api_client_id'): if token['client_id'] != self.api_client_id: self.error('Authenticated client is not token owner: %s != %s' - % (token['client_id'], self.api_client_id)) + % (token['client_id'], self._get_apistore_key('api_client_id'))) raise APIError(400, 'invalid_request') else: - self.api_client = self.cfg.datastore.getClient(token['client_id']) - if not self.api_client: + self._set_apistore_key( + 'api_client', + self.cfg.datastore.getClient(token['client_id']) + ) + if not self._get_apistore_key('api_client'): self.error('Token authentication with invalid client ID') raise APIError(400, 'invalid_client', 'client authentication error') @@ -198,11 +211,11 @@ class APIRequest(ProviderPageBase): % (self.required_scope, token['scope'])) raise APIError(403, 'insufficient_scope') - self.api_client_id = token['client_id'] - self.api_valid_token = True - self.api_username = token['username'] - self.api_scopes = token['scope'] - self.api_token = token + self._set_apistore_key('api_client_id', token['client_id']) + self._set_apistore_key('api_valid_token', True) + self._set_apistore_key('api_username', token['username']) + self._set_apistore_key('api_scopes', token['scope']) + self._set_apistore_key('api_token', token) def _authenticate_token(self, post_args): request = cherrypy.serving.request @@ -217,12 +230,12 @@ class APIRequest(ProviderPageBase): # Bearer token token = post_args['access_token'] self._handle_token_authentication(token) - if self.requires_valid_token and not self.api_token: + if self.requires_valid_token and not self._get_apistore_key('api_token'): self.error('No token provided in call that requires one') raise APIError(403, 'no_token_provided') def require_scope(self, scope): - if scope not in self.api_scopes: + if scope not in self._get_apistore_key('api_scopes'): raise APIError(403, 'insufficient_scope') @@ -244,10 +257,10 @@ class Token(APIRequest): return self._respond_error('invalid_request', 'invalid token') - if token['client_id'] != self.api_client_id: + if token['client_id'] != self._get_apistore_key('api_client_id'): self.error('Authz code owner does not match authenticated ' + 'client: %s != %s' % (token['client_id'], - self.api_client_id)) + self._get_apistore_key('api_client_id'))) return self._respond_error('invalid_request', 'invalid token for client ID') @@ -272,7 +285,7 @@ class Token(APIRequest): refresh_result = self.cfg.datastore.refreshToken( refresh_token, - self.api_client_id) + self._get_apistore_key('api_client_id')) if not refresh_result: return self._respond_error('invalid_grant', @@ -328,17 +341,17 @@ class UserInfo(APIRequest): required_scope = 'openid' def _get_userinfo(self, *args, **kwargs): - info = self.cfg.datastore.getUserInfo(self.api_token['userinfocode']) + info = self.cfg.datastore.getUserInfo(self._get_apistore_key('api_token')['userinfocode']) if not info: return self._respond_error('invalid_request', 'No userinfo for token') - if self.api_client.get('userinfo_signed_response_alg'): + if self._get_apistore_key('api_client').get('userinfo_signed_response_alg'): cherrypy.response.headers.update({ 'Content-Type': 'application/jwt' }) - if self.api_client.get('userinfo_signed_response_alg') == 'RS256': + if self._get_apistore_key('api_client').get('userinfo_signed_response_alg') == 'RS256': sig = JWT(header={'alg': 'RS256', 'kid': self.cfg.idp_sig_key_id}, claims=info)