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.

Sign up for GitHub

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

Jump to bottom

replace pyjwkest and cryptodome with pyjwt and cryptography #208

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

replace pyjwkest and cryptodome with pyjwt and cryptography #208

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 2 additions & 3 deletions oidc_provider/lib/utils/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
from hashlib import sha224

import django

if django.VERSION >= (1, 11):
try:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pep8 compliance change.

from django.urls import reverse
else:
except ImportError:
from django.core.urlresolvers import reverse

from django.http import HttpResponse
Expand Down
53 changes: 15 additions & 38 deletions oidc_provider/lib/utils/token.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,10 @@
import time
import uuid

from Cryptodome.PublicKey.RSA import importKey
from django.utils import dateformat, timezone
from jwkest.jwk import RSAKey as jwk_RSAKey
from jwkest.jwk import SYMKey
from jwkest.jws import JWS
from jwkest.jwt import JWT
import jwt
from cryptography.hazmat.primitives import serialization
from cryptography.hazmat.backends import default_backend

from oidc_provider.lib.utils.common import get_issuer
from oidc_provider.models import (
Expand Down Expand Up @@ -71,27 +69,25 @@ def encode_id_token(payload, client):
Represent the ID Token as a JSON Web Token (JWT).
Return a hash.
"""
keys = get_client_alg_keys(client)
_jws = JWS(payload, alg=client.jwt_alg)
return _jws.sign_compact(keys)


def decode_id_token(token, client):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function was completely unused and so I removed it.

"""
Represent the ID Token as a JSON Web Token (JWT).
Return a hash.
"""
keys = get_client_alg_keys(client)
return JWS().verify_compact(token, keys=keys)
key = client.client_secret
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the same: if the algorithm isn't RS256, then the key is the client's secret, otherwise it's the first private key in the database.
The key is passed to pyjwt so it can sign the jwt.
If the key is an rsa key, it must be passed to pyjwt in a format it understands.
This actually brings up another interesting issue: why have an RSAKey model when it could just be an rsakey that exists as a file on the server. I suspect it was just easier to start this way.

if client.jwt_alg == 'RS256':
rsakeys = RSAKey.objects.all()
if not rsakeys:
raise Exception('You must have an RSA Key.')
rsakey = rsakeys[0]
key = serialization.load_pem_private_key(rsakey.key.encode(), None, default_backend()).private_bytes(
encoding=serialization.Encoding.PEM,
format=serialization.PrivateFormat.TraditionalOpenSSL,
encryption_algorithm=serialization.NoEncryption()).decode('utf8')
return jwt.encode(payload, key, algorithm=client.jwt_alg).decode()


def client_id_from_id_token(id_token):
"""
Extracts the client id from a JSON Web Token (JWT).
Returns a string or None.
"""
payload = JWT().unpack(id_token).payload()
return payload.get('aud', None)
return jwt.decode(id_token, verify=False).get('aud', None)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically the same thing: from an id_token extract the aud. We set verify to False, because the provider is not the audience of this JWT.



def create_token(user, client, scope, id_token_dic=None):
Expand Down Expand Up @@ -138,22 +134,3 @@ def create_code(user, client, scope, nonce, is_authentication,
code.is_authentication = is_authentication

return code


def get_client_alg_keys(client):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is no longer necessary as the way to get the key has been simplified.

Copy link

Choose a reason for hiding this comment

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

By removing this, and switching to the new 'simplified' method, explicit checking for specific algorithms is lost, which opens the door to future issues. I would much prefer to see explicit checking added back - even if it adds some code complexity, the greater clarity in code paths and explicit exception for other algorithms is worth the cost.

"""
Takes a client and returns the set of keys associated with it.
Returns a list of keys.
"""
if client.jwt_alg == 'RS256':
keys = []
for rsakey in RSAKey.objects.all():
keys.append(jwk_RSAKey(key=importKey(rsakey.key), kid=rsakey.kid))
if not keys:
raise Exception('You must add at least one RSA Key.')
elif client.jwt_alg == 'HS256':
keys = [SYMKey(key=client.client_secret, alg=client.jwt_alg)]
else:
raise Exception('Unsupported key algorithm.')

return keys
12 changes: 9 additions & 3 deletions oidc_provider/management/commands/creatersakey.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
from Cryptodome.PublicKey import RSA
from cryptography.hazmat.primitives.asymmetric.rsa import generate_private_key
from cryptography.hazmat.primitives import serialization
from cryptography.hazmat.backends import default_backend
from django.core.management.base import BaseCommand

from oidc_provider.models import RSAKey
Expand All @@ -9,8 +11,12 @@ class Command(BaseCommand):

def handle(self, *args, **options):
try:
key = RSA.generate(1024)
rsakey = RSAKey(key=key.exportKey('PEM').decode('utf8'))
key = generate_private_key(public_exponent=65537, key_size=2048, backend=default_backend())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The public exponent is the same as the default in pycryptodome. I've upped the default bitsize of the key to 2048 instead of 1024. The key is generated, then serialized into the database in the PEM format.
See https://en.wikipedia.org/wiki/Key_size#Asymmetric_algorithm_key_lengths.

It's a little weird that we always have to specify the default_backend(), but that's for old reasons:
https://cryptography.io/en/latest/hazmat/backends/
Cryptography just uses OpenSSL as its backend.

Copy link

Choose a reason for hiding this comment

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

I like seeing the move to 2048 bit RSA - 1024 is too weak for use today. Anything less than 2048 should be avoided if at all possible (see for example the move away from 1024 by Web PKI).

rsakey = RSAKey(
key=key.private_bytes(
encoding=serialization.Encoding.PEM,
format=serialization.PrivateFormat.TraditionalOpenSSL,
encryption_algorithm=serialization.NoEncryption()).decode('utf8'))
Copy link

Choose a reason for hiding this comment

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

Ideally, there would be support for encrypted private keys, to reduce the risk of exposing the private key - this would add a defense in depth layer, though would add somewhat to configuration complexity.

This would be a nice improvement over the current state.

rsakey.save()
self.stdout.write(u'RSA key successfully created with kid: {0}'.format(rsakey.kid))
except Exception as e:
Expand Down
30 changes: 22 additions & 8 deletions oidc_provider/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,17 @@
except ImportError:
from urllib.parse import urlsplit, parse_qs, urlunsplit, urlencode

from Cryptodome.PublicKey import RSA
from base64 import urlsafe_b64encode
from django.contrib.auth.views import (
redirect_to_login,
logout,
)

import django
if django.VERSION >= (1, 11):
from struct import pack

try:
from django.urls import reverse
else:
except ImportError:
from django.core.urlresolvers import reverse

from django.contrib.auth import logout as django_user_logout
Expand All @@ -26,7 +27,9 @@
from django.views.decorators.clickjacking import xframe_options_exempt
from django.views.decorators.http import require_http_methods
from django.views.generic import View
from jwkest import long_to_base64

from cryptography.hazmat.primitives.serialization import load_pem_private_key
from cryptography.hazmat.backends import default_backend

from oidc_provider.lib.claims import StandardScopeClaims
from oidc_provider.lib.endpoints.authorize import AuthorizeEndpoint
Expand Down Expand Up @@ -284,18 +287,29 @@ def get(self, request, *args, **kwargs):


class JwksView(View):
def _num_to_b64_string(self, value):
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 think pyjwt actually provides an interface for generating a JWK from a key, so I'll change that in a future PR. For now, this is just the way JWK specifies how to give your public key.

int_array = []
while value:
value, r = divmod(value, 256)
int_array.insert(0, r)
char_array = pack('B'*len(int_array), *int_array)
return urlsafe_b64encode(char_array).rstrip(b'=').decode("ascii")

def get(self, request, *args, **kwargs):
dic = dict(keys=[])

for rsakey in RSAKey.objects.all():
public_key = RSA.importKey(rsakey.key).publickey()
public_numbers = load_pem_private_key(
rsakey.key.encode(),
None,
default_backend()).public_key().public_numbers()
dic['keys'].append({
'kty': 'RSA',
'alg': 'RS256',
'use': 'sig',
'kid': rsakey.kid,
'n': long_to_base64(public_key.n),
'e': long_to_base64(public_key.e),
'n': self._num_to_b64_string(public_numbers.n),
'e': self._num_to_b64_string(public_numbers.e),
})

response = JsonResponse(dic)
Expand Down
5 changes: 4 additions & 1 deletion setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,13 @@
test_suite='runtests.runtests',
tests_require=[
'pyjwkest>=1.3.0',
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 kept pyjwkest in the tests to show that this would all be compatible. Just want to show I'm not adding or removing functionality, just refactoring some libraries out.

'cryptography>==2.0',
'pyjwt>==1.5.0',
'mock>=2.0.0',
],

install_requires=[
'pyjwkest>=1.3.0',
'cryptography>==2.0',
'pyjwt>==1.5.0',
],
)
2 changes: 1 addition & 1 deletion tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,4 @@ commands=
basepython=python
deps=flake8
commands =
flake8 --max-line-length=120
flake8 --max-line-length=120 --exclude=.svn,CVS,.bzr,.hg,.git,__pycache__,.tox,oidc_provider/migrations