Use PyJWT instead of python-jose

[Natim]

Alternate to #48

Context

This package indirectly uses python-jose, which is affected by: GHSA-cjwg-qfpm-7377 which additionally seems to be abandoned by it's maintainers.

Move this package to use PyJWT to generate the JWK instead.

[Natim]

[yahel2410]

LGTM

[Natim]

@yahel2410 what's next? Can we merge?

[robert-mings]

Hi @Natim,

Thanks for this contribution! The move away from python-jose is a much needed change.

From my testing, I had a few questions/asks:

• It looks like the cryptography backend was being supplied as a python-jose dependency. I needed to install the cryptography package directly. Was this the same in your testing? If so, we'll need to add this as a dependency.
• Line 169 - I believe the verify method will need to be changed as it was previously supplied by python-jose.
• Do you mind adding versioning for PyJWT?

[Natim]

Hello Robert, no need for the cryptography package anymore with PyJWT.

As for the verify method, PyJWT provides the same method/API. The code is tested and we have been using it in production for more than a month.

In the Python dependencies philosophy, libraries should not enforce dependencies versioning so that project can handle it without library version conflicts.

This is because you can't use two different version of a lib in the same project with Python.

We could use the >= but since it works with all versions of PyJWT it isn't necessary.

[Natim]

Do you mind adding versioning for PyJWT?

Done, thanks

[robert-mings]

Thanks @Natim. Great to hear you've been using this in production.

• RE: cryptography - Based on the PyJWT docs, the RSA algorithm isn't supported by default until you install the cryptography package. Is there any chance this was being supplied as a dependency from another package in your build? In an isolated environment, I'm receiving exceptions until this is explicitly installed.
• RE: verify method - Can you share more information on this? I'm not seeing anywhere in the docs where it talks about a verify method/API.
• RE: package versions - Lower bounds look good, thanks for adding this.

[Natim]

cryptography - Based on the PyJWT docs, the RSA algorithm isn't supported by default until you install the cryptography package. Is there any chance this was being supplied as a dependency from another package in your build? In an isolated environment, I'm receiving exceptions until this is explicitly installed.

Ok then, let's add it 🙏

[Natim]

verify method - Can you shared more information on this? I'm not seeing anywhere in the docs where it talks about a verify method/API.

It's part of the PyJWK Algorithm API: https://github.com/jpadilla/pyjwt/blob/527fec277e8215a197f8facd3778b359043704ef/jwt/algorithms.py#L180-L185

[yahel2410]

Can you please proceed and merge this?

[keaton185]

@robert-mings are we any closer to being able to merge this one?

[robert-mings]

@Natim, @keaton185, @yahel2410 - We're close but I have yet to get this working as it's currently written. Facing AttributeError: 'PyJWK' object has no attribute 'verify'. I don't believe it's a namespace issue either as jwt is uninstalled.

Can I confirm if others are using this successfully as is?

[Natim]

Can you write a test for it I can look at? It supposed to be an Algorithm object not a JWK object that you use to verify.

[Natim]

@robert-mings can you try again, you were right. I believe we are missing a test for this part of the lib, I was expecting it to exist which I was wrong. The good news is that our production code doesn't seem to use this part of the code.

[robert-mings]

Thanks @Natim for adding this. Just an update from my side - this solved the AttributeError, however, this introduced a a number of additional errors.

Looks like we needed two additional arguments - padding and algorithm. Easy enough, passing those solves that error:
is_signature_valid = public_key.verify(message.encode('utf-8'), id_token_signature, algorithm='RS256', padding='PKCS1v15')

That then runs into another error: TypeError: Expected instance of hashes.HashAlgorithm

I'm almost thinking we can simplify and use the decode method since this already incorporates logic for validating the signature. This would also allow a bit of cleanup. Maybe something like:

   try:
        public_key = get_jwk(id_token_header['kid'], jwk_uri).key
        jwt.decode(id_token, public_key, audience=client_id, algorithms=['RS256'])
        return True
   except:
        return False

Thoughts?

[Natim]

@robert-mings can you try again like that?

[robert-mings]

Awesome @Natim - this looks good. Only required change is to import jwt since we are using the decode api directly (as well as the 2 required references).

I added a couple lines of cleanup as well. Let me know if you're able to make these changes, otherwise I can get this merged and follow-up directly after.

[robert-mings]

Looks great. Thanks again for the contribution @Natim.

Merging now and will get a new version released.