lua-resty-openidc icon indicating copy to clipboard operation
lua-resty-openidc copied to clipboard

Add JWE support

Open chrisFrodo opened this issue 2 years ago • 3 comments

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

chrisFrodo avatar Jun 16 '22 08:06 chrisFrodo

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

bodewig avatar Jun 26 '22 13:06 bodewig

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!

chrisFrodo avatar Nov 24 '22 14:11 chrisFrodo

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.

bodewig avatar Dec 27 '22 11:12 bodewig