replace pyjwkest and cryptodome with pyjwt and cryptography #208

psavoie · 2017-09-06T00:19:27Z

See issue #207

wojtek-fliposports · 2017-09-06T03:39:59Z

I must dive into this cryptography library because I don't understand your changes at all :)

psavoie · 2017-09-06T17:59:53Z

I'll leave some inline comments explaining things. It's functionally the same as before (that's why there are no new or changed tests). I can answer any additional questions.

psavoie

Just adding inline comments explaining my changes.

psavoie · 2017-09-06T18:04:46Z

oidc_provider/lib/utils/common.py


-if django.VERSION >= (1, 11):
+try:


Pep8 compliance change.

psavoie · 2017-09-06T18:05:31Z

oidc_provider/lib/utils/token.py

-    return _jws.sign_compact(keys)
-
-
-def decode_id_token(token, client):


This function was completely unused and so I removed it.

psavoie · 2017-09-06T18:11:23Z

oidc_provider/lib/utils/token.py

-    """
-    keys = get_client_alg_keys(client)
-    return JWS().verify_compact(token, keys=keys)
+    key = client.client_secret


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.

psavoie · 2017-09-06T18:12:43Z

oidc_provider/lib/utils/token.py



 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)


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.

psavoie · 2017-09-06T18:13:56Z

oidc_provider/lib/utils/token.py

@@ -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):


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

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.

psavoie · 2017-09-06T18:20:15Z

oidc_provider/management/commands/creatersakey.py

@@ -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())


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.

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).

psavoie · 2017-09-06T18:21:59Z

oidc_provider/views.py

@@ -284,18 +287,29 @@ def get(self, request, *args, **kwargs):


 class JwksView(View):
+    def _num_to_b64_string(self, value):


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.

psavoie · 2017-09-06T18:25:18Z

setup.py

@@ -38,10 +38,13 @@
    test_suite='runtests.runtests',
    tests_require=[
        'pyjwkest>=1.3.0',


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.

wojtek-fliposports · 2017-09-27T08:24:37Z

Thanks for this contribution. Really great job but I don't understand RSA generation.. sorry..

From RTD:

This is a “Hazardous Materials” module. You should ONLY use it if you’re 100% absolutely sure that you know what you’re doing because this module is full of land mines, dragons, and dinosaurs with laser guns.

ghost · 2017-10-01T14:27:09Z

oidc_provider/management/commands/creatersakey.py

+                key=key.private_bytes(
+                    encoding=serialization.Encoding.PEM,
+                    format=serialization.PrivateFormat.TraditionalOpenSSL,
+                    encryption_algorithm=serialization.NoEncryption()).decode('utf8'))


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.

ghost · 2017-10-01T16:08:59Z

@wojtek-fliposports RSA can be extremely tricky for those that aren't familiar with all the things that can go wrong. In this case, the hazmat module is used to provide two pieces of functionality:

RSA Key Generation
RSA Encoding / Decoding

These don't have the risk level associated with the direct cryptographic operations that can easily introduce vulnerabilities if mis-used.

For RSA Key Generation, it is using OpenSSL to generate a 2048-bit key (which is the minimum that should be used today), with e == 65537 (2^16+1), which is the standard value used (recommended by NIST and is mandated for Web PKI). These are the values that you want to see, and OpenSSL is well reviewed and trusted for RSA key generation.

For RSA Key Serialization, this simply encodes the key to the standard PEM format, as the code does today. This is standard and safe functionality.

For this functionality, IMO, the hazmat warning can be ignored. This is safe functionality, and is equivalent to, or an improvement on, the current functionality.

wojtek-fliposports · 2017-11-07T22:55:33Z

Unfortunatelly a lot of 3rd party libraries using OIDC auth flow requires RS256 key type and they are unable to perform or handle RS1024

franks42 · 2017-11-08T15:33:21Z

Hi Wojciech - could you elaborate on this RS256 vs RS1024? (or maybe you made a type with "rs1024" ;-) ) - thanks, Frank.

wojtek-fliposports · 2017-11-09T10:59:38Z

... blah.. delete my comment :/
But I can't go with your change. I will use some hints but I can't use it.. sorry. I'm to stupid

bpereto · 2018-10-14T09:33:09Z

What is the state of this? I would like to see pyca/cryptography beeing used.

cbouvier15 · 2019-01-08T12:43:50Z

Hi @juanifioren, are you planning to merge this soon?

replace pyjwkest and cryptodome with pyjwt and cryptography

991401e

psavoie commented Sep 6, 2017

View reviewed changes

ghost reviewed Oct 1, 2017

View reviewed changes

bpereto mentioned this pull request Oct 14, 2018

[Bandit] security report #293

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace pyjwkest and cryptodome with pyjwt and cryptography #208

replace pyjwkest and cryptodome with pyjwt and cryptography #208

psavoie commented Sep 6, 2017

wojtek-fliposports commented Sep 6, 2017

psavoie commented Sep 6, 2017

psavoie left a comment

psavoie Sep 6, 2017

psavoie Sep 6, 2017

psavoie Sep 6, 2017

psavoie Sep 6, 2017

psavoie Sep 6, 2017

ghost Oct 1, 2017

psavoie Sep 6, 2017

ghost Oct 1, 2017

psavoie Sep 6, 2017

psavoie Sep 6, 2017

wojtek-fliposports commented Sep 27, 2017 •

edited

ghost Oct 1, 2017

ghost commented Oct 1, 2017

wojtek-fliposports commented Nov 7, 2017

franks42 commented Nov 8, 2017 via email

wojtek-fliposports commented Nov 9, 2017 •

edited

bpereto commented Oct 14, 2018

cbouvier15 commented Jan 8, 2019

		return _jws.sign_compact(keys)


		def decode_id_token(token, client):

		@@ -284,18 +287,29 @@ def get(self, request, args, *kwargs):


		class JwksView(View):
		def _num_to_b64_string(self, value):

replace pyjwkest and cryptodome with pyjwt and cryptography #208

Are you sure you want to change the base?

replace pyjwkest and cryptodome with pyjwt and cryptography #208

Conversation

psavoie commented Sep 6, 2017

wojtek-fliposports commented Sep 6, 2017

psavoie commented Sep 6, 2017

psavoie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtek-fliposports commented Sep 27, 2017 • edited

Choose a reason for hiding this comment

ghost commented Oct 1, 2017

wojtek-fliposports commented Nov 7, 2017

franks42 commented Nov 8, 2017 via email

wojtek-fliposports commented Nov 9, 2017 • edited

bpereto commented Oct 14, 2018

cbouvier15 commented Jan 8, 2019

wojtek-fliposports commented Sep 27, 2017 •

edited

wojtek-fliposports commented Nov 9, 2017 •

edited