Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix google OpenID Connect #747

Closed
wants to merge 19 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions requirements-python3.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,4 @@ oauthlib>=0.3.8
requests-oauthlib>0.3.2
six>=1.2.0
PyJWT>=1.0.0
pyjwkest==1.0.1
1 change: 1 addition & 0 deletions requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,4 @@ oauthlib>=0.3.8
requests-oauthlib>=0.3.1
six>=1.2.0
PyJWT>=1.0.0
pyjwkest==1.0.1
5 changes: 4 additions & 1 deletion social/backends/google.py
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,10 @@ def get_user_id(self, details, response):

class GoogleOpenIdConnect(GoogleOAuth2, OpenIdConnectAuth):
name = 'google-openidconnect'
ID_TOKEN_ISSUER = "accounts.google.com"
OIDC_ENDPOINT = 'https://accounts.google.com'
# differs from value in discovery document
# http://openid.net/specs/openid-connect-core-1_0.html#rfc.section.15.6.2
ID_TOKEN_ISSUER = 'accounts.google.com'

def user_data(self, access_token, *args, **kwargs):
"""Return user data from Google API"""
Expand Down
163 changes: 133 additions & 30 deletions social/backends/open_id.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
import datetime
import six
import time
from calendar import timegm

from jwt import InvalidTokenError, decode as jwt_decode
from jwkest import JWKESTException
from jwkest.jwk import KEYS
from jwkest.jws import JWS

from openid.consumer.consumer import Consumer, SUCCESS, CANCEL, FAILURE
from openid.consumer.discover import DiscoveryFailure
Expand Down Expand Up @@ -271,17 +275,87 @@ def __init__(self, handle, secret='', issued=0, lifetime=0, assoc_type=''):
self.assoc_type = assoc_type # as state


class _cache(object):
"""
Cache decorator that caches the return value of a method for a
specified time.

It maintains a cache per class, so subclasses have a different cache entry
for the same cached method.

Does not work for methods with arguments.
"""
def __init__(self, ttl):
self.ttl = ttl
self.cache = {}

def __call__(self, fn):
def wrapped(this):
now = time.time()
last_updated = None
cached_value = None
if this.__class__ in self.cache:
last_updated, cached_value = self.cache[this.__class__]
if not cached_value or now - last_updated > self.ttl:
try:
cached_value = fn(this)
self.cache[this.__class__] = (now, cached_value)
except:
# Use previously cached value when call fails, if available
if not cached_value:
raise
return cached_value
return wrapped


def _autoconf(name):
"""
fget helper function to fetch the value of a property from the OIDC
configuration
"""
def getter(self):
return self.oidc_config().get(name)
return getter


class OpenIdConnectAuth(BaseOAuth2):
"""
Base class for Open ID Connect backends.

Currently only the code response type is supported.
"""
ID_TOKEN_ISSUER = None
DEFAULT_SCOPE = ['openid']
# Override OIDC_ENDPOINT in your subclass to enable autoconfig of OIDC
OIDC_ENDPOINT = None

DEFAULT_SCOPE = ['openid', 'profile', 'email']
EXTRA_DATA = ['id_token', 'refresh_token', ('sub', 'id')]
# Set after access_token is retrieved
id_token = None
REDIRECT_STATE = False
ACCESS_TOKEN_METHOD = 'POST'
REVOKE_TOKEN_METHOD = 'GET'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a USERNAME_KEY = 'preferred_username'

ID_KEY = 'sub'
USERNAME_KEY = 'preferred_username'

@_cache(ttl=86400)
def oidc_config(self):
return self.get_json(self.OIDC_ENDPOINT +
'/.well-known/openid-configuration')

ID_TOKEN_ISSUER = property(_autoconf('issuer'))
ACCESS_TOKEN_URL = property(_autoconf('token_endpoint'))
AUTHORIZATION_URL = property(_autoconf('authorization_endpoint'))
REVOKE_TOKEN_URL = property(_autoconf('revocation_endpoint'))
USERINFO_URL = property(_autoconf('userinfo_endpoint'))
JWKS_URI = property(_autoconf('jwks_uri'))

@_cache(ttl=86400)
def get_jwks_keys(self):
keys = KEYS()
keys.load_from_url(self.JWKS_URI)

# Add client secret as oct key so it can be used for HMAC signatures
_client_id, client_secret = self.get_key_and_secret()
keys.add({'key': client_secret, 'kty': 'oct'})
return keys

def auth_params(self, state=None):
"""Return extra arguments needed on auth process."""
Expand All @@ -291,14 +365,6 @@ def auth_params(self, state=None):
)
return params

def auth_complete_params(self, state=None):
params = super(OpenIdConnectAuth, self).auth_complete_params(state)
# Add a nonce to the request so that to help counter CSRF
params['nonce'] = self.get_and_store_nonce(
self.ACCESS_TOKEN_URL, state
)
return params

