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

Add JWE support #440

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

chrisFrodo
Copy link

Add JWE support that is missing currently, and fix a bug with userinfo

@bodewig
Copy link
Collaborator

bodewig commented Jun 26, 2022

Thanks @chrisFrodo - as you've probably realized this is not the most active project right now. I'm sorry for that.

I've got a few comments and questions, though.

  • string_starts is never used
  • The way you've added support for userinfo endpoints returning JWTs means openidc_parse_json_response now accepts JWTs for much more than just the userinfo endpoint. This probably should better be conditional.
  • openidc_load_jwt_and_verify_crypto already contains code to split the JWT to handle the case where the none alg has been used (only two parts of a JWT). I believe it would be cleaner and easier if we merged JWE support with the code just above - and likely we could then stick to match and destructuring.
  • please also update the README with an example as a minimal way of documenting how to configure things when JWEs are used
  • why would we want to restrict this to RSA - and only with a very specific key length and only one of the padding alternatives? Wouldn't the same work for other key sizes or even when using AES as well?
  • I would love to have tests for this. I know the way testing works is a bit convoluted and it likely takes some effort to make the test user info endpoint return a JWT but most stuff should be there already
  • please add yourself to the AUTHORS file

@chrisFrodo
Copy link
Author

Hi @bodewig !
I'm sorry for answering this late your questions about this PR, I might have been quite busy on other projects.

Following your suggestion I've made some modification on my previous work :

  • Unused function have been removed
  • Added a minimal example of the JWE related config options in the README.md file
  • Moved the JWS parsing of the userinfo to the openidc.call_userinfo_endpoint()function
  • Rework the openidc_load_jwt_and_verify_crypto() function to use the string.match() function.
    I had to modify the logic of booth the JWS and JWT handling so each case can be handled using the same extracted token parts.
    I may have added too much comments though...
  • Included new unit tests for this PR.

About your question about the cryptographic limitations, the fun begins !
These limitations are mainly linked to the lua-resty-jwt library and its own cryptographic constrains on decryption.
The current version of the library (v0.2.3) only support the following settings for JWE tokens :

  • Key management algorithm (ie : "alg") :
    • RSA-OAEP-256
    • DIR (RFC non-compliant value) :
      This value is used to tell the parse_jwe() function is used to directly a pre-shared secret to derivate the decryption key
  • Content encryption algorithm (ie : "enc") :
    • A256GCM
    • A128CBC_HS256
    • A256CBC_HS256

So, as described in the OpenID Connect CORE spec, the jwt lib is not complient on the encryption/decryption aspect. It is in fact lacking the support of the RSA1_5 algorithm and should allow non signed version of the CBC mode encryption algorithms.

Here are a list of the algorithm that should be supported, the bold ones being mandatory :

alg enc
RSA1_5 A128CBC
RSA-OAEP A256CBC
EDCH-ES A128GCM
A128KW A256GCM
A256KW
A128GCM
A256GCM

Fun fact : the HMAC signature of content encrypted with CBC mode is ment be optional

That means some modifications will have to be done on the lua-resty-jwt library in order to meet the minimun requirement of the OpenID Connect spec. I'll try to work on it this month.

Hope these answers helped you!

-- part1 = JOSE Header, part2 = Initialization Vector, part3 = Cyphertext, part4 = Authentication Tag , others are unused
-- or
-- part1 = JOSE Header, part2 = Pre-shared key, part3 = Initialization Vector, part4 = Cyphertext, part5 = Authentication Tag
local jwe_header = cjson.decode(unb64(part1))
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't that be urlsafe base64 decoding, i.e. openidc_base64_url_decode? Also it may be good to use cjson_s and check for an error.

@bodewig
Copy link
Collaborator

bodewig commented Dec 27, 2022

Thanks a lot @chrisFrodo , this looks great

apart from the small comment I left inline I wonder whether we need to do something about elliptic curves as well - see also #457 .

The EC MR makes me wonder whether it is a good idea to encode lua-resty-jwt's current limitations in lua-resty-openidc as it will always require a new release here if lua-resty-jwt learns new tricks that may be relevant to users of the library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants