From be54f177cd3f72356d35ad0ee85393229cd2c030 Mon Sep 17 00:00:00 2001 From: Benjamin Dauvergne Date: Fri, 14 Dec 2018 10:42:14 +0100 Subject: [PATCH 1/2] auth_oidc: verify and store id_token nonce (fixes #29009) --- src/authentic2/utils.py | 8 ++++---- src/authentic2_auth_oidc/backends.py | 9 +++++++-- src/authentic2_auth_oidc/views.py | 6 ++++-- tests/test_auth_oidc.py | 27 ++++++++++++++++++--------- 4 files changed, 33 insertions(+), 17 deletions(-) diff --git a/src/authentic2/utils.py b/src/authentic2/utils.py index d32a5a67..1ff7e89d 100644 --- a/src/authentic2/utils.py +++ b/src/authentic2/utils.py @@ -341,7 +341,7 @@ def get_nonce(request): return nonce -def record_authentication_event(request, how): +def record_authentication_event(request, how, nonce=None): '''Record an authentication event in the session and in the database, in later version the database persistence can be removed''' from . import models @@ -362,7 +362,7 @@ def record_authentication_event(request, how): 'who': unicode(request.user)[:80], 'how': how, } - nonce = get_nonce(request) + nonce = nonce or get_nonce(request) if nonce: kwargs['nonce'] = nonce event['nonce'] = nonce @@ -388,7 +388,7 @@ def last_authentication_event(session): return None -def login(request, user, how, service_slug=None, **kwargs): +def login(request, user, how, service_slug=None, nonce=None, **kwargs): '''Login a user model, record the authentication event and redirect to next URL or settings.LOGIN_REDIRECT_URL.''' from . import hooks @@ -400,7 +400,7 @@ def login(request, user, how, service_slug=None, **kwargs): if constants.LAST_LOGIN_SESSION_KEY not in request.session: request.session[constants.LAST_LOGIN_SESSION_KEY] = \ localize(to_current_timezone(last_login), True) - record_authentication_event(request, how) + record_authentication_event(request, how, nonce=nonce) hooks.call_hooks('event', name='login', user=user, how=how, service=service_slug) return continue_to_next_url(request, **kwargs) diff --git a/src/authentic2_auth_oidc/backends.py b/src/authentic2_auth_oidc/backends.py index 4db95dff..d03b959e 100644 --- a/src/authentic2_auth_oidc/backends.py +++ b/src/authentic2_auth_oidc/backends.py @@ -6,7 +6,6 @@ import requests from jwcrypto.jwt import JWT from jwcrypto.jwk import JWK -from django.core.exceptions import MultipleObjectsReturned from django.utils.timezone import now from django.contrib.auth import get_user_model from django.contrib.auth.backends import ModelBackend @@ -20,7 +19,7 @@ from . import models, utils class OIDCBackend(ModelBackend): - def authenticate(self, access_token=None, id_token=None, **kwargs): + def authenticate(self, access_token=None, id_token=None, nonce=None, **kwargs): logger = logging.getLogger(__name__) if id_token is None: return @@ -96,6 +95,12 @@ class OIDCBackend(ModelBackend): duration) return None + id_token_nonce = getattr(id_token, 'nonce', None) + if nonce and nonce != id_token_nonce: + logger.warning('auth_oidc: id_token nonce %r != expected nonce %r', + id_token_nonce, nonce) + return None + User = get_user_model() user = None if provider.strategy == models.OIDCProvider.STRATEGY_FIND_UUID: diff --git a/src/authentic2_auth_oidc/views.py b/src/authentic2_auth_oidc/views.py index ab384029..63441999 100644 --- a/src/authentic2_auth_oidc/views.py +++ b/src/authentic2_auth_oidc/views.py @@ -94,6 +94,8 @@ class LoginCallback(View): return redirect(request, settings.LOGIN_REDIRECT_URL) try: issuer = oidc_state.get('issuer') + oidc_request = oidc_state.get('request') + nonce = oidc_request.get('nonce') provider = get_provider_by_issuer(issuer) except models.OIDCProvider.DoesNotExist: messages.warning(request, _('Unknown OpenID connect issuer')) @@ -176,7 +178,7 @@ class LoginCallback(View): return self.continue_to_next_url() logger.info(u'got token response %s', result) access_token = result.get('access_token') - user = authenticate(access_token=access_token, id_token=result['id_token']) + user = authenticate(access_token=access_token, nonce=nonce, id_token=result['id_token']) if user: # remember last tokens for logout tokens = request.session.setdefault('auth_oidc', {}).setdefault('tokens', []) @@ -185,7 +187,7 @@ class LoginCallback(View): 'provider_pk': provider.pk, }) request.session.modified = True - login(request, user, 'oidc') + login(request, user, 'oidc', nonce=nonce) else: messages.warning(request, _('No user found')) return self.continue_to_next_url() diff --git a/tests/test_auth_oidc.py b/tests/test_auth_oidc.py index cecabbae..4dea423d 100644 --- a/tests/test_auth_oidc.py +++ b/tests/test_auth_oidc.py @@ -21,7 +21,7 @@ from authentic2_auth_oidc.utils import (base64url_decode, parse_id_token, IDToke has_providers) from authentic2_auth_oidc.models import OIDCProvider, OIDCClaimMapping from authentic2.models import AttributeValue -from authentic2.utils import timestamp_from_datetime +from authentic2.utils import timestamp_from_datetime, last_authentication_event from authentic2.a2_rbac.utils import get_default_ou from authentic2.crypto import base64url_encode @@ -166,7 +166,7 @@ def code(): def oidc_provider_mock(oidc_provider, oidc_provider_jwkset, code, extra_id_token=None, - extra_user_info=None, sub='john.doe'): + extra_user_info=None, sub='john.doe', nonce=None): token_endpoint = urlparse.urlparse(oidc_provider.token_endpoint) userinfo_endpoint = urlparse.urlparse(oidc_provider.userinfo_endpoint) token_revocation_endpoint = urlparse.urlparse(oidc_provider.token_revocation_endpoint) @@ -181,6 +181,8 @@ def oidc_provider_mock(oidc_provider, oidc_provider_jwkset, code, extra_id_token 'aud': str(oidc_provider.client_id), 'exp': timestamp_from_datetime(now() + datetime.timedelta(seconds=10)), } + if nonce: + id_token['nonce'] = nonce if extra_id_token: id_token.update(extra_id_token) @@ -277,6 +279,8 @@ def test_sso(app, caplog, code, oidc_provider, oidc_provider_jwkset, login_url, assert query['client_id'] == str(oidc_provider.client_id) assert query['scope'] == 'openid' assert query['redirect_uri'] == 'http://testserver' + reverse('oidc-login-callback') + # get the nonce + nonce = app.session['auth_oidc'][query['state']]['request']['nonce'] if oidc_provider.claims_parameter_supported: claims = json.loads(query['claims']) @@ -312,9 +316,12 @@ def test_sso(app, caplog, code, oidc_provider, oidc_provider_jwkset, login_url, with oidc_provider_mock(oidc_provider, oidc_provider_jwkset, code, extra_id_token={'aud': 'zz'}): response = app.get(login_callback_url, params={'code': code, 'state': query['state']}) + with utils.check_log(caplog, 'expected nonce'): + with oidc_provider_mock(oidc_provider, oidc_provider_jwkset, code): + response = app.get(login_callback_url, params={'code': code, 'state': query['state']}) assert not hooks.auth_oidc_backend_modify_user with utils.check_log(caplog, 'created user'): - with oidc_provider_mock(oidc_provider, oidc_provider_jwkset, code): + with oidc_provider_mock(oidc_provider, oidc_provider_jwkset, code, nonce=nonce): response = app.get(login_callback_url, params={'code': code, 'state': query['state']}) assert len(hooks.auth_oidc_backend_modify_user) == 1 assert set(hooks.auth_oidc_backend_modify_user[0]['kwargs']) >= set(['user', 'provider', 'user_info', 'id_token', 'access_token']) @@ -330,21 +337,22 @@ def test_sso(app, caplog, code, oidc_provider, oidc_provider_jwkset, login_url, assert user.attributes.last_name == 'Doe' assert AttributeValue.objects.filter(content='John', verified=True).count() == 1 assert AttributeValue.objects.filter(content='Doe', verified=False).count() == 1 + assert last_authentication_event(app.session)['nonce'] == nonce with oidc_provider_mock(oidc_provider, oidc_provider_jwkset, code, - extra_user_info={'family_name_verified': True}): + extra_user_info={'family_name_verified': True}, nonce=nonce): response = app.get(login_callback_url, params={'code': code, 'state': query['state']}) assert AttributeValue.objects.filter(content='Doe', verified=False).count() == 0 assert AttributeValue.objects.filter(content='Doe', verified=True).count() == 1 with oidc_provider_mock(oidc_provider, oidc_provider_jwkset, code, - extra_user_info={'ou': 'cassis'}): + extra_user_info={'ou': 'cassis'}, nonce=nonce): response = app.get(login_callback_url, params={'code': code, 'state': query['state']}) assert User.objects.count() == 1 user = User.objects.get() assert user.ou == cassis - with oidc_provider_mock(oidc_provider, oidc_provider_jwkset, code): + with oidc_provider_mock(oidc_provider, oidc_provider_jwkset, code, nonce=nonce): response = app.get(login_callback_url, params={'code': code, 'state': query['state']}) assert User.objects.count() == 1 user = User.objects.get() @@ -403,15 +411,16 @@ def test_strategy_find_uuid(app, caplog, code, oidc_provider, oidc_provider_jwks response = response.click(oidc_provider.name) location = urlparse.urlparse(response.location) query = check_simple_qs(urlparse.parse_qs(location.query)) + nonce = app.session['auth_oidc'][query['state']]['request']['nonce'] # sub=john.doe, MUST not work with utils.check_log(caplog, 'cannot create user'): - with oidc_provider_mock(oidc_provider, oidc_provider_jwkset, code): + with oidc_provider_mock(oidc_provider, oidc_provider_jwkset, code, nonce=nonce): response = app.get(login_callback_url, params={'code': code, 'state': query['state']}) # sub=simple_user.uuid MUST work with utils.check_log(caplog, 'found user using UUID'): - with oidc_provider_mock(oidc_provider, oidc_provider_jwkset, code, sub=simple_user.uuid): + with oidc_provider_mock(oidc_provider, oidc_provider_jwkset, code, sub=simple_user.uuid, nonce=nonce): response = app.get(login_callback_url, params={'code': code, 'state': query['state']}) assert urlparse.urlparse(response['Location']).path == '/' @@ -427,6 +436,6 @@ def test_strategy_find_uuid(app, caplog, code, oidc_provider, oidc_provider_jwks response = app.get(reverse('account_management')) with utils.check_log(caplog, 'revoked token from OIDC'): - with oidc_provider_mock(oidc_provider, oidc_provider_jwkset, code): + with oidc_provider_mock(oidc_provider, oidc_provider_jwkset, code, nonce=nonce): response = response.click(href='logout') assert 'https://idp.example.com/logout' in response.content -- 2.18.0