def get_and_store_nonce(self, url, state):
# Create a nonce
nonce = self.strategy.random_string(64)
Expand All @@ -310,7 +376,7 @@ def get_and_store_nonce(self, url, state):
def get_nonce(self, nonce):
try:
return self.strategy.storage.association.get(
server_url=self.ACCESS_TOKEN_URL,
server_url=self.AUTHORIZATION_URL,
handle=nonce
)[0]
except IndexError:
Expand All @@ -319,25 +385,31 @@ def get_nonce(self, nonce):
def remove_nonce(self, nonce_id):
self.strategy.storage.association.remove([nonce_id])

def validate_and_return_id_token(self, id_token):
"""
Validates the id_token according to the steps at
http://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation.
"""
def validate_claims(self, id_token):
if id_token['iss'] != self.ID_TOKEN_ISSUER:
raise AuthTokenError(self, 'Token error: Invalid issuer')

client_id, _client_secret = self.get_key_and_secret()
decryption_key = self.setting('ID_TOKEN_DECRYPTION_KEY')
try:
# Decode the JWT and raise an error if the secret is invalid or
# the response has expired.
id_token = jwt_decode(id_token, decryption_key, audience=client_id,
issuer=self.ID_TOKEN_ISSUER,
algorithms=['HS256'])
except InvalidTokenError as err:
raise AuthTokenError(self, err)
if isinstance(id_token['aud'], six.string_types):
id_token['aud'] = [id_token['aud']]
if client_id not in id_token['aud']:
raise AuthTokenError(self, 'Token error: Invalid audience')

if len(id_token['aud']) > 1 and 'azp' not in id_token:
raise AuthTokenError(self, 'Incorrect id_token: azp')

if 'azp' in id_token and id_token['azp'] != client_id:
raise AuthTokenError(self, 'Incorrect id_token: azp')

# Verify the token was issued in the last 10 minutes
utc_timestamp = timegm(datetime.datetime.utcnow().utctimetuple())
if id_token['iat'] < (utc_timestamp - 600):
if utc_timestamp > id_token['exp']:
raise AuthTokenError(self, 'Token error: Signature has expired')

if 'nbf' in id_token and utc_timestamp < id_token['nbf']:
raise AuthTokenError(self, 'Incorrect id_token: nbf')

# Verify the token was issued in the last 10 minutes
if utc_timestamp > id_token['iat'] + 600:
raise AuthTokenError(self, 'Incorrect id_token: iat')

# Validate the nonce to ensure the request was not modified
Expand All @@ -350,6 +422,22 @@ def validate_and_return_id_token(self, id_token):
self.remove_nonce(nonce_obj.id)
else:
raise AuthTokenError(self, 'Incorrect id_token: nonce')

def validate_and_return_id_token(self, jws):
"""
Validates the id_token according to the steps at
http://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation.
"""
try:
# Decode the JWT and raise an error if the sig is invalid
id_token = JWS().verify_compact(jws.encode('utf-8'),
self.get_jwks_keys())
except JWKESTException:
raise AuthTokenError(self,
'Token error: Signature verification failed')

self.validate_claims(id_token)

return id_token

def request_access_token(self, *args, **kwargs):
Expand All @@ -360,3 +448,18 @@ def request_access_token(self, *args, **kwargs):
response = self.get_json(*args, **kwargs)
self.id_token = self.validate_and_return_id_token(response['id_token'])
return response

def user_data(self, access_token, *args, **kwargs):
return self.get_json(self.USERINFO_URL,
headers={'Authorization':
'Bearer {0}'.format(access_token)})

def get_user_details(self, response):
username_key = self.setting('USERNAME_KEY', default=self.USERNAME_KEY)
return {
'username': response.get(username_key),
'email': response.get('email'),
'fullname': response.get('name'),
'first_name': response.get('given_name'),
'last_name': response.get('family_name'),
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be closer to

        values = {'username': '', 'email': '', 'fullname': '',
                  'first_name': '', 'last_name': ''}

        fullname = values.get('name') or ''
        first_name = values.get('given_name') or ''
        last_name = values.get('family_name') or ''
        email = values.get('email') or ''

        if not fullname and first_name and last_name:
            fullname = first_name + ' ' + last_name
        elif fullname:
            try:
                first_name, last_name = fullname.rsplit(' ', 1)
            except ValueError:
                last_name = fullname

        username_key = self.setting('USERNAME_KEY') or self.USERNAME_KEY
        values.update({'fullname': fullname, 'first_name': first_name,
                       'last_name': last_name,
                       'username': values.get(username_key) or
                                   (first_name.title() + last_name.title()),
                       'email': email})
        return values

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate your suggestion, but these fields have been standardized for OIDC in http://openid.net/specs/openid-connect-core-1_0.html#rfc.section.5.1. I therefore would prefer to use these as a sensible default. I guess most users would override the OpenIdConnectAuth to customize this for the actual values the backend provides (if the backend doesn't adhere to the RFC).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was mostly talking about username_key = self.setting('USERNAME_KEY') or self.USERNAME_KEY

Which lets users override the username in their own settings file and exists in other backends.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added the USERNAME_KEY setting.

2 changes: 2 additions & 0 deletions social/tests/backends/oauth.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ def do_start(self):
response = requests.get(start_url)
self.assertEqual(response.url, target_url)
self.assertEqual(response.text, 'foobar')
self.strategy.set_request_data(parse_qs(urlparse(start_url).query),
self.backend)
self.strategy.set_request_data(parse_qs(urlparse(target_url).query),
self.backend)
return self.backend.complete()
Expand Down
49 changes: 39 additions & 10 deletions social/tests/backends/open_id.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
# -*- coding: utf-8 -*-
from calendar import timegm

import os
import sys
import json
import datetime

from jwkest.jwk import RSAKey, KEYS
from jwkest.jws import JWS
from jwkest.jwt import b64encode_item

import requests
import jwt

from openid import oidutil

Expand Down Expand Up @@ -127,6 +131,29 @@ class OpenIdConnectTestMixin(object):
client_key = 'a-key'
client_secret = 'a-secret-key'
issuer = None # id_token issuer
openid_config_body = None
key = None

def setUp(self):
super(OpenIdConnectTestMixin, self).setUp()
here = os.path.dirname(__file__)
self.key = RSAKey(kid='testkey').load(os.path.join(here, '../testkey.pem'))
HTTPretty.register_uri(HTTPretty.GET,
self.backend.OIDC_ENDPOINT + '/.well-known/openid-configuration',
status=200,
body=self.openid_config_body
)
oidc_config = json.loads(self.openid_config_body)

def jwks(_request, _uri, headers):
ks = KEYS()
ks.add(self.key.serialize())
return 200, headers, ks.dump_jwks()

HTTPretty.register_uri(HTTPretty.GET,
oidc_config.get('jwks_uri'),
status=200,
body=jwks)

def extra_settings(self):
settings = super(OpenIdConnectTestMixin, self).extra_settings()
Expand All @@ -143,7 +170,7 @@ def access_token_body(self, request, _url, headers):
Get the nonce from the request parameters, add it to the id_token, and
return the complete response.
"""
nonce = parse_qs(request.body).get('nonce')
nonce = self.backend.data['nonce'].encode('utf-8')
body = self.prepare_access_token_body(nonce=nonce)
return 200, headers, body

Expand All @@ -165,7 +192,7 @@ def get_id_token(self, client_key=None, expiration_datetime=None,

return id_token

def prepare_access_token_body(self, client_key=None, client_secret=None,
def prepare_access_token_body(self, client_key=None, tamper_message=False,
expiration_datetime=None,
issue_datetime=None, nonce=None,
issuer=None):
Expand All @@ -174,15 +201,12 @@ def prepare_access_token_body(self, client_key=None, client_secret=None,

client_id -- (str) OAuth ID for the client that requested
authentication.
client_secret -- (str) OAuth secret for the client that requested
authentication.
expiration_time -- (datetime) Date and time after which the response
should be considered invalid.
"""

body = {'access_token': 'foobar', 'token_type': 'bearer'}
client_key = client_key or self.client_key
client_secret = client_secret or self.client_secret
now = datetime.datetime.utcnow()
expiration_datetime = expiration_datetime or \
(now + datetime.timedelta(seconds=30))
Expand All @@ -193,8 +217,13 @@ def prepare_access_token_body(self, client_key=None, client_secret=None,
client_key, timegm(expiration_datetime.utctimetuple()),
timegm(issue_datetime.utctimetuple()), nonce, issuer)

body['id_token'] = jwt.encode(id_token, client_secret,
algorithm='HS256').decode('utf-8')
body['id_token'] = JWS(id_token, jwk=self.key, alg='RS256').sign_compact().decode('utf-8')
if tamper_message:
header, msg, sig = body['id_token'].split('.')
id_token['sub'] = '1235'
msg = b64encode_item(id_token).decode('utf-8')
body['id_token'] = '.'.join([header, msg, sig])

return json.dumps(body)

def authtoken_raised(self, expected_message, **access_token_kwargs):
Expand All @@ -204,10 +233,10 @@ def authtoken_raised(self, expected_message, **access_token_kwargs):
with self.assertRaisesRegexp(AuthTokenError, expected_message):
self.do_login()

def test_invalid_secret(self):
def test_invalid_signature(self):
self.authtoken_raised(
'Token error: Signature verification failed',
client_secret='wrong!'
tamper_message=True
)

def test_expired_signature(self):
Expand Down
Loading